-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Uninhabited != NoReturn #4365
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
Uninhabited != NoReturn #4365
Conversation
Previously, isNever would return true for an equivalence class of uninhabited enums. The rest of the compiler, however, is using this in places that actually expect just the Never type. This means that code like this would compile properly enum Uninhabited {} func do() -> Uninhabited { /* No body here */ } and we wouldn’t diagnose the missing return.
@swift-ci please smoke test. |
Why do we want this? |
There's a clear difference between |
There's not intended to be. We need to diagnose "falling off the end" in either case. |
Right. To clarify, we want any uninhabited type to behave like Never, and mark a function as not returning. Right now the isNever check is inaccurate because it doesn't realize that a strict with an uninhabited field is also uninhabited, but that didn't seem worth fixing. |
@jrose-apple By definition we cannot fall off the end if the return type is Never, because the return is not reachable. |
When dealing with user-defined types it's more likely that they meant to write up the cases for the enum later and would expect a diagnostic about the function not returning than they decided to exploit enum X {}
func explode() -> X {
return explode()
} |
@swift-ci please smoke test Linux platform. Smoke test link |
If defer to @jckarter regarding this. It was his original suggestion to treat all empty enums as a 'Never' type. |
Actually, the fact that this doesn't produce a diagnostic is the real bug:
It happens for both 'E' and 'Never'. Mind if I take this? |
@@ -9,6 +9,11 @@ func singleBlock2() -> Int { | |||
y += 1 | |||
} // expected-error {{missing return in a function expected to return 'Int'}} | |||
|
|||
enum NoCasesButNotNever {} | |||
|
|||
func diagnoseNoCaseEnumMissingReturn() -> NoCasesButNotNever { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this is meant to be an error even with a 'Never' return. a Never-returning function should end with a call to some other Never-returning function, or the return should be unreachable.
That it happens for |
@CodaFi That's the thing, we should get the diagnostic for Never too. Remember, this was invalid before:
A @NoReturn function had to either end with a call to some other @NoReturn function, or end with an infinite loop, essentially. You weren't allowed to fall through the end. A return type of Never should behave exactly like a @NoReturn function did in Swift 2. It looks like something is wrong in the SIL unreachable diagnostics pass wrt the Never type. |
Sounds good. |
What's in this pull request?
Fix a case where we would fail to diagnose a missing return if an enum decl as the result of a function had no cases.
@slavapestov can you review?
Before merging this pull request to apple/swift repository:
Triggering Swift CI
The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:
Smoke Testing
A smoke test on macOS does the following:
device standard libraries are not built.
version of these tests are not run.
A smoke test on Linux does the following:
tests are not run.
Validation Testing
Lint Testing
Note: Only members of the Apple organization can trigger swift-ci.