Skip to content
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

Trailing comma implementation #2515

Merged
merged 1 commit into from
Apr 22, 2024

Conversation

mateusrodriguesxyz
Copy link
Contributor

This is a prototype implementation for Allow trailing comma in tuples, arguments and if/guard/while conditions gated behind -enable-experimental-feature TrailingComma.

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.

Some initial thoughts. My biggest comment is that we should implement the disambiguation between an additional condition element and the body using lookahead instead of by modifying the tree.

Sources/SwiftParser/Declarations.swift Outdated Show resolved Hide resolved
Sources/SwiftParser/Expressions.swift Outdated Show resolved Hide resolved
Sources/SwiftParser/Expressions.swift Outdated Show resolved Hide resolved
Sources/SwiftParser/Expressions.swift Outdated Show resolved Hide resolved
Sources/SwiftParser/Statements.swift Outdated Show resolved Hide resolved
Sources/SwiftParser/Statements.swift Outdated Show resolved Hide resolved
Tests/SwiftParserTest/TrailingCommaTests.swift Outdated Show resolved Hide resolved
@mateusrodriguesxyz
Copy link
Contributor Author

@ahoppen I've updated the implementation for conditionals to use a much simpler lookahead approach

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. The approach is the right one, I think. Getting the lookahead logic right is the key building block here, I think and we are on a very good track. I needed to bang my head really hard to come up with counterexamples, those are very contrived and maybe we can ignore them.

Sources/SwiftParser/Declarations.swift Outdated Show resolved Hide resolved
Sources/SwiftParser/Attributes.swift Show resolved Hide resolved
Sources/SwiftParser/Expressions.swift Outdated Show resolved Hide resolved
Sources/SwiftParser/Expressions.swift Outdated Show resolved Hide resolved
Sources/SwiftParser/Expressions.swift Outdated Show resolved Hide resolved
Sources/SwiftParser/Statements.swift Outdated Show resolved Hide resolved
Sources/SwiftParser/Statements.swift Outdated Show resolved Hide resolved
Sources/SwiftParser/Statements.swift Outdated Show resolved Hide resolved
Tests/SwiftParserTest/TrailingCommaTests.swift Outdated Show resolved Hide resolved
Tests/SwiftParserTest/TrailingCommaTests.swift Outdated Show resolved Hide resolved
@mateusrodriguesxyz
Copy link
Contributor Author

@ahoppen I've fixed minor problems with parseArgumentListElements and updated parseConditionList and atStartOfStatementBody implementations to address our conversation. I've also updated the tests and included some substructure assertions to ensure that the expected block is parsed as the statement body.

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.

I think we’re on the final stretch for this PR. Thanks for your continued work on this.

Sources/SwiftParser/Expressions.swift Outdated Show resolved Hide resolved
Sources/SwiftParser/Expressions.swift Outdated Show resolved Hide resolved
Sources/SwiftParser/Expressions.swift Outdated Show resolved Hide resolved
Sources/SwiftParser/Statements.swift Outdated Show resolved Hide resolved
Sources/SwiftParser/Statements.swift Outdated Show resolved Hide resolved
Sources/SwiftParser/Statements.swift Outdated Show resolved Hide resolved
Sources/SwiftParser/Statements.swift Outdated Show resolved Hide resolved
Tests/SwiftParserTest/TrailingCommaTests.swift Outdated Show resolved Hide resolved
Tests/SwiftParserTest/TrailingCommaTests.swift Outdated Show resolved Hide resolved
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.

Nice. Looking good to me 👍🏽

I would still like to hear a second opinion from @rintaro on the implementation.

Tests/SwiftParserTest/TrailingCommaTests.swift Outdated Show resolved Hide resolved
@rintaro
Copy link
Member

rintaro commented Mar 19, 2024

Could you let me know the plan for other comma separated lists like func parameter list, tuple type, generic paremeter/argument list etc?
I believe your proposal includes parameter list support at least, right?

