Skip to content

RFC: enable derive(From) for single-field structs #3809

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Kobzol
Copy link
Contributor

@Kobzol Kobzol commented May 6, 2025

Previously discussed as Pre-RFC on IRLO.

Rendered

@jieyouxu jieyouxu added the T-lang Relevant to the language team, which will review and decide on the RFC. label May 6, 2025
@clarfonthey
Copy link

Fully in support of this. A few things that probably should be elaborated:

  • Generic single fields
  • Transparent structs with extra zero-size fields like PhantomData

The transparent case, IMHO, is probably best left as a future extension, but it's an interesting case where Default impls on the extra fields would not be needed.

@Kobzol
Copy link
Contributor Author

Kobzol commented May 6, 2025

Fully in support of this. A few things that probably should be elaborated:

  • Generic single fields
  • Transparent structs with extra zero-size fields like PhantomData

The transparent case, IMHO, is probably best left as a future extension, but it's an interesting case where Default impls on the extra fields would not be needed.

PhantomData implements Default unconditionally for any T, so that would be fine. I agree it's best to leave it for "later" though.

Good point with the generics, I'll add a mention to the RFC.

@kennytm
Copy link
Member

kennytm commented May 7, 2025

"For later" - perhaps it could recognize #3681 fields and automatically skip them as well.

#[derive(From)]
struct Foo {
    a: usize, // no #[from] needed, because all other fields are explicitly default'ed
    b: ZstTag = ZstTag,
    c: &'static str = "localhost",
}

// generates

impl From<usize> for Foo {
    fn from(a: usize) -> Self {
        Self { a, .. }
    }
}

OTOH we may still want the #[from] in the above case for uniformity if we want to support #[derive(From)] on all fields being defaulted

#[derive(From, Default)]
struct Foo2 {
    #[from] // <-- perhaps still want this
    a: u128 = 1,
    b: u16 = 8080,
}

Co-authored-by: Jake Goulding <[email protected]>
@nielsle
Copy link

nielsle commented May 9, 2025

The crate named derive-new creates a new constructor, and it has several fancy options.

But perhaps this functionality belongs in a third party crate rather than the standard library.


We could make `#[derive(From)]` generate both directions, but that would make it impossible to only ask for the "basic" `From` direction without some additional syntax.

A better alternative might be to support generating the other direction in the future through something like `#[derive(Into)]`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be worth noting here that impl From<Newtype> for Inner and impl Into<Inner> for Newtype have slightly different semantics. Using derive(Into) to mean impl From could be confusing for generic types where there is a coherence issue, even if it does imply an Into impl.

(I've had similar issues with the derive_more version.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the derive-macro's name does not necessarily correspond to the impl'ed trait name anymore since #3621 (currently named #[derive(CoercePointee)], which actually does impl CoerceUnsized + DispatchFromDyn.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The #[derive(Into)] example is kind of hand-waving, I'm not sure if it's actually a good idea. I would like to avoid doing that in this RFC though, as that sounds like a separate can of worms, I explicitly tried to keep this RFC as simple as possible.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the derive-macro's name does not necessarily correspond to the impl'ed trait name anymore since #3621 (currently named #[derive(CoercePointee)], which actually does impl CoerceUnsized + DispatchFromDyn.)

Understood! But my comment was more about the confusing semantics, than the exact name of the impl'd trait.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, for the impl From<NewType> for Inner case, I think I would prefer something like impl_from!(Newtype -> Inner) or a more generic impl_from!(Newtype -> Inner = |source| source.0) or impl_from!(Newtype -> Inner = self.0) (restricted to using fields of NewType)

Although it is a little annoying, to have to repeat the type of Inner.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But my comment was more about the confusing semantics

I don't think there is any name more appropriate than #[derive(Into)] especially if this RFC is using #[derive(From)]. The final effect you get is still having an impl Into<Inner> for Self effectively, through the intermediate impl From<Self> for Inner + the blanket impl.

See JelteF/derive_more#13 for a brief discussion how derive_more still chooses to name it #[derive(Into)].

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd love to have a derive in the other direction, but full support for making that future work and not part of this RFC.

@Kobzol
Copy link
Contributor Author

Kobzol commented May 10, 2025

The crate named derive-new creates a new constructor, and it has several fancy options.

But perhaps this functionality belongs in a third party crate rather than the standard library.

While new constructors are pretty common, I would say that it's too opinionated for it to be included in the stdlib, as it's not a standard (library) trait like From is. In any case, that would be a separate RFC :)

Co-authored-by: teor <[email protected]>
@joshtriplett joshtriplett added the I-lang-nominated Indicates that an issue has been nominated for prioritizing at the next lang team meeting. label May 28, 2025
@joshtriplett
Copy link
Member

I think this proposal makes sense as written, and it's simple and straightforward.

