Make unnecessary_lambdas configurable to exclude some cases

[a14n]

On the Flutter codebase there are some cases we'd like to exclude for readability reason. It'd be great to make unnecessary_lambdas configurable to be able to exclude those methods/functions.

[bwilkerson]

I'm not sure what configuration data you want to be able to supply. Could you give me an example?

[a14n]

In the following example :

-    setState(() {
-      popCalcExpression();
-    });
+    setState(popCalcExpression);

@Hixie argues:

Please don't do this to setState. We're trying to make setState look like a block statement, where it's "obvious" that the statements inside the block are executed immediately. This change hides what's going on, it makes it look like we're taking a reference to call later or something.

So I'd like to tell to the lint unnecessary_lambdas to not provide lints for State.setState. I donn't really know how to provide this reference. Perhaps something like that:

  - unnecessary_lambdas:
      exclude:
      - library: package:flutter/src/widgets/framework.dart
        ref: State.setState

[alexeieleusis]

Probably we need a more comprehensive recommendation in the Effective Dart guideline. One thing we can do is whitelist certain functions/methods from being linted as a temporary workaround.

I understand Flutter model is such that you want a localized place where mutation happens and most things outside it are immutable. So probably more annotations and analysis around things that are explicitly mutable or immutable would make static analysis easier and above all code easier to understand.

As we have recently reviewed rules implementation and analysis results we have found cases that although are like what the rule describes most likely don't need to be changed but there is no easy way to discard them.

I wouldn't go as far a suggesting to annotate to ease static analysis, which is not a goal by itself, but certainly it can help to make intent explicit.

[bwilkerson]

My preference would be, if possible, to refine the rule so that it understands without further input which cases should be flagged and which shouldn't. This might be difficult, but if it's possible it's worth the effort.

I also wouldn't suggest adding annotations in order to ease static analysis, but if the annotation allows framework authors to express their intent, then it might be worth considering.

But I am highly resistant to adding per-rule configuration support to linter. I've seen what happens when that easy out exists, and there be dragons. (Without superhuman effort you quickly end up with a tool that is so hard to configure that no-one will use it.)

[Hixie]

What we've done in the past is to put an annotation in the code, so for example you could say:

class State ... {
  ...

  void setState(@avoidClosureReferences VoidCallback callback) {
    ...
  }

  ...
}

[JPaulsen]

Where is the annotation defined?

[bwilkerson]

It isn't. @Hixie was suggesting that we could add one and that they would use it to get the results they want. We would probably want the inverse lint rule to support this annotation (if we decide to add it) that would create a lint if the argument was not a literal closure.

[a14n]

For this case, I don't think the Flutter API should drive (with an annotation) how my Flutter application is writen. It's just a matter of readibilty and if I'm used to use setState(makeChange) instead of setState(() { makeChange(); }) that's ok. That's why I suggested an exclusion list in yaml.

[goderbauer]

I still wish we could enforce the lint, but selectively opt out certain methods. It would have caught https://github.com/flutter/flutter/pull/98409.

[pq]

Looping back to this, @goderbauer: besides setState where would you like to see opt-outs?

[goderbauer]

I just went through the warnings that this lint would currently produce in the framework.

setState is definitely the most important one to opt out. Personally, I would also opt out some of the API methods in the test framework using the same argument Hixie used above: These should really look like block-like statements. Examples:

  tearDown(() {
    LicenseRegistry.reset();
  });

and same for setUp, group, test, testWidgets.

expect would also be a little less readable without the closure:

expect(() => controller.stop(), throwsAssertionError);

To me, these would actually look rather strange without the lambda. I don't feel as strongly about these. The most important one to opt out is setState.

The lint finds a lot of cases in the framework where we should omit the closure, though. I am looking forward to enabling it once this issue is resolved.

[pq]

This is great. Thanks!

@natebosch might be interested in the test readability cases.

[pq]

Proposal added: https://github.com/dart-lang/sdk/issues/58701.