Show a hint when a local variable is never read after being assigned

[munificent]

The largest usability paint point by far with null safety is the fact that nullable fields do not promote to non-nullable after a null check:

class C {
  int? currentValue;
  test() {
    if (currentValue != null) {
      print(currentValue + 1); // Error: currentValue could be null.
    }
  }
}

This behavior is deliberate because it's not sound to promote fields. The value could change between when it is checked and when it is read again. Or the field could be overridden by a getter in a subclass that returns different values on different calls.

To work around this limitation, users often copy the field to a local variable first, and then use that:

class C {
  int? currentValue;
  test() {
    var currentValue = this.currentValue; // Copy.
    if (currentValue != null) { // Local variable promotes.
      print(currentValue + 1); // OK!
    }
  }
}

To make this pattern easier, we are looking at a number of language features that essentially desugar to declaring a local variable with the field's value: dart-lang/language#1191, dart-lang/language#1201, dart-lang/language#1210, dart-lang/language#1420, dart-lang/language#1514, dart-lang/language#1975, dart-lang/language#1991. All of these features (except dart-lang/language#1514) share a problem that the explicit pattern also has:

class C {
  int? currentValue;
  test() {
    var currentValue = this.currentValue; // Copy.
    if (currentValue != null) {
      currentValue = currentValue + 1; // Oops.
    }
  }
}

If you write to the shadowing local variable, it does not write to the backing field. It just updates the local variable. That's probably not what you want, though it may be sometimes.

There is a simple hint we can show that will catch the cases where this is clearly unintended: Warn if a local variable is never read after it is assigned.

It's a local variable, so we know the write has no side effects. (It's not calling a setter.) The variable is never read again, so we know the write is pointless. If the user wrote it, they probably think it does something, but it definitely doesn't. Telling them is a good way to help them notice this bug.

We could make this a lint instead (cc @pq). But I lean towards it being a hint because I think that's the severity level we use for other dead code diagnostics, like unused_local_variable. In my mind, this is essentially dead code: the assignment has no effect.

[bwilkerson]

I agree that an assignment to a local variable is dead code if the variable is never read, and I have no objections to making such a check a hint.

We use a modified form of this pattern in the analyzer codebase fairly frequently. The difference is that we make the local variable final so that any attempt to assign to it will be caught. Even if we had the new hint, I think it improves readability to mark the variable as final. (It would be useful, for that pattern, to have a lint that flags any assignment of the form var <name> = this.<name>; as needing to be made final.)

[munificent]

We use a modified form of this pattern in the analyzer codebase fairly frequently. The difference is that we make the local variable final so that any attempt to assign to it will be caught. Even if we had the new hint, I think it improves readability to mark the variable as final. (It would be useful, for that pattern, to have a lint that flags any assignment of the form var <name> = this.<name>; as needing to be made final.)

That's not a bad idea either. But I can see a user making this erroneous chain of logic:

1. I need this field to promote so I'll copy it to a local variable first, which I mark final.
2. I need to write back to the field, so I assign to it, not realizing that I'm actually only assigning to the local.
3. I get an error about the local being final, so I change it to var.
4. The error goes away. Now my code is error free but doesn't do what I want.

Definitely some users will see the error on step 3 and realize that it means they aren't writing to the field. But for users that don't realize that or don't think to use final at all, a hint will help guide them back onto the right path.

[eernstg]

We would have to take the notions of 'potentially evaluated' vs. 'definitely evaluated' into account, and the hint might be a false positive in the 'potentially' case—do we need to add code to use the value on the control flow paths where it's currently not used? Also, in the case where a new value may or may not have been written to the variable, do we insist that the variable should be evaluated because the new value might otherwise be improperly unused?

[lrhn]

I thought we already warned if a variable's value isn't used, but it doesn't seem to trigger on assignments after the variable has been used. "Unnecessary assignment" (to a local variable) is a perfectly good hint in general.

I can very easily imagine doing var foo = this.foo; and then go on to assign values to the local foo deliberately. Like:

var message = this.message;
if (message != null) {
  message = "$message (at line $line)";
} else {
  message = "Error (at line $line)";
}
report(message);

(I know, that's doable as a one-liner, imagine something a little more complicated!)

. I guess I can always dodge it by using a different name (but it's a good name!)

[munificent]

I can very easily imagine doing var foo = this.foo; and then go on to assign values to the local foo deliberately. Like:

var message = this.message;
if (message != null) {
  message = "$message (at line $line)";
} else {
  message = "Error (at line $line)";
}
report(message);

Yes, I think that pattern is totally fine and shouldn't cause an error. The problem is not assigning to a local which contains a copy of a field's value. It's doing that and then not using the local. So, as you say, it's the unnecessary assignment that's the problem.

[leafpetersen]

Just a comment - probably understood already, but to be precise "read after assigned" needs to account for variable capture in closures. That is, textually there may be no reads after the write, but if the variable has been captured then the write may not be dead.