[SIL] Fix Diagnosing Never-returning functions that don't call other Never-returning functions #4393
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.
CodaFi
changed the title
|
@swift-ci please test. |
| ERROR(missing_never_call,none, | ||
| "never-returning %select{function|closure}0 is missing call to " | ||
| "another never-returning function on all paths.", | ||
| (unsigned)) |
There was a problem hiding this comment.
I don't like the wording of this diagnostic.
There was a problem hiding this comment.
I think if I change isNever to isUninhabited and put back the original implementation I can make this
function with uninhabited return type is missing call to never-returning function on all paths
|
Build failed |
| ERROR(non_exhaustive_switch,none, | ||
| ERROR(missing_never_call,none, | ||
| "never-returning %select{function|closure}0 is missing call to " | ||
| "another never-returning function on all paths.", |
There was a problem hiding this comment.
Don't end diagnostic with '.'.
CodaFi
force-pushed
the
never-say-never
branch
from
f6f609b to
1b970ab
Compare
|
@swift-ci please test macOS platform. CI Job is here |
|
@slavapestov bump for re-review. |
| // function is marked 'noreturn'. | ||
| if (ResTy->isVoid() || F->isNoReturnFunction()) | ||
| // No action required if the function returns 'Void'. | ||
| if (ResTy->isVoid()) |
There was a problem hiding this comment.
Is even this necessary? SILGen ought to emit the return implicitly already if it's emitting a void function. We'd still want to diagnose things like guard or switch exhaustiveness where we use unreachables to indicate control flow that should be unreachable.
There was a problem hiding this comment.
Hm... I'm seeing what effect removing this would have, but you're right it seems this code path is unexercised.
There was a problem hiding this comment.
Looks like nothing exploded.
|
Build failed |
Turn on the noreturn diagnostic for cases where a reachable unreachable could be encountered. Previously, the diagnostic would not fire if the function was marked noreturn and any of its reachable unreachable calls were around. While this makes sense from a SILGen perspective (it Just Crashes tm), it is still wrong. We need to diagnose *everything* that has reachable unreachables.
CodaFi
force-pushed
the
never-say-never
branch
from
1b970ab to
5a2479c
Compare
|
@CodaFi Looks good, thanks for seeing this through. Can you cherry-pick this over to the 3.0 branch as well? |
|
Can do. |
|
@swift-ci please test macOS platform. Build status |
|
@swift-ci please test macOS platform. |
What's in this pull request?
A more principled fix than #4365. It turns out we have never handled this kind of thing correctly in the SIL Optimizer. Previous versions of Swift even allowed this to work before the uninhabited changes went through.
The SIL Optimizer now understands that
Voidis the only case where a missing return is acceptable and correctly diagnoses failing to call otherNever-returning functions from all paths inNever-returning functions.@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.