Tracking issue for `mutable_borrow_reservation_conflict` compatibility lint
pnkfelix opened this issue · 15 comments
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 #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: rust-lang/rfcs#2025
- 2017-08-26 2PB RFC accepted and merged rust-lang/rfcs#2025 (comment)
- 2017-12-06 2PB alpha implementation PRs: #46537, #47489, #48197
- 2018-02-22 2PB alpha implementation identified as overly general and broken; new beta implementation proposed #48431
- 2018-03-06 2PB beta implementation via PR #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 #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. #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"). #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 #58739
- 2019-03-06 results from crater run with restriction posted #58739 (comment)
- 2019-03-07 subset of T-lang design team discusses and @pnkfelix proposed PR #58739 be merged. #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 #58739, such as this summary comment from niko: #58739 (comment)
- 2019-03-25: compromise reached within T-lang design team: #58739 (comment)
Current status
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.)
Why are method receivers even handled before the arguments passed to the method?
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.
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 '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.
Thank you for following up, I appreciate it. I must have missed that.
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? :(
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, 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 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.)
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.
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.
@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 futureAnd I rewrote it as
self.password.chars().for_each(|c| {
let count = *(counts.get(&c).unwrap_or(&0));
let _ = counts.insert(c, count + 1);
});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?
@TonalidadeHidrica the warning will be going away in a future release, yes.