Skip to content

Fix a pair of crashes with '@unknown' #15889

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Apr 17, 2018
Merged

Fix a pair of crashes with '@unknown' #15889

merged 2 commits into from
Apr 17, 2018

Conversation

jrose-apple
Copy link
Contributor

Fixes a pair of issues @rintaro caught around misuse of @unknown default. Let's not crash on bad user input!

SR-7408, SR-7409

@jrose-apple
Don't crash when using @unknown default with a non-enum type.
a9f26ab 
Also improve the error message when using it with a very large space.

https://bugs.swift.org/browse/SR-7408
@jrose-apple jrose-apple requested review from xedin, rintaro and CodaFi April 12, 2018 02:30
@jrose-apple
Copy link
Contributor Author

@swift-ci Please smoke test

rintaro
rintaro approved these changes Apr 12, 2018
View reviewed changes
Copy link
Member

@rintaro rintaro left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Diagnostics look great. Thank you!

@CodaFi
Copy link
Contributor

CodaFi commented Apr 12, 2018 •
edited
Loading

If @unknown is a structural feature, I think it ought to go into the type checker proper before we enter the exhaustiveness checker. That'll let you keep your assertion.

@jrose-apple
Copy link
Contributor Author

Yeah, I forgot about limitedChecking when I wrote this. The first commit is probably still valuable, but the second should just be caught earlier.

@jrose-apple
Limit exhaustivity checking when weird things happen with '@unknown'
8157e80 
If someone uses it twice, or on a non-final case, or tries to use it
with a multiple-pattern case, don't bother checking the exhaustiveness
of the switch. (For other violations, the pattern or where-clause is
ignored.)

https://bugs.swift.org/browse/SR-7409
@jrose-apple jrose-apple force-pushed the ready-for-the-unknown branch from 217dbc4 to 8157e80 Compare April 13, 2018 03:32
@jrose-apple
Copy link
Contributor Author

New version of the second commit (SR-7409).

@swift-ci Please test

@jrose-apple
Copy link
Contributor Author

@swift-ci Please test source compatibility

@jrose-apple
Copy link
Contributor Author

Snuck in another PrettyStackTrace too.

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 217dbc465965acc9a40f41daa04d8651aaceebd2

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 217dbc465965acc9a40f41daa04d8651aaceebd2

@jrose-apple
Copy link
Contributor Author

@swift-ci Please test source compatibility

@jrose-apple jrose-apple merged commit 275e101 into swiftlang:master Apr 17, 2018
@jrose-apple jrose-apple deleted the ready-for-the-unknown branch April 17, 2018 17:18
This was referenced Apr 12, 2018
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rintaro rintaro rintaro approved these changes

@xedin xedin Awaiting requested review from xedin

@CodaFi CodaFi Awaiting requested review from CodaFi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jrose-apple @CodaFi @swift-ci @rintaro