Skip to content

Address default parameter regressions and add tests for ForInStmtSyntax #1517

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Apr 13, 2023
Merged

Address default parameter regressions and add tests for ForInStmtSyntax #1517

merged 7 commits into from
Apr 13, 2023

Conversation

stevapple
Copy link
Contributor

Fixes #1515

@stevapple stevapple requested a review from ahoppen as a code owner April 12, 2023 06:02
@ahoppen
Copy link
Member

ahoppen commented Apr 12, 2023

The problem here is that for some reason we are generating the .keyword(.await) default parameter for the initializer even though the child is marked as optional. I’m not sure why that is but I think this would be the thing that needs fixing. The child declaration as kind: .token(choices: [.keyword(text: "await")]), is correct.

Could you look into what’s causing us to generate .keyword(.await) instead of nil, which is what I would expect?

@stevapple
Copy link
Contributor Author

It turns out to be a pretty large fix… We used to have two different implementations of defaultInitialization, and both are not really correct. One of them caused the awaitKeyword regression in SwiftSyntaxBuilder, while the other synthesized wrong defaults for a series of identifier and operator tokens in SwiftSyntax.

ahoppen
ahoppen approved these changes Apr 12, 2023
View reviewed changes
Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Thank you 🙏🏽 This is so much better. And it fixes the case where we incorrectly generate IdentifierToken default values 😍

@ahoppen
Copy link
Member

ahoppen commented Apr 12, 2023

@swift-ci Please test

@ahoppen
Copy link
Member

ahoppen commented Apr 12, 2023

@swift-ci Please test Windows

@ahoppen ahoppen enabled auto-merge April 12, 2023 23:24
@kimdv
Copy link
Contributor

kimdv commented Apr 13, 2023

Looks like there is some formatting issues.
Can you format it again? 😄

auto-merge was automatically disabled April 13, 2023 06:25

Head branch was pushed to by a user without write access

@stevapple
Copy link
Contributor Author

Can you format it again? 😄

Done:)

@kimdv
Copy link
Contributor

kimdv commented Apr 13, 2023

@swift-ci please test

1 similar comment
@kimdv
Copy link
Contributor

kimdv commented Apr 13, 2023

@swift-ci please test

@stevapple stevapple changed the title Address ForInStmtSyntax regressions and add tests Address default parameter regressions and add tests for ForInStmtSyntax Apr 13, 2023
@kimdv
Copy link
Contributor

kimdv commented Apr 13, 2023

@swift-ci please test windows

@ahoppen ahoppen merged commit 82b5683 into swiftlang:main Apr 13, 2023
@stevapple stevapple deleted the fix-for-in branch April 13, 2023 18:05
ahoppen added a commit to ahoppen/swift-syntax that referenced this pull request Apr 18, 2023
@ahoppen
Merge pull request swiftlang#1517 from stevapple/fix-for-in
ae0b2e6 
Address default parameter regressions and add tests for `ForInStmtSyntax`
ahoppen added a commit to ahoppen/swift-syntax that referenced this pull request Apr 18, 2023
@ahoppen
Merge pull request swiftlang#1517 from stevapple/fix-for-in
a663fbb 
Address default parameter regressions and add tests for `ForInStmtSyntax`
@ahoppen ahoppen mentioned this pull request Apr 18, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahoppen ahoppen ahoppen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Various ForInStmtSyntax builder regressions
3 participants
@stevapple @ahoppen @kimdv