A lint for shadowing members?

xster commented 7 years ago

As an extension to https://www.dartlang.org/guides/language/effective-dart/usage#dont-use-this-when-not-needed-to-avoid-shadowing

Would it make sense to add a new lint type for (potentially unintentionally) shadowing an inherited member?


class A {
  @protected
  int get stuff => ... blah ...;
}

class B extends A {
  [either] int stuff = ... blah ...;

  [1000 lines later]

  void someMethod() {
    [or] int stuff = ... blah ...;
    int thingy = stuff; // I thought I was getting super.stuff
  }
}

Hixie commented 7 years ago

IMHO that one just shouldn't be allowed. It's this case I think we need a lint for:

class A {
  @protected
  int get stuff => ... blah ...;
}

class B extends A {
  void someMethod() {
    int stuff;
    // 10 lines later
    int thingy = stuff; // I thought I was getting super.stuff
  }
}

...which happens in particular when you renamed A.stuff and prior to that it didn't get shadowed by the local variable.

Hixie commented 7 years ago

cc @a14n

a14n commented 7 years ago

I made a rule avoid_shadowing available in https://github.com/a14n/linter/tree/avoid_shadowing branch.

The rule is perhaps a little too aggressive. It adds a lint if there's already a similar name visible (see test).

On Flutter I get 155 lints:

xster commented 7 years ago

Randomly clicked on a few. They all look reasonable. Perhaps for an extra level of leniency initially, we can omit local variables shadowing instance methods. Hopefully in those cases if a function is passed where it's expecting something else, it would crash soon enough anyway.

Hixie commented 7 years ago

Yeah, shadowing instance methods is probably ok.

Hixie commented 7 years ago

Actually I clicked on a few more and none of those were methods so maybe that's not helpful.

I wonder what the right heuristic is. Having a local variable called Size that overrides the instance field "size" is fine. Having a local variable "widget" that overrides the instance field "widget" is not... but why not?

a14n commented 7 years ago

Similarly I had the mixed feeling about the lines linted.

However I can imagine some people happy with this rule as it is today. If I look at Checkstyle in the Java world there's a HiddenFieldCheck that has a similar heuristic (that also applies to the parameter names - I should perhaps add their checkings to the rule)

jakemac53 commented 7 years ago

I just ran into a case today where this would have helped during a refactor, +1 to the lint.

My specific case was shadowing an instance field with a local variable on accident.

jakemac53 commented 6 years ago

@a14n curious if there has been any progress on this?

a14n commented 6 years ago

@jakemac53 I just opened #872 to add an avoid_shadowing rule. It would be great if you can run this ne rule on your project and see if it is fine or if you see some pattern to customize.

Running it on flutter lints the following lines:

@Hixie @xster does the results look good to you?

bwilkerson commented 6 years ago

I just looked at a couple of places and had some half-baked thoughts.

I found several lints for code similar to the following:

final double currentTheta = this.currentTheta;

Seems like a fairly common pattern used to avoid invoking a potentially expensive method multiple times when the author is confident that the result of the invoked method won't change during the invocation of the containing method. Perhaps the lint should allow the shadowing if the local name is initialized by invoking the shadowed name.

final Version version = new Version.parse(versionText);

It's also fairly common (though not necessarily good practice) to given variables names that are the same as the type of the variable. Perhaps the lint should allow this pattern as well.

Finally, I wonder whether users really care about following this rule in test code, or whether this rule should limit itself to production code.

Having a local variable called "size" that overrides the instance field "size" is fine. Having a local variable "widget" that overrides the instance field "widget" is not... but why not?

I can think of a couple of possible reasons, though I won't pretend to know which if any are valid.

First, it's possible that some names, such as "size", "length", and "name", are generic enough and common enough that we just expect them to be used as local variable names even when they are also used as member names. Having that expectation might make it easier for us to deal with the cognitive load from shadowing. On the other hand, a name like "widget", while both generic and common, has a specific enough meaning in this context that perhaps it negates that expectation.

Another possibility is that it has less to do with the name and more to do with the perceived complexity of the type of the variable. Perhaps we deal better with shadowing simple types than we do with shadowing complex types.

natebosch commented 6 years ago

Perhaps the lint should allow the shadowing if the local name is initialized by invoking the shadowed name.

I think we should allow this.

a14n commented 6 years ago

With the exclusion pattern var x = this.x; Flutter has the 82 below lints (121 without the pattern).

natebosch commented 6 years ago

Another pattern that we should allow:

String get someValue {
  final someValue = new StringBuffer();
  ...
  return '$someValue';
}

So we should allow shadowing a property inside the implementation of that property.

krp commented 4 years ago

+1 for this feature. It would be useful in certain instances where code has been refactored.

e.g. https://github.com/dart-lang/language/issues/737

filiph commented 4 years ago

Does this include closure variable shadowing? I just found a nasty bug that looks something like this (identifier names have been changed to protect their identities):

void prepareDeliciousMeal(Fridge f) {
  final e = f.getSausage();

  // ... more code ...

  final ingredients = f.everything
      .where((e) => e.goesWellWith.contains(e.id));
  cook(ingredients);
}

If this is not the bug for this, just let me know, I'll create another one.

dart-lang / linter

A lint for shadowing members? #597