fix: dep flag calculation (#8645)

- reverts pruning added in #8431, which
incorrectly prunes deps flagged as `peer` and `optional` - these flags
don't mean that this node is an optional peer!
- reverts much of #8579, which I think
mistakenly changed peer dep calculation logic
- rewrites calcDepFlags
- adds logic to avoid unsetting `extraneous` when following optional
peer edges (how #8431, should have been
fixed)
- updates my prev fix to avoid looking for missing optional peer deps
(`if ((!edge.to && edge.type !== 'peerOptional') || !edge.valid) {`)
- refactors dep flag unsetting and resetting into Node methods
- removes `shake out Link target timing issue` test, which was testing
code
[removed](2db6c08#diff-6778dbd4bbfddaeb827a8d2aa7248d4c9b329229f69e407d5fd487abe16dd942L333)
a while back
- avoids omitting flaky`selflink` fixture when writing snapshots

Fixes #8535

Author: liamcmitchell

package-lock.json

@@ -9,7 +9,9 @@ exports[`test/lib/commands/ls.js TAP ignore missing optional deps --json > ls --
 Array [
   "invalid: optional-wrong@3.2.1 {CWD}/prefix/node_modules/optional-wrong",
   "missing: peer-missing@1, required by test-npm-ls-ignore-missing-optional@1.2.3",
+  "extraneous: peer-optional-ok@1.2.3 {CWD}/prefix/node_modules/peer-optional-ok",
   "invalid: peer-optional-wrong@3.2.1 {CWD}/prefix/node_modules/peer-optional-wrong",
+  "extraneous: peer-optional-wrong@3.2.1 {CWD}/prefix/node_modules/peer-optional-wrong",
   "invalid: peer-wrong@3.2.1 {CWD}/prefix/node_modules/peer-wrong",
   "missing: prod-missing@1, required by test-npm-ls-ignore-missing-optional@1.2.3",
   "invalid: prod-wrong@3.2.1 {CWD}/prefix/node_modules/prod-wrong",
@@ -36,8 +38,8 @@ test-npm-ls-ignore-missing-optional@1.2.3 {CWD}/prefix
 +-- UNMET DEPENDENCY peer-missing@1
 +-- peer-ok@1.2.3
 +-- UNMET OPTIONAL DEPENDENCY peer-optional-missing@1
-+-- peer-optional-ok@1.2.3
-+-- peer-optional-wrong@3.2.1 invalid: "1" from the root project
++-- peer-optional-ok@1.2.3 extraneous
++-- peer-optional-wrong@3.2.1 invalid: "1" from the root project extraneous
 +-- peer-wrong@3.2.1 invalid: "1" from the root project
 +-- UNMET DEPENDENCY prod-missing@1
 +-- prod-ok@1.2.3

tap-snapshots/test/lib/commands/ls.js.test.cjs

@@ -351,7 +351,7 @@ module.exports = cls => class IdealTreeBuilder extends cls {
           filter: node => node,
           visit: node => {
             for (const edge of node.edgesOut.values()) {
-              if (!edge.to || !edge.valid) {
+              if ((!edge.to && edge.type !== 'peerOptional') || !edge.valid) {
                 this.#depsQueue.push(node)
                 break // no need to continue the loop after the first hit
               }
@@ -754,6 +754,7 @@ This is a one-time fix-up, please be patient...
 
     // have to re-calc dep flags, because the nodes don't have edges
     // until their packages get assigned, so everything looks extraneous
+    resetDepFlags(this.idealTree)
     calcDepFlags(this.idealTree)
 
     // yes, yes, this isn't the "original" version, but now that it's been
@@ -1508,11 +1509,7 @@ This is a one-time fix-up, please be patient...
     } else {
       // otherwise just unset all the flags on the root node
       // since they will sometimes have the default value
-      this.idealTree.extraneous = false
-      this.idealTree.dev = false
-      this.idealTree.optional = false
-      this.idealTree.devOptional = false
-      this.idealTree.peer = false
+      this.idealTree.unsetDepFlags()
     }
 
     // at this point, any node marked as extraneous should be pruned.
@@ -1555,12 +1552,7 @@ This is a one-time fix-up, please be patient...
 
   #idealTreePrune () {
     for (const node of this.idealTree.inventory.values()) {
-      // optional peer dependencies are meant to be added to the tree
-      // through an explicit required dependency (most commonly in the
-      // root package.json), at which point they won't be optional so
-      // any dependencies still marked as both optional and peer at
-      // this point can be pruned as a special kind of extraneous
-      if (node.extraneous || (node.peer && node.optional)) {
+      if (node.extraneous) {
         node.parent = null
       }
     }

workspaces/arborist/lib/arborist/build-ideal-tree.js

@@ -69,11 +69,7 @@ module.exports = cls => class VirtualLoader extends cls {
     if (!this.#rootOptionProvided) {
       // root is never any of these things, but might be a brand new
       // baby Node object that never had its dep flags calculated.
-      root.extraneous = false
-      root.dev = false
-      root.optional = false
-      root.devOptional = false
-      root.peer = false
+      root.unsetDepFlags()
     } else {
       this[flagsSuspect] = true
     }
@@ -93,11 +89,7 @@ module.exports = cls => class VirtualLoader extends cls {
         if (node.isRoot || node === this.#rootOptionProvided) {
           continue
         }
-        node.extraneous = true
-        node.dev = true
-        node.optional = true
-        node.devOptional = true
-        node.peer = true
+        node.resetDepFlags()
       }
       calcDepFlags(this.virtualTree, !this.#rootOptionProvided)
     }
@@ -255,11 +247,6 @@ To fix:
       sw.name = nameFromFolder(path)
     }
 
-    const dev = sw.dev
-    const optional = sw.optional
-    const devOptional = dev || optional || sw.devOptional
-    const peer = sw.peer
-
     const node = new Node({
       installLinks: this.installLinks,
       legacyPeerDeps: this.legacyPeerDeps,
@@ -270,18 +257,15 @@ To fix:
       resolved: consistentResolve(sw.resolved, this.path, path),
       pkg: sw,
       hasShrinkwrap: sw.hasShrinkwrap,
-      dev,
-      optional,
-      devOptional,
-      peer,
       loadOverrides,
+      // cast to boolean because they're undefined in the lock file when false
+      extraneous: !!sw.extraneous,
+      devOptional: !!(sw.devOptional || sw.dev || sw.optional),
+      peer: !!sw.peer,
+      optional: !!sw.optional,
+      dev: !!sw.dev,
     })
-    // cast to boolean because they're undefined in the lock file when false
-    node.extraneous = !!sw.extraneous
-    node.devOptional = !!(sw.devOptional || sw.dev || sw.optional)
-    node.peer = !!sw.peer
-    node.optional = !!sw.optional
-    node.dev = !!sw.dev
+
     return node
   }
 

workspaces/arborist/lib/arborist/load-virtual.js

@@ -1,144 +1,102 @@
-const { depth } = require('treeverse')
-
+// Dep flag (dev, peer, etc.) calculation requires default or reset flags.
+// Flags are true by default and are unset to false as we walk deps.
+// We iterate outward edges looking for dep flags that can
+// be unset based on the current nodes flags and edge type.
+// Examples:
+// - a non-optional node with a non-optional edge out, the edge node should not be optional
+// - a non-peer node with a non-peer edge out, the edge node should not be peer
+// If a node is changed, we add to the queue and continue until no more changes.
+// Flags that remain after all this unsetting should be valid.
+// Examples:
+// - a node still flagged optional must only be reachable via optional edges
+// - a node still flagged peer must only be reachable via peer edges
 const calcDepFlags = (tree, resetRoot = true) => {
   if (resetRoot) {
-    tree.dev = false
-    tree.optional = false
-    tree.devOptional = false
-    tree.peer = false
+    tree.unsetDepFlags()
   }
-  const ret = depth({
-    tree,
-    visit: node => calcDepFlagsStep(node),
-    filter: node => node,
-    getChildren: (node, tree) =>
-      [...tree.edgesOut.values()].map(edge => edge.to),
-  })
-  return ret
-}
-
-const calcDepFlagsStep = (node) => {
-  // This rewalk is necessary to handle cases where devDep and optional
-  // or normal dependency graphs overlap deep in the dep graph.
-  // Since we're only walking through deps that are not already flagged
-  // as non-dev/non-optional, it's typically a very shallow traversal
-
-  node.extraneous = false
-  resetParents(node, 'extraneous')
-  resetParents(node, 'dev')
-  resetParents(node, 'peer')
-  resetParents(node, 'devOptional')
-  resetParents(node, 'optional')
-
-  // for links, map their hierarchy appropriately
-  if (node.isLink) {
-    // node.target can be null, we check to ensure it's not null before proceeding
-    if (node.target == null) {
-      return node
-    }
-    node.target.dev = node.dev
-    node.target.optional = node.optional
-    node.target.devOptional = node.devOptional
-    node.target.peer = node.peer
-    return calcDepFlagsStep(node.target)
-  }
-
-  node.edgesOut.forEach(({ peer, optional, dev, to }) => {
-    // if the dep is missing, then its flags are already maximally unset
-    if (!to) {
-      return
-    }
-    // everything with any kind of edge into it is not extraneous
-    to.extraneous = false
-
-    // If this is a peer edge, mark the target as peer
-    if (peer) {
-      to.peer = true
-    } else if (to.peer && !hasIncomingPeerEdge(to)) {
-      unsetFlag(to, 'peer')
-    }
 
-    // devOptional is the *overlap* of the dev and optional tree.
-    // however, for convenience and to save an extra rewalk, we leave
-    // it set when we are in *either* tree, and then omit it from the
-    // package-lock if either dev or optional are set.
-    const unsetDevOpt = !node.devOptional && !node.dev && !node.optional && !dev && !optional
+  const seen = new Set()
+  const queue = [tree]
 
-    // if we are not in the devOpt tree, then we're also not in
-    // either the dev or opt trees
-    const unsetDev = unsetDevOpt || !node.dev && !dev
-    const unsetOpt = unsetDevOpt || !node.optional && !optional
+  let node
+  while (node = queue.pop()) {
+    seen.add(node)
 
-    if (unsetDevOpt) {
-      unsetFlag(to, 'devOptional')
+    // Unset extraneous from all parents to avoid removal of children.
+    if (!node.extraneous) {
+      for (let n = node.resolveParent; n?.extraneous; n = n.resolveParent) {
+        n.extraneous = false
+      }
     }
 
-    if (unsetDev) {
-      unsetFlag(to, 'dev')
+    // for links, map their hierarchy appropriately
+    if (node.isLink) {
+      // node.target can be null, we check to ensure it's not null before proceeding
+      if (node.target == null) {
+        continue
+      }
+      node.target.dev = node.dev
+      node.target.optional = node.optional
+      node.target.devOptional = node.devOptional
+      node.target.peer = node.peer
+      node.target.extraneous = node.extraneous
+      queue.push(node.target)
+      continue
     }
 
-    if (unsetOpt) {
-      unsetFlag(to, 'optional')
-    }
-  })
-
-  return node
-}
-
-const hasIncomingPeerEdge = (node) => {
-  const target = node.isLink && node.target ? node.target : node
-  for (const edge of target.edgesIn) {
-    if (edge.type === 'peer') {
-      return true
+    for (const { peer, optional, dev, to } of node.edgesOut.values()) {
+      // if the dep is missing, then its flags are already maximally unset
+      if (!to) {
+        continue
+      }
+
+      let changed = false
+
+      // only optional peer dependencies should stay extraneous
+      if (to.extraneous && !node.extraneous && !(peer && optional)) {
+        to.extraneous = false
+        changed = true
+      }
+
+      if (to.dev && !node.dev && !dev) {
+        to.dev = false
+        changed = true
+      }
+
+      if (to.optional && !node.optional && !optional) {
+        to.optional = false
+        changed = true
+      }
+
+      // devOptional is the *overlap* of the dev and optional tree.
+      // A node may be depended on by separate dev and optional nodes.
+      // It SHOULD NOT be removed when pruning dev OR optional.
+      // It SHOULD be removed when pruning dev AND optional.
+      // We only unset here if a node is not dev AND not optional because
+      // if we did unset, it would prevent any overlap deeper in the tree.
+      // We correct this later by removing if dev OR optional is set.
+      if (to.devOptional && !node.devOptional && !node.dev && !node.optional && !dev && !optional) {
+        to.devOptional = false
+        changed = true
+      }
+
+      if (to.peer && !node.peer && !peer) {
+        to.peer = false
+        changed = true
+      }
+
+      if (changed) {
+        queue.push(to)
+      }
     }
   }
-  return false
-}
 
-const resetParents = (node, flag) => {
-  if (node[flag]) {
-    return
-  }
-
-  for (let p = node; p && (p === node || p[flag]); p = p.resolveParent) {
-    p[flag] = false
-  }
-}
-
-// typically a short walk, since it only traverses deps that have the flag set.
-const unsetFlag = (node, flag) => {
-  if (node[flag]) {
-    node[flag] = false
-    depth({
-      tree: node,
-      visit: node => {
-        node.extraneous = node[flag] = false
-        if (node.isLink && node.target) {
-          node.target.extraneous = node.target[flag] = false
-        }
-      },
-      getChildren: node => {
-        const children = []
-        const targetNode = node.isLink && node.target ? node.target : node
-        for (const edge of targetNode.edgesOut.values()) {
-          if (edge.to?.[flag]) {
-            // For the peer flag, only follow peer edges to unset the flag
-            // Don't propagate peer flag through prod/dev/optional edges
-            if (flag === 'peer') {
-              if (edge.type === 'peer') {
-                children.push(edge.to)
-              }
-            } else {
-              // For other flags, follow prod edges (and peer edges for non-peer flags)
-              if (edge.type === 'prod' || edge.type === 'peer') {
-                children.push(edge.to)
-              }
-            }
-          }
-        }
-        return children
-      },
-    })
+  // Remove incorrect devOptional flags now that we have walked all deps.
+  seen.delete(tree)
+  for (const node of seen.values()) {
+    if (node.devOptional && (node.dev || node.optional)) {
+      node.devOptional = false
+    }
   }
 }
 

workspaces/arborist/lib/calc-dep-flags.js

@@ -1613,6 +1613,22 @@ class Node {
   [util.inspect.custom] () {
     return this.toJSON()
   }
+
+  resetDepFlags () {
+    this.extraneous = true
+    this.dev = true
+    this.optional = true
+    this.devOptional = true
+    this.peer = true
+  }
+
+  unsetDepFlags () {
+    this.extraneous = false
+    this.dev = false
+    this.optional = false
+    this.devOptional = false
+    this.peer = false
+  }
 }
 
 module.exports = Node

workspaces/arborist/lib/node.js

@@ -6,10 +6,6 @@
 // we can find the set that is actually extraneous.
 module.exports = tree => {
   for (const node of tree.inventory.values()) {
-    node.extraneous = true
-    node.dev = true
-    node.devOptional = true
-    node.peer = true
-    node.optional = true
+    node.resetDepFlags()
   }
 }