Also run UnusedBrokenConst on associated consts. #70017
Conversation
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
jumbatm
force-pushed
the
exceeding-bitshifts
branch
from
d7a845c
to
4af3649
Compare
| check_const(cx, body_id); | ||
| } | ||
| _ => {} | ||
| } | ||
| } | ||
| fn check_impl_item(&mut self, cx: &LateContext<'_, '_>, it: &hir::ImplItem<'_>) { |
There was a problem hiding this comment.
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.
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.
Let's do this in a separate PR though
| debug!("const_eval_poly: running on {:?}: {:?}", def_id, ty); | ||
|
|
||
| // FIXME: This check could be smarter / faster. | ||
| let is_generic = |t: Ty<'_>| -> bool { |
There was a problem hiding this comment.
this isn't the only way that it can be too generic, the final decision on whether it is too generic is made during const eval. why was it necessary to add this check? just for speed?
There was a problem hiding this comment.
It's to address this test case. In this case, the call to layout_of fails
rust/src/librustc_mir/interpret/place.rs
Line 310 in 5da2e53
because T isn't known yet -- we're not running on a specific instance of T. The error is bubbled up, transformed into a validation failure, and is caught in the catch-all here:
rust/src/librustc_mir/const_eval/eval_queries.rs
Lines 213 to 218 in e0f5df0
As I understand it, the test case failure boils down to not having a concrete type to substitute into T -- this is something we currently don't run into, because check_binary_op currently runs when const_prop reaches a use of the value, so there's always some actual type to substitute in to get a layout for.
Other TooGeneric failures are currently caught in that catch-all error emit above. My initial thought when I ran into this was to modify the functions along the callstack so instead of catching the error, turning it into a validation failure and reporting the error, they can throw TooGeneric all the way up and let the caller decide what to do with them (so, in the case of check_const, it would just ignore the TooGeneric and move on). This seemed a bit too specific, though, and checking here seemed sufficient -- and yes, maybe even a little bit faster.
I've just noticed the struct_error here is actually const eval's struct_error, which looks like could already bubble TooGeneric up. Hmm -- I'll have a look at how it goes if the layout_of caller does a check to throw TooGeneric instead of undefined pointer.
There was a problem hiding this comment.
yea, I think we should fix this in const eval. although the last time I tried, I did it wrongly. maybe it suffices to use a UserFacing ParamEnv for associated constants?
There was a problem hiding this comment.
Just having this change didn't end up solving the issue -- as it turns out, it wasn't const eval that was failing in the first place, but validation afterwards. I resolved this by changing validation to not fail on pointers to Params (which should hopefully only be reached when checking unused constants).
If this was the case, the behavior of |
| let substs = InternalSubsts::identity_for_item(self, def_id); | ||
| let instance = ty::Instance::new(def_id, substs); | ||
| let cid = GlobalId { instance, promoted: None }; | ||
| let param_env = self.param_env(def_id).with_reveal_all(); |
There was a problem hiding this comment.
right here ,if you skip doing the reveal all if the instance .needs_subst() you should hopefully not get an ICE anymore, because the relevant code bails out with TooGeneric
There was a problem hiding this comment.
Apologies if I wasn't clear -- I haven't been getting an ICE. In my previous comment, by an error bubbling up, I meant an Err being returned upwards (which ends in us gracefully emitting a diagnostic to the user). Hmm -- I'm wonder if doing this anyway will bail out with TooGeneric where I need it to.
There was a problem hiding this comment.
yes, without reveal all, such errors will not be emitted but become TooGeneric
There was a problem hiding this comment.
This hasn't been addressed.
There was a problem hiding this comment.
Most feedback hasn't yet. I'm still working on this pr. I think it's showing as outdated because I squashed & dropped some commits. I will ping you all and re-mark as waiting on review when this is ready to go again 👍
RalfJung
added
S-waiting-on-author
jumbatm
force-pushed
the
exceeding-bitshifts
branch
from
4af3649
to
b34843f
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@jumbatm there seem to be test failures? |
|
Ping from triage |
|
Apologies - I'm still working on this PR, and yet to address most of the review comments. The test failures are because I updated the test cases to include some cases that were missed before (and that my original set of changes didn't address). |
Previously, we would only end up doing checks for consts which const_prop was attempted on. This meant associated consts which didn't end up explicitly used in a crate weren't visited, so weren't checked.
It ain't much, but it's honest work.
jumbatm
force-pushed
the
exceeding-bitshifts
branch
from
b34843f
to
6d33a18
Compare
| // Creating pointers to T is valid. We only reach this case for unused | ||
| // associated consts. |
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 Undef -- whatever the pointer points to is already irrelevant here. I don't see how this change makes any sense.
However, validity certainly assumes that the type is fully known. It makes little sense to call validity at all when there are still ty::Param around.
There was a problem hiding this comment.
I still have ty::Param around in cases for unused associated consts, unfortunately, because I don't have a use-site to pull a type from. When ref_to_mplace calls layout_of, because the type is unknown, the layout is unknown, and validation fails. This breaks code which like this:
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 Undef of the pointer value itself. I got the impression the purpose of this check was to check that the pointee had a valid layout (ie, that it's okay to dereference the raw pointer), so I thought skipping this check made sense because it's not until the pointer is dereferenced that any issues could occur.
I see a lot of const errors are picked up during const_eval_raw, before validation even happens - does it make sense to just not run validation for unused associated consts, then?
There was a problem hiding this comment.
ie, that it's okay to dereference the raw pointer
Raw pointers may dangle. So such a check would be incorrect.
I still have ty::Param around in cases for unused associated consts, unfortunately, because I don't have a use-site to pull a type from.
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.
I think this (running const-eval on associated consts) is the wrong approach to fix #69021
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 const_eval reaches the const through some use of it. That is, all lints for associated consts are missed if the associated const is unused. For example, the out-of-range access here isn't picked up if S::N is left unused:
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.
I get the feeling you are sticking values into compiler passes that are not prepared to handle those values.
Mmm, I agree with your assessment. However, for the reason above, I'm inclined towards making const-eval work (but perhaps shoehorning const_eval_poly into this is the wrong approach).
There was a problem hiding this comment.
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.
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" do_something? It means nothing. You can only run do_something after picking some b and x.
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.
as some inputs are missing!
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 const_eval_raw.
back to topic:
I think we should do something where associated constants and regular constants get handled the same way by UnusedBrokenConst. Not sure what that is, but the current PR is heading in that direction although maybe not on the path that we want. The reason we even have UnusedBrokenConst instead of doing everything in const_prop is the statement at the entry function of const_prop: we're going to evaluate the constant anyway.
So... I'm not sure what I think is the best way forward, but in general doing const_prop is more powerful because it doesn't bail out early if there's an error.
There was a problem hiding this comment.
I feel associated consts are more like (const) fn: there might be inputs missing. Thus we need const_prop, which is prepared for that case.
Of course, the actual "propagation" part of const_prop makes little sense here. Maybe putting lint and transformation together was just a bad call? (Another weird effect of this is that some warnings only appear in release mode, such as this one.) (This is already fixed on beta.)
There was a problem hiding this comment.
I wonder if there was any specific reason you went for const_eval?
I looked into how unused non-assoc constants were linted and found UnusedBrokenConst, which runs const_eval_poly. I was hoping running const_eval_poly would handle all the cases where there weren't missing substitutions, and just bail out with TooGeneric in other cases:
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.
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_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.
in general doing const_prop is more powerful because it doesn't bail out early if there's an error
Ah, and that's why const_eval_poly was suitable for normal consts, but is not suitable for associated consts.
Maybe putting lint and transformation together was just a bad call
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.
| 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 |
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 (arithmetic_overflow and const_err), and if someone wanted to silence these errors, they would have to disable both lints. What's the go for duplicates like this?
|
☔ The latest upstream changes (presumably #70544) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Closing in favour of the PR I've just opened that uses const prop instead of const eval as suggested. The discussion here was really informative -- thanks, everyone! :) |
|
For future reference, that other PR is #70566 |
…op, r=RalfJung Don't bail out before linting in generic contexts. Fixes rust-lang#69021. cc rust-lang#70017 r? @RalfJung
…op, r=RalfJung Don't bail out before linting in generic contexts. Fixes rust-lang#69021. cc rust-lang#70017 r? @RalfJung
…op, r=RalfJung Don't bail out before linting in generic contexts. Fixes rust-lang#69021. cc rust-lang#70017 r? @RalfJung
Fixes #69021.
Changes UnusedBrokenConst to also run on associated consts (when previously, it only ran on consts). Previously, if an associated const was unused, we would not end up linting the value, because
const_prop(wherecheck_{unary, binary}_opare run) would never reach the value.r? @RalfJung