Should we allow derive(From) on single-field structs, to impl From<FieldType> for Struct?

@rfcbot merge

@rfcbot
Copy link
Collaborator

rfcbot commented May 28, 2025

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. labels May 28, 2025
@traviscross traviscross added the P-lang-drag-2 Lang team prioritization drag level 2. label May 28, 2025
@matthieu-m
Copy link

A better alternative might be to support generating the other direction in the future through something like #[derive(Into)].

I very much encourage distinct syntax for distinct directions.

In particular, as noted in the Email(String) example, newtype wrappers are regularly used to enforce invariants, in which case the #[derive(From)] implementation is outright wrong.

On the other hand, going back to the inner type should never violate any invariant, and therefore #[derive(Into)] makes perfect sense for Email.

@scottmcm
Copy link
Member

Unsure: is this really lang, @joshtriplett? It's an impl that could be done in a stable proc macro crate, AFAICT, which to me says that it's entirely libs-api (even if in actual implementation it'd be done inside the compiler today).


Personally I'm not concerned with "well this From might be wrong", because correctness is up to the caller to do properly (like how today derive(Clone) might be wrong if you're holding pointers or how derive(Hash) might be wrong if you implemented PartialEq manually.) I think in a hypothetical future with unsafe fields then perhaps this should fail, but that's not something this RFC would need to mention because there's no accepted RFC for that yet. (Though if it wanted to discuss that as future possibilities that would be nice-to-have.)

@traviscross
Copy link
Contributor

traviscross commented Jun 12, 2025

As a comparable, we handled RFC #3107 (#[default]) as a dual lang/libs-api FCP. Probably I think that makes sense here as well (e.g., it maybe leads to #[from]). This perhaps touches on general "flavor of the language" matters, and also raises for us questions about whether we might like to solve the motivation here in some other language-directed way.

@rfcbot fcp cancel
@rustbot labels +T-libs-api

@rfcbot
Copy link
Collaborator

rfcbot commented Jun 12, 2025

@traviscross proposal cancelled.

@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Jun 12, 2025
@rfcbot rfcbot removed proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. labels Jun 12, 2025
@traviscross
Copy link
Contributor

@rfcbot fcp merge

(And I'll recheck the box for Josh.)

@rfcbot
Copy link
Collaborator

rfcbot commented Jun 12, 2025

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. labels Jun 12, 2025
@jhpratt
Copy link
Member

jhpratt commented Jun 12, 2025

It's been a while, but I believe the reason for the two-team checkboxes back then was that no built-in derive had a helper attribute, so some precedent needed to be set. Proc macros had the ability but built-in macros did not at the time (ironically).

Personally I view this as solely T-libs-api territory given that the language side was settled in the previous RFC (in that helper attributes are permitted for built-in derives). Take that as you will; I'm not on any teams 🤷

@traviscross
Copy link
Contributor

Probably I should have added, "...and it seems maybe more straightforward to just do a proposed dual FCP rather than delving deeply into the jurisdictional question."

@clarfonthey
Copy link

Considering how straightforward and supported this RFC is, it probably would take longer to litigate which teams should review the RFC compared to just asking both teams to check their boxes.

Enabling this would make one more intuitive use-case in the language "just work", and would reduce boilerplate that Rust users either write over and over again or for which they have to use macros or external crates.

## Newtype pattern
As a concrete use-case, `#[derive(From)]` is particularly useful in combination with the very popular [newtype pattern](https://doc.rust-lang.org/rust-by-example/generics/new_types.html). In this pattern, an inner type is wrapped in a new type (hence the name), typically a tuple struct, to semantically make it a separate concept in the type system and thus make it harder to mix unrelated types by accident. For example, we can wrap a number to represent things like `Priority(i32)`, `PullRequestNumber(u32)` or `TcpPort(u16)`.
Copy link
Member

@RalfJung RalfJung Jun 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a pretty odd first motivation to list, since by having a From instance, the newtype does not actually represent an independent concept any more. In particular, if there are any invariants on the newtype, a From instance must not be used. And even if the goal is just "separation of concepts", a From instance seriously subverts this by making it easy to accidentally convert the field type to the newtype without even realizing that one is crossing a concept barrier.

Given that newtpyes with invariants are common, I think the text here at least needs to be more explicit that it is only talking about newtypes where the field is effectively public anyway.

Deriving Into would be a lot more useful for newypes, IMO...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that newtypes have several benefits that gradually provide more guarantees.

The first one (and in my view, the primary one, which is provided by all uses of a newtype) is to introduce a new entity in the type system. That's literally what the pattern says - newtype - introduce a new type. This, on its own, is incredibly useful, to avoid mixing unrelated things. I use it all the time, e.g. TaskId, WorkerId, UserId, all distinct types that represent all values of e.g. a u32. It's perfectly fine to implement From for these newtypes. So I think that the derive is very useful for this use-case.

Then, as an additional invariant, you can say that your newtype supports only a subset of values of the inner field. In that case, you of course shouldn't implement From, but have a custom fallible constructor.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first one (and in my view, the primary one, which is provided by all uses of a newtype) is to introduce a new entity in the type system. That's literally what the pattern says - newtype - introduce a new type. This, on its own, is incredibly useful, to avoid mixing unrelated things. I use it all the time, e.g. TaskId, WorkerId, UserId, all distinct types that represent all values of e.g. a u32.

I'm with you until here.

It's perfectly fine to implement From for these newtypes.

You lost me here. This seems to negate a large chunk of the benefits of making this a new type. Now it is very easy to take a random integer from $somewhere and turn it into a TaskId without even realizing that is what happens. By adding a From impl, you lose the enforced abstraction, and by adding impl Into<TaskId> functions you've lost nearly all benefits of this pattern.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, it turns out the newtypes in the compiler do implement From:
https://doc.rust-lang.org/nightly/nightly-rustc/rustc_abi/struct.FieldIdx.html

That is quite surprising, I wonder why that was done.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So maybe I am the odd one out with my strict use of newtypes? In many cases this seems to be used as Newtype::from(...), which I am entirely fine with, but I did find a few foo.into() relying on this as well and that's not something I would do in my codebases.

FWIW, Miri's newtype macro does not add a From, precisely because I never even considered that to be a reasonable thing to do with newtypes.^^

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. So you're fine with having a constructor like Newtype::new or Newtype::from, but you don't like .into(), got it.

Yes, exactly. :) I would make from an inherent method of the type, rather than getting it via impl From, but then one can only convert from a single type so one often ends up with from_foo and from_bar which is somewhat annoying... sometimes one can get away with from(x: impl Into<Something>), but that does not always work.

Copy link

@teohhanhui teohhanhui Jun 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a user, why would I not want .into()? It is good for ergonomics. Unless your goal is to annoy the callers as much as possible into using your preferred calling style.

The API promise is that if there's impl From<A> for B, then B can be constructed from A infallibly. Why should the callee get to dictate: you must write B::from(a), but not let b: B = a.into()? It seems like overreach. (And if that's really your goal in a project, it can probably still be enforced via clippy lints, right?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a user, why would I not want .into()?

I've explained this above. In my view, it is "good for ergonomics" the same way not having a type system is "good for ergonomics" (that's way more extreme, of course, but you get the point).

