Also run UnusedBrokenConst on associated consts. #70017
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -475,15 +475,25 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, 'tcx, M | |
| Ok(true) | ||
| } | ||
| ty::RawPtr(..) => { | ||
| let imm = self.ecx.read_immediate(value)?; | ||
| let pointee_kind = &imm.layout.ty.builtin_deref(true).unwrap().ty.kind; | ||
| match pointee_kind { | ||
| ty::Param(_) => { | ||
| // Creating pointers to T is valid. We only reach this case for unused | ||
| // associated consts. | ||
|
Comment on lines
+482
to
+483
There was a problem hiding this comment. This change only affects raw pointers. And it anyway was only checking that the pointer itself is not However, validity certainly assumes that the type is fully known. It makes little sense to call validity at all when there are still There was a problem hiding this comment. I still have rust/src/test/ui/consts/const-eval/ice-generic-assoc-const.rs Lines 9 to 10 in 8045865 I didn't realise that this was just checking for I see a lot of const errors are picked up during There was a problem hiding this comment.
Raw pointers may dangle. So such a check would be incorrect.
Such constant should not have validation run on them, anyway, We cannot validate them, after all, with some type information missing. I get the feeling you are sticking values into compiler passes that are not prepared to handle those values. I don't think this approach can work. In other words, I think this (running const-eval on associated consts) is the wrong approach to fix #69021. There was a problem hiding this comment.
After the poking I've done here, I've come to realise the issue is a bit broader than just missing bitshifts. We only get lints for associated consts if trait Foo {
const N: i32;
}
struct S;
impl Foo for S {
const N: i32 = [10][1];
}
fn main() {
//println!("{}", S::N);
}As for running const-eval being the wrong approach, I'd argue that associated consts are suitable for const eval in much of the same way normal consts are: by definition, they, like normal consts, must be able to be evaluated at compile time, and they also can be complex enough that just linting the AST without doing at least some form of const evaluation isn't enough.
Mmm, I agree with your assessment. However, for the reason above, I'm inclined towards making const-eval work (but perhaps shoehorning There was a problem hiding this comment.
This only makes sense for monomorphized associated consts. It makes no sense when there are still unknown generics around, like what you are seeing here. Associated consts with missing generics are much more like const functions than consts. And for const functions, we don't run const-eval either. And indeed the entire concept of executing such a constant/function is meaningless as some inputs are missing! const fn do_something(b: bool, x: i32) -> i32 {
if b { x } else { x << 44 }
}What does it mean do "run" I really wonder why you are so focused on const_eval. Most of these lints don't come from const_eval. They come from const_prop. That's how the lint is triggered in run-time (non-const) functions, such as this. Here is the code triggering the lint: rust/src/librustc_mir/transform/const_prop.rs Lines 536 to 554 in 8ab82b8 const_prop is an optimization pass that expects to run on polymorphic functions with partial information, unlike const_eval which expects to run on fully monomorphic code. Thus I think the right fix here is to run const_prop on associated consts. That is also why in this comment I talked about the relevant parts of const_prop. I wonder if there was any specific reason you went for const_eval? There was a problem hiding this comment.
I actually love that impl<T> Nullable for *const T {
const NULL: Self = core::ptr::null::<T>(); is evaluable polymorphically. Not sure if this is relevant for @davidtwco. If a constant (associated or not) can be evaluated polymorphically, then we don't need to run it again. We may experiment with this in back to topic: I think we should do something where associated constants and regular constants get handled the same way by So... I'm not sure what I think is the best way forward, but in general doing There was a problem hiding this comment. I feel associated consts are more like Of course, the actual "propagation" part of const_prop makes little sense here. Maybe putting lint and transformation together was just a bad call? There was a problem hiding this comment.
I looked into how unused non-assoc constants were linted and found UnusedBrokenConst, which runs rust/src/librustc/mir/interpret/queries.rs Lines 10 to 13 in 285519d On a second reading, and in light of everything you've both informed me here, I've realised I've focussed on the wrong part of that doc comment.
Ah, and that's why
I'm inclined to agree, and moreso when the transformation isn't always run. I'll pull the linting code into its own function as a very first step. |
||
| } | ||
| _ => { | ||
| // We are conservative with undef for integers, but try to | ||
| // actually enforce our current rules for raw pointers. | ||
| let place = try_validation!( | ||
| self.ecx.ref_to_mplace(imm), | ||
| "undefined pointer", | ||
| self.path | ||
| ); | ||
| if place.layout.is_unsized() { | ||
| self.check_wide_ptr_meta(place.meta, place.layout)?; | ||
| } | ||
| } | ||
| } | ||
| Ok(true) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| // Tests that associated constants are checked whether they are used or not. | ||
| // | ||
| // revisions: used unused | ||
| // compile-flags: -Copt-level=2 --emit link | ||
|
|
||
| #![cfg_attr(unused, allow(dead_code))] | ||
| #![deny(arithmetic_overflow)] | ||
|
|
||
| pub trait Foo { | ||
| const N: i32; | ||
| } | ||
|
|
||
| struct S; | ||
|
|
||
| impl Foo for S { | ||
| const N: i32 = 1 << 42; | ||
| //~^ ERROR this arithmetic operation will overflow | ||
| //~| ERROR any use of this value will cause an error | ||
| } | ||
|
|
||
| impl<T: Foo> Foo for Vec<T> { | ||
| const N: i32 = --T::N + (-i32::MIN); | ||
| //~^ ERROR this arithmetic operation will overflow | ||
| } | ||
|
|
||
| fn main() { | ||
| #[cfg(used)] | ||
| let _ = S::N; // FIXME: "erroneous constant used" -- awaiting #67176 | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| error: this arithmetic operation will overflow | ||
| --> $DIR/lints-used-unused.rs:16:20 | ||
| | | ||
| LL | const N: i32 = 1 << 42; | ||
| | ^^^^^^^ attempt to shift left with overflow | ||
| | | ||
| note: the lint level is defined here | ||
| --> $DIR/lints-used-unused.rs:7:9 | ||
| | | ||
| LL | #![deny(arithmetic_overflow)] | ||
| | ^^^^^^^^^^^^^^^^^^^ | ||
|
|
||
| error: any use of this value will cause an error | ||
| --> $DIR/lints-used-unused.rs:16:20 | ||
| | | ||
| LL | const N: i32 = 1 << 42; | ||
| | ---------------^^^^^^^- | ||
| | | | ||
| | attempt to shift left with overflow | ||
| | | ||
| = note: `#[deny(const_err)]` on by default | ||
|
|
||
| error: this arithmetic operation will overflow | ||
| --> $DIR/lints-used-unused.rs:22:29 | ||
| | | ||
| LL | const N: i32 = --T::N + (-i32::MIN); | ||
| | ^^^^^^^^^^^ attempt to negate with overflow | ||
|
|
||
| error: aborting due to 3 previous errors | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| error: this arithmetic operation will overflow | ||
| --> $DIR/lints-used-unused.rs:16:20 | ||
| | | ||
| LL | const N: i32 = 1 << 42; | ||
| | ^^^^^^^ attempt to shift left with overflow | ||
| | | ||
| note: the lint level is defined here | ||
| --> $DIR/lints-used-unused.rs:7:9 | ||
| | | ||
| LL | #![deny(arithmetic_overflow)] | ||
| | ^^^^^^^^^^^^^^^^^^^ | ||
|
|
||
| error: any use of this value will cause an error | ||
| --> $DIR/lints-used-unused.rs:16:20 | ||
| | | ||
| LL | const N: i32 = 1 << 42; | ||
| | ---------------^^^^^^^- | ||
| | | | ||
| | attempt to shift left with overflow | ||
| | | ||
| = note: `#[deny(const_err)]` on by default | ||
|
Comment on lines
+1
to
+21
There was a problem hiding this comment. Does it make sense to try to deduplicate these errors? From a front-facing perspective, they're two different lints ( |
||
|
|
||
| error: this arithmetic operation will overflow | ||
| --> $DIR/lints-used-unused.rs:22:29 | ||
| | | ||
| LL | const N: i32 = --T::N + (-i32::MIN); | ||
| | ^^^^^^^^^^^ attempt to negate with overflow | ||
|
|
||
| error: aborting due to 3 previous errors | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can also have associated constants with default values in trait definitions. Also consider const generics here, e.g.
foo::<{BAD_CONSTANT}>();.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in order to do this you can eval all anonconsts, which will also cover a few other cases (though I think they are already evaluated elsewhere)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do this in a separate PR though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing