Fix bug in yarn-cling

Author: rix0rrr

package.json

@@ -4,7 +4,16 @@ import { iterDeps, isPackage, type PackageLockFile, type PackageLockTree } from
 /**
  * Hoist package-lock dependencies in-place
  *
- * This happens in two phases:
+ * Packages are declared in two different roles here:
+ *
+ * - "requires" indicates where a package is consumed
+ * - "dependencies" indicates where a package is provided; it will be available
+ *   to the package it is provided under, as well as any of its children.
+ *
+ * This function manipulates the "dependencies" part of the package tree, minimizing
+ * the occurrences of packages in "dependencies" while keeping all "requires" satified.
+ *
+ * This happens by applying two basic operations:
  *
  * 1) Move every package into the parent scope (as long as it introduces no conflicts).
  *    Leave "moved" markers to indicate that a package used to be there and no
@@ -63,22 +72,31 @@ export function renderTree(tree: PackageLockTree): string[] {
 
 export function _addTombstones<A extends PackageLockTree>(root: A): A {
   let tree = structuredClone(root);
-  recurse(tree);
+  recurse(tree, [tree]);
   return tree;
 
-  function recurse(node: PackageLockTree) {
-    // For every node, all the packages they 'requires' should be in 'dependencies'.
+  function recurse(nodeToCheck: PackageLockTree, rootPathToAdd: PackageLockTree[]) {
+    // Rootpath is ordered deep -> shallow.
+
+    // For every node, all the packages they or any of their children 'requires' should be in 'dependencies'.
     // If it's not in 'dependencies', that must mean its at a higher level already, so we put
     // the 'moved' tombstone in to make sure we don't accidentally replace this package with a different version.
-    for (const name of Object.keys(node.requires ?? {})) {
-      if (!node.dependencies?.[name]) {
-        node.dependencies = node.dependencies ?? {};
-        node.dependencies[name] = 'moved';
+    // Also add 'moved' to all of its parents, until we find a node that has it in 'dependencies'.
+    for (const name of Object.keys(nodeToCheck.requires ?? {})) {
+
+      // For every dependency in 'nodeToCheck', add 'moved' to 'depend. As soon as we find
+      // the dependency provided declared anywhere, we stop.
+      for (const nodeToAdd of rootPathToAdd) {
+        if (nodeToAdd.dependencies?.[name]) {
+          break;
+        }
+        nodeToAdd.dependencies = nodeToAdd.dependencies ?? {};
+        nodeToAdd.dependencies[name] = 'moved';
       }
     }
 
-    for (const [_, dep] of iterDeps(node)) {
-      recurse(dep);
+    for (const [_, dep] of iterDeps(nodeToCheck)) {
+      recurse(dep, [dep, ...rootPathToAdd]);
     }
   }
 }

packages/@aws-cdk/yarn-cling/lib/hoisting.ts

@@ -87,7 +87,7 @@ class PackageGraphBuilder {
   }
 
   /**
-   * Render the tree by starting from the root keys and recursing, pushing every package as high as it can
+   * Render the tree by starting from the root keys and recursing.
    * go without conflicting
    */
   public makeDependencyTree(rootKeys: string[]): Record<string, PackageLockPackage> {

packages/@aws-cdk/yarn-cling/lib/index.ts

@@ -160,6 +160,38 @@ test('order of hoisting shouldnt produce a broken situation', () => {
   ]);
 });
 
+test('reproduce hoisting bug', () => {
+  // GIVEN
+  let tree = pkgFile({
+    // Prevent pushing child1 and child2 upwards
+    child2: pkg('999.999.999'),
+    child1: pkg('999.999.999'),
+
+    // sharedparent doesn't have anything to do with 'conflictdep' itself,
+    // but has 2 children that depend on conflicting versions.
+    //
+    // conflictdep@1 is at the top of the tree already.
+    // We want to avoid that conflictdep@2 gets pushed up because the
+    // slot in sharedparent.dependencies['conflictdep'] is "free" (because it
+    // isn't).
+    sharedparent: pkg('2.0.0', {
+      child2: pkg('4.0.0', {
+        conflictdep: pkg('2.0.0'),
+      }),
+      child1: pkg2('3.0.0', {
+        requires: { conflictdep: '1.0.0' },
+      }),
+    }),
+    conflictdep: pkg('1.0.0'),
+  });
+
+  // WHEN
+  tree = hoistDependencies(tree);
+
+  // THEN -- should not throw
+  _validateTree(tree);
+});
+
 function pkg(version: string, dependencies?: Record<string, PackageLockPackage>): PackageLockPackage {
   return {
     version,
@@ -170,6 +202,19 @@ function pkg(version: string, dependencies?: Record<string, PackageLockPackage>)
   };
 }
 
+interface Pkg2Options {
+  dependencies?: Record<string, PackageLockPackage>;
+  requires?: Record<string, string>;
+}
+
+function pkg2(version: string, options: Pkg2Options): PackageLockPackage {
+  return {
+    version,
+    dependencies: options.dependencies,
+    requires: options.requires,
+  };
+}
+
 function pkgFile(dependencies?: Record<string, PackageLockPackage>): PackageLockFile {
   return {
     lockfileVersion: 1,