Lint when a variable is unused after a compound assignment or pre/post-increment/decrement

[stereotype441]

Check out this buggy code I wrote recently. Can you spot the problem?

Set<Object> _computeExplicitlyTypedParameterSet(
    FunctionExpression functionExpression) {
  List<FormalParameter> parameters =
      functionExpression.parameters?.parameters ?? const [];
  Set<Object> result = {};
  for (var formalParameter in parameters) {
    int unnamedParameterIndex = 0;
    var key = formalParameter.isNamed
        ? formalParameter.identifier?.name ?? ''
        : unnamedParameterIndex++;
    if (formalParameter.isExplicitlyTyped) {
      result.add(key);
    }
  }
  return result;
}

The bug is, I accidentally put the declaration int unnamedParameterIndex = 0; inside the for-loop when I meant to have it before the loop. As a result, each invocation of unnamedParameterIndex++ evaluates to zero because it's applied to a fresh variable. This bug slipped past code review, but was fortunately detected before reaching customers' hands. (See https://dart-review.googlesource.com/c/sdk/+/242741)

It would have been nice to have a lint that detects that there are no further uses of the variable after the post-increment. That would have been a pretty sure sign that I was doing something wrong, and would have caught this bug before I committed it.

More generally, I believe the lint should behave like this: any time a compound assignment, pre-increment/decrement, or post-increment/decrement is encountered in the user's code, and the target is a local variable, call that point A, and then search for other reads of the variable. (A compound assignment or pre/post-increment/decrement counts as a read; in fact it is permissible for A and B to be the same variable reference). If such a read is found, call that point B. If, for any B, a control flow path exists between the A and B, and the variable stays in scope along that whole control flow path, then stop searching; there is no lint. If no such B is found, then report a lint.

In the code above, A and B would both be the unnamedParameterIndex++ expression. There's only one control flow path between A and B that's important, and that's the one that involves getting to the bottom of the while loop, returning to the top, and executing the loop body again. (We don't have to worry about the loop body executing more than twice because any control flow paths we find by considering more loop executions will equivalent for purposes of the lint). The variable doesn't stay in scope along this control flow path, so the lint fires and catches the bug.

If, however, we fix the bug by moving the declaration int unnamedParameterIndex = 0; before the while loop, then the variable stays in scope along the whole control flow path between A and B, so the lint doesn't fire.

Note that in this description I'm trying to appear to intuitions about correctness rather than come up with an efficient implementation. For implementation efficiency it would probably be better to do a single depth first walk through the source code maintaining a data structure that records possible A and B locations for each variable, and then have rules to bubble those data structures up the syntax tree and combine them appropriately wherever there is a flow control construct. This in turn would probably require adding some hooks to the LinterVisitor similar to the hooks used in flow analysis. I think that such hooks would be useful in a whole class of lints; @pq we should talk about this in person.

Also note that there probably need to be some special considerations for variables that are accessed inside function literals and local functions. It might be good enough to just disable the lint entirely for such variables.

[lrhn]

The original issue seems to be a variable not read after being assigned (like #29478).
It's not special to compound assignments, it's relevant to any assignment.

The value of the variable is not used after the assignment, so the assignment is unnecessary code. You could remove the assignment, and not change the code's behavior.
In this case, the assignment is part of ++ and the value of that expression is used, but the assigned value is never read again.

For precisely x++, just removing the assignment is actually not trivial because + 1 might have side effects, so print(x) is not the same as print(x++). It is the same as x + 1; print(x);, though. Still, that's the author's problem, the assignment is still unnecessary, and fair game to warn abotu.

Useless code is something I'd make a plain warning, not a lint.

So:

{ 
 var x = ...;
 x = 42;
 // x definitely never read again => warning.
}

That should (still IMO) be an analyzer warning. So should replacing the assignment with print(x = 42) because the variable is still not read, even if the value is used. That value is not read from the variable.

And both x++ and print(x++) in the same place as x = 42 would also do an unnecessary assignment.

Currently

void main() {
  var x = 0;
  print(x);
  x = 42;
}

gives no warning (in DartPad), so I guess we just don't have an unnecessary assignment warning.

For implementation, do we have a "dominator" analysis? We need something like that for definite-assignment, but that can probably be handled by flow and promotion alone, so the moment the variable is initialized, we no longer care which assignments dominate which other code. (Well, unless they promote.)

[srawlins]

I don't think we use any "dominator" analysis in our unused code tracking (outside of the "dead code" analysis which comes directly from null safety's flow analysis).

I know I've looked at var i = 0; i++; and thought "That should be marked as an unused variable!" But I think we have erred on the side of saying "Well any type implementing operator+ may implement it with side effects." And so we say that "++" is a "use" of the variable.

Similarly, I've had "unused variable" "bugs" like Paul's bug above, where I write var list = <int>[]; list.add(7); and never use the list again. Any time you call a method on a variable, we declare that to be a "use."

Would it be worth it to round up all of the "likely no side effects" functions? A more aggressive diagnostic might say that a getter call, a setter call, or an operator call do not constitute "use." And neither do these 900 methods on core Dart classes.

But I am 100% in favor of the unused-after-assignment diagnostic proposed, including with the compound assignment. Maybe we have to say "compound assignment on non-subtypable classes like int". And maybe we add a more aggressive one for compound assignment of other classes.

[scheglov]

As a lint we could be more aggressive. Even though many interfaces from dart: libraries can be implemented, it does not mean that this is a good idea to implement them.

[eernstg]

Cf. dart-lang/language#2219, you could also consider to special case "primitive" operations. If the lint flags + operations and its brethren on num/int/double then it might not matter so much that it doesn't look at + on MyFunnyClassThatHasSideEffectsOnPlus.

[lrhn]

"Unused variable" is tricky because almost any mention can be a "use".
"Unused assignment" feels like it should be safer. You either (potentially) read the variable again, or you don't (or you do so in a closure, and we can't tell when, but that's just normal.)

The { var i = something; i++; } may or may not use the initial value of the i variable (it definitely uses it, but not for anything that affects program behavior), but it definitely doesn't read the second value assigned to i.

[phildharmadr]

The following code produces a linter warning of "The value of local variable 'lineNumber isn't used'

int lineNumber = 0;
while (true) {
    lineNumber += 1;
    ...
}

This code does not produce the warning

int lineNumber = 0;
while (true) {
    lineNumber = lineNumber + 1;
    ...
}

Seems like the first case should not produce the warning.

[bwilkerson]

Or the second should. It certainly seems like they ought to do the same thing given that they're semantically equivalent.

The argument in favor of producing a diagnostic in both cases is that the real purpose of the lint is to help locate situations in which a variable declaration could be removed without impacting the semantics of the code. If lineNumber isn't referenced anywhere else, then removing both the declaration and the increment will have no effect on the rest of the code.