Linter: [flutter] (prefer) trailing_commas

[pq]

Following-up from https://github.com/flutter/flutter-intellij/issues/2980#issuecomment-451105228, we might consider a rule that enforces the flutter style guide suggestion:

Use a trailing comma for arguments, parameters, and list items, but only if they each have their own line.

Since a major motivation is to give analyzer something to hang quick-fixes from, we might consider enforcing just the first part and allowing dartfmt to handle line-breaks but maybe this would make the rule less useful for flutter repo committers.

@Hixie: any thoughts?

/cc @jaumard

[srawlins]

FYI I have a request for dartfmt --fix to add support for this too: https://github.com/dart-lang/dart_style/issues/753

[a14n]

I started a lint for this case and I think there should be at least 2 lints.

One to lint missing coma in the following cases (flutter code that don't use dartfmt)

void f(
  int p1,
  int p2
) {
  ....
}

An other one (really harder to do) that takes some list of params/args/elements and advices to add a trailing coma to make the formatted code (with ``dartfmt) looks better.

void f(int parameter1, int parameter2, int parameter3, int parameter4,
    int parameter5, int parameter6, int parameter7, int parameter8) {
  //
}

// after trailing coma

void f(
  int parameter1,
  int parameter2,
  int parameter3,
  int parameter4,
  int parameter5,
  int parameter6,
  int parameter7,
  int parameter8,
) {
  //
}

Another simpler lint could be to check trailing comas in widget trees.

[bwilkerson]

For the record, I'm not convinced that the value of a lint like this would justify the cost (cognitive load for users, maintenance cost for us). How many users would be helped by having this lint? (And for how long? Seems like after a couple of days most users would learn to add commas without needing a prompt.)

That said, writing the lint should be fairly trivial. For every list you want to check, get the last element, get it's end token, and look to see whether the next token is a comma. If you only want to check lists whose elements are already on separate lines (or whose first and last elements are on different lines, or whatever) then use the tokens to get offsets and use the LineInfo to map those offsets to line numbers and compare those.

[a14n]

That said, writing the lint should be fairly trivial. For every list you want to check, get the last element, get it's end token, and look to see whether the next token is a comma. If you only want to check lists whose elements are already on separate lines (or whose first and last elements are on different lines, or whatever) then use the tokens to get offsets and use the LineInfo to map those offsets to line numbers and compare those.

That's what I did :)

There are several problems.

In the flutter codebase (that don't use dartfmt) a missing trailing comma don't really have a impact until someone copy-paste code to put it in a place that will use dartfmt. Trees will suddently be flattened if a trailing comma is missing.

In a project that uses dartfmt the above rule to trigger the lint couldn't be used. As my second snippet shows, dartfmt formats f on two lines. No lint will be triggered here.

[Hixie]

Flutter would totally use this lint.

Seems like after a couple of days most users would learn to add commas without needing a prompt.

I've been writing code in this style for a couple of years and I still regularly omit the comma by mistake (then hate myself when I try to add another item to the list, and the compiler and analyzer lose their mind because they can't figure out what I did wrong).

[Hixie]

(Incidentally this is one case where using the formatter would make things ten times worse, because omitting the comma turns the entire list into a completely different style that is even less easy to add items to using copy and paste.)

[bwilkerson]

Flutter would totally use this lint.

Good to know, thanks.

In the flutter codebase (that don't use dartfmt) a missing trailing comma don't really have a impact until someone copy-paste code to put it in a place that will use dartfmt. Trees will suddently be flattened if a trailing comma is missing.

In a project that uses dartfmt the above rule to trigger the lint couldn't be used. As my second snippet shows, dartfmt formats f on two lines. No lint will be triggered here.

I think Flutter wants a rule that users should always include a comma at the end of parameter and argument lists. Is that true?

But I don't understand the non-flutter / dartfmt case. What is the rule you're proposing for that case?

[a14n]

I think Flutter wants a rule that users should always include a comma at the end of parameter and argument lists. Is that true?

(my feeling) 2 use-cases:

1) As a developer on Flutter codebase I don't use dartfmt so in trees of widget it's common to forget trailing commas. As example this code snippet is from flutter:

  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        backgroundColor: Theme.of(context).canvasColor,
        elevation: 0.0 // OOOOOOOOOOOOOOOOPS
      ),
      body: Column(
        crossAxisAlignment: CrossAxisAlignment.stretch,
        children: <Widget>[
          // Give the key-pad 3/5 of the vertical space and the display 2/5.
          Expanded(
            flex: 2,
            child: CalcDisplay(content: _expression.toString()) // OOOOOOOOOOOOOOOOPS
          ),
          const Divider(height: 1.0),
          Expanded(
            flex: 3,
            child: KeyPad(calcState: this) // OOOOOOOOOOOOOOOOPS
          ),
        ] // OOOOOOOOOOOOOOOOPS
      ) // OOOOOOOOOOOOOOOOPS
    );
  }

As you may have notice some trailing commas are missing. So when someone add an argument or a element in a list he could face errors described above by @Hixie. An other issue is that if someone copy/paste this code in a project that use dartfmt the widget tree will be flattened to:

  Widget build(BuildContext context) {
    return Scaffold(
        appBar: AppBar(
            backgroundColor: Theme.of(context).canvasColor, elevation: 0.0),
        body: Column(
            crossAxisAlignment: CrossAxisAlignment.stretch,
            children: <Widget>[
              // Give the key-pad 3/5 of the vertical space and the display 2/5.
              Expanded(
                  flex: 2, child: CalcDisplay(content: _expression.toString())),
              const Divider(height: 1.0),
              Expanded(flex: 3, child: KeyPad(calcState: this))
            ]));
  }

Here a good lint (for developers on flutter) would be to require a trailling comma if the closing parenthesis or bracket starts the line. (that's what I do in a lint I have locally - the 6224 fixes can be seen in https://github.com/a14n/flutter/commit/08f7785e649104293acbb28c179cdbf38fc32d91)

2) As a Flutter user that uses dartfmt on my project, I'd like to have a lint to make the above formatted code in something that looks better (like a tree). Here we could not have the same logic as for the lint above. Perhaps we could check if an Object created is a Widget and require its arguments to have a trailing commas. We could also add a @tree (or something like that) on classes (like Widget) to drive the lint rule when to check for trailing commas.

[bwilkerson]

Is this a fair summary?

• In hand-formatted (Flutter) code you're using the formatting as a signal as to when a comma was intended but forgotten.

• In auto-formatted code we're not sure what the right signal is (because the intent has been lost).

It would be great to find a shared signal so that we don't need two rules, so let me ask a couple of questions.

Why are some widgets (such as AppBar and Expanded) not formatted on multiple lines? (My guess, having not looked at a lot of Flutter code) is that the widgets with children are formatted differently than widgets without children.)

Does the style of formatting arguments on multiple lines apply to anything other than widgets?

[Hixie]

I think Flutter wants a rule that users should always include a comma at the end of parameter and argument lists. Is that true?

Here are some examples where you wouldn't want a trailing comma:

  void foo(String bar) {
  }

  var bar = [1, 2, 3];

  quux(bar, baz);

Here are some where you would:

  void foo(
    String bar,
  ) {
  }

  var bar = [
    1,
    2,
    3
  ];

  quux(
    bar,
    baz
  );

[a14n]

Why are some widgets (such as AppBar and Expanded) not formatted on multiple lines?

My above sample (without commas for AppBar and Expanded) was code from a sample that should have used trailing commas. There are just typo that need to be fixed. So AppBar and Expanded are not exceptions.

Regarding the samples @Hixie put in its last comment. It's what I described as the logic for the first rule and what I already have implemented locally.

