Skip to content

Commit ddbb4c9

Browse files
robertvokacrobertvokac
committed
fix(core): a negative unsigned value overflows instead of failing to parse (#2362)
Byte, UInt16, UInt32 and UInt64's Parse/TryParse now accept a negative-indicating token grammatically and reject it as a RANGE failure, matching .NET (Number.Parsing.cs:157, `!TInteger.IsSigned && number.IsNegative`). This ends a deliberate deviation that had been documented on TryParseUnsignedCore for about a year. UInt32::Parse("-1") was FormatException is OverflowException UInt32::Parse("-0") was FormatException is 0 UInt32::Parse("-0.0") was FormatException is OverflowException THE OLD ARGUMENT WAS WRONG ON ITS OWN TERMS, and that is the whole ticket. The deviation was justified on the grounds that the only practical effect was "which exception TYPE a clearly-invalid input throws". But "-0" is not a clearly-invalid input. It is a VALID one that returns 0, because Number.Parsing.Common.cs:259-268 clears IsNegative when StateNonZero was never set and no decimal separator was seen. So the old rule did not substitute one exception for another -- it rejected an input .NET accepts. THE TWO ROWS COULD NOT BE SEPARATED. Repairing "-0" alone would have aligned one spelling and left "-1" throwing the wrong type, which is worse than the old consistent rule. So the grammar (accept '-' and a closed "(...)"), the all-zero normalisation (clear the sign when there is no nonzero digit and no decimal separator) and the negative-is-overflow rejection all landed together. The asymmetry between "-0" and "-0.0" is .NET's, not a convenience: a decimal separator sets StateDecimal and the sign is then NOT cleared. That single `!sawDecimal` guard is the only thing separating them -- and this makes #2356's mutation M2 OBSERVABLE for the first time. #2356 transcribed the same guard into the signed core and recorded honestly that its mutation was not caught, because negating zero is zero either way. It is caught now, in the unsigned core. Note also that the rejection sits in the SAME disjunction as the digit-count overflow in .NET, so a caller cannot tell the two apart -- both are OverflowException -- which is why the order between them here is free, and the comment says so rather than implying a derivation that does not exist. Three pins inverted (two of them asserting FormatException for "-1", one asserting it for "-0"), one case added, and two further pins' comments corrected: they kept passing across the change because TryParse reports both failures the same way, which is precisely why the new case pins the exception TYPE that TryParse cannot show. Six mutations caught: drop the rejection; drop the sign half of the normalisation; clear the sign ignoring sawDecimal; leading loop stops accepting '-'; leading loop stops accepting '('; drop the unclosed-paren check. A SEVENTH IS A NO-OP AND IS RECORDED AS ONE rather than counted as a pass: reinstating the old grammar-level rejection immediately before the digit scan changes nothing, because the leading-token loop has already consumed the '-' by then. That is the same shape #2138 recorded -- an allow-list and a deny-list are equivalent once the token is gone. Downstream, measured per SA-2 condition 5: neither cna nor mobile-eggbert calls any unsigned Parse/TryParse -- zero sites in both. Neither was modified. TryParse callers see no change at all; only the exception type moved, plus the one row that stopped failing. Gate: 17,302 run, 17,302 passed, 0 failed, 0 skipped across 38 executables, GREEN. docs/Migration-UnsignedNegativeOverflows.md
1 parent c079845 commit ddbb4c9

6 files changed

Lines changed: 268 additions & 50 deletions

File tree

‎CLAUDE.md‎

Lines changed: 1 addition & 1 deletion
Large diffs are not rendered by default.

‎docs/Migration-UnsignedNegativeOverflows.md‎

Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,118 @@
1+
<!-- SPDX-License-Identifier: MIT -->
2+
<!-- Copyright (c) Robert Vokac and contributors -->
3+
4+
# Migration — a negative unsigned value now overflows instead of failing to parse (ticket #2362)
5+
6+
*2026-08-18.* `Byte`, `UInt16`, `UInt32` and `UInt64`'s `Parse`/`TryParse` now accept a
7+
negative-indicating token grammatically and reject it as a **range** failure, matching .NET.
8+
`UInt32::Parse("-1")` raises `OverflowException` where it used to raise `FormatException`, and
9+
`UInt32::Parse("-0")` returns **0** where it used to throw at all.
10+
11+
Landed under `docs/StandingApprovals.md` SA-5. No signature change, no layout change, no
12+
`noexcept` change.
13+
14+
---
15+
16+
## 1. What changed
17+
18+
| Call | Was | Is |
19+
|---|---|---|
20+
| `UInt32::Parse("-1")` | `FormatException` | `OverflowException` |
21+
| `UInt32::Parse("-0")` | `FormatException` | **`0`** |
22+
| `UInt32::Parse("-000")`, `"-0E30"` | `FormatException` | `0` |
23+
| `UInt32::Parse("-0.0")` (`NumberStyles::Number`) | `FormatException` | `OverflowException` |
24+
| `UInt32::Parse("(1)")` (`AllowParentheses`) | `FormatException` | `OverflowException` |
25+
| `UInt32::Parse("(0)")` | `FormatException` | `0` |
26+
| `UInt32::Parse("1-")` (`AllowTrailingSign`) | `FormatException` | `OverflowException` |
27+
| `UInt32::Parse("(1")` — unclosed | `FormatException` | `FormatException` (unchanged) |
28+
| `UInt32::Parse("--1")`, `"-1-"` | `FormatException` | `FormatException` (unchanged) |
29+
| `UInt32::Parse("+42")` | `42` | `42` (unchanged) |
30+
| every input with no negative token | — | **unchanged** |
31+
32+
`TryParse` still returns `false` for every row that throws, so a caller using `TryParse` sees no
33+
difference at all. Only the exception **type** moved, plus the one row that stopped failing.
34+
35+
## 2. Why the family had to move together
36+
37+
This was a documented, deliberate deviation, recorded on `TryParseUnsignedCore` for about a year.
38+
The argument for it was that the only practical effect was *which exception type a clearly
39+
invalid input throws*.
40+
41+
**That argument was wrong on its own terms, and the reference is why.** `"-0"` is not a clearly
42+
invalid input. It is a **valid** one that returns `0`:
43+
44+
```csharp
45+
// Number.Parsing.Common.cs:259-268
46+
if ((state & StateNonZero) == 0)
47+
{
48+
if (number.Kind != NumberBufferKind.Decimal) number.Scale = 0;
49+
if ((number.Kind == NumberBufferKind.Integer) && (state & StateDecimal) == 0)
50+
number.IsNegative = false;
51+
}
52+
```
53+
54+
So the old rule did not merely substitute one exception for another — it **rejected an input .NET
55+
accepts**. And the two rows could not be separated: repairing `"-0"` alone would have aligned one
56+
spelling while leaving `"-1"` diverging, which is worse than the old consistent rule. The grammar,
57+
the all-zero normalisation and the negative-is-overflow rejection therefore landed in one change.
58+
59+
The rejection itself is `Number.Parsing.cs:157`:
60+
61+
```csharp
62+
if ((i > TInteger.MaxDigitCount) || (i < number.DigitsCount)
63+
|| (!TInteger.IsSigned && number.IsNegative) || number.HasNonZeroTail)
64+
```
65+
66+
Note it sits in the **same disjunction** as the digit-count overflow, so a caller cannot tell the
67+
two apart — both are `OverflowException` — which is why the order between them in this port is
68+
free.
69+
70+
## 3. The asymmetry is .NET's, not a convenience
71+
72+
`"-0"` is `0` and `"-0.0"` overflows, because a decimal separator sets `StateDecimal` and the sign
73+
is then **not** cleared. That single `!sawDecimal` guard is the only thing separating the two rows.
74+
75+
Ticket #2356 transcribed the same guard into the **signed** core and recorded honestly that its
76+
mutation was *not caught* — negating zero is zero either way, so it was unobservable there. It is
77+
observable here, and `Fix2362_ANegativeUnsignedValueOverflowsAndMinusZeroIsZero` is the test that
78+
makes it so.
79+
80+
## 4. To migrate
81+
82+
Catch `OverflowException` as well as `FormatException`, or catch `SystemException`:
83+
84+
```cpp
85+
// before
86+
try { value = UInt32::Parse(text); }
87+
catch (const System::FormatException&) { value = 0; }
88+
89+
// after
90+
try { value = UInt32::Parse(text); }
91+
catch (const System::FormatException&) { value = 0; }
92+
catch (const System::OverflowException&) { value = 0; }
93+
```
94+
95+
Code that already used `TryParse` needs no change.
96+
97+
## 5. One mutation that is not a mutation
98+
99+
Reinstating the old grammar-level rejection **immediately before the digit scan** changes nothing
100+
and is not evidence of a gap: the leading-token loop has already consumed the `'-'` by then, so
101+
the check never fires. This is the same shape #2138 recorded — an allow-list and a deny-list are
102+
equivalent once the token is gone. The meaningful mutation is removing the acceptance from the
103+
loop itself, and that is caught by four tests.
104+
105+
| Mutation | Caught |
106+
|---|---|
107+
| Drop the negative-is-overflow rejection | ✅ (4 tests) |
108+
| Drop the sign half of the all-zero normalisation | ✅ (2 tests) |
109+
| Clear the sign unconditionally, ignoring `sawDecimal` | ✅ |
110+
| Leading loop stops accepting `'-'` | ✅ (4 tests) |
111+
| Leading loop stops accepting `'('` | ✅ (2 tests) |
112+
| Drop the unclosed-paren check | ✅ |
113+
| Reinstate the grammar rejection after the loop | **no-op, see above** |
114+
115+
## 6. Downstream, measured
116+
117+
Per SA-2 condition 5: neither `cna` nor `mobile-eggbert` calls `Byte`, `UInt16`, `UInt32` or
118+
`UInt64` `Parse`/`TryParse` — **zero sites in both**. Neither repository was modified.

‎modules/core/include/System/detail/IntegerNumberStylesParser.hpp‎

Lines changed: 71 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -118,17 +118,28 @@ using System::Globalization::NumberStyles;
118118
// (e.g. trailing garbage after the number) still take precedence over this overflow, matching
119119
// real .NET's own explicitly-commented precedence rule.
120120
//
121-
// A deliberate, documented DEVIATION from real .NET for the *unsigned* parsers specifically:
122-
// real .NET's general (non-Integer-style) parse path allows a negative-indicating token (a
123-
// literal '-' or a closed '(...)') to reach an unsigned type's buffer conversion, where it then
124-
// fails with OverflowException (via TryNumberBufferToBinaryInteger's
125-
// `!TInteger.IsSigned && number.IsNegative` check) rather than FormatException -- except for an
126-
// all-zero magnitude ("-0"), which real .NET actually accepts as positive zero. This port does
127-
// not replicate that: consistent with this file's pre-existing Integer-style convention (a
128-
// leading '-' was already a hard, immediate reject before this extension), any negative-
129-
// indicating token in an unsigned parse is rejected outright as a format failure. This keeps
130-
// unsigned-parsing behavior uniform across every style rather than importing one more real-.NET
131-
// edge case whose only practical effect is which exception TYPE a clearly-invalid input throws.
121+
// THE UNSIGNED PARSERS' DEVIATION IS GONE (ticket #2362, 2026-08-18). It stood for a year and
122+
// it was recorded here, so it is worth stating what it was and why removing it was the right
123+
// call rather than the tidy one.
124+
//
125+
// Real .NET lets a negative-indicating token (a literal '-' or a closed "(...)") reach an
126+
// unsigned type's buffer conversion, where `!TInteger.IsSigned && number.IsNegative`
127+
// (Number.Parsing.cs:157) turns it into an OverflowException -- except for an all-zero magnitude
128+
// with no decimal separator, which is accepted as positive zero. This port used to reject every
129+
// such token in the GRAMMAR, as a FormatException, on the argument that the only practical
130+
// effect was which exception TYPE a clearly-invalid input throws.
131+
//
132+
// That argument was wrong on its own terms, and the reference is why: "-0" is not a
133+
// clearly-invalid input, it is a VALID one that returns 0. So the old rule did not merely
134+
// substitute one exception for another -- it rejected an input .NET accepts. And the two rows
135+
// could not be separated: repairing "-0" alone would have left "-1" diverging, which is worse
136+
// than the old consistent rule. The grammar, the all-zero normalisation and the
137+
// negative-is-overflow rejection therefore landed together.
138+
//
139+
// UInt32::Parse("-1") OverflowException
140+
// UInt32::Parse("-0") 0
141+
// UInt32::Parse("-0.0") OverflowException (a decimal separator keeps the sign)
142+
//
132143
struct IntegerNumberStylesParser {
133144

134145
// Invariant-culture separators/symbol this port's Parse/TryParse grammar uses -- see the
@@ -443,12 +454,26 @@ struct IntegerNumberStylesParser {
443454
return true;
444455
}
445456

446-
// Unsigned counterpart of TryParseSignedCore. NumberStyles.Integer/.Number/.Currency as
447-
// applied to an unsigned type still allow AllowLeadingSign/AllowTrailingSign in principle
448-
// for a literal "+" (matching real .NET's UInt32.Parse accepting a leading '+'), but any
449-
// token that would indicate a negative value ('-', or a closed "(...)") is always rejected
450-
// as a format failure -- see the class doc-comment's "deliberate DEVIATION" note for why
451-
// this port doesn't chase real .NET's negative-unsigned-throws-OverflowException quirk.
457+
// Unsigned counterpart of TryParseSignedCore, and now a near-exact mirror of it.
458+
//
459+
// TICKET #2362 (2026-08-18) ended a long-standing deliberate deviation. This core used to
460+
// reject a '-' or a '(' in the GRAMMAR, before any digit was folded, so UInt32::Parse("-1")
461+
// was a FormatException. .NET accepts the sign grammatically and fails later, in
462+
// TryNumberBufferToBinaryInteger's `(!TInteger.IsSigned && number.IsNegative)`
463+
// (Number.Parsing.cs:157, and again at :369) -- which is an OverflowException.
464+
//
465+
// THE FAMILY HAD TO MOVE TOGETHER, because there are TWO rows and they disagree:
466+
//
467+
// UInt32::Parse("-1") .NET: OverflowException here, before: FormatException
468+
// UInt32::Parse("-0") .NET: 0 here, before: FormatException
469+
// UInt32::Parse("-0.0") .NET: OverflowException here, before: FormatException
470+
//
471+
// "-0" does not fail at all, because Number.Parsing.Common.cs:259-268 clears IsNegative when
472+
// StateNonZero was never set and no decimal separator was seen. Repairing only that row
473+
// would have aligned one spelling and left "-1" diverging, which is worse than the old
474+
// consistent rule -- so the grammar, the normalisation and the rejection all land together.
475+
//
476+
// A literal '+' was already accepted, matching .NET.
452477
static bool TryParseUnsignedCore(const std::string& s, NumberStyles style,
453478
SharpRuntime::ulongcs& result, bool& overflowed) {
454479
overflowed = false;
@@ -475,19 +500,21 @@ struct IntegerNumberStylesParser {
475500
// consumed a sign" check, so e.g. UInt32::TryParse("++5", NumberStyles::Integer, ...)
476501
// incorrectly returned true with result 5, and "5++" (multiple trailing signs) was
477502
// likewise wrongly accepted -- confirmed via a standalone repro before this fix.
478-
bool haveSign = false, haveCurrency = false;
503+
bool haveSign = false, haveCurrency = false, haveParen = false, negative = false;
479504
for (bool matched = true; matched; ) {
480505
matched = false;
481506
if (allowLeadingWhite && i < n && std::isspace(static_cast<unsigned char>(s[i]))) {
482507
while (i < n && std::isspace(static_cast<unsigned char>(s[i]))) ++i;
483508
matched = true;
484-
} else if (allowLeadingSign && !haveSign && i < n && s[i] == '+') { haveSign = true; ++i; matched = true; }
485-
else if (allowCurrency && !haveCurrency &&
509+
} else if (allowLeadingSign && !haveSign && i < n && (s[i] == '+' || s[i] == '-')) {
510+
negative = (s[i] == '-'); haveSign = true; ++i; matched = true;
511+
} else if (allowParens && !haveSign && i < n && s[i] == '(') {
512+
haveParen = true; haveSign = true; negative = true; ++i; matched = true;
513+
} else if (allowCurrency && !haveCurrency &&
486514
s.compare(i, kCurrencySymbol.size(), kCurrencySymbol) == 0) {
487515
haveCurrency = true; i += kCurrencySymbol.size(); matched = true;
488516
}
489517
}
490-
if (i < n && (s[i] == '-' || (allowParens && s[i] == '('))) return false;
491518

492519
DigitScan scan;
493520
ScanDigitsAndExponent(s, i, style, scan);
@@ -497,31 +524,43 @@ struct IntegerNumberStylesParser {
497524
while (i < n && std::isspace(static_cast<unsigned char>(s[i]))) ++i;
498525

499526
// Trailing tokens: same interleaved-whitespace treatment as the leading loop above (and
500-
// TryParseSignedCore's trailing loop) -- see those comments for the full rationale. The
501-
// trailing-minus rejection is checked after the loop settles, so it still correctly
502-
// rejects a '-' appearing after a trailing '+'/currency/whitespace has been consumed.
527+
// TryParseSignedCore's trailing loop) -- see those comments for the full rationale.
503528
// `haveSign` is the SAME flag the leading loop above uses, so a sign already consumed on
504529
// either side blocks a second one on the other -- see that loop's comment for why.
505530
for (bool matched = true; matched; ) {
506531
matched = false;
507-
if (allowTrailingSign && !haveSign && i < n && s[i] == '+') { haveSign = true; ++i; matched = true; }
508-
else if (allowCurrency && !haveCurrency &&
532+
if (allowTrailingSign && !haveSign && i < n && (s[i] == '+' || s[i] == '-')) {
533+
negative = (s[i] == '-'); haveSign = true; ++i; matched = true;
534+
} else if (haveParen && i < n && s[i] == ')') {
535+
haveParen = false; ++i; matched = true;
536+
} else if (allowCurrency && !haveCurrency &&
509537
s.compare(i, kCurrencySymbol.size(), kCurrencySymbol) == 0) {
510538
haveCurrency = true; i += kCurrencySymbol.size(); matched = true;
511539
} else if (allowTrailingWhite && i < n && std::isspace(static_cast<unsigned char>(s[i]))) {
512540
while (i < n && std::isspace(static_cast<unsigned char>(s[i]))) ++i;
513541
matched = true;
514542
}
515543
}
516-
if (i < n && s[i] == '-') return false; // trailing minus: same rejection as leading
517-
544+
if (haveParen) return false; // '(' opened but never closed
518545
if (i != n) return false;
519546

520547
// .NET's all-zero normalisation, the same transcription as the signed core above
521-
// (`Number.Parsing.Common.cs:259-268`). Only the scale half applies here: a '-' never
522-
// reaches this point on the unsigned path, which is this parser's own long-standing
523-
// deviation and is documented on TryParseUnsignedCore.
524-
if (scan.digitsCount == 0) scan.scale = 0;
548+
// (`Number.Parsing.Common.cs:259-268`). BOTH halves matter here, and the sign half is
549+
// the whole reason #2362 could not repair one row at a time: it is what makes
550+
// `UInt32::Parse("-0")` a plain 0 while `UInt32::Parse("-0.0")` overflows.
551+
//
552+
// Note for #2356: the `!sawDecimal` guard is UNOBSERVABLE in the signed core -- negating
553+
// zero is zero either way, and that ticket recorded its mutation as not caught. Here it
554+
// is observable, and it is the only thing separating those two rows.
555+
if (scan.digitsCount == 0) {
556+
scan.scale = 0;
557+
if (!scan.sawDecimal) negative = false;
558+
}
559+
560+
// .NET's `(!TInteger.IsSigned && number.IsNegative)` (Number.Parsing.cs:157). It sits in
561+
// the SAME disjunction as the digit-count overflow there, so the two are indistinguishable
562+
// to a caller -- both are OverflowException -- and the order between them here is free.
563+
if (negative) { overflowed = true; return true; }
525564

526565
if (!TryFoldScale(scan)) { overflowed = true; return true; }
527566

0 commit comments

Comments
 (0)