Add support for optional trailing commas in type argument and type parameter lists.

modulovalue commented 2 years ago

This issue is a feature request for allowing optional trailing commas in non-empty type argument lists and type parameter lists. The proposed change is purely syntactical and non-breaking.

Motivation

The dart formatter uses optional trailing commas in formal argument/parameter lists to guide the heuristics that it uses to produce its output. A trailing comma gives the dart formatter a signal that the members of the affected list should be indented and output on a new line (roughly). Optional trailing commas in type argument/parameter lists would allow the dart formatter to apply the same behavior that it does for formal argument/parameter lists to type argument/parameter lists.

https://github.com/dart-lang/language/issues/524 will increase the syntactical overhead of declaring type parameters. I believe that optional trailing commas with support from the dart formatter will make that syntactic overhead more manageable.

Example

The proposed change would make the following program a valid Dart program:

void foo<
  T,
  U extends Object,
  V extends MapEntry<
    int,
    int,
  >,
>() {
  foo<
    int,
    int,
    MapEntry<
      int,
      int,
    >
  >();
}

Grammar changes

The dart grammar would change from:

typeParameters
    :    '<' typeParameter (',' typeParameter)* '>'
    ;

typeArguments
    :    '<' type (',' type)* '>'
    ;

to

typeParameters
    :    '<' typeParameter (',' typeParameter)* ','? '>'
    ;

typeArguments
    :    '<' type (',' type)* ','? '>'
    ;

Related issue:

https://github.com/dart-lang/language/issues/1402 proposes a different policy. This is about type argument lists and type parameter lists only.

Edit:

An alternative way to handle this issue could be to make commas entirely optional see: https://github.com/elm/compiler/issues/979

A discussion about optional commas that includes opposing views can be found here: https://github.com/elm-lang/elm-plans/issues/2

munificent commented 2 years ago

While I'm not generally a huge fan of trailing commas, I agree completely that if we support them, we should support them uniformly. Heck, we already even allow them in assert() statements, so we should definitely allow them in type parameter and type argument lists.

modulovalue commented 2 years ago

While I'm not generally a huge fan of trailing commas [...]

Why? Is it personal preference or is there some non-obvious reason why you have that opinion?

lrhn commented 2 years ago

My personal preference is that the trailing comma has no semantic meaning, so it's effectively dead code. Your source should not contain unnecessary or dead code.

