Fix #1454: hyparquet-floor-pin: dedupe check reddens on compound/OR/x-range declarations npm would dedupe cleanly

src/core/semver.js

@@ -1,64 +1,32 @@
 // @ts-check
 
 /**
- * Minimal semver range matcher for the small set of operators Phase 1
- * exercises: `^X.Y.Z`, `~X.Y.Z`, `>=`, `>`, `<=`, `<`, exact, and `*`.
- * Plugin manifests don't use the full npm grammar (no `||`, no
- * hyphen ranges, no pre-release fences), and we don't want a fresh
+ * Minimal semver range matcher for the npm range grammar this repo meets:
+ * `^X.Y.Z`, `~X.Y.Z`, `>=`, `>`, `<=`, `<`, exact and `*`, plus the range set
+ * built out of them - `||` alternatives, space-separated compound ranges,
+ * `x`-ranges (`1.29.x`, `1.29`, `1.x`) and hyphen ranges (`1.2.3 - 2.0.0`).
+ * A shape outside that grammar answers `false` rather than guessing, so a
+ * caller gating on the answer stays conservative, and we don't want a fresh
  * runtime dependency on `semver` just to cover what we use.
  *
+ * Pre-release and build metadata parse but do not order; nothing here ships
+ * one.
+ *
  * @param {string} version
  * @param {string|undefined} range
  * @returns {boolean}
  */
 export function matchesSemverRange(version, range) {
   if (range === undefined || range === null) return true
   const trimmed = String(range).trim()
-  if (trimmed === '' || trimmed === '*' || trimmed === 'x' || trimmed === 'X') return true
+  if (isWildcard(trimmed)) return true
   const v = parseSemver(version)
   if (!v) return false
-
-  if (trimmed.startsWith('^')) {
-    const r = parseSemver(trimmed.slice(1))
-    if (!r) return false
-    if (r.major === 0) {
-      if (r.minor === 0) {
-        return v.major === 0 && v.minor === 0 && v.patch === r.patch
-      }
-      return v.major === 0 && v.minor === r.minor && cmp(v, r) >= 0
-    }
-    return v.major === r.major && cmp(v, r) >= 0
-  }
-
-  if (trimmed.startsWith('~')) {
-    const r = parseSemver(trimmed.slice(1))
-    if (!r) return false
-    return v.major === r.major && v.minor === r.minor && cmp(v, r) >= 0
-  }
-
-  if (trimmed.startsWith('>=')) {
-    const r = parseSemver(trimmed.slice(2))
-    return r ? cmp(v, r) >= 0 : false
-  }
-  if (trimmed.startsWith('<=')) {
-    const r = parseSemver(trimmed.slice(2))
-    return r ? cmp(v, r) <= 0 : false
+  for (const alternative of trimmed.split('||')) {
+    const comparators = comparatorsOf(alternative)
+    if (comparators && comparators.every(c => satisfies(v, c))) return true
   }
-  if (trimmed.startsWith('>')) {
-    const r = parseSemver(trimmed.slice(1))
-    return r ? cmp(v, r) > 0 : false
-  }
-  if (trimmed.startsWith('<')) {
-    const r = parseSemver(trimmed.slice(1))
-    return r ? cmp(v, r) < 0 : false
-  }
-  if (trimmed.startsWith('=')) {
-    const r = parseSemver(trimmed.slice(1))
-    return r ? cmp(v, r) === 0 : false
-  }
-
-  const r = parseSemver(trimmed)
-  return r ? cmp(v, r) === 0 : false
+  return false
 }
 
 /**
@@ -74,11 +42,10 @@ export function isValidSemver(version) {
 }
 
 /**
- * True when `range` is a range this matcher understands: `*`/`x`, an
- * exact/`=` version, or one of the `^ ~ >= <= > <` operators applied to
- * a parseable `X.Y.Z`. Mirrors the operator set in `matchesSemverRange`
- * so the doctor rejects manifest `hypaware_api` ranges the kernel could
- * never satisfy.
+ * True when `range` is a range this matcher understands. Mirrors the grammar
+ * `matchesSemverRange` reads, so the doctor rejects manifest `hypaware_api`
+ * ranges the kernel could never satisfy, and a caller that reports "cannot
+ * judge" reports it for exactly the shapes the matcher cannot judge.
  *
  * @param {unknown} range
  * @returns {range is string}
@@ -87,15 +54,148 @@ export function isValidRange(range) {
   if (typeof range !== 'string') return false
   const trimmed = range.trim()
   if (trimmed === '') return false
-  if (trimmed === '*' || trimmed === 'x' || trimmed === 'X') return true
-  const body = /^[\^~]/.test(trimmed)
-    ? trimmed.slice(1)
-    : /^(>=|<=)/.test(trimmed)
-      ? trimmed.slice(2)
-      : /^[<>=]/.test(trimmed)
-        ? trimmed.slice(1)
-        : trimmed
-  return parseSemver(body) !== null
+  if (isWildcard(trimmed)) return true
+  return trimmed.split('||').every(alternative => comparatorsOf(alternative) !== null)
+}
+
+/**
+ * @param {string} range
+ * @returns {boolean}
+ */
+function isWildcard(range) {
+  return range === '' || range === '*' || range === 'x' || range === 'X'
+}
+
+/** One simple range: an optional operator and a possibly partial version. */
+const SIMPLE = /^(>=|<=|>|<|=|\^|~)?v?(\d+|[xX*])(?:\.(\d+|[xX*])(?:\.(\d+|[xX*])(?:[-+][\w.-]+)?)?)?$/
+
+/**
+ * Every primitive comparator a `||` branch expands to, or null when any part
+ * of it is a shape this matcher does not know. An empty array is a branch that
+ * admits everything (`*`).
+ *
+ * @param {string} alternative
+ * @returns {{ op: string, v: { major: number, minor: number, patch: number } }[] | null}
+ */
+function comparatorsOf(alternative) {
+  const trimmed = alternative.trim()
+  if (trimmed === '') return null
+  // `>= 1.2.3` is one comparator, not two tokens, so the space after an
+  // operator closes up before the compound range splits on whitespace.
+  const parts = trimmed.replace(/([<>=^~]+)\s+/g, '$1').split(/\s+/)
+  if (parts.length === 3 && parts[1] === '-') return hyphenComparators(parts[0], parts[2])
+  const comparators = []
+  for (const part of parts) {
+    const simple = simpleComparators(part)
+    if (simple === null) return null
+    comparators.push(...simple)
+  }
+  return comparators
+}
+
+/**
+ * @param {string} part one simple range
+ * @returns {{ op: string, v: { major: number, minor: number, patch: number } }[] | null}
+ */
+function simpleComparators(part) {
+  const p = parseSimple(part)
+  if (!p) return null
+  // An `x` in the major position names no version. npm reads that as `*` for
+  // the bare and caret/tilde forms; a relational operator has nothing to
+  // compare against, so it is a shape this matcher declines to judge.
+  if (p.major === undefined) return p.op === '' || p.op === '=' || p.op === '^' || p.op === '~' ? [] : null
+  const low = { major: p.major, minor: p.minor ?? 0, patch: p.patch ?? 0 }
+  if (p.op === '>=') return [{ op: '>=', v: low }]
+  if (p.op === '<') return [{ op: '<', v: low }]
+  const ceiling = exclusiveCeiling(p.op, p.major, p.minor, p.patch)
+  // A partial version bounds a whole span, so `>1.29` is everything after that
+  // span rather than everything after 1.29.0, and `<=1.29` is everything
+  // before the span ends.
+  if (p.op === '>') return ceiling ? [{ op: '>=', v: ceiling }] : [{ op: '>', v: low }]
+  if (p.op === '<=') return ceiling ? [{ op: '<', v: ceiling }] : [{ op: '<=', v: low }]
+  return ceiling ? [{ op: '>=', v: low }, { op: '<', v: ceiling }] : [{ op: '=', v: low }]
+}
+
+/**
+ * The two comparators a hyphen range expands to. The upper endpoint is
+ * inclusive of everything the partial version names, so `1.2.3 - 2.0` ends
+ * after 2.0.x.
+ *
+ * @param {string} from
+ * @param {string} to
+ * @returns {{ op: string, v: { major: number, minor: number, patch: number } }[] | null}
+ */
+function hyphenComparators(from, to) {
+  const lo = parseSimple(from)
+  const hi = parseSimple(to)
+  if (!lo || !hi || lo.op !== '' || hi.op !== '') return null
+  if (lo.major === undefined || hi.major === undefined) return null
+  const low = { op: '>=', v: { major: lo.major, minor: lo.minor ?? 0, patch: lo.patch ?? 0 } }
+  const ceiling = exclusiveCeiling('', hi.major, hi.minor, hi.patch)
+  if (ceiling) return [low, { op: '<', v: ceiling }]
+  return [low, { op: '<=', v: { major: hi.major, minor: hi.minor ?? 0, patch: hi.patch ?? 0 } }]
+}
+
+/**
+ * The lowest version a simple range excludes above itself, or undefined when
+ * it names one exact version and so bounds nothing on its own.
+ *
+ * @param {string} op
+ * @param {number} major
+ * @param {number|undefined} minor
+ * @param {number|undefined} patch
+ * @returns {{ major: number, minor: number, patch: number }|undefined}
+ */
+function exclusiveCeiling(op, major, minor, patch) {
+  if (op === '^') {
+    if (major !== 0) return { major: major + 1, minor: 0, patch: 0 }
+    if (minor === undefined) return { major: 1, minor: 0, patch: 0 }
+    if (minor !== 0) return { major: 0, minor: minor + 1, patch: 0 }
+    if (patch === undefined) return { major: 0, minor: 1, patch: 0 }
+    return { major: 0, minor: 0, patch: patch + 1 }
+  }
+  if (op === '~') {
+    if (minor === undefined) return { major: major + 1, minor: 0, patch: 0 }
+    return { major, minor: minor + 1, patch: 0 }
+  }
+  if (minor === undefined) return { major: major + 1, minor: 0, patch: 0 }
+  if (patch === undefined) return { major, minor: minor + 1, patch: 0 }
+  return undefined
+}
+
+/**
+ * @param {string} part
+ * @returns {{ op: string, major: number|undefined, minor: number|undefined, patch: number|undefined }|null}
+ */
+function parseSimple(part) {
+  const m = SIMPLE.exec(part)
+  if (!m) return null
+  return { op: m[1] ?? '', major: partNumber(m[2]), minor: partNumber(m[3]), patch: partNumber(m[4]) }
+}
+
+/**
+ * @param {string|undefined} part
+ * @returns {number|undefined} undefined for an absent or wildcard position
+ */
+function partNumber(part) {
+  if (part === undefined || part === 'x' || part === 'X' || part === '*') return undefined
+  return +part
+}
+
+/**
+ * @param {{ major: number, minor: number, patch: number }} v
+ * @param {{ op: string, v: { major: number, minor: number, patch: number } }} c
+ * @returns {boolean}
+ */
+function satisfies(v, c) {
+  const d = cmp(v, c.v)
+  switch (c.op) {
+    case '>=': return d >= 0
+    case '>': return d > 0
+    case '<=': return d <= 0
+    case '<': return d < 0
+    default: return d === 0
+  }
 }
 
 /**

test/core/hyparquet-floor-pin.test.js

@@ -226,6 +226,11 @@ test('a declaration dedupes when the root pin satisfies it, not when it matches
   for (const spec of ['1.29.2', '^1.29.2', '^1.28.2', '~1.29.0', '>=1.28.2', '*']) {
     assert.equal(against(spec), '', `${spec} dedupes onto the root pin`)
   }
+  // The rest of the npm range grammar, which a dependency is free to publish
+  // and npm dedupes exactly as it dedupes the single comparators above.
+  for (const spec of ['>=1.28.0 <2.0.0', '^1.29.0 || ^2.0.0', '1.29.x', '1.29', '1.28.0 - 1.30.0']) {
+    assert.equal(against(spec), '', `${spec} dedupes onto the root pin`)
+  }
   // A declaration the pin cannot satisfy is the failure this check is for, in
   // both directions. One message serves both, so what is asserted is that it
   // carries each remedy beside the direction that wants it: matching only
@@ -236,9 +241,16 @@ test('a declaration dedupes when the root pin satisfies it, not when it matches
     assert.match(against(spec), /ABOVE the root pin, move the ROOT pin up/)
     assert.match(against(spec), /BELOW it wants an `overrides` entry naming 1\.29\.2/)
   }
-  // An unfamiliar range shape says that is what happened rather than claiming
-  // the declaration is out of range.
-  assert.match(against('>=1.28.0 <2.0.0'), /cannot judge/)
+  // The same grammar out of range is still out of range rather than
+  // unjudgeable, so it names a remedy instead of asking to be read by hand.
+  for (const spec of ['>=1.30.0 <2.0.0', '^1.30.0 || ^2.0.0', '1.27.x', '1.26.0 - 1.28.0']) {
+    assert.match(against(spec), /does not satisfy/, `${spec} is out of range for the root pin`)
+  }
+  // A range shape outside that grammar still says so rather than claiming the
+  // declaration is out of range: what the matcher cannot read reddens.
+  for (const spec of ['npm:hyparquet-fork@^1.29.0', 'github:hyparam/hyparquet#main', '>=1.28.0 <garbage']) {
+    assert.match(against(spec), /cannot judge/, `${spec} is not a range this check can read`)
+  }
   // A declaration with nothing in it is unreadable, not satisfied: the matcher
   // answers `true` for an empty or null range, so judging satisfaction first
   // would wave these through as deduped.

test/core/manifest-semver.test.js

@@ -169,3 +169,29 @@ test('matchesSemverRange preserves zero-major caret behavior', () => {
   assert.equal(matchesSemverRange('0.0.3', '^0.0.3'), true)
   assert.equal(matchesSemverRange('0.0.4', '^0.0.3'), false)
 })
+
+test('matchesSemverRange covers compound, alternative, x and hyphen ranges', () => {
+  // A compound range is every comparator at once, an alternative set is any
+  // one of them, and both are shapes an ordinary npm dependency publishes.
+  assert.equal(matchesSemverRange('1.29.2', '>=1.28.0 <2.0.0'), true)
+  assert.equal(matchesSemverRange('2.0.0', '>=1.28.0 <2.0.0'), false)
+  assert.equal(matchesSemverRange('1.29.2', '>= 1.28.0 < 2.0.0'), true)
+  assert.equal(matchesSemverRange('2.1.0', '^1.29.0 || ^2.0.0'), true)
+  assert.equal(matchesSemverRange('3.0.0', '^1.29.0 || ^2.0.0'), false)
+  // An `x` (or an omitted position) stands for the whole span below it, and
+  // bounds a comparator by where that span ends rather than by 0.
+  assert.equal(matchesSemverRange('1.29.9', '1.29.x'), true)
+  assert.equal(matchesSemverRange('1.30.0', '1.29'), false)
+  assert.equal(matchesSemverRange('0.9.0', '0.x'), true)
+  assert.equal(matchesSemverRange('1.0.0', '0.x'), false)
+  assert.equal(matchesSemverRange('1.30.0', '>1.29'), true)
+  assert.equal(matchesSemverRange('1.29.9', '>1.29'), false)
+  assert.equal(matchesSemverRange('1.29.9', '<=1.29'), true)
+  assert.equal(matchesSemverRange('1.2.3', '1.2.3 - 2.0.0'), true)
+  assert.equal(matchesSemverRange('2.0.0', '1.2.3 - 2.0.0'), true)
+  assert.equal(matchesSemverRange('2.0.1', '1.2.3 - 2.0.0'), false)
+  assert.equal(matchesSemverRange('2.0.1', '1.2.3 - 2.0'), true)
+  // A shape outside the grammar is false rather than a guess.
+  assert.equal(matchesSemverRange('1.2.3', 'npm:other@1.2.3'), false)
+  assert.equal(matchesSemverRange('1.2.3', '>=1.2.3 <garbage'), false)
+})

test/core/semver-validity.test.js

@@ -20,9 +20,17 @@ test('isValidRange accepts the operators the kernel matcher understands', () =>
   }
 })
 
+test('isValidRange accepts the rest of the npm range grammar the matcher reads', () => {
+  for (const r of ['1.x', '1.2', '>=1.0.0 <2.0.0', '^1.2.3 || ^2.0.0', '1.2.3 - 2.0.0', '>= 1.0.0']) {
+    assert.equal(isValidRange(r), true, `${r} should be valid`)
+  }
+})
+
 test('isValidRange rejects empty and unparseable ranges', () => {
   assert.equal(isValidRange(''), false)
   assert.equal(isValidRange('^garbage'), false)
-  assert.equal(isValidRange('1.x'), false)
+  assert.equal(isValidRange('npm:hyparquet@1.0.0'), false)
+  assert.equal(isValidRange('>=1.0.0 <garbage'), false)
+  assert.equal(isValidRange('1.0.0 -'), false)
   assert.equal(isValidRange(undefined), false)
 })