Skip to content

Commit

Permalink
Avoid crash in simplifyRanges by removing subsets up front (#6459)
Browse files Browse the repository at this point in the history
## What's the problem this PR addresses?

Resolves #6373. The problem is that `simplifyRanges` doesn't correctly
reduce redundant OR ranges. For example, `~1.0.1 || ~1.0.2` should be
simplified to `~1.0.1`. As the algorithm runs, it will effectively
calculate every _combination_ of terms in such ranges. For example,
given two ranges like `~1.0.1 || ~1.0.2`, the `nextAlternatives` array
will end up with 2*2 = 4 entries; if you have 100 such ranges you'll end
up with 2^100 entries. Growing exponentially like this it's not hard to
crash the process.

Arguably packages should not specify peer deps with this sort of
redundant range, but sometimes they do (I'm working on cleaning up my
project now that I know what the problem is!) Regardless, yarn shouldn't
crash when it happens.

## How did you fix it?

At the beginning of `simplifyRanges`, I reduce any range of this sort by
splitting it apart and using `sember.subset` to check if one part of the
range is a subset of another, in which case it can be excluded from the
simplified range. I short circuit if the range only has one term, to
avoid any excess parsing.

I think this is the right fix, but I'm happy to take feedback or hand it
off if someone knows better. (Maybe @arcanis as author of this code?)

## Checklist

<!--- Don't worry if you miss something, chores are automatically
tested. -->
<!--- This checklist exists to help you remember doing the chores when
you submit a PR. -->
<!--- Put an `x` in all the boxes that apply. -->
- [x] I have read the [Contributing
Guide](https://yarnpkg.com/advanced/contributing).

<!-- See
https://yarnpkg.com/advanced/contributing#preparing-your-pr-to-be-released
for more details. -->
<!-- Check with `yarn version check` and fix with `yarn version check
-i` -->
- [x] I have set the packages that need to be released for my changes to
be effective.

<!-- The "Testing chores" workflow validates that your PR follows our
guidelines. -->
<!-- If it doesn't pass, click on it to see details as to what your PR
might be missing. -->
- [x] I will check that all automated PR checks pass before the PR gets
reviewed.
  • Loading branch information
smikula authored Sep 13, 2024
1 parent a22486f commit 758a8be
Show file tree
Hide file tree
Showing 3 changed files with 57 additions and 1 deletion.
34 changes: 34 additions & 0 deletions .yarn/versions/c255c43f.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
releases:
"@yarnpkg/cli": patch
"@yarnpkg/core": patch

declined:
- "@yarnpkg/plugin-compat"
- "@yarnpkg/plugin-constraints"
- "@yarnpkg/plugin-dlx"
- "@yarnpkg/plugin-essentials"
- "@yarnpkg/plugin-exec"
- "@yarnpkg/plugin-file"
- "@yarnpkg/plugin-git"
- "@yarnpkg/plugin-github"
- "@yarnpkg/plugin-http"
- "@yarnpkg/plugin-init"
- "@yarnpkg/plugin-interactive-tools"
- "@yarnpkg/plugin-link"
- "@yarnpkg/plugin-nm"
- "@yarnpkg/plugin-npm"
- "@yarnpkg/plugin-npm-cli"
- "@yarnpkg/plugin-pack"
- "@yarnpkg/plugin-patch"
- "@yarnpkg/plugin-pnp"
- "@yarnpkg/plugin-pnpm"
- "@yarnpkg/plugin-stage"
- "@yarnpkg/plugin-typescript"
- "@yarnpkg/plugin-version"
- "@yarnpkg/plugin-workspace-tools"
- "@yarnpkg/builder"
- "@yarnpkg/doctor"
- "@yarnpkg/extensions"
- "@yarnpkg/nm"
- "@yarnpkg/pnpify"
- "@yarnpkg/sdks"
21 changes: 20 additions & 1 deletion packages/yarnpkg-core/sources/semverUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ export function stringifyComparator(comparator: Comparator) {
}

export function simplifyRanges(ranges: Array<string>) {
const parsedRanges = ranges.map(range => validRange(range)!.set.map(comparators => comparators.map(comparator => getComparator(comparator))));
const parsedRanges = ranges.map(removeSubsets).map(range => validRange(range)!.set.map(comparators => comparators.map(comparator => getComparator(comparator))));

let alternatives = parsedRanges.shift()!.map(comparators => mergeComparators(comparators))
.filter((range): range is Comparator => range !== null);
Expand Down Expand Up @@ -233,3 +233,22 @@ export function simplifyRanges(ranges: Array<string>) {

return alternatives.map(comparator => stringifyComparator(comparator)).join(` || `);
}

function removeSubsets(rangeString: string) {
const parts = rangeString.split(`||`);
if (parts.length > 1) {
const newParts: Set<string> = new Set();
for (const potentialSubset of parts) {
if (!parts.some(part => part !== potentialSubset && semver.subset(potentialSubset, part))) {
newParts.add(potentialSubset);
}
}

if (newParts.size < parts.length) {
const newRange = [...newParts].join(` || `);
return newRange;
}
}

return rangeString;
}
3 changes: 3 additions & 0 deletions packages/yarnpkg-core/tests/semverUtils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,9 @@ describe(`semverUtils`, () => {
[[`<=1.5.3`, `1.5.3`], `1.5.3`],
[[`1.5.3`, `1.5.3`], `1.5.3`],
[[`1.5.0`, `1.5.3`], null],
[[`~1.0.1 || ~1.0.2`], `~1.0.1`],
[[`~1.0.1 || ~1.0.2`, `~1.0.1 || ~1.0.2`], `~1.0.1`],
[(new Array(1000)).fill(`~1.0.1 || ~1.0.2`), `~1.0.1`],
])(`should simplify %s into %s`, (ranges, expected) => {
expect(semverUtils.simplifyRanges(ranges)).toEqual(expected);
});
Expand Down

0 comments on commit 758a8be

Please sign in to comment.