Unless your goal is to annoy

You are not commenting in good faith. Please behave better than that.

Copy link

@teohhanhui teohhanhui Jun 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my view, it is "good for ergonomics" the same way not having a type system is "good for ergonomics" (that's way more extreme, of course, but you get the point).

I think what you're arguing for is akin to "type inference considered harmful", which is what Into is about, right?

You are not commenting in good faith. Please behave better than that.

That's being taken out of context by cutting off half of the sentence.

Copy link
Member

@RalfJung RalfJung Jun 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I love type inference. I also love abstractions. And having var.into() convert a u32 into an Idx silently breaks the abstraction since it is not explicit that I am creating an Idx here.

I explained this above already. I won't reply further unless you actually add something new, or ask a question that has not yet been answered.

Comment on lines 54 to 55
### Simplifying test code
Apart from the `From` trait being generally useful in various situations, it is especially handy in tests, where various fixture functions can simply receive `T: Into<Newtype>` to avoid test code having to use `.into()` or a newtype struct constructor everywhere.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If an API does this I question the use of a newtype makes any sense. The entire point is to avoid accidental mixups, but then these kinds of functions completely subvert that point.

Are there real-world examples of newtypes doing this?

Copy link
Contributor Author

@Kobzol Kobzol Jun 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is nothing implicit about this. I still need to call .into() to convert u32 to the newtype. Even in generic functions taking Into, I can't pass a value of NewtypeB by accident. For that, the function would need to take something like X: Into<u32> and then you'd need to call .into().into() on that to "accidentally" convert from NewtypeB to NewtypeA.

One more point about implicitness, implementing From for a newtype of course makes it a bit easier to go from the inner type to the newtype (it's still not implicit though! and I explicitly mentioned this being usedbin test code, where I want to avoid boilerplate). But it doesn't make it easier to go from NewtypeA to NewtypeB, which I see as the main important thing. If something is u32, it's not very "type safe" anyway.

A real-world example are essentially all helper functions that I write in my tests :D

Copy link
Member

@RalfJung RalfJung Jun 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is fully implicit: foo.do_task(13) will just work if do_task takes an Impl<TaskId> and there is a derived From.

I don't think test code is the best argument for an RFC. We shouldn't optimize for quick-and-dirty throw-away code as we often see it in a test suite.

you'd need to call .into().into() on that to "accidentally" convert from NewtypeB to NewtypeA.

That seems entirely realistic, I've thrown into() into my code until it works when dealing with str/String/OsString/PathBuf/... and the compiler can even recommend adding .into() which tools can then auto-apply.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked the compiler and could not find a use of impl Into for a newtype like this. Phew. ;)

I admit I overlooked the part where it said "in tests" here when I first read the RFC. Still, I don't think the RFC should be arguing for this pattern. Most of the time, and certainly in the public API of a crate, I'd consider this an anti-pattern. (I haven't seen your test code, I could imagine that it is sufficiently local and otherwise explicit that the benefits outweigh the downsides.) Anti-patterns don't make for great motivation for new features.

But it doesn't make it easier to go from NewtypeA to NewtypeB, which I see as the main important thing.

If NewtypeA implements Into<u32> (which I consider to be entirely reasonable), then a.into() could now be passed to an impl Into<NewtyeB> function and the conversion would be very implicit. Or does that break due to inference variable ambiguity?

If something is u32, it's not very "type safe" anyway.

Agreed. That's why it should not be possible to call do_task(some_u32).

@RalfJung
Copy link
Member

RalfJung commented Jun 12, 2025

Personally I'm not concerned with "well this From might be wrong", because correctness is up to the caller to do properly

Sure, but I think the RFC needs to make a sufficiently clear argument that this is not "wrong most of the time". At least in the way I have encountered newtypes, I think it is wrong most of the time. Most of the time, in my experience, one specifically does not want implicit conversions from the inner type to the newtype. As-is, the RFC doesn't even acknowledge that this might ever be wrong, which is concerning.

The one exception I can think of is when the newtype is solely added to overcome the orphan rule, but one doesn't actually care about introducing a separate concept. But that usecase is not even mentioned in the RFC. Instead, the RFC mentions "introducing a separate concept" and then goes on to describe an API design that makes is super easy to accidentally mix up the new concept with its field type.

@Kobzol
Copy link
Contributor Author

Kobzol commented Jun 12, 2025

Depends on the use-case, I guess, most of the newtypes I create implement From, and they can hold all values of the inner type.

As a more general point, this "just" makes it easier to implement From. It can be used on anything, not just the newtype pattern specifically, and we can't ofc guarantee that the user does the right thing. It's just a syntactical shortcut to generate a boilerplate implementation of From (and I hope in the future also AsRef, Deref, Iterator, etc., with additional RFCs, of course).

@RalfJung
Copy link
Member

As a more general point, this "just" makes it easier to implement From. It can be used on anything, not just the newtype pattern specifically, and we can't ofc guarantee that the user does the right thing. It's just a syntactical shortcut to generate a boilerplate implementation of From

Yeah, I get that. But I find it concerning that the one concrete motivation given in the RFC is a big red flag IMO -- I'd not accept such code in a codebase I maintain, since I consider it as subverting the newtype benefits.

I can't think of any From instance I ever wrote that would be aided by this derive (unlike for many of the other traits you mention). Maybe I am using the language in a weird way, it's obviously quite hard to say whether your patterns or mine are more representative.

@RalfJung
Copy link
Member

To be clear, I'm not very opposed to the feature, I just think the RFC doesn't do a good job arguing for it, and it fails to acknowledge the downsides.

@Kobzol
Copy link
Contributor Author

Kobzol commented Jun 12, 2025

I even included some statistics in the RFC on how common this is, so that shows that it is being done in the ecosystem. But I don't think that matters very much for this RFC, tbh. The true downsides of this feature could be things like forward (in)compatibility or interaction with other language features.

The fact whether actually implementing From for a given type is a good or a bad idea shouldn't be decided by the compiler, that's up to the user. All this RFC proposes is to make it easier to implement From, that's it. I included the newtype to sufficiently motivate the use-case of "there are situations where you have exactly one field and From is useful". Since I implement From for most of my newtypes, and found many usages of this in the ecosystem, I considered it to be a valid use-case, but I don't think it's load-bearing for this RFC. Note that it is named "enable derive(From) for single-field structs", not "enable derive(From) for newtypes".

@Kobzol
Copy link
Contributor Author

Kobzol commented Jun 12, 2025

Removed the motivation for V: Into<Newtype> for tests (I agree that it seems "too dirty" without seeing the context where it does help to remove boilerplate), and reworded a paragraph about when it makes or doesn't make sense to implement From.

@RalfJung
Copy link
Member

Thanks, that helps. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. I-lang-nominated Indicates that an issue has been nominated for prioritizing at the next lang team meeting. P-lang-drag-2 Lang team prioritization drag level 2. proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. T-lang Relevant to the language team, which will review and decide on the RFC. T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.