Refactor const_eval diagnostics #113297

victor-timofei · 2023-07-03T18:04:46Z

Implement IntoDiagnostic for const_eval newtypes.

rustbot · 2023-07-03T18:04:53Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @compiler-errors (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

compiler-errors · 2023-07-03T18:07:54Z

@fee1-dead weren't you planning on refactoring some of this code? or was it a different set of const-eval diagnostics?

fee1-dead · 2023-07-03T18:09:22Z

I opened the linked issue.

compiler-errors · 2023-07-03T18:10:04Z

i can't read

compiler/rustc_const_eval/src/errors.rs

compiler/rustc_mir_transform/src/const_prop.rs

compiler/rustc_const_eval/src/const_eval/error.rs

compiler/rustc_mir_transform/src/const_prop.rs

compiler/rustc_const_eval/src/const_eval/error.rs

fee1-dead · 2023-07-18T18:21:10Z

Marking as waiting on author. When you are done use @rustbot ready.

@rustbot author

victor-timofei · 2023-07-18T20:35:32Z

@rustbot ready

bors · 2023-07-21T17:27:56Z

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

bors · 2023-07-25T17:06:43Z

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

fee1-dead · 2023-07-26T04:09:49Z

@rustbot author

victor-timofei · 2023-07-27T09:44:24Z

@rustbot ready

fee1-dead · 2023-08-12T09:04:14Z

compiler/rustc_const_eval/src/errors.rs

+            }
+            BoundsCheckFailed { len, index } => {
+                eagerly_translate_with_args!(
+                    handler,
+                    const_eval_bounds_check_failed,
+                    "len",
+                    len,
+                    "index",
+                    index,
+                )
+            }
+            DivisionByZero => {
+                eagerly_translate_with_args!(handler, const_eval_division_by_zero,)
+            }


There's a lot of code duplication and I think we should avoid that. Perhaps this should be an associated function in UndefinedBehaviorInfoExt that uses a hack similar to what I have done in miri:

rust/src/tools/miri/src/diagnostics.rs

Lines 339 to 347 in b08dd92

let e = {

let handler = &ecx.tcx.sess.parse_sess.span_diagnostic;

let mut diag = ecx.tcx.sess.struct_allow("");

let msg = e.diagnostic_message();

e.add_args(handler, &mut diag);

let s = handler.eagerly_translate_to_string(msg, diag.args());

diag.cancel();

s

};

Are you saying this because of the *Ext and *Ext2 variants both existing? *Ext is the old implementation which I will remove.

I.. don't think that will work out? But feel free to try. I'm mostly thinking that what *Ext2 is renamed to should mostly use *Ext's code.

*Ext types are no longer needed, they are an IntoDiagnostic implementation and can be completely removed since the current implementation doesn't use IntoDiagnostic.

Ah okay I see. Go ahead and remove it.

fee1-dead · 2023-08-12T09:09:22Z

compiler/rustc_const_eval/src/errors.rs

+impl AddToDiagnostic for ValidationErrorInfoExt2<'_> {
+    fn add_to_diagnostic_with<F>(self, diag: &mut Diagnostic, _: F)
+    where


ValidationErrorInfoExt2 is an unhelpful name. I think this should be an associated function for ValidationErrorInfoExt named as_subdiagnostic and this type can be called ValidationErrorInfoAsSubdiagnosticExt. Because the type name would be so long, I think you should seal it in a private module so that no one except rustc_const_eval::errors can refer to it.

Would you like all *Ext2 variants to be renamed in a similar fashion?

I've gotten rid of the *Ext and *Ext2 confusion and merged it into *Ext. Would you still like proceed with the renaming of the types?

victor-timofei · 2023-08-15T16:19:52Z

@rustbot ready

rustbot · 2023-08-18T06:46:39Z

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

RalfJung · 2023-08-19T14:21:27Z

src/tools/miri/src/diagnostics.rs

