Tracking issue for mutable_borrow_reservation_conflict compatibility lint

[pnkfelix]

This is the summary issue for the mutable_borrow_reservation_conflict future-compatibility warning and other related errors. The goal of this page is describe why this change was made and how you can fix code that is affected by it. It also provides a place to ask questions or register a complaint if you feel the change should not be made. For more information on the policy around future-compatibility warnings, see our breaking change policy guidelines.

What is this lint about

A two-phase borrow is a mutable-borrow that is initially reserved at one point in the program's control-flow, and then subsequently activated at a later point in the control-flow.

For example, given a vector v, v.push(v.len()) first reserves a borrow of v when it evaluates the method receiver, but does not activate that borrow until later when transferring control to the push method itself, after v.len() has been evaluated.

This lint detects instances where the reservation itself conflicts with some pre-existing borrow. For example:

   let mut v = vec![0, 1, 2];
   let shared = &v;
             // ~~ 
             // a shared borrow of `v` starts here ...
   v.push(shared.len());
// ~      ~~~~~~
// |      ... and that shared borrow is used here...
// |
// ... but that comes after the reservation of the
//     mutable borrow of `v` here (the reservation
//     is subsequently activated when `push` is invoked)

The latter code is an example of code that was accepted when two-phased borrows (2PB) initially landed, as part of non-lexical lifetimes (NLL) deployment in the 2018 edition.

This lint detects such cases, and warns that this pattern may be rejected by a future version of the compiler.

This is much further discussion of this at the following places:

• Issue [NLL] prohibit "two-phase borrows" with existing borrows? #56254
• RalfJung blog post: https://www.ralfj.de/blog/2018/11/16/stacked-borrows-implementation.html
  (The history section below attempts to provide a summary of the events that led to this lint being developed and the current status of the lint.)

How to fix this warning/error

Revise the code so that the initial evaluation of the mutable borrow (the "reservation") always comes after all uses of shared borrows it conflicts with.

One example revision of the example above:

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

Now, the last use of shared comes before the reservation in v.push(len), and thus there is no conflict between the shared borrow and the mutable borrow reservation.

Historical background

At the time NLL was stabilized, this borrowing pattern was not meant to be accepted, because it complicates the abstract model for borrows and thus poses problems for unsafe code authors and for future compiler optimizations. (How much complication it introduces is a matter of debate, which is in part why this restriction is being given treatment different from other future compatibility lints.)

In other words: at the time that NLL was stabilized, the compiler's acceptance of this borrowing pattern was categorized by the NLL team as a "known bug". The NLL team assumed that, as a bug fix, the compiler would be allowed to start rejecting the pattern in the future.

Whether a future version of the compiler rejects the code will depend on an investigation of potential abstract models of the language semantics; we will not convert the lint to a hard error without first performing an evaluation of such abstract models.

Timeline:

• 2017-03-01 2PB blog post
• 2017-06-09 2PB RFC posted: RFC: Enable nested method calls rfcs#2025
• 2017-08-26 2PB RFC accepted and merged RFC: Enable nested method calls rfcs#2025 (comment)
• 2017-12-06 2PB alpha implementation PRs: [MIR-borrowck] Two phase borrows #46537, NLL: Limit two-phase borrows to autoref-introduced borrows #47489, Allow two-phase borrows of &mut self in ops #48197
• 2018-02-22 2PB alpha implementation identified as overly general and broken; new beta implementation proposed NLL: Redo two-phase borrows with conservative, narrow focus #48431
• 2018-03-06 2PB beta implementation via PR Two phase borrows rewrite #48770
• 2018-08-07: @RalfJung proposes "stacked borrows" semantic model for Rust borrowing and aliasing of references: blog post internals discussion
• 2018-11-16 stacked borrows implemented blog post
• 2018-11-26 @RalfJung points out that the stacked borrows model does not allow for pre-existing shares to overlap with reservations for a two-phase borrow [NLL] prohibit "two-phase borrows" with existing borrows? #56254
• 2018-11-28 The NLL team decides that it should not attempt to rush to make a backportable change implementing the pre-existing share restriction. [NLL] prohibit "two-phase borrows" with existing borrows? #56254 (comment)
• 2018-11-29 The T-lang design team discussed the matter; it did not reach consensus about whether the NLL team was correct in its assumption that it could land the restriction later (by classifying the change as a "soundness fix"). [NLL] prohibit "two-phase borrows" with existing borrows? #56254 (comment)
• For the next four months, debate continued off and on about whether this borrowing pattern should be rejected or not.
• 2019-02-25 Pull request implementing the proposed restriction is posted: PR More restrictive 2 phase borrows - take 2 #58739
• 2019-03-06 results from crater run with restriction posted More restrictive 2 phase borrows - take 2 #58739 (comment)
• 2019-03-07 subset of T-lang design team discusses and @pnkfelix proposed PR More restrictive 2 phase borrows - take 2 #58739 be merged. More restrictive 2 phase borrows - take 2 #58739 (comment)
• For next three weeks, various people both within and apart from T-lang debate whether the restriction can be landed, given that NLL with a less-restricted 2PB was stabilized. Much of that debate is encoded in comments on PR More restrictive 2 phase borrows - take 2 #58739, such as this summary comment from niko: More restrictive 2 phase borrows - take 2 #58739 (comment)
• 2019-03-25: compromise reached within T-lang design team: More restrictive 2 phase borrows - take 2 #58739 (comment)

