[dianne]

Background

Consider the place expression x[i][j], given x: &[&[T]]. When lowering this to MIR, we perform each bounds check once, testing first that i is within x's bounds, then that j is within x[i]'s bounds.

Consider, then, x[i][{ x = y; j }]. Since y's length could be less than i, the assignment invalidates the first bounds check; we can't safely index into y[i]. To avoid repeating the check, we require that x can't be reassigned while indexing. For each indexed-into slice for which we need its bounds check to hold through later index expressions, we fake-borrow any references/pointers the slice is nested under, store those fake borrows in temporaries, and put fake reads of those temporaries after the final bounds check¹. This keeps the fake borrow temporaries live, ensuring that we can use the final place²; replacing an indexed-into slice in a subsequent index expression would conflict with a fake borrow.

So what's wrong?

If we try to build a MIR place with at least three instances of slice indexing, such as in x[0][{ x = y; 0 }][{ return; 0 }], we can invalidate the first bounds check, then diverge before the fake reads at the end. This diverges before the place can actually be used, but when we do the second bounds check there, we test that 0 is withinx[0]'s bounds.. after we've replaced x with y, which could potentially be empty. We need the check that 0 is within x's bounds to hold during that second bounds check, but the divergent control flow means the temporary holding the fake borrow on x isn't live there, so the borrow goes out of scope. This is #161852.

We need to keep those fake borrow temporaries live

To fix #161852, we need fake borrows on indexed-into slice pointers to live through subsequent bounds checks. This PR does that (by emitting fake reads for them after all subsequent bounds checks).

Consequences

The following is now rejected:

fn main() {
    let mut x: &[&[&[u32]]] = &[&[&[0]]];
    let y: &[&[&[u32]]] = &[];
    x[0][{ x = y; 0 }][{ return; 0 }];
    //~^ ERROR: cannot assign `x` in indexing expression
}

Non-consequences

This does not affect locations from which bounds-checks are unreachable. The following was already and is still accepted:

fn main() {
    let mut x: &[&[&[u32]]] = &[&[&[0]]];
    let y: &[&[&[u32]]] = &[];
    x[0][{ x = y; return; 0 }];
}

Footnotes

1. slice and array indexing are the only expressions that both contain arbitrary subexpressions and lower directly to MIR place projections (after evaluating their indices, storing them in temporaries, doing bounds checks, etc.). if anything between the final bounds check and uses of the place in MIR can replace any pointers to slices, special care is necessary! this is possible with match guards, so in addition to the fake borrows we normally create to keep tests results from changing, we fake-borrow references/pointers that pattern bindings are accessed through. ↩

2. this also relies on not being able to change any indices. we create fresh temporaries for slice and array indices so they can't be messed with. ↩

[dianne]

I think this makes the total number of fake reads quadratic in how much nested slice indexing we do, but hopefully that'd only matter in stress tests? Surprisingly, no MIR building tests needed blessing, so I don't have an example to point to.

View changes since the review

[matthewjasper]

This needs 3 or more nested array/slice indexes, so it's not that surprising that no tests are affected. It definitely doesn't seem likely that there's any real code that's going to have large enough numbers of nested indexes for this to have a significant impact.

[dianne]

r? @matthewjasper

Like #161581, I'm not fully sure where this falls between lang and types in terms of design responsibility, but I'll start by lang-nominating it alongside that. The ask is equivalent: from a user's perspective, what's the language property we're expressing with fake borrows in indexing expressions? Does this approach fit that, or should we be stricter?

[matthewjasper]

The code here looks good. I'll wait for lang to discuss.

The original motivation for the fake borrows here was to make a minimal change to (try to) fix the soundness issue. I considered mimicking the borrows done by the Index trait, but I think that was too restrictive and broke too much code.

View changes since this review

[dianne]

Following up on yesterday's lang team meeting, I've opened #162242 as a more restrictive alternative to this. Lang was interested to see a version that consistently prohibits mutating fake-borrowed places without regard to CFG shape.

[traviscross]

