Skip to content

Add a new lint to catch dead code in try catch around syncronous code working with futures #59111

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
maBarabas opened this issue Apr 14, 2023 · 4 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-request P3 A lower priority bug or feature request type-enhancement A request for a change that isn't a bug

Comments

@maBarabas
Copy link
Contributor

Describe the rule you'd like to see added and to what rule set
I have a function like this in a library wrapper class:

/// Read from a characteristic
///
/// Throws [MyException] on error.
Future<List<int>> readCharacteristic(
    QualifiedCharacteristic characteristic,
 ) {
  try {
    return _impl.readCharacteristic(characteristic);
  } on ImplementationException catch (e, stacktrace) {
    throw MyException();
  }
}

This does not work as intended, because the function is not marked async. Instead of catching the error from the implementation, it returns the Future.error value containing an ImplementationException.

The correct implementation would look like this:

/// Read from a characteristic
///
/// Throws [MyException] on error.
Future<List<int>> readCharacteristic(
    QualifiedCharacteristic characteristic,
 ) async {
  try {
    return await _impl.readCharacteristic(characteristic);
  } on ImplementationException catch (e, stacktrace) {
    throw MyException();
  }
}

Additional context
I would like some sort of lint message for the function, as it contains dead code in the try catch.

I'm not sure if this can be generalised for all functions. Dart can't really know if I wanted to catch a synchronous or async error with the try catch. Maybe having a lint that forces all functions returning Futures to be async would help? I would prefer if there were no syncronous functions operating on futures at all in the project to avoid issues like this.

@devoncarew devoncarew transferred this issue from dart-archive/lints Apr 26, 2023
@lrhn
Copy link
Member

lrhn commented Apr 28, 2023

It's not that the catch cannot fire from an error thrown by _impl.readCharacteristic(characteristic);. The code is not definitely dead.

It's reasonable, as a heuristic, to assume that a function returning Future probably doesn't throw synchronously, so as a lint it would make sense to say that the catch does not look reachable.

@bwilkerson
Copy link
Member

There would probably be fewer false positives if we only reported unawaited futures inside the try block of a try-catch statement.

@bwilkerson bwilkerson added P3 A lower priority bug or feature request linter-lint-request labels May 5, 2023
@maBarabas
Copy link
Contributor Author

There is some relevant discussion about whether functions that return a Future should ever throw synchronously in
#58920. The author there seems to prefer if functions returning a Future will always deliver their errors in the Future. We can make the same assumption for this lint and warn about any synchronous try catch around functions returning Futures.

@srawlins srawlins added the type-enhancement A request for a change that isn't a bug label Mar 29, 2024
@stuartmorgan-g
Copy link
Contributor

See also #58510, which is related (I found both of these while checking to see if the lint I wanted had already been filed). The lint proposed there might be a way to address the same underlying use case as this one, since it's likely not the case that the code is dead that's the main problem here; i.e., if the code were:

Future<List<int>> readCharacteristic(
    QualifiedCharacteristic characteristic,
 ) {
  try {
    doSomethingSynchronousThatCouldThrowImplementationException();
    return _impl.readCharacteristic(characteristic);
  } on ImplementationException catch (e, stacktrace) {
    throw MyException();
  }
}

the catch would definitely not be dead, but it still probably wouldn't be what the author intended.

@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-request 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

6 participants