Never return type is not respected in missing_return checks for opted out libraries

[jakemac53]

This is causing some problems for google3, specifically string_scanner migrated the error method to return Never and removed the @alwaysThrows annotation. (see here).

This is causing a problem internally at at least one call site in the glob package, which is not opted in:

ERROR: <third party>/glob/lib/src/parser.dart:95
  This function has a return type of 'int', but doesn't end with a return statement. #missing_return

Target //<third party>/glob:glob failed Dart checks.

cc @leafpetersen @davidmorgan

[jakemac53]

Simpler repro:

Given a.dart:

// @dart=2.10
Never error() => throw StateError('broken');

And b.dart:

// @dart=2.9
import 'a.dart';

int foo() {
  error();
}

Running dartanalyzer --enable-experiment=non-nullable . gives:

  hint • This function has a return type of 'int', but doesn't end with a return statement. • b.dart:4:5 • missing_return

Internally we treat this hint as an error, so it will make migrations difficult.

[leafpetersen]

cc @pq @bwilkerson It seems to me that it would be reasonable to treat a return type of Never as equivalent to having the annotation @alwaysThrows, even in opted out code, WDYT?

[bwilkerson]

That sounds reasonable to me too. The annotation is just a hint, and Never is an equally valid hint even if the runtime semantics aren't being enforced yet.

[jakemac53]

Any idea how quickly this can be implemented? This is currently going to block the next SDK roll internally, so we need to figure out if we need to add the annotation back externally or not (in multiple packages, and tracking them all down will take some work that we would like to avoid).

[bwilkerson]

I believe the fix is fairly local. I think we just need to add a check of the return type to the condition here (https://github.com/dart-lang/sdk/blob/master/pkg/analyzer/lib/src/dart/resolver/exit_detector.dart#L426). I'm about to go into a meeting, but can look at it after if nobody has assigned this issue to themselves before that.

[leafpetersen]

Another possibility would be to temporarily disable that hint/lint from breaking the build. cc @davidmorgan

[scheglov]

Not so easy, LEGACY_ERASURE replaces Never return type with Null.
I guess we could break into the "legacyfied" ExectaubleElement and get the original return type.

[scheglov]

https://dart-review.googlesource.com/c/sdk/+/155500

[scheglov]

There seems to be pre-existing failing flutter-analyze-try, and front-end-linux-release-x64-try is down.
So, I force-submitted my CL.