Following the same reasoning as in #163948 (comment), modulo that here we need to ensure that bounds checks we've done previously remain valid for later checks, I propose that we do this too, adopting the precise approach as in #163948. This is a (breaking) soundness fix; unlike the other PR, this one does not accept any new code.

@rfcbot fcp merge lang

(Cc @rust-lang/types, @rust-lang/opsem. As in the other proposed FCP, if you want in, let me know, and I'll happily restart.)

[lcnr]

I'd consider fake borrows to be an impl detail of borrowck, so it feels better to me for this to be owned by types, given that we're responsible for borrow checking more generally, e.g. with polonius. Please involve Types 😁

I guess my question would be more whether lang wants to be involved here :p

[traviscross]

@rfcbot fcp merge lang,types

[rust-rfcbot]

@traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

• @BoxyUwU
• @jackh726
• @joshtriplett
• @lcnr
• @nikomatsakis
• @oli-obk
• @scottmcm
• @spastorino
• @tmandry
• @traviscross

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[traviscross]

I'd consider fake borrows to be an impl detail of borrowck, so it feels better to me for this to be owned by types, given that we're responsible for borrow checking more generally, e.g. with polonius. Please involve Types 😁

Thanks. Happily. Restarted.

I guess my question would be more whether lang wants to be involved here :p

For my part, I consider this and #163948 to be lowering matters that involve questions of the mental model that we're exposing to users and what language properties we mean to express, as in #163948 (comment), so I'd expect lang involvement.

[tmandry]

Of course we shouldn't accept this code. The model here looks unchanged, essentially it's a bug fix of the existing one. You shouldn't be able to mutate a container you're in the middle of indexing.

Paraphrasing what I heard from @dianne to preserve my own understanding of the bug: The fake borrow was not being kept alive through paths that diverged in the middle of indexing.

Thank you for finding and fixing this, @dianne!

@rfcbot reviewed

[scottmcm]

Thanks for catching this! Definitely agree we need to guard this.

One thing from the conversation I thought important: for paths that go through Index this doesn't come up since the &self means there's a "real" borrow involved. So this being for the special built-in indexing to catch a missed requirement makes sense to me.

@rfcbot reviewed

---

One PR-level (non-lang) request, though: could you add a MIR test that shows the fake borrows in a success case, in addition to the failing UI test? I think that would be a good place to have the "we intentionally need these fake borrows" note to help avoid the possibility of accidentally removing them at some other point.

[jackh726]

I'm not going to register a concern (for now) - but with my T-types hat on, please add more details about the problem and the approach to the OP @dianne.

[dianne]

@jackh726 done. adding all the context about MIR lowering made it a bit verbose, but hopefully it's self-contained enough now?

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[dianne]

One PR-level (non-lang) request, though: could you add a MIR test that shows the fake borrows in a success case, in addition to the failing UI test? I think that would be a good place to have the "we intentionally need these fake borrows" note to help avoid the possibility of accidentally removing them at some other point.

@scottmcm also done! unfortunately no FileCheck (unless there's some way to do that for pre-CleanupPostBorrowck MIR?), so hopefully an explanatory top-level comment suffices. I tried to be exact enough that it can explain all the examples, particularly those that involve a mix of slice and array indexing.

You shouldn't be able to mutate a container you're in the middle of indexing.

@tmandry it's a bit more specific than that: once you index into a slice, you can't change anything within subsequent index expressions that could affect the result of its bounds check. you're still free to e.g. mutate arrays you're indexing into, since the type system makes sure you can't invalidate their bounds checks by swapping out arrays (though once you index into a slice nested within that, all pointers the slice is under become immutable after the slice's bounds check, rendering the arrays immutable after that point as well). similarly, you can swap out a pointer to a slice you're about to index into, since you haven't done a bounds check for it yet.

if you can't reach another bounds check, this immutability also doesn't apply, though maybe it's arguable that you're not indexing anymore at that point?

The fake borrow was not being kept alive through paths that diverged in the middle of indexing.

yep! as a general description, I agree with this. I didn't want to get too deep into edge cases during the meeting. ^^