lint for unused result?

[eseidel]

C++ has lints like this. Would be nice to have something for Dart.

Tonight I had this in my code:

Foo createFoo(bar) {
if (bar == 1) new Baz();
if (bar == 2) return new Barf();
return null;
}

I forgot the return statement before new Baz(). :(

[pq]

cc @scheglov @bwilkerson

[scheglov]

It is probably useful to catch bugs, but there is a small chance of false positives. new Baz() could have a side effect, e.g. starting some processing, or listening for events, etc. Should be OK, I think, for a lint, i.e. opt-in, and when false positives are rare.

[a14n]

There's already a unnecessary_statements rule that lints some cases (see #756). The issue for new Baz() is exactly what @scheglov described: it can have side-effect (eg new Timer(duration, callback)). See also discussions in #615

[pq]

Great discussion, thanks! FWIW I generally dislike side-effecting constructors (they feel like bad grammar to me). That said, there are a few common ones.

I wonder if we shouldn't fold this check into unnecessary_statements? Seems reasonable to allow for the possibility of occasional false-positives in the interest of catching bugs...

As for false-positives, a few thoughts.

1. perhaps we could special case known (core-lib) side-effecting constructors, and possibly
2. consider an annotation to identify exceptions (e.g., something like @impure as discussed for Scala).

[eseidelGoogle]

Couple notes: 1. I filed this from my personal account during some free-time hacking with Dart, this is far from critical for Flutter. :) 2. C++ has a lint/warning like this (which I suspect is more important there due to lack of GC) and yes, when I've used it in the past, you definitely have to mark it as ignored in some places, as it has false positives.

[bwilkerson]

I still believe that we want to aim for zero false positives in lints. I'm also not convinced that it's useful to have an annotation whose only role is to act like // ignore comments, although I could see an argument from a documentation perspective.

[lambda-fairy]

Today I saw a colleague mistype textStyle.copyWith() as textStyle..copyWith() (note the two dots). This made it past code review, and almost ended up in a release.

Another tried list..take() – luckily that was in a test, so had no user impact.

So I think something like Rust's #[must_use] would be useful here.

[bwilkerson]

Today I saw a colleague mistype textStyle.copyWith() as textStyle..copyWith() (note the two dots).

The lint avoid_single_cascade_in_expression_statements would catch these two case.

[lambda-fairy]

That will indeed catch

textStyle..copyWith();

on its own line.

But in the actual example, that expression was passed as an argument (to Text), and I don't think avoid_single_cascade_in_expression_statements covers that.