Open FMorschel opened 2 months ago
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.
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?
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.)
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.
This is related to this assist issue.
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 agetter
and that would break the call to the tear-off if the instance is not the same.Kind
Guard against errors.
Bad Examples
Good Examples
Discussion
Inspired by the discussion I had for https://github.com/dart-lang/linter/issues/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 toFlutter Style Guide
since lots ofWidget
s have callbacks that could be a pitfall for the user.Discussion checklist