dart-lang/linter

proposal: `avoid_mutable_tearoffs`

FMorschel opened this issue · 5 comments

FMorschel commented

avoid_mutable_tearoffs

Description

Avoid capturing methods from mutable variables as tear-offs, as this can result in unintended behavior by referencing the method of a specific instance rather than dynamically calling the method on the current value of the variable.

Details

When a method is captured as a tear-off from a mutable variable, the tear-off references the specific method of the instance at the time of the capture. If the variable is later reassigned to a different instance, the tear-off still references the method of the original instance, leading to potential bugs.

Even if the variable is final at the instance level, the class could be extended and in that case be overridden for a getter and that would break the call to the tear-off if the instance is not the same.

Kind

Guard against errors.

Bad Examples

class MyClass {
  void doSomething() {
    print('Doing something');
  }
}

void main() {
  var myObject = MyClass();
  var tearOff = myObject.doSomething; // LINT: Avoid capturing tear-off from a mutable variable

  myObject = MyClass();
  tearOff(); // Calls doSomething on the old instance
}

Good Examples

class MyClass {
  void doSomething() {
    print('Doing something');
  }
}

void main() {
  final myObject = MyClass(); 
  var tearOff = myObject.doSomething; // OK: Final variable, so reassignment is not possible
  tearOff();

  var myObject2 = MyClass();
  var callback = () => myObject2.doSomething(); // OK: Using a lambda to ensure the latest instance method is called
  myObject2 = MyClass(); 
  callback(); // Calls doSomething on the new instance
}

Discussion

Inspired by the discussion I had for #5056. It should be triggering most of the time (if not always) that unnecessary_lambdas wouldn't trigger.

I believe whenever it lands, it should be added to Effective Dart but if not, definitely to Flutter Style Guide since lots of Widgets have callbacks that could be a pitfall for the user.

Discussion checklist

  • List any existing rules this proposal modifies, complements, overlaps or conflicts with.
  • List any relevant issues (reported here, the [SDK Tracker], or elsewhere).
  • If there's any prior art (e.g., in other linters), please add references here.
  • If this proposal corresponds to [Effective Dart] or [Flutter Style Guide] advice, please call it out. (If there isn't any corresponding advice, should there be?)
  • If this proposal is motivated by real-world examples, please provide as many details as you can. Demonstrating potential impact is especially valuable.
lrhn commented

This would generally mean to only use tear-offs of constants or non-instance final variables.
That's probably too restrictive.

Doing a tear-off from a local variable can be perfectly reasonable, and local variables don't need to be final even if they aren't mutated. That's a valid style.
The lint should at least recognize non-mutated local variables as equivalent to non-mutable ones.

Considering any final instance variable as potentially mutated in a subclass is likely also too strict. Most of them aren't.

Then there is an issue of aliasing.
If an instance variable is mutable, and a method reads it into a final local variable to work with it, then which variable is the one that's determines whether the value can change out not?

Whether a variable can change isn't necessarily significant for whether a tear-off should or shouldn't be done on the current value. Sometimes that is what is intended, other times it's not. Sometimes the tearoff is updated when the value changes. Whether it's correct might depend on the life cycle of what the tearoff is passed to, and whether it is compatible with the life-cycle of the value. Or generally what the tearoff is used for. I don't think that's something it is possible for a lint to predict or recognize.

No matter how this lint is defined, I'll definitely be against having it recommended, unless we generally discourage the use of tear-offs.
The "from a mutable variable" part yof just too fragile and insignificant to use as a signal.

FMorschel commented

This would generally mean to only use tear-offs of constants or non-instance final variables.

Yes, that would be it. But it could be as simple as adding a quick fix to create () => foo() or even to create a method within the current class to do that forwarding.

The lint should at least recognize non-mutated local variables as equivalent to non-mutable ones.

I agree.

If an instance variable is mutable, and a method reads it into a final local variable to work with it, then which variable is the one that's determines whether the value can change out not?

In that case, as mentioned at #5056 the current behaviour is that the aliased variable is the one considered. I think this would be expected.

Whether a variable can change isn't necessarily significant for whether a tear-off should or shouldn't be done on the current value. Sometimes that is what is intended, other times it's not.

That's when //ignore comes into action. It sends a signal to anyone reading that "I know what this implies and I want to keep it".

The "from a mutable variable" part yof just too fragile and insignificant to use as a signal.

What do you mean? I saw you have commented at #5056 after commenting here. Have any new thoughts come to your mind after reading that thread?

lrhn commented

Reading this and #5056 made me start worrying that the unnecessary_lambdas lint is just fragile. Sometimes you want a tear-off of a mutable variable. Sometimes you don't want a tear-off from a final variable (because aliasing is non-trivial to determine).

What matters is whether the life-time of the tear-off matches, or should match, the life-time of the variable value.
If the variable changes, should whatever code that uses the tear-off start using the new value's function?
Maybe. It's impossible to say in general.
There are valid reasons to have a function that always forwards to a method of the current value of some variable at the time of calling, and there are valid reasons to create a tear-off of the current value and keep it around forever (or as long as necessary), even if the value it's torn off from changes. Maybe the value lives on somewhere else. Maybe the closure itself is what keeps it alive.

It's easy to constructor false positives, which makes it a questionable lint. (The redeeming factor would be that false positives are rare in real code.)

I'm questioning this suggested lint even more because I think the source value being mutable is not sufficiently significant for whether to use a tear-off if the current value or a closure that looks up the value when called.
It's not better than the unnecessary_lambdas, just different. If anything I expect (relatively) more false positives, and I expect it to be hard to define when the "source is mutable".

I can't tell you whether to use someIterator.current.method and () => someIterator.current.method. It depends on what you intend to do, and any suggestion, in either direction, can be wrong. And it doesn't matter if someIterator is a final variable. Assuming that any getter is mutable is safe, but useless. Many aren't, and you don't actually know whether something is a getter or not when accessing an interface.

The unnecessary_lambdas lint is at least simple: Can be tear-off ⇒ should be tear-off. (Going by the documentation, which should be the specification.)

FMorschel commented

The unnecessary_lambdas lint is at least simple: Can be tear-off ⇒ should be tear-off. (Going by the documentation [...])

Yes, but it is not being followed or something is missing here.

As per #5056 original description, I'll place here the code sample:

class _HomeState extends State<Home> with TickerProviderStateMixin {
  late final AnimationController animationController;

  @override
  void initState() {
    super.initState();
    animationController = AnimationController(
      duration: const Duration(milliseconds: 500),
      vsync: this,
    );
  }

  @override
  Widget build(BuildContext context) {
    return IconButton(
      icon: const Icon(Icons.settings),
      onPressed: () {
        animationController.toggle();
      },
    );
  }
}

The onPressed here doesn't trigger. Not following Can be tear-off ⇒ should be tear-off.

Neither at the new sample for it (same as above):

class _HomeState extends State<Home> with TickerProviderStateMixin {
  AnimationController get animationController => {...}

  @override
  Widget build(BuildContext context) {
    var c = animationController;
    return IconButton(
      icon: const Icon(Icons.settings),
      onPressed: () {
        c.toggle();
      },
    );
  }
}

I'm really unsure what to take from this then. As you said, from that discussion, I agree with you that the unnecessary_lambdas lint is just fragile.

FMorschel commented

This is related to this assist issue.