@ahoppen
Copy link
Member

ahoppen commented Mar 19, 2024

I just suggested to keep the PR as-is for now and implement trailing closures for other constructs in a follow-up PR (#2515 (comment)). I’ve already spent considerable time reviewing this PR and it’s easier review-wise if we can consider the implications of trailing commas for other constructs in a separate PR.

@mateusrodriguesxyz
Copy link
Contributor Author

I believe your proposal includes parameter list support at least, right?

That's right. The current proposal and implementation includes tuples, functions parameter/argument list and if, guard and while conditions. I'm keeping minimal and included the ones that I consider the most demanded use cases.

@mateusrodriguesxyz
Copy link
Contributor Author

BTW @ahoppen thank you so much for all the help and patience while reviewing this PR and sorry for all the troubles. That's my first time trying to contribute to the language and whatever the outcome I'm happy with all that I've learned.

Sources/SwiftParser/Statements.swift Outdated Show resolved Hide resolved
Comment on lines 1081 to 1088
// If the current token is at the start of a line, it is most likely a statement body. The only exceptions are:
if withLookahead({ $0.atStartOfStatementBody() }) {
// If a statement body starts after this on, we actually have a closure followed by the statement body e.g.
// if true, { true }
// {
// print("body")
// }
return false
Copy link
Member

@rintaro rintaro Mar 19, 2024

Choose a reason for hiding this comment

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

Sorry if I missed the previous discussion, but do we really care this case? Closure alone cannot be a condition. it must be used/called some way (e.g. call it, or make it a part of a sequence expression). This example case is ambiguous, it can be missing () after { true } or missing () after { print("body") }. I don't think it's worth to lookahead again unless there's any strong reason.

Copy link
Contributor Author

@mateusrodriguesxyz mateusrodriguesxyz Mar 19, 2024

Choose a reason for hiding this comment

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

I agree that's a odd case but today that's a valid condition and the parser reports a Function produces expected type 'Bool'; did you mean to call it with '()'? error so without this lookahead there would be a regression.

Copy link
Member

Choose a reason for hiding this comment

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

That's not a valid condition today, it's just parsed as a condition expression and diagnosed in type checker. Since it's invalid anyway, I wouldn't consider it's a regression even if we don't parse it as a closure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't feel comfortable changing existing behavior and as a user I consider the type checker diagnostic more helpful because is probably more close to the user intention.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can add at(.leftBrace) before the withLookahead to avoid actually looking ahead if not necessary.

Copy link
Member

@rintaro rintaro Mar 19, 2024

Choose a reason for hiding this comment

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

The current behavior makes sense before the trailing comma support. There was no ambiguity. But supporting trailing comma introduces some ambiguity, for example, I believe the user intention is that { return true } is the body in the following case:

func test() -> Bool {
  if
    condition1,
    condition2,
  {
    return true
  }
  
  { print("something") }
}

Copy link
Contributor Author

@mateusrodriguesxyz mateusrodriguesxyz Apr 4, 2024

Choose a reason for hiding this comment

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

Sorry, I took I time from this PR and missed your last answer. That's a really good counterexample. Honestly, I'm good with either behavior, whichever has a consensus so this can move forward. Could you weight in @ahoppen?

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @rintaro.

Copy link
Contributor Author

@mateusrodriguesxyz mateusrodriguesxyz Apr 4, 2024

Choose a reason for hiding this comment

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

I will change then and also resolve the conflicts with main branch.

@rintaro
Copy link
Member

rintaro commented Mar 19, 2024

The current proposal and implementation includes tuples, functions parameter/argument list and if, guard and while conditions.

I don't see implementation/tests for functions parameter list. e.g. from your proposal

func foo(
    a: Int = 0, 
    b: Int = 0, 
) {
}

Am I missing anything?

@rintaro
Copy link
Member

rintaro commented Mar 19, 2024

To be clear I'm not arguing that this PR should include parameter list support. I just want to make sure I'm not missing anything, and know the plan.

@mateusrodriguesxyz
Copy link
Contributor Author

I don't see implementation/tests for functions parameter list. e.g. from your proposal

I think I didn't add this test case because arguments and parameters are parsed by the same parsePostfixExpressionSuffix so testing arguments covers parameters, but I can add it to make it clear.

@rintaro
Copy link
Member

rintaro commented Mar 19, 2024

arguments and parameters are parsed by the same parsePostfixExpressionSuffix

Parameters are parsed with parseParameterClause() not parsePostfixExpressionSuffix() :)

@mateusrodriguesxyz
Copy link
Contributor Author

mateusrodriguesxyz commented Mar 19, 2024

Parameters are parsed with parseParameterClause() not parsePostfixExpressionSuffix() :)

