Skip to content

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

Closed
wants to merge 2 commits into from
Closed

Conversation

CodaFi
Copy link
Contributor

@CodaFi CodaFi commented Aug 18, 2016

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:

  • Test pull request on Swift continuous integration.

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

Platform Comment
All supported platforms @swift-ci Please smoke test
All supported platforms @swift-ci Please smoke test and merge
OS X platform @swift-ci Please smoke test OS X platform
Linux platform @swift-ci Please smoke test Linux platform

A smoke test on macOS does the following:

  1. Builds the compiler incrementally.
  2. Builds the standard library only for macOS. Simulator standard libraries and
    device standard libraries are not built.
  3. lldb is not built.
  4. The test and validation-test targets are run only for macOS. The optimized
    version of these tests are not run.

A smoke test on Linux does the following:

  1. Builds the compiler incrementally.
  2. Builds the standard library incrementally.
  3. lldb is built incrementally.
  4. The swift test and validation-test targets are run. The optimized version of these
    tests are not run.
  5. lldb is tested.

Validation Testing

Platform Comment
All supported platforms @swift-ci Please test
All supported platforms @swift-ci Please test and merge
OS X platform @swift-ci Please test OS X platform
OS X platform @swift-ci Please benchmark
Linux platform @swift-ci Please test Linux platform

Lint Testing

Language Comment
Python @swift-ci Please Python lint

Note: Only members of the Apple organization can trigger swift-ci.

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.
@CodaFi
Copy link
Contributor Author

CodaFi commented Aug 18, 2016

@swift-ci please smoke test.

@slavapestov
Copy link
Contributor

Why do we want this?

@CodaFi
Copy link
Contributor Author

CodaFi commented Aug 18, 2016

There's a clear difference between Never the type and any user-defined enumeration, regardless of whether it has cases or not.

@jrose-apple
Copy link
Contributor

There's not intended to be. We need to diagnose "falling off the end" in either case.

@slavapestov
Copy link
Contributor

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.

@slavapestov
Copy link
Contributor

@jrose-apple By definition we cannot fall off the end if the return type is Never, because the return is not reachable.

@CodaFi
Copy link
Contributor Author

CodaFi commented Aug 18, 2016

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 ex falso. There is only one bottom type - Never. If the user wishes to explore alternate expressions of that fact, they can always reach for the classic

enum X {}

func explode() -> X {
  return explode()
}

@CodaFi
Copy link
Contributor Author

CodaFi commented Aug 18, 2016

@swift-ci please smoke test Linux platform. Smoke test link

@slavapestov
Copy link
Contributor

If defer to @jckarter regarding this. It was his original suggestion to treat all empty enums as a 'Never' type.

@slavapestov
Copy link
Contributor

Actually, the fact that this doesn't produce a diagnostic is the real bug:

enum E {}

func f() -> E {}

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 {
Copy link
Contributor

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.

@CodaFi
Copy link
Contributor Author

CodaFi commented Aug 18, 2016

That it happens for E is what this fixes. For Never there shouldn't be a diagnostic, right?

@slavapestov
Copy link
Contributor

slavapestov commented Aug 18, 2016

@CodaFi That's the thing, we should get the diagnostic for Never too.

Remember, this was invalid before:

@noreturn func f() {}

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.

@CodaFi
Copy link
Contributor Author

CodaFi commented Aug 18, 2016

Sounds good.

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.

3 participants