fix: make Node.children() a case-insensitive Map

In other words, do not allow a node to have both `FOO` and `foo` in a
parent node's `children` collection.

In order to maintain proper semantics for package resolution, this also
means that Node.edgesOut has to be case-insensitive as well, meaning
that a package may not *depend* on both `FOO` and `foo`.

Lastly, do not add removed dependency folders to the trashlist.  There
is no need, since they are retired and then not un-retired, and removing
the package folder itself (which was always a rimraf of a non-existent
path) could remove something we want, if you are replacing `FOO` with
`foo`.

This has the effect of making Arborist.buildIdealTree() and
Arborist.reify() properly collide packages that differ only in case,
thus generating trees that are safe for use across a variety of file
systems.

This is an improvement on the previous commit's "non-directory detected
during sparse tree creation" check, because it will avoid cases where
the package-lock.json does not match what we actually end up creating on
disk, and sidestep the collisions altogether.

Note that it _is_ still possible to end up with collisions on Windows
systems, if for example you have a package.json like this:

```json
{
  "dependencies": {
    "longer-than-eight-char-name": "file:/some/arbitrary/path",
    "LONGER~1": "npm:arbitrary-write@*"
  }
}
```

In that case, the sparse dir check in the previous commit will be
triggered, since the folder's non-directory-ness will be detected while
creating the sparse folder tree.  The user will end up with a mismatched
lockfile, but importantly, they *won't* have arbitrary writes being sent
through a symbolic link.

On case _sensitive_ systems, this means that the `node_modules` folder
may not contain case-variant packages, and any projects currently in
this state will need to be reinstalled or updated.  As this is extremely
rare (since uppercase and non-url-safe characters have not been allowed
in package names on the public registry for many years), it is unlikely
to affect any users in the wild.

Author: isaacs

lib/arborist/reify.js

@@ -79,7 +79,7 @@ const _addOmitsToTrashList = Symbol('addOmitsToTrashList')
 const _packageLockOnly = Symbol('packageLockOnly')
 const _dryRun = Symbol('dryRun')
 const _validatePath = Symbol('validatePath')
-const _reifyPackages = Symbol('reifyPackages')
+const _reifyPackages = Symbol.for('reifyPackages')
 
 const _omitDev = Symbol('omitDev')
 const _omitOptional = Symbol('omitOptional')
@@ -330,12 +330,13 @@ module.exports = cls => class Reifier extends cls {
       ideal: this.idealTree,
     })
 
-    for (const node of this.diff.removed) {
-      // a node in a dep bundle will only be removed if its bundling dep
-      // is removed as well.  in which case, we don't have to delete it!
-      if (!node.inDepBundle)
-        this[_addNodeToTrashList](node)
-    }
+    // we don't have to add 'removed' folders to the trashlist, because
+    // they'll be moved aside to a retirement folder, and then the retired
+    // folder will be deleted at the end.  This is important when we have
+    // a folder like FOO being "removed" in favor of a folder like "foo",
+    // because if we remove node_modules/FOO on case-insensitive systems,
+    // it will remove the dep that we *want* at node_modules/foo.
+
     process.emit('timeEnd', 'reify:diffTrees')
   }
 

lib/edge.js

