Skip to content

const_eval: Consider array length constant even if array is not #99594

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

const_eval: Consider array length constant even if array is not #99594

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions compiler/rustc_const_eval/src/interpret/step.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

use rustc_middle::mir;
use rustc_middle::mir::interpret::{InterpResult, Scalar};
use rustc_middle::ty;
use rustc_middle::ty::layout::LayoutOf;

use super::{InterpCx, Machine};
Expand Down Expand Up @@ -251,9 +252,14 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {

Len(place) => {
let src = self.eval_place(place)?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let mplace = self.force_allocation(&src)?;
let len = mplace.len(self)?;
self.write_scalar(Scalar::from_machine_usize(len, self), &dest)?;
if let &ty::Array(_, len) = src.layout.ty.kind() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@RalfJung RalfJung Jul 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let len = self.const_to_op(len, Some(dest.layout))?;
self.copy_op(&len, &dest, /*allow_transmute*/ false)?;
} else {
let mplace = self.force_allocation(&src)?;
let len = mplace.len(self)?;
self.write_scalar(Scalar::from_machine_usize(len, self), &dest)?;
}
}

AddressOf(_, place) | Ref(_, _, place) => {
Expand Down
8 changes: 8 additions & 0 deletions src/test/ui/const_prop/issue-98444-const-index-out-of-bounds.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// build-fail
// Need to use build-fail because check doesn't run constant propagation.

fn main() {
let xs: [i32; 5] = [1, 2, 3, 4, 5];
let _ = &xs;
let _ = xs[7]; //~ ERROR this operation will panic at runtime
}
10 changes: 10 additions & 0 deletions src/test/ui/const_prop/issue-98444-const-index-out-of-bounds.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
error: this operation will panic at runtime
--> $DIR/issue-98444-const-index-out-of-bounds.rs:7:13
|
LL | let _ = xs[7];
| ^^^^^ index out of bounds: the length is 5 but the index is 7
|
= note: `#[deny(unconditional_panic)]` on by default

error: aborting due to previous error

1 change: 1 addition & 0 deletions src/test/ui/consts/issue-65348.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// check-pass
#![allow(unconditional_panic)]

struct Generic<T>(T);

Expand Down