Implementation for @unknown default
#14382
Conversation
|
(yes, I know it needs rebasing) |
jrose-apple
force-pushed
the
unknown-case
branch
2 times, most recently
from
f93ffd1 to
de085ad
Compare
jrose-apple
force-pushed
the
unknown-case
branch
from
de085ad to
ef52640
Compare
|
Rebased on top of the new implementation of frozen enums! (already in-tree) @CodaFi, I'm sorry to subject you to such a diff, but I welcome your SpaceEngine wisdom once again. |
|
@swift-ci Please test |
|
@swift-ci Please test compiler performance |
|
@swift-ci Please test source compatibility |
|
Build failed |
|
Build failed |
|
Oops, the very last change I did is wrong. |
jrose-apple
force-pushed
the
unknown-case
branch
from
ef52640 to
60b4ef7
Compare
|
@swift-ci Please test |
|
Build failed |
|
Build failed |
|
This fails on Linux, and I can reproduce the failure, but I have no idea why. It's emitting the fix-its in a different order, but I don't see anything that should be non-deterministic or implementation-defined. |
jrose-apple
force-pushed
the
unknown-case
branch
from
60b4ef7 to
6ee50b2
Compare
|
…should have known it'd be the std::sort. @swift-ci Please test |
|
Build failed |
|
Build failed |
| @@ -78,13 +78,22 @@ namespace { | |||
| Type, | |||
| Constructor, | |||
| Disjunct, | |||
| BooleanConstant | |||
| BooleanConstant, | |||
| UnknownCase, | |||
There was a problem hiding this comment.
You wound up making this a space kind anyway? For those that want context, we discussed this in private. The idea then was that you could traverse the pattern space to look for these - like we're already doing to find downgradable spaces - rather than using a subtraction and flattening them out.
There was a problem hiding this comment.
Ah, heh, I forgot about that conversation. I think I threw out most of those ideas (mistakenly?) once we decided unknown could match patterns containing non-frozen enums as well. Do you still think I should keep "future cases" out of the space kinds?
There was a problem hiding this comment.
The space traversal for downgradable patterns looks into nested patterns as well - unless I'm not quite understanding you here. I haven't kept up with the Swift Evolution discussion around this all that well.
There was a problem hiding this comment.
I'll take a look tomorrow. Given that the user side of unknown had to be handled specially anyway, what I think you're saying might make more sense.
There was a problem hiding this comment.
…yeah, I'm not sure this is going to work. I can't figure out how to deal with this example that way:
/*non-frozen*/ public enum Test {
case a, b, c
}
@inlinable public func consume(_ x: Test) {
switch x {
case .a, .b: break
}
}In this case the compiler should suggest adding both case .c and unknown. If you decompose the Test space, you lose the non-frozen-ness, but if you don't decompose you can't subtract out the a and b cases.
I think the other way worked when the only thing that could match future cases was irrefutable patterns, and so the suggestion for the above example could just be default. But now that we have to distinguish these situations I think an Unknown space does make sense.
jrose-apple
force-pushed
the
unknown-case
branch
from
6ee50b2 to
0621166
Compare
|
@swift-ci Please smoke test |
jrose-apple
force-pushed
the
unknown-case
branch
from
0621166 to
202ef43
Compare
|
@swift-ci Please smoke test |
| @@ -440,7 +493,8 @@ namespace { | |||
| PAIRCASE (SpaceKind::Type, SpaceKind::Disjunct): | |||
| PAIRCASE (SpaceKind::Constructor, SpaceKind::Disjunct): | |||
| PAIRCASE (SpaceKind::Disjunct, SpaceKind::Disjunct): | |||
| PAIRCASE (SpaceKind::BooleanConstant, SpaceKind::Disjunct): { | |||
| PAIRCASE (SpaceKind::BooleanConstant, SpaceKind::Disjunct): | |||
| PAIRCASE (SpaceKind::UnknownCase, SpaceKind::Disjunct): { | |||
There was a problem hiding this comment.
Just a note: It looks to me like these two blocks (where disjunction is on the either side) could be refactored to be a single block since intersection operation supposed to be commutative (at least from the comment).
There was a problem hiding this comment.
Yeah, I'd kinda like to do that for all the symmetrical ones. I didn't change that structure in this PR, though.
There was a problem hiding this comment.
Makes sense, just wanted to point it out!
Get these simple changes out of the way first. No functionality change.
There was a problem hiding this comment.
Looks great, @jrose-apple! I think it makes sense to have Unknown as a type of space and check for it only after initial exhaustiveness check.
This is our first statement attribute, made more complicated by the fact that a 'case'/'default' isn't really a normal statement. I've chosen /not/ to implement a general statement attribute logic like we have for types and decls at this time, but I did get the compiler parsing arbitrary attributes before 'case' and 'default'. As a bonus, we now treat all cases within functions as being switch-like rather than enum-like, which is better for recovery when not in a switch.
The first half of Sema support for '@unknown'. The other part is handling when the user /does/ write '@unknown', which results in /other/ things being downgraded to warnings. The diagnostics here are still pretty minimal; they should explain what's going on with '@unknown' to someone who hasn't read the Swift 5 release notes.
The other half of '@unknown' in Sema. Again, the diagnostics here could be improved; rather than a generic "switch must be exhaustive", it could say something about unknown case handling known cases. One interesting detail here: '@unknown' is only supposed to match /fully/ missing cases. If a case is /partly/ accounted for, not handling the rest is still an error, even if an unknown case is present. This only works with switches over single enum values, not values that contain an enum with unknown cases. That's coming in a later commit. (It was easier to get this part working first.)
We still model the enums as non-exhaustive so that someone /can/ handle unknown cases, which may be important for imported enums. But we won't diagnose a problem if the only missing case is '@unknown'.
That is, when matching non-frozen enums at non-top-level positions:
switch (nonFrozenEnum1, nonFrozenEnum2) {
case (.singleKnownCase1, .singleKnownCase2): ...
unknown: ...
}
...it's sufficient to use '@unknown' to match
(.singleKnownCase1, .someFutureCase2)
(.someFutureCase1, .singleKnownCase2)
(.someFutureCase1, .someFutureCase2)
If a client wants to defend themselves against new cases in libraries they use, or even in their own code, they're allowed to.
- Combine the common logic for editor mode and non-editor mode. - Do a better job minimizing fix-its. - If '@unknown' is the only missing case, put `fatalError()` in the Xcode placeholder, since that's what the compiler would have done.
- Must be a single pattern - Must be last in the switch - Must not have a 'where' clause - Must specifically be the "any" pattern if using 'case' We may lift some of these restrictions in the future, but that would need a new proposal.
jrose-apple
force-pushed
the
unknown-case
branch
from
202ef43 to
1f14d03
Compare
jrose-apple
changed the title
unknown case@unknown case _
jrose-apple
changed the title
@unknown case _@unknown default
|
@swift-ci Please test source compatibility |
|
Build failed |
|
Build failed |
|
@swift-ci Please test source compatibility |
|
@swift-ci Please test |
| children=[ | ||
| Child('UnknownKeyword', kind='Token'), | ||
| Child('Colon', kind='ColonToken'), | ||
| ]), |
There was a problem hiding this comment.
This SwitchUnknownLabel is not used. Can I remove this?
There was a problem hiding this comment.
Whoops, I thought I took it out already. Yes, sorry!
Part of the revised SE-0192, building on top of #11961. The first PR is big enough that I'd like to land this part separately afterwards.
Still to do: write some SIL tests to make sure(done!)unknown caseis properly getting treated likedefault.