Skip to content

Fix behaviour of divergence in while loop conditions #51049

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

Merged
merged 3 commits into from
May 26, 2018
Merged

Fix behaviour of divergence in while loop conditions #51049

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a38a1ce
Fix behaviour of divergence in while loop conditions
varkor May 24, 2018
03d816f
Add a test for returning inside a while loop
varkor May 25, 2018
d5bf4de
--bless the tests
varkor May 25, 2018
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
14 changes: 10 additions & 4 deletions src/librustc_typeck/check/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3841,10 +3841,10 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
let ctxt = BreakableCtxt {
// cannot use break with a value from a while loop
coerce: None,
may_break: true,
may_break: false, // Will get updated if/when we find a `break`.
};

self.with_breakable_ctxt(expr.id, ctxt, || {
let (ctxt, ()) = self.with_breakable_ctxt(expr.id, ctxt, || {
self.check_expr_has_type_or_error(&cond, tcx.types.bool);
let cond_diverging = self.diverges.get();
Copy link
Contributor

Choose a reason for hiding this comment

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

So what's confusing me here: there is somehow an implicit break should the condition be false (this is why may_break used to be considered true), and I can't see where that is accounted for.

For example, does this code accept:

let x: ! = {
  while false { return; };
};

( Do we have a test for this? It annoys me that this question is so hard to answer =) )

Copy link
Contributor

Choose a reason for hiding this comment

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

Traditionally we have considered while loops as always potentially breaking — but it seems like your change could allow us to handle cases like this one (for example)

'a: loop {
    let x: ! = {
        while (continue 'a) { };
    };
}

Copy link
Member Author

@varkor varkor May 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

For example, does this code accept:

No, this code fails to type-check both in the current version and after this PR. I'll add a test for it, though. The divergence of the body of the loop is completely ignored at the moment — it's just the condition that is taken into account. This PR ignores divergence in the condition too if break was involved.

Actually, even a case like:

let _: ! = 'a: while continue 'a {};

doesn't type-check at the moment, though intuitively it could do. (This isn't affected by this PR.) I think such a change would be more in scope for #51053, though. It's not breaking anything at the moment (as it's conservative).

self.check_block_no_value(&body);
Expand All @@ -3853,6 +3853,12 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
self.diverges.set(cond_diverging);
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, this is the bit I was missing before (which ignores the divergence of the body). Very good.

});

if ctxt.may_break {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like not having this before was the obvious bug...

// No way to know whether it's diverging because
// of a `break` or an outer `break` or `return`.
self.diverges.set(Diverges::Maybe);
}

self.tcx.mk_nil()
}
hir::ExprLoop(ref body, _, source) => {
Expand All @@ -3871,7 +3877,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {

let ctxt = BreakableCtxt {
coerce,
may_break: false, // will get updated if/when we find a `break`
may_break: false, // Will get updated if/when we find a `break`.
};

let (ctxt, ()) = self.with_breakable_ctxt(expr.id, ctxt, || {
Expand All @@ -3880,7 +3886,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {

if ctxt.may_break {
// No way to know whether it's diverging because
// of a `break` or an outer `break` or `return.
// of a `break` or an outer `break` or `return`.
self.diverges.set(Diverges::Maybe);
}

Expand Down
39 changes: 39 additions & 0 deletions src/test/ui/break-while-condition.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// Copyright 2018 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(never_type)]

fn main() {
// The `if false` expressions are simply to
// make sure we don't avoid checking everything
// simply because a few expressions are unreachable.

if false {
let _: ! = { //~ ERROR mismatched types
'a: while break 'a {};
};
}

if false {
let _: ! = {
while false { //~ ERROR mismatched types
break
}
};
}

if false {
let _: ! = {
while false { //~ ERROR mismatched types
return
}
};
}
}
43 changes: 43 additions & 0 deletions src/test/ui/break-while-condition.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
error[E0308]: mismatched types
--> $DIR/break-while-condition.rs:19:20
|
LL | let _: ! = { //~ ERROR mismatched types
| ____________________^
LL | | 'a: while break 'a {};
LL | | };
| |_________^ expected !, found ()
|
= note: expected type `!`
found type `()`

error[E0308]: mismatched types
--> $DIR/break-while-condition.rs:26:13
|
LL | fn main() {
| - expected `()` because of default return type
...
LL | / while false { //~ ERROR mismatched types
LL | | break
LL | | }
| |_____________^ expected !, found ()
|
= note: expected type `!`
found type `()`

error[E0308]: mismatched types
--> $DIR/break-while-condition.rs:34:13
|
LL | fn main() {
| - expected `()` because of default return type
...
LL | / while false { //~ ERROR mismatched types
LL | | return
LL | | }
| |_____________^ expected !, found ()
|
= note: expected type `!`
found type `()`

error: aborting due to 3 previous errors

For more information about this error, try `rustc --explain E0308`.