Skip to content

json: document that an unquoted float can be rounded twice - #701

Merged
iainmcgin merged 2 commits into
anthropics:mainfrom
farmer-data:fix/float-double-rounding-647
Oct 14, 2026
Merged

iainmcgin merged 2 commits into
anthropics:mainfrom
farmer-data:fix/float-double-rounding-647

Conversation

@farmer-data

Copy link
Copy Markdown
Contributor

Refs #647. This documents the behaviour rather than changing it, so the issue can stay open to track a real fix through #482.

An unquoted JSON float with a fraction or exponent reaches json_helpers::float as the f64 serde parsed and is then narrowed with as f32, so it is rounded twice. A quoted decimal, and an unquoted integer in the i64/u64 range, is rounded once. A decimal close enough to an f32 midpoint becomes that midpoint as an f64 and then rounds to even, so 1.0000000596046448 decodes to 1.0 unquoted and 1.0000001 quoted.

The visitor sees only the f64, and an f64 sitting exactly on a midpoint does not say which side the decimal was on, so the helper cannot correct it. Rounding once needs the token text: arbitrary_precision (#482), or a raw-value capture, which would tie the helpers to serde_json while they stay generic over Deserializer. As the issue suggests for that case, this PR documents the behaviour:

  • json_helpers::float rustdoc: when the two paths differ (by one f32 ulp), the issue's example, the overflow-threshold case (rejected unquoted, accepted quoted), that protobuf C++ narrows the same way while protobuf-go rounds once, and that only a producer can avoid it, by quoting.
  • A pointer to it on FloatValue's Deserialize impl (buffa-types/src/wrapper_ext.rs) and in DynamicMessage::from_json, since users of generated messages, wrappers and reflection never name json_helpers directly.
  • A paragraph in the JSON section of docs/guide.md, next to the arbitrary_precision note.
  • Tests pinning the double-rounded and single-rounded results, and the single rounding of integer tokens, for the generated helper and for DynamicMessage::from_json. They compare exact bits, so a future single-rounding fix will fail them and prompt a docs update. The example input gives the same f64 with or without serde_json's float_roundtrip.
  • A Changed changelog fragment marked as documentation, with no behaviour change.

Verification

  • task lint, task doc and task test-local (4186 passed, 0 failed), with the repository-pinned protoc 33.5.
  • Reviewed with the repo's rust-code-reviewer and rust-api-ergonomics-reviewer per CLAUDE.md: no Critical or High findings. Addressed the Mediums: the midpoint bound in the rustdoc no longer assumes float_roundtrip, the note now also appears on FloatValue, DynamicMessage::from_json and in the guide, and the guidance says only the producer can avoid the difference. Also addressed the Lows: integer tokens are excluded (with a test), the overflow case is noted, and the changelog is Changed with "No behaviour change".

Refs anthropics#647.

An unquoted JSON number with a fraction or exponent reaches
`json_helpers::float` as the `f64` the deserializer parsed and is then
narrowed to `f32`, while a quoted decimal (and an unquoted integer in the
i64/u64 range) is rounded once. A decimal close enough to an `f32`
midpoint becomes that midpoint as an `f64`, and narrowing rounds it to
even, so `1.0000000596046448` decodes to 1.0 unquoted and 1.0000001
quoted. A serde visitor sees only the `f64`, so the original decimal
cannot be recovered; rounding once needs the token text
(`arbitrary_precision`, see anthropics#482, or a raw-value capture).

Document the behaviour where users look for it: the
`json_helpers::float` rustdoc, the `FloatValue` `Deserialize` impl,
`DynamicMessage::from_json`, and the JSON section of the guide. Pin the
double-rounded and single-rounded paths, and the single rounding of
integer tokens, for generated helpers and `DynamicMessage::from_json`.
@github-actions

Copy link
Copy Markdown

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

The change adds documentation and two tests that pin the current
behaviour. A Changed entry would read as a behaviour change in the
release notes.

@iainmcgin iainmcgin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[claude code] Pushed one commit (befada9) that removes .changes/unreleased/changed-20261013-062603.yaml. The PR is documentation plus two tests that pin current behaviour, and a docs-only change carries no fragment here: a Changed entry would read as a behaviour change in the release notes. Everything else is as you wrote it.

@iainmcgin
iainmcgin added this pull request to the merge queue Oct 14, 2026
Merged via the queue into anthropics:main with commit e4d44d1 Oct 14, 2026
11 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Oct 14, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants