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

NLL migration mode is accepting code that both NLL and AST-borrowck reject! #53026

Closed
pnkfelix opened this issue Aug 3, 2018 · 3 comments · Fixed by #53045
Closed

NLL migration mode is accepting code that both NLL and AST-borrowck reject! #53026

pnkfelix opened this issue Aug 3, 2018 · 3 comments · Fixed by #53045
Assignees
Labels
A-NLL Area: Non Lexical Lifetimes (NLL) I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness NLL-sound Working towards the "invalid code does not compile" goal P-high High priority

Comments

@pnkfelix
Copy link
Member

pnkfelix commented Aug 3, 2018

D'oh: while investigating #52979, I found a case where we are downgrading the NLL errors to warnings (due to the migration mode added in #52681), but AST-borrowck issues an error for the test! (This may explain #53004, at least in part...)

Namely, ui/borrowck/issue-45983.rs. Here is the test:

fn give_any<F: for<'r> FnOnce(&'r ())>(f: F) {
f(&());
}
fn main() {
let x = None;
give_any(|y| x = Some(y));
//~^ ERROR borrowed data cannot be stored outside of its closure
}

Here is the AST-borrowck output (a hard error):

error: borrowed data cannot be stored outside of its closure
--> $DIR/issue-45983.rs:17:27
|
LL | let x = None;
| - borrowed data cannot be stored into here...
LL | give_any(|y| x = Some(y));
| --- ^ cannot be stored outside of its closure
| |
| ...because it cannot outlive this closure
error: aborting due to previous error

And here is the NLL output (another hard error):

warning: not reporting region error due to nll
--> $DIR/issue-45983.rs:17:27
|
LL | give_any(|y| x = Some(y));
| ^
error: borrowed data escapes outside of closure
--> $DIR/issue-45983.rs:17:18
|
LL | let x = None;
| - `x` is declared here, outside of the closure body
LL | give_any(|y| x = Some(y));
| - ^^^^^^^^^^^ `y` escapes the closure body here
| |
| `y` is a reference that is only valid in the closure body
error[E0594]: cannot assign to `x`, as it is not declared as mutable
--> $DIR/issue-45983.rs:17:18
|
LL | let x = None;
| - help: consider changing this to be mutable: `mut x`
LL | give_any(|y| x = Some(y));
| ^^^^^^^^^^^ cannot assign
error: aborting due to 2 previous errors

But here is the output from -Z borrowck=migrate -Z two-phase-borrows:

warning: not reporting region error due to nll
  --> ../src/test/ui/borrowck/issue-45983.rs:17:27
   |
17 |     give_any(|y| x = Some(y));
   |                           ^

warning: borrowed data escapes outside of closure
  --> ../src/test/ui/borrowck/issue-45983.rs:17:18
   |
16 |     let x = None;
   |         - `x` is declared here, outside of the closure body
17 |     give_any(|y| x = Some(y));
   |               -  ^^^^^^^^^^^ `y` escapes the closure body here
   |               |
   |               `y` is a reference that is only valid in the closure body
   |
   = warning: This error has been downgraded to a warning for backwards compatibility with previous releases.
           It represents potential unsoundness in your code.
           This warning will become a hard error in the future.

warning[E0594]: cannot assign to `x`, as it is not declared as mutable
  --> ../src/test/ui/borrowck/issue-45983.rs:17:18
   |
16 |     let x = None;
   |         - help: consider changing this to be mutable: `mut x`
17 |     give_any(|y| x = Some(y));
   |                  ^^^^^^^^^^^ cannot assign
   |
   = warning: This error has been downgraded to a warning for backwards compatibility with previous releases.
           It represents potential unsoundness in your code.
           This warning will become a hard error in the future.

ui/borrowck/issue-45983.rs is one particular instance of this, but now that I've found one, I'm certain there are more.

This is pretty bad. Very high priority to fix.

@pnkfelix pnkfelix self-assigned this Aug 3, 2018
@pnkfelix pnkfelix added A-NLL Area: Non Lexical Lifetimes (NLL) WG-compiler-nll P-high High priority I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness NLL-sound Working towards the "invalid code does not compile" goal labels Aug 3, 2018
@matthewjasper
Copy link
Contributor

This code looks suspicious:

if will_later_be_reported_by_nll && self.tcx.use_mir_borrowck() {
// With `#![feature(nll)]`, we want to present a nice user
// experience, so don't even mention the errors from the
// AST checker.
if self.tcx.features().nll {
return;
}

@pnkfelix
Copy link
Member Author

pnkfelix commented Aug 3, 2018

Ha I should have just left this to @matthewjasper to fix, since we seem to have independently identified the culprit.

@matthewjasper
Copy link
Contributor

I think I've already assigned myself to enough issues.

bors added a commit that referenced this issue Aug 6, 2018
…-ast-borrowck, r=estebank

Fix NLL migration mode so that reports region errors when necessary.

The code here was trying to be clever, and say "lets not report diagnostics when we 'know' NLL will report an error about them in the future."

The problem is that in migration mode, when no error was reported here, the NLL error that we "knew" was coming was downgraded to a warning (!).

Thus causing #53026

(I hope it is the only instance of such a scenario, but we will see.)

Anyway, this PR fixes that by only doing the "clever" skipping of region error reporting when we are not in migration mode. As noted in the FIXME, I'm not really thrilled with this band-aid, but it is small enough to be back-ported easily if that is necessary.

Rather than make a separate test for issue 53026, I just took the test that uncovered this in a first place, and extended it (via our revisions system) to explicitly show all three modes in action: AST-borrowck, NLL, and NLL migration mode.

(To be honest I hope not to have to add such revisions to many tests. Instead I hope to adopt some sort of new `compare-mode` for either borrowck=migrate or for the 2018 edition as a whole.)

Fix #53026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-NLL Area: Non Lexical Lifetimes (NLL) I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness NLL-sound Working towards the "invalid code does not compile" goal P-high High priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants