[dianne]

View all comments

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

[lcnr]

there is just one other use of this function in

rust/compiler/rustc_borrowck/src/polonius/legacy/loan_kills.rs

Line 120 in 232b0ff

let places_conflict = places_conflict::places_conflict(

I feel like we should remove places_conflict and always use an explicit access as getting this wrong is apparently unsound 🤔

View changes since the review

[oli-obk]

Once legacy polonius is gone, we can get rid of the places_conflict function entirely and avoid this footgun in the future.

this will disappear on its own, but we can also just get rid of it now and inline it manually

[lqd]

Once legacy polonius is gone

(we can also make that happen sooner rather than later, I was already looking into the rustc_borrowck::consumers users for other reasons/breakage)

[lcnr]

The match handling code is somewhat confusing to me. @dianne related to #161578. For that I am somewhat confused that we read the fake borrow at the end of the guard instead of before the next match arm 🤔

First, can we slap a big and scary comment somewhere that the "does this kill X" needs to match (or be weaker than) "does this overlap" 🤔

I feel out of my depth here. However, given that @oli-obk, @lqd and @matthewjasper have looked over it and I think adding fewer kills is always less permissive, I am going to check my box.

I really want to spend a few months diving into the actual borrowck part of borrowck so that I understand how it works!

@rfcbot reviewed

[dianne]

@lcnr with regard to where fake reads are inserted for guards, we need them both on guard success before making by-value bindings¹ and on guard failure before doing tests for further match arms². those could respectively be framed simply as "before making by-value bindings" and "before the next arms" rather than as "after the guard". needing them on both paths is only really relevant when guards can diverge after mutating matched-on places³, which I expect is why we currently only have fake reads before making by-value bindings.

having worked on both this and that, the main commonality I noticed is that fake borrows are subtle. ^^ for #161578 and #161852, how we place fake reads happens to independently be wrong for both matches and indexing: unreachable fake reads means we don't infer big enough regions for the shallow borrows, so they won't conflict with problematic writes. for this, how we compute the set of borrows in scope at each point is wrong for shallow borrows, which affects both matches and indexing. in either case, subverting a fake borrow means we can write unsound code.

regarding the big scary comment, I'd also like that, but it might take me some time to feel confident adding it to this PR myself; some parts are at the edge of my understanding. I can justify the change made by this PR⁴, but there's more going on surrounding it that I'm still learning about too, which I'd want to inform a comprehensive explanation.

Footnotes

1. e.g. if the arm's pattern is Some(x), we need the guard not to have changed the scrutinee to None, since creating the binding x would be unsound ↩

2. e.g. if the arm's pattern is Some(_), we need the guard not to have changed the scrutinee to None, since match lowering assumes that the scrutinee is still Some. tests and bindings for later arms' patterns may treat the scrutinee as a Some and access its field, which is unsound if it's been changed to None. ending up in the wrong arm isn't great in general ↩

3. if the guard diverges, it's possible for only one of "making by-value bindings" and "the other match arms" to be reachable without leaving the match entirely (thereby resetting our assumptions about what tests have been performed). if we wanted to have the fake reads all in one place, we'd need to store whether the guard succeeded, do fake reads, then branch ↩

4. if a place is only shallowly borrowed, assigning to its sub-places doesn't conflict with the borrow (because it's shallow), so we don't report a conflict error if that happens. by killing the borrow at that point, we're removing it from the set of borrows in scope, meaning it's not around to conflict with accesses that it's meant to conflict with, such as direct assignments to the shallowly borrowed place (when we check for access conflicts, we only consider borrows in scope at that point). killing a borrow after an assignment that conflicts with it is fine: if the borrow is in scope for that assignment, we'll report an error when we check that assignment for conflicts ↩

[rust-rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rust-rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

[oli-obk]

@bors r+ rollup

View changes since this review

[rust-bors]

📌 Commit 4952963 has been approved by oli-obk

It is now in the queue for this repository.