kvark / mint

Math Interoperability Types
MIT License
252 stars 20 forks source link

Implement `IntoMint` trait #68

Closed LPGhatguy closed 2 years ago

LPGhatguy commented 2 years ago

Closes #59.

This PR implements a version of the proposed IntoMint trait. It is not intended to be merged, at least yet, but instead used as a discussion point and a practical implementation that people can experiment with.

I am using this branch to try to find a solution to LPGhatguy/crevice#34. I thought it'd be useful to share my implementation while I do that.

I created a corresponding branch of glam which implements the trait.

Design Decisions

I added an extra trait bound and default implementation of the into_mint method. This makes it very easy to add to existing types and gives them a free #[inline] annotation.

I also implemented the trait for all numeric primitives. This seems kind of silly since these types aren't technically part of mint, but I think it simplifies code that is generic over "a type that can be turned into a generic math library type", like in Crevice.

kvark commented 2 years ago

So what this gives you extra is an associated type. And I can see how crevice benefits from it. Good thing is, the trait doesn't have to live in this crate. I'm really trying to keep its scope minimal, since it needs to be exposed in all of the APIs, and any complexity here will cost everybody. So how do you feel about having the trait living in crevice for now? If other people reach out with the same feature request, we'll come back to this and reconsider moving it into mint itself.

Ralith commented 2 years ago

it needs to be exposed in all of the APIs, and any complexity here will cost everybody.

I don't think there's anything that makes downstream code need to implement this trait, it's just extra convenient for users if they do.

So how do you feel about having the trait living in crevice for now?

I think it'd be a much harder sell for all the downstream crates to add yet another optional dep than to marginally expand the scope of their existing mint impls.

If other people reach out with the same feature request

There's been a number of people expressing interest in https://github.com/kvark/mint/pull/45 and https://github.com/kvark/mint/issues/59. What's the threshold?

LPGhatguy commented 2 years ago

I do want to clarify that I don't know if this solves our use case in Crevice. There's still a bit to solve before I can comfortably push for this much more for my own use case.

any complexity here will cost everybody.

The added complexity for math library authors is essentially zero. Here's the diff of adding IntoMint implementations in glam: https://github.com/LPGhatguy/glam-rs/commit/46f651188bc346e41dfbc93efe4efed53deeaf5b

So how do you feel about having the trait living in crevice for now?

I cannot expect math libraries to depend on Crevice. I think I would probably add direct dependencies to math libraries in that case and implement traits on their types. Given the semver churn in all extant math libraries, that's not very desirable. Solving that problem feels like what mint should do.

kvark commented 2 years ago

@Ralith so you are saying it would help #45, because you'd write x.into_mint().into() (which is closer to x.convert() than what we have now). In #59, at least @cwfitzgerald expressed a good need for this. So that's something, let's see how we can get it.

If we add this trait in a patch version, we are going to have no way to enforce that math libraries support it. Some users would start relying on it, and then face an issue with some of the libraries being incompatible. There would have to be a coordinated effort to bring the changes to match libraries, and to publish their updates.

Is that how you see things processing? Or where you thinking about a breaking change?

LPGhatguy commented 2 years ago

I've figured out how to make this trait work for what I wanted to do in Crevice!

If we add this trait in a patch version, we are going to have no way to enforce that math libraries support it.

There is already no way to ensure that a math library provides conversions for all Mint types. Introducing IntoMint is not much different than introducing a new type.

I'm interested in bringing up PRs to all the major math libraries to have them support IntoMint. I have branches of both cgmath and glam that do, and it's pretty straightforward.

kvark commented 2 years ago

There is already no way to ensure that a math library provides conversions for all Mint types. Introducing IntoMint is not much different than introducing a new type.

Right, and we don't add types. We have the minimum we need.

I'm interested in bringing up PRs to all the major math libraries to have them support IntoMint. I have branches of both cgmath and glam that do, and it's pretty straightforward.

That's very helpful, thank you! In this case, let's proceed.

Ralith commented 2 years ago

@Ralith so you are saying it would help #45, because you'd write x.into_mint().into() (which is closer to x.convert() than what we have now).

Right, algebraically it's basically the same thing, and downstream users can easily write a oneliner to make it more concise if desired, or convert could be added as a provided method, backwards-compatibly.

Are there any semantic differences between inheriting from Into vs. constraining the associated type with From?

LPGhatguy commented 2 years ago

Are there any semantic differences between inheriting from Into vs. constraining the associated type with From?

I figure that Into is strictly more general than From even though it's unlikely to matter in practice here.

Otherwise, I think these two implementations of IntoMint should be identical due to the relationship that From and Into have.

trait IntoMint: Into<Self::MintType> {
    type MintType;

    fn into_mint(self) -> Self::MintType;
}
trait IntoMint {
    type MintType: From<Self>;

    fn into_mint(self) -> Self::MintType;
}
kvark commented 2 years ago

Could you bump the patch version and add a changelog entry, please? @Ralith I assume you are good with this change?

cwfitzgerald commented 2 years ago

I'm so glad this is getting merged in, this looks like exactly what I need. 👍🏻 from me.

Ralith commented 2 years ago

One minor thought does occur to me: should we drop the into_mint method entirely? The important thing is the associated type; once you have that, mint could instead have

fn from<T: IntoMint>(x: T) -> T::MintType {
    x.into()
}

which could then be unambiguously used like

let foo = mint::from(x);

This is a little bit cheeky, but has the advantage of not requiring the caller to import a trait, and removes any possibility of differing behavior between the trait and Into. Implementers would be unaffected.

I don't have particularly strong feelings here; just a thought.

LPGhatguy commented 2 years ago

I think we should also add docs for this before we merge.

I like the idea of dropping the into_mint method and I'll see if I can get away with that in Crevice's use case in practice.

cwfitzgerald commented 2 years ago

It's pretty cheeky, but I like it. I don't want to bikeshed a possible convert method this issue, but that method of from usage would fit well with convert.

kvark commented 2 years ago

I'm sold on avoiding duplicate implementations. Thanks for bringing up this idea!

kvark commented 2 years ago

TODO

LPGhatguy commented 2 years ago

Docs, changelog, and version bump is up. Let me know what you think!

kvark commented 2 years ago

This is published now. Thank you @LPGhatguy and everyone contributing to the discussion!