Consider removing ProjectionElem::Index

[jonas-schievink]

It is the only projection element that uses another local besides the place's base local, and requires special care in many circumstances. All other ProjectionElems perform static operations that do not depend on runtime values.

In #71003, I initially did not handle this special case, which would have resulted in an unsound optimization. The visitor logic for them was also broken (fixed in #71013) because it was missing a special case.

I'm not sure what to adequately replace them with. It seems like Rvalue::Index would be a reasonable choice, but it might have other drawbacks.

cc @rust-lang/wg-mir-opt

[eddyb]

This has come up before, at least as early as last year's allhands.
So far we've flattened Place, that was the other big item, I think.
cc @nikomatsakis

[spastorino]

👍, I have this in my todo list. May end tackling it very soon if nobody beats me to do it 😄.

[nikomatsakis]

One challenge here is that the borrow checker is kind of smart. For example, this code compiles (playground):

struct Foo {
    x: u32,
    y: u32
}

fn main() {
    let foos: &mut [Foo] = &mut [Foo { x: 0, y: 0 }, Foo { x: 0, y: 0 }];
    let p = &mut foos[0].x;
    let q = &mut foos[0].y;
    *q += 1;
    *p += 1;
}

The challenge here is that &mut foos[0].x cannot be compiled as two separate MIR statements like

TMP0 = &mut foos[0];
TMP1 = &mut TMP0.x

because then it would conflict with the later access to foos[0].y.

This leads us back to that design I was pushing for earlier of having a notion of "place values" that we build up bit-by-bit and "finalize" to actually do a borrow.

cc @matthewjasper

[oli-obk]

Considering that

#![feature(raw_ref_op)]
struct Foo {
    x: u32,
    y: u32
}

fn main() {
    unsafe {
        let foos: &mut [Foo] = &mut [Foo { x: 0, y: 0 }, Foo { x: 0, y: 0 }];
        let p = &raw mut foos[0];
        let p = &raw mut (*p).x;
        let q = &raw mut foos[0].y;
        *q += 1;
        *p += 1;
    }
}

is legal in stacked borrows (at least miri happily evaluates this cc @RalfJung ), would it make sense to teach borrowck about

struct Foo {
    x: u32,
    y: u32
}

fn main() {
    let foos: &mut [Foo] = &mut [Foo { x: 0, y: 0 }, Foo { x: 0, y: 0 }];
    let p = &mut foos[0];
    let r = &mut p.x;
    let q = &mut foos[0].y;
    *q += 1;
    *r += 1;
}

which could be fine since p is never used again after q is created. If we do this, we could remove the ability to have projection lists in MIR and just have a single projection per Place (or even no projections anymore in Place, and require an intermediate local instead)

[RalfJung]

Yeah that first example seems fine in terms of Stacked Borrows -- since everything is raw pointers, there are no aliasing requirements.

I think Stacked Borrows would also accept the second example if it was accepted by the borrow checker; it is basically equivalent to

struct Foo {
    x: u32,
    y: u32
}

fn main() {
    let foos: &mut [Foo] = &mut [Foo { x: 0, y: 0 }, Foo { x: 0, y: 0 }];
    let r = &mut foos[0].x;
    let q = &mut foos[0].y;
    *q += 1;
    *r += 1;
}

The fact that p gets invalidated on [0].y is no problem for still using r (derived from p) on other locations.

[nikomatsakis]

I'm not necessarily opposed to extending the borrow checker here, but I don't immediately see how to do it. We're also still in the midst of transitioning to polonius -- I guess I would want to make this change in that context.

Currently what happens is that we get a relation 'p: 'r -- i.e., the origin for p is a subset of the origin for r, and so whenever the value r is live, the loan for p is also live (and hence modifications to foos[0] are not permitted).

Right now, the borrow checker doesn't really know that p (in your example) is created by borrowing foos[0] -- well, in polonius, it could know that by looking at the origin 'p, which would be a singleton set, but in general it might not be a singleton set.

So I guess we'd be looking at some sort of rule that tries to specialize this case -- reborrowing a singleton set -- and handle it in some other way.

Anyway, TL;DR, seems plausible but complicated.

To recall, my current plan was to have "place temporaries" for use when building up paths like foos[0].x. These would be linear by construction and we would have some "bless" that converts them into borrows. So the borrow checker would know that it can track the precise path they are borrowed from, in other words. I guess this is fairly "related" to what we would be teaching polonius to look for in the general case.

[oli-obk]

To recall, my current plan was to have "place temporaries" for use when building up paths like foos[0].x. These would be linear by construction and we would have some "bless" that converts them into borrows. So the borrow checker would know that it can track the precise path they are borrowed from, in other words. I guess this is fairly "related" to what we would be teaching polonius to look for in the general case.

Yes that's what @spastorino and I talked about, when the scheme explained in my previous comment came up. We'll look into implementing the place temporaries scheme. I have some ideas how to do this in a way so we can switch between the current scheme and the new scheme on the fly via a -Z parameter. This will make it easier to develop and merge in steps instead of having one huge PR that will change everything in one go.

[nikomatsakis]

I think an MCP would be appropriate to describe the plan.