Improve the ergonomics of using strongly typed Lengths

[nical]

This PR renames all foo_typed into get_foo which I find to be nicer, and Introduces the ValueOrLength trait which makes it possible to for functions to take either lengths or scalar values directly. Here is the example from the doc:

use euclid::{Length, TypedVector2D};
struct WorldSpace;
struct ScreenSpace;
type WorldVec2 = TypedVector2D<f32, WorldSpace>;
type WorldLength = Length<f32, WorldSpace>;
type ScreenLength = Length<f32, ScreenSpace>;
// Convenient synatx:
let v1 = WorldVec2::new(1.0, 2.0);
// Statically checked synatx:
let v2 = WorldVec2::new(WorldLength::new(1.0), WorldLength::new(2.0));
// This would give a compile time error because units do not match:
// let not_good = WorldVec2::new(ScreenLength:new(1.0), ScreenLength::new(2.0));

---

This change is 

[nical]

I am quite happy about the ValueOrLength trick, as it provides the same ergonomics in function parameters for scalar values and lengths.

It is a breaking change, because this pattern confuses the type inference:

fn foo<T: Zero, U>() -> TypedVector2D<T, U> {
  TypedVector2D::new(Zero::zero(), Zero::zero()); // compile error, can't infer type of teh parameter.
}

However the same code can be written as:

fn foo<T: Zero, U>() -> TypedVector2D<T, U> {
  TypedVector2D::new(T::zero(), T::zero());
}

Which in my opinion is a better practice (easier to understand at a glance that zero returns a T).

r? @kvark

[kvark]

Sorry, I'm still not convinced about the benefits of get_XXX here. Since you want to introduce a breaking change, wouldn't it be easier to just make length() return Length (the users would then do x.length().0 if they want the old behavior) and enforce the same convention for everything, e.g. .x() is typed and just .x is not.

[kvark]

please no! This is a hack, and essentially could be replaced by Into<T> but better not be replace at all.

[kvark]

in terms of rust naming conventions, this would have been into_value()

[nical]

the users would then do x.length().0

The problem is that the vast majority of users stick to the raw scalar access and don't use lengths, so I am not willing to compromise on the ergonomics the raw scalar access because it means that in practice we'd lose on the overall ergonomics.

please no! This is a hack, and essentially could be replaced by Into but better not be replace at all.

I tried and Into does not work a few reasons the main one being that the existing blanket impls in the standard library create conflicts making it impossible to implement Into<Length<T, U>> for T, and something else I don't remember clearly that required the trait to have both the T and the U to not upset the orphan rules.

I am very surprised that you consider this to be a hack. This is exactly the conversion trait for parameters pattern documented in elegant APIs in rust. I would have used one of the standard conversion traits like Into if it was possible but it unfortunately isn't.

Even if using Into was possible I would actually prefer ValueOrLength, because it is a lot easier to document and understand. It has a single, well defined purpose that is apparent in its name, and integrates well with the documentation. In the docs, click on it and you get an explanation that says that functions taking this can take either a length or a scalar. Compare this with clicking on "Into" in the doc and landing on a very generic piece of the standard library (you basically need to know about common rust design patterns to understand what is going on).

[nical]

(oops)

[kvark]

@nical

The problem is that the vast majority of users stick to the raw scalar access and don't use lengths, so I am not willing to compromise on the ergonomics the raw scalar access because it means that in practice we'd lose on the overall ergonomics.

We are only talking about length here, not the fields, so the ergonomic penalty is small. Also, this is not about "raw access", since length is not stored anywhere.

impossible to implement Into<Length<T, U>> for T

Your new traits go from Length -> value, not the other way around (like you tried to implement). So not sure why you needed Into<Length<T, U>>? I'd expect to only need From<Length<T, U>> for T.

I am very surprised that you consider this to be a hack. This is exactly the conversion trait for parameters pattern documented in elegant APIs in rust.

For one, this link recommends using the standard conversion traits, which I also suggested (Into), not introduce new ones. Secondly, this is just adding another level of type indirection, making it more difficult to reason about program API and behavior to me.

Even if using Into was possible I would actually prefer ValueOrLength, because it is a lot easier to document and understand. It has a single, well defined purpose that is apparent in its name,

The name is anything but idiomatic. And it is the case where if naming is problematic than we are probably doing it wrong :)

[nical]

The name is anything but idiomatic. And it is the case where if naming is problematic than we are probably doing it wrong :)

Could you elaborate on this? If you mean that an idiomatic rust trait has to be a verb or an action that the trait represents (Add, Into, AsRef, etc.), then I strongly disagree. If you look at WebRender's source code, less than half of the traits adhere to this principle. Take a popular crate like num-traits, most of the traits are named after what the thing is rather than what it does (Float, Signed, PrimInt, etc.), Iterator is probably the most commonly used trait and isn't name 'IterateorNext`.
In fact I bet that if you enlarge this kind of study to most of the code out there on crates.io, you'll find more trait names that are names of things rather than actions/verbs.

For one, this link recommends using the standard conversion traits, which I also suggested (Into), not introduce new ones.

Yes, and I challenge you to make this work :) This is what I tried first and I couldn't make either Into and From work in this specific case because of interactions between blanket impls and orphan rules.
Using the standard conversion traits is well and good when it can be made to work but when they don't work, there shouldn't be any issue with applying the same design pattern with a trait that actually works.

We are only talking about length here, not the fields, so the ergonomic penalty is small.

It's not just vector.length, this applies to the various rect.max_x/min_y/etc, side_offsets.horizontal, and all other methods that return a single number.

