Const-eval InterpError: error enum variants vs throw_ub_custom!

[RalfJung]

Currently we have dedicated error variants for many interpreter errors in our InterpError enum, but for some errors we use throw_ub_custom! to directly pick a translatable diagnostic instead of a variant.

The original purpose of these variants was to

• make sure the same error looks the same when raised from multiple locations
• avoid expensive operations such as string formatting when constructing an error, if it is never actually shown to the user (because it is caught again later, e.g. during value validation)

The first point doesn't really apply any more with translatable diagnostics. And the overhead of these error variants is quite significant; they each need an arm in diagnostic_message and add_args. (And these two things need to be carefully synced, since diagnostic_message decides which arguments need to be added later! Would be nice if these could be syntactically together somehow. Right now not only are we using an entirely untyped system here without any checks whether the right arguments are being added, we also have setting the arguments quite far removed from the only place that could potentially tell us which arguments are the right ones. That's pretty bad for maintenance.) That makes it tempting to convert errors to throw_ub_custom! and reduce this boilerplate, but I don't know if that is a good idea -- @fee1-dead suggested we should avoid throw_ub_custom!.

I also see some throw_ub_custom! do expensive work on error creation (such as this), though under the hood the macro puts this into a thunk so -- if the error is never rendered, does the format! ever happen?

Either way, we should come up with some kind of consistent policy here. We have a lot of throw_ub_custom! currently (34 to be precise), I don't quite see the point of turning them all into variants -- that would be a lot of work, for which gain?

[RalfJung]

There's something else about this that I do not understand... part of the issue here was that we needed a diagnostics type that includes a Span, and so we have this entire duplication of error types in compiler/rustc_const_eval/src/errors.rs.

However, when emitting a translatable lint, we are using diagnostics types without a span! Here are some examples. Only when emitting an error do we need a diagnostic type with span. This makes it a bunch of work to transition between lint and hard error, and also causes a lot of the pain in const-eval.

Why can't we emit hard errors the same way we emit lints, i.e. with these two separate arguments (that's from emit_spanned_lint)?

        span: impl Into<MultiSpan>,
        decorator: impl for<'a> DecorateLint<'a, ()>,

Instead, emit_err takes an err: impl IntoDiagnostic<'a>.

Cc @davidtwco

[davidtwco]

I'm aware of this specific issue, and addressing the various paper cuts around the diagnostic structs and translatable diagnostics is near the top of my to-do list (I'm also unhappy with the impact of the translatable diagnostic structs on our ergonomics), so I'll assign myself to this and make sure to loop back to it when I've got a little bit more time to write something that would useful.

However, I can quickly answer this:

Why can't we emit hard errors the same way we emit lints, i.e. with these two separate arguments (that's from emit_spanned_lint)?

Lints are handled differently from diagnostics, because the lint machinery uses the Span to decide on whether a lint should be silenced entirely, a warning or an error (the Span lets it look for #[allow(..)] or #[deny(..)], etc). We probably could do hard errors in the same way, I just hadn't thought to do that at the time. Including the Span in the error itself is useful though, because then you can easily add additional warnings/notes/labels to that same Span by just annotating the field.

We have an implementation of IntoDiagnostic on Spanned and that might let you separate out the Span from the type, and ideally re-use the type part as a lint - though we would need unifying the DecorateLint and IntoDiagnostic traits for that, which might be achievable.

[RalfJung]

Including the Span in the error itself is useful though, because then you can easily add additional warnings/notes/labels to that same Span by just annotating the field.

Ah I see, makes sense.

So yeah, having the option of giving the span and diagnostics info separately for hard errors might be useful, both for porting lints to hard errors and for making lints have less overhead in const-eval.

[RalfJung]

Lints are handled differently from diagnostics, because the lint machinery uses the Span to decide on whether a lint should be silenced entirely, a warning or an error (the Span lets it look for #[allow(..)] or #[deny(..)], etc).

Hm, looking at the API again I am actually getting doubts about this. The lint-emitting functions take both a hir_id and a span. I thought it is the hir_id that is used to determine which #[allow(..)] etc apply? There are various parts in the compiler where a lint_root function is used to compute said HIR ID.

[RalfJung]

However, when emitting a translatable lint, we are using diagnostics types without a span! Here are some examples. Only when emitting an error do we need a diagnostic type with span. This makes it a bunch of work to transition between lint and hard error, and also causes a lot of the pain in const-eval.

This feels somewhat separate from the const-eval issue, so I have opened a separate issue to track this: #121077.

[RalfJung]

I wonder if we can use the derive(LintDiagnostic) machinery in const-eval somehow to get automatic code generation for diagnostics without a span, and then have an "adapter" that adds the span so this can be fed into the hard-error APIs? Or would that be too much of a hack?^^

Like, I am imagining something like

#[derive(Debug,LintDiagnostic)]
pub enum UndefinedBehaviorInfo<'tcx> {
    /// Unreachable code was executed.
    #[diag(const_eval_ub_unreachable)]
    Unreachable,
    /// A slice/array index projection went out-of-bounds.
    #[diag(const_eval_ub_bounds_check_failed)]
    BoundsCheckFailed { len: u64, index: u64 },
    /// Something was divided by 0 (x / 0).
    #[diag(const_eval_ub_division_by_zero)]
    DivisionByZero,
// ...

Not sure if that is even remotely realistic though.

[davidtwco]

I'd like to be able to do something like that eventually, from my earlier comment:

We have an implementation of IntoDiagnostic on Spanned and that might let you separate out the Span from the type, and ideally re-use the type part as a lint - though we would need unifying the DecorateLint and IntoDiagnostic traits for that, which might be achievable.

I'm working towards this at the moment.