When flow analysis can't figure out that a switch is exhaustive, user experience is poor

[stereotype441]

Looking at dart-lang/sdk#51926, it occurs to me that we may be boxing users in to a kind of unfortunate situation where the only way to avoid an error is with an "unreachable code" warning. Consider a switch statement like this:

int addFirstTwoValues(List<int> x) {
  switch (x) {
    case []: return 0;
    case [a]: return a;
    case [a, b, ...]: return a + b;
  }
}

This code will produce a compile error, because flow analysis is not smart enough to understand that the switch is exhaustive. Exhaustiveness checking will understand that the switch is exhaustive, but since it happens later than flow analysis, it's too late. (In theory we could suppress the error, but in general there's no way to un-do the other conclusions that flow analysis may have drawn based on its assumption that the switch is non-exhaustive, so I don't think it's a good idea).

To avoid the error, someone would have to do something like this:

int addFirstTwoValues(List<int> x) {
  switch (x) {
    case []: return 0;
    case [a]: return a;
    case [a, b, ...]: return a + b;
    default: throw 'unreachable';
  }
}

But now they will get an unreachable code warning for the default case (which may make them tempted to remove it!)

This seems bad and I'm wondering if there's some way to give the user a better experience here.

[stereotype441]

CC @dart-lang/language-team

[lrhn]

The obvious answer is that everything must happen in the same step, as usual. (Type inference influences control flow, control flow influences type inference, and now exhaustiveness influences, and is influenced by, both.)

If we can't do that (and even if we could, probably not before Wednesday), maybe delaying the decision is the right choice.

This looks like it should only be an issue for switches with no code after them.
If there is code afterwards, we should assume it's reachable and treat it as such, even if we cannot find a way into it. (Analyze all the code!)

A switch with no default/catch-all clause, and where none of the cases can complete normally, can be considered as being in a quantum state of either being exhaustive or not, until exhaustiveness analysis has happened.

