Conversation
| // No single dom range covers this sub range, but the union of | ||
| // multiple dom ranges might. Only attempt this when there are | ||
| // multiple OR branches in the dom. | ||
| if (dom.set.length > 1 && unionSubset(simpleSub, dom, options)) { |
There was a problem hiding this comment.
This bypasses the prerelease-tuple admission rule that simpleSubset enforces above. So subset('>=1.0.0-pre', '<2.0.0 || >=1.0.0') will return true in non-includePrerelease mode
There was a problem hiding this comment.
Good catch, confirmed: subset('>=1.0.0-pre', '<2.0.0 || >=1.0.0') returned true, but 1.0.0-pre satisfies the sub and neither dom branch. Same for a prerelease upper bound (<2.0.0-pre vs <1.0.0 || >=0.5.0 <3.0.0).
Fixed in 77cafb6. Outside of includePrerelease mode a branch only admits the prereleases of a tuple one of its own comparators names, so branches aren't contiguous around prereleases and the interval sweep can't model that. unionSubset now leaves a sub with a prerelease bound to simpleSubset, which still applies the admission rule per branch. <X.Y.Z-0 keeps the existing exception (it's the same as <X.Y.Z). With includePrerelease, prereleases are ordinary versions and the union path still applies.
| ['>=1.0.0 <3.0.0', '1.0.0 || 2.0.0', false], // dom is all eq-sets | ||
| ['>=0.0.0', '<2.0.0 || >=1.0.0', true], // dom branch with -infinity lower | ||
| ['>=1.0.0 <10.0.0', '>=1.0.0 <3.0.0 || >=5.0.0 <7.0.0 || >=9.0.0', false], // gap | ||
| ['>=0.0.0-0', '* || >=1.0.0', true, { includePrerelease: true }], |
There was a problem hiding this comment.
Adding negative tests here for the prerelease lower/upper bounds in non-includePrerelease mode would be great to make sure a regression doesn't happen.
There was a problem hiding this comment.
Added in 77cafb6:
- negative cases for prerelease lower and upper bounds (
>=/>/</<=) in non-includePrereleasemode; - positive controls: one branch that names the tuple,
<X.Y.Z-0, and the same input withincludePrerelease.
The new tests fail on the previous head and pass now. I also ran a differential check: random sub/dom pairs (including prerelease bounds, = branches and includePrerelease), comparing subset() against satisfies() over a dense set of versions. It showed no false positives and no false negatives on this branch, and it did catch both issues from this review on the previous head.
| for (const simpleDom of dom.set) { | ||
| const b = extractBounds(simpleDom, options) | ||
| if (b) { | ||
| domIntervals.push(b) |
There was a problem hiding this comment.
eq-only branches are dropped here, so a singleton like 2.0.0 can't bridge <2.0.0 and >2.0.0 in the union
There was a problem hiding this comment.
Right, a lone = branch was dropped. Fixed in 77cafb6: a branch that is a single = comparator is now the interval [V, V]. So >=1.0.0 <=3.0.0 ⊂ >=1.0.0 <2.0.0 || 2.0.0 || >2.0.0 <=3.0.0 and >=1.0.0 ⊂ <2.0.0 || 2.0.0 || >2.0.0 are now true, and 2.5.0 in the same spot doesn't bridge (test included). An = combined with other comparators is still left to simpleSubset.
While testing this I found one more gap and fixed it in the same commit: a dom branch that ends before the sub starts stopped the sweep, so >=1.5.0 ⊂ <1.0.0 || >=1.0.0 <2.0.0 || >=2.0.0 was false. Such branches are now skipped.
I also rebased onto current main, which picks up #867. npm test passes with 100% coverage.
- Do not use the union path for a sub range with a prerelease bound
outside of includePrerelease mode. A prerelease bound admits
prereleases in its tuple, which the interval sweep cannot account
for, so `subset('>=1.0.0-pre', '<2.0.0 || >=1.0.0')` returned true
although 1.0.0-pre is not in the dom. Such ranges are still checked
by simpleSubset, per branch.
- Treat a dom branch that is a single = comparator as the interval
[V, V], so that `2.0.0` bridges `<2.0.0` and `>2.0.0`.
- Skip dom intervals that end before the sub range starts instead of
stopping at them, eg the `<1.0.0` branch in
`<1.0.0 || >=1.0.0 <2.0.0 || >=2.0.0` for `>=1.5.0`.
- Add negative tests for prerelease lower and upper bounds outside of
includePrerelease mode.
e4cebfa to
77cafb6
Compare
Summary
Fixes #703
subset('>=17.2.0', '^17.2.0 || >17')incorrectly returnsfalse.The root cause is that
subset()checks each OR-branch of the dom range independently. No single branch of^17.2.0 || >17covers>=17.2.0on its own, but their union does:Approach
When no single dom OR-branch covers a sub comparator set, fall back to an interval-sweep algorithm:
[lower, upper)bounds from the sub and every dom OR-branchKey details:
<18.0.0-0and>=18.0.0are treated as adjacent in non-prerelease mode (no release version exists between them)>5.0.0 <3.0.0) are detected and skipped*/ ANY bounds are handled as[-∞, +∞)domhas multiple OR-branchesTest plan
subset('>=17.2.0', '^17.2.0 || >17')now returnstrue*ranges, inclusive/exclusive bounds,includePrereleaseoption