Lint when `member` might need to be `this.member` in constructor bodies

[jamesderlin]

The other day I modified some non-null-safe code from:

class Foo {
  int x;

  Foo({int x = 42}) {
    // Other work.
  }
}

to:

class Foo {
  int x;

  Foo({int x})
    : x = x ?? 42 {
    // Other work.
  }
}

It turns out that this was wrong because that "Other work" portion in the constructor body referenced x, and because the local x parameter understandably shadows the x member, it therefore was not using the expected default value. Oops. I didn't realize this mistake until I later attempted to migrate the code for null-safety.

This is probably less of a problem with null-safety enabled, although it could still happen. For example:

class Foo {
  int x;
  late String string;

  Foo({int? x}) : x = x ?? 42 {
    string = x.toString();
  }
  
  @override
  String toString() => string;
}

void main() {
  print(Foo()); // Want '42', but prints 'null'.
}

Or it could just be something like:

class Foo {
  int x;

  Foo({int? x}) : x = x ?? 42 {
    int updatedValue = someMemberFunction();
    x = updatedValue;
  }

It would be nice if there were some lint that suggested using this.x instead of x in the constructor body for these situations. In the specific case of constructors, if a parameter name and a member name are the same, it seems rare to me that someone would prefer the parameter; I think it would almost always be a mistake. This isn't fully baked, but I think criteria would be something like:

• Class T has a member variable x and a constructor with a parameter named x. That constructor has an initializer list that initializes x that is not x = x or this.x = x. (Intention is to lint only if the member and parameter potentially have different values.)
• Class T has a setter named x and a constructor with a parameter named x. That constructor has a body that assigns to x.

[jamesderlin]

(Edit: Ignore, I'm the one who was confused.)

[bwilkerson]

In order to prevent false positives we would need to be able to statically determine when the user intended to refer to the field rather than the parameter, and I'm not sure that's feasible. But we could have a style lint that disallowed using the name of a field as a parameter (with the obvious exception for field formal parameters).

It might be useful to run some analysis of existing code to find out how often there are non-field formal parameters whose name is the same as a field from the enclosing class (or an inherited field?).

[jamesderlin]

1. It would be restricted to constructors. Constructors are the most likely place where unexpected shadowing would occur since:
   • It's typical to give constructor parameters the same names as members.
   • Constructors are where people are people are most likely to expect member to be implicitly this.member (which happens for initializer lists and initializing formals). People (which included me) might not realize that constructor parameters sometimes introduce local variables and sometimes don't:

     class Foo {
       int x;
       int y;
     
       Foo(int x, this.y) : x = x {
         x = x * x; // Assigns to local x.
         y = y * y; // Assigns to this.y.
       }
     }

2. I claim that reassigning function parameters in general is uncommon, and reassigning constructor parameters would be even less common. Therefore if a constructor parameter is reassigned in the constructor body and if the parameter has the same name as a setter in the enclosing class (or in a superclass), it's almost certainly an accident and should trigger a lint.
3. That leaves cases where the constructor body gets the value from a constructor parameter with the same name as a getter in the enclosing class. This is a lot trickier, but I think restricting it according to the criteria I mentioned in my original comment might be good enough to catch common cases without being so broad that it'd trigger many false positives.

The lint probably doesn't need to be perfect and catch all cases; catching common cases would be an improvement.