Now that's strange because I never touched this functions but if I add your example as a test case parsePostfixExpressionSuffix is also called and it does parse correctly so I assumed that was it 🤔 It was my intention from the beginning to also support parameters.

@rintaro
Copy link
Member

rintaro commented Mar 19, 2024

That's... concerning (for current parser) 😅

FWIW parsePostfixExpressionSuffix is called whenever parsing an expression. In this case parsing 0 (default parameter value) causes parsePostfixExpressionSuffix

@mateusrodriguesxyz
Copy link
Contributor Author

mateusrodriguesxyz commented Mar 19, 2024

That's... concerning (for current parser) 😅

I think parseParameterClausealready supports trailing comma because !self.at(.endOfFile, .rightParen) breaks the while even if the last parameter has as trailing comma so I think parsePostfixExpressionSuffix was the one blocking it from working it.

https://github.com/apple/swift-syntax/blob/18cd7f90af1615eec95e173d84823a970b7469c6/Sources/SwiftParser/Parameters.swift#L291-L302

@rintaro
Copy link
Member

rintaro commented Mar 19, 2024

Yeah, I mean we've just discovered a bug in the current parser. We should disallow it unless trailingComma experimental feature is enabled.
(I don't think it's a scope of this PR)

@mateusrodriguesxyz
Copy link
Contributor Author

I've update swiftlang/swift#71975 implementation to match this PR.

@mateusrodriguesxyz
Copy link
Contributor Author

mateusrodriguesxyz commented Apr 5, 2024

I've update atStartOfStatementBody implementation and resolved the conflicts. I think that's it? @ahoppen @rintaro Thank you both for the review!

@ahoppen
Copy link
Member

ahoppen commented Apr 5, 2024

One last thing: Could you squash your commits? It makes for a nicer Git history https://github.com/apple/swift-syntax/blob/main/CONTRIBUTING.md#authoring-commits

Other than that, there’s anything to be done from my side. I would like to wait for @rintaro’s approval as well though.

@mateusrodriguesxyz
Copy link
Contributor Author

@rintaro do you have any more to add before I squash the commits?

@rintaro
Copy link
Member

rintaro commented Apr 18, 2024

@swift-ci Please test

@mateusrodriguesxyz
Copy link
Contributor Author

@rintaro I had forget to format the source code after I merged with main, sorry. I think it will pass all the checks now.

@rintaro
Copy link
Member

rintaro commented Apr 18, 2024

@swift-ci Please test

@ahoppen
Copy link
Member

ahoppen commented Apr 18, 2024

@swift-ci Please test Windows

@mateusrodriguesxyz
Copy link
Contributor Author

One last thing: Could you squash your commits?

@ahoppen done!

@kimdv
Copy link
Contributor

kimdv commented Apr 21, 2024

@swift-ci please test

@kimdv
Copy link
Contributor

kimdv commented Apr 21, 2024

@swift-ci please test windows

@ahoppen ahoppen merged commit a85aa07 into swiftlang:main Apr 22, 2024
3 checks passed
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.

5 participants