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

Implement derives for generic wrapper types #37

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

Conversation

oli-cosmian
Copy link

fixes #19

Copy link
Member

@cuviper cuviper left a comment

Choose a reason for hiding this comment

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

This is cool -- Generics::split_for_impl makes this a lot easier than I thought it would be.

Could you rebase for the conflicts from #35? Mostly just need to change _num_traits to a dynamic #import.

@oli-cosmian
Copy link
Author

Rebased

Copy link
Member

@cuviper cuviper left a comment

Choose a reason for hiding this comment

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

Just missed a couple spots.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@oli-cosmian
Copy link
Author

Just missed a couple spots.

oops, resolved and grepped to see if there are any others (there weren't).

@oli-cosmian
Copy link
Author

oli-cosmian commented Jun 30, 2020

Oh. I just realized that

#[derive(
    Debug,
    Clone,
    Copy,
    PartialEq,
    PartialOrd,
    ToPrimitive,
    FromPrimitive,
    NumOps,
    NumCast,
    One,
    Zero,
    Num,
)]
struct MyThing<T>(Wrapping<T>);

does not work, because there's no T: Num bound on the Num impl, causing rustc to fail to infer that Wrapping<T>: Num holds. If we add T: Num bounds, that makes struct MyThing<T, U>(SomeTypeImplingNumEvenIfTDoesNot<T>); not work anymore.

)]
struct MyThing<T>(Wrapping<T>);

impl<T> PartialEq for MyThing<T> where Wrapping<T>: PartialEq {
Copy link
Author

Choose a reason for hiding this comment

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

Since Num requires PartialEq, and the libstd derive causes PartialEq to get a T: PartialEq bound instead of a Wrapping<T>: PartialEq bound, this workaround is required to use the new num derives from this PR with such types.

Copy link
Member

Choose a reason for hiding this comment

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

Is there a meaningful difference? The std implementation looks like this:

impl<T> PartialEq<Wrapping<T>> for Wrapping<T>
where
    T: PartialEq<T>,

You can't impl PartialEq for Wrapping<LocalType>, so it seem like T: PartialEq and Wrapping<T>: PartialEq should be equivalent.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure why there's a difference, but there definitely is one. I'll look into it

@cuviper
Copy link
Member

cuviper commented Jul 5, 2020

because there's no T: Num bound on the Num impl, causing rustc to fail to infer that Wrapping<T>: Num holds. If we add T: Num bounds, that makes struct MyThing<T, U>(SomeTypeImplingNumEvenIfTDoesNot<T>); not work anymore.

Hmm, now it seems like we're in that gray area of rust-lang/rust#26925, where it's not really obvious whether you should want bounds on the generic parameters or on the structural types. By adding #inner_ty: Trait, we're leaking that private type in a way that may not be desirable. I think we should probably only constrain the generic parameters, as the compiler does.

So far we've avoided this question because the newtype derivations are unconditional.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Derive for newtypes with generics
2 participants