Mark enums with non-zero discriminant as non-zero

[petrochenkov]

cc rust-lang/rfcs#1230
r? @eddyb

[eddyb]

LGTM although I believe it could be simpler.

[eddyb]

You already have the range (see rustc_trans::adt for a way to determine if a value is in the range), why would this additional field be needed?

[petrochenkov]

enum E {
    A = -1,
    B = 1,
}

0 is in the range, but the enum is still non-zero.
(Also, non_zero: bool fits into previous padding so it doesn't increase the size of Layout)

[eddyb]

That's annoying. What I expect to eventually have would use 2 there, that's why I don't like non_zero: it's a special-case that makes sense mostly on pointers.
If you want to discuss the full niche optimization, I'd be down for it (IRC?).
If you don't want to change this implementation I'll r+ and take out this later (and/or switch to multi-ranges - LLVM actually supports them for !range).

[petrochenkov]

I didn't plan to implement the full optimization so far.

[eddyb]

Maybe make this match produce the Primitive type and do the return after the match?

[petrochenkov]

Updated.

[eddyb]

A few people hit this, I guess it got removed by accident. Thanks!

[jonas-schievink]

Note that #37177 already contains this (and is in the queue)

[petrochenkov]

#37202 contains it too :D
The sooner it lands the better.

[eddyb]

@bors r+

[bors]

📌 Commit 49e6b46 has been approved by eddyb

[Manishearth]

Btw, this caused some rather tricky issues in Servo (rust-lang/rust-bindgen#225).

IMO when changing reprs for things which may come from FFI (e.g. an enum) we should be more careful and have a cycle of warnings or a compiler note or something. Not sure how it would be implemented. (I agree that it's non ideal for C++ to abuse enums as bitflags but apparently it's a common pattern)

The only reason we could figure this out was because I knew how Rust's encoded enum debuginfo works, and I also knew that this PR had happened -- which is a rather high bar for being able to debug things like this. It's otherwise quite an opaque and nonsensical bug that feels like a compiler bug (in this case, setting foo = Some(thing) made it stay None)

cc @eddyb @nikomatsakis

[nikomatsakis]

Hmm. @Manishearth I've not read this PR, but I assume that the enums in question were not marked as #[repr(C)] or any such thing?

(In which case, was the "safe for C" lint not firing, or was it disabled?)

[Manishearth]

They were marked as repr(u8) which IIRC is equivalent?

[eddyb]

That's just setting the size of the enum, it's still UB to have a value other than the assigned ones.
EDIT: Not saying #[repr(C)] doesn't also have that UB property still, neither escape it.
EDIT2: Looks like gross misuse, nothing to see here.