Use the informative error as the main const eval error message #141698

oli-obk · 2025-05-28T15:17:37Z

I only did the minimal changes necessary to the const eval error machinery. I'd prefer not to mix test changes with refactorings 😆

rustbot · 2025-05-28T15:17:44Z

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

bors · 2025-05-29T09:44:43Z

☔ The latest upstream changes (presumably #141716) made this pull request unmergeable. Please resolve the merge conflicts.

compiler/rustc_const_eval/src/const_eval/eval_queries.rs

compiler/rustc_const_eval/src/const_eval/error.rs

RalfJung · 2025-05-30T12:37:44Z

compiler/rustc_const_eval/src/const_eval/eval_queries.rs

+            diag.arg("error_kind", kind);
+            for frame in frames {
+                diag.subdiagnostic(frame);
+            }


Please add a comment explaining what we are doing here and why. Why is the error code now repeated in a bunch of places where it was central before? Seems like this is now doing by hand what previously was done by the derive macros for the diagnostic types? Is there no way to still use a diagnostic type?

There is, but not without the aforementioned refactoring

Is it because diagnostic error types need to have a single fixed primary message and can't read it from their fields or have it otherwise altered?

they can read it from their fields, but it needs work to set up as we can't just use the enums we have, or even just use a fluent identifier as a derive field. We'd have to eagerly format the error to a string, put that in a field and then make the derive just print that field. But that also needs a refactoring because the current logic isn't set up to do that

RalfJung · 2025-05-30T12:40:05Z

tests/ui/consts/assert-type-intrinsics.stderr

@@ -1,20 +1,20 @@
-error[E0080]: evaluation of constant value failed
+error[E0080]: evaluation panicked: aborted execution: attempted to instantiate uninhabited type `!`


Shouldn't this say "compile-time evaluation panicked" or so? Previously, the primary message "evaluation of constant value failed" set the context, but now there's no more such context so it's unclear what evaluation panicked. It's not the compiler itself, after all.

well imo we should just not have anything in the main message and instead just list the user specified error message, but that's a different story. All the information is still in the diagnostic, just the places changed

RalfJung · 2025-05-30T12:41:37Z

tests/ui/consts/assoc_const_generic_impl.stderr

@@ -1,8 +1,8 @@
-error[E0080]: evaluation of `<u32 as ZeroSized>::I_AM_ZERO_SIZED` failed
-  --> $DIR/assoc_const_generic_impl.rs:9:34
+error[E0080]: index out of bounds: the length is 1 but the index is 4


This really needs more context, e.g. by starting with "compile-time evaluation failed: ...".

heh fun, this is a case where the diagnostic looks exactly like I want it to.

I don't think we should be adding more stuff to the main message. The main message is the core information of the error and everything establishing context should be in the labels and notes

It makes no sense to me that we would put the context after the message that needs to be interpreted in that context. I don't think that's what we do elsewhere either? We give some extra details later, sure, but we need to start by roughly putting things in the right place, e.g. whether it's about a variable having the wrong type or a trait bound not being satisfied -- or a constant failing to evaluate.

If I compare this error with before the PR, I think the old one is strictly better in terms of how it looks in a terminal. I haven't checked how this looks in an IDE.

that's what we do everywhere? The main message for almost every trait solver or borrowck error is basically "x went boom" and then there are labels and notes pointing to why and where

but either way, I don't want to change this here. We have an established system and I'm happy arguing about it (as I want to change it), but in this PR I just want to flip the labels, not change anything

The PR fundamentally changes everything about const-eval errors by changing the order -- and in its current form, I think it's a change for the worse.

You are right that the other errors do not say "borrow checking failed: ...". They just say e.g. "cannot return reference to local variable y". I guess that never bothered me because having compile errors from the type checker or borrow checker is, like, the normal case. Having them from running some code feels different and feels to me like one has to prepare the user for a fundamentally different kind of problem to be solved.

I think it would be nice if the main error message was fully user controlled (whatever the panic message at const eval time is), and just like with diagnostics manipulated by diagnostics attributes, we show the detailed information in labels and notes.

The PR fundamentally changes everything about const-eval errors by changing the order

fair

and in its current form, I think it's a change for the worse.

I don't think it is. And I'm not going to be rebasing this PR for a few months trying to improve something I don't think needs improving. I would like to merge this PR or close it, but whomever tries again will have to tackle 200+ issues that need their annotations edited, so I'm not holding my breath on anyone tackling this

I think it would be nice if the main error message was fully user controlled (whatever the panic message at const eval time is),

Ah I guess this here is a panic message... but I was not talking about panics specifically, maybe I should have picked a different example.

And I'm not going to be rebasing this PR for a few months trying to improve something I don't think needs improving. I would like to merge this PR or close it, but whomever tries again will have to tackle 200+ issues that need their annotations edited, so I'm not holding my breath on anyone tackling this

Yeah I know, this is a super bitrotty PR so we can't just discuss for weeks. In retrospective it'd have been better to get consensus on the new error format before putting in all that work. Alas, too late...

I'll think some more about this later tonight after getting some other work done. As mentioned in the other message, I'd also appreciate feedback from more people here.

RalfJung · 2025-05-30T13:14:45Z

tests/ui/consts/assert-type-intrinsics.stderr

  --> $DIR/assert-type-intrinsics.rs:11:9
   |
 LL |         MaybeUninit::<!>::uninit().assume_init();
-   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ evaluation panicked: aborted execution: attempted to instantiate uninhabited type `!`
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ evaluation of constant value failed


This note attached to the span also looks very strange to me. It doesn't even point at a constant value.

Why doesn't it say the name of the constant here, but it does say the name for some of the other errors?
I guess that shouldn't be changed in this PR but it seems worth an issue.

// If the current item has generics, we'd like to enrich the message with the // instance and its args: to show the actual compile-time values, in addition to // the expression, leading to the const eval error.

#85767

unless there are generics involved, we do not show the path

Hm, interesting. With this being in the span I think we should definitely change that. In a separate PR.

tests/ui/consts/assoc_const_generic_impl.stderr

oli-obk · 2025-05-30T14:30:52Z

sorry about the grumpiness, I assumed this was a slam dunk thing, and seeing that expectation broken annoyed me. It's on me for assuming that, and I still don't see any other incremental way forward than this one, but that doesn't mean I should just let my grumpiness out on you.

RalfJung · 2025-05-30T14:38:55Z

Hey no worries, I was worried I'm being too harsh here. :) I'll let it sit for a bit and come back later tonight or tmr with a fresh eye. It's so hard to evaluate these subjective things...

FWIW I'd also welcome feedback from other people, it's entirely possible that I'm the odd one out here and we shouldn't optimize for my sensitivities.^^

RalfJung · 2025-05-31T07:14:05Z

After sleeping over it, I think I'm fine with landing this. It does indeed make const-eval errors look more like the rest of our errors, as can also be seen by it working much better with how we do ui test annotations.

r=me after adding comments for these two closures that now manually do the work that used to be done by the derive macro.

oli-obk · 2025-06-03T06:46:55Z

@bors r=RalfJung

bors · 2025-06-03T06:46:58Z

📌 Commit 020216c has been approved by RalfJung

It is now in the queue for this repository.

Rollup of 8 pull requests Successful merges: - #137725 (Add `iter` macro) - #141455 (std: abort the process on failure to allocate a TLS key) - #141569 (Replace ad-hoc ABI "adjustments" with an `AbiMap` to `CanonAbi`) - #141698 (Use the informative error as the main const eval error message) - #141925 (Remove bootstrap cfgs from library/) - #141943 (Remove pre-expansion AST stats.) - #141945 (Remove `Path::is_ident`.) - #141957 (Add missing `dyn` keywords to tests that do not test for them Part 2) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of #141698 - oli-obk:ctfe-err-flip, r=RalfJung Use the informative error as the main const eval error message r? `@RalfJung` I only did the minimal changes necessary to the const eval error machinery. I'd prefer not to mix test changes with refactorings 😆

Rollup of 8 pull requests Successful merges: - rust-lang/rust#137725 (Add `iter` macro) - rust-lang/rust#141455 (std: abort the process on failure to allocate a TLS key) - rust-lang/rust#141569 (Replace ad-hoc ABI "adjustments" with an `AbiMap` to `CanonAbi`) - rust-lang/rust#141698 (Use the informative error as the main const eval error message) - rust-lang/rust#141925 (Remove bootstrap cfgs from library/) - rust-lang/rust#141943 (Remove pre-expansion AST stats.) - rust-lang/rust#141945 (Remove `Path::is_ident`.) - rust-lang/rust#141957 (Add missing `dyn` keywords to tests that do not test for them Part 2) r? `@ghost` `@rustbot` modify labels: rollup

Use the informative error as the main const eval error message r? `@RalfJung` I only did the minimal changes necessary to the const eval error machinery. I'd prefer not to mix test changes with refactorings 😆

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#137725 (Add `iter` macro) - rust-lang#141455 (std: abort the process on failure to allocate a TLS key) - rust-lang#141569 (Replace ad-hoc ABI "adjustments" with an `AbiMap` to `CanonAbi`) - rust-lang#141698 (Use the informative error as the main const eval error message) - rust-lang#141925 (Remove bootstrap cfgs from library/) - rust-lang#141943 (Remove pre-expansion AST stats.) - rust-lang#141945 (Remove `Path::is_ident`.) - rust-lang#141957 (Add missing `dyn` keywords to tests that do not test for them Part 2) r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned RalfJung May 28, 2025

rustbot added S-waiting-on-review T-compiler labels May 28, 2025

This comment has been minimized.

Sign in to view

oli-obk force-pushed the ctfe-err-flip branch from dd68967 to aacfd08 Compare May 28, 2025 16:31

RalfJung reviewed May 30, 2025

View reviewed changes

oli-obk force-pushed the ctfe-err-flip branch from aacfd08 to 8ec0bd2 Compare May 30, 2025 13:09

RalfJung reviewed May 30, 2025

View reviewed changes

tests/ui/consts/assoc_const_generic_impl.stderr Outdated Show resolved Hide resolved

oli-obk force-pushed the ctfe-err-flip branch from 8f9fc74 to 5fa39d9 Compare May 30, 2025 14:14

This comment has been minimized.

Sign in to view

oli-obk force-pushed the ctfe-err-flip branch from 5fa39d9 to 238f2f4 Compare June 2, 2025 08:35

This comment has been minimized.

Sign in to view

oli-obk force-pushed the ctfe-err-flip branch from 238f2f4 to 9d5d183 Compare June 2, 2025 13:01

This comment has been minimized.

Sign in to view

oli-obk force-pushed the ctfe-err-flip branch from 9d5d183 to 3f29739 Compare June 2, 2025 14:37

This comment has been minimized.

Sign in to view

oli-obk force-pushed the ctfe-err-flip branch from 3f29739 to 2e3d1ed Compare June 2, 2025 15:34

oli-obk added 2 commits June 2, 2025 15:37

Use the informative error as the main const eval error message

b331b8b

Clarify why we are talking about a failed const eval at a random place

020216c

oli-obk force-pushed the ctfe-err-flip branch from 2e3d1ed to 020216c Compare June 2, 2025 15:37

bors added S-waiting-on-bors and removed S-waiting-on-review labels Jun 3, 2025

matthiaskrgr mentioned this pull request Jun 3, 2025

Rollup of 8 pull requests #141984

Merged

bors merged commit f574c0e into rust-lang:master Jun 4, 2025
10 checks passed

rustbot added this to the 1.89.0 milestone Jun 4, 2025

		@@ -1,20 +1,20 @@
		error[E0080]: evaluation of constant value failed
		error[E0080]: evaluation panicked: aborted execution: attempted to instantiate uninhabited type `!`

Use the informative error as the main const eval error message #141698

Use the informative error as the main const eval error message #141698

Uh oh!

Conversation

oli-obk commented May 28, 2025

Uh oh!

rustbot commented May 28, 2025

Uh oh!

This comment has been minimized.

bors commented May 29, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oli-obk May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

oli-obk commented May 30, 2025

Uh oh!

RalfJung commented May 30, 2025

Uh oh!

This comment has been minimized.

RalfJung commented May 31, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

oli-obk commented Jun 3, 2025

Uh oh!

bors commented Jun 3, 2025

Uh oh!

Uh oh!

Uh oh!

RalfJung May 30, 2025 •

edited

Loading

oli-obk May 30, 2025 •

edited

Loading