dotnet / dotnet/runtime

HalfConverter diverges from float and double on non-finite values

Open
#131,525 1 comment 0 reactions 0 assignees View on GitHub
area-System.Text.Json
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.