Skip to content

Add trailing comma support in cases missing from Swift 6.1 #81612

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

calda
Copy link
Contributor

@calda calda commented May 19, 2025

Corresponding swift-syntax PR: swiftlang/swift-syntax#3080


In Swift 6.1, there are many cases where trailing commas should be supported but currently are not:

This includes cases like:

let foo: (
  bar: String,
  quux: String, // unexpectedly not supported in Swift 6.1
)

let closure: (
  String,
  String, // unexpectedly not supported in Swift 6.1
) -> (
  bar: String,
  quux: String, // unexpectedly not supported in Swift 6.1
)

let foo: Foo<
  String,
  Int,
  Boo, // unexpectedly not supported in Swift 6.1
>

let foo = Foo<
  String,
  Int,
  Boo, // unexpectedly not supported in Swift 6.1
>()

typealias Bar<
  T1,
  T2
> = Foo<
  T1,
  T2,
  Bool, // unexpectedly not supported in Swift 6.1
>

protocol Baaz<
  T1,
  T2, // unexpectedly not supported in Swift 6.1
> {
  associatedtype T1
  associatedtype T2
}

@calda calda force-pushed the cal--trailing-comma-missing-from-6.1 branch from ea63030 to 1a3d71c Compare May 19, 2025 16:19
@rintaro
Copy link
Member

rintaro commented May 19, 2025

Thank you Cal for looking into this!

I think builtin attributes (and availability spec list?) were deliberately NOT implemented in the original PR ( #74522 (comment) .) @xwu What was the verdict for this? Should the compiler accept them?

@calda
Copy link
Contributor Author

calda commented May 19, 2025

Thanks for the link, @xwu's here discussion make sense:

If the proposal had been accepted with the original scope that everything that's a comma-separated list with an unambiguous terminator should support a trailing comma, then we would need to puzzle over which of these attributes have such a regular list, but we don't have to do that now :)

As I was working through the changes it was indeed unclear which attributes should support trailing commas vs which attributes shouldn't. Excluding all built-in attributes appears to be an intentional decision as a part of the acceptance. I removed that part from the PR.

The other cases are more clearly just bugs that should be fixed.

@calda calda force-pushed the cal--trailing-comma-missing-from-6.1 branch from 6492aaf to 9072d86 Compare May 19, 2025 23:00
Copy link
Member

@rintaro rintaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code changes look good to me. Thank you!

T1,
T2,
Bool,
>
Copy link
Member

@rintaro rintaro May 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind adding test cases for generic arguments in expression positions to prevent future regressions? Like let _ = Generic<Int, Bool,>.self and let _ = Generic<Int, Bool,>(). It also uses parseGenericArguments() (currently) so it should work with your changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for remembering to check that! It didn't work originally, we also had to update the lookahead code that disambiguated the < between a generic list and a less than operator. Updated, working now :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah canParseGenericArguments(). That makes sense. Thank you for fixing it!

@calda calda force-pushed the cal--trailing-comma-missing-from-6.1 branch from ac24f18 to 9a3f33d Compare May 20, 2025 01:57
@rintaro
Copy link
Member

rintaro commented May 20, 2025

@swift-ci Please smoke test

@mateusrodriguesxyz
Copy link
Contributor

@calda Thanks for fixing these cases that I missed!

@calda
Copy link
Contributor Author

calda commented May 20, 2025

The CI job failed with:

error: unexpected error produced: new Swift parser generated errors for code that C++ parser accepted

@rintaro, can you trigger CI again with the Swift Syntax changes from swiftlang/swift-syntax#3080 included? Thanks!

@rintaro
Copy link
Member

rintaro commented May 20, 2025

swiftlang/swift-syntax#3080
@swift-ci Please smoke test

@rintaro rintaro closed this May 20, 2025
@rintaro rintaro reopened this May 20, 2025
@rintaro
Copy link
Member

rintaro commented May 20, 2025

(closed/reopened by mistake)

@rintaro
Copy link
Member

rintaro commented May 20, 2025

swiftlang/swift-syntax#3080
@swift-ci Please smoke test macOS

@rintaro
Copy link
Member

rintaro commented May 20, 2025

swiftlang/swift-syntax#3080
@swift-ci Please smoke test Linux

@rintaro
Copy link
Member

rintaro commented May 20, 2025

@calda Everything LGTM, and I will merge this and the swift-syntax PR as soon as the CI passes.
Could you open cherry-pick PRs for release/6.2?

@rintaro
Copy link
Member

rintaro commented May 20, 2025

swiftlang/swift-syntax#3080
@swift-ci Please smoke test macOS

@rintaro
Copy link
Member

rintaro commented May 20, 2025

swiftlang/swift-syntax#3080
@swift-ci Please smoke test Linux

@rintaro
Copy link
Member

rintaro commented May 21, 2025

swiftlang/swift-syntax#3080
@swift-ci Please smoke test macOS

1 similar comment
@rintaro
Copy link
Member

rintaro commented May 21, 2025

swiftlang/swift-syntax#3080
@swift-ci Please smoke test macOS

@rintaro
Copy link
Member

rintaro commented May 21, 2025

@swift-ci Please smoke test macOS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants