proposal: use_build_context_synchronously should lint unsafe usages of sync then(), catchError() and whenComplete() callbacks

[navaronbracke]

use_build_context_synchronously

Description

"Don't use BuildContexts across async gaps inside Future callbacks" (This wording can definitely be better)

Details

Currently the use_build_context_synchronously warns if a BuildContext is used after an await, without a mounted check on said context. The same rule also applies to uses inside Future.then(), Future.catchError() and Future.whenComplete(), but only if said functions use awaits themselves.

A developer could write code using the then/catchError/whenComplete callbacks and not use any async/await keyword at all, but the underlying rule still applies.

I filed #4892 in the past, where I did mention the callbacks as well, but for setState((){}) uses.

Kind

This enhancement would guard against errors.

Bad Examples

Future<int>.delayed(Duration(seconds: 1), (_) {
  return 42;
}).then((int value) {
 Navigator.of(context).pop();
});

Future<int>.delayed(Duration(seconds: 1), (_) {
  return 42;
}).catchError((Object error, StackTrace stackTrace) {
 Navigator.of(context).pop();
});

Future<int>.delayed(Duration(seconds: 1), (_) {
  return 42;
}).whenComplete(() {
 Navigator.of(context).pop();
});

Good Examples

Future<int>.delayed(Duration(seconds: 1), (_) {
  return 42;
}).then((int value) {
 if (!context.mounted) return;

 Navigator.of(context).pop();
});

Future<int>.delayed(Duration(seconds: 1), (_) {
  return 42;
}).catchError((Object error, StackTrace stackTrace) {
 if (!context.mounted) return;

 Navigator.of(context).pop();
});

Future<int>.delayed(Duration(seconds: 1), (_) {
  return 42;
}).whenComplete(() {
 if (!context.mounted) return;

 Navigator.of(context).pop();
});

Discussion

Add any other motivation or useful context here.

Discussion checklist

• List any existing rules this proposal modifies, complements, overlaps or conflicts with.
• List any relevant issues (reported here, the SDK Tracker, or elsewhere).
• If there's any prior art (e.g., in other linters), please add references here.
• If this proposal corresponds to Effective Dart or Flutter Style Guide advice, please call it out. (If there isn't any corresponding advice, should there be?)
• If this proposal is motivated by real-world examples, please provide as many details as you can. Demonstrating potential impact is especially valuable.

[bwilkerson]

@goderbauer

[goderbauer]

I agree with the issue author. Ideally, we'd warn in those cases as well.

[srawlins]

What is the triggering condition here? A function literal inside a Future.then callback? Plus a few others? What about a function otherwise:

void f(int value) {
 Navigator.of(context).pop();
}

Future<int>.delayed(Duration(seconds: 1), (_) {
  return 42;
}).then(f);

In static analysis we cannot track how functions are called through an arbitrary number of functions. It would not be hard to just trigger on function literals passed to Future.then (plus a few others), but very hard to track arbitrary functions being passed to Future.then. Is the former enough?

[navaronbracke]

It would not be hard to just trigger on function literals passed to Future.then (plus a few others)

This would already be a big step in the right direction.

[goderbauer]

It would not be hard to just trigger on function literals passed to Future.then (plus a few others), but very hard to track arbitrary functions being passed to Future.then. Is the former enough?

That's similar to how the rule works today for the async/await case, right? For example, in the following code you will not get any lints on the problematic use of context inside the methodThatWillUseContext?

await future;
methodThatWillUseContext();

So, I think just warning on this in function literals would be a good idea and match what we do in other cases.

[srawlins]

That's similar to how the rule works today for the async/await case, right?

That's right. I hadn't thought about how this request mirrors the existing code pretty well, and the same limitation would apply. This should be technically simple, but we'll see where the dragons lie when applying it to the flutter repos :D

[srawlins]

This could potentially apply to gobs of Dart SDK function callbacks. Here's what I've got so far:

• new Future, as the first positional argument,
• new Future.delayed, as the second positional argument,
• new Future.microtask, as the first positional argument,
• Future.catchError, as the first positional argument, or the test
  named argument,
• Future.onError, as the first positional argument, or the test
  named argument,
• Future.then, as the first positional argument, or the onError named
  argument,
• Future.timeout, as the onTimeout named argument,
• Future.whenComplete, as the first positional argument,
• Future.doWhile, as the first positional argument,
• Future.forEach, as the second positional argument,
• Future.wait, as the cleanUp named argument,
• new Stream.multi, as the first positional argument,
• new Stream.periodic, as the second positional argument,
• Stream.any, as the first positional argument,
• wow many more Stream methods...

[srawlins]

Fixed with dart-lang/sdk@a6762dd