-
Notifications
You must be signed in to change notification settings - Fork 13.4k
const-eval error: always say in which item the error occurred #142008
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.
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
base: master
Are you sure you want to change the base?
Conversation
@@ -2,7 +2,7 @@ error[E0080]: attempt to compute `u8::MAX + 1_u8`, which would overflow | |||
--> $DIR/default-param-wf-concrete.rs:4:28 | |||
| | |||
LL | struct Foo<const N: u8 = { 255 + 1 }>; | |||
| ^^^^^^^ evaluation of constant value failed here | |||
| ^^^^^^^ evaluation of `Foo::{constant#0}` failed here |
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.
This isn't great, ideally it'd say something like "evaluation of unnamed constant inside Foo
failed here" or so...
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.
yea, this may be sth we want to improve for all mentions of unnamed consts. The disambiguators don't help anyone but compiler devs.
@@ -2,7 +2,7 @@ error[E0080]: evaluation panicked: explicit panic | |||
--> $DIR/issue-81899.rs:6:24 | |||
| | |||
LL | const _CONST: &[u8] = &f(&[], |_| {}); | |||
| ^^^^^^^^^^^^^^ evaluation of constant value failed here | |||
| ^^^^^^^^^^^^^^ evaluation of `f::<{closure@$DIR/issue-81899.rs:6:31: 6:34}>` failed here |
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.
I am not entirely sure why this doesn't say _CONST
...
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.
Uh, we take the instance from the top of the stack. Possibly a remnant from early miri days? We should just take the current item's global id
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.
I haven't found where in the code we do that...
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.
Nope, we don't. huh. This is very weird. Even if some promotion thing were going on, we should be reporting the promoted in that case
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.
Right, you're sharing my confusion now. ;)
I can only look at this more closely next weekend -- I thought this would be a largely editorial change.^^
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.
lmao I found it
rust/compiler/rustc_const_eval/src/errors.rs
Lines 286 to 290 in b331b8b
impl Subdiagnostic for FrameNote { | |
fn add_to_diag<G: EmissionGuarantee>(self, diag: &mut Diag<'_, G>) { | |
diag.arg("times", self.times); | |
diag.arg("where_", self.where_); | |
diag.arg("instance", self.instance); |
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.
Oh wow, this is nasty form of global state...
@@ -2,7 +2,7 @@ error[E0080]: writing to ALLOC0 which is read-only | |||
--> $DIR/issue-100313.rs:18:5 | |||
| | |||
LL | x.set_false(); | |||
| ^^^^^^^^^^^^^ evaluation of constant value failed here | |||
| ^^^^^^^^^^^^^ evaluation of `T::<&true>::set_false` failed here |
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.
Why does this use the name of the function rather than the name of the surrounding constant...?
@@ -14,7 +14,7 @@ error[E0080]: calling non-const function `double` | |||
--> $DIR/const_fn_ptr_fail2.rs:19:18 | |||
| | |||
LL | const Z: usize = bar(double, 2); // FIXME: should fail to typeck someday | |||
| ^^^^^^^^^^^^^^ evaluation of constant value failed here | |||
| ^^^^^^^^^^^^^^ evaluation of `bar` failed here |
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.
Okay I think this is just using the wrong instance
? It seems to be the instance of the top frame, rather than the surrounding constant.
I entirely forgot that we print the backtrace backwards now. Still not sure if that's really a good idea. But that means the error itself always points inside the constant, right? So maybe there's no reason to put the constant name in the error message after all.
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.
Yeah something is very odd here: #142010
The job Click to see the possible cause of the failure (guessed by this bot)
|
Once #142015 lands, do we even still want this? The first label always points inside the "root constant" (I forgot about that), so we don't have to repeat the name of that constant. It's kind of unnecessary that we spell out whether it's a static or const but 🤷 |
I think we still want this PR just as a simplification without any downsides |
☔ The latest upstream changes (presumably #142081) made this pull request unmergeable. Please resolve the merge conflicts. |
I don't see why "is this generic" should make a difference. It may be reasonable to key this on whether the error occurs in a
const fn
that was invoked by a const (making it non-obvious which constant it is) vs inside the body of the const.r? @oli-obk