apple
Trailing comma implementation
This is a prototype implementation for Allow trailing comma in tuples, arguments and if/guard/while conditions gated behind -enable-experimental-feature TrailingComma.
@ahoppen I've updated the implementation for conditionals to use a much simpler lookahead approach
@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.
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?
I just suggested to keep the PR as-is for now and implement trailing closures for other constructs in a follow-up PR (https://github.com/apple/swift-syntax/pull/2515#discussion_r1530160204). 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.
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.
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.
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?
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.
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.
arguments and parameters are parsed by the same parsePostfixExpressionSuffix
Parameters are parsed with parseParameterClause() not parsePostfixExpressionSuffix() :)
Parameters are parsed with
parseParameterClause()notparsePostfixExpressionSuffix():)
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.
That's... concerning (for current parser) 😅
FWIW parsePostfixExpressionSuffix is called whenever parsing an expression. In this case parsing 0 (default parameter value) causes parsePostfixExpressionSuffix
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
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)
I've update https://github.com/apple/swift/pull/71975 implementation to match this PR.
I've update atStartOfStatementBody implementation and resolved the conflicts. I think that's it? @ahoppen @rintaro Thank you both for the review!
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.
@rintaro do you have any more to add before I squash the commits?
@rintaro I had forget to format the source code after I merged with main, sorry. I think it will pass all the checks now.
