builtins.toJSON changed floats in 2.12

[srhb]

Describe the bug

Before 09f00dd:

~/src/nix > nix shell --command nix repl
nix-repl> builtins.toJSON 1.000001
"1"

After 09f00dd:

~/src/nix > nix shell --command nix repl
nix-repl> builtins.toJSON 1.000001
"1.000001"

Related:

Actual change: #7313
Wish for this change: #5733

(Edit: accidentally included the same copy-paste twice, hiding the problem)

[fricklerhandwerk]

Discussed in the Nix team meeting:

• related: Improve representation for "floats" and fix repro bugs #8274
• @edolstra: floats themselves are specified by IEEE, but printing is not that well defined
  • previously we truncated to 6 digits using a custom implementation, now we use nlohmann that works slightly differently
  • it may be actually more correct now
• @thufschmitt: should make printing determinstic
• @edolstra: while the change broke backwards compatibility arguably the old behavior was a bug
• @thufschmitt: should add regression tests to prevent that in the future
• @roberth: we may consider deprecating floating points altogether
• needs discussion

[nixos-discourse]

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-05-05-nix-team-meeting-minutes-52/27893/1

[Ericson2314]

See also #6238 and #3480, which increase precision when printing (non JSON).

[thufschmitt]

Discussed again in today's Nix team meeting.

nlohmann json doesn't really provide any guaranty for byte-per-byte reproducibility (nlohmann/json#2526), but doesn't really change in practice. So a reasonable strategy would be to add some extensive enough testing that the output stays stable and keep relying on it as long as this doesn't break.

EDIT: More discussions going on actually

[srhb]

So a reasonable strategy would be to add some extensive enough testing that the output stays stable and keep relying on it as long as this doesn't break.

The old output or the new output? Just trying to figure out whether I should expect NixOS 23.05 to contain the breakage or not, before I explode prod. :)

[srhb]

Ah, your edit happened just as I submitted. Awaiting eagerly 🙏

[thufschmitt]

Summary of the extra discussion:

Getting back to the old behaviour would be prohibitively costly (the initial reason why it got changed was removing a bunch of in-house code), and we don't really know how to move forward if we go that way. So we'll keep and commit to the new behaviour which is more correct (the old one wasn't round-tripping in particular) and much easier to maintain in the long run.

We don't do this lightly, but we have weighed our options and considering that the floating point logic is unlikely to affect actual packages, the cost of mismatched hashes only applies to configuration files, such as those that use types.float in NixOS. Instead of putting a disproportionate amount of effort into a questionable implementation plan, we decide to move forward and instead assure that the new representation of floats will be tested properly.

[ajs124]

How will this change in behavior be communicated to users?

[Ericson2314]

@ajs124 Release notes for starters. If you have other suggestions I'm sure we'd be happy to hear them.

It is not useful for users to "rely on bugs" "rely on undefined bheavior" etc. The old behavior was a lawless accident. The new behavior has a law, which is that it round-trips property. (As a corollary, we also know that it must not truncate.)

It was always the intention that from* and to* functions should round-trip as much as is feasible.

[srhb]

Sounds good to me! And yes, I agree that it's an improvement, even if it breaks reproducibility across nix versions in a surprising way, my only concern is that it will ruin someone's day bigtime if it isn't noticed beforehand. Strong warnings in the NixOS release notes will certainly help aleviate some of that. :)

[RaitoBezarius]

If someone can communicate this in our NixOS release notes, that'd be great too.

[nixos-discourse]

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-05-22-nix-team-meeting-minutes-57/28447/1

[thufschmitt]

If someone can communicate this in our NixOS release notes, that'd be great too.

Where are these?

[thufschmitt]

If someone can communicate this in our NixOS release notes, that'd be great too

Done in NixOS/nixpkgs#233975