Skip to content

proposal: sync_throw_in_async #58920

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

Open
5 tasks
lrhn opened this issue Nov 8, 2022 · 0 comments
Open
5 tasks

proposal: sync_throw_in_async #58920

lrhn opened this issue Nov 8, 2022 · 0 comments
Labels
area-devexp For issues related to the analysis server, IDE support, linter, `dart fix`, and diagnostic messages. devexp-linter Issues with the analyzer's support for the linter package linter-lint-proposal linter-status-pending P3 A lower priority bug or feature request type-enhancement A request for a change that isn't a bug

Comments

@lrhn
Copy link
Member

lrhn commented Nov 8, 2022

sync_throw_in_async.

Description

A non-async function which returns a Future should not also throw synchronously.

Details

Users expect functions to be either synchronous or asynchronous, the latter being recognized by returning a Future.
If a function returns a Future, users may choose to call catchError on the future, and assume that to catch all errors.
However, a non-async function which returns a Future can also choose to throw synchronously instead of
returning future containing an error.
Users will likely not expect that, and won't catch the error.

The lint will trigger on a throw statement in the body of a non-async function which (statically) returns a Future type.

(Does not apply to methods which may return a future only if a type variable ends up bound to a future type. That has its own potential problems, but this lint is not trying to fix those. Such a function is not considered "asynchronous", it's just passing futures around as values.)

Kind

This lint guards against errors by providing consistent and predictable APIs to users, where an asynchronous function (returning a Future) always provides its entire result in that future, and doesn't throw independently of that.
It means callers don't need two layers of error handling, which they'll most likely forget to do.

Good Examples

Future<int> validateParse(String input) async { // Be async.
  if (!isValid(input)) throw FormatException("Bad format"); // Error returned in future.
  return await asyncParse(input);
}

Future<int> validateParse2(String input) {
  // Return a future with the error.
  if (!isValid(input)) return Future.error(FormatException("Bad format"));
  return asyncParse(input);
}

Bad Examples

Future<int> validateParse(String input) { // Not `async`, returns `Future`.
  if (!isValid(input)) throw FormatException("Bad format"); // Throws directly.
  return asyncParse(input);
}

Discussion

This has come up multiple times, and it's a rule that the platform libraries try to enforce (haven't always, and that's also always caused problems).

See #50361 for one example of this behavior not being expected.

It's not a big issue if people use async for all functions which interact with futures, but that's not required. (The discarded_futures lint).

Discussion checklist

  • List any existing rules this proposal modifies, complements, overlaps or conflicts with.

Should not directly affect other lints.

  • List any relevant issues (reported here, the [SDK Tracker], or elsewhere).

#50361 is one example. I'm sure there have been others.

  • If there's any prior art (e.g., in other linters), please add references here.

There are other lints that restrict "something" around async in non-async functions (discarded_futures disallows creating a future in a non-async function). This lint would recognize non-async functions with a Future return type.

  • 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?)

It's, slightly surprisingly, not something we have said explicitly,
It's a general recommendation that I've repeated many times, that a function returning a Future should return all its errors in that future, but it's not written on the Future class, and not easily found in our other async documentation.
(Should probably fix that.)

  • If this proposal is motivated by real-world examples, please provide as many details as you can. Demonstrating potential impact is especially valuable.

The issue above shows one person hitting this. I have seen others.
The problem is usually subtle, because it only shows if an error happens. Most of the time, no error happens, so you won't recognize the issue. The someone, somewhere, uses your library slightly wrong, and they can't catch the error using Future.catchError.

@pq pq added the P3 A lower priority bug or feature request label May 22, 2023
@srawlins srawlins added the type-enhancement A request for a change that isn't a bug label Mar 27, 2024
@devoncarew devoncarew added devexp-linter Issues with the analyzer's support for the linter package legacy-area-analyzer Use area-devexp instead. labels Nov 19, 2024
@devoncarew devoncarew transferred this issue from dart-archive/linter Nov 19, 2024
@bwilkerson bwilkerson added area-devexp For issues related to the analysis server, IDE support, linter, `dart fix`, and diagnostic messages. and removed legacy-area-analyzer Use area-devexp instead. labels Feb 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-devexp For issues related to the analysis server, IDE support, linter, `dart fix`, and diagnostic messages. devexp-linter Issues with the analyzer's support for the linter package linter-lint-proposal linter-status-pending P3 A lower priority bug or feature request type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

5 participants