From bcab95a966413b978dc1e7bdbcb8f495b63303cd Mon Sep 17 00:00:00 2001 From: isaacs Date: Mon, 20 Apr 2020 00:10:10 -0700 Subject: [PATCH] Reduce unnecessary Comparators in Range set There are a few cases that lead to Range's Comparator sets being longer than strictly necessary. This reduces performance of methods that iterate over ranges repeatedly (for example, `intersects` and `subset`), and leads to some confusing toString output like turning `x || * || X` into `||||` instead of `*`. - If any simple range in the set contains the null set <0.0.0-0, then the entire simple range is the null set. `2.x <0.0.0-0` is the same as just `<0.0.0-0`. (This is used for `>*` and `<*`, which cannot match anything.) - Ensure that a given Comparator will only occur once within each simple range set. `2.3.x ^2.3` doesn't need to include `>=2.3.0` more than once. - If a simple range set contains more than one comparator, remove any `*` comparators. `* >=2.3.4` is the same as just `>=2.3.4`. This was already being done in the cast to a string, but some `ANY` Comparators would be left behind in the set used for matching. - If a Range set contains the simple range `*`, then drop any other simple ranges in the set. `* || 2.x` is the same as `*`. There's still some unnecessary comparators in there. For example, the range `2.3 ^2.3.4` parses to `>=2.3.0 <2.4.0-0 >=2.3.4 <3.0.0-0`. Of course, anything that is `<2.4.0-0` is also `<3.0.0-0`, and anything that is `>=2.3.4` is also `>=2.3.0`, so the `<3.0.0-0` and `>=2.3.0` Comparators are not necessary. But simplifying those out would be a bit more work. To do that, we could walk the set of Comparators checking to see if they are a subset of any other Comparators in the list, and if so, removing them. The subset check would not have to be a full Range.subset(); we could just see if the gtlt points in the same direction, and if one semver is greater than the other. It's an O(n^2) operation, but one on typically very small n. PR-URL: https://github.com/npm/node-semver/pull/324 Credit: @isaacs Close: #324 --- classes/range.js | 39 ++++++++++++++++++++++++++++++++++- test/fixtures/range-parse.js | 6 ++++-- test/ranges/to-comparators.js | 8 +++++-- 3 files changed, 48 insertions(+), 5 deletions(-) diff --git a/classes/range.js b/classes/range.js index 83f89677..c1f5be88 100644 --- a/classes/range.js +++ b/classes/range.js @@ -46,6 +46,24 @@ class Range { throw new TypeError(`Invalid SemVer Range: ${range}`) } + // if we have any that are not the null set, throw out null sets. + if (this.set.length > 1) { + // keep the first one, in case they're all null sets + const first = this.set[0] + this.set = this.set.filter(c => !isNullSet(c[0])) + if (this.set.length === 0) + this.set = [first] + else if (this.set.length > 1) { + // if we have any that are *, then the range is just * + for (const c of this.set) { + if (c.length === 1 && isAny(c[0])) { + this.set = [c] + break + } + } + } + } + this.format() } @@ -87,15 +105,31 @@ class Range { // ready to be split into comparators. const compRe = loose ? re[t.COMPARATORLOOSE] : re[t.COMPARATOR] - return range + const rangeList = range .split(' ') .map(comp => parseComparator(comp, this.options)) .join(' ') .split(/\s+/) + // >=0.0.0 is equivalent to * .map(comp => replaceGTE0(comp, this.options)) // in loose mode, throw out any that are not valid comparators .filter(this.options.loose ? comp => !!comp.match(compRe) : () => true) .map(comp => new Comparator(comp, this.options)) + + // if any comparators are the null set, then replace with JUST null set + // if more than one comparator, remove any * comparators + // also, don't include the same comparator more than once + const l = rangeList.length + const rangeMap = new Map() + for (const comp of rangeList) { + if (isNullSet(comp)) + return [comp] + rangeMap.set(comp.value, comp) + } + if (rangeMap.size > 1 && rangeMap.has('')) + rangeMap.delete('') + + return [...rangeMap.values()] } intersects (range, options) { @@ -155,6 +189,9 @@ const { caretTrimReplace } = require('../internal/re') +const isNullSet = c => c.value === '<0.0.0-0' +const isAny = c => c.value === '' + // take a set of comparators and determine whether there // exists a version which can satisfy it const isSatisfiable = (comparators, options) => { diff --git a/test/fixtures/range-parse.js b/test/fixtures/range-parse.js index 42414903..7ae23f14 100644 --- a/test/fixtures/range-parse.js +++ b/test/fixtures/range-parse.js @@ -38,7 +38,7 @@ module.exports = [ ['>=0.2.3 || <0.0.1', '>=0.2.3||<0.0.1'], ['>=0.2.3 || <0.0.1', '>=0.2.3||<0.0.1'], ['>=0.2.3 || <0.0.1', '>=0.2.3||<0.0.1'], - ['||', '||'], + ['||', '*'], ['2.x.x', '>=2.0.0 <3.0.0-0'], ['1.2.x', '>=1.2.0 <1.3.0-0'], ['1.2.x || 2.x', '>=1.2.0 <1.3.0-0||>=2.0.0 <3.0.0-0'], @@ -79,7 +79,7 @@ module.exports = [ ['>01.02.03', null], ['~1.2.3beta', '>=1.2.3-beta <1.3.0-0', { loose: true }], ['~1.2.3beta', null], - ['^ 1.2 ^ 1', '>=1.2.0 <2.0.0-0 >=1.0.0 <2.0.0-0'], + ['^ 1.2 ^ 1', '>=1.2.0 <2.0.0-0 >=1.0.0'], ['1.2 - 3.4.5', '>=1.2.0 <=3.4.5'], ['1.2.3 - 3.4', '>=1.2.3 <3.5.0-0'], ['1.2 - 3.4', '>=1.2.0 <3.5.0-0'], @@ -87,4 +87,6 @@ module.exports = [ ['>1.2', '>=1.3.0'], ['>X', '<0.0.0-0'], ['* 2.x', '<0.0.0-0'], + ['>x 2.x || * || { ['>=0.2.3 || <0.0.1', [['>=0.2.3'], ['<0.0.1']]], ['>=0.2.3 || <0.0.1', [['>=0.2.3'], ['<0.0.1']]], ['>=0.2.3 || <0.0.1', [['>=0.2.3'], ['<0.0.1']]], - ['||', [[''], ['']]], + ['||', [['']]], ['2.x.x', [['>=2.0.0', '<3.0.0-0']]], ['1.2.x', [['>=1.2.0', '<1.3.0-0']]], ['1.2.x || 2.x', [['>=1.2.0', '<1.3.0-0'], ['>=2.0.0', '<3.0.0-0']]], @@ -72,7 +72,11 @@ test('comparators test', (t) => { ['1.2.3 - 3.4', [['>=1.2.3', '<3.5.0-0']]], ['1.2.3 - 3', [['>=1.2.3', '<4.0.0-0']]], ['>*', [['<0.0.0-0']]], - ['<*', [['<0.0.0-0']]] + ['<*', [['<0.0.0-0']]], + ['>X', [['<0.0.0-0']]], + ['* 2.x', [['<0.0.0-0']]], + ['>x 2.x || * || { const found = toComparators(pre) const jw = JSON.stringify(wanted)