The second case is not only specific to Flutter. If you look at dart-lang/dart_style#753 it's related to angular. That's why I mentioned a hypothetical @tree.

[zanderso]

@a14n If you already have a something drawn up for use case (1), I'd really like to use it. Is it possible to factor the two lints you have in mind so they can be discussed/landed separately?

[a14n]

@zanderso I just pushed https://github.com/a14n/linter/commit/39c59d6ae4a8a59055024002f1ba4840292e1b5e available in https://github.com/a14n/linter/tree/prefer_trailing_commas branch

I run the lint on flutter codebase (only on packages/flutter) and there are 410 occurances.

[a14n]

FWIW I think prefer_trailing_commas should live in a Flutter analyzer plugin beside all kind of formatting rules that could enforce the Flutter Style Guide (until a flutter formatting tool arises). Unfortunatelly it is blocked by flutter/flutter#28327

[cah4a]

I think it could be very useful!
But I think lint should depend on line length. If line fits well inside one line it has to be a one-liner.

Bad:

void foo(
    String bar,
) {
   print(
      bar,
   );

   make(
      Foo(
          a: 1,
          b: 1,
          c: 1,
          d: 1,
          e: 1,
          g: 1,
      ),
   );
}

void bar(int parameter1, int parameter2, int parameter3, int parameter4,
    int parameter5, int parameter6, int parameter7, int parameter8) {
  fn(parameter1, parameter2, parameter3, parameter4, parameter5, parameter6,
    parameter7, parameter8);
}

Good:

void foo(String bar) {
  print(bar);

  make(Foo(
    a: 1,
    b: 1,
    c: 1,
    d: 1,
    e: 1,
    g: 1,
  ));
}

void bar(
  int parameter1,
  int parameter2,
  int parameter3,
  int parameter4,
  int parameter5,
  int parameter6,
  int parameter7,
  int parameter8,
) {
  print([
    parameter1,
    parameter2,
    parameter3,
    parameter4,
    parameter5,
    parameter6,
    parameter7,
    parameter8,
  ]);
}

[nepaul]

FWIW I think prefer_trailing_commas should live in a Flutter analyzer plugin beside all kind of formatting rules that could enforce the Flutter Style Guide (until a flutter formatting tool arises). Unfortunatelly it is blocked by flutter/flutter#28327

Yep! 👍

[SeasonLeee]

It's September 18, 2020... almost a year since this commit opened, any update?

[Galti]

+

[incendial]

If anyone is still interested in this rule we have implemented it in https://github.com/wrike/dart-code-metrics (along with other rules that can be useful). Any feedback is welcome.

[lauweijie]

I've written a lint for this and have reproduced the test case below for discussion.

Use trailing commas for all function calls and definitions.

Exception: If the function call or definition, from the start of the function name up to the closing parenthesis, fits in a single line.

Exception: If the final parameter is positional (vs named) and either a function literal implemented using curly braces, a literal map, a literal set or a literal array. This exception only applies if the final parameter does not fit entirely on one line.

class RequireTrailingCommaExample {
  RequireTrailingCommaExample.constructor1(Object param1, Object param2);

  RequireTrailingCommaExample.constructor2(
    Object param1,
    Object param2,
    Object param3,
  );

  RequireTrailingCommaExample.constructor3(
      Object param1, Object param2, Object param3); // LINT

  RequireTrailingCommaExample.constructor4(
    Object param1,
    Object param2, [
    Object param3 = const [
      'test',
    ],
  ]);

  RequireTrailingCommaExample.constructor5(Object param1, Object param2,
      [Object param3 = const [
        'test',
      ]]); // LINT

  void method1(Object param1, Object param2, {Object param3, Object param4}) {}

  void method2(
    Object param1,
    Object param2,
    Object param3,
    Object param4,
    Object param5,
  ) {}

  void method3(
    Object param1,
    Object param2, {
    Object param3,
    Object param4,
    Object param5,
  }) {}

