Skip to content

Fix TryParsePartial under-reporting charsConsumed for leading whitespace - #132270

Merged
tannergooding merged 2 commits into
mainfrom
copilot/fix-tryparsepartial-charsconsumed
Aug 13, 2026
Merged

Fix TryParsePartial under-reporting charsConsumed for leading whitespace#132270
tannergooding merged 2 commits into
mainfrom
copilot/fix-tryparsepartial-charsconsumed

Conversation

Copilot AI commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

INumberBase<T>.TryParsePartial omitted leading whitespace from charsConsumed whenever the format provider's signs are not exactly "+"/"-" (sv-SE, fi-FI, Arabic/Hebrew locales, ~96 of 786 cultures) and the style allows both leading whitespace and a sign, as NumberStyles.Integer does. Callers advancing by the count rewind into the number they already read:

int.TryParsePartial(" 5", NumberStyles.Integer, new CultureInfo("sv-SE"), out int v, out int consumed);
// v = 5, consumed = 1 — expected 2

Cause

In Number.TryParseBinaryIntegerStyle, the non-invariant sign branch reassigned value = value.Slice(index) and reset index = 0 so the culture's sign could be matched with StartsWith. That discarded the whitespace offset, leaving the final elementsConsumed = index relative to the sliced span. The branch is entered even when no sign is present, so any input with leading whitespace under such a culture is affected — for every binary integer type, since they share this path.

Changes

  • Number.Parsing.cs: slice into a local remaining span used only for the sign StartsWith checks, keeping value and index in the original coordinate space. The remaining uses of value (value[index], value.Length, ConsumeTrailingNulls, the trailing-whitespace loop, InvalidExit's index == value.Length) are shift-equivalent, so behavior is unchanged apart from the count. The non-partial callers (Enum, Version) discard elementsConsumed.
  • Int32Tests / UInt32Tests: added TryParsePartial theory rows using a NumberFormatInfo with NegativeSign = "\u2212" (the sv-SE case) — leading whitespace alone, with +, with the multi-byte negative sign, and combined with trailing whitespace. These fail without the product change.

Adjacent, not addressed here

Number.TryParseBigIntegerNumber leaves elementsConsumed non-zero when NumberToBigInteger fails after a successful TryStringToNumber, so BigInteger.TryParsePartial("1.5", NumberStyles.Float, ...) returns false with charsConsumed = 3 rather than 0. Pre-existing and a separate defect; left for its own change.

Copilot AI lite review requested due to automatic review settings August 13, 2026 10:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot wasn't able to review any files in this pull request.

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
16 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

…pace

Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 13, 2026 11:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/libraries/System.Private.CoreLib/src/System/Number.Parsing.cs:385

  • Comment says "Slice a copy" but ReadOnlySpan<T>.Slice doesn't copy data; it creates another span over the same memory. Consider rewording to avoid implying an allocation/copy while preserving the intent of keeping index relative to the original input.
                    // Slice a copy rather than reassigning value, so that index (and thus the number
                    // of elements reported as consumed) stays relative to the original input.
                    ReadOnlySpan<TChar> remaining = value.Slice(index);

@EgorBo
EgorBo marked this pull request as ready for review August 13, 2026 11:35
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI changed the title [WIP] Fix charsConsumed reporting in TryParsePartial for cultures with whitespace Fix TryParsePartial under-reporting charsConsumed for leading whitespace Aug 13, 2026
Copilot AI requested a review from EgorBo August 13, 2026 11:39
@EgorBo
EgorBo requested a review from tannergooding August 13, 2026 11:43
@EgorBo

EgorBo commented Aug 13, 2026

Copy link
Copy Markdown
Member

PTAL @tannergooding, .NET 11.0 only issue, not-OOB related, just incorrect value returned.

@tannergooding

Copy link
Copy Markdown
Member

/ba-g #132030 and an "artifact already exists" failure

@tannergooding
tannergooding merged commit 57fe570 into main Aug 13, 2026
124 of 127 checks passed
@tannergooding
tannergooding deleted the copilot/fix-tryparsepartial-charsconsumed branch August 13, 2026 22:56
@tannergooding

Copy link
Copy Markdown
Member

@EgorBo #132278 is the fix for the BigInteger one that copilot called out (where the consumed count wasn't 0 on failure)

@EgorBo

EgorBo commented Aug 13, 2026

Copy link
Copy Markdown
Member

@EgorBo #132278 is the fix for the BigInteger one that copilot called out (where the consumed count wasn't 0 on failure)

Ah, I missed that. Do we have a contract what should happen in such case (when Try* returns false) or it's fine for it to be UB value? (I think we have a few APIs like that).

@tannergooding

Copy link
Copy Markdown
Member

The contract, particularly for mainstream APIs, is that it should return 0

We do have a few outliers that are allowed to return non-zero values, but they tend to be rare/specialized. We typically expect it to be some OperationStatus API if its allowed to do partial results like that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TryParsePartial under-reports charsConsumed when the input has leading whitespace

4 participants