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

MIR-borrowck: ICE during diagnostic search for assignment (unwrap of None) #45199

Closed
pnkfelix opened this issue Oct 11, 2017 · 5 comments
Closed
Labels
A-diagnostics Area: Messages for errors, warnings, and lints I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Milestone

Comments

@pnkfelix
Copy link
Member

pnkfelix commented Oct 11, 2017

PR #44806 added some code to search for the lexically first assignment in order to report an illegal assignment.

However, oddly enough, for some tests this search, which ends in an unwrap, is causing the compiler to ICE.

For example:

%  build/x86_64-apple-darwin/stage1/bin/rustc  -Z borrowck-mir \
     ../src/test/compile-fail/borrowck/borrowck-for-loop-head-linkage.rs
[...]
error[E0384]: re-assignment of immutable variable `val` (Mir)
  --> ../src/test/compile-fail/borrowck/borrowck-for-loop-head-linkage.rs:15:5
   |
15 |        for &x in &vector {
   |   _____^
   |  |_____|
   | ||
16 | ||         let cap = vector.capacity();
17 | ||         vector.extend(repeat(0));      //~ ERROR cannot borrow
18 | ||         vector[1] = 5;   //~ ERROR cannot borrow
19 | ||     }
   | ||     ^
   | ||_____|
   | |______re-assignment of immutable variable
   |        first assignment to `val`

error[E0384]: re-assignment of immutable variable `x` (Mir)
  --> ../src/test/compile-fail/borrowck/borrowck-for-loop-head-linkage.rs:15:10
   |
15 |     for &x in &vector {
   |          ^
   |          |
   |          re-assignment of immutable variable
   |          first assignment to `x`

error: internal compiler error: unexpected panic

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports

note: rustc 1.22.0-dev running on x86_64-apple-darwin

thread 'rustc' panicked at 'called `Option::unwrap()` on a `None` value', /Users/fklock/Dev/Mozilla/rust.git/src/libcore/option.rs:335:20
@pnkfelix pnkfelix added WG-compiler-nll I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 11, 2017
@pnkfelix
Copy link
Member Author

Its worth noting that the diagnostic reported by AST-borrowck is very different from that for MIR-borrowck. This is almost certainly because the MIR-borrowck is seeing a desugared for-loop (and thus we're seeing reports of an assignment to val that does not actually occur in the surface level syntax.

Nonetheless, part of resolving this bug might include trying figure out how to make the MIR-borrowck diagnostic more understandable to the end user in this scenario.

@pnkfelix pnkfelix added the A-diagnostics Area: Messages for errors, warnings, and lints label Oct 11, 2017
@pnkfelix
Copy link
Member Author

(another approach that I did not consider earlier: Change the MIR construction to accommodate for-loops. E.g. maybe the MIR assignment statement could take a flag indicating that it is an assignment arising from for-desugaring, and thus it is "safe" even when it is reassigning a variable declared without mut...)

@arielb1 arielb1 changed the title MIR-borrowck: ICE during diagnostic search for assignment MIR-borrowck: ICE during diagnostic search for assignment (unwrap of None) Nov 1, 2017
@matthewjasper
Copy link
Contributor

Working on this (and this error message in general)

@nikomatsakis
Copy link
Contributor

@matthewjasper nice! please stop in on gitter with any questions, if you've not done so already.

@arielb1 arielb1 added this to the NLL prototype milestone Nov 15, 2017
@matthewjasper
Copy link
Contributor

Minified case (that still ICEs after #45936)

fn main() {
    let x = String::new();
    x = String::new();
}

arielb1 pushed a commit to arielb1/rust that referenced this issue Nov 27, 2017
…rror, r=arielb1

Mir Borrowck: Parity with Ast for E0384 (Cannot assign twice to immutable)

- Closes rust-lang#45199
- Don't allow assigning to dropped immutable variables
- Show the "first assignment" note on the first assignment that can actually come before the second assignment.
- Make "first assignment" notes point to function parameters if needed.
- Don't show a "first assignment" note if the first and second assignment have the same span (in a loop). This matches ast borrowck for now, but maybe this we should add "in previous loop iteration" as with some other borrowck errors. (Commit 2)
- Use revisions to check mir borrowck for the existing tests for this error. (Commit 3)

~~Still working on a less ad-hoc way to get 'first assignment' notes to show on the correct assignment. Also need to check mutating function arguments.~~ Now using a new dataflow pass.
bors added a commit that referenced this issue Nov 27, 2017
…elb1

Mir Borrowck: Parity with Ast for E0384 (Cannot assign twice to immutable)

- Closes #45199
- Don't allow assigning to dropped immutable variables
- Show the "first assignment" note on the first assignment that can actually come before the second assignment.
- Make "first assignment" notes point to function parameters if needed.
- Don't show a "first assignment" note if the first and second assignment have the same span (in a loop). This matches ast borrowck for now, but maybe this we should add "in previous loop iteration" as with some other borrowck errors. (Commit 2)
- Use revisions to check mir borrowck for the existing tests for this error. (Commit 3)

~~Still working on a less ad-hoc way to get 'first assignment' notes to show on the correct assignment. Also need to check mutating function arguments.~~ Now using a new dataflow pass.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants