Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Declaration order confuses CFE's notion of sealed when mixins involved #52048

Closed
stereotype441 opened this issue Apr 15, 2023 · 4 comments
Closed
Assignees
Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues. P1 A high priority bug; for example, a single project is unusable or has many test failures

Comments

@stereotype441
Copy link
Member

The following program is accepted by both the CFE and the analyzer:

main() {
  f(B());
}

f(A a) {
  switch (a) {
    case B():
      print("It's a B");
  }
}

sealed class A {}

class B extends A with M {
  B();
}

mixin M {}

However, if you swap the order of declaration of classes A and B, i.e.:

main() {
  f(B());
}

f(A a) {
  switch (a) {
    case B():
      print("It's a B");
  }
}

class B extends A with M {
  B();
}

sealed class A {}

mixin M {}

Then you get the following error from the CFE:

../../tmp/proj/test.dart:6:11: Error: The type 'A' is not exhaustively matched by the switch cases since it doesn't match '_B&A&M()'.
 - 'A' is from '../../tmp/proj/test.dart'.
Try adding a default case or cases that match '_B&A&M()'.
  switch (a) {
          ^

Reproduced as of 90d84c8.

If we can fix this before the release, I suspect it will be worth cherry-picking to stable.
CC @johnniwinther

@stereotype441 stereotype441 added P1 A high priority bug; for example, a single project is unusable or has many test failures area-front-end Use area-front-end for front end / CFE / kernel format related issues. labels Apr 15, 2023
@leafpetersen
Copy link
Member

If this is just restricted to mixing in things onto a sealed class (why do we even have that lever?) I think I'm on the fence about cherry picking, it doesn't feel likely to be a widespread problem. Definitely worth fixing though, and we can then consider. Separately, the error message there is pretty terrible.

@stereotype441
Copy link
Member Author

@leafpetersen

If this is just restricted to mixing in things onto a sealed class (why do we even have that lever?)...

I guess our intuitions disagree about how useful it is to do that. That's fine--to each their own.

But for what it's worth, I didn't find this bug by deliberately poking at edge cases. I found it because I was trying out patterns in a side project over the weekend, and it arose naturally from a genuine attempt to write a useful program 😃

(The details of what I was trying to do, if you're curious: I had a sealed class with several derived classes, and I had a mixin (in a different library) that I could use to add some extra debugging functionality to a class. I wanted to add the extra debugging functionality to some of the derived classes but not all of them. I was using the standard "sort declarations" feature in the IDE to keep my file organized, and so I happened to hit on the sort order that provoked the bug.)

@stereotype441
Copy link
Member Author

Since Kallen is on vacation this week, I'll look into this.

@stereotype441
Copy link
Member Author

This should fix it: https://dart-review.googlesource.com/c/sdk/+/297901

copybara-service bot pushed a commit that referenced this issue Apr 26, 2023
…kSupertypes phase.

Previously this step happened during `buildOutlineNodes`, but since
`buildOutlineNodes` happens in source order, this means that anonymous
mixins would only get their sealed and final attributes properly
inferred if they appeared *after* their immediate supertypes in source
order.  By moving this step to `checkSupertypes`, we ensure that the
computation is correct regardless of source order, because
`checkSupertypes` happens in class hierarchy order.

Fixes: #52169.

Bug: #52048
Change-Id: Ife69b582d53fcd49be87a570cd969389a58689e4
Cherry-pick: https://dart-review.googlesource.com/c/sdk/+/297901
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/298200
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Reviewed-by: Chloe Stefantsova <cstefantsova@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues. P1 A high priority bug; for example, a single project is unusable or has many test failures
Projects
None yet
Development

No branches or pull requests

3 participants