fix: resolve override conflicts and apply correct versions (#8089)

A first step in fixing the overrides feature. In this PR I'm aiming to fix 3 bugs:

1. When we add an edge going into a node we update the node's overrides, but we don't update the overrides of that node's outgoing edges, and so forth. We need the up-to-date overrides to filter through.
2. When we remove an edge going into a node we don't update the overrides at all (and we don't update the outgoing edges like in the previous bug).
3. When we add an edge going in, and we already have a different override set for the node, we just ignore the existing override set and overwrite it with that of the new edge. Instead, this PR chooses the most specific override set. This still isn't the absolutely correct logic, since different override sets can have implications down the line of the dependency chain, but it has the advantage of being **consistent** (instead of just going with the last edge in). Also, it raises an error if it encounters a real conflict, meaning two incoming edges with override sets that aren't just a subset of one another.
So if we have dependency chains A->B->C and A->C, and we override C under B, then C will be overridden.

## References
Fixes some of the override [issues](#5850).

Primary author: @AlonNavo
Co-author: @owlstronaut

Author: owlstronaut

workspaces/arborist/lib/dep-valid.js

@@ -101,7 +101,7 @@ const depValid = (child, requested, requestor) => {
       })
     }
 
-    default: // unpossible, just being cautious
+    default: // impossible, just being cautious
       break
   }
 

workspaces/arborist/lib/edge.js

@@ -4,6 +4,7 @@
 const util = require('node:util')
 const npa = require('npm-package-arg')
 const depValid = require('./dep-valid.js')
+const OverrideSet = require('./override-set.js')
 
 class ArboristEdge {
   constructor (edge) {
@@ -103,7 +104,7 @@ class Edge {
   }
 
   satisfiedBy (node) {
-    if (node.name !== this.#name) {
+    if (node.name !== this.#name || !this.#from) {
       return false
     }
 
@@ -112,7 +113,31 @@ class Edge {
     if (node.hasShrinkwrap || node.inShrinkwrap || node.inBundle) {
       return depValid(node, this.rawSpec, this.#accept, this.#from)
     }
-    return depValid(node, this.spec, this.#accept, this.#from)
+
+    // If there's no override we just use the spec.
+    if (!this.overrides?.keySpec) {
+      return depValid(node, this.spec, this.#accept, this.#from)
+    }
+    // There's some override. If the target node satisfies the overriding spec
+    // then it's okay.
+    if (depValid(node, this.spec, this.#accept, this.#from)) {
+      return true
+    }
+    // If it doesn't, then it should at least satisfy the original spec.
+    if (!depValid(node, this.rawSpec, this.#accept, this.#from)) {
+      return false
+    }
+    // It satisfies the original spec, not the overriding spec. We need to make
+    // sure it doesn't use the overridden spec.
+    // For example:
+    //   we might have an ^8.0.0 rawSpec, and an override that makes
+    //   keySpec=8.23.0 and the override value spec=9.0.0.
+    //   If the node is 9.0.0, then it's okay because it's consistent with spec.
+    //   If the node is 8.24.0, then it's okay because it's consistent with the rawSpec.
+    //   If the node is 8.23.0, then it's not okay because even though it's consistent
+    //   with the rawSpec, it's also consistent with the keySpec.
+    //   So we're looking for ^8.0.0 or 9.0.0 and not 8.23.0.
+    return !depValid(node, this.overrides.keySpec, this.#accept, this.#from)
   }
 
   // return the edge data, and an explanation of how that edge came to be here
@@ -181,11 +206,9 @@ class Edge {
     if (this.overrides?.value && this.overrides.value !== '*' && this.overrides.name === this.#name) {
       if (this.overrides.value.startsWith('$')) {
         const ref = this.overrides.value.slice(1)
-        // we may be a virtual root, if we are we want to resolve reference overrides
-        // from the real root, not the virtual one
-        const pkg = this.#from.sourceReference
-          ? this.#from.sourceReference.root.package
-          : this.#from.root.package
+        const pkg = this.#from?.sourceReference
+          ? this.#from?.sourceReference.root.package
+          : this.#from?.root?.package
         if (pkg.devDependencies?.[ref]) {
           return pkg.devDependencies[ref]
         }
@@ -234,10 +257,15 @@ class Edge {
         } else {
           this.#error = 'MISSING'
         }
-      } else if (this.peer && this.#from === this.#to.parent && !this.#from.isTop) {
+      } else if (this.peer && this.#from === this.#to.parent && !this.#from?.isTop) {
         this.#error = 'PEER LOCAL'
       } else if (!this.satisfiedBy(this.#to)) {
         this.#error = 'INVALID'
+      } else if (this.overrides && this.#to.edgesOut.size && OverrideSet.doOverrideSetsConflict(this.overrides, this.#to.overrides)) {
+        // Any inconsistency between the edge's override set and the target's override set is potentially problematic.
+        // But we only say the edge is in error if the override sets are plainly conflicting.
+        // Note that if the target doesn't have any dependencies of their own, then this inconsistency is irrelevant.
+        this.#error = 'INVALID'
       } else {
         this.#error = 'OK'
       }
@@ -250,15 +278,26 @@ class Edge {
 
   reload (hard = false) {
     this.#explanation = null
-    if (this.#from.overrides) {
-      this.overrides = this.#from.overrides.getEdgeRule(this)
+
+    let needToUpdateOverrideSet = false
+    let newOverrideSet
+    let oldOverrideSet
+    if (this.#from?.overrides) {
+      newOverrideSet = this.#from.overrides.getEdgeRule(this)
+      if (newOverrideSet && !newOverrideSet.isEqual(this.overrides)) {
+        // If there's a new different override set we need to propagate it to the nodes.
+        // If we're deleting the override set then there's no point propagating it right now since it will be filled with another value later.
+        needToUpdateOverrideSet = true
+        oldOverrideSet = this.overrides
+        this.overrides = newOverrideSet
+      }
     } else {
       delete this.overrides
     }
-    const newTo = this.#from.resolve(this.#name)
+    const newTo = this.#from?.resolve(this.#name)
     if (newTo !== this.#to) {
       if (this.#to) {
-        this.#to.edgesIn.delete(this)
+        this.#to.deleteEdgeIn(this)
       }
       this.#to = newTo
       this.#error = null
@@ -267,15 +306,19 @@ class Edge {
       }
     } else if (hard) {
       this.#error = null
+    } else if (needToUpdateOverrideSet && this.#to) {
+      // Propagate the new override set to the target node.
+      this.#to.updateOverridesEdgeInRemoved(oldOverrideSet)
+      this.#to.updateOverridesEdgeInAdded(newOverrideSet)
     }
   }
 
   detach () {
     this.#explanation = null
     if (this.#to) {
-      this.#to.edgesIn.delete(this)
+      this.#to.deleteEdgeIn(this)
     }
-    this.#from.edgesOut.delete(this.#name)
+    this.#from?.edgesOut.delete(this.#name)
     this.#to = null
     this.#error = 'DETACHED'
     this.#from = null

workspaces/arborist/lib/node.js

@@ -40,6 +40,7 @@ const debug = require('./debug.js')
 const gatherDepSet = require('./gather-dep-set.js')
 const treeCheck = require('./tree-check.js')
 const { walkUp } = require('walk-up-path')
+const { log } = require('proc-log')
 
 const { resolve, relative, dirname, basename } = require('node:path')
 const util = require('node:util')
@@ -344,7 +345,28 @@ class Node {
   }
 
   get overridden () {
-    return !!(this.overrides && this.overrides.value && this.overrides.name === this.name)
+    if (!this.overrides) {
+      return false
+    }
+    if (!this.overrides.value) {
+      return false
+    }
+    if (this.overrides.name !== this.name) {
+      return false
+    }
+
+    // The overrides rule is for a package with this name, but some override rules only apply to specific
+    // versions. To make sure this package was actually overridden, we check whether any edge going in
+    // had the rule applied to it, in which case its overrides set is different than its source node.
+    for (const edge of this.edgesIn) {
+      if (edge.overrides && edge.overrides.name === this.name && edge.overrides.value === this.version) {
+        if (!edge.overrides.isEqual(edge.from.overrides)) {
+          return true
+        }
+      }
+    }
+
+    return false
   }
 
   get package () {
@@ -822,9 +844,6 @@ class Node {
       target.root = root
     }
 
-    if (!this.overrides && this.parent && this.parent.overrides) {
-      this.overrides = this.parent.overrides.getNodeRule(this)
-    }
     // tree should always be valid upon root setter completion.
     treeCheck(this)
     if (this !== root) {
@@ -1006,10 +1025,21 @@ class Node {
       return false
     }
 
-    // XXX need to check for two root nodes?
-    if (node.overrides !== this.overrides) {
-      return false
+    // If this node has no dependencies, then it's irrelevant to check the override
+    // rules of the replacement node.
+    if (this.edgesOut.size) {
+      // XXX need to check for two root nodes?
+      if (node.overrides) {
+        if (!node.overrides.isEqual(this.overrides)) {
+          return false
+        }
+      } else {
+        if (this.overrides) {
+          return false
+        }
+      }
     }
+
     ignorePeers = new Set(ignorePeers)
 
     // gather up all the deps of this node and that are only depended
@@ -1077,8 +1107,13 @@ class Node {
       return false
     }
 
-    // if we prefer dedupe, or if the version is greater/equal, take the other
-    if (preferDedupe || semver.gte(other.version, this.version)) {
+    // if we prefer dedupe, or if the version is equal, take the other
+    if (preferDedupe || semver.eq(other.version, this.version)) {
+      return true
+    }
+
+    // if our current version isn't the result of an override, then prefer to take the greater version
+    if (!this.overridden && semver.gt(other.version, this.version)) {
       return true
     }
 
@@ -1249,10 +1284,6 @@ class Node {
       this[_changePath](newPath)
     }
 
-    if (parent.overrides) {
-      this.overrides = parent.overrides.getNodeRule(this)
-    }
-
     // clobbers anything at that path, resets all appropriate references
     this.root = parent.root
   }
@@ -1346,9 +1377,87 @@ class Node {
     this.edgesOut.set(edge.name, edge)
   }
 
-  addEdgeIn (edge) {
+  recalculateOutEdgesOverrides () {
+    // For each edge out propogate the new overrides through.
+    for (const edge of this.edgesOut.values()) {
+      edge.reload(true)
+      if (edge.to) {
+        edge.to.updateOverridesEdgeInAdded(edge.overrides)
+      }
+    }
+  }
+
+  updateOverridesEdgeInRemoved (otherOverrideSet) {
+    // If this edge's overrides isn't equal to this node's overrides, then removing it won't change newOverrideSet later.
+    if (!this.overrides || !this.overrides.isEqual(otherOverrideSet)) {
+      return false
+    }
+    let newOverrideSet
+    for (const edge of this.edgesIn) {
+      if (newOverrideSet && edge.overrides) {
+        newOverrideSet = OverrideSet.findSpecificOverrideSet(edge.overrides, newOverrideSet)
+      } else {
+        newOverrideSet = edge.overrides
+      }
+    }
+    if (this.overrides.isEqual(newOverrideSet)) {
+      return false
+    }
+    this.overrides = newOverrideSet
+    if (this.overrides) {
+      // Optimization: if there's any override set at all, then no non-extraneous node has an empty override set. So if we temporarily have no
+      // override set (for example, we removed all the edges in), there's no use updating all the edges out right now. Let's just wait until
+      // we have an actual override set later.
+      this.recalculateOutEdgesOverrides()
+    }
+    return true
+  }
+
+  // This logic isn't perfect either. When we have two edges in that have different override sets, then we have to decide which set is correct.
+  // This function assumes the more specific override set is applicable, so if we have dependencies A->B->C and A->C
+  // and an override set that specifies what happens for C under A->B, this will work even if the new A->C edge comes along and tries to change
+  // the override set.
+  // The strictly correct logic is not to allow two edges with different overrides to point to the same node, because even if this node can satisfy
+  // both, one of its dependencies might need to be different depending on the edge leading to it.
+  // However, this might cause a lot of duplication, because the conflict in the dependencies might never actually happen.
+  updateOverridesEdgeInAdded (otherOverrideSet) {
+    if (!otherOverrideSet) {
+      // Assuming there are any overrides at all, the overrides field is never undefined for any node at the end state of the tree.
+      // So if the new edge's overrides is undefined it will be updated later. So we can wait with updating the node's overrides field.
+      return false
+    }
+    if (!this.overrides) {
+      this.overrides = otherOverrideSet
+      this.recalculateOutEdgesOverrides()
+      return true
+    }
+    if (this.overrides.isEqual(otherOverrideSet)) {
+      return false
+    }
+    const newOverrideSet = OverrideSet.findSpecificOverrideSet(this.overrides, otherOverrideSet)
+    if (newOverrideSet) {
+      if (!this.overrides.isEqual(newOverrideSet)) {
+        this.overrides = newOverrideSet
+        this.recalculateOutEdgesOverrides()
+        return true
+      }
+      return false
+    }
+    // This is an error condition. We can only get here if the new override set is in conflict with the existing.
+    log.silly('Conflicting override sets', this.name)
+  }
+
+  deleteEdgeIn (edge) {
+    this.edgesIn.delete(edge)
     if (edge.overrides) {
-      this.overrides = edge.overrides
+      this.updateOverridesEdgeInRemoved(edge.overrides)
+    }
+  }
+
+  addEdgeIn (edge) {
+    // We need to handle the case where the new edge in has an overrides field which is different from the current value.
+    if (!this.overrides || !this.overrides.isEqual(edge.overrides)) {
+      this.updateOverridesEdgeInAdded(edge.overrides)
     }
 
     this.edgesIn.add(edge)