Analysis should understand exit function

[rspilker]

Calling exit(int) should be treated by the analyzer as such.

Example:

import 'dart:io';

String test(bool keepRunning) {
  if (keepRunning) {
    return 'OK';
  }
  exit(0);
  print('should not happen');
}

I expect:

[hint] Dead code. (example.dart, line 8, col 3)

Instead I get:

[hint] This function declares a return type of 'String', but doesn't end with a return statement. (example.dart, line 3, col 1)

[lrhn]

While tempting, I fear that it won't be enough in practice, and I think that such a very specific rule isn't worth its own weight. (The analyzer implementors are obviously free to disagree :smile: )

If you make a function printAndExit(s) { print(s); exit(1); } then I wouldn't expect the analyzer to recognize that that function also always exits. Likewise, if you had called reportError() { throw new Error("wohoo"); } instead of exit, your following code would be just as dead, but you it wouldn't be recognized either.

A full "always throws/exits" analysis might be interesting, though, and hooking exit into that could make sense - just hard-code it to be "always throwing". (If we get non-nullable types, and a non-nullable bottom type, then a function with that as return type would automatically be known to not return normally.) As a slightly curious thought, a finally block around an exit call is actually dead code!

[rspilker]

I find it amusing that the ExitDetector doesn't detect the call to exit.

If I would create a pull request, including tests and updates to the documentation, would that make a chance to be accepted?

[bwilkerson]

Lasse is right, this would only be a partial solution, and as such I'm somewhat disinclined to add it.

We've talked in the past about using annotations for this purpose. We could, for example, define an annotation named @alwaysThrows that could be applied to functions that are guaranteed to always throw an exception and one named @alwaysExits that marks functions that always call exit. We'd have to add these annotations to the SDK in order to be able to put the second one on exit.

It would be a complete solution in one sense, but would have to be used everywhere in order to be effective. I don't know how many false positives it would prevent (in part because the answer is dependent on adoption) so I don't know whether it's worth the effort. It might be better to just live with the irony of the current situation. :-)

[rspilker]

If I look at the code in the analyzer, the third annotation would be @alwaysLoops or @neverCompletes.

From my experience in static code analysis in java, and given the already impressive type inference in Dart, I would expect that only a few pieces of library code would need to be annotated, and the rest could be inferred.

I'm happy to let this one go and live with the irony.

[rspilker]

On the other hand, special casing the call to exit looks trivial to me, and current analysis already has precedents for special casing in its loop detection. There would be no false positives.

[shilangyu]

Seems to be already fixed in newer dart thanks to the Never type. Analyzer correctly reports dead code in the given example.

[lrhn]

Agree