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?

[RalfJung]

Putting it into one of the other three.^^

[TheDarkula]

Ah. I thought it was a distinct enough condition to be separate. Which one do you think it would fit best into?

[RalfJung]

Well I don't know, that's the point. ;)

[eddyb]

Isn't infinite loop a resource exhaustion problem? Where are the other resource limits enforced?

[RalfJung]

I suppose that would make a good 4th category: Resource exhaustion. We also have a stack size limit and, uh, we used to have a memory limit but it seems that was removed.

[oli-obk]

The stack size limit is a poor man's infinite recursion detector. we could just as well do the infinite recursion check the same way we do the infinite loop check, the same arguments to a function will always yield the same result if the memory is the same otherwise. In the miri engine that is. miri itself has mutable statics and env vars which complicate these things

[saleemjaffer]

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 Could you explain this a bit?

[RalfJung]

(FWIW the very related rust-lang/miri#417 made its way very high up my TODO list, I planned to tackle it on the week-end.)

But I actually also wonder what @oli-obk meant there.^^

[oli-obk]

Well, anythin like a panic, index out of bound or arithmetic overflow is allowed inside promoteds, as that's just a panic that can continue. If we encounter something unpromotable at evaluation time, we screwed up, but should not produce UB.

I believe we have cleaned up promotion well enough now that this isn't a problem anymore, so... Ignore that comment ;)

[RalfJung]

@saleemjaffer when you make a PR for an issue (like rust-lang/rust#60951), please mention that in the issue. (The backlinks GH auto-inserts do not help as they do not trigger email notifications.)

It is pure luck that I didn't have time last weekend to start working on the error messages, which would have had bad conflicts with your PR.

[saleemjaffer]

@RalfJung Yeah, will take care of this.

[RalfJung]

@saleemjaffer you mean you want to continue working on this issue?

That's great! However, some of it isn't just plain refactoring, it is about correctly classifying UB vs "unsupported"-style errors. And we'll probably have to review every single error site in librustc/mir/interpret and librustc_mir/interpret to make sure we do not missclassify anything.

So I think it would be worth talking here about how you intend to split InterpError into several sub-enums, before you spend a lot of time doing that.

(Or maybe I misinterpreted what you said. I have no idea what the "this" is you want to take care of.)

[RalfJung]

Cc rust-lang/rust#62927

[saleemjaffer]

By "this" I meant

@saleemjaffer when you make a PR for an issue (like rust-lang/rust#60951), please mention that in the issue. (The backlinks GH auto-inserts do not help as they do not trigger email notifications.)

I was also working on the same PanicMessage thing :D

But I can still continue to work on "UB vs "unsupported"-style errors".

[saleemjaffer]

@RalfJung

when you make a PR for an issue (like rust-lang/rust#60951), please mention that in the issue. (The backlinks GH auto-inserts do not help as they do not trigger email notifications.)

This should definitely go into the contributing guide in the rustdoc! What do you think?

[RalfJung]

But I can still continue to work on "UB vs "unsupported"-style errors".

Sure, if you want we can do this with mentoring. :)

My proposal for the enum would be

pub enum InterpError<'tcx, O> {
    /// The program panicked.
    Panic(PanicMessage<'tcx, O>),
    /// The program caused undefined behavior.
    UndefinedBehaviour(UndefinedBehaviourMessage<'tcx>),
    /// The program did something the interpreter does not support (some of these *might* be UB but the interpreter is not sure).
    Unsupported(UnsupportedMessage<'tcx>),
    /// The program was invalid (ill-typed, not sufficiently monomorphized, ...).
    InvalidProgram(InvalidProgramMessage<'tcx>),
    /// The program exhausted the interpreter's resources (stack/heap too big, execution takes too long, ..).
    ResourceExhaustion(ResourceExhaustionMessage<'tcx>),
}

Here, ResourceExhaustion is for the infinite-loop error (so ResourceExhaustionMessage is likely an alias for String), and InvalidProgram is for TooGeneric, ReferencedConstant and (if that is even still used anywhere) TypeckError. @oli-obk does that seem reasonable?

The interesting ones would be UndefinedBehaviourMessage and UnimplementedMessage, which share what is currently in InterpError. However, we likely do not need all those variants; having a variant is only necessary if (a) the error is ever caught (then we want to create it without having to allocate), or (b) if the error is raised in multiple places (and we want to keep the error message consistent). The other cases can just become a generic String variant (like the current Unimplemented). But remember that Miri also uses this error type!

OTOH, we do not have to arrive at the perfect final state in one step. If in doubt, an error should be considered "unsupported" rather than "UB"; then we can slowly go over the "unsupported" errors and figured out which of those are really UB. Oh and please base your work on top of rust-lang/rust#62927, or wait until that lands.

This should definitely go into the contributing guide in the rustdoc! What do you think?

That seems like a good idea!

[RalfJung]

Note that I am not entirely sure about that Message suffix. Error would make more sense for most of them, but PanicMessage already exists and PanicError is weird.

[saleemjaffer]

@RalfJung @oli-obk I have started to work refactoring InterpError here

[eddyb]

Couple ideas about the suffix:

• PanicDetails, UndefinedBehaviorDetails
• PanicInfo, UndefinedBehaviorInfo

[RalfJung]

I think I like info. It is also fairly short.

@oli-obk @saleemjaffer what do you think?

[oli-obk]

*Info sgtm

[saleemjaffer]

@RalfJung @oli-obk @eddyb I have moved some enum variants to where I felt they are appropriate. Haven't implemented anything though. Probably you guys can give your inputs here

[RalfJung]

One interesting kind of error that comes up in a few places is programs that use lang items or intrinsics the wrong way. What do we want to do for those? We could ICE, call then InvalidProgram, or call them Unsupported. I don't really have a strong preference as long as we are consistent about it.

So far we marked some of them as "ABI violation", which is confusing because I first thought it would be about our ABI checks on function calls. In CTFE, calling a non-const fn is "unsupported" right now, as is doing something that still needs an RFC. Maybe the former should be "invalid" as well? But by original intention for "invalid" was something that at least stable Rust can never cause except when programs get run that have not passed typeck, or things are not monomorphized, like const_prop sometimes does IIRC -- and one day stable Rust might be allowed to transmute function pointers around in CTFE and then it can trigger the "called non-const function" error.

[RalfJung]

Another question, @oli-obk, about this match here: can / should we make it exhaustive for InvalidProgram? The one variant it currently does not handle is ReferencedConstant. Should it handle that variant? If not, should that variant maybe move elsewhere?

[oli-obk]

Lang item or intrinsic abi or api misuse ICEs the compiler, so go right ahead with doing the same

ReferencedConstant could be moved elsewhere, but it's unclear whether all of these variants should be silent. There's some work in rust-lang/rust#63152 but it's entirely unclear to me how yo fix all that nicely

[RalfJung]

Lang item or intrinsic abi or api misuse ICEs the compiler, so go right ahead with doing the same

All right.

ReferencedConstant could be moved elsewhere, but it's unclear whether all of these variants should be silent. There's some work in rust-lang/rust#63152 but it's entirely unclear to me how yo fix all that nicely

I am not sure which "elsewhere" would fit though.
But I suppose for now I will wait how that issue gets resolved.

[RalfJung]

Actually, now that Miri supports panics properly, the "panic" variants of these errors do not actually make any sense any more. A panic not an "interpreter error condition". So I think we should get rid of it.

But see rust-lang/rust#66874 (comment) for why const_prop makes that non-trivial.

[RalfJung]

I think this can be closed now, the boxes are all checked and I am mostly happy with our error enum now. :)