-
Notifications
You must be signed in to change notification settings - Fork 684
Fix trailing comma incorrectly added to generic argument lists within expressions in Swift 6.2 #2471
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
Closed
+98
−1
Closed
Fix trailing comma incorrectly added to generic argument lists within expressions in Swift 6.2 #2471
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
b06d761
Initial plan
Copilot 837da90
Initial plan for fixing trailing comma regression
Copilot 660c05b
Fix trailing comma regression in generic argument lists within expres…
Copilot 7aa0009
Fix trailing comma regression in generic argument lists within expres…
Copilot 3a8269e
Fix trailing comma incorrectly added to generic argument lists within…
Copilot File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot, this is a new regression in the past few weeks. Can you find what specific change caused the regression?
This code is pretty complicated. Maybe it would be better to revert the change that caused the regression, or scope that change to only Swift 6.3+.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regression is in the
>endOfScope handler, which unconditionally setstrailingCommaSupported = truefor allswiftVersion >= "6.2"contexts since commit02e45820. However, trailing commas in generic argument lists within expressions (likeArray<T>()) require Swift 6.3+. Swift 6.2 only supports them in type contexts and declarations.The regression surfaces when
>ends up on its own line (e.g., afterwrapArgumentsformats the code), at which pointaddOrRemoveTrailingCommainserts a comma before>.Rather than reverting
02e45820(which would lose all the correct Swift 6.2 improvements), I've scoped the expression-context exclusion toswiftVersion < "6.3": when>is followed by(, we check if it's a declaration (func foo<T>(),init<T>(),init?<T>(),subscript<T>()). If not, it's expression context and no trailing comma is added. Updated in commit 7aa0009.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot, I'm very certain it is not commit
02e45820that introduced the regression. That commit is from September. The regression was introduced within the past few weeks.