-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
typeck: use NoExpectation to check return type of diverging fn #35883
Conversation
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
NoExpectation | ||
} else { | ||
ExpectHasType(fcx.ret_ty) | ||
}); |
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.
I'd put this in a variable or call a different version of check_block
in the TyNever
case.
r=me with nit fixed. |
Nit fixed. This also needs to be |
@bors r+ |
📌 Commit 9d53faf has been approved by |
This won't allow something like |
@canndrew It's the same logic as before, I believe liveness is what prevents control-flow from exiting the function if the function is supposed to be divergent (in a more sound manner). |
typeck: use NoExpectation to check return type of diverging fn Fixes rust-lang#35849. Thanks @eddyb.
@canndrew @eddyb yep we got a problem here:
|
Whoa how did that happen?! That's bad, because the code erroring before wasn't in typeck AFAIK. @bors r- |
IRC investigation reveals that #35162 removed an important check from the liveness pass, which I will reinstate whenever LLVM finishes building. Now I'm wondering if that PR removed anything else important. |
Are you talking about the check in |
@canndrew Maybe that can work, but we'll always need a sanity check - if typeck ends up being able to handle divergence correctly, we'd just have a proper CFG-based check ICE instead of erroring. |
Maybe just add a "redundant" return type check always, like MIR does? |
I'm generally uncomfortable making special cases for |
@canndrew The special cases need to be there until there's a RFC that says |
cf #35499 |
I don't think that liveness should have special cases for
Why? This can get typechecked correctly: fn foo() -> ! {
[panic!()];
} So there's some logic that can see the |
@canndrew No, that doesn't get type-checked either. The error you get when the function is declared to return |
Amended to restore the special case in |
Nits knitted. |
@@ -1479,7 +1479,10 @@ impl<'a, 'tcx> Liveness<'a, 'tcx> { | |||
self.ir.tcx.region_maps.call_site_extent(id, body.id), | |||
&self.fn_ret(id)); | |||
|
|||
if self.live_on_entry(entry_ln, self.s.no_ret_var).is_some() { | |||
if fn_ret.is_never() && self.live_on_entry(entry_ln, self.s.clean_exit_var).is_some() { |
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.
These should not be combined, i.e. the else
below should be to fn_ret.is_never()
alone, to match the old semantics.
Knit nitted. |
@bors r+ |
📌 Commit ddebea0 has been approved by |
This fixes rust-lang#35849, a regression introduced by the typeck refactoring around TyNever/!.
Fixed the travis failure and added some FIXME comments. We unfortunately needed to ignore a test added for |
@bors r+ |
📌 Commit 702ea71 has been approved by |
typeck: use NoExpectation to check return type of diverging fn Fixes rust-lang#35849. Thanks @eddyb.
typeck: use NoExpectation to check return type of diverging fn Fixes rust-lang#35849. Thanks @eddyb.
⌛ Testing commit 702ea71 with merge 5f31fda... |
@bors: retry force clean
|
Fixes #35849.
Thanks @eddyb.