dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.22k stars 1.57k forks source link

Auto-fix for turning `() => fn()` into `fn` #54088

Open lukehutch opened 11 months ago

lukehutch commented 11 months ago

I want the Dart language server to provide a suggested fix for turning lambda expressions into function references, where possible. For example:

parlough commented 11 months ago

I don't think there's an always-on assist for this currently, but if you enable the unnecessary_lambdas lint, at least some cases of this should be offered as a quick fix.

A general assist makes sense to me too.

srawlins commented 11 months ago

I think it makes sense to restrict this to a quick fix. We could have a stance of "offer all quick fixes for (recommended? core?) lint rules as assists (even if the lint rule is disabled)" but I don't think we have that stance. And I don't think I'd support it (too many offers; bad UX).

bwilkerson commented 11 months ago

I agree. We absolutely don't want to offer all quick fixes for lint rules as assists.

I do think there are some fixes for which it does make sense to offer the fix as an assist. Specifically I think that "fixes" that convert code from one form to another often make sense (such as changing a function body from a block to an expression, or a sequence of if-else statements to a switch). These conversions are often useful even when a particular style isn't being enforced everywhere.

@lukehutch Does enabling the lint so that these closures could automatically be caught work for you, or is there some reason why enabling the lint wouldn't be desirable?

rakudrama commented 11 months ago

Don't forget the tear-off case, e.g. (x) { list.add(x); }list.add.

I see a tension here with other recommendations We advise to rewrite y.forEach((x) { list.add(x); }); as a for-in loop, but it would often be better to rewrite to y.forEach(list.add);.

bwilkerson commented 11 months ago

If the lint/fix doesn't handle the tear-off case then we should definitely consider making them do so.

If the lint catches the tear-off case, then the forEach case would also be handled.

lukehutch commented 11 months ago

I enabled the lint, and for the three cases I raised, the linter gives "Closure should be a tearoff", and VS Code replaces this with a tearoff on save. Nice!

The extra case that @rakudrama raises is not detected by the current analyzer code. So I'll leave this issue open, unless you all think that that case is out of scope.

As an aside, it looks like there are a million linter options that I didn't know existed. How is the decision made as to which of them should be enabled by default? It might be helpful to add an option all (unless there is something equivalent that I missed), to enable all lint options, like -Wall in gcc. It would be much easier to discover specific linter options that way.

bwilkerson commented 11 months ago

How is the decision made as to which of them should be enabled by default?

Good question. Right now it's fairly hard: read https://dart.dev/tools/linter-rules and decide for each rule whether or not you want to enable it. We do have three Dart and Flutter team curated lists of rules that you can use. They are described on that page.

Longer term we have plans to add additional documentation that will make it easier to understand what's available and how it could help.

@MaryaBelanger

It might be helpful to add an option all (unless there is something equivalent that I missed) ...

No, there's nothing equivalent. The reason we don't have such an option is because some of the lints are there to support user preferences and actually conflict. For example, we have one rule to enforce that all string literals are written using single quotes, and another enforcing the use of double quotes. Enabling both wouldn't make much sense. (Most of the lints are there to prevent errors, and those don't have conflicting versions.)

parlough commented 11 months ago

To add on to Brian's response, again noting there is work planned to make lints easier to discover and evaluate, some personal insights:

Generally I recommend finding and using a premade lint set that matches your style and preferred level of strictness. That way you can just update as they make new releases. You can find many of these packages, including the Dart and Flutter teams' collections, under the #lints topic on pub.dev.

As Brian mentioned, the issue with enabling all linter rules is that some are incompatible with each other and many are stylistic. Despite this, some developers do like to enable all of them, and then disable until they are happy. There's an example analysis_options.yaml with every released lint enabled at https://dart.dev/tools/linter-rules/all for that use case.

lukehutch commented 11 months ago

Thanks for the help in finding linter rule sets!

@parlough @bwilkerson one suggestion on how to make lints easier to discover: what if disabled lints were popped up once, but only as new code is being edited (i.e. not applied across the entire codebase), and the user is then asked whether they want to enable or disable this lint, for this project or for the whole IDE?

OK, to re-focus this issue then, this is the one outstanding thing that could be added to the linter:

(x) { list.add(x); }list.add

andrechalella commented 3 weeks ago

By the way, is there a way to make this better?

@override
State<MainApp> createState() => _MainAppState();