Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatically implement AsRepr and allow deriving FromRepr for fieldless enums #81642

Closed

Conversation

illicitonion
Copy link

@illicitonion illicitonion commented Feb 1, 2021

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
Copy link
Collaborator

r? @estebank

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 1, 2021
@illicitonion
Copy link
Author

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

@rust-log-analyzer

This comment has been minimized.

@estebank
Copy link
Contributor

estebank commented Feb 2, 2021

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

@estebank estebank added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Feb 2, 2021
@Lokathor
Copy link
Contributor

Lokathor commented Feb 3, 2021

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
Copy link
Contributor

estebank commented Feb 3, 2021

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.

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 23, 2021
@crlf0710 crlf0710 added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Mar 12, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 29, 2021
@Dylan-DPC-zz Dylan-DPC-zz added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Mar 29, 2021
@crlf0710 crlf0710 added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Apr 17, 2021
@joshtriplett
Copy link
Member

This seems entirely reasonable to me, and quite useful.

@joshtriplett
Copy link
Member

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
Copy link
Member

m-ou-se commented Apr 21, 2021

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
Copy link
Member

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
Copy link
Member

m-ou-se commented Apr 21, 2021

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

@joshtriplett
Copy link
Member

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
Copy link
Member

m-ou-se commented Apr 21, 2021

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Member

m-ou-se commented Apr 21, 2021

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
Copy link
Member

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.

@bors
Copy link
Contributor

bors commented Jan 24, 2022

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

@yaahc yaahc added I-lang-nominated Nominated for discussion during a lang team meeting. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Jan 26, 2022
@camelid camelid added the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Jan 29, 2022
@pnkfelix
Copy link
Member

(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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Feb 22, 2022
@Mark-Simulacrum Mark-Simulacrum removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Feb 22, 2022
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 20, 2022
@JohnCSimon
Copy link
Member

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

@Dylan-DPC Dylan-DPC added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 25, 2022
@JohnCSimon
Copy link
Member

@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

@JohnCSimon JohnCSimon closed this May 22, 2022
@rustbot rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label May 22, 2022
@the8472
Copy link
Member

the8472 commented May 22, 2022

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

@the8472 the8472 reopened this May 22, 2022
@JohnCSimon
Copy link
Member

JohnCSimon commented May 22, 2022

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

@the8472 the8472 removed the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label May 22, 2022
@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 19, 2022
@Dylan-DPC Dylan-DPC added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 20, 2022
@apiraino
Copy link
Contributor

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

cc @jswrenn @illicitonion

@jswrenn
Copy link
Member

jswrenn commented Jul 29, 2022

@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
Copy link
Member

@illicitonion any status on this?

@illicitonion
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. S-blocked Status: Blocked on something else such as an RFC or other implementation work. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.