Add require_trailing_comma lint rule.

[lauweijie]

Add require_trailing_comma lint rule.

DO use trailing commas for all function calls and definitions unless the function call or definition, from the start of the function name up to the closing parenthesis, fits in a single line.

GOOD:

void run() {
  method(
    'does not fit on one line',
    'test test test test test test test test test test test',
  );
}

BAD:

void run() {
  method('does not fit on one line',
      'test test test test test test test test test test test');
}

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.

Fixes #1338

[coveralls]

Coverage increased (+0.01%) to 94.29% when pulling 0a7a68b on lauweijie:trailing-comma into 983cbf8 on dart-lang:master.

[srawlins]

Heads up: I suspect this part will be controversial. I don't have a strong opinion yet, but I suspect for many people, requiring a trailing comma for a single parameter/argument (on a long line) may be a bridge too far.

[bwilkerson]

Overall the implementation looks good.

I didn't address any questions about whether this implements the semantics we want, only whether the semantics this implements were implemented correctly.

I do have a larger concern. This implementation depends on having first run the formatter over the code. This has some negative consequences.

1. Sometimes users won't be told about missing commas until after they format the code. This could lead to a workflow in which users need to format their code, add commas, and then format again, which is less than ideal.
2. Sometimes users will be told that a comma is missing even if the formatter could put the whole invocation on a single line, which could lead to unnecessary commas.
3. Users will never be told when they could remove a trailing comma (such as when one or more of the arguments are extracted into local variables).

Removing that requirement would be very difficult, so I'm not sure the best way to proceed from here.

[lauweijie]

Thanks for the review, @bwilkerson and @srawlins. I've addressed most comments.

Sometimes users won't be told about missing commas until after they format the code. This could lead to a workflow in which users need to format their code, add commas, and then format again, which is less than ideal.

You're right, this assumes dartfmt has been run over the code, which could indeed be less than ideal. As you mentioned, figuring out when to require a trailing comma without dartfmt is tricky. Suggestions welcome!

Users will never be told when they could remove a trailing comma (such as when one or more of the arguments are extracted into local variables).

True, this lint does not enforce the reverse when everything fits on a single line.
Perhaps that could be a separate lint: avoid_trailing_comma?

@pq, any thoughts?

[pq]

You're right, this assumes dartfmt has been run over the code

I agree that this is awkward (and complicates quick fixes) but I'm not sure it's a show-stopper. It would definitely be good to capture this in the rule documentation somehow.

[pq]

Perhaps that could be a separate lint: avoid_trailing_comma?

This is an interesting idea. Maybe unnecessary_trailing_commas would be a good partner lint?

[bwilkerson]

Perhaps that could be a separate lint: avoid_trailing_comma?

My perspective is that this is a rule designed to enforce a particular style concerning when (and when not) to use a trailing comma in parameter and argument lists. I think it would be much better for it to enforce the whole style than for it to require a companion rule to enforce some portion of the style.

