HalfConverter diverges from float and double on non-finite values
- Dominant language
- C#
- Stars
- 18.3k
- Forks
- 5.6k
- PR merge metrics
- PR metrics pending
Description
`HalfConverter` rejects JSON number tokens that overflow to infinity, and reports a misleading error when asked to serialize a non-finite value. Both behaviors differ from `float` and `double`, and both were noticed while reviewing #131523, which added converters for `BFloat16` and the IEEE 754 decimal types by porting `HalfConverter`.
## Reading a number that overflows to infinity
```csharp
JsonSerializer.Deserialize("1e10000"); // ∞
JsonSerializer.Deserialize("1e10000"); // ∞
JsonSerializer.Deserialize("1e10000"); // JsonException: The JSON value could not be
// converted to System.Half.
```
`float` and `double` are the conforming ones here. IEEE 754 rounds a magnitude beyond the largest finite value to infinity, and `float.Parse`/`double.Parse` stopped throwing `OverflowException` in .NET Core 3.0 ([documented breaking change](https://learn.microsoft.com/dotnet/core/compatibility/3.0#floating-point-parsing-operations-no-longer-fail-or-throw-an-overflowexception)). This was raised as #113680 and closed as by-design.
The `Half` deviation looks unintentional. It comes from the guard added with `Half` support in #88962, whose own comment states a different purpose:
> `Half.TryParse` is more lax with floating-point literals than other S.T.Json floating-point types e.g: it parses `"naN"` successfully. Only succeed with the exact match.
`float` and `double` read number tokens through `Utf8Parser.TryParse`, which does not recognize named literals at all, so they need no such guard. `Half` uses `Half.TryParse` with `NumberStyles.Float`, which does. Rejecting overflow-to-infinity is collateral damage from closing that gap, not the goal.
The guard is also only load-bearing on one path. `HalfConverter.ReadCore` is reached either from a `Number` token — already validated against the JSON number grammar by the reader, so `"naN"` cannot appear — or from the `AllowReadingFromString` path, where the token contents are arbitrary. Only the second needs protection, and that is exactly where `Utf8JsonReader.GetSingleWithQuotes` places the equivalent `float.IsFinite(value)` check for `float`.
So the fix is to narrow the guard to string and property-name tokens rather than to remove it, which keeps `"naN"` rejected and lets `1e10000` produce `Half.PositiveInfinity`. `Half.TryParse("1e10000")` already returns `true` with `Half.PositiveInfinity`, so no work is needed in the numeric type itself. The `Debug.Assert(!Half.IsNaN(result) && !Half.IsInfinity(result))` in `ReadCore` would need to go with it.
## Writing a non-finite value
```csharp
JsonSerializer.Serialize(float.PositiveInfinity);
// ArgumentException: .NET number values such as positive and negative infinity cannot be written
// as valid JSON. To make it work when using 'JsonSerializer', consider specifying
// 'JsonNumberHandling.AllowNamedFloatingPointLiterals' …
JsonSerializer.Serialize(Half.PositiveInfinity);
// JsonReaderException: 'I' is an invalid start of a value. LineNumber: 0 | BytePositionInLine: 0.
```
Both throw, so this is a diagnostics problem rather than a correctness one, but the `Half` message leaks `WriteRawValue` internals and gives the caller nothing to act on. `float` and `double` go through `JsonWriterHelper.ValidateSingle`/`ValidateDouble`, which call the existing `ThrowHelper.ThrowArgumentException_ValueNotSupported`. `HalfConverter.WriteCore` and `WriteAsPropertyNameCore` should do the same.
## Compatibility
Both changes are behavioral breaks on a converter that has shipped since .NET 8:
- Input that previously threw `JsonException` would start succeeding, for values that overflow `Half`.
- The exception type for serializing a non-finite `Half` would change from `JsonReaderException` to `ArgumentException`.
I would expect the second to be uncontroversial and the first to need a breaking-change note. Neither seemed appropriate to fold into #131523, which is an unrelated API addition, so the new converters there ship with the corrected behavior and `Half` is left alone pending this discussion.
## Related divergence
While confirming the above, one more inconsistency turned up that is worth deciding on at the same time. Reading a `"NaN"` property name succeeds for `float` (`GetSingleWithQuotes` tries `JsonReaderHelper.TryGetFloatingPointConstant` first) but writing one throws (`WritePropertyName(float)` calls `ValidateSingle`). `Half` rejects it on both sides, which is at least self-consistent. I think `Half` has the better behavior here and `float`/`double` are the ones worth revisiting, but that is a separate and more disruptive change.
cc @PranavSenthilnathan, who raised the underlying observations in #131523.
> [!NOTE]
> This issue was authored by GitHub Copilot.
Contributor guide
Research direction
Start at HalfConverter.ReadCore, WriteCore, and WriteAsPropertyNameCore, then compare their behavior with Utf8JsonReader.GetSingleWithQuotes and JsonWriterHelper.ValidateSingle/ValidateDouble. Done means numeric overflow can produce Half infinity, string and property-name NaN remain rejected, and non-finite writes use the existing unsupported-value ArgumentException path.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- backend-api-design
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 58/100