search

dart-lang / linter

Linter for Dart.
https://dart.dev/tools/linter-rules
BSD 3-Clause "New" or "Revised" License
627 stars 172 forks source link

proposal: update `unnecessary_lambdas` to allow lambdas in exceptional cases #3341

Open pq opened 2 years ago

pq commented 2 years ago

unnecessary_lambdas (exceptions)

Details

TL;DR: a few function invocations are considered generally easier to read and understand w/ closures so we should special-case them in unnecessary_lambdas.

Specifically:

Follow-up from https://github.com/dart-lang/linter/issues/498#issuecomment-1093392561.

Kind

Style.

Good Examples

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

Bad Examples

tearDown(LicenseRegistry.reset);
setState(popCalcExpression);

Discussion

More discussion in https://github.com/dart-lang/linter/issues/498.

Discussion checklist

bwilkerson commented 2 years ago

I'm not fond of the idea of special casing these methods. We should perhaps consider adding an annotation to control this, perhaps something like @preferClosure that can be placed on function-valued parameters.

srawlins commented 2 years ago

Agreed. This rule is backed by a "DON'T" Effective Dart rule. https://dart.dev/guides/language/effective-dart/usage#dont-create-a-lambda-when-a-tear-off-will-do

I'd be interested in hearing from @natebosch and @munificent re: readability and Effective Dart.

pq commented 2 years ago

This rule is backed by a "DON'T" Effective Dart rule.

True but there's a tension w/ Flutter best practice which is why the rule hasn't been adopted (despite the upsides).

I'm not fond of the idea of special casing these methods.

Ah well, you know me, I'm really not either! In general I do strongly prefer the annotation approach over an allow-list so you wouldn't have to twist my arm. 😄

I guess the question bounces to the framework authors,

Thanks for the feedback!

goderbauer commented 2 years ago

I would also prefer an annotation-based solution to this over special-casing these methods. If we had the annotation, Flutter would put them on setState, and potentially testWidget as well as our re-export of the relevant methods from package:test. We could then finally turn on the unnecessary_lambdas lint - and in the future also consider it for inclusion in package:lints.

srawlins commented 2 years ago

