fix: issue-5850 - Settle override conflicts between edges and propagate changes

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

@@ -112,7 +112,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.overridden) {
+      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
@@ -179,6 +203,11 @@ class Edge {
 
   get spec () {
     if (this.overrides?.value && this.overrides.value !== '*' && this.overrides.name === this.#name) {
+      // If this edge has the same overrides field as the source, then we're not applying an override for this edge.
+      if (this.overrides === this.#from.overrides) {
+        return this.#spec
+      }
+
       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
@@ -238,6 +267,11 @@ class Edge {
         this.#error = 'PEER LOCAL'
       } else if (!this.satisfiedBy(this.#to)) {
         this.#error = 'INVALID'
+      } else if (this.overrides && this.#to.edgesOut.size && !this.#to.findSpecificOverrideSet(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 +284,26 @@ class Edge {
 
   reload (hard = false) {
     this.#explanation = null
+
+    let needToUpdateOverrideSet = false
+    let newOverrideSet
+    let oldOverrideSet
     if (this.#from.overrides) {
-      this.overrides = this.#from.overrides.getEdgeRule(this)
+      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)
     if (newTo !== this.#to) {
       if (this.#to) {
-        this.#to.edgesIn.delete(this)
+        this.#to.deleteEdgeIn(this)
       }
       this.#to = newTo
       this.#error = null
@@ -268,12 +313,17 @@ class Edge {
     } else if (hard) {
       this.#error = null
     }
+    else if (needToUpdateOverrideSet) {
+      // 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.#to = null

workspaces/arborist/lib/node.js

@@ -65,6 +65,8 @@ const CaseInsensitiveMap = require('./case-insensitive-map.js')
 
 const querySelectorAll = require('./query-selector-all.js')
 
+const log = require('proc-log')
+
 class Node {
   #global
   #meta
@@ -342,7 +344,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 () {
@@ -820,9 +843,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) {
@@ -1004,10 +1024,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
@@ -1075,8 +1106,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
     }
 
@@ -1247,10 +1283,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
   }
@@ -1344,9 +1376,101 @@ 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) {
+      edge.reload(true)
+      if (edge.to) {
+        edge.to.updateOverridesEdgeInAdded(edge.overrides)
+      }
+    }
+  }
+
+  findSpecificOverrideSet (first, second) {
+    for (let overrideSet = second; overrideSet; overrideSet = overrideSet.parent) {
+      if (overrideSet.isEqual(first)) {
+        return second
+      }
+    }
+    for (let overrideSet = first; overrideSet; overrideSet = overrideSet.parent) {
+      if (overrideSet.isEqual(second)) {
+        return first
+      }
+    }
+    log.silly('Conflicting override sets', this, first, second)
+  }
+
+  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 = this.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 = this.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.
+    throw Object.assign(new Error(`Conflicting override requirements for node ${this.name}`), { code: 'EOVERRIDE' })
+  }
+
+  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)

workspaces/arborist/lib/override-set.js

@@ -44,6 +44,43 @@ class OverrideSet {
     }
   }
 
+  childrenAreEqual (other) {
+    if (this.children.size !== other.children.size) {
+      return false
+    }
+    for (const [key] of this.children) {
+      if (!other.children.has(key)) {
+        return false
+      }
+      if (this.children.get(key).value !== other.children.get(key).value) {
+        return false
+      }
+      if (!this.children.get(key).childrenAreEqual(other.children.get(key))) {
+        return false
+      }
+    }
+    return true
+  }
+
+  isEqual (other) {
+    if (this === other) {
+      return true
+    }
+    if (!other) {
+      return false
+    }
+    if (this.key !== other.key || this.value !== other.value) {
+      return false
+    }
+    if (!this.childrenAreEqual(other)) {
+      return false
+    }
+    if (!this.parent) {
+      return !other.parent
+    }
+    return this.parent.isEqual(other.parent)
+  }
+
   getEdgeRule (edge) {
     for (const rule of this.ruleset.values()) {
       if (rule.name !== edge.name) {
@@ -55,7 +92,8 @@ class OverrideSet {
         return rule
       }
 
-      let spec = npa(`${edge.name}@${edge.spec}`)
+      // We need to use the rawSpec here, because the spec has the overrides applied to it already.
+      let spec = npa(`${edge.name}@${edge.rawSpec}`)
       if (spec.type === 'alias') {
         spec = spec.subSpec
       }