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

Serialization of float minus zero should include the minus sign. #20596

Closed
SimonSapin opened this issue Jan 5, 2015 · 14 comments · Fixed by #24379
Closed

Serialization of float minus zero should include the minus sign. #20596

SimonSapin opened this issue Jan 5, 2015 · 14 comments · Fixed by #24379
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.

Comments

@SimonSapin
Copy link
Contributor

Test case: println!("{}", -0_f64), expected result -0, actual result: 0.

@Gankra Gankra added the E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. label Jan 22, 2015
@Diggsey
Copy link
Contributor

Diggsey commented Jan 22, 2015

While that's clearly desirable for the Show trait, it's not obvious that fmt::String should have that behaviour?

@SimonSapin
Copy link
Contributor Author

I don’t see why not. +0 and -0 are different values in IEEE 754.

@Diggsey
Copy link
Contributor

Diggsey commented Jan 23, 2015

Right, but as per the recent RFC, the String trait (being renamed to Display) is intended for user-facing output. For users, negative zero makes no sense, as they're unlikely to even be aware of IEEE 754.

The Show trait (being renamed to Debug) is what's used for debugging, so that should distinguish the two.

@SimonSapin
Copy link
Contributor Author

shrug

@steveklabnik
Copy link
Member

@aturon what do you think?

@SimonSapin
Copy link
Contributor Author

Even if it's not the default, this behaviour should be available somehow. At the moment I have to do silly things like if value == 0.0 && value.is_negative().

@aturon
Copy link
Member

aturon commented Feb 16, 2015

I agree with @Diggsey that doing this in Debug output but not Display output is probably the right way to go (and means that @SimonSapin can get the behavior he wants by writing {:?} instead.)

@rprichard
Copy link
Contributor

println!("{:+}", -0_f64) prints +0, but the {:+?} format is going to print -0. The formatters for scientific notation ('e' and 'E') also have this behavior where they omit the minus sign for negative zero, and emit a plus sign when a sign is explicitly requested, but there's no way to get scientific notation and the correct IEEE-754 sign.

I think I'd prefer that we be consistent and transparent, and print -0 for negative zero even for Display:

  • It's consistent with most of the other languages I've tested, including C, Java, Haskell, OCaml, Python, Ruby, and Lua. On the other hand, I did find exceptions -- JavaScript has negative zero but omits the '-'. C# and Go have negative zero and can print +0, though C# hides the '+' by default.
  • There is also NaN, which is even less readable to a non-technical audience than -0. A displayed NaN usually indicates a bug in the code, IMO.

Regardless, if the new behavior is desired, then we should test for the edge cases I've described.

Correction: C# does not print +0 for negative zero. Instead, it omits the sign. Its ToString method allows for specifying three formats, one each for positive, negative, and zero.

@rprichard
Copy link
Contributor

I noticed that this test case currently prints -0. I think it will continue doing so with this PR:

println!("{}", -1e-9);

@hanna-kruppe
Copy link
Contributor

@rprichard Yes, my PR does not (well, should not) change behavior for small negative numbers. The existing code is pretty naive w.r.t. rounding (and probably rather inexact at the rounding it does perform), but this is a separate issue.

As for scientific notation, I'm not sure if this is desired: like Display, it's probably more commonly used for user-facing output, right? If it's desired, I could add it pretty easily.

alexcrichton added a commit to alexcrichton/rust that referenced this issue Apr 14, 2015
Fixes rust-lang#20596 by making `Debug` render negative zero with a `-` without affecting the behavior of `Display`.

While I was at it, I also removed some dead code from `float_to_str_bytes_common` (the one from `libcore/fmt/float.rs`, not the function of the same name in `libstd/num/strconv.rs`). It had support for different bases, and for negative numbers, but the function is internal to core and the couple places that call it (all in `libcore/fmt/mod.rs`) never use those features: They pass in `num.abs()` and base 10.
@rprichard
Copy link
Contributor

The user could easily be another computer system or another programmer, which is frequently the case for, say, the str type. In the case of scientific notation, the user could be a scientist or engineer (likely also a programmer). If they're using floating-point, they probably should be aware of IEEE-754 semantics.

@SimonSapin Both {} and {:?} truncate after 6 decimal places (and they never use the exponent form). Is this OK for CSS serialization? It seems odd to care about negative zero, but not about precision of small numbers.

Neither {} nor {:?} are generally acceptable for floating-point serialization due to this truncation. To faithfully show large and small numbers, you'd need to use the exponent form, but then it will change the sign. TBH, this truncation seems like a flaw in the Display and Debug implementation for floating-point. It should do something like Go and display an exponent form by default, or it should do the same thing as everyone else (C++, Java, Haskell, OCaml, Python, Ruby, Lua, Javascript, C#) and select between non-exponent and exponent forms depending on the value's magnitude.

Further comments on other languages:

  • Go's print converts negative zero to positive zero, but its fmt.Printf("%f, x) shows negative zero faithfully.
  • Javascript's only number values are floats, so it's a little more justifiable that it would hide negative zero. Showing it would break its 54-bit integer abstraction.

Basically, I think it should be the programmer's responsibility to convert negative zero to positive, if that's what's appropriate for the given context. Debug ought to be a refinement of the printed value, but with this PR, it will contradict Display. Using Debug for non-debug output is also awkward -- is its format stable? Display and Debug are truncating small numbers, which is particularly wrong for Debug, so relying on the current Debug format is dodgy.

@rprichard
Copy link
Contributor

@SimonSapin Could you answer the question I posed above? i.e. Why does CSS serialization care about negative zero, but is OK with truncating small floating-point values (e.g. println!("{:?}", 1e-8) prints 0 rather than 1e-8 (or similar).)

@SimonSapin
Copy link
Contributor Author

It’s a combination of multiple things.

In CSS you can write things like :nth-child(2n+1) or unicode-range: U+00-FF, but their syntax were designed (as far as I can tell) with little consideration as to how it fits in the CSS tokenizer. In some cases like 4n -0 or U+4-0, a - is tokenized as the sign of a number token, but its presence is significant.

Additionally, invariants like parse(s) == parse(serialize(parse(s))) for any string s should be preserved. (It’d be surprising if element.style.someProperty = element.style.someProperty were not a no-op). And the value of a CSS variable / custom property is (at least conceptually) a sequence of arbitrary tokens.

Taken together, the serialization tests of rust-cssparser fail if the sign of negative zero is not accounted for.

This is a very contrived situation, and arguably this specific issue could be worked around in other ways. But I maintain that Display for f32 ignoring the sign of zero seems like a bug to me, regardless of CSS. A sign can be more significant than a rounding error.

@pnkfelix
Copy link
Member

(if I had my druthers, Display on floats that happen to be integers would always include a trailing .0, and include the sign on negative values. But I'm a schemer, so i'm used to seeing that sort of output when working with inexact values...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants