Sdk: Static analyzer to ensure every Future is await-ed for

[dgrove]

many of us lived in synchronous world most of the time and it's very easy to forget await on a future, e.g.

await initializeAbc();
workOnAbc(); // this typically assumes Abc is initialized but it is not without the await.

It typically gets into race condition without await and can be in-deterministic and therefore difficult to debug. It will be best if we could have static analyzer to alert us.

[bwilkerson]

We need to define the rules for the hint/lint a little more carefully. Here's an example:

Within a function body that is marked as being async, any invocation of a function (including methods and getters) that returns a Future must be wrapped in an await expression.

Does that cover the cases you're thinking of? How many false positives is a rule like that likely to generate?

[srawlins]

cc @DrMarcII @goderbauer

[zoechi]

I think it's normal to have calles to async functions where you don't want to await.
For example assignment of event handlers to streams.

[llamadonica]

My lint would blow up...

There are many times that I call a function, get back a future, and then await at a later time or never at all or. For instance deleting a resource on my database. I don't care when it happen, just that it'll happen sometime.

[jmesserly]

@zoechi ... can you give more info about that example? Stream.listen returns a StreamSubscription ... it operates synchronously in attaching your event handler. AFAIK.

@llamadonica ... for a database delete, wouldn't you want to wait, e.g in case an error happens so it bubbles from the right place? Or if something else tries to add that record, I can imagine scenarios where a race condition would happen because of the missing await.

[llamadonica]

It swings both ways. There are cases where you await, cases where you never await, and cases when you await, but at a later time.

This particular situation is on a shelf server. Lots of legitimate awaits. The resource being deleted is just every-day session cleanup that is triggered as part of a request, and shouldn't hold up a request by itself. If there's an error it'll be logged, but not sent to the end user. Race conditions are possible here, but harmless and not completely avoidable since multiple requests could be coming in at a time anyway.

[zoechi]

@jmesserly you are right StreamSubscription is not a future.

I still do regularily something like

await callAsyncA();
await callAsyncB();
callAsyncC().then((e) => doSomething, onError: (e) => handleError);
await callAsyncD();

I _use_ await most of the time but
IMHO not being able to occasionally not to wait for a future to complete somehow defeats the purpose of async programming.

[rkj]

Using Future.then is waiting for a future to complete. The problematic code would be:

callAsyncC();

where client was not aware that the code is async and ignored this aspect hoping that it would just invoke. Another dangerous case is when someone changes void foo() method to Future<void> foo() because they use await inside - and suddenly clients are broken, and there is no warning nor error from the compiler.

[zoechi]

@rjk from this issues POV it shouldn't matter whether .then(...) is appended or not, just the fact this call returns a future which is not awaited for. I just tried to make clear that not awaiting a future is not the same as fire-and-forget, instead its fire-and-care-somewhere-else.

Changing from non-Future to Future is a great example!

[jmesserly]

ah, yeah, that's a good point on Future.then

[floitschG]

As discussed on the mailing-list: variables called "_" should never have "unused variable" hints.
This would be an easy way to silence spurious warnings.

await callAsyncA();
await callAsyncB();
var _ = callAsyncC().then((e) => doSomething, onError: (e) => handleError);
await callAsyncD();

[sanfordredlich]

We'd like to be able to enable this specifically for tests because in tests almost everything that returns a Future should be awaited. Would that be possible?

[sanfordredlich]

Sorry, let's make it a little bigger: we'd like the warning for everything that's outside the application under test. So the PageObjects and the test itself, at the moment.

[floitschG]

This is such a common mistake, that there should always be a hint, if a future isn't awaited for. However, there should be an easy way to disable the hint if necessary (like by assigning it to _, or similar).

[zoechi]

+1 for _

[BlackHC]

This is nice. However, if you go the way of enforcing awaits for returned Futures in async methods.... why not just get rid of await and auto-await?

Re: unused variables: could we also just cast a returned Future to `Object' instead?

callAsyncB() as Object;

Adding unused variables is a bit cumbersome and might trigger warnings of its own, doesn't it?

[zoechi]

@BlackHC It's not about enforcing await. It's about getting a hint if you actually did want to await but forgot to add await and a way to disable the hint if you don't want to await. Variables named _ are already ignored by some checks.

[ochafik]

For the record: I've prototyped a simple heuristic in sCiSSors (also see visitor, tests).

  Future fut() => null;

  foo()       { fut(); }           // OK: assuming fire-and-forget semantics.
                                   //     Could consider a hint here.
  foo() async { fut(); }           // Warning
  foo() async {
    // ignore: UNAWAITED_FUTURE
    fut();                         // OK: ignored
  }
  foo() async { await fut(); }     // OK
  foo() async { var x = fut(); }   // OK
  foo() async {
    new Future.delayed(d);         // Warning
    new Future.delayed(d, bar);    // OK: special case
  }
  foo() async {
    var map = <String, Future>{};
    map.putIfAbsent('foo', fut()); // OK: special case
  }

It only triggers for non-assignment expression statements in async function bodies, with only a couple of special cases.

I ran it on a medium-large codebase and found many bugs (especially in tests) with very few false positives (which can be ignored with the syntax shown above).

cc/ @leafpetersen

[devoncarew]

@ochafik, is this something you'd be willing to contribute to the linter? (/cc @pq)

[ochafik]

Hey @devoncarew, I've actually already contributed unawaited_futures to the linter two months ago :-)

https://github.com/dart-lang/linter/pull/256

Still wish this were somehow turned on by default in the analyzer, but at least now I can enable it with an analysis_options file.

[sanfordredlich]

Cool, why not turn it on by default, at least in tests? It is a common
problem, especially for beginners.

On Mon, Aug 15, 2016 at 12:57 AM Olivier Chafik [email protected]
wrote:

Hey @devoncarew https://github.com/devoncarew, I've actually already
contributed unawaited_futures to the linter two months ago :-)

dart-lang/linter#256 https://github.com/dart-lang/linter/pull/256

Still with this were somehow turned on by default in the analyzer, but at
least now I can enable it with an analysis_options file.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/dart-lang/sdk/issues/24171#issuecomment-239747954,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABJz6d60gDwx32G3mJslrBJ1vdyEN977ks5qgBvWgaJpZM4FwG1y
.

[ochafik]

Side-note: not sure ignoring lints works well in the analyzer (would need to check before turning this on by default, so people have a way out), last time I checked it didn't.

[bwilkerson]

We have, so far, taken the approach that lints should be opt-in, so, no, there is no mechanism for disabling lints.

[bwilkerson]

Given that we have a lint for this now, I'm going to close this issue.