The comma does allow you to more easily remove or shuffle elements of the comma-separated sequence without having to special-case the last one, at least when the sequence is formatted with one element per line. That's convenient. But it's convenience that comes at a cost to the author, you have to add an otherwise meaningless comma. It reveals a flaw in the underlying grammar design, one that every separated list suffers from. (Similarly to the C/Pascal debacle where C uses ; as a terminator, Pascal uses it as a separator. I'd say C won that popularity context, so maybe trailing commas should be mandatory instead? Or maybe Lisp just got it right first, and everybody since has bungled how to do lists.)

Then there is the hack of making dartfmt recognize the comma and putting the elements on a line of their own. Which makes sense, because that's the only situation where the comma has value anyway. That makes the comma meta-semantically significant. It makes a difference on how a tool behaves on the source code, so now it's not really optional any more. It's not source code, it's a meta-annotation intended for dartfmt. But it looks like source code.

But dartfmt is opinionated and doesn't otherwise let you control how it formats the program. The trailing comma breaks that. You can add or remove the, otherwise irrelevant, comma in order to control layout of your program, when formatted by a program which exists solely to prevent you from having to worry about formatting. You can say that trailing commas being used is proof that dartfmt's mission has failed. (Maybe people just want to control some parts of their source layout, because they sometimes know what's best for readability.)

But, as @munificent said, we've gone down this road, we should take it to its conclusion and allow trailing commas in all comma-separated syntactic constructs. The ones we are lacking today are:

Type parameters
Type arguments
Declarations with multiple variables (var x = 1, y = 2, ;, which looks odd because of the ,;, but formatted like y = 2,\n; it might be fine. Or we should just drop that syntax entirely in Dart 3.0.)
catch clauses (catch (e, s,), analyzer at least, seems like dart2js accepts it. Nobody will ever need the comma there, because you don't want to spread two short words on multiple lines).

(I thought we might have missed for-loop increment expressions (for (;; x++, z++,);) too, but seems those are accepted generally.)

srawlins commented 2 years ago

Also initializer lists, with-clauses, implements-clauses, and on-clauses.

Edit: and show-combinators and hide-combinators.

lrhn commented 2 years ago

The show and hide combinators actually can't (easily) allow trailing commas because show and hide are valid identifers:

import "foo.dart" show show, hide hide baz;

is currently valid, so we'll have to use look-ahead to see whether the , after show is a trailing comma or an internal comma. Not impossible (but nigh impossible to read!).

With clauses and implements clauses have similar readability issues, but since implements and { are not valid in the preceding list, it's not as locally ambiguous.

Initializer lists are fair game.

modulovalue commented 2 years ago

Thank you Lasse for the more detailed analysis of both styles.

But, as munificent said, we've gone down this road, we should take it to its conclusion and allow trailing commas in all comma-separated syntactic constructs. The ones we are lacking today are: [...]

I want to express the opinion that I don't think that is a good idea because it sounds like consistency for consistency's sake. (Wouldn't trailing semicolons have to be made optional as well for full consistency? where should the line be drawn?)

This issue is motivated by the syntactical overhead that variance modifiers will add, and I think that optional trailing commas with the support from the dart formatter will make them more digestible.

Furthermore, this is a more abstract argument, but since programming languages seem to evolve towards more expressive type systems, terms and types will eventually have to converge (see: https://en.m.wikipedia.org/wiki/Lambda_cube) Therefore, I'd argue that initializer lists, with/implements clauses, and others aren't even related to this issue since they deal with an unrelated subset of the language and should be dealt with separately, but making terms i.e. expressions and types behave in a way that brings them closer together seems to me to be a step forward.

lrhn commented 2 years ago

I can see fixing this particular case without caring about other cases. I'd also be fine with fixing initializer lists at the same time, because they do have similar issues with long lists of verbose expressions. The rest are ... not as important, I agree.

modulovalue commented 2 years ago

I'd also be fine with fixing initializer lists at the same time

Here is an example of what that could look like:

class Foo {
  final int i;
  final int j;
  final int k;

  Foo.a({
    required int x,
  }) :
    i = 0,
    j = 1,
    k = 3,
  ;

  Foo.b() :
    i = 0,
    j = 1,
    k = 3,
  ;

  Foo.c() :
    i = 0,
    j = 1,
    k = 3, {
    //
  }

  Foo.d() :
    i = 0,
    j = 1,
    k = 3,
  {
    //
  }
}

I think Foo.a and Foo.b are an improvement as they would produce cleaner diffs and look more like other more familiar formatting rules. I don't know about Foo.c vs. Foo.d.

munificent commented 2 years ago

Is it personal preference or is there some non-obvious reason why you have that opinion?

Like Lasse said, I don't generally like having two ways to say the same thing where the difference isn't really useful. Just seems like a mostly needless bit of syntactic sugar.

In your examples, I think Foo.a and Foo.b don't look great. The whole point of allowing ; as a constructor body is to not take up space when there's no body. Moving it to the next line nullifies that. But putting it immediately after the trailing , on the same line defeats the purpose of the , and looks really ugly.

Likewise, Foo.c defeats the purpose of the trailing comma. I think Foo.d` looks... OK, but not great.

Personally, I would suggest we don't support trailing commas in initializer lists or show/hide clauses. I think it's reasonable to allow trailing commas when the commas-separated list is in some bracket-delimited construct. But it gets a lot weirder when the list is terminated by ; or something else. (I realize that we do allow trailing commas before the ; in enhanced enums but... that also doesn't look good and is, I think, mostly for compatibility with non-enhanced enums.)

Even if we do allow trailing commas in these places, I'd want a lint that asks you to remove them if they are immediately before a ; since it just looks wrong regardless of where you put the ;.

eernstg commented 9 months ago

@munificent, does the new kind of formatting cause trailing commas to be less relevant (perhaps even useless) in the situations mentioned above?:

formal type parameter lists
actual type argument lists
declarations with multiple variables
catch clauses (catch (e, s,))
initializer lists
type lists in a with, implements, or on clause

lrhn commented 9 months ago

For the record, I have found myself writing enums with both , and ;:

enum Foo {
   something(Multiple, "arguments", here),
   other(Multiple, "arguments", here),
   more(Multiple, "arguments", here),
   ;
   // ...
}

I'm doing for the exact same reason as everywhere else: So that all the similar lines have the same syntax, and I can add, remove or reorder with having to change any ; to , or , to ;.

Same reasoning applies to long lists of anything. I don't like the commas, but they serve a practical purpose. If something is comma separated on multiple lines, the last line having to be different is an annoyance.

(I'd allow trailing commas everywhere we have a comma separated list, and have the formatter automatically add or remove the last comma depending on whether it orders things as one-per-line or not. The one thing I don't want is having to choose, or manually having to add or remove it. It might not be practical for show/hide lists, but I'm still willing to only allow one of them in each declaration. And remove multi-variable-declarations entirely, no matter how much it would impact code golfing.)

modulovalue commented 9 months ago

and I can add, remove or reorder with having to change any ; to , or , to ; [...] but they serve a practical purpose.

Here are 3 UX improvements that become available with optional trailing commas:

(always be able to easily) reorder with keyboard shortcuts

https://github.com/dart-lang/language/assets/24444584/d4f17e2a-6eeb-4a87-91c9-22e2ca8832f7

(always be able to easily) reorder with the mouse

https://github.com/dart-lang/language/assets/24444584/8bbeaeca-7e5f-4a2c-aeb8-b0b8dd6c2dec

(always be able to easily) swap elements between different lists

https://github.com/dart-lang/language/assets/24444584/13a86110-2407-44e2-9c97-d1c8d823950c

[...] and have the formatter automatically add or remove the last comma depending on whether it orders things as one-per-line or not

(See: https://github.com/dart-lang/dart_style/issues/1253#issuecomment-1689764531) I don't think that it would always be helpful if the formatter had to ability to decide to remove optional trailing commas. I think that interpreting the optional trailing comma as a clear signal to the formatter that "I need things to be on different lines" would be more helpful. This approach wouldn't be able to come in the way of the UX improvements that I've pointed out above.

does the new kind of formatting cause trailing commas to be less relevant (perhaps even useless)

For the reasons mentioned above, I don't think that any changes to the formatter could entirely replace any optional trailing comma on the grammar level.

munificent commented 8 months ago

@munificent, does the new kind of formatting cause trailing commas to be less relevant (perhaps even useless) in the situations mentioned above?:

With the new style, the formatter will add and remove trailing commas on your behalf. Whenever the formatter splits a construct that supports trailing commas across multiple lines, it will add a trailing comma. If the constructor is formatted onto a single line, it removes the trailing comma. (There are a couple of edge cases that won't go into here.)

So, in general, the new style makes trailing commas more common because they'll appear whenever an argument list, parameter list, collection literal, etc. splits. But it makes them less important because as a user, you don't have to think about whether to add or remove them.

formal type parameter lists

actual type argument lists

Yes, it will add and remove trailing commas here.

declarations with multiple variables

I don't think these do support trailing commas. If so, it will discard them.

catch clauses (catch (e, s,))

Today I learned that catches support trailing commas. Filed https://github.com/dart-lang/dart_style/issues/1424. Currently the formatter fails with a parse error, which is weird since it uses the same parser as the VM, which happily runs it.

initializer lists

Those don't support trailing commas, and I don't think should.

type lists in a with, implements, or on clause

Likewise, those also don't support trailing commas.

I really don't want us to support trailing commas in places like with and show where there isn't a closing bracket. If we allow trailing commas there, it will get trickier if we ever want to do optional semicolons.

lrhn commented 8 months ago

I would actually want trailing commas in initializer lists, after a final initializer, but not after a final super-constructor invocation. Because it allows reordering initializers. (Except that we put the : on the first initializer's line, we should put it in the preceding like when formatting initializers on multiple lines.)

munificent commented 8 months ago

I would actually want trailing commas in initializer lists, after a final initializer, but not after a final super-constructor invocation.

I mean... if there's a final super-constructor invocation after it, then those preceding initializers don't have trailing commas. They just have regular old separating commas, and those are most certainly required. :)

lrhn commented 7 months ago

Exactly. I want a comma after a final initializer, one that doesn't have a super constructor invocation after it

munificent commented 7 months ago

I want a comma after a final initializer, one that doesn't have a super constructor invocation after it

Nooooo.

I'm not aware of any widely used style for constructor initializers that doesn't put the subsequent { or ; on the same line as the final initializer, so this wouldn't buy you anything. If we did allow trailing commas here, the formatter would probably just remove them anyway because that would be the idiomatic style.

lrhn commented 7 months ago

Wouldn't it be great if the editor's "move line" was delimiter aware, so that moving a line that ends in ; up would swap the two line contents, but keep the delimiters?

Until then, some people will want every line to end in a comma, even if that puts a semicolon on the next line.

munificent commented 7 months ago

some people will want every line to end in a comma, even if that puts a semicolon on the next line.

For what it's worth, it was the Flutter team that pushed hardest for supporting trailing commas in parameter and argument lists and the hand-formatted Flutter repo does not use a style for constructor initializers that would benefit from trailing commas. They always put the { or ; on the same line as the last initializer.

It's possible that they would move the { or ; to the next line if constructor initializers did support trailing commas, but given that they don't move { or ; to a subsequent line anywhere else, I suspect they wouldn't. I really think it's not worth supporting trailing commas on constructor initializers.

Or with, hide, and show clauses. I only want to support them for syntax where there is an explicit closing delimiter bracket like [, (, or {. Enum values are an annoying special case because there is an explicit closing bracket after the last value if the enum does not have members, but there's a ; after the last value if it does. Currently, the new formatting style will remove a trailing comma on a list of enum values if there are members because I think that looks best.

dart-lang / language