avoid_shadowing #872
avoid_shadowing #872
Conversation
lib/src/rules/avoid_shadowing.dart
Outdated
| } | ||
|
|
||
| current = current.parent; | ||
| if (current == null) { |
There was a problem hiding this comment.
The body of this if could be moved outside the while.
|
|
||
| // test w/ `pub run test -N avoid_shadowing` | ||
|
|
||
| var top = null; |
There was a problem hiding this comment.
I don't see any tests for shadowing parameter names.
Also, I suspect that the rule as written will produce multiple lints for the same location if the name being shadowed is defined in multiple locations. Consider a test such as:
class C {
int x;
m(int x) {
int x; // LINT
}
}
(Can we verify that only one lint is produced in this case?)
There was a problem hiding this comment.
done.
It looks like several report on the same token for the same rule do not result in lint duplications.
|
That still seems like a lot, and it leaves me concerned that we haven't really defined correctly what we want to have linted and what we don't. It might be worth adding those results to the original issue to see what the reaction from the Flutter team is. |
|
@a14n What happened to this? Did you check if allowing @natebosch 's example helps? |
Yes there's a test with that sample.
I really don't remember why this rule was put aside. |
lib/src/rules/avoid_shadowing.dart
Outdated
| // for details. All rights reserved. Use of this source code is governed by a | ||
| // BSD-style license that can be found in the LICENSE file. | ||
|
|
||
| library linter.src.rules.avoid_shadowing; |
There was a problem hiding this comment.
Please remove the library directive.
|
|
||
| bool skipInstanceMembers = false; | ||
| AstNode current = node; | ||
| while (current != null) { |
There was a problem hiding this comment.
This doesn't seem very efficient, but perhaps in practice it doesn't matter. Have we done any timing measures for this rule?
One easy improvement would be to return before line 98 if variables is empty.
|
Ah, I didn't think to re-read the conversation, so I'd forgotten the issue about false positives. It would be interesting to see how many we still have and whether there are other patterns we should allow (before merging this). |
|
Thanks! I'm still convinced that we don't have a reasonable definition of what does and does not need to be linted. Until we get that we won't have a rule that's worth adding. On a meta-level, I have to wonder whether this rule would have enough value (assuming we can define it well) to warrant the amount of effort it will likely take to finish defining the rule. It might be better to put our time toward something with a higher value. |
|
There are two cases which this lint rule does not catch (or at least they're not tested). I've hit both recently in real code import prefixesimport 'dart:math' as m;
void main() {
var rand = m.Random();
}
void fn(String m) { // lint here
m.Random x = null; // dart2js breaks here.
}This even passes the analyzer, and breaks on, .e.g, dart2js compile. A type parameter shadowing a thingy (e.g. class)class Foo {}
class Bar<Foo> {} // lint hereI could catch this in avoid_shadowing_type_parameters, but I think these two lint rules should be more separated by the thingies they track (the shadowees), rather than the offenders, the shadowers. Seems like the code and performance would be cleaner in each. |
|
Hi @a14n this appears stale; can we close it? |
|
This would be a great lint, for a very real problem. Is there a way to resurrect this PR? Or is it already implemented by some other lint? Looking at the available lints today, it looks like we have type parameter shadowing lint, but no regular identifier shadowing lint (at least not by that name). |
|
I believe this fell due to concerns about an overwhelm of false-positives. I'm not sure that was ever validated though. Or maybe the logic could be dialed back to be less aggressive? /cc @a14n |
|
@filiph The reason this PR was closed is because we did not get to a definition of the rule that provided enough value to outweigh the cost of the false positives. While we do allow a small number of false positives from lint rules, we have a very low tolerance for them. This rule, as written in this PR, did not meet that standard. I believe that the core of the issue is that, while shadowing causes lots of problems, there are far too many places where shadowing is standard practice in the developer community. Perhaps a better path forward would be to follow the pattern of |
|
Thanks for the explanation! FWIW, I think a more conservative (== less false positives) lint rule would be very useful, even if it doesn't get into |
|
Absolutely! Shadowing causes all lots of bugs and anything we can do to help users find them would be great. But studies show that developers will stop using any lint that produces too many false positives [1], so there's no value doing this unless we can identify the patterns that won't cause false positives (which is doable, it just takes effort). I'd encourage every reading this thread to open an issue with the specific use cases you've found that you'd like to have a lint for. [1] Actually, it's worse than that. The study actually shows that a significant portion of users will ignore all lints if any one lint produces too many false positives. So it's not really "no value" it's "negative value". |
|
Arrived here because I have something like this that I'd like to avoid: definitions.forEach((id, definition) {
// ...other code
for (var id in definition.downRefIds) { // <-- I'd like to know I'm shadowing `id` here
if (definitions[id] != null) {
definitions[id]!.addUpRefId(id); // <-- causes bug here
}
}
for (var id in definition.upRefIds) { // <-- and here
definitions[id]!.addDownRefId(id); // <-- and bug here
}
});Coming from JS/TS, there is a rule to complain about it and I like the way they did it there: https://palantir.github.io/tslint/rules/no-shadowed-variable/ |
Fixes dart-lang/sdk#57537