@@ -77,7 +77,7 @@ class Edge {
   }
 
   satisfiedBy (node) {
-    return depValid(node, this.spec, this.accept, this.from)
+    return node.name === this.name && depValid(node, this.spec, this.accept, this.from)
   }
 
   explain (seen = []) {
@@ -167,7 +167,7 @@ class Edge {
   [_loadError] () {
     return !this[_to] ? (this.optional ? null : 'MISSING')
       : this.peer && this.from === this.to.parent && !this.from.isTop ? 'PEER LOCAL'
-      : !depValid(this.to, this.spec, this.accept, this.from) ? 'INVALID'
+      : !this.satisfiedBy(this.to) ? 'INVALID'
       : 'OK'
   }
 

lib/node.js

@@ -66,6 +66,7 @@ const relpath = require('./relpath.js')
 const consistentResolve = require('./consistent-resolve.js')
 
 const printableTree = require('./printable.js')
+const CaseInsensitiveMap = require('./case-insensitive-map.js')
 
 class Node {
   constructor (options) {
@@ -148,7 +149,7 @@ class Node {
     this.hasShrinkwrap = hasShrinkwrap || pkg._hasShrinkwrap || false
     this.legacyPeerDeps = legacyPeerDeps
 
-    this.children = new Map()
+    this.children = new CaseInsensitiveMap()
     this.fsChildren = new Set()
     this.inventory = new Inventory({})
     this.tops = new Set()
@@ -181,7 +182,7 @@ class Node {
     }
 
     this.edgesIn = new Set()
-    this.edgesOut = new Map()
+    this.edgesOut = new CaseInsensitiveMap()
 
     // have to set the internal package ref before assigning the parent,
     // because this.package is read when adding to inventory
@@ -1014,8 +1015,20 @@ class Node {
 
   replace (node) {
     this[_delistFromMeta]()
-    this.path = node.path
-    this.name = node.name
+
+    // if the name matches, but is not identical, we are intending to clobber
+    // something case-insensitively, so merely setting name and path won't
+    // have the desired effect.  just set the path so it'll collide in the
+    // parent's children map, and leave it at that.
+    const nameMatch = node.parent &&
+      node.parent.children.get(this.name) === node
+    if (nameMatch)
+      this.path = resolve(node.parent.path, 'node_modules', this.name)
+    else {
+      this.path = node.path
+      this.name = node.name
+    }
+
     if (!this.isLink)
       this.realpath = this.path
     this[_refreshLocation]()

tap-snapshots/test/arborist/reify.js.test.cjs

@@ -1912,6 +1912,100 @@ ArboristNode {
 }
 `
 
+exports[`test/arborist/reify.js TAP collide case-variant dep names > tree 1 1`] = `
+ArboristNode {
+  "children": Map {
+    "abbrev" => ArboristNode {
+      "edgesIn": Set {
+        EdgeIn {
+          "from": "",
+          "name": "abbrev",
+          "spec": "1",
+          "type": "prod",
+        },
+      },
+      "location": "node_modules/abbrev",
+      "name": "abbrev",
+      "path": "{CWD}/test/arborist/tap-testdir-reify-collide-case-variant-dep-names/node_modules/abbrev",
+      "resolved": "https://registry.npmjs.org/abbrev/-/abbrev-1.1.1.tgz",
+      "version": "1.1.1",
+    },
+  },
+  "edgesOut": Map {
+    "abbrev" => EdgeOut {
+      "name": "abbrev",
+      "spec": "1",
+      "to": "node_modules/abbrev",
+      "type": "prod",
+    },
+  },
+  "fsChildren": Set {
+    ArboristNode {
+      "dev": true,
+      "extraneous": true,
+      "location": "target",
+      "name": "target",
+      "optional": true,
+      "packageName": "ABBREV",
+      "path": "{CWD}/test/arborist/tap-testdir-reify-collide-case-variant-dep-names/target",
+      "peer": true,
+      "version": "1.0.0",
+    },
+  },
+  "isProjectRoot": true,
+  "location": "",
+  "name": "tap-testdir-reify-collide-case-variant-dep-names",
+  "path": "{CWD}/test/arborist/tap-testdir-reify-collide-case-variant-dep-names",
+}
+`
+
+exports[`test/arborist/reify.js TAP collide case-variant dep names > tree 2 1`] = `
+ArboristNode {
+  "children": Map {
+    "abbrev" => ArboristNode {
+      "edgesIn": Set {
+        EdgeIn {
+          "from": "",
+          "name": "abbrev",
+          "spec": "^1.1.1",
+          "type": "prod",
+        },
+      },
+      "location": "node_modules/abbrev",
+      "name": "abbrev",
+      "path": "{CWD}/test/arborist/tap-testdir-reify-collide-case-variant-dep-names/node_modules/abbrev",
+      "resolved": "https://registry.npmjs.org/abbrev/-/abbrev-1.1.1.tgz",
+      "version": "1.1.1",
+    },
+  },
+  "edgesOut": Map {
+    "abbrev" => EdgeOut {
+      "name": "abbrev",
+      "spec": "^1.1.1",
+      "to": "node_modules/abbrev",
+      "type": "prod",
+    },
+  },
+  "fsChildren": Set {
+    ArboristNode {
+      "dev": true,
+      "extraneous": true,
+      "location": "target",
+      "name": "target",
+      "optional": true,
+      "packageName": "ABBREV",
+      "path": "{CWD}/test/arborist/tap-testdir-reify-collide-case-variant-dep-names/target",
+      "peer": true,
+      "version": "1.0.0",
+    },
+  },
+  "isProjectRoot": true,
+  "location": "",
+  "name": "tap-testdir-reify-collide-case-variant-dep-names",
+  "path": "{CWD}/test/arborist/tap-testdir-reify-collide-case-variant-dep-names",
+}
+`
+
 exports[`test/arborist/reify.js TAP create link deps > expect resolving Promise 1`] = `
 ArboristNode {
   "children": Map {
@@ -4540,7 +4634,7 @@ ArboristNode {
         EdgeIn {
           "from": "",
           "name": "abbrev",
-          "spec": "1",
+          "spec": "latest",
           "type": "prod",
         },
       },
@@ -4550,48 +4644,14 @@ ArboristNode {
       "resolved": "https://registry.npmjs.org/abbrev/-/abbrev-1.1.1.tgz",
       "version": "1.1.1",
     },
-    "ABBREV" => ArboristLink {
-      "edgesIn": Set {
-        EdgeIn {
-          "from": "",
-          "name": "ABBREV",
-          "spec": "file:target",
-          "type": "prod",
-        },
-      },
-      "location": "node_modules/ABBREV",
-      "name": "ABBREV",
-      "path": "{CWD}/test/arborist/tap-testdir-reify-move-aside-symlink-clutter/node_modules/ABBREV",
-      "realpath": "{CWD}/test/arborist/tap-testdir-reify-move-aside-symlink-clutter/target",
-      "resolved": "file:../target",
-      "target": ArboristNode {
-        "location": "target",
-      },
-      "version": "1.0.0",
-    },
   },
   "edgesOut": Map {
     "abbrev" => EdgeOut {
       "name": "abbrev",
-      "spec": "1",
+      "spec": "latest",
       "to": "node_modules/abbrev",
       "type": "prod",
     },
-    "ABBREV" => EdgeOut {
-      "name": "ABBREV",
-      "spec": "file:target",
-      "to": "node_modules/ABBREV",
-      "type": "prod",
-    },
-  },
-  "fsChildren": Set {
-    ArboristNode {
-      "location": "target",
-      "name": "target",
-      "packageName": "ABBREV",
-      "path": "{CWD}/test/arborist/tap-testdir-reify-move-aside-symlink-clutter/target",
-      "version": "1.0.0",
-    },
   },
   "isProjectRoot": true,
   "location": "",