If there is no code after it, and the function cannot return without a value, then that back-propagates into a "must be exhaustive" requirement, which the later exhaustiveness algorithm then validates.
(If it can return without a value, then it really doesn't matter.)

If there is code after it, that code is either unreachable or reachable, and we treat it as being reachable. If the switch is
not exhaustive, that's the code that's going to run, and it must be valid by itself.
That should probably affect type inference. The code is only reachable if the switch is not exhaustive, which means that the types from before the switch are the ones propagated to the continuation.
If the switch turns out to be exhaustive, it's just dead code, but that's not an error by itself.

It's a more interesting case, for type inference, it the switch also has cases that complete normally.

num x = 42;
switch (x) {
  case double(): return;
  case int(): x = 37; break;
}
// What is the type of `x` here.
// `int` if exhaustice, `num` if not.

If num had not been sealed, then we wouldn't know that x would always be promoted to int after the switch.

Later we could go back and check that the switch is exhaustive, but too late to promote x.

(Is there any reason we cannot decide exhaustiveness right after analyzing every switch case? We've already looked at all the case pattern types, so we should know all about them at that time, including whether they're sealed or enums.
Is the issue just how things are currently coded?)

[leafpetersen]

Would the user get a dead code warning if they were to instead add a meaningless "return" or "throw" after the switch? If not, that might be the best workaround.

In terms of avoiding this entirely, it seems difficult. In principle, you could possibly defer the error about a missing return until after exhaustiveness. I think you'd have to record a "possible" error, something like "emit an error if any of these switches fails to be exhaustive". That seems like it might require some somewhat tricky book-keeping. Also, I'd be slightly worried that there are other errors you'd need to track, but maybe it's just this one?

[stereotype441]

Would the user get a dead code warning if they were to instead add a meaningless "return" or "throw" after the switch? If not, that might be the best workaround.

Yeah, that works for the exact scenario I opened the issue with. But in the more general case, we need to worry about all the things that flow analysis might conclude based on the switch's exhaustiveness. Many of them would have easy workarounds too, but there's no one-size-fits-all workaround. For example, consider:

void printSumOfFirstTwoValues(List<int> x) {
  int sum;
  switch (x) {
    case []: sum = 0;
    case [a]: sum = a;
    case [a, b, ...]: sum = a + b;
  }
  print('The sum is $sum');
}

In this case, since flow analysis doesn't understand that the switch is exhaustive, it will complain that at the time of the call to print, sum is not definitely assigned. The user could work around that by giving sum a default value or marking it late.

Here's an example involving type promotion:

void foo(List<int> x, Object o) {
  late int sum;
  switch (x) {
    case []: sum = (o as int) + 0;
    case [a]: sum = (o as int) + a;
    case [a, b, ...]: sum = (o as int) + a + b;
  }
  print('The sum is $sum');
  print('$o of that came from `o`');
  print('The remainder is ${sum - o}');
}

In this case, since flow analysis doesn't understand that the switch is exhaustive, it doesn't promote o to int, so it will complain at the time of the final call to print that Object is not valid to pass to int.operator-. The user could work around that by replacing o with (o as int) in that print statement.

In terms of avoiding this entirely, it seems difficult. In principle, you could possibly defer the error about a missing return until after exhaustiveness. I think you'd have to record a "possible" error, something like "emit an error if any of these switches fails to be exhaustive". That seems like it might require some somewhat tricky book-keeping. Also, I'd be slightly worried that there are other errors you'd need to track, but maybe it's just this one?

In principle, anything type inference does could potentially be affected, because reachability affects type promotion, and type promotion affects type inference.

[leafpetersen]

In principle, anything type inference does could potentially be affected, because reachability affects type promotion, and type promotion affects type inference.

Yes, I was ignoring the flow analysis itself, because my understanding was that this was just POR. That is, we talked about this before at some point, and my understanding was that we decided that:

• For switch expressions, we just assume they are exhaustive, because exhaustiveness later on will validate this
• For switch statements over exhaustive types, ditto
• For switch statements over non-exhaustive types, if flow analysis couldn't show exhaustiveness it would just assume not.

In general, as you note, this is mostly observable via flow analysis doing a less than ideal job of promoting types. Not everything we'd like, but probably ok?

The missing return case does seem more obnoxious to me though. And possibly also the missing "definite assignment" case, though that's probably relatively rare. My impression is that people don't actually take advantage of definite assignment that much.

Ideally I suppose, we'd do exhaustiveness during flow analysis so that this didn't arise, but short of that, I don't have a good suggestion for how to do better.

[munificent]

The missing return case does seem more obnoxious to me though.

In practice, I don't think this will be a huge pain point.

This only comes in play when:

• You have a function with a non-void return type.
• It ends with a switch statement (on type that isn't always-exhaustive).
• Every case of the switch statement has a return.
• There is no return after the switch statement.

In most of those cases, you can hoist the returns out of the cases and turn it into a switch expression, like:

int addFirstTwoValues(List<int> x) {
  return switch (x) {
    [] => 0,
    [var a] => a,
    [var a, var b, ...] => a + b,
  };
}

This doesn't always work gracefully since some of the switch statement cases could have multiple statements before the final return, but I think it will address a lot of them.

[leafpetersen]

Worth mentioning that there is one obvious solution to this which is to require all switch statements to be exhaustive, just as we do for switch expressions. We've considered and discarded this before for various reasons, but thought I'd mention it again.

[lrhn]

I think the optimal solution is to include exhaustiveness analysis into the switch control flow analysis. That should also give the correct promotion behavior.

If control flow analysis itself cannot determine that a switch is exhaustive (no default clause, or catch-all clause for the matched value type), and it doesn't have to be exhaustive (so a switch statement, or an eventual switch element), it runs the exhaustiveness algorithm after the last case has been type-inferred.
(If the switch has to be exhaustive, and isn't obviously, probably run the exhaustiveness algorithm at the same time anyway, to give the error early. But it can be delayed then.)

Only if the switch is still not exhaustive, the flow-through control flow path is added, which we currently add in these cases where the flow-analysis cannot see exhaustiveness.
Then type inference can continue as normal.

That seems both optimal and consistent. Is there any problem with doing the exhaustiveness checking at that point?

[stereotype441]

That seems both optimal and consistent. Is there any problem with doing the exhaustiveness checking at that point?

Yeah, I agree that this would be optimal and consistent. And I would be open to doing it in a future release of Dart. The main blocker I'm aware of is #2807 (to support modular compilation with Bazel, it needs to be possible to provide each modular compilation step with a description of how all possible calls to .fromEnvironment should behave).

[lrhn]

Which is why we want to let "unresolved constants" not count towards exhaustiveness. Makes sense.

Until we actually do that, we're between a rock and a hard place - we can't do a good job with type inference before we know all the constants, but we haven't specified a way out of depending on the constants.
Exhaustiveness is one more way constants can affect control flow, but one where it's very visible because it depends on the actual value, not just the static type of the constant.

That was probably true before as well, but only for switches over enum values.

Or rather, we avoided that by being obtuse:

enum E {a, b}
void main(){
  const p = bool.fromEnvironment("not.there", defaultValue: false);
  const x = p ? E.a : E.b;
  const b = E.b;
  print(p); // false
  int i;
  switch (x) {
    case E.a: i = 1; print("1"); break;
//    case E.b: i = 2; print("2"); break;  
    case b: i = 3; print("3"); break;
    case x: i = 4; print("4"); break;
  }
  print(i);
}

Here we have exhaustiveness (i definitely assigned) if I uncomment case E.b:, but if not, the case b doesn't count towards exhausting. And that's even though the analyzer is quite capable of warning me that b and E.b is the same value. That x doesn't count isn't surprising when b doesn't. That b doesn't count is surprising.

The new exhaustiveness is based on values, not syntax, because the spaces are defined in terms of values, so we can't ignore the value of b or x any more.

[Mike278]

Should the issue description say:

Exhaustiveness checking will understand that the switch is exhaustive

instead?

[munificent]

Good catch, fixed!

[stereotype441]

This issue has come up again during the work on dart-lang/sdk#54575. I'm currently working around it by suppressing the "unreachable switch default" warning in the circumstance where the type of the switch scrutinee isn't an "always exhaustive" type. But this is not ideal, because it means that in some (fortunately rare) circumstances, the user is not warned about code that is indeed dead (and that the analyze can, in principle, tell is dead). Further discussion here.