Automatically implement AsRepr and allow deriving FromRepr for fieldless enums

[illicitonion]

This auto-impl and derive are only allowed when all of the following are true:

1. The derive is on an enum.
2. The enum has an explicit repr (in an aim to show the deriver has
   considered what type this enum should be convertable into).
3. The repr is a fixed-size integer type (i.e. not C).
4. None of the enum's variants may hold data.
5. None of the enum's variants are declared as holding an empty tuple as
   data (because for enum E { Empty() }, E::Empty as isize converts
   the address of the function pointer, rather than the discriminant).
6. None of the enum's variants are declared as holding empty named
   values (i.e. enum E { Empty{} }) for consistency.

Currently this tends to be done using as, which has two flaws:

1. It will silently truncate. e.g. if you have a repr(u64) enum, and
   you write value as u8 (perhaps because the repr changed), you get
   no warning or error that truncation may occur.
2. You cannot use enums in code which is generic over From or Into.

Other types, such as primitive numeric types, support both as casts
and From/Into conversions, so making this trivial for primitive
enums too seems reasonable.

There are a number of crates which provide macros to provide this
implementation, as well as TryFrom implementations. The advantages of
using infallible conversions, rather than possibly-truncating ones feels
like it warrants a place in std.

[rust-highfive]

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

[illicitonion]

There is some discussion of a wider change, to start discouraging or deprecating as conversions, in RFC 3040 and RFC 3046, but this change felt small and standalone-useful enough to split off and PR.

I also wasn't sure if there was a way to make a derive macro not insta-stable, (particularly where the trait they derive is already in the prelude) - there are few enough derive macros that I'm not sure there's prior art for doing so...

[estebank]

I believe that @rust-lang/lang needs to chime in on this.

[Lokathor]

I don't think that this does anything new from a lang perspective. Extremely useful to automate, but nothing that you couldn't do before by yourself. It seems like purely the domain of libs and libs-impl to me.

[estebank]

It's fair that technically it falls under the purview of @rust-lang/libs, but this might have implications for things that t-lang might have discussed in the past.

[joshtriplett]

This seems entirely reasonable to me, and quite useful.

[joshtriplett]

I think this needs a derive_into feature gate. While we can't feature-gate trait implementations, we can feature-gate derives.

Assuming nobody on @rust-lang/libs objects, I think we should add this once it has a feature gate.

[m-ou-se]

I'm a bit worried about how generic the name Into is. I can imagine other use cases for derive(Into).

Another thing that is a bit confusing about the name is that you write derive(Into), but it actually implements From for an integer type.

[Mark-Simulacrum]

This is a little bike-shed-y, but it feels like Into may not be the best name for this derive. In particular, there are potentially other use cases we may want in the future for Into derives, though not necessarily on enums, and the name as-is is not particularly evocative of the behavior given (unlike the other std derives). Maybe something like "IntoDiscriminant" or similar may be preferable?

The disadvantage is that this doesn't match the trait name anymore, which is a departure from the norm, but I at least feel like Into may be confusing as a derive without some added context. (But maybe this is something people will just get used to).

Other than this concern I definitely think this is nice, though I admit to wanting the inverse (TryFrom<u32> for Foo) much more in some sense; that definitely is harder to add, though.

[m-ou-se]

Als note that this will probably break code using use derive_more::*; .. #[derive(Into)] .. even when #[unstable].

[joshtriplett]

I'm a bit worried about how generic the name Into is. I can imagine other use cases for derive(Into).

That's a reasonable point. I do think there's value in matching the name of the trait, but at the same time, I can imagine having other possible ways to derive From/Into that would conflict with this.

What if we don't put this in the prelude, and instead put it in a standard module, so that you can #[derive(std::enums::Into)] or similar? (Path subject to bikeshedding.) That would avoid conflicts with existing crates, as well as conflicts with future alternative derivations.

Another thing that is a bit confusing about the name is that you write derive(Into), but it actually implements From for an integer type.

I think it's reasonable to refer to Into here. derive(From) suggests we're implementing From on the enum type, but we're not; we're implementing From on integer types and targeting the enum type, which has the side effect of supplying an Into impl. I think it'd be more confusing to refer to From.

[m-ou-se]

I think it'd be more confusing to refer to From.

Sure. I'm absolutely not suggesting we call it derive(From). I'm just pointing out that derive(Into) would be the first derive we have that doesn't just implement a trait of the same name, and also the first derive that implements a trait for another type than the one it is applied to. I have no good suggestion for another name, but considering how the Into/From split already causes plenty of confusion, I'm not sure adding a derive(Into) that implements From is a great idea.

[Lokathor]

In terms of a name that is not just Into but also explains what you're driving, IntoRepr is explanatory, terse, and does fairly literally what it says it does.

[illicitonion]

Thanks for the feedback!

On naming, I've definitely struggled with the combination of:

1. All std derives derive exactly their named trait, whereas From definitely feels like the right thing to implement, whatever we call it, so no matter what we're doing something novel. But hey, that gives us freedom to be more novel, right? :)
2. As far as I know this is the first derive trait to have an (implicit) associated type?
3. Different folks may have different instincts for what name to look for - Into feels reasonable as in this case there is a natural type to convert into, but I agree that there may be different uses for it in the future we may want to avoid conflicting with. IntoRepr, IntoDiscriminant, IntoPrimitive all feel reasonable.

