Analyzer should lint if a statement consists of only an == expression

[Hixie]

I would like the following to flag a lint:

void main() {
  bool a = true;
  a == false;
}

It would have caught a bug that was reported today (it's especially hard to tell that something fishy is going on when both "a" and "false" are actually much longer expressions).

[MichaelRFairhurst]

I keep meaning to argue for my lint rule that catches this dart-archive/linter#756 but haven't really invested the time to push through controversy.

Will gladly note that this caused a bug for you there :)

[bwilkerson]

@MichaelRFairhurst I just want to know that this rule has a near zero false positive rate. Some of the cases you proposed catching can clearly have side-effects, and the false positive rate might just be high enough to make the lint useless. Should be easy enough to validate by running the lint over some reasonable code.

(Technically, even == can produce false positives because the operator is user-definable and therefore could have side-effects, but it's uncommon enough, and strongly enough discouraged, that it isn't worth blocking the rule for.)

[Hixie]

Oh man, yeah, a lint to just flag every "useless" line would be fantastic.
I would consider flagging an intentional line of the form a == b where a.== has a side-effect to be a true positive, not a false positive. Relying on operator side-effects is bad and a lint that would catch such things would be great.

FWIW, we would enable such a lint on Flutter's codebase in a second. I'm curious to see what errors it reports today! We'd be happy to have our code base be a place to test for false positives.

[bwilkerson]

Flutter was one of the code bases I suggested as a test.

[MichaelRFairhurst]

Yes -- I should say, I have not yet run the tests that Brian suggested. And a good test I might add. So its not fair to say that controversy blocked me, but rather, that the possibility of controversy blocked me. Which is on me and me only.

Edit: but it is still on my list of things to get to.

[Hixie]

Well I definitely am eager to see the results of running it on Flutter, whether or not you do anything with it afterwards!

[MichaelRFairhurst]

I'll try to consider this an impetus to not leave it there dying :)

I also didn't know how to run such a test, but I've started the process of figuring that out by asking Brian on the PR :) And I'll assign myself here.

Edit: Seems I can't assign anyone on tickets here

[a14n]

I ran this rule on flutter and here are the results :

