Skip to content

SIP-27 Trailing Commas #533

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

Merged
merged 9 commits into from
Sep 23, 2016
Merged

SIP-27 Trailing Commas #533

merged 9 commits into from
Sep 23, 2016

Conversation

dwijnand
Copy link
Member

@dwijnand dwijnand commented Jun 27, 2016

This is my first SIP. I'm happy to take feedback and iterate where needed.

An initial implementation has also been proposed in scala/scala#5245.

Link to Scala grammar breakdown: #533 (comment)

Link to proposing trailing commas only for multi-line: #533 (comment)


Summary of subjective pros and cons (added by @cvogt):

Pros:

  • avoids problems with last parameter/argument
  • eases commenting out
  • eases swapping
  • avoids merge conflicts
  • less reliance on tool support or manual labor

Cons:

  • one more terminating grammar rule per construct
  • room for accidental correctness bug def foo(a: Int, b: Int = 0) = 0; foo(1,)
  • harms readability (are there many people who feel this way?)


The implementation is a simple change to the parser, allowing for a trailing comma, for the groups detailed above, and has been proposed in [scala/scala#5245][].

## Drawbacks
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth to mention that tooling/parsers will have to catch up. I can think of intellij-scala, scalariform, scala.meta and scalaparse.

(I would love to have trailing commas so big +1 from me.)

Copy link
Member Author

Choose a reason for hiding this comment

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

By scalaparse do you mean https://github.com/lihaoyi/fastparse/tree/master/scalaparse?

Btw it was my intent to submit changes to those as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I meant fastparse/scalaparse. My only comment was to include this as a drawback, since updating those tools will require some work.

Copy link
Member Author

Choose a reason for hiding this comment

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

@olafurpg I didn't add it because I thought it might be true for all SIPs, but then maybe not. Added, thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

You should mention that it massively simplifies generating scala source code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@xeno-by
Copy link
Contributor

xeno-by commented Jul 15, 2016

Hello @dwijnand!

Welcome to the new edition of the SIP process. My name's Eugene, and I'm going to be your reviewer. This means that I will be the contact person between you and the SIP committee, outlining your progress and providing you with our feedback.

During the SIP meeting on 13 Jul 2016, I presented your proposal to the committee according to the "Formal presentation" phase described in the new rules (http://docs.scala-lang.org/sips/sip-submission.html). At this phase, we are supposed to decide whether following through your SIP is a good idea in principle.

I have good news. We decided that your proposal has the potential to move Scala in the right direction, so we'd like to clear the "Formal presentation" phase and move the proposal to "Formal evaluation". We will now merge this pull request as SIP-27 and will proceed with evaluation.

During the "Formal evaluation" phase, we will work with you to make sure that the proposal reaches its maximum potential. When we're sure that we have a comprehensive understanding of your proposal and its consequences on Scala, we will have a final vote and will announce our verdict.

As the first step of the evaluation process, we would like to ask you to address the following feedback:

  • Update your implementation to cover all cases in the Scala grammar that have comma-separated lists. You can follow Denys Shabalin's suggestions from his comment on your pull request to the Scala compiler for an exhaustive list of such cases.
  • Update your write-up to state and analyze potential corner cases that will be created by the proposal. During the meeting, the committee discussed one of such corner cases: (x,). We would like to make sure that we understand the potential caveats completely.

Thank you for contributing to the future of Scala. Looking forward to hearing from you again!

@dwijnand
Copy link
Member Author

dwijnand commented Jul 15, 2016

Hey @xeno-by, that's excellent news!

I'll spend more time on this soon, but I have a quick question: should I really jump to updating my implementation for all cases? Because I think there are some parts of the grammar where trailing commas don't really make sense nor are common enough to be worth doing it anyways, such as val a, b, = 1.

@xeno-by
Copy link
Contributor

xeno-by commented Jul 15, 2016

Thank you for your swift reply! Please also ask further questions in this thread - I believe that open discussion greatly benefits language evolution.

What I meant is that I think it's a good idea to explore all cases, both theoretically and practically. As you rightfully pointed out, there may be cases where trailing commas aren't worth it. Please feel free to ignore these cases in your implementation. But in any case do document your findings - I'm confident that they will be of help to our subsequent discussions.

@dwijnand dwijnand changed the title SIP - Trailing Commas SIP-27 Trailing Commas Jul 15, 2016
@jvican
Copy link
Member

jvican commented Jul 22, 2016

Hello @dwijnand, the design and implementation of this SIP will be discussed in the next SIP meeting on 10th August. I encourage you to provide more insights into the possible corner cases of your implementation and how you would handle them. It would be good to update this document to reflect those and explain all the interactions with existing language features. This will surely speed up the proceess and improve the discussions in our meeting, making them more productive. For more information, please get in touch with your reviewer @xeno-by.

@dwijnand
Copy link
Member Author

I didn't realise it was so soon, but, yes, I'll make sure to respond to the initial round of feedback, such that this SIP can (hopefully) progress next meeting.

Wasn't this PR meant to be merged if it's provisionally accepted as is, and then go through some finessing and final decision?

@jvican
Copy link
Member

jvican commented Jul 22, 2016

Yes Dale, that's the plan, I'm planning to merge it soon. Please, note that even if this PR is merged, all the feedback will still go into this PR, so that all the discussion is centralized in the same place.

@dwijnand
Copy link
Member Author

dwijnand commented Aug 5, 2016

I've studied the relevant parts of the Scala grammar with regards to adding trailing commas (breakdown and my thoughts below).

Summary (TL;DR)

In my opinion not all comma-separated elements of the grammar make sense to support trailing commas, but they could, for across-the-board consistency. I leave it to the discretion of the SIP committee to let me know what they think.

I focussed particularly on the interaction between trailing commas and Tuple1. For context the issue is that Scala allows both individual terms and individual types to be surrounded by optional parentheses which causes ambiguity for the Tuple1 case, ambiguity that can be removed by using a trailing comma to signal a Tuple1.

My proposal for these ambiguous cases is to follow @sjrd's advice and make them not compile, like they don't currently. The alternative is make it syntax for Tuple1. I leave it to the discretion to SIP committee to let me know which alternative is preferred.

Once the SIP committee responds to these notes I'll incorporate them into the SIP document.

Breakdown

SimpleExpr1

spec

      Exprs ::= Expr {‘,’ Expr}
SimpleExpr1 ::= ‘(’ [Exprs] ‘)’

examples

(1)
(1,)
(1, 2)
(1, 2,)

Thoughts

As highlighted in the SIP meeting, there is an edge-case here with regards to Tuple1. Currently an expression like (1,) doesn't compile, and a naive implementation of adding trailing commas would make it compile as an Int by simply ignoring the trailing comma. But (1,) would be very convenient syntax for Tuple1 (indeed it's the way the REPL prints Tuple1 and it's the syntax used in Python).

Having a syntax for Tuple1 is of increasing importance as there is a desire to be able to abstract over tuples/hlist-like tuples in a future version of Scala.

I propose we allow trailing commas in expressions, but, following @sjrd's advice, we make expressions like (1,) not compile, like it does today, leaving it available for Tuple1 in the future.

The alternative is to change Scala such that (1,) is the syntax for a Tuple1, but perhaps this is beyond the scope of this SIP.

ArgumentExprs

spec

        Exprs ::= Expr {‘,’ Expr}
ArgumentExprs ::= ‘(’ [Exprs] ‘)’

examples

Seq(1)
Seq(1,)
Seq(1, 2)
Seq(1, 2,)

Thoughts

During the SIP meeting @odersky wondered if a trailing comma should be allowed here, because to avoid auto-tupling ambiguity perhaps a trailing comma could differentiate between a single argument hlist-like tuple and multiple arguments.

Personally I think trailing commas should be optionally available for both argument lists and hlist-like tuples, with clarifying parenteses for the latter, as is needed currently to avoid the warning under -Xlint.

Params and ClassParams

spec

     Params ::= Param {‘,’ Param}
ClassParams ::= ClassParam {‘,’ ClassParam}

examples

def foo(a: Int)
def foo(a: Int,)
def foo(a: Int, b: Int)
def foo(a: Int, b: Int,)

class Foo(a: Int)
class Foo(a: Int,)
class Foo(a: Int, b: Int)
class Foo(a: Int, b: Int,)

There is no concern here about Tuple1.

ImportSelectors

spec

ImportSelectors ::= ‘{’ {ImportSelector ‘,’} (ImportSelector | ‘_’) ‘}’

examples

import foo.{ Bippy }
import foo.{ Bippy, }
import foo.{ Bippy, Dingo }
import foo.{ Bippy, Dingo, }

There is no concern here about Tuple1.

ids, ValDcl, VarDcl, VarDef and PatDef

spec

   ids ::= id {‘,’ id}
ValDcl ::= ids ‘:’ Type
VarDcl ::= ids ‘:’ Type
VarDef ::= ids ‘:’ Type ‘=’ ‘_’
PatDef ::= Pattern2 {‘,’ Pattern2} [‘:’ Type] ‘=’ Expr

examples

val a = 1
val a, = 1
val a, b = 1
val a, b, = 1

var a = 1
var a, = 1
var a, b = 1
var a, b, = 1

var a = _
var a, = _
var a, b = _
var a, b, = _

var Foo(a) = bippy
var Foo(a), = bippy
var Foo(a), Bar(b) = bippy
var Foo(a), Bar(b), = bippy

Thoughts

There is no concern here about Tuple1, but I don't think that a trailing comma would used much here, and so I didn't include it originally in my proposal. But for consistency it could be added.

TypeArgs, TypeParamClause and FunTypeParamClause

spec

             Types ::= Type {‘,’ Type}
          TypeArgs ::= ‘[’ Types ‘]’

  VariantTypeParam ::= {Annotation} [‘+’ | ‘-’] TypeParam
   TypeParamClause ::= ‘[’ VariantTypeParam {‘,’ VariantTypeParam} ‘]’
FunTypeParamClause ::= ‘[’ TypeParam {‘,’ TypeParam} ‘]’

examples

Foo[A]
Foo[A,]
Foo[A, B]
Foo[A, B,]

def foo[A]
def foo[A,]
def foo[A, B]
def foo[A, B,]

Thoughts

There is no concern here about Tuple1, and I think here it makes more sense to allow optional trailing commas, as sometimes the types in a type argument list can be very long, so I would be happy to add support here.

SimpleType and FunctionArgTypes

spec

           Types ::= Type {‘,’ Type}
       ParamType ::= Type | (‘=>’ Type) | (Type ‘*’)
      SimpleType ::= ‘(’ Types ‘)’
FunctionArgTypes ::= ‘(’ [ ParamType {‘,’ ParamType } ] ‘)’

examples

(Int)
(Int,)
(Int, Int)
(Int, Int,)

def m(f: (Int) => Int)
def m(f: (Int,) => Int)
def m(f: (Int, Int) => Int)
def m(f: (Int, Int,) => Int)

Thoughts

The story here is similar to SimpleExpr1: problematic with regards to Tuple1. This is because (Int) means Int and (Int) => Int means Int => Int, where (Int,) and (Int,) => Int would be convenient syntax for the types Tuple1[Int] and Function1[Tuple1[Int], Int].

My proposal here matches my proposal for SimpleExpr1: allow trailing commas but make the Tuple1 cases compilation failures.

Bindings

spec

   Binding ::= (id | ‘_’) [‘:’ Type]
  Bindings ::= ‘(’ Binding {‘,’ Binding} ‘)’
      Expr ::= Bindings ‘=>’ Expr
ResultExpr ::= Bindings ‘=>’ Block

examples

f((x) => 1)
f((x,) => 1)
f((x, y) => 1)
f((x, y,) => 1)

Thoughts

With "funciton arity adaptation" (from @odersky's slides for both Scala Days keynotes) there is also concern here with Tuple1. However I don't know how common, and therefore necessary, trailing commas are here, so I wouldn't change anything. But for consistency it could be added, with the usual exclusing for (x,).

SimplePattern

spec

     Patterns ::= (Pattern [‘,’ Patterns]) | (‘_’ *)
SimplePattern ::= [StableId] ‘(’ [Patterns] ‘)’

examples

case (1)
case (1,)
case (1, 2)
case (1, 2,)

case Seq(1)
case Seq(1,)
case Seq(1, 2)
case Seq(1, 2,)

Thoughts

For the case where StableId is missing, ie. the tuple syntax, again there is ambiguity for the 1-arity case. Again I propose we just make it a compilation failure.

Import

spec

Import ::= ‘import’ ImportExpr {‘,’ ImportExpr}

examples

import foo._
import foo._,
import foo._, Bippy._
import foo._, Bippy._,

Thoughts

There is no concern here, but I really don't think it's necessary to add trailing comma support here, except for consistency.

Final notes

@xeno-by Let me know if there's anything else you need for me for the next SIP meeting. Sorry for leaving it so late.

@jvican
Copy link
Member

jvican commented Aug 5, 2016

Thanks for the thorough explanation Dale. As a next step, it would be good to add this to the SIP document. We'll discuss this in the next meeting.

)
{% endhighlight %}

Also such feature would massively simplify generating Scala source code.
Copy link
Member

Choose a reason for hiding this comment

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

s/massively/modestly/ IMO — "massively" just seems like hyperbole. or is it really that big an issue in your experience? (I do have some experience writing code generators that omitted trailing commas and found it annoying, but really not that a big deal, either.)

@dwijnand
Copy link
Member Author

@SethTisue thank you very much for the review, I've made some changes from your feedback. I hope you think that's more appropriate and let me know if not.

@jvican Thank you, I hope you're ok with me updating the document with the breakdown once the SIP committee has provided some feedback.

@Ichoran
Copy link

Ichoran commented Aug 10, 2016

I am not very enthusiastic about this proposal for a number of reasons.

  1. As read now, comma means "there is another one coming". With this SIP, comma means, "I dunno if there's another one coming or not, but there might be". This is bad for several reasons: it is strictly less informative, it does not follow the conventions of Western languages like English,
  2. Commas gain weird special cases. Although the size of the grammar doesn't really change, the complexity of the grammar does as the pattern is not always something like {Item ',' More} | Item. This directly impacts the cognitive burden on people learning the language.
  3. It encourages more verbose code not less, making code generally messier not cleaner. Since it is optional, it will likely be used both ways, raising the cognitive burden of reading others' code and writing in a consistent style on multiple projects.
  4. The problem that it addresses just doesn't seem very serious to me. I move lists of things around all the time. It takes a few extra seconds to fix up the separators. Yes, it's a bit irritating. I wish my editor would handle it for me, along with all sorts of other things like understanding when I'm opening a new pair of braces and when I'm just replacing an opening brace that I accidentally deleted.

I've used this feature in Julia and I don't like it; when it's very important to know the size of one's vector, not having an at-a-glance count of the number of commas be one less than the length is a minor but unwelcome distraction. I can't see why I'd like it any better in Scala.

That said, if it is decided that something like this is desired, I would much prefer if commas and semicolons acted similarly. That is, when you can have multiple semicolons you can throw as many of them in as you want between expressions as long as you have at least one (and empty expressions don't count). For instance:

scala> val x = if (true) {;;; 7 ;;;} else 2
x: Int = 7

Actually using this capability is rare, but it is regular in that semicolon acts like a type of super-whitespace; just as Scala doesn't care about how much whitespace there is or whether there's any at all, as long as the tokens are delimited somehow, it also doesn't care about just how many semicolons there are or whether there's any statement at all.

Commas now are special in that they always and only separate items of some sort. Making them obey the same conceptual rules as semicolons and whitespace would, I think, simplify the language conceptually overall. There's only one separator concept; you just have three kinds of separation (token, item, expression, ) and thus need different tokens to distinguish them (with newline usually sufficing for semicolon).

Thus, we would allow

val ,,,,, x= if (true) Seq(,,, 7 ,,,) else Seq(2)

Not that you should write it this way, but I think picking the "nice" syntax from the spectrum of logically consistent syntax is the job more of linters and style checkers than the language specification itself. (Unless the language is Python.)

Again, I think the benefits of having comma be a singular separator outweigh it being conceptually consistent with whitespace and semicolon, but I think the existing proposal is less desirable than either.

@DavidDudson
Copy link

If this proposal were to go ahead it would at least be good to have a compiler warning for multiple trailing commas/semi-colons eg (a,,) That way at least the recommendation from scalas language point of view is at most 1 trailing comma

@cvogt
Copy link
Contributor

cvogt commented Aug 10, 2016

@Ichoran

I doubt commas could become optional like semi-colons, as that would make it ambiguous when an expressions spans multiple lines or not. So we can't make the rules identical to ;. What makes it ok in my eyes, that {} and () already have different rules as they cater to different use cases (while having overlaps in semantics). ; as a supporting concept for {} and , as a supporting concept for () working differently seems fine as those work differently.

I don't see why the grammar would be much more complicated. It would just need another case terminating repetition including a comma.

I think one of the main reasons for the proposal of this change is what some perceive as a usability problem. Writing text in an editor is the user interface for developing programs. Developers may use this interface (text) differently, but there seems to be a significant group (I among them), who have the following problem: When parameters or arguments are indented one per line, commenting out the last one requires also removing the trailing comma from the previous line. We perceive this as a usability problem, an irregularity, a distraction from getting stuff done. This might not be the biggest problem there is, but annoying enough to a good number of people to bother making an effort to change it.

@retronym
Copy link
Member

retronym commented Aug 10, 2016

I agree with Rex's sentiments here. We've come to regret having more than one way to do things syntactically (e.g. procedure syntax).

IntelliJ users can move elements within a comma separated list with the "Code -> Move element {left, right}" (or the corresponding shortcut):

image

I'm also uncomfortable about the following code that reports a syntax error into something that typechecks. , currently means "I'm about to add something".

scala> def foo(a: Int, b: Int = 0) = 0; foo(1,)
<console>:1: error: illegal start of simple expression
def foo(a: Int, b: Int = 0) = 0; foo(1,)
                                       ^

Incidentally, I found a similar enhancement request for Java that suggested to generalize its support trailing commas in array initializers to all comma separated parts of the syntax. That request wasn't accepted, although there wasn't much constructive discussion of the reasons why. Scala has a history on mimicing Java's syntactic decisions closely.

@cvogt
Copy link
Contributor

cvogt commented Aug 10, 2016

@retronym

Many of us do not use IntelliJ for usability reasons in particular responsiveness. Different people choose different trade-offs here.

Editing the last element of argument lists happens all the time. I can see your described use case, but it seems rare to me personally. But I understand your case, potential correctness problems have gravity. Everything's a trade-off. I personally tend towards usability in this particular case.

Scala tries to have syntax that is easier to work with than Java, yet familiar to Java-style language folks. I see trailing commas supporting the first and yet not violating the second.

@cvogt
Copy link
Contributor

cvogt commented Aug 10, 2016

But good point. It's not only commenting out parameters/arguments, also moving them that has this usability issue.

@Ichoran
Copy link

Ichoran commented Aug 10, 2016

@cvogt - Can't you just

List(
  fiddesticks /*,
  humbug*/
)

instead of changing the language?

Alternatively, can't you fix your editor to make it easy for you so it works for all languages, not just Scala? There are fewer heavily-used editors than languages.

Healing line-by-line commas is simple: if you comment out a line, and the next line is a close of some sort, and the previous line ends in a comma, comment out the comma as well as the line. If you uncomment it, and the next line is a close, and the previous line has a comment that starts with a comma, remove the comment around the comma also. Compared to semantics-aware refactoring, this is a pretty easy operation. Adding a new color for "syntactically necessary comment", and making that be close to the background color, would remove the visual distraction also.

Moving lines around isn't so hard either.

Moving stuff around on the same line is hard because there is nothing to cue off of except a parse of the language at that point. That's much heavier lifting for an editor / IDE. But this seems pretty esoteric for me, and as I mentioned in chat, it doesn't solve the problem of how to comment stuff out on the same line. (For that to be easier you need commas-anywhere.)

@OlegYch
Copy link

OlegYch commented Aug 10, 2016

@Ichoran this is the sort of gymnastics i've been doing for years, and it's tiresome..
fixing editors might be a good idea, but everyone has a unique set of editors they use (and probably not comfortable at modifying) and we see a new editor popup on a weekly basis...

@cvogt
Copy link
Contributor

cvogt commented Aug 10, 2016

@Ichoran This proposal has a significant number of supporters because we find exactly that a usability hinderance. People are different :).

Regarding editor plugins: Many of us prefer syntax that is easily editable as much as possible without external tooling support. For everything that requires tool support we limit ourselves to tools that support this feature and have to remember how to use it and there is a limited memory budget for things like that, I'd prefer to spend on other things.

Suggestion: Recognizing that a significant number of people would feel their life becomes easier with this, let's focus on how other people feel their life would become harder with this and then weigh off the trade-off.

@dwijnand
Copy link
Member Author

dwijnand commented Sep 4, 2016

@soc

I still think that this SIP should be rejected, because:

It introduces another pointless way to write exactly the same thing. We spent roughly the last 5 years of getting rid of these variations throughout the language. This will undo all the progress we have made in the last half decade.

I disagree strongly that it's pointless, and I invite you to look at the positive feedback the original Scala PR and this SIP has attracted in order to acknowledge that a lot of people also disagree with you.

I also personally think it's unnecessary hyperbole to say that this "will undo all the progress we have made in the last half decade".

I care about your point of view and your feedback, but you make it difficult for me to take it seriously when you disregard obvious evidence and exagerate like that. Continuing to mock the effort put behind this SIP and importance some people give to this SIP by calling it "SIP-commas" doesn't help you either.

This is the job of the editor/IDE. Just like we shouldn't have to worsen our code-style (don't align rows!!!) to get nice diffs in git, we shouldn't have to worsen our syntax to make editing easier.

It's a digression, but know that aligning rows actually doesn't get nice diffs in git, but it does improve the legibility of code, in my opinion.

Instead of pushing the burden of managing how to add, remove or move elements in a comma-separated list to each and every editing tool out there, I'm proposing that scalac learns how to manage trailing commas instead.

Code is read a lot more often than it is written. People reading the code shouldn't have to suffer from the syntax which only exists to make editing slightly nicer.

I believe that the trade-off between how much people suffer to read and ignore a trailing comma and how much people suffer to micro-managing the lack of a trailing comma when editing code is heavily in favour of the latter. It's the duty of the committee to decide and vote if they agree with me on this.

What is the style guide supposed to tell users?

If you're asking my personal opinion, I would say it should say that trailing commas on multi-lines are accepted, for the conveniences specified in this SIP.

@dwijnand
Copy link
Member Author

dwijnand commented Sep 4, 2016

@Ichoran

Can we get a comparison of the pros/cons of allowing this only for multi-line expressions, and a comparison of how bad this is compared to forgetting the commas in the first place (which I not infrequently do when I'm writing dependencies into SBT)?

In particular, the rule would be that instead of whatever ~ ')' you could have whatever ~ (',' ~ newline ~ ')' | ')') to use FastParse-inspired syntax. This seems like it would retain all the listed advantages without exposing some of the disadvantages like making it harder to quickly judge the arity.

I think that it wouldn't help with single-line lists, but that wasn't mentioned, and when moving things on single lines you have to be all fiddly with making sure you grab the right comma anyway, so it's less of a big deal than lines where you're grabbing lines anyway, except the commas don't tear away properly when you do it.

I agree with you, which is why I'm proposing we restrict this change to only multi-line comma-separated elements.

@dwijnand
Copy link
Member Author

dwijnand commented Sep 4, 2016

@som-snytt

I like the simplifying restriction that it apply only for multiline exprs.

As do I.

@dwijnand
Copy link
Member Author

dwijnand commented Sep 4, 2016

@fanf

You all are aware that most of the pros can be achieved just by adopting an other breaking line convention:

List(
  a
, b
, c
)

Quoting the pros, we have all of them for free (safe for the first line, but arguably it changes much less often) :

avoids problems with last parameter/argument

  • eases commenting out
  • eases swapping
  • avoids merge conflicts
  • less reliance on tool support or manual labor

I am aware, but I reject it as a workaround for 3 reasons.

First, as you mention, it moves the problem to the first element, which only maybe might not be a problem.

Secondly, in the case where a change must happen near the first element (either by adding one or more elements before the first element, or by remove the first element, either entirely or by commenting out) it makes the change a lot harder, because it requires micro-managing the leading comma, replacing it in situ with a space.

Thirdly, it's a large departure from the "normal" style - in contrast I'm only proposing an optional trailing comma.

One final note, which isn't an objective reason but my own personal taste, I can't stand that style.

You might be interesting to know that @olafurpg has added an experimental poorMansTrailingCommasInConfigStyle to his tool scalafmt (shipped with version 0.3.1): https://github.com/olafurpg/scalafmt/pull/412

@dwijnand
Copy link
Member Author

dwijnand commented Sep 4, 2016

@fanf

On the other hand, if trailing comma are added, I woukd like the support of heading comma to also fully support the style presented.

I'm personally not interesting in supporting that, but if you feel strongly enough about it, I invite you to consider creating a SIP for it.

@dwijnand
Copy link
Member Author

dwijnand commented Sep 4, 2016

@eed3si9n

I have a pro and a con to add.

VCS authorship attribution

As many have noted, we spend more time reading source than writing the code. In a distributed development, I often read the source code using Github by using t shortcut to search, and then using git blame view, and tracking the history of when/how a bug may have been added.
One of the rationale that's often mentioned is diff for code reviews, but I think having correct commit history (because with trailing comma you don't have to fix the last item) would have longer lasting effect for the code base.

That's a good point, thank you, I'll add it to the SIP.

Cross building hindrance

One negative point I can think about, assuming this doesn't get back ported to 2.10 and 2.11 is that it diverges the shape of the source and prevents cross building that may have been possible otherwise.

While I agree that it's a con that must be managed and considered, it's also not unique to this SIP.

@dwijnand
Copy link
Member Author

dwijnand commented Sep 4, 2016

@odersky

Personally, the only issue I care about is a consistent syntax for HLists.

Please see my grammar breakdown, where I address the concern about tuples/HLists and Tuple1.

@dwijnand
Copy link
Member Author

dwijnand commented Sep 4, 2016

@fanf

And if sometimes, a trailing comma is mandatory and sometimes not, it is just an enormous cognitive weight, even more for new users. And a tar pit for linters.

No, the proposal doesn't mandate trailing commas.

@dwijnand
Copy link
Member Author

dwijnand commented Sep 4, 2016

@xeno-by

1. Questions for the SIP committee:

  1. What does the commitee think if trailing commas were only allowed for multi-line comma-separated elements?

This would mean that this would be allowed:

class Foo(
  bippy: String,
  dingo: Int,
)

but this would not:

class Foo(bippy: String, dingo: Int, )

Then there is the question of how much of the Scala grammar should support trailing commas.

In, what I consider to be, decreasing order of importance:

Important to support:

  1. Should trailing commas be supported for ArgumentExprs?

  2. Should trailing commas be supported for Params and ClassParams?

Less important to support:

  1. Should trailing commas be supported for SimpleExpr1? And if yes should (1,) not compile or compile to a Tuple1?

  2. Should trailing commas be supported for TypeArgs, TypeParamClause and FunTypeParamClause?

  3. Should trailing commas be supported for SimpleType and FunctionArgTypes?

  4. Should trailing commas be supported for SimplePattern?

  5. Should trailing commas be supported for ImportSelectors?

Not important to support, except for consistency reasons:

  1. Should trailing commas be supported for Import?

  2. Should trailing commas be supported for Bindings?

  3. Should trailing commas be supported for ids, ValDcl, VarDcl, VarDef and PatDef?

2. Compare to the HList discussion in Dotty

To my limited understanding of https://github.com/lampepfl/dotty/issues/964, I don't see any conflict between my proposal and Dotty's proposal.

3. Salvaging non-controversial parts

My initial proposal was restricted to a small subset of (what I considered) non-controversial parts of the grammar, then the SIP committee requested to explore all the cases of the grammar that have comma-separated lists, which I did. Personally I'm happy to go either way on this.

4. Two ways of doing the same thing

From very early in this SIP it has been acknowledged that this is a drawback/trade-off. Perhaps restricting trailing commas to multi-line reduces the gravity of this drawback.

@dwijnand
Copy link
Member Author

dwijnand commented Sep 4, 2016

@jvican While the details of this SIP are still being worked out, specifically given the number of questions I have for the committee, I would like to hold off from committing even more time changing (and re-changing) the specifics in the actual SIP document until the situation is clearer.

You have my word though that even in the event that the SIP is rejected I would update the SIP document accordingly.

@Ichoran
Copy link

Ichoran commented Sep 4, 2016

Personally, I have far fewer issues with trailing commas only when newline is allowed. There is still the concern about whether there is a missing item, but unlike the inline case where a missing entry is likely and the editing help you get is minimal, with separate lines, a missing entry is unlikely and the editing help you get is maximal. Definitely shifts the risk/reward ratio.

I remain concerned about making the specification more complex, but if it's isolated to multiple lines and phrased like semicolon inference but backwards, it feels a lot tidier.

And I would make it apply always, no exceptions. If you can have something separated by commas and it can go on separate lines, you can always have a trailing comma and it's cool.

@dwijnand
Copy link
Member Author

dwijnand commented Sep 4, 2016

And I would make it apply always, no exceptions. If you can have something separated by commas and it can go on separate lines, you can always have a trailing comma and it's cool.

I personally could live with that, but I'm not sure if everyone would.

Also it would have to be a gradual change though, particularly given there are libraries that cross-build to multiple versions of Scala.

@Ichoran
Copy link

Ichoran commented Sep 4, 2016

I can't comfortably live with anything other than a completely consistent rule. I have too many other things to pay attention to.

@jvican
Copy link
Member

jvican commented Sep 5, 2016

@dwijnand I'm glad to see that you have so many interesting questions for the Committee! I'll ask them in our next meeting, that coincidentally takes place at Scala World, on the 13th of September (that's next week).

Thanks for your commitment, Dale.


Given that this is a change in syntax, another drawback is that it requires changing the existing tools, such as those that parse Scala: intellij-scala, scalariform, scala.meta and scalaparse.

Another drawback is that for projects that cross build to previous versions of Scala they would have to take into account that this feature wouldn't be available for those versions (assuming this feature isn't backported).
Copy link
Member

Choose a reason for hiding this comment

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

How is this any worse than for any other change to Scala? Even simple additions to the standard library are subject to this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed

I thought it's fair to mention it.

Copy link
Member

Choose a reason for hiding this comment

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

IMO this is noise. I was trying to figure out what made this SIP special about this. IMO this paragraph should be removed.

Copy link

Choose a reason for hiding this comment

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

It leads to source incompatibility at the language level. It implies a major version bump, not only a minor one (as far as I understand Scala policy regarding that, for ex. https://twitter.com/viktorklang/status/735772498780819457)

Copy link
Member

Choose a reason for hiding this comment

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

It is not forward source compatible (most things aren't), but it is backwards source compatible. So an existing codebase will always compile with a new compiler. This only requires a minor version bump.

FTR, adding a method to the standard library is less compatible than that. It can always break backward source compatibility of some codebases, if they pimped a method of the same name via implicits: the new method would suddenly take over instead of the implicit pimp. If the semantics are different, this breaks code.

This change is much more compatible than adding a method, as it can never break existing codebases.

Copy link
Member

Choose a reason for hiding this comment

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

@sjrd I specifically mentioned Cross Build Hinderance (#533 (comment)) as a drawback.

It's a drawback worth considering for this SIP in particular because the added benefits are not as feature-based, and closer to the pro/con 50:50 point than, let's say SIP-14 Futures and Promises. Sure adding Future to the standard library would prevent cross building, but it does more things.

Another reason to think about this specifically for this SIP is that the surface area that might affect this SIP is potentially all Scala sources, which I don't think can be said of just adding a method to the standard library. Closer analogy might be SIP-18 Modularizing Language Features which might have added import language.implicitConversions etc many existing source. The potential surface area becomes relevant especially given the rise of auto formatting of Scala source using Scalariform or Scalafmt.

@sjrd
Copy link
Member

sjrd commented Sep 12, 2016

The anywhere-in-grammar but only-for-separate-lines variant seems very nice to me. Clearly an improvement wrt the initial proposal, IMO :) As @Ichoran put it, it definitely improves the benefit/cost ratio.

@xeno-by
Copy link
Contributor

xeno-by commented Sep 23, 2016

Hello everyone!

I am here to officially announce that SIP-27 Trailing Commas has been accepted by the SIP committee during the meeting on 20 Sep 2016.

According to the formal rules of the process (http://docs.scala-lang.org/sips/sip-submission.html), the committee proposes that compiler maintainers include this new language feature in a future release of the Scala compiler.

Moreover, I would like to ask @dwijnand to update the specification and implementation according to our discussions. Concretely, the committee has voted in favor of a proposal that allows trailing commas everywhere in the grammar given that the commas are immediately followed by a newline. @dwijnand: Please change the SIP and the prototype implementation to reflect that.

This ends the SIP process for the trailing commas proposal. I would like to thank @dwijnand for the thorough work on this language proposal and everyone for the insightful discussion. It is a pleasure to collaborate on building the future of the language together.

@jvican jvican merged commit eae494c into scala:master Sep 23, 2016
@dwijnand dwijnand deleted the sip-trailing-commas branch September 23, 2016 16:02
@the-Arioch
Copy link

With regard to "ease commenting out first/last element" maybe what could solve it would be generalizing of Unit as a terminator. It would be consistent with lists ending with ::Nil

Example

import
dir1.dir2.dir3.file4 ,
dir9.dir8.dir7.file6 ,
dirA.dirB.dirC.fileD ,
dirZ.dirY.dirX.fileW ,
();

Here you go - add, remove, (un)comment, reorder - the explicit stub-terminator would prevent compilation error.

@sjrd
Copy link
Member

sjrd commented Sep 24, 2016

@the-Arioch That doesn't generalize beyond imports. You can't do the same for actual parameters or for bindings, since an additional , () there is already valid and has another meaning.

@the-Arioch
Copy link

You can use sone other terminator as well, like lists using Nil or HNil

@sjrd
Copy link
Member

sjrd commented Sep 24, 2016

No, Nil and HNil suffer from the same issue, including for imports because they're just identifiers. You would need to find some piece of syntax that is currently never valid in any of the contexts where comma-separated lists exist.

The above proposal, which has been approved, uses a bare newline as this unique piece of syntax.

@adriaanm
Copy link
Contributor

adriaanm commented Feb 8, 2017

scala/scala#5245 was just merged, meaning this SIP's implementation will be available as of Scala 2.12.2 (due out by the end of the Feb)

bishabosha pushed a commit to bishabosha/docs.scala-lang that referenced this pull request Mar 21, 2022
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.