Expand trailing comma implementation #2689
Conversation
There was a problem hiding this comment.
Thanks for your continued work on this @mateusrodriguesxyz!
| @@ -700,7 +704,7 @@ extension Parser { | |||
| arena: self.arena | |||
| ) | |||
| ) | |||
| } while keepGoing != nil && self.hasProgressed(&loopProgress) | |||
| } while keepGoing != nil && self.hasProgressed(&loopProgress) && !self.atWhereClauseListTerminator() | |||
There was a problem hiding this comment.
Generic where clauses don’t necessarily need to be followed by a brace. For example you can have the following, which is valid.
protocol Foo {
func test<T>(abc: T) where T: Comparable
}Because of the lack of a proper terminated for the where classes, I’m actually not sure if we should allow trailing commas here. Checking if the next token is a keyword also doesn’t work because it could be a contextual keyword like nonisolated on the following function declaration.
| mutating func atEnumCaseListTerminator() -> Bool { | ||
| return self.experimentalFeatures.contains(.trailingComma) | ||
| && (self.atStartOfLine || self.at(.rightBrace) || self.at(.semicolon)) | ||
| } | ||
|
|
There was a problem hiding this comment.
Wouldn’t this mean that we’d stop parsing the following as valid because we classify the comma as a trailing comma and then mark bar as unexpected?
enum A {
case foo,
bar
}Similar to the where clause, I’m not sure if we want to allow a trailing comma here because there is no list terminator.
There was a problem hiding this comment.
Completely missed this, which is ironic because that's exactly how one would use a list with trailing comma 😅 I will double check all comma-separated list to make sure there is a clear terminator and remove the one without it.
| mutating func atInheritanceListTerminator() -> Bool { | ||
| return self.experimentalFeatures.contains(.trailingComma) && (self.at(.leftBrace) || self.at(.keyword(.where))) | ||
| } | ||
|
|
There was a problem hiding this comment.
I think this doesn’t work in general because associatedtype can also have an inheritance clause and it is not followed by { or where.
Eg.
associatedtype Iterator: IteratorProtocol, would probably fail to parse wit this change.
I think we should limit the trailing comma to the types where it is guaranteed to be followed by where or { (ie. everything except associatedtype).
There was a problem hiding this comment.
@ahoppen I believe it's just a matter of update the proposal text mentioning that trailing comma won't work for inheritance clause of an associatedtype, right? The same for where clauses for protocol definition. In general, make it clear that trailing comma will only be allowed when there's a clear terminator.
There was a problem hiding this comment.
I’m only speaking as a reviewer of this PR here but I think it would make sense to exclude trailing commas for these situations without a clear terminator from the evolution proposal for now.
There might be ways of getting them right, but doing so is more complicated than the cases that have a clear terminator. Highlighting these situations in a Future directions section would make sense, I think. That way your proposal can focus on the majority of cases and if we do choose to extend trailing commas eg. case lists, the corner cases will get the attention they deserve.
There was a problem hiding this comment.
I've updated the proposal text highlighting these cases. Thanks for catching them.
|
@ahoppen I've updated the PR to match swiftlang/swift#74522. |
|
@swift-ci Please test |
|
@ahoppen Don't you prefer that I squash the commits before merging? |
Head branch was pushed to by a user without write access
mateusrodriguesxyz
force-pushed
the
trailing-comma-2
branch
from
58c4b5d to
725d6c2
Compare
|
@swift-ci Please test |
|
@swift-ci Please test Windows |
|
Could you run swift-format on your changes? |
Head branch was pushed to by a user without write access
mateusrodriguesxyz
force-pushed
the
trailing-comma-2
branch
from
725d6c2 to
1d11c43
Compare
|
@swift-ci Please test |
1 similar comment
|
@swift-ci Please test |
This continues the work that I began in #2515 (comment). After feedback from the language steering group I have expanded the original proposal to allow trailing comma in more places. See the updated proposal text in swiftlang/swift-evolution#2344.