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

New lint suggestion: uncaught_async_error_in_try_catch #59364

Open
gmpassos opened this issue Jan 1, 2024 · 4 comments
Open

New lint suggestion: uncaught_async_error_in_try_catch #59364

gmpassos opened this issue Jan 1, 2024 · 4 comments
Labels
analyzer-linter Issues with the analyzer's support for the linter package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. linter-lint-request P4 type-enhancement A request for a change that isn't a bug

Comments

@gmpassos
Copy link
Contributor

gmpassos commented Jan 1, 2024

I suggest the lint uncaught_async_error_in_try_catch (or a better name), for the example bellow:

/// Here the call to [_computeImpl] should trigger a warning,
/// since the `try` block won't catch any error from `_computeImpl`:
Future<int?> compute1(String value) async {
  try {
    return _computeImpl(value) ;
  } catch(_) {
    return null ;
  }
}

/// This is the fixed version, where the `await` ensures
/// that the `try` block will catch errors.
Future<int?> compute2(String value) async {
  try {
    return await _computeImpl(value) ;
  } catch(_) {
    return null ;
  }
}

/// A simple computation that can trigger an [ArgumentError].
Future<int?> _computeImpl(String value) async {
  if (value.isEmpty) {
    throw ArgumentError("Empty `value`");
  }

  return value.length ;
}

Without a lint the issue above is not simple to detect and debug and can generate many bugs.

@eernstg
Copy link
Member

eernstg commented Jan 2, 2024

Good catch, @gmpassos!

The behavior which is described here is a bug, reported in #44395. So the async error should have been caught.

However, a fix for the bug was created already a couple of years ago, and then it was never landed because it gave rise to subtle breakages. In short, we kept the bug because it seemed more disruptive to fix it.

The issue is (finally) being handled in the coming months, as described in this comment.

It may or may not be justified to create a lint as requested here, given that the issue is gone as soon as we have an implementation of dart-lang/language#870, and the code has been migrated to use that. On the other hand, it certainly won't hurt to have a lint like this. It would nudge developers in the direction of pre-migrating their code to the form that it needs to have anyway when dart-lang/language#870 has been implemented.

@gmpassos
Copy link
Contributor Author

gmpassos commented Jan 2, 2024

If, in the future, Dart doesn't allow returning a Future without await inside an async block, it could potentially break a lot of code.

IMHO I consider it a bug only if it's inside a "try" block, as without it (try block), the behavior remains the same with or without await.

It might be challenging to implement this language change, and it could take a while to release it. A lint could help resolve the issue for now, in a simple and compatible way.

Also, consider allowing the return of Future (or anything) without await for a method returning FutureOr to avoid disrupting optimizations, since a method that returns FutureOr usually is attempting to optimize things. See https://pub.dev/packages/async_extension

@bwilkerson bwilkerson added the P4 label Jan 2, 2024
@eernstg
Copy link
Member

eernstg commented Jan 3, 2024

@gmpassos, you might want to add this comment to dart-lang/language#870, it seems to be more relevant there.

@gmpassos
Copy link
Contributor Author

gmpassos commented Jan 3, 2024

THX. I have just commented there.

@srawlins srawlins added the type-enhancement A request for a change that isn't a bug label Apr 3, 2024
@devoncarew devoncarew added analyzer-linter Issues with the analyzer's support for the linter package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. labels Nov 20, 2024
@devoncarew devoncarew transferred this issue from dart-lang/linter Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-linter Issues with the analyzer's support for the linter package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. linter-lint-request P4 type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

5 participants