const_eval: Consider array length constant even if array is not

[TheWastl]

Fixes #98444.

[rustbot]

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

[rust-highfive]

r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

[leonardo-m]

So is this reasonable code now allowed?

fn main() {
    let a = [1_u32, 2, 3];
    let b = [0; a.len()];
}

[TheWastl]

So is this reasonable code now allowed?

fn main() {
    let a = [1_u32, 2, 3];
    let b = [0; a.len()];
}

No, this PR should only affect constant propagation for the purposes of optimization and lints. (Ok, maybe the title is a bit misleading.)

In particular, your code already fails a step earlier: accessing a is forbidden, regardless of what you do with it. For instance, this fails, too:

fn main() {
    let a = [1_u32, 2, 3];
    let b = [0; { a; 3 }];
}

[RalfJung]

This doesn't look right. It is certainly not right without extensive comments explaining what is happening and why the old code is wrong.

[oli-obk]

It's a fast path that does the right thing (codegen does this opt after all or mir opts do it for codegen).

I wonder if this has any effect on miri at all (the force alloc will just happen later?). If there is no benefit to miri, we should just teach const prop about the Len Rvalue

[RalfJung]

Miri shouldn't usually have fast paths though. We want to catch all the UB, not take any shortcuts. ;)

However, OpTy also has a len method these days, so I think a patch could be made that avoids force_allocation. That sounds like a good idea. I don't know if it helps ConstProp though.

[TheWastl]

Miri shouldn't usually have fast paths though. We want to catch all the UB, not take any shortcuts. ;)

Ok, that makes sense.

However, OpTy also has a len method these days, so I think a patch could be made that avoids force_allocation. That sounds like a good idea. I don't know if it helps ConstProp though.

I did some testing, and using OpTy doesn't seem to help ConstProp.

If there is no benefit to miri, we should just teach const prop about the Len Rvalue.

There's two ConstProps: const_prop and const_prop_lint (since #94934). I guess this should be added to both?

And should I force-push this PR, or open a new one? Changing this one might make the existing discussion a bit confusing for future readers.

(sorry for the delay)

[RalfJung]

The issue you want to fix is about the lint, so const_prop_lint is the most relevant one.

I did some testing, and using OpTy doesn't seem to help ConstProp.

Ah, place_to_op will still want to read the local and that is where ConstProp bails.

[RalfJung]

this PR should only affect constant propagation for the purposes of optimization and lints.

Sorry, but I don't think we want to have changes in the interpret folder that only benefit const-prop. The code in that folder is rather important to get right for CTFE and Miri, so if ConstProp needs something weird, it should hack around that on its own terms. ConstProp is already grossly misusing the interpreter, and I am going to object any further technical debt being imposed on the interpreter in the name of ConstProp, except if it is absolutely needed to fix ICEs.

@oli-obk has plans for making ConstProp independent of the interpreter. That is the right way to fix these problems.

[shamatar]

One may try to change the .len() for arrays to be not just coercion to slice and most likely it will be sufficient

N.B. It may be not the only place where some information (type level constant in array's case) is lost

[RalfJung]

You should be able to do src.place_to_op(src)?, which will give you a OpTy, and then call len on that. That would avoid force_allocation.

[jackh726]

@RalfJung I'm going to assign you as reviewer here. Feel free to reassign if you can't review

r? @RalfJung

[TheWastl]

Closing in favour of #100160 (wow 6 digits).