Skip to content

fix: compare large numeric prerelease ids without precision loss#880

Open
spokodev wants to merge 1 commit into
npm:mainfrom
spokodev:fix/compare-large-numeric-prerelease
Open

fix: compare large numeric prerelease ids without precision loss#880
spokodev wants to merge 1 commit into
npm:mainfrom
spokodev:fix/compare-large-numeric-prerelease

Conversation

@spokodev

Copy link
Copy Markdown

Problem

Two valid versions whose prerelease numeric identifiers are >= 2^53 are reported equal and mis-sorted:

semver.compare('1.0.0-9007199254740992', '1.0.0-9007199254740993') // returns 0, should be -1
semver.eq('1.0.0-9007199254740992', '1.0.0-9007199254740993')      // returns true, should be false
semver.gt('1.0.0-9007199254740993', '1.0.0-9007199254740992')      // returns false, should be true

Both versions are valid SemVer (there is no magnitude cap on numeric prerelease identifiers in §9), so this violates SemVer 2.0.0 §11.4.1:

Identifiers consisting of only digits are compared numerically.

9007199254740992 and 9007199254740993 are distinct integers and must order accordingly.

Root cause

classes/semver.js deliberately keeps numeric prerelease ids >= MAX_SAFE_INTEGER as strings to avoid IEEE-754 precision loss in storage:

if (num >= 0 && num < MAX_SAFE_INTEGER) {
  return num
}
return id // kept as string for large ids

But internal/identifiers.js compareIdentifiers re-introduced that loss for any all-digit identifier:

if (anum && bnum) {
  a = +a
  b = +b
}

+'9007199254740992' and +'9007199254740993' both evaluate to the same double (9007199254740992), so the comparison sees them as equal.

Fix

Compare all-digit identifiers with BigInt instead of Number, preserving full integer precision:

if (anum && bnum) {
  a = BigInt(a)
  b = BigInt(b)
}

The already-numberified fast path (typeof a === 'number' && typeof b === 'number', for ids < MAX_SAFE_INTEGER) is untouched, and mixed alphanumeric ordering is unchanged.

Tests

Added a unit case in test/internal/identifiers.js for compareIdentifiers/rcompareIdentifiers with ids beyond 2^53, and a fixture entry in test/fixtures/comparisons.js exercising compare/gt/eq end to end. Both fail on the unpatched code and pass with the fix. Full npm test suite (including lint and 100% coverage) is green.

Numeric prerelease identifiers >= MAX_SAFE_INTEGER (2^53) are kept as
strings by classes/semver.js to avoid IEEE-754 precision loss, but
compareIdentifiers re-introduced that loss by coercing all-digit ids
with Number (`+a`). This collapsed 2^53 and 2^53+1 to the same double,
so compare/eq/gt reported distinct versions as equal, violating
SemVer 2.0.0 §11.4.1 (numeric identifiers compared numerically).

Compare all-digit identifiers with BigInt to preserve full integer
precision. Behavior for normal numeric ids and alphanumeric ids is
unchanged.
@spokodev spokodev requested a review from a team as a code owner June 25, 2026 18:32
@ljharb

ljharb commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

npm doesn't follow semver 2, it follows semver 1, which only says

Precedence SHOULD be determined by lexicographic ASCII sort order.

https://semver.org/spec/v1.0.0.html

so realistically they should always be compared with .localeCompare

@spokodev

Copy link
Copy Markdown
Author

Good point on the lineage, and I should have framed this around behavior rather than a spec number.

Here is why I think it still needs a change. node-semver already orders all-digit identifiers numerically today: compare('1.0.0-beta.2', '1.0.0-beta.11') is -1. Pure lexicographic ASCII would make that +1, so the numeric path is load-bearing, and moving everything to plain .localeCompare would flip established orderings.

For the reported pair the choice does not matter:

  • numeric: 9007199254740992 < 9007199254740993
  • lexicographic ASCII: same result, they match up to the final digit
  • '9007199254740992'.localeCompare('9007199254740993'): -1

All three give -1. The current code returns 0 only because +id rounds both ids to 9007199254740992.

The PR keeps that numeric ordering and only removes the precision loss. If BigInt is unwelcome in this path, I can switch it to a.localeCompare(b, undefined, { numeric: true }), which preserves the exact current semantics (including treating equal numeric values like 01 and 1 as equal) without BigInt. Your call.

@ljharb

ljharb commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

BigInt seems fine if a numeric comparison is what's needed (and already present).

@spokodev

spokodev commented Jul 2, 2026

Copy link
Copy Markdown
Author

Right, that matches the scope here. The PR keeps the existing numeric comparison for all digit identifiers and only changes the parse to BigInt, so identifiers longer than Number.MAX_SAFE_INTEGER compare without precision loss. Non numeric identifiers still fall back to string comparison, so nothing about the lexicographic ordering changes.

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