Skip to content

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

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

Merged
merged 2 commits into from
Jun 4, 2025

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented May 28, 2025

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 😆

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 28, 2025
@rustbot
Copy link
Collaborator

rustbot commented May 28, 2025

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

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented May 29, 2025

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

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

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is, but not without the aforementioned refactoring

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@RalfJung RalfJung May 30, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@oli-obk oli-obk May 30, 2025

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

--> $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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

   // 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

Copy link
Member

Choose a reason for hiding this comment

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

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

@oli-obk
Copy link
Contributor Author

oli-obk commented May 30, 2025

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
Copy link
Member

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.^^

@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member

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.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 3, 2025

@bors r=RalfJung

@bors
Copy link
Collaborator

bors commented Jun 3, 2025

📌 Commit 020216c has been approved by RalfJung

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 3, 2025
bors added a commit that referenced this pull request Jun 3, 2025
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
@bors bors merged commit f574c0e into rust-lang:master Jun 4, 2025
10 checks passed
@rustbot rustbot added this to the 1.89.0 milestone Jun 4, 2025
rust-timer added a commit that referenced this pull request Jun 4, 2025
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 😆
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Jun 4, 2025
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
bjorn3 pushed a commit to rust-lang/rustc_codegen_cranelift that referenced this pull request Jun 5, 2025
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants