Remove mutable_borrow_reservation_conflict lint and allow the code pattern

[jackh726]

This was the only breaking issue with the NLL stabilization PR. Lang team decided to go ahead and allow this.

r? @nikomatsakis
Closes #59159
Closes #56254

[jackh726]

Not sure if this needs FCP or not. There's technically no new behavior.

[pnkfelix]

I really want to go through the exercise of verifying that the current version of stacked-borrows does indeed allow the coding pattern.

Its not that I don't believe @RalfJung when they told me that it's fine and stacked-borrows has to handle this, but ... its still better to double check that key assumption first...

[pnkfelix]

Okay well I at least started digging into it and got far enough in the playground to convince myself that there's nothing in today's miri that is going to "obviously" explode. :)

More simply put, miri successfully executed this:

    #![allow(mutable_borrow_reservation_conflict)]
    let mut v = vec![0, 1, 2];
    let shared = &v;
    v.push(shared.len());
    dbg!(v);

so I'm a little less worried than I was 17 minutes ago.

[joshtriplett]

Let's go ahead and confirm consensus on this (momentarily switching labels to T-lang for rfcbot):

@rfcbot merge

[rfcbot]

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

• @cramertj
• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[joshtriplett]

@rfcbot f? @RalfJung

[joshtriplett]

@rfcbot concern confirmation-from-ralf

@RalfJung, can we get explicit confirmation in this issue that Stacked Borrows won't have an issue with this anymore?

[RalfJung]

@pnkfelix we even have that (and more) as a testcase in Miri. ;)

can we get explicit confirmation in this issue that Stacked Borrows won't have an issue with this anymore?

Basically any aliasing model has no issue with 2-phase-borrows if it just entirely gives up on doing anything interesting with them, and treats them like (tagged) raw pointers. So as long as we are okay with (in the worst case) not doing any alias-based optimizations on 2phase-borrows, we are fine. This is exactly what I ended up doing with Stacked Borrows, after failing to do anything smarter.

[jackh726]

Concern didn't go through...maybe because it's now tagged T-compiler again?

[cramertj]

@RalfJung

So as long as we are okay with (in the worst case) not doing any alias-based optimizations on 2phase-borrows, we are fine.

Does this mean that we would potentially plan to actually detect places where two-phase is not necessary and optimize only those places, or is this a de-facto "we're not going to do alias-based optimizations"?

If the former, that sounds scary. If the latter, it seems odd to me that one might have to revert to using non-two-phase-borrows-style code in order to unlock optimizations, but that seems like probably a niche enough occurrence that it wouldn't be a huge deal in practice? I have a very hard time judging the impact of these kinds of decisions. Are you able to shed any light on the kinds of optimizations that could be missed in common two-phase-borrows usecases?

[bjorn3]

How easy would it be to support this pattern if we were to move borrowck to THIR after we switch to polonius? (If moving it to THIR is possible in the first place that is.)

[RalfJung]

Does this mean that we would potentially plan to actually detect places where two-phase is not necessary and optimize only those places, or is this a de-facto "we're not going to do alias-based optimizations"?

Many (most?) borrows are not two-phase borrows (including all the noalias we currently emit, since they are at function boundaries); those are unaffected.

But I admit I am not entirely familiar with the details of when two-phase borrows vs regular mutable borrows are used inside a function. (Shared references are fine anyway.)

The optimizations are the usual ones for aliasing guarantees -- reordering reads and writes around other code. The Stacked Borrows paper contains some examples.

How easy would it be to support this pattern if we were to move borrowck to THIR after we switch to polonius? (If moving it to THIR is possible in the first place that is.)

Is this directed at me? The questions are about the run-time aliasing model, so borrowck has no bearing on this.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[bjorn3]

Is this directed at me?

Not necessarily. Just throwing it out.

[saethlin]

How do we teach this? The only thing that I can imagine is that we now need to teach that "&mut requires uniqueness, except if you write code in waves hands this sort of pattern, in that case &mut isn't unique".

That does not sound good.

[rdrpenguin04]

That's basically where I'm at.

Hello, I'm writing the borrow checker for LCCC, and this behavior seems underspecified. If someone could define why this pattern works, then I'd be all for allowing it; as it stands, this is a nightmare to be compatible with.

I strongly recommend holding off on removing this lint until we have a document specifying exactly why this is valid.

Edit: Okay, we do have a document, RFC 2025 (activating borrows). This should be put into documentation though; this is the first I had heard of two-phase borrows.

[asquared31415]

I would like to point out that it is always sufficient as far as I am aware to transform the code that requires this:

    let mut v = vec![0, 1, 2];
    let shared = &v;
    v.push(shared.len());
    dbg!(v);

into code that instead uses a temporary binding for the inner expression:

    let mut v = vec![0, 1, 2];
    let shared = &v;
    let inner = shared.len();
    v.push(inner);
    dbg!(v);

and there is precedent for requiring users to rearrange code to satisfy the borrow checker or other things, such as reordering unique borrows to not overlap or creating a binding to hold a temporary.

[rdrpenguin04]

To be fair, there is precedent for requiring reordering, but that's not something we should aspire to do. Once I learned the rules for activating borrows, it made sense; the only question now is teaching it, which should be doable.

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[saethlin]

So as long as we are okay with (in the worst case) not doing any alias-based optimizations on 2phase-borrows, we are fine.

@RalfJung does this mean it is incorrect to emit noalias for a &mut which is actually a 2-phase-borrow?

[nikomatsakis]

@rfcbot fcp reviewed

[RalfJung]

does this mean it is incorrect to emit noalias for a &mut which is actually a 2-phase-borrow?

Yes, but we don't currently do that. Function arguments are always proper mutable borrows, on the callee side.

[nikomatsakis]

r=me if the lint is deprecated "properly"

[nikomatsakis]

I think we don't normally remove these altogether but rather move them to some list of deprecated lints ...

[jackh726]

It gets registered as removed here: https://github.com/rust-lang/rust/pull/96268/files#diff-cfb1d20a5949dd50b9769edb62eaf10a0de66734637fe71d35bdcf8fb06d767bR494

[jackh726]

@bors r=nikomatsakis rollup=iffy

Note to those that might try to add this to a rollup: tests might have changed that require a re-bless, but I'm not sure.

[bors]

📌 Commit 2300401 has been approved by nikomatsakis

[bors]

⌛ Testing commit 2300401 with merge 9a25164...

[bors]

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing 9a25164 to master...

[rust-highfive]

📣 Toolstate changed by #96268!

Tested on commit 9a25164.
Direct link to PR: #96268

💔 miri on windows: test-pass → test-fail (cc @oli-obk @RalfJung @eddyb).
💔 miri on linux: test-pass → test-fail (cc @oli-obk @RalfJung @eddyb).

[rust-timer]

Finished benchmarking commit (9a25164): comparison url.

Summary:

• Primary benchmarks: no relevant changes found
• Secondary benchmarks: 🎉 relevant improvement found

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count1	0	0	0	1	0
mean2	N/A	N/A	N/A	-1.7%	N/A
max	N/A	N/A	N/A	-1.7%	N/A

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

1. number of relevant changes ↩

2. the arithmetic mean of the percent change ↩