Fix: Prevents hoisting through peer dependencies (#4086)

**Summary**

Fixes #3933. Currently, the code hoists depedencies without ensuring hoisting doesn't break the final structure's `peerDependencies` requirements. This patch fixes that by adding extra checks before hoisting. It comes with a little performance impact but shouldn't be too much.

**Test plan**

A new integration test that simulates the reported behavior and ensures `yarn` now behaves correctly.

__tests__/commands/install/integration.js

@@ -49,6 +49,12 @@ async function mockConstants(base: Config, mocks: Object, cb: (config: Config) =
 beforeEach(request.__resetAuthedRequests);
 afterEach(request.__resetAuthedRequests);
 
+test.concurrent('install should not hoist packages above their peer dependencies', async () => {
+  await runInstall({}, 'install-should-not-hoist-through-peer-deps', async (config): Promise<void> => {
+    expect(await fs.exists(`${config.cwd}/node_modules/a/node_modules/c`)).toEqual(true);
+  });
+});
+
 test.concurrent('install optional subdependencies by default', async () => {
   await runInstall({}, 'install-optional-dependencies', async (config): Promise<void> => {
     expect(await fs.exists(`${config.cwd}/node_modules/dep-b`)).toEqual(true);

__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/a/index.js

@@ -0,0 +1 @@
+require("c")

__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/a/package.json

@@ -0,0 +1 @@
+{"name":"a", "version":"1.0.0", "dependencies":{"b":"file:../b-2", "c":"file:../c"}}

__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/b-1/package.json

@@ -0,0 +1 @@
+{"name":"b", "version":"1.0.0"}

__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/b-2/package.json

@@ -0,0 +1 @@
+{"name":"b", "version":"2.0.0"}

__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/c/index.js

@@ -0,0 +1 @@
+console.log(require("b/package.json").version)

__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/c/package.json

@@ -0,0 +1 @@
+{"name":"c", "version":"1.0.0", "peerDependencies":{"b":"2.0.0"}}

__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/d/package.json

@@ -0,0 +1 @@
+{"name":"d", "version":"1.0.0", "dependencies":{"b":"file:../b-1"}}

__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/e/package.json

@@ -0,0 +1 @@
+{"name":"e", "version":"1.0.0", "dependencies":{"b":"file:../b-1"}}

__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/index.js

@@ -0,0 +1 @@
+require("a")

__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/package.json

@@ -0,0 +1 @@
+{"dependencies":{"a":"file:./a", "d":"file:./d", "e":"file:./e"}}

__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/yarn.lock

@@ -0,0 +1,28 @@
+# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
+# yarn lockfile v1
+
+
+"a@file:./a":
+  version "1.0.0"
+  dependencies:
+    b "file:../../../Users/mael/Library/Caches/Yarn/v1/b-2"
+    c "file:../../../Users/mael/Library/Caches/Yarn/v1/c"
+
+"b@file:b-1":
+  version "1.0.0"
+
+"b@file:b-2":
+  version "2.0.0"
+
+"c@file:c":
+  version "1.0.0"
+
+"d@file:./d":
+  version "1.0.0"
+  dependencies:
+    b "file:../../../Users/mael/Library/Caches/Yarn/v1/b-1"
+
+"e@file:./e":
+  version "1.0.0"
+  dependencies:
+    b "file:../../../Users/mael/Library/Caches/Yarn/v1/b-1"

src/package-hoister.js

@@ -121,7 +121,41 @@ export default class PackageHoister {
         return sortAlpha(aPattern, bPattern);
       });
 
-      for (const [pattern, parent] of queue) {
+      // sort the queue again to hoist packages without peer dependencies first
+      let sortedQueue = [];
+      const availableSet = new Set();
+
+      let hasChanged = true;
+      while (queue.length > 0 && hasChanged) {
+        hasChanged = false;
+
+        for (let t = 0; t < queue.length; ++t) {
+          const pattern = queue[t][0];
+          const pkg = this.resolver.getStrictResolvedPattern(pattern);
+
+          const peerDependencies = Object.keys(pkg.peerDependencies || {});
+          const areDependenciesFulfilled = peerDependencies.every(peerDependency => availableSet.has(peerDependency));
+
+          if (areDependenciesFulfilled) {
+            // Move the package inside our sorted queue
+            sortedQueue.push(queue[t]);
+            queue.splice(t--, 1);
+
+            // Add it to our set, so that we know it is available
+            availableSet.add(pattern);
+
+            // Schedule a next pass, in case other packages had peer dependencies on this one
+            hasChanged = true;
+          }
+        }
+      }
+
+      // We might end up with some packages left in the queue, that have not been sorted. We reach this codepath if two
+      // packages have a cyclic dependency, or if the peer dependency is provided by a parent package. In these case,
+      // nothing we can do, so we just add all of these packages to the end of the sorted queue.
+      sortedQueue = sortedQueue.concat(queue);
+
+      for (const [pattern, parent] of sortedQueue) {
         const info = this._seed(pattern, {isDirectRequire: false, parent});
         if (info) {
           this.hoist(info);
@@ -259,13 +293,13 @@ export default class PackageHoister {
     const stack = []; // stack of removed parts
     const name = parts.pop();
 
-    //
     for (let i = parts.length - 1; i >= 0; i--) {
       const checkParts = parts.slice(0, i).concat(name);
       const checkKey = this.implodeKey(checkParts);
       info.addHistory(`Looked at ${checkKey} for a match`);
 
       const existing = this.tree.get(checkKey);
+
       if (existing) {
         if (existing.loc === info.loc) {
           // switch to non ignored if earlier deduped version was ignored (must be compatible)
@@ -291,8 +325,24 @@ export default class PackageHoister {
       }
     }
 
+    const peerDependencies = Object.keys(info.pkg.peerDependencies || {});
+
     // remove redundant parts that wont collide
-    while (parts.length) {
+    hoistLoop: while (parts.length) {
+      // we must not hoist a package higher than its peer dependencies
+      for (const peerDependency of peerDependencies) {
+        const checkParts = parts.concat(peerDependency);
+        const checkKey = this.implodeKey(checkParts);
+        info.addHistory(`Looked at ${checkKey} for a peer dependency match`);
+
+        const existing = this.tree.get(checkKey);
+
+        if (existing) {
+          info.addHistory(`Found a peer dependency requirement at ${checkKey}`);
+          break hoistLoop;
+        }
+      }
+
       const checkParts = parts.concat(name);
       const checkKey = this.implodeKey(checkParts);