  void method4(Object param1, Object param2, Object param3, Object param4,
      Object param5) {} // LINT

  void method5(Object param1, Object param2,
      {Object param3, Object param4, Object param5}) {} // LINT

  void method6(Object param1, Object param2,
      {Object param3,
      Object param4,
      Object param5,
      Object param6,
      Object param7}) {} // LINT

  void run() {
    void test(Object param1, Object param2, {Object param3}) {}

    test('fits on one line, no need trailing comma', 'test');

    test(
      'does not fit on one line, requires trailing comma',
      'test test test test test',
    );

    test('does not fit on one line, requires trailing comma',
        'test test test test test'); // LINT

    test('test', () {
      // Function literal implemented using curly braces.
    });

    test('test', () {
      // Function literal implemented using curly braces.
    }, param3: 'test'); // LINT

    test(
      'test',
      () {
        // Function literal implemented using curly braces.
      },
      param3: 'test',
    );

    test('test', 'test', param3: () {
      // Function literal implemented using curly braces.
    }); // LINT

    test(
      'test',
      'test',
      param3: () {
        // Function literal implemented using curly braces.
      },
    );

    test(
      () {
        // Function literal implemented using curly braces.
      },
      'test',
    );

    test(() {
      // Function literal implemented using curly braces.
    }, 'test'); // LINT

    test('map literal', {
      'one': 'test',
      'two': 'test',
    });

    test({
      'one': 'test',
      'two': 'test',
    }, 'map literal'); // LINT

    test('set literal', {
      'one',
      'two',
    });

    test({
      'one',
      'two',
    }, 'set literal'); // LINT

    test('list literal', [
      'one',
      'two',
    ]);

    test([
      'one',
      'two',
    ], 'list literal'); // LINT

    test(
      'no exception for set literal as it fits entirely on 1 line',
      const {'one', 'two', 'three'},
    );

    test('no exception for set literal as it fits entirely on 1 line',
        const {'one', 'two', 'three'}); // LINT

    test('exception for set literal as it spans multiple lines', const {
      'one',
      'two',
      'three',
    });

    test('exception for set literal as it spans multiple lines', const <
        AnExtremelyLongClassNameOneTwoThreeFourFiveSixSevenEightNineTen>{});

    test(
      'no exception for array literal as it fits entirely on 1 line',
      const ['one', 'two', 'three'],
    );

    test('no exception for array literal as it fits entirely on 1 line',
        const ['one', 'two', 'three']); // LINT

    test('exception for array literal as it spans multiple lines', const [
      'one',
      'two',
      'three',
    ]);

    test('exception for array literal as it spans multiple lines', const <
        AnExtremelyLongClassNameOneTwoThreeFourFiveSixSevenEightNineTen>[]);

    test(
      'no exception for map literal as it fits entirely on 1 line',
      const {'one': '1', 'two': '2', 'three': '3'},
    );

    test('no exception for map literal as it fits entirely on 1 line',
        const {'one': '1', 'two': '2', 'three': '3'}); // LINT

    test('exception for map literal as it spans multiple lines', const {
      'one': '1',
      'two': '2',
      'three': '3',
    });

    test('exception for map literal as it spans multiple lines', const <String,
        AnExtremelyLongClassNameOneTwoThreeFourFiveSixSevenEightNineTen>{});

    test(
      'no exception for function literal as it fits entirely on 1 line',
      () {},
    );

    test('no exception for function literal as it fits entirely on 1 line',
        () {}); // LINT

    assert(true);

    assert('a very very very very very very very very very long string'
        .isNotEmpty); // LINT

    assert(
      'a very very very very very very very very very long string'.isNotEmpty,
    );

    assert(false, 'a short string');

    assert(false,
        'a very very very very very very very very very long string'); // LINT

    assert(
      false,
      'a very very very very very very very very very long string',
    );
  }
}

class AnExtremelyLongClassNameOneTwoThreeFourFiveSixSevenEightNineTen {}