wanting the inverse (TryFrom<u32> for Foo) much more in some sense; that definitely is harder to add, though.

I would definitely like to add this too in the future, and perhaps we should work out the derive-names for both of these traits together?

Als note that this will probably break code using use derive_more::*; .. #[derive(Into)] .. even when #[unstable].

I did not realise this, but it does indeed: error[E0659]: Into is ambiguous (glob import vs any other name from outer scope during import/macro resolution). If we do end up sticking with this name, just not putting it in the prelude, so that it has an explicit path (either in a use or at the derive-site) seems reasonable

I think this needs a derive_into feature gate. While we can't feature-gate trait implementations, we can feature-gate derives.

Fantastic! I don't suppose you could point me at an example I can crib off, or some documentation I could read?

[m-ou-se]

Fantastic! I don't suppose you could point me at an example I can crib off, or some documentation I could read?

You probably only need to apply a #[unstable(feature = "...")] attribute to the pub macro item you added in library/core, but derive macros might work a bit differently. (You don't need to declare the feature name anywhere. Just using it an #[unstable] attribute makes it exist.)

[scottmcm]

IntoRepr, IntoDiscriminant, IntoPrimitive all feel reasonable.

What about potentially making one of those a "real" trait? Being able to .into_discriminant() or whatever without needing to know what the type actually is would be a nice thing to be able to do (like an opt-in DiscriminantKind) in addition to making the derive invocation clearer.

[joshtriplett]

Based on discussions in today's @rust-lang/lang meeting, and #91161 , we need to add a further restriction on AsRepr: we should not impl AsRepr for an enum that has a #[non_exhaustive] variant.

[illicitonion]

Good call @joshtriplett - I've implemented the non_exhaustive checks

[joshtriplett]

I think this is now ready for compiler to review. The semantics seem correct.

[jswrenn]

Is AsRepr implemented for fieldless-but-non-unit-like-enums-with-explicit-reprs? E.g.:

#[repr(u8)]
enum FieldlessExplicit {
    Tuple() = 1,
    Struct{} = 3,
    Unit = 9,
}

As-casting such enums is (currently) forbidden by #89234.

[illicitonion]

Is AsRepr implemented for fieldless-but-non-unit-like-enums-with-explicit-reprs? E.g.:

#[repr(u8)]
enum FieldlessExplicit {
    Tuple() = 1,
    Struct{} = 3,
    Unit = 9,
}

No, it's not implemented - the check is currently done by looking for whether any variants are non-unit, not whether they actually have fields.

[joshtriplett]

cc @jswrenn

[bors]

☔ The latest upstream changes (presumably #92915) made this pull request unmergeable. Please resolve the merge conflicts.

[pnkfelix]

I am not sure we want to land this without first resolving the T-lang questions surrounding our plans for enums going forward (work that @jswrenn has been driving a fair amount).

I'm going to mark this S-waiting-on-team (T-lang) for now.

[pnkfelix]

(I know @joshtriplett made statements above that might contradict my assertion that this needs T-lang input. but I want to circle back around with them first.)

[pnkfelix]

(also unassigned estebank because I'm not sure this belongs in their review queue.)

[bors]

☔ The latest upstream changes (presumably #93202) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #93028) made this pull request unmergeable. Please resolve the merge conflicts.

[pnkfelix]

(T-lang thinks this is currently waiting on some design work from @jswrenn ; I'm going to hack representing that info by making jswrenn the reviewer and marking this S-waiting-on-review.)

@rustbot label: +S-waiting-on-review -S-waiting-on-team
@rustbot assign @jswrenn

[JohnCSimon]

Ping from triage:
@illicitonion can you please address the merge conflicts?

[JohnCSimon]

@illicitonion
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you do please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive

[the8472]

@JohnCSimon maybe you should consider the last comments before closing? In #81642 (comment) it was said to be blocked on work of another rust-lang member. I assume the merge conflicts should be irrelevant to the state of the PR until that is resolved.

[JohnCSimon]

@the8472 - sorry, I missed that comment. :(
I was running through thirty other PRs.

[apiraino]

Hello, what's the current status w.r.t. this comment? thanks for an update!

cc @jswrenn @illicitonion

[jswrenn]

@illicitonion and I are going to meet in the very near future (early next week?) to discuss possibilities. In essence: I believe there's an edition-boundary opportunity to streamline the semantics of enums that complements the edition-boundary opportunity to fix/remove as-casting. We might also be able to make use of the existing core::mem::Discriminant trait.

[JohnCSimon]

@illicitonion any status on this?

[illicitonion]

@illicitonion any status on this?

Sorry for the delay in responding! @jswrenn and I met (some time ago!) and discussed plans here, and I suspect this PR isn't going to go anywhere, so I'll close it for now.

I do think that as-cast reform would be a great thing to work on in general - https://hackmd.io/i2RG5HzjQrGMCTEfIQjXGg and https://hackmd.io/FejspdTdSX-xaPrcFc5W9w are some collections of thoughts we discussed about possible directions for making as-casts and enum conversion in general more reasonable.