[lint] Statements should have a clear effect. (dev/bots/test.dart, line 266, col 3)
[lint] Statements should have a clear effect. (dev/automated_tests/flutter_test/test_async_utils_unguarded_test.dart, line 19, col 3)
[lint] Statements should have a clear effect. (dev/automated_tests/flutter_test/test_async_utils_guarded_test.dart, line 22, col 3)
[lint] Statements should have a clear effect. (dev/automated_tests/flutter_test/exception_handling_test.dart, line 16, col 5)
[lint] Statements should have a clear effect. (dev/benchmarks/microbenchmarks/lib/stocks/animation_bench.dart, line 42, col 3)
[lint] Statements should have a clear effect. (examples/layers/rendering/touch_input.dart, line 135, col 3)
[lint] Statements should have a clear effect. (examples/layers/rendering/flex_layout.dart, line 89, col 3)
[lint] Statements should have a clear effect. (examples/layers/rendering/hello_world.dart, line 12, col 3)
[lint] Statements should have a clear effect. (examples/layers/rendering/custom_coordinate_systems.dart, line 24, col 3)
[lint] Statements should have a clear effect. (examples/layers/rendering/spinning_square.dart, line 46, col 3)
[lint] Statements should have a clear effect. (examples/flutter_gallery/lib/gallery/example_code.dart, line 195, col 1)
[lint] Statements should have a clear effect. (examples/flutter_gallery/lib/gallery/example_code.dart, line 133, col 1)
[lint] Statements should have a clear effect. (examples/flutter_gallery/lib/gallery/example_code.dart, line 47, col 1)
[lint] Statements should have a clear effect. (examples/flutter_gallery/lib/gallery/example_code.dart, line 206, col 1)
[lint] Statements should have a clear effect. (examples/flutter_gallery/lib/gallery/example_code.dart, line 18, col 1)
[lint] Statements should have a clear effect. (examples/flutter_gallery/lib/gallery/example_code.dart, line 121, col 1)
[lint] Statements should have a clear effect. (examples/flutter_gallery/lib/gallery/example_code.dart, line 85, col 1)
[lint] Statements should have a clear effect. (examples/flutter_gallery/lib/gallery/example_code.dart, line 37, col 1)
[lint] Statements should have a clear effect. (examples/flutter_gallery/lib/gallery/example_code.dart, line 59, col 1)
[lint] Statements should have a clear effect. (examples/flutter_gallery/lib/gallery/example_code.dart, line 28, col 1)
[lint] Statements should have a clear effect. (examples/flutter_gallery/lib/gallery/example_code.dart, line 183, col 1)
[lint] Statements should have a clear effect. (examples/flutter_gallery/lib/gallery/example_code.dart, line 170, col 1)
[lint] Statements should have a clear effect. (examples/flutter_gallery/lib/gallery/example_code.dart, line 149, col 1)
[lint] Statements should have a clear effect. (examples/flutter_gallery/lib/gallery/example_code.dart, line 97, col 1)
[lint] Statements should have a clear effect. (examples/flutter_gallery/lib/demo/material/overscroll_demo.dart, line 29, col 5)
[lint] Statements should have a clear effect. (packages/flutter/test/gestures/gesture_tester.dart, line 14, col 5)
[lint] Statements should have a clear effect. (packages/flutter/test/material/popup_menu_test.dart, line 108, col 9)
[lint] Statements should have a clear effect. (packages/flutter/test/animation/animations_test.dart, line 162, col 17)
[lint] Statements should have a clear effect. (packages/flutter/test/services/image_provider_test.dart, line 12, col 7)
[lint] Statements should have a clear effect. (packages/flutter/test/widgets/scrollable_semantics_test.dart, line 43, col 5)
[lint] Statements should have a clear effect. (packages/flutter/test/widgets/scrollable_semantics_test.dart, line 137, col 5)
[lint] Statements should have a clear effect. (packages/flutter/test/widgets/scrollable_semantics_test.dart, line 79, col 5)
[lint] Statements should have a clear effect. (packages/flutter/test/widgets/directionality_test.dart, line 69, col 7)
[lint] Statements should have a clear effect. (packages/flutter/lib/src/material/text_field.dart, line 216, col 7)
[lint] Statements should have a clear effect. (packages/flutter/lib/src/foundation/print.dart, line 74, col 5)
[lint] Statements should have a clear effect. (packages/flutter/lib/src/rendering/image.dart, line 131, col 5)
[lint] Statements should have a clear effect. (packages/flutter/lib/src/widgets/binding.dart, line 812, col 7)
[lint] Statements should have a clear effect. (packages/flutter_driver/lib/src/extension.dart, line 65, col 3)
[lint] Statements should have a clear effect. (packages/flutter_test/lib/src/binding.dart, line 878, col 9)
[lint] Statements should have a clear effect. (packages/flutter_test/lib/src/binding.dart, line 130, col 9)
[lint] Statements should have a clear effect. (packages/flutter_test/lib/src/binding.dart, line 132, col 9)
[lint] Statements should have a clear effect. (packages/flutter_tools/test/compile_test.dart, line 175, col 3)
[lint] Statements should have a clear effect. (packages/flutter_tools/lib/src/base/utils.dart, line 228, col 5)

[Hixie]

Looks like the main false-positive is calling a constructor. That should be treated like calling a method (e.g. new Timer()).

[lrhn]

I'd also take that as an argument that a constructor that you call for its side-effect should probably have been a static method. Creating an object and immediately discarding it is, at the very least, confusing.

Timer is special because it does create a useful object, you just don't always want to use it. I'd lean towards making it a static factory method if I wrote it today, but it's not worth changing now.

[Hixie]

In general I agree. The cases I found when spot-checking the above fall into the following buckets:

• Calling the binding constructors. The binding is a singleton, the first time you call the constructor it puts the instance in a well-defined private location that can only be set by constructing the binding. You can't call it a second time (it'll throw), which is how we prevent two instances of the binding classes being created. If we were to take a reference there isn't much we could do with it. The binding is the result of mixing in many classes, so unlike Timer a static method couldn't do it. Basically this is using constructors as a work-around for the lack of virtual statics and virtual constructors, which is how I would implement this with other languages.
• Calling new Timer(), where the timer won't be canceled.
• Calling constructors that have side-effects from tests, to verify what the side-effects are. If we were to take a reference, the analyzer would complain we had an unused variable.
• Sample code that shows how you would construct an instance of a widget in a build method. Normally you'd return it or use it as the expression of an argument in another widget. In the sample code it's clearer just to do the constructor call alone.

These all seem like valid use cases. I think therefore that the lint should be changed to allow calling new on a class where at least one of the constructors has a body.

[MichaelRFairhurst]

Big thanks to @a14n for running on flutter for me!

I agree with your findings @Hixie , especially "Calling constructors that have side-effects from tests, to verify what the side-effects are. If we were to take a reference, the analyzer would complain we had an unused variable."

Seems like a simple reasonable change to allow constructors. I'll get to that by EOD

[MichaelRFairhurst]

Almost missed my EOD deadline!

that PR has been updated.

[MichaelRFairhurst]

Brian merged this PR. After the next linter+sdk release, simply use the rule unnecessary_statements.