Tweak unnecessary_statements lint to ignore getters; they may have side effects. #1019
Conversation
|
/cc @bwilkerson |
| @override | ||
| visitPrefixedIdentifier(PrefixedIdentifier node) { | ||
| // Allow getters; they may have a side effect. | ||
| if (node.identifier.bestElement is PropertyAccessorElement) return; |
There was a problem hiding this comment.
Hmm, I think this is potentially too broad...I believe this will flag the following case:
class A {
int x;
}
use(A a) {
a.x;
}
I believe we can distinguish this vs. int get x because I believe the latter is a PropertyInducingElement and the former is not.
OTOH, I'm curious what others think. Its possible to do
class B implements A {
int get x => throw ...;
}
So, we'd still have false positives for getters inside this lint in some perhaps common perhaps uncommon case. I may still defend them, due to the fact that that class of false errors would be more confusing (indirect) than accessing a custom defined getter directly.
There was a problem hiding this comment.
Good catch, thanks. Added test cases for this. (Three, one for each added clause). Fix was to check isSynthetic.
I'm not too worried about the cases with overrides; I think they're very rare.
| @@ -160,6 +159,14 @@ class _ReportNoClearEffectVisitor extends UnifyingAstVisitor { | |||
| // Has a clear effect | |||
| } | |||
|
|
|||
| @override | |||
| visitPrefixedIdentifier(PrefixedIdentifier node) { | |||
| // Allow getters; they may have a side effect. | |||
There was a problem hiding this comment.
I would clarify, we aren't allowing them because the may have a side effect. So do operators since those could be overloaded too.
I would maybe say something more like, allow getters since they commonly have side effects? Or, because in practice they make up most of the false positives?
|
Thanks! |
|
Thank you both! |
Fixes dart-lang/sdk#57718