Length and ScaleFactor with statically-checked units

[mbrubeck]

For servo/servo#2226. See code comments for details.

[pcwalton]

I'm confused as to why this uses phantom types. I'd think it would be simpler to just use newtype wrappers, like Au does today.

[pcwalton]

In particular I'm a bit concerned about the complexity of the trait signatures here.

[SimonSapin]

Looks good to me. (r=me modulo @pcwalton’s concerns.)

[mbrubeck]

I'm confused as to why this uses phantom types. I'd think it would be simpler to just use newtype wrappers, like Au does today.

My earlier drafts used newtypes, but I found that it made it harder to write generic code and ended up requiring a lot more boilerplate. The current design allows all of the implementation to be generic, so adding a new unit requires only a single line of code. (See servo/servo#2444 for some examples.) I'm still pretty new to Rust, so maybe I was missing some possibilities. I'll try exploring this avenue more.

One thing that the phantom types helped with was writing generic code to cast between types like Length<DevicePixel, int> to Length<DevicePixel, f32>, where the unit is the same but the representation is different.

Au is hard-coded to an i32 representation. This seems bad because it would lead to an explosion of newtypes like struct DevicePixel(f32) and struct DevicePixelInt(i32), each requiring its own trait implementations.

Even if we parameterize the representation (struct DevicePixel<T>(T)), we'd need at least per-unit trait implementations, and possibly additional ones like impl IntLength for DevicePixel<int>.

In particular I'm a bit concerned about the complexity of the trait signatures here.

If the concern is about readability, we can use typedefs to hide the full type signatures of commonly-used types from most users of those types.

[pcwalton]

I'm confused…why do we want DevicePixel to be sometimes in floats and sometimes in ints? Wouldn't it be simpler to have just one machine representation?

[mbrubeck]

I'm confused…why do we want DevicePixel to be sometimes in floats and sometimes in ints? Wouldn't it be simpler to have just one machine representation?

Possibly. DevicePixel coordinates, for example, are used as framebuffer indices, so it really makes sense for them to be uint. However, we may sometimes need a signed type to represent differences between coordinates. And we need to convert them to f32 at least as an intermediate step to multiply them by f32 scale values. And we need to accept DevicePixel coordinates as f64 from GLFW and as c_int from GLUT, and pass them as i32 to Azure.

We can definitely handle all of those cases by simply dropping typed units before casting to any other representation (and applying new units afterward as appropriate), though I think we can catch more errors and simplify the code if the values get tagged with units at the earliest point possible, remain tagged through any casts and calculations, and become untagged only at the last possible point before being passed to external code.

[pcwalton]

Could we perhaps use macros to reduce the boilerplate of defining new newtype wrappers?

[mbrubeck]

My earlier drafts used newtypes, but I found that it made it harder to write generic code and ended up requiring a lot more boilerplate.

To make this more concrete: When using newtypes, I had to reimplement Add/Sub/Mul/Div/etc. for every single type. Length<Unit, T> lets me do this once in in rust-geom. This means that adding a new unit is now one line instead of around 50 lines of code.

In cases where you do want to enforce a single machine representation (and therefore don't need any type params), then with just two extra lines you can just add a public alias to a private type:

enum DevicePixelPrivate {}
pub type DevicePixels = Length<DevicePixelPrivate, uint>;
pub fn DevicePixels(x: uint) -> DevicePixels { Length(x) }

To the user of this code, it looks just like a newtype struct:

let x: DevicePixels = DevicePixels(1);

Could we perhaps use macros to reduce the boilerplate of defining new newtype wrappers?

I expect so, but I'm curious what the benefits would be versus the above. And it would still lose the ability to safely cast between representations while automatically keeping the same units, which I still believe is beneficial.

[pcwalton]

It just feels like a lot of type-level machinery to do something simple. Long type signatures have a significant readability cost.

[brendanzab]

What we really need is a separate units of measure library with compile-time dimensional analysis. Perhaps with macros for defining new types. I am ok with phantom types when they are used in the appropriate places, but this doesn't seem like it.

[metajack]

This should probably say u64 instead of uint64

[brendanzab]

I am a little concerned about this. This would cause unpredictable results of methods impled on T. I would rather an explicit .get() or .unwrap() method to properly enforce type safety.

[mbrubeck]

I agree; I'll change this to an explicit method. get or unwrap are both good names; I'm also considering val or value.

[brendanzab]

Why can't we just enforce that points must have length units?

[SimonSapin]

We could, but we’d have to switch all of Servo in one go to typed points. This PR so far is backwards-compatible.

[brendanzab]

Again, concerned about the possibimities of auto-deref here.

[brendanzab]

This is cool 👍

[brendanzab]

After re-reading this I think I am warming to the idea.

[brendanzab]

Is there a way of making the wrapped type generic too?

[mbrubeck]

Yes, it would be straightforward to make this generic; the only drawback would be a bunch more type parameters throughout the implementation. I'm inclined to make this change, in order to make the rust-geom library as general as possible, though I'm also curious whether Servo could do without it. (For comparison, Gecko's ScaleFactor uses a hard-coded single-precision float.)

[mbrubeck]

I think this is ready to land. r? @metajack

[metajack]

Can this not be #[deriving(Clone)]?

[metajack]

@mbrubeck One small question, otherwise looks good.

[metajack]

r+