Rust: Tracking issue for `mutable_borrow_reservation_conflict` compatibility lint

Created on 13 Mar 2019  路  11Comments  路  Source: rust-lang/rust

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:

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:

Current status

  • [x] PR #58739 introduces the mutable_borrow_reservation_conflict lint as warn-by-default
  • [ ] PR #76104 makes the mutable_borrow_reservation_conflict lint deny-by-default
  • [ ] PR ? makes the mutable_borrow_reservation_conflict lint a hard error
A-NLL A-lint C-future-compatibility T-compiler
Source
picture pnkfelix
👍2

Most helpful comment

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

picture nox on 12 Apr 2019
👍4

All 11 comments

From https://github.com/rust-lang/rust/pull/58739#issuecomment-476387184, 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.)
pnkfelix picture pnkfelix on 28 Mar 2019

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

nox picture nox on 12 Apr 2019
👍4

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.

nox picture nox on 12 Apr 2019
👍2

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?

andrewchambers picture andrewchambers on 1 Jun 2019
👍2

@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.

pnkfelix picture pnkfelix on 9 Jul 2019

Thank you for following up, I appreciate it. I must have missed that.

andrewchambers picture andrewchambers on 10 Jul 2019

Hi!
I've hit this warning recently and I've got big concerns about some usage patterns. Basically, let's consider the following code:

use std::ops::Range;

pub struct RangedDirtyFlag<T> {
    pub range: Option<Range<T>>,
}

impl<T: PartialOrd + Copy> RangedDirtyFlag<T> {
    pub fn set(&mut self, ix: T) {
        match &self.range {
            None => self.range = Some(Range { start: ix, end: ix }),
            Some(r) => {
                if ix < r.start {
                    self.range = Some(Range { start: ix, ..(*r) })
                } else if ix > r.end {
                    self.range = Some(Range { end: ix, ..(*r) })
                }
            }
        }
    }
}

It has a terrible amount (at least in my taste) of repetitions that I would like to refactor. The only possibility to refactor it without affecting performance that I see now is the following:

use std::ops::Range;

pub struct RangedDirtyFlag<T> {
    pub range: Option<Range<T>>,
}

impl<T: PartialOrd + Copy> RangedDirtyFlag<T> {
    pub fn replace_range(&mut self, new_range: Range<T>) {
        self.range = Some(new_range)
    }

    pub fn set(&mut self, ix: T) {
        match &self.range {
            None => self.replace_range(Range { start: ix, end: ix }),
            Some(r) => {
                if ix < r.start {
                    self.replace_range(Range { start: ix, ..(*r) })
                } else if ix > r.end {
                    self.replace_range(Range { end: ix, ..(*r) })
                }
            }
        }
    }
}

Which will be now rejected by the compiler ... unless this issue will consider non-lexical-lifetimes, as it does in the original code. Until its done, we cannot refactor such boilerplate to nicer code, are we? :(

wdanilo picture wdanilo on 9 Oct 2019

@wdanilo

impl<T: PartialOrd + Copy> RangedDirtyFlag<T> {
    pub fn set(&mut self, ix: T) {
        let r = match &mut self.range {
            None => return self.range = Some(ix..ix),
            Some(range) => range,
        };
        if ix < r.start {
            r.start = ix;
        } else if ix > r.end {
            r.end = ix;
        }
    }
}
nox picture nox on 9 Oct 2019

@nox, Thanks for the reply! Ok, that was a pretty simple example. But imagine that in my example the replace_range is a really complex method, mutating the object. And I just want to use it from inside of the pattern match. Such thing (mutating an object based on some pattern match) is rather a popular operation in languages that offer ADTs. This would be a problem after this change, right?

wdanilo picture wdanilo on 11 Oct 2019

@wdanilo the rewrite suggested above in the description (under "How to fix this warning/error") seems like it applies fine to your case (play):

impl<T: PartialOrd + Copy> RangedDirtyFlag<T> {
    pub fn set(&mut self, ix: T) {
        match &self.range {
            None => self.replace_range(Range { start: ix, end: ix }),
            Some(r) => {
                if ix < r.start {
                    let new_range = Range { start: ix, ..(*r) };
                    self.replace_range(new_range)
                } else if ix > r.end {
                    let new_range = Range { end: ix, ..(*r) };
                    self.replace_range(new_range)
                }
            }
        }
    }
}

This seems to me like it satisfies the desired attributes for your hypothetical refactoring; it just needs to explicitly ensure the new ranges are constructed (and the lifetime of r ends) before the invocation of replace_range. Are you expecting this to have a different performance profile than the original example you wrote?

(Also, note these are non-lexical lifetimes: The lifetime 'a associated with the reference r: &'a Range is decoupled from the lexical scope of r itself, and instead ends at the last usage of r, as demonstrated above.)

pnkfelix picture pnkfelix on 13 Nov 2019

FYI, I've opened #76104 in order to put the lint in deny mode by default since its been 1 year and a half since the warning has been merged.

marmeladema picture marmeladema on 1 Sep 2020
👍2
Was this page helpful?
0 / 5 - 0 ratings

Related issues

tikue picture tikue  路  3Comments
defuz picture defuz  路  3Comments
drewcrawford picture drewcrawford  路  3Comments
lambda-fairy picture lambda-fairy  路  3Comments
Robbepop picture Robbepop  路  3Comments