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

[maBarabas]

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.

[lrhn]

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]

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

[maBarabas]

There is some relevant discussion about whether functions that return a Future should ever throw synchronously in https://github.com/dart-lang/linter/issues/3822. 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.

[stuartmorgan]

See also https://github.com/dart-lang/linter/issues/2946, 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.