Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lint on unawaited futures #731

Open
gnprice opened this issue Jun 12, 2024 · 2 comments
Open

Lint on unawaited futures #731

gnprice opened this issue Jun 12, 2024 · 2 comments
Assignees
Labels
a-tools Our own development tooling, scripts, and infrastructure

Comments

@gnprice
Copy link
Member

gnprice commented Jun 12, 2024

There's a pair of lint rules in the Dart analyzer that detect when we have a Future and neglect to await it:
https://dart.dev/tools/linter-rules/unawaited_futures
https://dart.dev/tools/linter-rules/discarded_futures

(Despite the name, unawaited_futures doesn't cover one major class of unawaited futures: those where the enclosing function isn't even async. Those are covered by discarded_futures.)

It'd be good to enable those. The bugs they catch can be pretty subtle otherwise.

@gnprice gnprice added the a-tools Our own development tooling, scripts, and infrastructure label Jun 12, 2024
@gnprice gnprice added this to the Beta 3: Summer 2024 milestone Jun 12, 2024
@PIG208 PIG208 self-assigned this Sep 10, 2024
@PIG208
Copy link
Member

PIG208 commented Oct 1, 2024

Related to discarded_futures: dart-lang/linter#3739

@gnprice
Copy link
Member Author

gnprice commented Oct 2, 2024

Related to discarded_futures: dart-lang/linter#3739

Expanding on that: Zixuan and I looked at the output of this lint rule yesterday, and found that it probably won't make sense for us to turn it on.

  • It found a fair number of places where we're choosing to start some asynchronous work and then move on without awaiting it, which would benefit from an explicit unawaited. This is useful and is a motivation to adopt the lint rule, if we can keep its noise level down.

    (Though I believe unawaited won't actually currently help for that rule, due to the issue at the end below.)

  • Then there are a fair number of calls to just a couple of functions from the Flutter API, like Navigator.push, where the return type is a Future but it isn't meant to be awaited by most callers — the work named by the function is already complete before the function returns, and the future is one that will complete when some later event happens.

    This source of noise is shared with unawaited_futures; more discussion at: lightbox: Replace onDismiss with await showErrorDialog #937 (comment)
    In short, I see this as a problem with the APIs in question: in this situation the function should return some object that has a Future as a field, not a Future itself. (To put it more neutrally: it's a mismatch between those functions' API choices, and an API convention that is implicitly expected by the unawaited_futures and discarded_futures lint rules. I think that convention is a good one.) And even without changes in Flutter upstream, we can handle this by making our own small wrappers around the handful of offending functions.

    So these first two categories are no reason not to adopt the lint rule.

  • But then there are also a lot of places where we invoke some function that returns a Future, and then we return the future. These futures are not "discarded" in any reasonable meaning of that word — they go right to the function's caller, just as effectively as if the function had said async and await. And yet the lint complains about them.

    Checking the upstream tracker, it turns out the problem is more general than that: if you call a function returning a Future, and then you pass the result to some other function that consumes a Future, the lint complains about that too. This makes it impossible to use a variety of perfectly reasonable APIs, like FutureBuilder and Stream.fromFuture, without silencing the lint. That tracker has a steady stream of reports of this issue; the one with the most discussion seems to be this one:
    Possible false positive in discarded_futures lint dart-lang/linter#3739

    The rule's documentation accurately describes its actual behavior. But it's not clear anybody wants the rule's actual behavior; what there's clear demand for is the behavior the rule's name suggests, of spotting discarded futures with some reasonable rate of false positives.

    Hopefully at some point we'll get a lint rule that behaves like that (either as an update to discarded_futures, or as a new rule with a different name). Until then, it won't make sense for us to adopt discarded_futures.

gnprice added a commit to gnprice/zulip-flutter that referenced this issue Oct 5, 2024
This brings the file test/api/core_test.dart up to being clean
against the `unawaited_futures` lint rule, toward zulip#731.

It does so with a smaller diff, and simpler resulting code,
than by adding `await` on each of the futures.  For comparison:
  zulip#934 (comment)
gnprice added a commit to gnprice/zulip-flutter that referenced this issue Oct 7, 2024
This brings the file test/api/core_test.dart up to being clean
against the `unawaited_futures` lint rule, toward zulip#731.

It does so with a smaller diff, and simpler resulting code,
than by adding `await` on each of the futures.  For comparison:
  zulip#934 (comment)
PIG208 pushed a commit to PIG208/zulip-flutter that referenced this issue Oct 8, 2024
This brings the file test/api/core_test.dart up to being clean
against the `unawaited_futures` lint rule, toward zulip#731.

It does so with a smaller diff, and simpler resulting code,
than by adding `await` on each of the futures.  For comparison:
  zulip#934 (comment)
gnprice added a commit to gnprice/zulip-flutter that referenced this issue Oct 8, 2024
This brings the file test/api/core_test.dart up to being clean
against the `unawaited_futures` lint rule, toward zulip#731.

It does so with a smaller diff, and simpler resulting code,
than by adding `await` on each of the futures.  For comparison:
  zulip#934 (comment)
PIG208 added a commit to PIG208/zulip-flutter that referenced this issue Oct 10, 2024
Partially fixes: zulip#731

Signed-off-by: Zixuan James Li <[email protected]>
PIG208 added a commit to PIG208/zulip-flutter that referenced this issue Oct 11, 2024
Fixes-partly: zulip#731

Signed-off-by: Zixuan James Li <[email protected]>
PIG208 added a commit to PIG208/zulip-flutter that referenced this issue Oct 11, 2024
Fixes-partly: zulip#731

Signed-off-by: Zixuan James Li <[email protected]>
PIG208 added a commit to PIG208/zulip-flutter that referenced this issue Oct 11, 2024
Fixes-partly: zulip#731

Signed-off-by: Zixuan James Li <[email protected]>
PIG208 added a commit to PIG208/zulip-flutter that referenced this issue Oct 11, 2024
Fixes-partly: zulip#731

Signed-off-by: Zixuan James Li <[email protected]>
PIG208 added a commit to PIG208/zulip-flutter that referenced this issue Oct 11, 2024
Fixes-partly: zulip#731

Signed-off-by: Zixuan James Li <[email protected]>
PIG208 added a commit to PIG208/zulip-flutter that referenced this issue Oct 11, 2024
Fixes-partly: zulip#731

Signed-off-by: Zixuan James Li <[email protected]>
PIG208 added a commit to PIG208/zulip-flutter that referenced this issue Oct 11, 2024
Fixes-partly: zulip#731

Signed-off-by: Zixuan James Li <[email protected]>
gnprice pushed a commit to PIG208/zulip-flutter that referenced this issue Oct 12, 2024
Fixes-partly: zulip#731

Signed-off-by: Zixuan James Li <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-tools Our own development tooling, scripts, and infrastructure
Projects
Status: No status
Development

No branches or pull requests

2 participants