I was thinking about the annotation idea, and wonder if it would be convenient to have a broader annotation API here (I'm thinking something like @pragma, like maybe @pragmaticStyle({#preferClosure}) or @lintPragma({#preferClosure})? Or even the existing @pragma?). A whole annotation, like@preferClosure seems so extremely special case. It is a lint rule we add so that ~5 parameters in the universe get annotated?

If we had the annotation, Flutter would put them on setState, and potentially testWidget ...

Are any of the potential sites in dart:ui? Can you reference package:meta from dart:ui? If the expectation is that the only likely APIs to be annotated are in packages like package:flutter, package:test, I think we're OK.

... as well as our re-export of the relevant methods from package:test.

Er... do you mean a literal export? I don't see how the annotation would work there. It would have to be a wrapping:

import 'package:test_api/test_api.dart' as test_api;
void test(@preferClosure whatever, ...) => test_api.test(...);

(I mean, I think we'd support annotating the original package:test parameters.)

goderbauer commented 2 years ago

Er... do you mean a literal export? I don't see how the annotation would work there. It would have to be a wrapping:

Yeah, we do wrap these methods, e.g.

https://github.com/flutter/flutter/blob/fb006175859aa673bcfa7d3ee7e2b58568f3d34b/packages/flutter_test/lib/src/test_compat.dart#L156

Are any of the potential sites in dart:ui?

None of them are in dart:ui, but it is a valid point that the annotation wouldn't work there since we cannot import package:meta from dart:ui, I believe.

pq commented 2 years ago

FWIW: https://dart-review.googlesource.com/c/sdk/+/240900 (food for thought).

As for your general point Sam, I hear you and we've felt this tension before. After a lot of discussion we've avoid pragmas for a bunch of reasons (but it's possible the use-case hadn't entirely lined up). My gut is that a pragma isn't actually any easy to maintain and brings with it its own set of complications.

All of that said, I welcome the conversation! (And treat the meta PR as just a conversation starter.)

pq commented 2 years ago

It is a lint rule we add so that ~5 parameters in the universe get annotated?

Exactly right. And if this is a closed set, one has to ask if even a pragma (assuming it's more light-weight actually) is really better than inlining.

natebosch commented 2 years ago

I don't think we should special case any package:test APIs for this lint. This lint works well while editing tests in my experience, and I don't consider any of the listed APIs meaningfully different from other APIs where it is important that the call sites "look" a certain way. I do find that the lint comes up rarely in tests, and when it comes up I find no readability issues following it.

If we had the annotations available we would not use them in package:test. I would be particularly concerned with treating expect specially here, and I would argue against adding an annotation to a re-export from flutter_test. If flutter_test wants to treat the others as special I would not push back hard. I can see a small argument for this lint increasing the editing distance when going from 2 to 1 steps during setup or teardown - but those cases are rare because most setup and teardown ends up touching local variables and the lint wouldn't fire anyway.

For full disclosure, I also don't see any problem with following the lint for the setState case, but that is not an API I use in my day to day code.

jakemac53 commented 2 years ago

Agreed with Nate here, I don't see what makes any of the test apis different than any other API in terms of this lint (or setState for that matter...).

I find this justification troubling actually:

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.

That is not what is actually happening here, and teaching users to treat tearoffs versus closures differently in terms of the execution model is a mistake. It is always up to the function itself when a function passed to it is invoked. It might be invoked once, many times, immediately, not at all, etc. Users should always be looking at the documentation of the function to understand its behavior.

In fact, it makes it look like we're taking a reference to call later or something is exactly what we are doing... it just happens synchronously before the function call returns.

goderbauer commented 2 years ago

For Flutter's setState the immediate execution is exactly what is happening semantically, though. And that's why we want the code to look like that - without users having to refer to the documentation to understand what's going on.

I believe Flutter did some user studies early on around setState usage and they indicated that there was less confusion around what setState does if it was written as 

setState(() {
  // Do something here...
});

@Hixie may have some more context on this as that predates me being on the team.

(I don't have a strong feeling about the test methods, though. Although I think that conceptually people have an easier time understanding how the API works if they are written in a block-like closure fashion, I could live without it there.)

jakemac53 commented 2 years ago

For Flutter's setState the immediate execution is exactly what is happening semantically, though.

Yes I understand this is what it does, but it isn't a property of the closure, or the fact that it "looks like a block".

Closures like this are passed to all kinds of functions, many of which do not execute it right away (event handlers, etc). We don't want to train users to think something that is not correct.

I believe Flutter did some user studies early on around setState usage and they indicated that there was less confusion around what setState does if it was written as

I think it probably just looks more like a function (if you just see setState(foo.bar) you might wonder, what is bar?). So if you are in a user study looking at a piece of code, in particular outside the context of an IDE (but even there), I definitely am not surprised that people prefer the closure.

I don't think there is anything specific to setState there though. Imo, this is really just a signal that people don't understand tearoffs as intuitively as closures. I think you would get the same results for any function that takes a callback.

This lint exists I believe because there is a real performance benefit to not creating the closure (and potentially capturing all variables in scope). It is not about readability, I don't believe. From my perspective, the closure is always more readable, and also it is easier to modify later with a more manageable diff.

pq commented 2 years ago

It is not about readability, I don't believe.

I think that's exactly right.

One thing to note: the proposal here is not to require closures for the special cases, it's just to permit them. That said, it's true that if we agree that we're missing an opportunity to properly train people that's a bummer. Special cases also introduce their own cognitive burden (e.g., why is main so different, etc.).

Thanks for the thoughtful conversation!

munificent commented 2 years ago

I find this justification troubling actually:

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.

Yes, this logic seems exactly backwards to me. The whole reason most APIs accept closures is to explicitly defer executing the argument expression. Seeing the () { ... } sends a clear signal to the reader "this may not be called immediately". If you have an API that always immediately invokes the closure argument exactly once before executing any other code... it should probably just take that argument as a value. You get the exact same effect with better performance and brevity.

Heck, you could probably define setState() like:

void setState(void fnOrThing) {
  // Assert stuff...
  if (fnOrThing is Function()) fnOrThing();
  // More assert stuff...
}

It would have the same behavior as it currently does but would let users write simpler, likely faster code:

// Instead of:
                        onDateTimeChanged: (DateTime newDate) {
                          setState(() => date = newDate);
                        },

// Just:
                        onDateTimeChanged: (DateTime newDate) {
                          setState(date = newDate);
                        },

I understand that setState() is designed the way it is because it helps users remember to call it which is an extremely easy mistake to make... but it's a pretty odd outlier in terms of API design. It takes a mandatory closure that it calls before executing any code, and then it doesn't use the returned value. In most APIs that would just be:

date = newDate;
setState();

I get why Flutter works this way, but I don't know if it's helpful to have the linter optimize for this style.