-
Notifications
You must be signed in to change notification settings - Fork 13.4k
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
Conversation
This comment has been minimized.
This comment has been minimized.
☔ 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); | ||
} |
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.
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 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.
There is, but not without the aforementioned refactoring
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.
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?
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.
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 `!` |
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.
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.
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.
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 |
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 really needs more context, e.g. by starting with "compile-time evaluation failed: ...".
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.
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
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.
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.
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.
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
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.
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
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.
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.
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 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.
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 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
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 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 |
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 note attached to the span also looks very strange to me. It doesn't even point at a constant value.
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 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.
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.
// 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.
unless there are generics involved, we do not show the path
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.
Hm, interesting. With this being in the span I think we should definitely change that. In a separate PR.
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. |
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.^^ |
This comment has been minimized.
This comment has been minimized.
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. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@bors r=RalfJung |
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
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
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 😆