fix: avoid infinite loops in peer dep replacements

Fix: #325
Fix: npm/cli#3787

Replacing a peerDep and leaving it to buildIdealTree to do the right
thing can lead to infinite regress in some cases.

For example:

```
project -> (a)
a -> (x, y, j@1)
x -> PEER(j@1)
y -> PEER(j@2)
```

In this case, when we get to the `j` peer dependency, we find that
there's nowhere to place it (since it's relied upon by a dependency at
the top level, either x or y), so we replace the conflicted version, and
expect that buildIdealTree will find a new home for it.

However, the next pass does the same thing, and so on.

This patch addresses the problem in the following way.

1. Collect all the peerEntrySets containing the dependency to be replaced.
2. If all of them _can_ be placed deeper in the tree, we remove the
   conflicting peerEntrySet, and mark all incoming edges as needing
   re-evaluation.  In the case described above, this means that `x` will be
   removed as well as `j`, and eventually placed under `a`, where there is
   no conflict.
3. If any peerEntrySet _cannot_ be placed deeper in the tree, then we mark
   the offending edge as overridden, so that it won't be re-evaluated by
   buildIdealTree.

This properly finds the nested tree fully when it is possible, and prints
the appropriate ERESOLVE warning when it is not.

PR-URL: #326
Credit: @isaacs
Close: #326
Reviewed-by: @lukekarrys

lib/arborist/build-ideal-tree.js

@@ -993,8 +993,13 @@ This is a one-time fix-up, please be patient...
             return
           }
 
-          // lastly, also check for the missing deps of the node we placed
+          // lastly, also check for the missing deps of the node we placed,
+          // and any holes created by pruning out conflicted peer sets.
           this[_depsQueue].push(placed)
+          for (const dep of pd.needEvaluation) {
+            this[_depsSeen].delete(dep)
+            this[_depsQueue].push(dep)
+          }
 
           // pre-fetch any problem edges, since we'll need these soon
           // if it fails at this point, though, dont' worry because it

lib/place-dep.js

@@ -63,6 +63,8 @@ class PlaceDep {
     this.parent = parent
     this.peerConflict = null
 
+    this.needEvaluation = new Set()
+
     this.checks = new Map()
 
     this.place()
@@ -365,6 +367,8 @@ class PlaceDep {
   }
 
   replaceOldDep () {
+    const target = this.oldDep.parent
+
     // XXX handle replacing an entire peer group?
     // what about cases where we need to push some other peer groups deeper
     // into the tree?  all the tree updating should be done here, and track
@@ -383,8 +387,47 @@ class PlaceDep {
         oldDeps.push(...gatherDepSet([edge.to], e => e.to !== edge.to))
       }
     }
+
+    // gather all peer edgesIn which are at this level, and will not be
+    // satisfied by the new dependency.  Those are the peer sets that need
+    // to be either warned about (if they cannot go deeper), or removed and
+    // re-placed (if they can).
+    const prunePeerSets = []
+    for (const edge of this.oldDep.edgesIn) {
+      if (this.placed.satisfies(edge) ||
+          !edge.peer ||
+          edge.from.parent !== target ||
+          edge.overridden) {
+        // not a peer dep, not invalid, or not from this level, so it's fine
+        // to just let it re-evaluate as a problemEdge later, or let it be
+        // satisfied by the new dep being placed.
+        continue
+      }
+      for (const entryEdge of peerEntrySets(edge.from).keys()) {
+        // either this one needs to be pruned and re-evaluated, or marked
+        // as overridden and warned about.  If the entryEdge comes in from
+        // the root, then we have to leave it alone, and in that case, it
+        // will have already warned or crashed by getting to this point.
+        const entryNode = entryEdge.to
+        const deepestTarget = deepestNestingTarget(entryNode)
+        if (deepestTarget !== target && !entryEdge.from.isRoot) {
+          prunePeerSets.push(...gatherDepSet([entryNode], e => {
+            return e.to !== entryNode && !e.overridden
+          }))
+        } else {
+          this.warnPeerConflict(edge, this.dep)
+        }
+      }
+    }
+
     this.placed.replace(this.oldDep)
     this.pruneForReplacement(this.placed, oldDeps)
+    for (const dep of prunePeerSets) {
+      for (const edge of dep.edgesIn) {
+        this.needEvaluation.add(edge.from)
+      }
+      dep.root = null
+    }
   }
 
   pruneForReplacement (node, oldDeps) {
@@ -485,19 +528,22 @@ class PlaceDep {
     return false
   }
 
-  warnPeerConflict () {
-    this.edge.overridden = true
-    const expl = this.explainPeerConflict()
+  warnPeerConflict (edge, dep) {
+    edge = edge || this.edge
+    dep = dep || this.dep
+    edge.overridden = true
+    const expl = this.explainPeerConflict(edge, dep)
     log.warn('ERESOLVE', 'overriding peer dependency', expl)
   }
 
-  failPeerConflict () {
-    const expl = this.explainPeerConflict()
+  failPeerConflict (edge, dep) {
+    edge = edge || this.top.edge
+    dep = dep || this.top.dep
+    const expl = this.explainPeerConflict(edge, dep)
     throw Object.assign(new Error('could not resolve'), expl)
   }
 
-  explainPeerConflict () {
-    const { edge, dep } = this.top
+  explainPeerConflict (edge, dep) {
     const { from: node } = edge
     const curNode = node.resolve(edge.name)