Current status

• PR More restrictive 2 phase borrows - take 2 #58739 introduces the mutable_borrow_reservation_conflict lint as warn-by-default
• PR Switch mutable_borrow_reservation_conflict lint to deny by default #76104 makes the mutable_borrow_reservation_conflict lint deny-by-default
• PR ? makes the mutable_borrow_reservation_conflict lint a hard error

[pnkfelix]

From #58739 (comment), some follow-up tasks are associated with this 2-phase borrow (2PB) issue:

• We need to develop and evaluate alternative semantic models,
• We need to pin down more precisely the optimization impact, even if it is only a potential impact as of now,
• We need to specify the impact of this choice on the rules for unsafe code.
• (Also, on a meta-level, the T-lang design team needs do a retrospective on our processes and find changes that would help prevent repeat occurrences of the mistake (or at least misunderstanding) that occurred here.)

[nox]

Why are method receivers even handled before the arguments passed to the method?

[nox]

I just connected my two last brain cells alive on this Friday evening and just realised that evaluating the method receiver last would make for a very confusing control flow with chained method calls, disregard me.

[andrewchambers]

I am registering a complaint - Very irritating to be told by users about this warning appearing in previously released crates. So old crates will just break without an edition change? How is that supposed to work?

Has there been a survey of crates.io to determine how many crates will stop compiling?

[pnkfelix]

@andrewchambers 's complaint above included the question:

Has there been a survey of crates.io to determine how many crates will stop compiling?

And the answer is Yes. In particular, the timeline in the issue description includes a bullet from 2019-03-06, describing the crater run we did as part of investigating whether to add the diagnostic detecting violations of the 2 phase borrows restriction.

[svanderbleek]

This came up for me I had to turn

let parent_name = self.paths.get(&parent).expect(&error);
if param {
  self.paths.insert(id, format!("{}/:{}", parent_name, name));
} else {
  self.paths.insert(id, format!("{}/{}", parent_name, name));
}

into

let parent_name = self.paths.get(&parent).expect(&error);
if param {
  let id_name = format!("{}/:{}", parent_name, name);
  self.paths.insert(id, id_name);
} else {
  let id_name = format!("{}/{}", parent_name, name);
  self.paths.insert(id, id_name);
}

which seems like a misapplication of the trigger for the warning. I was able to reproduce with the following code

let mut h: HashMap<String, String> = HashMap::new();
h.insert(String::from("a"), String::from("a"));
let a = h.get("a").unwrap();
h.insert(String::from("b"), format!("{}", a));

which produces

warning: cannot borrow `h` as mutable because it is also borrowed as immutable
 --> src/main.rs:7:3
  |
6 |   let a = h.get("a").unwrap();
  |           - immutable borrow occurs here
7 |   h.insert(String::from("b"), format!("{}", a));
  |   ^ mutable borrow occurs here              - immutable borrow later used here
  |
  = note: `#[warn(mutable_borrow_reservation_conflict)]` on by default
  = warning: this borrowing pattern was not meant to be accepted, and may become a hard error in the future
  = note: for more information, see issue #59159 <https://github.com/rust-lang/rust/issues/59159>

into

let mut h: HashMap<String, String> = HashMap::new();
h.insert(String::from("a"), String::from("a"));
let a = h.get("a").unwrap();
let v = format!("{}", a);
h.insert(String::from("b"), v);

which produces no warnings.

Is there anywhere I should follow this up? I'm new to Rust.

[kristof-mattei]

@svanderbleek https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=6ed725446c9194c9ad0314cfbdb9967b

If you clone a before inserting you're good.

I ran into this warning-to-become as well:

let mut counts: HashMap<char, u32> = HashMap::new();

self.password
    .chars()
    .for_each(|c| match counts.get(&c) {
        None => {
            counts.insert(c, 1);
        }
        Some(t) => {
            let _ = counts.insert(c, *t + 1);
        }
    });

yielded

warning: cannot borrow `counts` as mutable because it is also borrowed as immutable
  --> src/day_2/part_1.rs:23:29
   |
18 |             .for_each(|c| match counts.get(&c) {
   |                                 ------ immutable borrow occurs here
...
23 |                     let _ = counts.insert(c, *t + 1);
   |                             ^^^^^^           -- immutable borrow later used here
   |                             |
   |                             mutable borrow occurs here
   |
   = note: `#[warn(mutable_borrow_reservation_conflict)]` on by default
   = warning: this borrowing pattern was not meant to be accepted, and may become a hard error in the future

And I rewrote it as

self.password.chars().for_each(|c| {
    let count = *(counts.get(&c).unwrap_or(&0));

    let _ = counts.insert(c, count + 1);
});

[TonalidadeHidrica]

As this issue was closed by merging #96268 , can we now safely ignore this warning? I'm using 1.61.0 stable. Will this warning disappear if I updated to 1.62.0 in the future?

[nikomatsakis]

@TonalidadeHidrica the warning will be going away in a future release, yes.