[flutter] (prefer) trailing_commas

[pq]

Following-up from flutter/flutter-intellij#2980 (comment), 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: dart-lang/dart_style#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 a14n/flutter@08f7785)

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 a14n@39c59d6 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! 👍

[renntbenrennt]

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 {}

[jackforesightmobile]

How can I use this rule in my analysis_options.yaml file? It doesn't seem to be recognised as a rule as it's unreleased here.

Is this rule not available to be used yet or am I missing something?

[TheGlorySaint]

@jackforesightmobile Hope you found your solution, if not

in VSCode with Flutter 2.5.3 and Dart 2.14.4
in your analysis_options.yaml add
require_trailing_commas: true
then save the file.

[renntbenrennt]

@TheGlorySaint Hi, is this lint only work at VSCode? I am using Android Studio and I am also having this issue where I can't seem to have this lint working in my project, meaning there's no warning report to code like this:

body: const Center(child: Text('Hello World')),

Is there anything else I have to do to have this lint working? Or maybe it's because this PR is being blocked that I can't have this lint working as expected?

And here's `my flutter doctor -v`

[✓] Flutter (Channel stable, 2.5.3, on macOS 11.6 20G165 darwin-x64, locale
    en-US)
    • Flutter version 2.5.3 at /Users/benjaminli/software_dev/sdks/flutter
    • Upstream repository https://github.com/flutter/flutter.git
    • Framework revision 18116933e7 (7 weeks ago), 2021-10-15 10:46:35 -0700
    • Engine revision d3ea636dc5
    • Dart version 2.14.4

[✓] Android toolchain - develop for Android devices (Android SDK version 30.0.2)
    • Android SDK at /Users/benjaminli/Library/Android/Sdk
    • Platform android-31, build-tools 30.0.2
    • ANDROID_HOME = /Users/benjaminli/Library/Android/Sdk
    • Java binary at: /Applications/Android Studio.app/Contents/jre/Contents/Home/bin/java
    • Java version OpenJDK Runtime Environment (build 11.0.10+0-b96-7281165)
    • All Android licenses accepted.

[✓] Xcode - develop for iOS and macOS
    • Xcode at /Applications/Xcode.app/Contents/Developer
    • Xcode 13.1, Build version 13A1030d
    • CocoaPods version 1.10.1

[✓] Chrome - develop for the web
    • Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome

[✓] Android Studio (version 2020.3)
    • Android Studio at /Applications/Android Studio.app/Contents
    • Flutter plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/9212-flutter
    • Dart plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/6351-dart
    • Java version OpenJDK Runtime Environment (build 11.0.10+0-b96-7281165)

[✓] VS Code (version 1.62.3)
    • VS Code at /Applications/Visual Studio Code.app/Contents
    • Flutter extension version 3.29.0

[✓] Proxy Configuration
    • HTTP_PROXY is set
    • NO_PROXY is localhost,127.0.0.1,LOCALHOST
    • NO_PROXY contains 127.0.0.1
    • NO_PROXY contains localhost

[✓] Connected device (2 available)
    • macOS (desktop) • macos  • darwin-x64     • macOS 11.6 20G165 darwin-x64
    • Chrome (web)    • chrome • web-javascript • Google Chrome 96.0.4664.55

• No issues found!

And yes, I have turn on the lint in my analysis_options.yaml file.

I just can't have it... help....

[TheGlorySaint]

@SeasonLeee Did you added

flutter_lints: ^1.0.4 to the DEV_Dependencies?
With your example I do not get the Trailing Comma annotation.

Please try it with Following(longer) Code

  Widget build(BuildContext context) {
    return Scaffold(
        appBar: AppBar(
          title: const Text(
            'E-Mail Verifizieren',
          ),
        ),
        body: Column(
          crossAxisAlignment: CrossAxisAlignment.center,
          children: const [
            Center(
              child: Text(
                'Damit du mit deiner Registrierung weiter machen kann, musst du deine E-Mail-Adresse verifizieren. Dazu wurde an deine angegebene E-Mail-Adresse ein Link mit Anweisungen geschickt.',
              ),
            ),
          ],
        ));//Here should be the Trailing Comma
  }

[renntbenrennt]

@TheGlorySaint

Thank you so much for your advice on that package version there. I bumped the version to 1.0.4 and now I get the pertinent warning but it's not the way I expected, which I will report to that pr

So, now I get the warning only if the comma is missing like this:

But I thought it should have waring when there's double right round bracket like this:

hmm... I'm not sure if it's by designed or anything... But I will raise this doubt to that PR anyway, thanks again!😄

[TheGlorySaint]

@SeasonLeee Your Welcome, that my Answer helped you to get it running.

How I understood the Trailing Comma is working, it is the length of the Line and a few "Logical" Commas like your first Screenshot.

But at the second screenshot the length is fine(for the Trailing Comma) but logically there should be another Trailing Comma after the Text Widget String and I think the Logic isn't far or good enough to Support that feature for that.

[kuhnroyal]

But at the second screenshot the length is fine(for the Trailing Comma) but logically there should be another Trailing Comma after the Text Widget String and I think the Logic isn't far or good enough to Support that feature for that.

The 2nd condition is that there is only one parameter. Both screens look correct.

[renntbenrennt]

😅 okay, after seeing what you guys talking about the triggering logic, I did a test. And now I think we may have some linter triggering issue😂, check this out:

@TheGlorySaint @kuhnroyal

[kuhnroyal]

Then the condition is one named parameter, didn't remember exactly :)