dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.25k stars 1.58k forks source link

Dartanalyzer unused-variable hint is incorrect #29478

Closed mkustermann closed 4 years ago

mkustermann commented 7 years ago

Here is an example where a variable is used but the analyzer thinks it is not used:

class Foo {
  Foo operator |(Foo _) {
    return this;
  }
}
main() {
  var f = new Foo();
  f |= new Foo();
}
$ dartanalyzer test.dart
Analyzing [test.dart]...
[hint] The value of the local variable 'f' isn't used. (test.dart, line 9, col 7)
1 hint found.
$ dartanalyzer --version
 dartanalyzer version 1.22.1
lrhn commented 7 years ago

I think the analyzer is correct. The variable is used, but the value of the variable, assigned by |=, is not. It means that the assignment is superfluous, and you could replace the line with f | new Foo();.

mkustermann commented 7 years ago

That is an interesting observation, but:

  1 class Foo {
  2   Foo operator |(Foo _) {
  3     return this;
  4   }
  5 }
  6 
  7 main() {
  8   var f = new Foo();
  9   f |= new Foo();
 10 }
$ dartanalyzer test.dart
Analyzing [test.dart]...
[hint] The value of the local variable 'f' isn't used. (test.dart, line 8, col 7)
1 hint found.

The location the analyzer is pointing to is the variable itself (line 8) not the location of the |= assignment.

Clearly the variable f, as well as one of it's assignments, is used, namely as the receiver of f.|(new Foo()).

A variable can have several different assignments. If the value after a variable assignment is not used until the next assignment, then that specific assignment should be marked as being unnecessary, but not the variable itself.

@lrhn Would you agree?

bwilkerson commented 7 years ago

Having some analysis to point out when a local variable is not referenced after an assignment (and hence the assignment is not necessary) would be useful. But it is different from what this was attempting to find, which is local variables that are not used at all. The point is not that you can eliminate the assignment, the point is that you can eliminate the variable by replacing both lines with new Foo() | new Foo().

mkustermann commented 7 years ago

the point is that you can eliminate the variable by replacing both lines with new Foo() | new Foo()

Sure, but that can be said for this code as well:

var x = new Foo();
x.bar()

Here we can just do new Foo().bar() instead, but shouldn't we let the programmer decide he wants to use a variable or not?

srawlins commented 5 years ago

Duplicate of #27705