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

Blanket impl for Real trait prevents parameterized implementations. #49

Closed
olson-sean-k opened this issue Mar 6, 2018 · 3 comments
Closed

Comments

@olson-sean-k
Copy link

I'm working on stabilizing a crate for constraints and total orderings of floating-point types. My crate integrates with num-traits, and has introduced subsets of the Float trait: Encoding, Infinite, Nan, and Real. The 0.2.* series of num-traits introduces a Real trait of its own, which I would prefer to use instead, but the blanket impl<T> Real for T where T: Float makes this difficult. See this issue.

I know this is a long shot, since this would likely involve breaking changes, but is there any chance this impl and the relationship between Real and Float could be reconsidered? Just a few thoughts:

  • Today, Real does not really behave as a subset given the blanket implementation. Users either implement Real xor Float, rather than implementing Real and any additional traits to satisfy Float. That relationship seems a bit backwards to me.
  • The blanket implementation makes it difficult to generically implement Real and Float. In my crate, this is a problem, because I implement both based on constraints on a wrapped type T. Attempting an implementation of Float will always clash with an implementation of Real regardless of the constraints. This is the difficulty I'm referencing in the title of this issue.

My guess is that the second point will almost always be a problem for parameterized types (i.e., wrappers) that may want to implement Float and/or Real.

The most obvious workaround for my crate is to remove the parameterized impls for Real and Float and duplicate a lot of code to implement them for the concrete type definitions that the crate re-exports. I can reduce duplication with a macro, but it makes the code more fragile, because type definitions that should pick up such implementations based on the input types alone would no longer get them implicitly.

Thanks for taking a look at this. Thoughts?

@cuviper
Copy link
Member

cuviper commented Mar 6, 2018

FWIW, Real was added in 0.1.42, and it was done this way to avoid breaking changes to Float. Actually 0.2 didn't make breaking API changes either, only in feature definitions for no_std.

I think the blanket implementation makes sense in context, but it is unfortunate that it prevents you from being generic. Now we have FloatCore too, which couldn't have a blanket impl, making the sub-typing relationship even fuzzier.

If we were to redesign this (in a breaking change), it would probably be something like RealCore and FloatCore: RealCore, then with std adding Real: RealCore and Float: Real + FloatCore. Implementors could deal with methods just once in their particular trait, and I think there wouldn't need to be any blanket implementations.

The most obvious workaround for my crate is to remove the parameterized impls for Real and Float and duplicate a lot of code to implement them for the concrete type definitions that the crate re-exports.

I think you could keep your parameterized impl for Float, no? Then Real will still have to be manually implemented for non-Float types, unfortunately, but I expect these are less common.

@olson-sean-k
Copy link
Author

FWIW, Real was added in 0.1.42

Ah, I see. I discovered the trait when I attempted to upgrade to 0.2.0 and somehow I got it in my head that that version introduced the trait. My mistake.

If we were to redesign this (in a breaking change), it would probably be something like ... and I think there wouldn't need to be any blanket implementations.

That sounds more ideal to me. Are there any plans to move towards a model like this in a future release?

I think you could keep your parameterized impl for Float, no?

Whoops, you're right. I would only need one-off implementations of Real; Float implementations would be unaffected.

@cuviper
Copy link
Member

cuviper commented Mar 6, 2018

Are there any plans to move towards a model like this in a future release?

Sure, there are informal plans, right here in this discussion. 😉

I've added this issue to #47. I fear it will be a nightmare to ever actually making breaking changes, but we can at least track ideas. In the meantime I'm going to close this issue as postponed, but feel free to continue the discussion if you think of more.

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

No branches or pull requests

2 participants