Serialize number to no more than necessary precision

[upsuper]

I'm not sure whether it can be simplified... and I probably need some more idea for how should I test it... But things look fine so far.

This should probably goes to an independent crate at some point.

It should fix servo/servo#17205.

---

This change is 

[upsuper]

cc @nox @bzbarsky @Manishearth

[upsuper]

r? @SimonSapin

[SimonSapin]

After experimenting a bit, I believe that the standard library’s default float formatting already uses the shortest representation that parses to the same float. The issue in servo/servo#17205 is specifically with percentages. I can reproduce it with the given test case, but not when changing % to px. This is because we divide the value of percentage tokens by 100.0 during parsing, and multiply it by 100.0 during serialization.

"15%" does not serialize as "15%" because (15_f32 / 100.) * 100. is not equal to 15_f32.

Float formatting can be very subtle, and I’m not at all familiar with the details. Responsibility for maintaining code in this repository is de-facto mine, so I’d much rather find another solution than adding a bunch of float formatting code. What do you think of not dividing percentage token value in the parser and only doing so in an accessor for layout code?

[bzbarsky]

I believe that the standard library’s default float formatting already uses the shortest representation that parses to the same float.

That's true, but the "same float" is not the important thing here. I'll follow up in servo/servo#17205, though I'll be largely repeating things I already said there....

[SimonSapin]

Alright, it sounds like we do want to round to 6 decimal places (from the first significant digit, not from the fraction separator). Still, I’d feel better if that code were not in cssparser. @upsuper, would you agree to maintain it in a separate crate?

[upsuper]

I'm okay with maintaining it in a separate crate, although I don't have a good sense of how should I name that crate.

[bors-servo]

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

[upsuper]

I created a new crate dtoa-short for that purpose. It would be appreciated if you can also review the code in that crate.

[SimonSapin]

Why can’t this write write to dest directly? Can we find some way to avoid allocating a String?

[SimonSapin]

Oh, I see contains below. Could something be added to dtoa_short so that it tells us that information?

[upsuper]

In the initial commit here I added a "has_exp" out parameter, but that feels pretty broken as a public interface... Probably I can add a return value for dtoa_short::write to return the form of the number serialized.

[SimonSapin]

Unless you need to extend and existing value, out parameters are not idiomatic in Rust and can be replaced by returning a tuple. Or this case, replacing Result<(), Error> with Result<bool, Error>, or maybe a custom enum instead of bool.

[SimonSapin]

I read through dtoa-short, the code looks ok. @upsuper, could you run cargo owner -a github:servo:cargo-publish dtoa-short to allow people in the servo team to publish a new version in case you’re on vacation or otherwise away?

[upsuper]

I got error: failed to add owners to crate dtoa-short: api errors: only members of github:servo:cargo-publish can add it as an owner. Probably you need to add me into the team first? Or probably I can add you to the owner of the crate, and you can add servo team then.

[upsuper]

Updated the patch with updated dtoa-short.

[SimonSapin]

r=me

Presumably you’ll also want a version number bump to get this on crates.io?

[upsuper]

Oh, hmm, yep.

[upsuper]

@bors-servo r=SimonSapin

[bors-servo]

📌 Commit 2181f2f has been approved by SimonSapin

[bors-servo]

⌛ Testing commit 2181f2f with merge c2f8e1f...

[bors-servo]

☀️ Test successful - status-travis
Approved by: SimonSapin
Pushing c2f8e1f to master...

[Scotchester]

I want to call out @bzbarsky's comment from servo/servo#17205:

The basic question is what the thing is that we want to preserve.

The current behavior assumes that the floating point value is the thing we care about and that serializing should preserve it with as much fidelity as possible. But from the point of view of a web author the internal floating-point representation is not something they care about. Furthermore, it's commonly lossy from the web developer's point of view in all sorts of cases. That includes not just percentages, but various calc() cases, computed style cases, etc.

The other option, and what I think we should do, is to aim at preserving "the original string representation" when serializing. This is impossible in general, of course, because lots of strings map to the same float. But all 6-digit decimal strings can be encoded in distinct floats. So if we always serialize to a 6-digit string, we may produce something that parses to a different float value (or may not, depending on what processing happens in addition to the parsing), but we are more likely to produce something that round-trips the actual original input.

I ran into an issue today where I have some SCSS being run through Parcel – whose Lightning CSS transformer relies on rust-cssparser – and the calculated values in the compiled CSS were all being rounded to 6 digits. This resulted in some values that weren't exactly what I was expecting.

This project is well over my head, but the quoted comment from @bzbarsky resonates with me – doing this sort of rounding in some cases results in "lossy" conversions from Sass/SCSS to CSS. Is there anything that can be done?