Skip to content

Fix assertion from empty switch over uninhabited enum #19979

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

Merged
merged 1 commit into from
Oct 29, 2018

Conversation

mdiep
Copy link
Contributor

@mdiep mdiep commented Oct 22, 2018

Fixes SR-8933 | rdar://problem/45216708.

Swift's uninhabited checking is conservative. An enum might not be found to be uninhabited, but all of its cases might. In that case, the switch can be empty even though the type isn't known to be uninhabited.

Fix an assertion that assumed there would always be at least one case for types that aren't known to be uninhabited.

This is the approach that was suggested by @jckarter in the ticket. (I hope I've done it correctly.)

Fixes SR-8933.

Swift's uninhabited checking is conservative. An enum might not be found to be uninhabited, but all of its cases might. In that case, the switch can be empty even though the type isn't known to be uninhabited.

Fix an assertion that assumed there would always be at least one case for types that aren't known to be uninhabited.
Copy link
Contributor

@jckarter jckarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@jckarter
Copy link
Contributor

@swift-ci Please test

@jckarter
Copy link
Contributor

@swift-ci Please test source compatibility

@mdiep
Copy link
Contributor Author

mdiep commented Oct 27, 2018

Looks like those failures aren't due to my changes here. 😅 (Side note: Is there an easy way to reproduce and debug those?)

Hopefully this is good to go. 😄

@gottesmm
Copy link
Contributor

@mdiep those are known deserialization failures. @clackary is it possible to XFAIL those/file an SR?

@clackary
Copy link

Yes, I might have some time later today to do this.

@clackary
Copy link

Looked into this a bit further, and the source compatibility failures appear to be duplicates of SR-9084, which has a fix in #20091. So far, testing looks good. Unless things take a turn for the worse, this should be resolved upon merge. I'm going to hold off on adding any xfails at this point, since the fix is imminent.

Neither of the source compatibility project failures appear to be related to changes in this pull request, so unless getting a passing build from source compatibility (also note that it's only failing in debug... the release configuration has passed), feel free to proceed.

@jckarter
Copy link
Contributor

@clackary Sounds good, I'll go ahead and merge. Thanks @mdiep!

@jckarter jckarter merged commit 7b5e83d into swiftlang:master Oct 29, 2018
@mdiep mdiep deleted the SR-8933 branch October 29, 2018 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants