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

Issue 48697 -- ICE in old borrowck mode #49666

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
16 changes: 11 additions & 5 deletions src/librustc_borrowck/borrowck/check_loans.rs
Original file line number Diff line number Diff line change
Expand Up @@ -417,11 +417,17 @@ impl<'a, 'tcx> CheckLoanCtxt<'a, 'tcx> {
RegionKind::ReLateBound(..) |
RegionKind::ReFree(..) |
RegionKind::ReStatic => {
self.bccx
.tcx
.sess.delay_span_bug(borrow_span,
&format!("unexpected region for local data {:?}",
loan_region));
if !self.bccx.tcx.nll() {
self.bccx
.tcx
.sess.delay_span_bug(borrow_span,
&format!("unexpected region for local data {:?}",
loan_region));
} else {
// When in NLL mode, invalid region errors get
// suppressed; it ought to be reported later, by
// the NLL region analysis.
}
return
}

Expand Down
15 changes: 6 additions & 9 deletions src/librustc_borrowck/borrowck/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,15 +120,12 @@ fn borrowck<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, owner_def_id: DefId)
used_mut_nodes: RefCell::new(FxHashSet()),
};

// Eventually, borrowck will always read the MIR, but at the
// moment we do not. So, for now, we always force MIR to be
// constructed for a given fn, since this may result in errors
// being reported and we want that to happen.
//
// Note that `mir_validated` is a "stealable" result; the
// thief, `optimized_mir()`, forces borrowck, so we know that
// is not yet stolen.
tcx.mir_validated(owner_def_id).borrow();
// Force the MIR-based borrow checker to run; this also forces the
Copy link
Member

Choose a reason for hiding this comment

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

I don't 100% understand your logic in making this change part of this PR.

If you had not already added a guard elsewhere with a "if tcx.sess.nll(), then do not call bug()", then maybe I could understand forcing MIR borrowck to run.

But why do you need to force MIR borrow check to be run when you have the aforementioned guard in place on the ICE? Doesn't that avoid the problem (since we will call bug() when we are not in feature(nll) mode in that case?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you are correct that this change is not needed. I thought it would allow me to use span-delay-bug initially but that turned out not to be the case.

// MIR to be built. This may lead to errors being reported,
// particularly in NLL mode, where some ordinary region errors are
// suppressed. It's important that those errors get reported to
// avoid ICEs like #48697.
let _ = tcx.mir_borrowck(owner_def_id);

// option dance because you can't capture an uninitialized variable
// by mut-ref.
Expand Down
8 changes: 7 additions & 1 deletion src/librustc_mir/transform/elaborate_drops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,13 @@ impl MirPass for ElaborateDrops {
_ => return
}
let param_env = tcx.param_env(src.def_id);
let move_data = MoveData::gather_moves(mir, tcx).unwrap();
let move_data = match MoveData::gather_moves(mir, tcx) {
Ok(d) => d,
Err(_) => {
tcx.sess.delay_span_bug(mir.span, "ElaborateDrops: failed to gather move data");
return;
}
};
let elaborate_patch = {
let mir = &*mir;
let env = MoveDataParamEnv {
Expand Down
28 changes: 28 additions & 0 deletions src/test/ui/nll/issue-48697.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// 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.

// Test that we do not ICE when we have suppressed a lexical region
// error that would otherwise cause AST borrowck to die.
//
// Regression test for #48697.
//
// run-pass

#![feature(nll)]
#![allow(warnings)]

fn foo(x: &i32) -> &i32 {
let z = 4;
let f = &|y| { y };
let k = f(&z);
f(x)
}

fn main() {}
8 changes: 8 additions & 0 deletions src/test/ui/nll/issue-48697.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
error: internal compiler error: unexpected region for local data ReFree(DefId(0/0:3 ~ issue_48697[317d]::foo[0]), BrAnon(0))
Copy link
Member

Choose a reason for hiding this comment

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

@nikomatsakis : You say you are removing the ICE, but the expected stderr you have encoded here is an ICE?

--> $DIR/issue-48697.rs:22:16
|
LL | let k = f(&z);
| ^

error: aborting due to previous error