Consistency is usually a string value for you, maybe think of it like this: this would break euclid's rule of "Simple and convenient first, extra safety opt-in (second)" which is currently consistently applied throughout the API.
Among the users of euclid, most don't make use of the units and Length (some probably don't even know it's there). The ones who do, easily(-ish) opt into it, but for the majority who don't, returning a typed Length for something as common as getting the length of a vector isn't intuitive nor nice. It would go from raw scalars being the default with opt-in strong typing to raw scalars being the default for everything except for all functions that return a single number.

Also, this is not about "raw access", since length is not stored anywhere.

Sure, I meant "raw scalars" as in "not wrapped in a Length" whether or not they are members or computed.

[kvark]

Could you elaborate on this?

I mean AorB kind of name is never pretty, and I don't recall a precedent in stdlib.

Yes, and I challenge you to make this work :)

I take your word for it doesn't work :)

It's not just vector.length, this applies to the various rect.max_x/min_y/etc, side_offsets.horizontal, and all other methods that return a single number.

All of those combined are significantly less used than the fields access, so I think we can still expect the users to adopt? It's not even the "no method xxx()` found kind of breakage. It's one where the type is different, and compiler happily points to it.

Although, this is countered by:

Among the users of euclid, most don't make use of the units and Length (some probably don't even know it's there).

But in this case, why bother with ValueOrLength at all? Either admit that typed values are marginal use cases, or let's make them work consistently across the API.

this would break euclid's rule of "Simple and convenient first, extra safety opt-in (second)" which is currently consistently applied throughout the API.

You know better, but for the record - I never saw euclid prioritizing simplicity for safety. Quite the opposite - it's one of the safest math libraries, at least for the presence of typed coordinates.

[nical]

I mean AorB kind of name is never pretty, and I don't recall a precedent in stdlib.

What seduced me about this name is that it's unambiguous and self-documenting, but if you don't like it we can certainly come up with some other name.

You know better, but for the record - I never saw euclid prioritizing simplicity for safety. Quite the opposite - it's one of the safest math libraries, at least for the presence of typed coordinates.

I was the one who lobbied (stubbornly) until I convinced enough people that euclid should be rewritten to have typed coordinates (and as a result pretty much took over it). I do see euclid as rust's safe math library, but I don't want it to be the library that is safer but that nobody use because the other libs offer less friction.

That's the guideline I have been trying to follow since I pretty much rewrote the crate. I am convinced that we can make the typed APIs a lot nicer without having to compromise on the default untyped ones. If it comes at the expense of a few unusual type or function names, that's fine by me as long as whole thing remains easy to understand, and ergonomic enough that people want to use it.

Ideological debate aside, this PR has two sides:

• The trait for input parameters, which I totally didn't anticipate you would not like because while it unfortunately cant use a standard trait, it is a common pattern. We can find another name if it's really what puts you off.
• The _typed vs get_ rename which is a compromise to make the names a bit nicer while keeping a consistent naming scheme (without which getting your approval is hard). You don't think it changes much, but I still think it's a worthwhile improvement (and an effortless one to make because these APIs not having many users, as we are about to publish a new version we can rename them without rewriting much servo code).

If I was to compromise on something to get more value out of this renaming, it would without hesitation be the consistent naming scheme: having x() instead of x_typed() and get_horizontal() instead of horizontal_typed() is as close as we can probably get to a nice API under my own constraint of not affecting the untyped API. This is at the expense of only some methods having get_ prefixes to disambiguate them but I don't mind that.

[kvark]

Let's add this item to the list of topic to discuss when you are in Toronto. It appears that all arguments have been heard, and no clear consensus reached. How about we consider me abstained from voting on the PR and have another interested party merging and/or expressing their opinion? cc @nox

[pizzaiter]

I made a branch that uses only From/Into. I implemented From<T> for Length<T, U> and then made all constructors generic over Into<Length<T, U>>. Type inference suffers a bit more than I expected, e.g.

let l = Length::<f64, MyUnit>::new(0.0);
let p = TypedPoint2D::new(l, l); // cannot infer type for `T`

Anyway, I hope this helps the discussion.

[nical]

I made a branch that uses only From/Into. I implemented From for Length<T, U> and then made all constructors generic over Into<Length<T, U>>. Type inference suffers a bit more than I expected

Yeah I ran into that too. It's not really usable that way unfortunately.

[pizzaiter]

I am still surprised that the compiler cannot figure out the type. Strangely it is only a problem with T, it has no problem infering U. Does the example work with ValueOrLength?

[pizzaiter]

I tested it and ValueOrLength has the same inference problems in the above example. Or am I missing something?

For both (ValueOrLength<T, U> and Into<Length<T, U>>) the problem seems to be that the compiler has to also consider TypedPoint2D<Length<f64, MyUnit>, MyUnit>.

So we need a way to avoid

impl ValueOrLength<Length<f64, MyUnit>> for Length<f64, MyUnit>

and

impl From<Length<f64, MyUnit>> for Length<Length<f64, MyUnit>, MyUnit>

respectively.

One way to do this is to define a trait Scalar: ::num_traits::Num + ... with a blanket impl and change the code to

impl<T: Scalar, U> ValueOrLength<T, U> for T

and

impl<T: Scalar, U> From<T> for Length<T, U>

respectively. Since Scalar is not implemented by Length the inference works.

The downside is that most other impls would also need to be changed to impl<T: Scalar>. On the other hand, this can make the type signatures shorter.

[bors-servo]

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