Skip to content

Compare oversized numeric prerelease identifiers numerically - #315

Open
jackwalkerlabs wants to merge 1 commit into
Masterminds:masterfrom
jackwalkerlabs:fix/large-numeric-prerelease
Open

jackwalkerlabs wants to merge 1 commit into
Masterminds:masterfrom
jackwalkerlabs:fix/large-numeric-prerelease

Conversation

@jackwalkerlabs

Copy link
Copy Markdown

Valid numeric prerelease identifiers can exceed uint64. Compare currently treats those identifiers as alphanumeric after strconv.ParseUint returns a range error, producing incorrect ordering both against other large numbers and against alphanumeric identifiers.

Distinguish range errors from nonnumeric input. Keep the existing numeric fast path; for larger numeric identifiers compare digit counts and then digits, as valid numeric identifiers have no leading zeroes. Regressions cover both comparison directions, nested prerelease identifiers, the uint64 boundary, and numeric-versus-alphanumeric precedence.

This follows SemVer precedence rule 11. The large-number ordering cases fail on the base revision.

Validation: go test -race ./..., go vet ./..., gofmt and git diff --check pass on Go 1.27.1 / macOS arm64.

Disclosure: Codex implemented and tested this change under the submitting account's authorization.

@kimjooyoon kimjooyoon left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reviewed 6b70f07c0514add8239075b083a3f06b39d137bd on Go 1.26.9 / darwin-arm64. The existing suite passes before adding the probe, but four new comparison pairs regress relative to the base. The same ten-pair probe gives base 8/10, this PR 6/10, and #323 10/10; #323 also passes the full suite with -race including the probe.

Reproduction and original logs include exact source bindings and a standalone test file. This is local evidence, not hosted CI or a full Go-version matrix. Prepared with Codex.

Comment thread version.go

oi, n1 := strconv.ParseUint(o, 10, 64)
si, n2 := strconv.ParseUint(s, 10, 64)
oNumeric := n1 == nil || errors.Is(n1, strconv.ErrRange)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[P2] Classify the whole identifier before accepting a range error

ParseUint can overflow on the initial digits before it reaches a later letter or hyphen. On Go 1.26.9, strconv.ParseUint("18446744073709551616a", 10, 64) returns ErrRange, so these checks classify a valid alphanumeric identifier as numeric.

left, _ := StrictNewVersion("1.0.0-18446744073709551616a")
right, _ := StrictNewVersion("1.0.0-1000000000000000000000")
left.Compare(right) // this head: -1; expected: +1

Both versions parse successfully. SemVer 11.4.3 places the numeric identifier below the nonnumeric one. The base returns +1; this head reverses it, and the reverse comparison is wrong too. Please check that the entire identifier consists of ASCII digits before taking the numeric branch. The approach already in #323 handles this boundary; the linked probe also covers a hyphen suffix, two alphanumeric identifiers and a nested case.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants