[cfe] Make `on` edges from mixins count with respect to exhaustiveness

[eernstg]

This issue is a request for implementation of the breaking change described in https://github.com/dart-lang/sdk/issues/53325. This issue is the CFE specific part of https://github.com/dart-lang/sdk/issues/53390.

For the CFE, the requested behavior seems to be implemented already, which means that this issue could be a no-op.

[lrhn]

... but we should add tests to verify this.

[eernstg]

Indeed: https://github.com/dart-lang/co19/issues/2253.

[sgrekhov]

CFE is not perfect. There is a missed exhaustivness error in the following case

sealed class S {}

mixin M on S {}

class C extends S {}

class F implements M {}

void main() {
  S s = F();

  int i1 = switch (s) { C _ => 1 }; // CFE Error: The type 'S' is not exhaustively matched by the switch cases since it doesn't match 'M()'.

  int i2 = switch (s) { C _ => 1, F _ => 2 }; // No error
}

In case of i2 we should expect an error It's funny that if to switch the switches then the error is reported

sealed class S {}

mixin M on S {}

class C extends S {}

class F implements M {}

void main() {
  S s = F();

  int i2 = switch (s) { C _ => 1, F _ => 2 }; // CFE Error: The type 'S' is not exhaustively matched by the switch cases since it doesn't match 'M()'.

  int i1 = switch (s) { C _ => 1 }; // CFE Error: The type 'S' is not exhaustively matched by the switch cases since it doesn't match 'M()'.
}

Tested on Dart SDK version: 3.2.0-edge.9394b30b779296c07971d3692db703c023b2b2a0 (be) (Mon Sep 18 06:44:08 2023 +0000) on "macos_arm64"

cc @eernstg to confirm that it is related issue

[eernstg]

Yes it is, thanks!

[sgrekhov]

The following tests fail in CFE

https://github.com/dart-lang/co19/blob/master/LanguageFeatures/Patterns/Exhaustiveness/exhaustiveness_sealed_A02_t04.dart https://github.com/dart-lang/co19/blob/master/LanguageFeatures/Patterns/Exhaustiveness/exhaustiveness_sealed_A02_t05.dart

[eernstg]

This is nearly working, it would be great if we can close it. However, there is still one surprising behavior:

sealed class S {}
base mixin M on S {}
class C extends S {}
base class F extends S with M {}

void main() {
  S s = F();
  // int i1 = switch (s) { C() => 1 };
  // int i2 = switch (s) { F() => 1 };
  var i4 = switch (s) { C() => 1, F() => 2 };
}

This example causes the front end to report a compile-time error at the declaration of i4 because the initializing expression is non-exhaustive (it doesn't cover M), which is fine.

However, if the line declaring i1 or the line declaring i2 is commented in (or both) then the error about i4 disappears.

@johnniwinther, WDYT, is this a bug?

[lrhn]

I think it looks correct. M is not sealed, so its one subclass F cannot be assumed to exhaust it. (F is also a direct subtype of S, so it at least exhausts itself.) And M cannot be sealed because we don't allow mixins to be sealed.

That makes mixins on sealed classes pretty useless, since you can never exhaust the sealed type.

That is why I wanted mixins on sealed types to not count as a direct subtype for exhaustion, and that we should instead prevent you from implementing them or mixing in on the sealed class itself, so that the only concrete subtypes of the mixin is a mixin application. Those would necessarily be on a subtype of one of the exhausting subclasses of the sealed class, and would therefore be included normally in the exhaustiveness.

Allowing you to have a mixin on a sealed class, and having it count as a direct subtype, including towards exhaustiveness, is pretty much the worst combination we can choose. I'd rather just not allow mixins on sealed classes.

[eernstg]

The point is that the error reported for the line where i4 is declared disappears if the line where i1 or i2 is declared (or both) is commented in (that is, included). It looks like a bug that this error should no longer exist in the i4 line just because those other lines are present.

[lrhn]

Both of those earlier declarations should themselves be errors. It makes sense to possibly not have a later related error downstream from an existing error.

If those earlier errors are reported, obliviously. Then it's a design decision whether one error silenced another.

[eernstg]

Both of those earlier declarations should themselves be errors.

Yes, indeed.

Then it's a design decision whether one error silenced another.

I would expect some errors to remain unreported because they are derived from existing ones:

class C extends int {} // Bogus declaration of a class. An error is reported.

void main() {
  1 as C; // Using the bogus class, can't generate code for this, but no error is reported.
}

This is desirable behavior: It has no value to know that 1 as C can't be right because C is wrong. It is associated with declarations and/or expressions that depend on each other. The expectation is that all the derived errors will go away as soon as the original error (here: the declaration of C) is corrected.

However, in the example I listed the declarations are independent. So it's more like the following:

void main() {
  // String i1 = 1;
  // String i2 = 2;
  String i4 = 4; // Error reported as expected!
}

In this case we do report a compile-time error for multiple locations if we comment in the declarations of i1 and/or i2, because each of them is wrong, independently.

The behavior that I'm calling out amounts to being silent about the error in the declaration of i4 in the case where the declaration of i1 and/or i2 is present, but reporting the error with i4 when the declaration of i1 and i2 are commented out.

OK, in the example whose behavior I'm reporting it is the initializing expression itself which is an error (they are switch expressions whose series of cases isn't exhaustive), but I can't see how that should have this effect.

[eernstg]

@sgrekhov, @johnniwinther and others weighed in on this. The conclusion is that the apparently missing compile-time errors should be considered to be secondary errors (that is, we shouldn't test them).

The problem arises because one faulty switch expression gives rise to results during flow analysis that justify extra assumptions about the scrutinee, and then we don't get some error messages from later switch statements on the same scrutinee because flow analysis thinks things like "s cannot possibly have any other type than C because the previous switch expression would have thrown if s did not have that type".

(This implies that when a switch expression is non-exhaustive, we do get the compile-time error for that, but then the flow analysis assumes that the switch is actually compiled with an extra _ => throw "something" clause. We do not have an obviously better way to deal with the fact that the code at some point has an error, and flow analysis must do something, so there are no plans to change this approach.)

This means that in order to avoid having a bunch of tests that are (potentially, and in a couple of cases actually) testing secondary compile-time errors, we should make sure that the flow analysis doesn't have a long history to work with for each scrutinee.

This could be done by changing the affected tests as follows:

// -------------------- Before

void main() {
  S s = F();
  int i1 = switch (s) { C _ => 1 };
//         ^^^^^^
// [analyzer] unspecified
// [cfe] unspecified

  int i2 = switch (s) { F _ => 1 };
//         ^^^^^^
// [analyzer] unspecified
// [cfe] unspecified
  ...
}

// -------------------- After

void main() {
  {
    S s = F();
    int i1 = switch (s) { C _ => 1 };
//           ^^^^^^
// [analyzer] unspecified
// [cfe] unspecified
  }
  {
    S s = F();
    int i2 = switch (s) { F _ => 1 };
//           ^^^^^^
// [analyzer] unspecified
// [cfe] unspecified
  }
  ...
}

There could be many other ways to fix it, but the point is just that we do not use a scrutinee whose type (promoted or not) has been influenced by erroneous switch expressions earlier in the control flow. It may actually be enough to just assign a new value to s, as in s = F(); just before each of the switch expressions.

[eernstg]

We should then be able to close this issue when (1) the failing tests are adjusted to avoid this flow analysis based surprise, and (2) the updated tests run successfully on the CFE.