-        diag.cancel();
-        s
-    };
+    let e = InterpErrorExt{


Suggested change

let e = InterpErrorExt{

let e = InterpErrorExt {

RalfJung · 2023-08-19T14:26:31Z

I was hoping this would simplify error handling, but a +600 line diff (!!) sounds like this is even more complicated than before? Wasn't the point that we should be able to derive a whole bunch of things?

Does this PR even further increases the number of places where I have to change things when an error message changes? I first thought so due to all the new struct types, but now I'm now sure any more. The entire thing became so messy it's hard to say what happens.^^

RalfJung · 2023-08-19T14:35:36Z

compiler/rustc_const_eval/src/errors.rs

+            [
+                $(($key.into(), $value.into_diagnostic_arg()),)*
+            ].iter()
+                .map(|(a, b)| (a, b))


What does this do? Seems to be an identity map which doesn't do anything?
Please either remove or add a comment explaining its purpose.

davidtwco

I'd really like to avoid making diagnostics a lot more complicated for maintainers of various parts of the compiler just for translation/diagnostic structs - we've not done a great job of that so far and it's something I'm going to be focusing on.

That said, I really appreciate the work you've put into this @victor-timofei and I've got a few ideas of how we might go about doing this in a way which is less invasive for the const_eval folks.

I'm curious about the use of eager translation. I added eager translation to support a very specific circumstance - when we had a Vec<Subdiagnostic> and that meant the same argument would be getting set multiple times - unless we translate after doing the set_arg for each element, we'd only have the final values when translating at emission time. I intended that eager translation would otherwise be unnecessary. If it's required here, then I'd be curious to learn why that is :)

I think what I'd like to see, is that we use the diagnostic macros on types like InterpError or ValidationErrorInfo. These wouldn't be sufficient, because they're missing the Span, so we'd still have InterpErrorExt or ValidationErrorInfoExt that contains the original type and the Span. We'd implement IntoDiagnostic for these types, and they would basically just do:

fn into_diagnostic(self, handler: &'a Handler) -> DiagnosticBuilder<'a, T> {
    let diag = self.err.into_diagnostic(handler);
    diag.set_span(self.span);
    diag
}

I think this would avoid a lot of the boilerplate that you have right now. We could go a little bit further though, and add support to the macros for this type of pattern, because it's not the first time I've seen it.

#[derive(Diagnostic)]
struct SpannedInterpError {
    #[primary_span]
    span: Span
    #[original]
    error: InterpError,
}

Do you think this would work?

RalfJung · 2023-08-21T15:20:08Z

I should also add, thanks to @victor-timofei for helping us explore this! I didn't mean to discourage you with my comment, I was just hoping that there'd be a better solution with less boilerplate. :)

The original const_eval translation PR had a +1200 line diff, so with this one we are at a total of almost +2000, which to me indicates the approach might be flawed -- but I don't understand the approach enough to suggest better ways, unfortunately. Thanks @davidtwco for chiming in!

victor-timofei · 2023-08-21T15:42:03Z

Hi @davidtwco, @RalfJung thanks a lot for the review I'll try experimenting a more with the derivable Diagnostic now that we added the Spans. Hopefully it will reduce some of the code.

@RalfJung no worries, I understand your point of view. 😃 I'm also still learning how the Diagnostic works so I don't mind it. 😛

victor-timofei · 2023-08-21T15:47:56Z

src/tools/miri/src/diagnostics.rs

@@ -336,15 +336,11 @@ pub fn report_error<'tcx, 'mir>(

    // FIXME(fee1-dead), HACK: we want to use the error as title therefore we can just extract the


@davidtwco const_eval errors are used here as titles, that is why we need to eagerly translate.

We are also doing it to avoid args "pollution" in the DiagnosticBuilder.

cc: @fee1-dead

Even if you use the derive macro, the hack that worked before still works:

let e = { let handler = &ecx.tcx.sess.parse_sess.span_diagnostic; let mut diag = ecx.tcx.sess.struct_allow(""); let msg = e.diagnostic_message(); e.add_args(handler, &mut diag); // ^^ NOTE that these two functions above don't exist for `IntoDiagnostic`, // but we can use `create_diagnostic` for this let s = handler.eagerly_translate_to_string(msg, diag.args()); diag.cancel(); s };

the FIXME is just there to encourage someone to find a better way than creating a dummy diagnostic to extract the title, and doesn't have to be fixed by this PR.

Unless I'm still misunderstanding something, the approach I've suggested should work for this - because the InterpErrorExt will call IntoDiagnostic on InterpError, it'll get the InterpError's primary message, and if it is the opposite of that which you want, then you can always change it in InterpErrorExt's implementation.

fee1-dead · 2023-08-22T08:15:55Z

cc #115089

r? @davidtwco

bors · 2023-08-30T08:40:36Z

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

victor-timofei · 2023-09-04T17:05:16Z

Hi @davidtwco sorry for the delayed response. I needed some time to process this, and also was busy with other stuff 😝

So implementing the following for the spanned newtypes:

#[derive(Diagnostic)]
struct SpannedInterpError {
    #[primary_span]
    span: Span
    #[original]
    error: InterpError,
}

Would still require to implement the IntoDiagnostic trait for the "original" types, which would require moving the trait implementations and fluent messages to rustc-middle.

Still this wouldn't save us from having to manually implement IntoDiagnostic since:

some enum variants (i.e.: UndefinedBehaviorInfo here, UnsupportedOpInfo here) emit custom fluent slugs.
for <dyn MachineStopType> we need to iterate over a Vec<args> (although this MachineStopType does not cause the majority of the boilerplate code)
and the "original" types would still be missing Span.

davidtwco · 2023-09-06T14:45:53Z

Would still require to implement the IntoDiagnostic trait for the "original" types, which would require moving the trait implementations and fluent messages to rustc-middle.

👍

Still this wouldn't save us from having to manually implement IntoDiagnostic since:

some enum variants (i.e.: UndefinedBehaviorInfo here, UnsupportedOpInfo here) emit custom fluent slugs.

for <dyn MachineStopType> we need to iterate over a Vec<args> (although this MachineStopType does not cause the majority of the boilerplate code)

Without looking too closely, you would probably still need manual implementations in these cases. I'll look more closely once you've implemented the rest :)

and the "original" types would still be missing Span.

That's fine, you don't need a Span to use the derive. #[primary_span] is optional.

RalfJung · 2023-09-06T15:59:58Z

some enum variants (i.e.: UndefinedBehaviorInfo here, UnsupportedOpInfo here) emit custom fluent slugs.

For these two, can't we have a float diagnostic that is just const_eval_custom_ub = {$message}? Then they should behave like all other diagnostics.

apiraino · 2023-10-05T12:59:55Z

Think I can switch to waiting on author (if I understand correctly). @victor-timofei Feel free to request a review with @rustbot ready, thanks!

@rustbot author

fee1-dead · 2023-10-16T03:44:43Z

@victor-timofei: friendly ping :) are you still working on this?

JohnCSimon · 2024-02-11T20:04:59Z

@victor-timofei
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you are going to continue please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive

rustbot assigned compiler-errors Jul 3, 2023

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 Jul 3, 2023

fee1-dead reviewed Jul 5, 2023

View reviewed changes

compiler/rustc_const_eval/src/errors.rs Outdated Show resolved Hide resolved

compiler/rustc_const_eval/src/errors.rs Outdated Show resolved Hide resolved

compiler-errors assigned fee1-dead and unassigned compiler-errors Jul 6, 2023

This comment has been minimized.

Sign in to view

victor-timofei commented Jul 13, 2023

View reviewed changes

compiler/rustc_mir_transform/src/const_prop.rs Outdated Show resolved Hide resolved

victor-timofei commented Jul 13, 2023

View reviewed changes

compiler/rustc_const_eval/src/const_eval/error.rs Outdated Show resolved Hide resolved

fee1-dead reviewed Jul 16, 2023

View reviewed changes

compiler/rustc_mir_transform/src/const_prop.rs Outdated Show resolved Hide resolved

compiler/rustc_const_eval/src/const_eval/error.rs Outdated Show resolved Hide resolved

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 18, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 18, 2023

victor-timofei force-pushed the refactor-const-eval-diagnostics branch from 4ebf1e6 to 4a1770c Compare July 22, 2023 22:12

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 26, 2023

victor-timofei force-pushed the refactor-const-eval-diagnostics branch from 4a1770c to 8c8e300 Compare July 27, 2023 09:19

rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 27, 2023

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 11, 2023

fee1-dead reviewed Aug 12, 2023

View reviewed changes

victor-timofei added 2 commits August 13, 2023 12:32

Remove old *Ext types and fix type visibility

74063d4

Rename *Ext2 types to *Ext

f2af515

Dylan-DPC marked this pull request as ready for review August 18, 2023 06:46

RalfJung reviewed Aug 19, 2023

View reviewed changes

davidtwco requested changes Aug 21, 2023

View reviewed changes

victor-timofei commented Aug 21, 2023

View reviewed changes

rustbot assigned davidtwco and unassigned fee1-dead Aug 22, 2023

This was referenced Aug 22, 2023

More detail when expecting expression but encountering bad macro argument #114292

Merged

resolve: Stop creating NameBindings on every use, create them once per definition instead #113408

Merged

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 5, 2023

JohnCSimon closed this Feb 11, 2024

rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Feb 11, 2024

	let e = {
	let handler = &ecx.tcx.sess.parse_sess.span_diagnostic;
	let mut diag = ecx.tcx.sess.struct_allow("");
	let msg = e.diagnostic_message();
	e.add_args(handler, &mut diag);
	let s = handler.eagerly_translate_to_string(msg, diag.args());
	diag.cancel();
	s
	};

		@@ -336,15 +336,11 @@ pub fn report_error<'tcx, 'mir>(

		// FIXME(fee1-dead), HACK: we want to use the error as title therefore we can just extract the

Refactor const_eval diagnostics #113297

Refactor const_eval diagnostics #113297

Conversation

victor-timofei commented Jul 3, 2023 • edited Loading

rustbot commented Jul 3, 2023

compiler-errors commented Jul 3, 2023

fee1-dead commented Jul 3, 2023

compiler-errors commented Jul 3, 2023

This comment has been minimized.

This comment has been minimized.

fee1-dead commented Jul 18, 2023

victor-timofei commented Jul 18, 2023

bors commented Jul 21, 2023

bors commented Jul 25, 2023

fee1-dead commented Jul 26, 2023

victor-timofei commented Jul 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victor-timofei commented Aug 15, 2023

rustbot commented Aug 18, 2023

Choose a reason for hiding this comment

RalfJung commented Aug 19, 2023 • edited Loading

Choose a reason for hiding this comment

davidtwco left a comment • edited Loading

Choose a reason for hiding this comment

RalfJung commented Aug 21, 2023

victor-timofei commented Aug 21, 2023

Choose a reason for hiding this comment

fee1-dead Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fee1-dead commented Aug 22, 2023

bors commented Aug 30, 2023

victor-timofei commented Sep 4, 2023

davidtwco commented Sep 6, 2023

RalfJung commented Sep 6, 2023

apiraino commented Oct 5, 2023

fee1-dead commented Oct 16, 2023

JohnCSimon commented Feb 11, 2024

victor-timofei commented Jul 3, 2023 •

edited

Loading

RalfJung commented Aug 19, 2023 •

edited

Loading

davidtwco left a comment •

edited

Loading

fee1-dead Aug 22, 2023 •

edited

Loading