Skip to content

borrowck dataflow: respect borrow shallowness when killing borrows that conflict with assignments #160902

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.

Sign up for GitHub

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

Open
dianne wants to merge 1 commit into
base: main
Choose a base branch
from
+59 −4
Open

borrowck dataflow: respect borrow shallowness when killing borrows that conflict with assignments #160902

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions compiler/rustc_borrowck/src/dataflow.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@ use rustc_mir_dataflow::impls::{
use rustc_mir_dataflow::{Analysis, GenKill, JoinSemiLattice};
use tracing::debug;

use crate::{BorrowSet, PlaceConflictBias, PlaceExt, RegionInferenceContext, places_conflict};
use crate::{
AccessDepth, BorrowSet, PlaceConflictBias, PlaceExt, RegionInferenceContext, places_conflict,
};

// This analysis is different to most others. Its results aren't computed with
// `iterate_to_fixpoint`, but are instead composed from the results of three sub-analyses that are
Expand Down Expand Up @@ -474,7 +476,7 @@ impl<'a, 'tcx> Borrows<'a, 'tcx> {

// If the borrowed place is a local with no projections, all other borrows of this
// local must conflict. This is purely an optimization so we don't have to call
// `places_conflict` for every borrow.
// `places_conflict::borrow_conflicts_with_place` for every borrow.
if place.projection.is_empty() {
if !self.body.local_decls[place.local].is_ref_to_static() {
state.kill_all(other_borrows_of_local);
Expand All @@ -487,11 +489,13 @@ impl<'a, 'tcx> Borrows<'a, 'tcx> {
// will be assured that two places being compared definitely denotes the same sets of
// locations.
let definitely_conflicting_borrows = other_borrows_of_local.filter(|&i| {
places_conflict(
places_conflict::borrow_conflicts_with_place(
self.tcx,
self.body,
self.borrow_set[i].borrowed_place,
place,
self.borrow_set[i].kind,
place.as_ref(),
AccessDepth::Deep,

@dianne dianne Aug 11, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied the AccessDepth::Deep from places_conflict::places_conflict, so this will have the same behavior as before in that regard, but I'm wary about it. kill_borrows_on_place is called on assignments and StorageDeads, which are shallow accesses. Consequently, definitely_conflicting_borrows can contain non-conflicting borrows, which are then killed. e.g.

fn example(x: &mut u8, something_else: &mut u8) {
    let mut y = (x,);
    // This introduces a borrow of `*y.0`.
    let z = &mut *y.0;
    // This kills the borrow of `*y.0`, despite it not conflicting with that.
    y.0 = something_else;
    // At this point, no borrows are in scope, according to `borrows_in_scope`.
    z;
}

I haven't been able to coax unsoundness out of it, but it feels strange. I tried changing it to AccessDepth::Shallow(None) to see what would happen, but it broke some tests that rely on non-conflicting borrows being killed, e.g. tests/ui/borrowck/issue-62007-assign-box.rs and tests/ui/borrowck/issue-62007-assign-field.rs. As I understand it, the borrows there go through derefs, so the assignments don't technically conflict with them (as in the above example), but they need to be killed for the loops to work.

The fast path for assignments to locals also seems not to account for assignments being shallow accesses. e.g.

fn example(x: &mut u8, something_else: &mut u8) {
    let mut y = x;
    // This introduces a borrow of `*y`.
    let z = &mut *y;
    // This kills the borrow of `*y`, despite it not conflicting with that.
    y = something_else;
    // At this point, no borrows are in scope, according to `borrows_in_scope`.
    z;
}

Maybe I'm missing something?

View changes since the review

@matthewjasper matthewjasper Aug 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is definitely a part of borrowck with a confusing implementation. The check here is looking for cases where the assignment overwrites the reference to the borrowed place. The assumption is that this would be cases where there is a conflicts with AccessDepth::Deep but not AccessDepth::Shallow(None). The reason that the kill only checks AccessDepth::Deep here is that the check with AccessDepth::Shallow(None) in visit_after_early_statement_effect is done on the borrow state before the kill happens, so there's an error either way.

PlaceConflictBias::NoOverlap,
)
});
Expand Down
32 changes: 32 additions & 0 deletions tests/ui/borrowck/dont-kill-shallow-borrows-on-child-writes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
//! Regression test for <https://github.com/rust-lang/rust/issues/160599>. On assignment statements,
//! borrowck's dataflow analysis kills borrows that it knows conflict with the assigment: a borrow
//! of a place can't be live anymore after that place is assigned over. Previously, this didn't
//! account for fake borrows being shallow: it would kill any shallow borrows that would have
//! conflicted if they were normal borrows. This made it possible to circumvent fake borrows for
//! match guards and indexing expressions.

fn test_match_guard() {
let mut a = (Some(&42u64), 0u8);
let mut b = (None::<&u64>, 0u8);
let mut p = &mut a;
// Writing to `(*p).1` in the match guard previously killed the fake borrow of `p` in the guard,
// making it possible to mutate `p` despite `(*p).0` being matched on. This would reach the
// `Some(r)` branch with `(*p).0` being `None`, so the `r` binding was invalid.
match p.0 {
Some(_) if { p.1 = 1; p = &mut b; false } => unreachable!(),
//~^ ERROR: cannot assign `p` in match guard
Some(r) => println!("{r}"),
None => unreachable!(),
}
}

fn test_indexing() {
let mut x: &mut [&mut [u32]] = &mut [&mut [0]];
let y: &mut [&mut [u32]] = &mut [];
// Writing to `x[0][0]` previously killed the fake borrow of `x` in the index expression, making
// it possible to access `y[0]` without a bounds-check.
x[0][{ x[0][0] = 1; x = y; 0 }];
//~^ ERROR: cannot assign `x` in indexing expression
}

fn main() {}
19 changes: 19 additions & 0 deletions tests/ui/borrowck/dont-kill-shallow-borrows-on-child-writes.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
error[E0510]: cannot assign `p` in match guard
--> $DIR/dont-kill-shallow-borrows-on-child-writes.rs:16:31
|
LL | match p.0 {
| --- value is immutable in match guard
LL | Some(_) if { p.1 = 1; p = &mut b; false } => unreachable!(),
| ^^^^^^^^^^ cannot assign

error[E0510]: cannot assign `x` in indexing expression
--> $DIR/dont-kill-shallow-borrows-on-child-writes.rs:28:25
|
LL | x[0][{ x[0][0] = 1; x = y; 0 }];
| ---- ^^^^^ cannot assign
| |
| value is immutable in indexing expression

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0510`.
Loading