Implement trailing comma for comma-separated lists

[mateusrodriguesxyz]

This is a implementation for swiftlang/swift-evolution#2344 gated behind -enable-experimental-feature TrailingComma. After feedback from the language steering group I have expanded the original proposal and implementation that I began in #71975.

[xwu]

swiftlang/swift-syntax#2689

@swift-ci build toolchain

[xwu]

swiftlang/swift-syntax#2689

@swift-ci build toolchain

[xwu]

@mateusrodriguesxyz Would it be wise to add tests that exercise the code paths where you've made changes in response to @ahoppen?

[xwu]

swiftlang/swift-syntax#2689

@swift-ci build toolchain

[xwu]

Toolchain links:

• Ubuntu
• Windows
• macOS

[mateusrodriguesxyz]

@hborla as code owner and member of the language steering group do you have advice about how should I proceed now that SE-0439 has been accepted with modifications? Should I remove all code unrelated with the accepted design or the group wants to keep some of it (e.g: if/guard/while conditions) under an experimental flag? Should I ungate the accepted parts and remove the flag? I would appreciate any direction, thanks!

[hborla]

@mateusrodriguesxyz Good question, thanks for asking!

Should I remove all code unrelated with the accepted design or the group wants to keep some of it (e.g: if/guard/while conditions) under an experimental flag? Should I ungate the accepted parts and remove the flag?

Since this proposal doesn't need the experimental feature flag to be promoted to an upcoming feature flag, I recommend un-gating the accepted parts of the design, and leaving the flag in place to gate the parts of the design that were subsetted out. I also recommend splitting up the tests so that the parts of the tests that need the flag are in a separate file that enable the experimental feature, while the bulk of the tests don't enable the flag.

[mateusrodriguesxyz]

@hborla following your recomendations I've ungated the accepted parts and moved the parts of the tests that need the flag to a diffeent file. I also removed support from #available because it really doens't make sense and while running the full suit of parser tests I noticed that it made the diagnostic worse when * is missing.

[xwu]

@mateusrodriguesxyz Would you mind throwing in tests to ensure that [Int,] in type position, @inline(never,), and the like remain invalid? And have you been able to look into which other built-in attributes have custom parameter parsing that doesn't work like a comma-separated parameter list to ensure that we're not adding support for trailing commas in those positions?

[mateusrodriguesxyz]

And have you been able to look into which other built-in attributes have custom parameter parsing that doesn't work like a comma-separated parameter list to ensure that we're not adding support for trailing commas in those positions?

I will look into this as soon as possible.

[mikeash]

There are a lot of formatting changes here. Not sure if we want those changes, but at the very least they should be in their own PR to avoid cluttering the functional changes here, and to make it easier to merge/revert/cherry-pick/etc. this work.

[mateusrodriguesxyz]

Makes sense, I run the git-format and forget about this 😫

[xwu]

Got it. I'm not certain, so the safer route is not to add trailing comma support.

[mikeash]

I guess git-format got a little too eager? The clang-format-diff.py script that comes with clang has worked well for me to apply clang-format to just my changes.

[eeckstein]

Yes, please do not mass-reformat existing code. It makes it nearly impossible to merge between branches without creating a large amount of conflicts.

[mateusrodriguesxyz]

Sorry about the mess. Should I force push to replace the merge commit or make one reverting the formatting?

[al45tair]

FWIW, using git clang-format HEAD^n where n is the number of commits you've made is fine, since that will only reformat things you've changed. Reformatting as you've done here also has the downside that you've pulled in far more reviewers than are likely needed for your actual change.

I'd use force-push to undo the format changes; if you leave them in and add another commit to undo them, I think we can still end up with merge conflicts when trying to merge between branches.

[mikeash]

That's a much nicer looking diff now, thanks!

[ahoppen]

@swift-ci Please smoke test

[nervenes]

hey, just a novice here, what does it mean for a feature to be experimental, like this one? i can see that many features seems to be gated behind experimental flags, but what does it imply? is there a testing period (if so how long is it usually?) where good feedback will make the feature available by default? how does it work. sorry if this might not be the best place to ask it.

also, i can see in the proposal that this was accepted with modifications, what are the modifications? will we be able to have trailing commas for function arguments?

[xwu]

You can refer to the Swift Forums for information about new language features adopted through the Swift Evolution process. In this case, the acceptance with modifications is documented here. This feature isn't experimental, and no flags will be required to use it when it ships. It has not, however, shipped in any officially released version of Swift yet. You can check the release notes of any release to see what features are included in that release. You can try out development snapshots which contain new features not yet released by going to the Swift.org Install page.

Experimental flags are a way to allow work-in-progress to be integrated and tested; they can be added and removed arbitrarily and, for end users, should be disregarded. User-facing features that are meant to be made available to end users generally go through the Swift Evolution process, which is conducted on the Swift Forums. While a feature is being pitched or reviewed as part of the Swift Evolution process, there may be experimental flags mentioned specifically or even custom toolchains so that everyone can try it out and give feedback, but you should not expect any experimental flags to continue working in the same way or at all.

[ahoppen]

@mateusrodriguesxyz What’s the state of this feature? I can see that the Swift Evolution proposal SE-0439 has been accepted but the features is still guarded behind an experimental feature flag.

[xwu]

I believe the experimental feature flag should be guarding those portions of the feature that were not accepted and everything else should be already enabled by default, as per @hborla's suggestion above. The proposal text itself has not been edited to incorporate the steering group's decision.

[mateusrodriguesxyz]

The proposal text itself has not been edited to incorporate the steering group's decision.

Yes, the text is just outdated. I've been meaning to update the proposal to incorporate the steering group's decision but I've been pretty busy the last months.
The accepted parts are already ungated and working without experimental flag in the nightly-main build, just checked on https://swiftfiddle.com.

[ahoppen]

Ah, I remember now. Thanks for clarifying, @mateusrodriguesxyz.