Making enums more specific in src/librustc/mir/interpret/error.rs

[TheDarkula]

While chatting with @oli-obk, we came up with this schema for separating the EvalErrorKind enum into two parts.
What's everyone's thoughts on this?

---

• Classify the unclassified variants of UnsupportedOpInfo
• Figure out what to do with panics (see Miri engine: proper support for Assert MIR terminators rust#66874 (comment))
• Errors caused by misuse of lang items or intrinsics should bug!
• Review every use of throw_unsup and err_unsup to make sure we really want them to be marked as "unsupported", not something else

[oli-obk]

The reason we talked about this was to have a clear separation between the variants that produce panics when occurring inside a promoted, and the variants that produce an abort (or a compiler-error pending unconst RFCs and such)

[oli-obk]

cc @RalfJung @eddyb you might have opinions on this. We could alternatively just have a method that decides which variants report what, but it seems more fragile wrt future additions

[eddyb]

I like the enum split.

[RalfJung]

Related miri issue: rust-lang/miri#417

So if we are touching errors at all, I would like to also separate "operation not implemented" from "your program just caused UB". So I imagine a top-level enum with three variants (unimplemented, panic, UB), and then sub-enums (Unimplemented can likely directly be a string like it is now).

Furthermore, I'd like to reevaluate the use of having so many enum variants. Many of them miss information that would be very useful to have when an error occurs, e.g. ReadBytesAsPointer does not say which bytes. Is it really worth having so many different structural errors, or should we give up an use strings more? We already use strings for some things.

Also, EvalErrorPanic::Panic should (optionally?) take a panic string. miri can actually get that string even when formatting is involved.

[eddyb]

@RalfJung we need to be really careful with strings: if we don't intern them, we could "leak" memory from things that may not be an user-facing error.
That is, anything that could fail because some generic parameters are not fully specified yet, should be as cheap as possible, because we'll certainly generate tons of them.

[RalfJung]

I do not fully understand. What would be an example of a problem?

Maybe instead of using strings we can use the failure crate, which makes creating error enums with nice formatting much nicer? :D

[eddyb]

@RalfJung The "problem" just increased memory usage, although I doubt it would be significant. Also, does the failure crate give you a hashable enum with no extra dynamic allocations?
If so, it's probably okay to use (I now wonder if we could do this for diagnostics in general).

[oli-obk]

Also, EvalErrorPanic::Panic should (optionally?) take a panic string. miri can actually get that string even when formatting is involved.

Already done in rust-lang/rust#52011

[RalfJung]

Another inconsistency: We have dedicated error variants for invalid bool/char that are used when such an invalid value occurs in a normal operation. But for strings, we use a ValidationError.

[TheDarkula]

@RalfJung for your "top-level" design, were you thinking something like this:

#[derive(Clone, RustcEncodable, RustcDecodable)]
pub enum EvalErrorKind<'tcx, O> {
    Panic(EvalErrorPanic<'tcx, O>),
    UndefinedBehaviour(EvalErrorUndefinedBehaviour<'tcx, O>),
    Unimplemented(EvalErrorUnimplemented<'tcx, O>),
}

With each variant being the nested corresponding enum?

[oli-obk]

Is it really worth having so many different structural errors, or should we give up an use strings more? We already use strings for some things.

Since we catch some of these errors and silently do something else, we should not be allocating (and especially not formatting) strings.

Any truly terminal error variant could be merged into a string error variant though. Maybe really just ValidationError.

[RalfJung]

@TheDarkula yes, that looks great! I hope only the EvalErrorPanic will even need the O though -- AssertMessage should be just an EvalErrorPanic, I think.

Since we catch some of these errors and silently do something else, we should not be allocating (and especially not formatting) strings.

Yeah I guess we'll have to see in each of these variants where strings are appropriate. I think for EvalErrorUnimplemented there is no reason for anything but a string.

---

Now, one thing I am not sure where to put is when an endless loop was detected. That's not UB and not unimplemented, and not really a panic either. What do you think?

[TheDarkula]

Now, one thing I am not sure where to put is when an endless loop was detected. That's not UB and not unimplemented, and not really a panic either. What do you think?
Do you just want a fourth sub-enum like:

#[derive(Clone, RustcEncodable, RustcDecodable)]
pub enum EvalErrorKind<'tcx, O> {
    Panic(EvalErrorPanic<'tcx, O>),
    UndefinedBehaviour(EvalErrorUndefinedBehaviour<'tcx, O>),
    Unimplemented(EvalErrorUnimplemented<'tcx, O>),
    InfiniteLoop(String),
}

[RalfJung]

That would be the other option.

[TheDarkula]

What other idea(s) did you have in mind?