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: broken MIR moving out of Lvalue #44830

Closed
pnkfelix opened this issue Sep 25, 2017 · 5 comments
Closed

MIR-borrowck: ICE: broken MIR moving out of Lvalue #44830

pnkfelix opened this issue Sep 25, 2017 · 5 comments
Labels
A-borrow-checker Area: The borrow checker C-bug Category: This is a bug. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. 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.

Comments

@pnkfelix
Copy link
Member

pnkfelix commented Sep 25, 2017

There are many tests in compile-fail/borrowck/ that ICE with the same failure: "broken MIR moving out of Lvalue"

  • You can see list of such tests in the discrepancy spreadsheet.
  • But for ease of reference, I am also transcribing that list into the first comment on this issue.

pnkfelix believes the origin of this problem is that some code processing MIR (probably MIR dataflow) was written under the assumption that all valid MIR will not have moves out of Lvalues, at least for non-Copy values. But of course such an assumption only holds for MIR that has actually passed the borrowck analysis (at least under our current separation of concerns in the static analyses of rustc), and therefore any dataflow code running prior to borrowck on some (erroneous) Rust source file is going to potentially encounter such MIR.

  • Zoxc has pointed out that it is probably librustc_mir/dataflow/move_paths/builder.rs that is the culprit here.

The answer here is not to panic from rustc, but instead to fail gracefully in some manner and report a nice error diagnostic to the user.

@pnkfelix
Copy link
Member Author

pnkfelix commented Sep 26, 2017

Transcription of relevant tests according to the discrepancy spreadsheet. (caveat: data is based on out-of-date pnkfelix/mir-borrowck4 branch)

tests failing with ICE: broken MIR moving out of Lvalue ...
borrowck-fn-in-const-a.rs
borrowck-for-loop-correct-cmt-for-pattern.rs
borrowck-issue-2657-2.rs
borrowck-move-by-capture.rs
borrowck-move-error-with-note.rs
borrowck-move-in-irrefut-pat.rs
borrowck-move-out-of-overloaded-auto-deref.rs
borrowck-move-out-of-overloaded-deref.rs
borrowck-move-out-of-static-item.rs
borrowck-move-out-of-struct-with-dtor.rs
borrowck-move-out-of-tuple-struct-with-dtor.rs
borrowck-move-out-of-vec-tail.rs
borrowck-overloaded-index-move-from-vec.rs
borrowck-struct-update-with-dtor.rs
borrowck-vec-pattern-nesting.rs
move-in-static-initializer-issue-38520.rs

@pnkfelix pnkfelix added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-compiler-nll E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Sep 26, 2017
@TimNN TimNN added A-borrow-checker Area: The borrow checker C-bug Category: This is a bug. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ labels Sep 27, 2017
@pnkfelix
Copy link
Member Author

pnkfelix commented Oct 3, 2017

I'm going to focus on this bug.

@arielb1
Copy link
Contributor

arielb1 commented Oct 3, 2017

This shouldn't be so hard. You'll need to remove the ICE and replace it with proper permission checking that emits the correct errors.

@pnkfelix
Copy link
Member Author

pnkfelix commented Oct 3, 2017

Yeah, the biggest part is probably just the refactoring of the existing errors being collected and emitted within AST borrowck, and moving those to the common diagnostics that live in rustc_mir::util::borrowck_errors.

@pnkfelix
Copy link
Member Author

pnkfelix commented Oct 3, 2017

(my basic goal at this point is to remove all of the ICE's first so that we can then focus on the more interesting places where mir-borrowck is actually producing wrong answers.)

bors added a commit that referenced this issue Oct 8, 2017
…rrors, r=nikomatsakis

MIR-borrowck: gather and signal any move errors

When building up the `MoveData` structure for a given MIR, also accumulate any erroneous actions, and then report all of those errors when the construction is complete.

This PR adds a host of move-related error constructor methods to `trait BorrowckErrors`. I think I got the notes right; but we should plan to audit all of the notes before turning MIR-borrowck on by default.

Fix #44830
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-borrow-checker Area: The borrow checker C-bug Category: This is a bug. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. 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