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

@oli-obk

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

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)

@RalfJung

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

I like the enum split.

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.

@RalfJung

@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.

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

@RalfJung

@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).

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

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.

@RalfJung

@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?

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.

@TheDarkula

@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?

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),
}

That would be the other option.

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

Rollup merge of rust-lang#62969 - saleemjaffer:declutter_interperror,…

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.

@oli-obk

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?

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

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.

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.

Rollup merge of rust-lang#69839 - RalfJung:miri-error-cleanup, r=oli-obk

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

Navigation Menu

Repository navigation

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

24 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

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

Description

Activity

oli-obk commented on Aug 19, 2018

oli-obk commented on Aug 19, 2018

eddyb commented on Aug 20, 2018

RalfJung commented on Aug 21, 2018

eddyb commented on Aug 22, 2018

RalfJung commented on Aug 22, 2018

eddyb commented on Aug 22, 2018

oli-obk commented on Aug 22, 2018

RalfJung commented on Aug 25, 2018

TheDarkula commented on Sep 3, 2018

oli-obk commented on Sep 3, 2018

RalfJung commented on Sep 17, 2018

TheDarkula commented on Sep 17, 2018

RalfJung commented on Sep 17, 2018

TheDarkula commented on Sep 17, 2018

24 remaining items

RalfJung commented on Aug 3, 2019

RalfJung commented on Aug 3, 2019

oli-obk commented on Aug 3, 2019

RalfJung commented on Aug 3, 2019

RalfJung commented on Nov 30, 2019

RalfJung commented on Sep 12, 2020

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions