Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correctly handle prereleases/ANY ranges in subset #377

Merged
merged 2 commits into from
Mar 23, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 67 additions & 7 deletions ranges/subset.js
Original file line number Diff line number Diff line change
@@ -1,20 +1,28 @@
const Range = require('../classes/range.js')
const { ANY } = require('../classes/comparator.js')
const Comparator = require('../classes/comparator.js')
const { ANY } = Comparator
const satisfies = require('../functions/satisfies.js')
const compare = require('../functions/compare.js')

// Complex range `r1 || r2 || ...` is a subset of `R1 || R2 || ...` iff:
// - Every simple range `r1, r2, ...` is a subset of some `R1, R2, ...`
// - Every simple range `r1, r2, ...` is a null set, OR
// - Every simple range `r1, r2, ...` which is not a null set is a subset of
// some `R1, R2, ...`
//
// Simple range `c1 c2 ...` is a subset of simple range `C1 C2 ...` iff:
// - If c is only the ANY comparator
// - If C is only the ANY comparator, return true
// - Else return false
// - Else if in prerelease mode, return false
// - else replace c with `[>=0.0.0]`
// - If C is only the ANY comparator
// - if in prerelease mode, return true
// - else replace C with `[>=0.0.0]`
// - Let EQ be the set of = comparators in c
// - If EQ is more than one, return true (null set)
// - Let GT be the highest > or >= comparator in c
// - Let LT be the lowest < or <= comparator in c
// - If GT and LT, and GT.semver > LT.semver, return true (null set)
// - If any C is a = range, and GT or LT are set, return false
// - If EQ
// - If GT, and EQ does not satisfy GT, return true (null set)
// - If LT, and EQ does not satisfy LT, return true (null set)
Expand All @@ -23,13 +31,16 @@ const compare = require('../functions/compare.js')
// - If GT
// - If GT.semver is lower than any > or >= comp in C, return false
// - If GT is >=, and GT.semver does not satisfy every C, return false
// - If GT.semver has a prerelease, and not in prerelease mode
// - If no C has a prerelease and the GT.semver tuple, return false
// - If LT
// - If LT.semver is greater than any < or <= comp in C, return false
// - If LT is <=, and LT.semver does not satisfy every C, return false
// - If any C is a = range, and GT or LT are set, return false
// - If GT.semver has a prerelease, and not in prerelease mode
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be the following (could be wrong):

Suggested change
// - If GT.semver has a prerelease, and not in prerelease mode
// - If LT.semver has a prerelease, and not in prerelease mode

// - If no C has a prerelease and the LT.semver tuple, return false
// - Else return true

const subset = (sub, dom, options) => {
const subset = (sub, dom, options = {}) => {
if (sub === dom)
return true

Expand Down Expand Up @@ -58,8 +69,21 @@ const simpleSubset = (sub, dom, options) => {
if (sub === dom)
return true

if (sub.length === 1 && sub[0].semver === ANY)
return dom.length === 1 && dom[0].semver === ANY
if (sub.length === 1 && sub[0].semver === ANY) {
if (dom.length === 1 && dom[0].semver === ANY)
return true
else if (options.includePrerelease)
sub = [ new Comparator('>=0.0.0-0') ]
else
sub = [ new Comparator('>=0.0.0') ]
}

if (dom.length === 1 && dom[0].semver === ANY) {
if (options.includePrerelease)
return true
else
dom = [ new Comparator('>=0.0.0') ]
}

const eqSet = new Set()
let gt, lt
Expand Down Expand Up @@ -102,10 +126,32 @@ const simpleSubset = (sub, dom, options) => {

let higher, lower
let hasDomLT, hasDomGT
// if the subset has a prerelease, we need a comparator in the superset
// with the same tuple and a prerelease, or it's not a subset
let needDomLTPre = lt &&
!options.includePrerelease &&
lt.semver.prerelease.length ? lt.semver : false
let needDomGTPre = gt &&
!options.includePrerelease &&
gt.semver.prerelease.length ? gt.semver : false
// exception: <1.2.3-0 is the same as <1.2.3
if (needDomLTPre && needDomLTPre.prerelease.length === 1 &&
lt.operator === '<' && needDomLTPre.prerelease[0] === 0) {
needDomLTPre = false
}

for (const c of dom) {
hasDomGT = hasDomGT || c.operator === '>' || c.operator === '>='
hasDomLT = hasDomLT || c.operator === '<' || c.operator === '<='
if (gt) {
if (needDomGTPre) {
if (c.semver.prerelease && c.semver.prerelease.length &&
c.semver.major === needDomGTPre.major &&
c.semver.minor === needDomGTPre.minor &&
c.semver.patch === needDomGTPre.patch) {
needDomGTPre = false
}
}
if (c.operator === '>' || c.operator === '>=') {
higher = higherGT(gt, c, options)
if (higher === c && higher !== gt)
Expand All @@ -114,6 +160,14 @@ const simpleSubset = (sub, dom, options) => {
return false
}
if (lt) {
if (needDomLTPre) {
if (c.semver.prerelease && c.semver.prerelease.length &&
c.semver.major === needDomLTPre.major &&
c.semver.minor === needDomLTPre.minor &&
c.semver.patch === needDomLTPre.patch) {
needDomLTPre = false
}
}
if (c.operator === '<' || c.operator === '<=') {
lower = lowerLT(lt, c, options)
if (lower === c && lower !== lt)
Expand All @@ -134,6 +188,12 @@ const simpleSubset = (sub, dom, options) => {
if (lt && hasDomGT && !gt && gtltComp !== 0)
return false

// we needed a prerelease range in a specific tuple, but didn't get one
// then this isn't a subset. eg >=1.2.3-pre is not a subset of >=1.0.0,
// because it includes prereleases in the 1.2.3 tuple
if (needDomGTPre || needDomLTPre)
return false

return true
}

Expand Down
35 changes: 32 additions & 3 deletions test/ranges/subset.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,29 @@ const cases = [
['>2 <1', '3', true],
['1 || 2 || 3', '>=1.0.0', true],

// everything is a subset of *
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment should probably say something new, since this is not always true 😅

['1.2.3', '*', true],
['^1.2.3', '*', true],
['^1.2.3-pre.0', '*', false],
['^1.2.3-pre.0', '*', true, { includePrerelease: true }],
['1 || 2 || 3', '*', true],

// prerelease edge cases
['^1.2.3-pre.0', '>=1.0.0', false],
['^1.2.3-pre.0', '>=1.0.0', true, { includePrerelease: true }],
['^1.2.3-pre.0', '>=1.2.3-pre.0', true],
['^1.2.3-pre.0', '>=1.2.3-pre.0', true, { includePrerelease: true }],
['>1.2.3-pre.0', '>=1.2.3-pre.0', true],
['>1.2.3-pre.0', '>1.2.3-pre.0 || 2', true],
['1 >1.2.3-pre.0', '>1.2.3-pre.0', true],
['1 <=1.2.3-pre.0', '>=1.0.0-0', false],
['1 <=1.2.3-pre.0', '>=1.0.0-0', true, { includePrerelease: true }],
['1 <=1.2.3-pre.0', '<=1.2.3-pre.0', true],
['1 <=1.2.3-pre.0', '<=1.2.3-pre.0', true, { includePrerelease: true }],
['<1.2.3-pre.0', '<=1.2.3-pre.0', true],
['<1.2.3-pre.0', '<1.2.3-pre.0 || 2', true],
['1 <1.2.3-pre.0', '<1.2.3-pre.0', true],

['*', '*', true],
['', '*', true],
['*', '', true],
Expand All @@ -23,9 +46,16 @@ const cases = [
// >=0.0.0 is like * in non-prerelease mode
// >=0.0.0-0 is like * in prerelease mode
['*', '>=0.0.0-0', true, { includePrerelease: true }],

// true because these are identical in non-PR mode
['*', '>=0.0.0', true],

// false because * includes 0.0.0-0 in PR mode
['*', '>=0.0.0', false, { includePrerelease: true }],
['*', '>=0.0.0-0', false],

// true because * doesn't include 0.0.0-0 in non-PR mode
['*', '>=0.0.0-0', true],

['^2 || ^3 || ^4', '>=1', true],
['^2 || ^3 || ^4', '>1', true],
['^2 || ^3 || ^4', '>=2', true],
Expand Down Expand Up @@ -73,9 +103,8 @@ const cases = [
['>2.0.0', '>=2.0.0', true],
]


t.plan(cases.length + 1)
cases.forEach(([sub, dom, expect, options = {}]) => {
cases.forEach(([sub, dom, expect, options]) => {
const msg = `${sub || "''"} ⊂ ${dom || "''"} = ${expect}` +
(options ? ' ' + Object.keys(options).join(',') : '')
t.equal(subset(sub, dom, options), expect, msg)
Expand Down