And from that perspective, perhaps we should rename the lint to indicate that it is (or could be) more encompassing (though I don't have a good suggestion off the top of my head).

Also, I'm concerned that require_trailing_commas and avoid_trailing_commas sound like opposites, and hence appear to be mutually exclusive.

As you mentioned, figuring out when to require a trailing comma without dartfmt is tricky.

It would be prohibitively expensive, but dart_style does provide a programatic interface to invoke the formatter, so theoretically we could format the content first, then parse it, figure out where commas should have been, and then map those back to the current source code.

We might also be able to do some close-enough computation of the length of various code snippets to guess at whether multiple lines are required. For example, given an invocation like

method(
  a,
  b,
  c,
);

we could add the lengths of the individual argument, plus commas and spaces, and determine that this would all fit on one line if the trailing comma were removed. I'm not sure what kind of nasty corner cases we might run into, but it might be worth exploring at some point.

[lauweijie]

We might also be able to do some close-enough computation of the length of various code snippets to guess at whether multiple lines are required. For example, given an invocation like

method(
  a,
  b,
  c,
);

we could add the lengths of the individual argument, plus commas and spaces, and determine that this would all fit on one line if the trailing comma were removed. I'm not sure what kind of nasty corner cases we might run into, but it might be worth exploring at some point.

Removing trailing commas by simply counting the length of each individual argument could be tricky due to corner cases like this:

// Comment prevents everything from fitting on 1 line.
method(
  a, // comment
  b,
);

For this case, I should be able to easily detect that the token spans multiple lines and skip:

// Function literal that has few characters but prevents everything from being on 1 line.
method(
  a,
  () {
    // doSomething
  },
);

If we do count chars, we also have to remember to count in the named params:

method(
  aVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryExtremelyLongNamedParameter: a,
  b,
);

My thought: if we can get this to a good state that's agreeable by most, shall we move forward with what we have currently and propose further improvements later?

[bwilkerson]

if we can get this to a good state that's agreeable by most, shall we move forward with what we have currently and propose further improvements later?

Yes, I'm fine with that. If the changes we're contemplating would be breaking, then we might want to mark it as experimental in the meantime, but other than that I'm always in favor of making incremental progress on a feature.

[bwilkerson]

Consider adding "and might produce false positives until that's happened" to make the implications more clear.

[lauweijie]

Updated, although I feel that this could be a given.

The assumption that dartfmt has been run is not unique to this lint rule.

curly_braces_in_flow_control_structures also has similar assumptions but is undocumented.

[lauweijie]

Yes, I'm fine with that. If the changes we're contemplating would be breaking, then we might want to mark it as experimental in the meantime, but other than that I'm always in favor of making incremental progress on a feature.

Sounds good! What are the implications of a rule being marked experimental? Does that mean it's unenforced by default but can be manually turned on?

[bwilkerson]

What are the implications of a rule being marked experimental?

It's basically just a signal to the community that we reserve the right to change its behavior. With lints that aren't marked as being experimental we're very careful to consider the possible impact on clients (such as new lints being generated) before making changes to them.

Does that mean it's unenforced by default but can be manually turned on?

All lints are disabled by default and need to be explicitly enabled in the analysis options file. So yes, but not because it's marked experimental.

[lauweijie]

@bwilkerson Marked the lint as experimental, PTAL

[a14n]

Only 7211 lints found on flutter package 😆 🥵

Some random samples that might be discutable / need an exception:

• single argument (that I excluded in my implementation of this lint 18 month ago)

  print('''
    --help      Print a usage message.
    --temp-dir  A location where temporary files may be written. Defaults to a
                directory in the system temp folder. If a temp_dir is not
                specified, then the default temp_dir will be created, used, and
                removed automatically.
    ''');

    await tester.pumpWidget(Wrap(
      alignment: WrapAlignment.spaceAround,
      spacing: 5.0,
      textDirection: TextDirection.rtl,
      children: const <Widget>[
        SizedBox(width: 100.0, height: 10.0),
        SizedBox(width: 200.0, height: 20.0),
        SizedBox(width: 310.0, height: 30.0),
      ],
    ));

    assert(() {
      _debugIsRenderViewInitialized = true;
      return true;
    }());

• test/map/closure pattern

    testWidgets('debugIsLocalCreationLocation test', (WidgetTester tester) async {
      // several statements
    }, skip: !WidgetInspectorService.instance.isWidgetCreationTracked()); // Test requires --track-widget-creation flag.

        Timeline.instantSync('ImageCache.evict', arguments: <String, dynamic>{
          'type': 'keepAlive',
          'sizeInBytes': image.sizeBytes,
        });

[lauweijie]

Only 7211 lints found on flutter package 😆 🥵

I've written a tool to automate most of the fixes and will be happy to help with the effort to apply this to the flutter package.

single argument

I'm of the thought that single argument calls shouldn't be given this exception if it spans more than one line. Of course, open to discussion and would like to hear from the community.

test/map/closure pattern

The lint rule already excepts the trailing comma if the last arg is a function literal or literal set/map/array. Exemption does not apply if the last param is named rather than positional.

Again, open to discussion, but I think keeping the rule simple while covering most cases should be the goal.

[bwilkerson]

I've written a tool to automate most of the fixes ...

Ideally we should add a fix in the analysis server for this lint and make it possible to apply the fix through dart fix.

[a14n]

Regarding the lints reported on flutter I would recommand that the lint triggers:

• always when there are 2+ arguments where the argument list is on several lines.
• never when there is only 1 argument except if this argument is on a new line.

[lauweijie]

Ideally we should add a fix in the analysis server for this lint and make it possible to apply the fix through dart fix.

Agree. Shall we start by opening an issue on dart-lang/sdk for this request?

Regarding the lints reported on flutter I would recommand that the lint triggers:

• always when there are 2+ arguments where the argument list is on several lines.
• never when there is only 1 argument except if this argument is on a new line.

@jefflim-google any thoughts on the single-arg exception proposed by @a14n?

[bwilkerson]

Shall we start by opening an issue on dart-lang/sdk for this request?

If you'd like to create an issue, then that's the best place for it. Not sure whether we need an issue, though.

[a14n]

I've just took another look at some flutter diagnostics for this lint. I can see 2 common patterns that should be excluded with a single argument:

• anonymous functions in asserts:

assert(() {
  // code
  return true;
} ());

• if the single argument is an instance creation or a method call that fit on the first line:

// example with method call
        rect: animation.drive(slideFromLeadingEdge(
          fromKey: bottomComponents.middleKey,
          fromNavBarBox: bottomNavBarBox,
          toKey: topComponents.backLabelKey,
          toNavBarBox: topNavBarBox,
        )),

// example with constructor
              style: animation.drive(TextStyleTween(
                begin: bottomTitleTextStyle,
                end: topBackButtonTextStyle,
              )),