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

[dianne]

It's not a conflict to write to a sub-place of a shallowly borrowed place, so it's unsound to kill shallow borrows when we encounter assignments to their sub-places.

Fixes #160599 on the default borrow checker and under -Zpolonius=next. I haven't touched -Zpolonius=legacy since I'm not sure how much we're maintaining it, but at a glance the fix looks like it would be the same.

[dianne]

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]

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.

[dianne]

I thought it'd be easier to ask for forgiveness than permission to fix this myself, but after digging into the parts of Borrows::kill_borrows_on_place I didn't change, I've started to have doubts about that. I'd already written the PR, though. Forgive me!

r? types

[oli-obk]

r? @lqd but happy to take it again if you think it's general enough to not need your borrowck and polonius expertise

[lqd]

@oli-obk I've looked it over, but I'm also not an expert on shallowness, so while this change does make sense to me I would love an additional pair of eyes like yours (or maybe @matthewjasper's) to spot check it as well.

[oli-obk]

@bors try

let's also crater it

[rust-bors]

☀️ Try build successful (CI)
Build commit: c0dfa9a (c0dfa9ae292642b7683cf7162ffb6880170df098)
Base parent: 444dcce (444dcce484c66c8d626806a8556be3b4d3ef381a)

[lqd]

@craterbot check

[craterbot]

👌 Experiment pr-160902 created and queued.
🤖 Automatically detected try build c0dfa9a
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[theemathas]

@craterbot cancel

See #162233

[craterbot]

🗑️ Experiment pr-160902 deleted!

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more