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.

Sign up for GitHub

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

Jump to bottom

Mark block exits as reachable if the block can break. #45316

Merged
merged 1 commit into from
Oct 20, 2017
Merged

Mark block exits as reachable if the block can break. #45316

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/librustc/hir/lowering.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2547,7 +2547,7 @@ impl<'a> LoweringContext<'a> {
};

// Err(err) => #[allow(unreachable_code)]
// return Carrier::from_error(From::from(err)),
// return Try::from_error(From::from(err)),
let err_arm = {
let err_ident = self.str_to_ident("err");
let err_local = self.pat_ident(e.span, err_ident);
Expand Down
7 changes: 7 additions & 0 deletions src/librustc_typeck/check/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4284,6 +4284,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
CoerceMany::with_coercion_sites(coerce_to_ty, tail_expr)
};

let prev_diverges = self.diverges.get();
let ctxt = BreakableCtxt {
coerce: Some(coerce),
may_break: false,
Expand Down Expand Up @@ -4333,6 +4334,12 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
}
});

if ctxt.may_break {
// If we can break from the block, then the block's exit is always reachable
// (... as long as the entry is reachable) - regardless of the tail of the block.
self.diverges.set(prev_diverges);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this doesn't seem quite right because of the possibility of dead code. Consider something like this:

let x = 'a: {
    panic!(); break 'a 22;
};

or perhaps

let x = do catch {
    panic!("wtf");
    Err(22)?;
    Ok(())
};

That said, I think that ExprLoop is comparably imprecise. I'm trying to remember all the implications of this and make sure there's not some kind of soundness issue here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Thoughts?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it unsound to mark code as potentially reachable (erm, Diverges::Maybe) when it in fact is never reachable? Seems like it shouldn't be - from my understanding, it should only be unsound to make the wrong call in the opposite direction.

Also maybe we could skip marking .may_break if the break itself is unreachable, though I wouldn't personally advocate for it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is currently the case for loop (is there a test for it? there ought to be):

fn main() {
    let x: u32 = { //~ ERROR mismatched types
        loop {
            panic!();
            break;
        };
    };
}

From a soundness perspective, IIRC if an arm is never reachable, then it is potentially reachable, so regarding something that always diverges as something that potentially reaches its exit should always be sound.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for delay. Busy couple of days. Anyway, I agree that ExprLoop is treated the same way, and I also agreed it ought not to be unsound to approximate in this way. (In particular, I think we only care when things definitely diverge -- this makes sense since clearly we can't tell that things definitely don't diverge (damn halting problem).)

I was thinking more about what behavior we actually want. I actually think it's consistent with our general strategy around typing dead code to allow break -- even in dead code -- to make a loop or block be considered non-diverging. For example, the example that @arielb1 gave here] seems consistent with the rules we adopted for dead-code, which mean that e.g. the following code doesn't build:

#![allow(unreachable_code)]

fn main() {
    let x: u32 = { //~ ERROR mismatched types
        panic!();
        'a'
    };
}

In particular, I would consider break 'a' to be equivalent to using 'a' as a tail expression.

Essentially -- to the extent possible -- we are aiming to type-check dead-code as if it could become live. This includes not only the types of values produced by dead-code but also the control-flow that the dead-code would have introduced.

We might want to tweak that last bit but, regardless, this PR is consistent with what we do now.

}

let mut ty = ctxt.coerce.unwrap().complete(self);

if self.has_errors.get() || ty.references_error() {
Expand Down
24 changes: 24 additions & 0 deletions src/test/run-pass/issue-45124.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// Copyright 2017 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#![feature(catch_expr)]

fn main() {
let mut a = 0;
let () = {
let _: Result<(), ()> = do catch {
let _ = Err(())?;
return
};
a += 1;
};
a += 2;
assert_eq!(a, 3);
}