-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Ensure StorageDead is created even if variable initialization fails #52046
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
Conversation
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Ping from triage @eddyb! This PR needs your review. |
src/librustc_mir/build/scope.rs
Outdated
DropKind::Value { .. } => if !needs_drop { return }, | ||
DropKind::Storage => { | ||
match *place { | ||
Place::Local(index) if index.index() > self.arg_count => (), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be replaced with an assert, that the place is a local, and that the index is larger than arg_count
? Given the changes in args_and_body
, I expect this to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
477f3b1
to
162cba7
Compare
@eddyb A bunch of other mir-opt tests fail as a result of this PR due to it introducing duplicate |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
src/librustc_mir/build/scope.rs
Outdated
index.index(), | ||
self.arg_count) | ||
}, | ||
_ => panic!("`schedule_drop` called with non-`Local` place {:?}", place), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're not using assert_eq
etc, you can use bug!
instead of panic!
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switched to span_bug
.
b6242d3
to
1e5c6a7
Compare
1e5c6a7
to
97903bf
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
97903bf
to
48ecb2e
Compare
kind: StatementKind::StorageDead(index) | ||
}); | ||
// Drop the storage for both value and storage drops. | ||
// Only temps and vars need their storage dead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have a check above, I don't think you need another here. You can keep an assert though, and have an unreachable!()
in the _ => {}
arm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/test/mir-opt/issue-49232.rs
Outdated
// option. This file may not be copied, modified, or distributed | ||
// except according to those terms. | ||
|
||
// compile-flags: -Z identify_regions -Z emit-end-regions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need these two lines? (cc @nikomatsakis @arielb1) I expect the MIR to be shorter and easier to read without them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done-- made it a bit shorter.
r=me with nits fixed |
48ecb2e
to
9c15a66
Compare
@bors r=eddyb |
📌 Commit 9c15a66 has been approved by |
} | ||
|
||
// END RUST SOURCE | ||
// START rustc.main.mir_map.0.mir |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, what happens if you use the MIR after the first simplifycfg? There should be mir-opt
examples around that to that. I'm hoping the distinction is not only visible, but much clearer. Right now there are a bunch of pointless blocks being created.
☀️ Test successful - status-appveyor, status-travis |
Rebase and slight cleanup of #51109
Fixes #49232
r? @eddyb