Skip to content
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

Merged
merged 1 commit into from
Aug 24, 2016

Conversation

durka
Copy link
Contributor

@durka durka commented Aug 21, 2016

Fixes #35849.

Thanks @eddyb.

@rust-highfive
Copy link
Collaborator

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

NoExpectation
} else {
ExpectHasType(fcx.ret_ty)
});
Copy link
Member

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.

@eddyb
Copy link
Member

eddyb commented Aug 22, 2016

r=me with nit fixed.

@durka
Copy link
Contributor Author

durka commented Aug 22, 2016

Nit fixed. This also needs to be beta-nominated.

@eddyb
Copy link
Member

eddyb commented Aug 22, 2016

@bors r+

@bors
Copy link
Contributor

bors commented Aug 22, 2016

📌 Commit 9d53faf has been approved by eddyb

@eddyb eddyb added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Aug 22, 2016
@canndrew
Copy link
Contributor

This won't allow something like fn foo() -> ! { 3 } to compile will it?

@eddyb
Copy link
Member

eddyb commented Aug 22, 2016

@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).

eddyb added a commit to eddyb/rust that referenced this pull request Aug 22, 2016
typeck: use NoExpectation to check return type of diverging fn

Fixes rust-lang#35849.

Thanks @eddyb.
@durka
Copy link
Contributor Author

durka commented Aug 22, 2016

@canndrew @eddyb yep we got a problem here:

[root@li1424-173 gh35849]# build/x86_64-unknown-linux-gnu/stage1/bin/rustc - -o bad <<<'fn foo() -> ! { 42 } fn main() { foo(); }'
warning: broken MIR (return = const 42i32): bad assignment (! = i32): Sorts(ExpectedFound { expected: !, found: i32 })
 --> <anon>:1:17
  |
1 | fn foo() -> ! { 42 } fn main() { foo(); }
  |                 ^^

[root@li1424-173 gh35849]# ./bad
Segmentation fault (core dumped)

@eddyb
Copy link
Member

eddyb commented Aug 22, 2016

Whoa how did that happen?! That's bad, because the code erroring before wasn't in typeck AFAIK.

@bors r-

@durka durka mentioned this pull request Aug 22, 2016
@durka
Copy link
Contributor Author

durka commented Aug 22, 2016

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.

@canndrew
Copy link
Contributor

Are you talking about the check in check_ret which was removed? If so, I think I was assuming that any block which diverges would get typed ! (including ones ending in an expression). Would it better to fix that than to make a special case for ! not having to be type-check correctly and then make another special case for it further down the pipeline?

@eddyb
Copy link
Member

eddyb commented Aug 22, 2016

@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.

@arielb1
Copy link
Contributor

arielb1 commented Aug 22, 2016

Maybe just add a "redundant" return type check always, like MIR does?

@canndrew
Copy link
Contributor

I'm generally uncomfortable making special cases for ! - part of the motivation for this RFC is that a lot of this sort of thing can be handled naturally by the type system. Plus ! isn't the only diverging type.

@eddyb
Copy link
Member

eddyb commented Aug 22, 2016

@canndrew The special cases need to be there until there's a RFC that says [x: !] is typed ! instead of [!; 1] or that [!; 1] is-a-subtype-of/coerces-to !, and until the definition of "diverging function" changes.
We risk too much if we don't have sanity checks for the CFG of a diverging function - the MIR warning being one of them, we should probably make those warnings ICE now.

@durka
Copy link
Contributor Author

durka commented Aug 22, 2016

we should probably make those warnings ICE now

cf #35499

@canndrew
Copy link
Contributor

I don't think that liveness should have special cases for ! at all. As far as its is concerned ! isn't a special case, it's just one of many uninhabited types. If liveness can handle Void it should be able to handle ! as well. The only reason it can't is that we just added another special case for ! which broke typechecking and allowed clear nonsense like fn foo() -> ! { 3 } to get through. It's one special case begetting another.

The special cases need to be there until there's a RFC that says [x: !] is typed ! instead of [!; 1] or that [!; 1] is-a-subtype-of/coerces-to !

Why? This can get typechecked correctly:

fn foo() -> ! {
    [panic!()];
}

So there's some logic that can see the ! in the middle of that expression. Can we make that logic run on the block's final expression as well and not just it's statements?

@eddyb
Copy link
Member

eddyb commented Aug 22, 2016

@canndrew No, that doesn't get type-checked either. The error you get when the function is declared to return T and you have a semicolon and no divergence is not all control paths return a value, also from liveness.
That is, if you don't have a trailing expression, it's not checked at all, although this seems to only happen at the outermost level, a type error is produced if you nest blocks.

@durka
Copy link
Contributor Author

durka commented Aug 22, 2016

Amended to restore the special case in Liveness::check_ret.

@durka
Copy link
Contributor Author

durka commented Aug 22, 2016

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() {
Copy link
Member

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.

@durka
Copy link
Contributor Author

durka commented Aug 22, 2016

Knit nitted.

@eddyb
Copy link
Member

eddyb commented Aug 22, 2016

@bors r+

@bors
Copy link
Contributor

bors commented Aug 22, 2016

📌 Commit ddebea0 has been approved by eddyb

This fixes rust-lang#35849, a regression introduced by the typeck refactoring
around TyNever/!.
@durka
Copy link
Contributor Author

durka commented Aug 23, 2016

Fixed the travis failure and added some FIXME comments.

We unfortunately needed to ignore a test added for #![feature(never_type)] because it can't be done with the current structure of the liveness code. A fix may be possible, but the consensus was that we shouldn't try to get it into this PR, to keep it backportable. I filed a separate bug.

@eddyb
Copy link
Member

eddyb commented Aug 23, 2016

@bors r+

@bors
Copy link
Contributor

bors commented Aug 23, 2016

📌 Commit 702ea71 has been approved by eddyb

eddyb added a commit to eddyb/rust that referenced this pull request Aug 23, 2016
typeck: use NoExpectation to check return type of diverging fn

Fixes rust-lang#35849.

Thanks @eddyb.
eddyb added a commit to eddyb/rust that referenced this pull request Aug 23, 2016
typeck: use NoExpectation to check return type of diverging fn

Fixes rust-lang#35849.

Thanks @eddyb.
@bors
Copy link
Contributor

bors commented Aug 24, 2016

⌛ Testing commit 702ea71 with merge 5f31fda...

@alexcrichton
Copy link
Member

@bors: retry force clean

  • restarted buildbot

@bors
Copy link
Contributor

bors commented Aug 24, 2016

⌛ Testing commit 702ea71 with merge 03e23c7...

bors added a commit that referenced this pull request Aug 24, 2016
typeck: use NoExpectation to check return type of diverging fn

Fixes #35849.

Thanks @eddyb.
@bors bors merged commit 702ea71 into rust-lang:master Aug 24, 2016
@nrc nrc added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Aug 25, 2016
@brson brson removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Sep 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants