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

Use of unimplemented!() causing ICE with NLL #52058

Merged
merged 3 commits into from Jul 7, 2018

Conversation

Projects
None yet
4 participants
@davidtwco
Member

davidtwco commented Jul 4, 2018

Fixes #51345.

r? @nikomatsakis

None,
"never found an activation for this borrow!",
);

This comment has been minimized.

@nikomatsakis

nikomatsakis Jul 4, 2018

Contributor

I am a bit concerned here because I don't know where two-phase borrows that wind up unactivated will be set to TwoPhaseActivation::NotActivated. I think they will erroneously be recorded as not two phase. This means we will get an error with e.g. this code:

fn main() {
    let mut vec = vec![];
    vec.push((vec.len(), panic!()));
}

I expect this to error because the borrow of vec for calling push will incorrectly be considered "not two phase".

This comment has been minimized.

@nikomatsakis

nikomatsakis Jul 4, 2018

Contributor

To fix this, I think we can modify insert_as_pending_if_two_phase to set the activation_location field to NotActivated instead of NotTwoPhase. Then when we do find an activation we can assert that the field is "not activated".

This comment has been minimized.

@nikomatsakis

nikomatsakis Jul 4, 2018

Contributor

If I am correct about all this btw please add that above test. =)

This comment has been minimized.

@davidtwco

davidtwco Jul 4, 2018

Member

Pushed a commit that addresses this.

@@ -237,6 +237,14 @@ impl<'a, 'gcx, 'tcx> Visitor<'tcx> for GatherBorrows<'a, 'gcx, 'tcx> {
TwoPhaseActivation::NotActivated
}
_ => {
// Double check: We should have found an activation for every pending

This comment has been minimized.

@nikomatsakis

nikomatsakis Jul 4, 2018

Contributor

I think this commit is not quite correct -- that is, what we are checking here is that (a) this is indeed a 2-phase borrow and (b) we've not found some other activation (though we also check that above), right?

This comment has been minimized.

@davidtwco

davidtwco Jul 4, 2018

Member

Pushed improved comments.

@@ -322,6 +330,10 @@ impl<'a, 'gcx, 'tcx> GatherBorrows<'a, 'gcx, 'tcx> {
);
};
// Consider the borrow not activated.

This comment has been minimized.

@nikomatsakis

nikomatsakis Jul 4, 2018

Contributor

I'd say "Consider the borrow not activated to start. When we find an activation, we'll update this field."

This comment has been minimized.

@davidtwco

davidtwco Jul 4, 2018

Member

Pushed improved comments.

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Jul 4, 2018

@bors r+

@bors

This comment has been minimized.

Contributor

bors commented Jul 4, 2018

📌 Commit f90eada has been approved by nikomatsakis

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jul 7, 2018

Rollup merge of rust-lang#52058 - davidtwco:issue-51345, r=nikomatsakis
Use of unimplemented!() causing ICE with NLL

Fixes rust-lang#51345.

r? @nikomatsakis

bors added a commit that referenced this pull request Jul 7, 2018

Auto merge of #52123 - Mark-Simulacrum:rollup, r=Mark-Simulacrum
Rollup of 9 pull requests

Successful merges:

 - #51901 (Rc: remove unused allocation and fix segfault in Weak::new())
 - #52058 (Use of unimplemented!() causing ICE with NLL)
 - #52067 (Visit the mir basic blocks in reverse-postfix order)
 - #52083 (Dont run ast borrowck on mir mode)
 - #52099 (fix typo in stable `--edition` error message)
 - #52103 (Stabilize rc_downcast)
 - #52104 (Remove unnecessary feature gate.)
 - #52117 (Dedupe filetime)
 - #52120 (ARM: expose the "mclass" target feature)

Failed merges:

r? @ghost

@bors bors merged commit f90eada into rust-lang:master Jul 7, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@davidtwco davidtwco deleted the davidtwco:issue-51345 branch Aug 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment