swiftlang
Implement trailing comma for comma-separated lists
This is a implementation for https://github.com/swiftlang/swift-evolution/pull/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 https://github.com/apple/swift/pull/71975.
@mateusrodriguesxyz Would it be wise to add tests that exercise the code paths where you've made changes in response to @ahoppen?
@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!
@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.
@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.
@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?
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.
@xwu I've updated the tests with built-in attributes. None of them currently support trailing comma but maybe backDeployed and storageRestrictions should because they take a variadic list of value as the last argument?
For the places where trailing commas are currently only allowed behind the experimental feature, could you also add a test case that checks that we reject the syntax if the experimental feature is not passed?
I have added these tests at the end: https://github.com/swiftlang/swift/blob/e4ff4846a78633e1361f7f6e735f06ae16771562/test/Parse/trailing-comma.swift#L112-L139
Did you mean this?
Could you also add a test case that checks that we reject the syntax if the experimental feature is not passed?
Sorry, I'm not following. Could you please give an example of what you mean?
Sorry, I'm not following. Could you please give an example of what you mean?
I was stupid. Didn’t realize that trailing-comma.swift vs trailing-comman-experimental.swift is exactly what I wanted. Sorry for the confusion.
@xwu I've updated the tests with built-in attributes. None of them currently support trailing comma but maybe backDeployed and storageRestrictions should because they take a variadic list of value as the last argument?
IMO, whatever @backDeployed is doing @available should also, but as this hasn't been worked through, if all the list handling in built-in attributes are custom and your PR never made any changes to the status quo then we can consider them not part of the proposal and leave it alone since that would be most consistent with the review decision.
If the proposal had been accepted with the original scope that everything that's a comma-separated list with an unambiguous terminator should support a trailing comma, then we would need to puzzle over which of these attributes have such a regular list, but we don't have to do that now :)
if all the list handling in built-in attributes are custom
FWIW backDeployed actually uses parseListItem with AllowSepAfterLast set to false.
Ship it?
Hi @xwu, just one last thing: Should I remove @attached trailing support since it's a built-in attribute?
I'm kind of surprised to learn it was included in the first place, as it's not really a list. I had thought you'd audited all the built-in attributes and none support trailing comma in your PR? Or are you referring to the named(...) part?
No, I mean something like @attached(extension, conformances: P1, P2,). I think I've implented this one way back in the PR because it's parsed in a different way then the others built-in attributes. I will remove it.
Got it. I'm not certain, so the safer route is not to add trailing comma support.
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.
Yes, please do not mass-reformat existing code. It makes it nearly impossible to merge between branches without creating a large amount of conflicts.
Sorry about the mess. Should I force push to replace the merge commit or make one reverting the formatting?
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.
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?
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.