fix: inconsistent resolution of a peer shared through a diamond (#12081)

* fix: inconsistent resolution of a peer shared through a diamond

When a package peer-depends both another package and one of that
package's own peer dependencies (e.g. @typescript-eslint/eslint-plugin
peer-depends both @typescript-eslint/parser and typescript, and
@typescript-eslint/parser peer-depends typescript), pnpm reused a
hoisted instance of the shared peer that was resolved against a
different version, producing an inconsistent resolution.

Close #12079

* test: cover the peer-diamond resolution in pnpm and pacquet

Add @pnpm.e2e/peer-diamond-* fixtures modeling #12079 (plugin
peer-depends both parser and ts; parser peer-depends ts) and
integration tests on both stacks. The pnpm test guards the fix; the
pacquet test confirms pacquet already resolves the diamond consistently
(its merge always prefers the node's own child).

* docs: fix grammar in changeset (peer-depends on both)

.changeset/twelve-peers-diamond.md

@@ -0,0 +1,6 @@
+---
+"@pnpm/installing.deps-resolver": patch
+"pnpm": patch
+---
+
+Fixed inconsistent resolution of a peer dependency that is shared through a diamond. When a package peer-depends on both another package and one of that package's own peer dependencies (for example `@typescript-eslint/eslint-plugin` peer-depends on both `@typescript-eslint/parser` and `typescript`, and `@typescript-eslint/parser` peer-depends on `typescript`), pnpm no longer reuses a hoisted instance of the shared peer that was resolved against a different version [#12079](https://github.com/pnpm/pnpm/issues/12079).

installing/deps-installer/test/install/peerDependencies.ts

@@ -2044,6 +2044,26 @@ test('peer dependency cache is invalidated correctly when the peer of a peer mis
   expect(lockfile.snapshots['@pnpm.e2e/repeat-peers.d@1.0.0(@pnpm.e2e/repeat-peers.b@1.0.0(@pnpm.e2e/repeat-peers.a@2.0.0))']).toBeTruthy()
 })
 
+// https://github.com/pnpm/pnpm/issues/12079
+// peer-diamond-plugin peer-depends both peer-diamond-parser and peer-diamond-ts,
+// and peer-diamond-parser peer-depends peer-diamond-ts. The plugin's parser and
+// its ts must agree. A top-level parser resolved against ts@2.0.0 must not be
+// reused for the plugin, which is nested under ts@1.0.0.
+test('a peer shared through a diamond is resolved consistently', async () => {
+  const project = prepareEmpty()
+  await addDependenciesToPackage({}, [
+    '@pnpm.e2e/peer-diamond-ts@2.0.0',
+    '@pnpm.e2e/peer-diamond-parser@1.0.0',
+    '@pnpm.e2e/peer-diamond-app@1.0.0',
+  ], testDefaults({ autoInstallPeers: true }))
+
+  const lockfile = project.readLockfile()
+  const pluginSnapshots = Object.keys(lockfile.snapshots).filter((key) => key.includes('peer-diamond-plugin'))
+  expect(pluginSnapshots).toStrictEqual([
+    '@pnpm.e2e/peer-diamond-plugin@1.0.0(@pnpm.e2e/peer-diamond-parser@1.0.0(@pnpm.e2e/peer-diamond-ts@1.0.0))(@pnpm.e2e/peer-diamond-ts@1.0.0)',
+  ])
+})
+
 // Covers https://github.com/pnpm/pnpm/issues/8759
 test('detection of circular peer dependencies should not crash with aliased dependencies', async () => {
   prepareEmpty()

installing/deps-resolver/src/resolvePeers.ts

@@ -429,7 +429,10 @@ async function resolvePeersOfNode<T extends PartialResolvedPackage> (
     const _parentPkgsMatch = parentPkgsMatch.bind(null, ctx.dependenciesTree)
     for (const [newParentPkgName, newParentPkg] of Object.entries(newParentPkgs)) {
       if (parentPkgs[newParentPkgName]) {
-        if (!_parentPkgsMatch(parentPkgs[newParentPkgName], newParentPkg)) {
+        if (
+          !_parentPkgsMatch(parentPkgs[newParentPkgName], newParentPkg) ||
+          inheritedParentPkgBreaksPeerDiamond(ctx, parentPkgs, parentPkgs[newParentPkgName], newParentPkg, children)
+        ) {
           newParentPkg.occurrence = parentPkgs[newParentPkgName].occurrence + 1
           parentPkgs[newParentPkgName] = newParentPkg
         }
@@ -664,6 +667,68 @@ function parentPkgsMatch<T> (
   return currentParentResolvedPkg.name === newParentResolvedPkg.name
 }
 
+// A package that is both inherited from an ancestor and present among the
+// current node's own children is normally not duplicated: the inherited
+// instance is reused (see https://github.com/pnpm/pnpm/issues/8370). That reuse
+// is unsafe when a sibling peer-depends both this package and one of this
+// package's own peer dependencies. The sibling must see a single, consistent
+// instance of that shared peer, but the inherited instance resolved it in a
+// different context. In that case the node's own child has to be used instead.
+// See https://github.com/pnpm/pnpm/issues/12079
+function inheritedParentPkgBreaksPeerDiamond<T extends PartialResolvedPackage> (
+  ctx: {
+    dependenciesTree: DependenciesTree<T>
+    parentPkgsOfNode: ParentPkgsOfNode
+    allPeerDepNames: Set<string>
+  },
+  parentPkgs: ParentRefs,
+  inheritedParentPkg: ParentRef,
+  ownChildParentPkg: ParentRef,
+  children: ChildrenMap
+): boolean {
+  if (inheritedParentPkg.nodeId == null || ownChildParentPkg.nodeId == null) return false
+  if (inheritedParentPkg.nodeId === ownChildParentPkg.nodeId) return false
+  const inheritedContext = ctx.parentPkgsOfNode.get(inheritedParentPkg.nodeId)
+  if (inheritedContext == null) return false
+  const parentPkg = ctx.dependenciesTree.get(ownChildParentPkg.nodeId)?.resolvedPackage as T | undefined
+  if (parentPkg == null) return false
+
+  const conflictingPeers = new Set<string>()
+  for (const peerName of Object.keys(parentPkg.peerDependencies)) {
+    if (!ctx.allPeerDepNames.has(peerName)) continue
+    const inheritedPeer = inheritedContext[peerName]
+    const currentPeer = parentPkgs[peerName]
+    if (inheritedPeer == null || currentPeer == null) continue
+    if (parentPeerDiffers(ctx.dependenciesTree, currentPeer, inheritedPeer)) {
+      conflictingPeers.add(peerName)
+    }
+  }
+  if (conflictingPeers.size === 0) return false
+
+  for (const childNodeId of Object.values(children)) {
+    const childPeerDependencies = (ctx.dependenciesTree.get(childNodeId)?.resolvedPackage as T | undefined)?.peerDependencies
+    if (childPeerDependencies == null || childPeerDependencies[parentPkg.name] == null) continue
+    for (const peerName of conflictingPeers) {
+      if (childPeerDependencies[peerName] != null) return true
+    }
+  }
+  return false
+}
+
+function parentPeerDiffers<T extends PartialResolvedPackage> (
+  dependenciesTree: DependenciesTree<T>,
+  currentPeer: ParentRef,
+  inheritedPeer: ParentPkgInfo
+): boolean {
+  if (inheritedPeer.pkgIdWithPatchHash != null) {
+    if (currentPeer.nodeId == null || (typeof currentPeer.nodeId === 'string' && currentPeer.nodeId.startsWith('link:'))) {
+      return true
+    }
+    return (dependenciesTree.get(currentPeer.nodeId)?.resolvedPackage as T | undefined)?.pkgIdWithPatchHash !== inheritedPeer.pkgIdWithPatchHash
+  }
+  return currentPeer.version !== inheritedPeer.version
+}
+
 function findHit<T extends PartialResolvedPackage> (ctx: {
   parentPkgsOfNode: ParentPkgsOfNode
   peersCache: PeersCache

installing/deps-resolver/test/resolvePeers.ts

@@ -925,4 +925,130 @@ describe('dedupePeers', () => {
     expect(dependenciesByProjectId['project-a'].get('plugin')).toBe('plugin/1.0.0(react@17.0.0)')
     expect(dependenciesByProjectId['project-b'].get('plugin')).toBe('plugin/1.0.0(react@18.0.0)')
   })
+
+  // https://github.com/pnpm/pnpm/issues/12079
+  test("a peer's own peer is shared with a sibling that peer-depends both", async () => {
+    // plugin peer-depends both parser and typescript; parser peer-depends typescript.
+    // So plugin's parser and plugin's typescript must agree. A top-level parser that
+    // resolved typescript@2.0.0 must not shadow umbrella's own parser, which resolves
+    // typescript@1.0.0 — the version that plugin itself uses.
+    const ts1Pkg = {
+      name: 'typescript',
+      pkgIdWithPatchHash: 'typescript/1.0.0' as PkgIdWithPatchHash,
+      version: '1.0.0',
+      peerDependencies: {} as PeerDependencies,
+      id: '' as PkgResolutionId,
+    }
+    const ts2Pkg = {
+      name: 'typescript',
+      pkgIdWithPatchHash: 'typescript/2.0.0' as PkgIdWithPatchHash,
+      version: '2.0.0',
+      peerDependencies: {} as PeerDependencies,
+      id: '' as PkgResolutionId,
+    }
+    const parserPkg = {
+      name: 'parser',
+      pkgIdWithPatchHash: 'parser/1.0.0' as PkgIdWithPatchHash,
+      version: '1.0.0',
+      peerDependencies: { typescript: { version: '*' } },
+      id: '' as PkgResolutionId,
+    }
+    const pluginPkg = {
+      name: 'plugin',
+      pkgIdWithPatchHash: 'plugin/1.0.0' as PkgIdWithPatchHash,
+      version: '1.0.0',
+      peerDependencies: { parser: { version: '*' }, typescript: { version: '*' } },
+      id: '' as PkgResolutionId,
+    }
+    const umbrellaPkg = {
+      name: 'umbrella',
+      pkgIdWithPatchHash: 'umbrella/1.0.0' as PkgIdWithPatchHash,
+      version: '1.0.0',
+      peerDependencies: { typescript: { version: '*' } },
+      id: '' as PkgResolutionId,
+    }
+    const appPkg = {
+      name: 'app',
+      pkgIdWithPatchHash: 'app/1.0.0' as PkgIdWithPatchHash,
+      version: '1.0.0',
+      peerDependencies: {} as PeerDependencies,
+      id: '' as PkgResolutionId,
+    }
+    const { dependenciesGraph } = await resolvePeers({
+      allPeerDepNames: new Set(['typescript', 'parser']),
+      projects: [
+        {
+          directNodeIdsByAlias: new Map([
+            ['typescript', '>typescript/2.0.0>' as NodeId],
+            ['parser', '>parser/1.0.0>' as NodeId],
+            ['app', '>app/1.0.0>' as NodeId],
+          ]),
+          topParents: [],
+          rootDir: '' as ProjectRootDir,
+          id: '.',
+        },
+      ],
+      resolvedImporters: {},
+      dependenciesTree: new Map<NodeId, DependenciesTreeNode<PartialResolvedPackage>>([
+        ['>typescript/2.0.0>' as NodeId, {
+          children: {},
+          installable: true,
+          resolvedPackage: ts2Pkg,
+          depth: 0,
+        }],
+        ['>parser/1.0.0>' as NodeId, {
+          children: {},
+          installable: true,
+          resolvedPackage: parserPkg,
+          depth: 0,
+        }],
+        ['>app/1.0.0>' as NodeId, {
+          children: {
+            typescript: '>app/1.0.0>typescript/1.0.0>' as NodeId,
+            umbrella: '>app/1.0.0>umbrella/1.0.0>' as NodeId,
+          },
+          installable: true,
+          resolvedPackage: appPkg,
+          depth: 0,
+        }],
+        ['>app/1.0.0>typescript/1.0.0>' as NodeId, {
+          children: {},
+          installable: true,
+          resolvedPackage: ts1Pkg,
+          depth: 1,
+        }],
+        ['>app/1.0.0>umbrella/1.0.0>' as NodeId, {
+          children: {
+            plugin: '>app/1.0.0>umbrella/1.0.0>plugin/1.0.0>' as NodeId,
+            parser: '>app/1.0.0>umbrella/1.0.0>parser/1.0.0>' as NodeId,
+          },
+          installable: true,
+          resolvedPackage: umbrellaPkg,
+          depth: 1,
+        }],
+        ['>app/1.0.0>umbrella/1.0.0>plugin/1.0.0>' as NodeId, {
+          children: {},
+          installable: true,
+          resolvedPackage: pluginPkg,
+          depth: 2,
+        }],
+        ['>app/1.0.0>umbrella/1.0.0>parser/1.0.0>' as NodeId, {
+          children: {},
+          installable: true,
+          resolvedPackage: parserPkg,
+          depth: 2,
+        }],
+      ]),
+      virtualStoreDir: '',
+      virtualStoreDirMaxLength: 120,
+      lockfileDir: '',
+      peersSuffixMaxLength: 1000,
+      workspaceProjectIds: new Set(),
+    })
+    // plugin and its parser both use typescript@1.0.0; the typescript@2.0.0 parser
+    // from the top level is not pulled into plugin.
+    const depPaths = Object.keys(dependenciesGraph)
+    expect(depPaths).toContain('plugin/1.0.0(parser/1.0.0(typescript/1.0.0))(typescript/1.0.0)')
+    expect(depPaths).not.toContain('plugin/1.0.0(parser/1.0.0(typescript/2.0.0))(typescript/1.0.0)')
+  })
 })

pacquet/crates/cli/tests/install.rs

@@ -366,6 +366,53 @@ fn auto_install_peers_hoists_missing_peers_at_importer() {
     drop((root, mock_instance)); // cleanup
 }
 
+/// `peer-diamond-plugin` peer-depends both `peer-diamond-parser` and
+/// `peer-diamond-ts`, and `peer-diamond-parser` peer-depends
+/// `peer-diamond-ts`. The plugin's parser and its ts must agree: when
+/// the plugin resolves `ts@1.0.0`, its parser peer must also be the
+/// `ts@1.0.0` instance, not a `ts@2.0.0` parser hoisted at the root.
+///
+/// This is the scenario behind the pnpm regression in
+/// [pnpm/pnpm#12079](https://github.com/pnpm/pnpm/issues/12079). pacquet
+/// resolves it consistently — `bump_occurrence_on_shadow` always prefers
+/// the node's own child over an inherited same-version instance, so it
+/// never reuses the root-level `ts@2.0.0` parser for the nested plugin.
+/// Mirrors the upstream coverage in
+/// [`installing/deps-installer/test/install/peerDependencies.ts`](https://github.com/pnpm/pnpm/blob/762e80be49/installing/deps-installer/test/install/peerDependencies.ts).
+#[test]
+fn peer_shared_through_a_diamond_is_resolved_consistently() {
+    let CommandTempCwd { pacquet, root, workspace, npmrc_info, .. } =
+        CommandTempCwd::init().add_mocked_registry();
+    let AddMockedRegistry { mock_instance, .. } = npmrc_info;
+
+    let manifest_path = workspace.join("package.json");
+    let package_json_content = serde_json::json!({
+        "dependencies": {
+            "@pnpm.e2e/peer-diamond-ts": "2.0.0",
+            "@pnpm.e2e/peer-diamond-parser": "1.0.0",
+            "@pnpm.e2e/peer-diamond-app": "1.0.0",
+        },
+    });
+    fs::write(&manifest_path, package_json_content.to_string()).expect("write to package.json");
+
+    pacquet.with_arg("install").assert().success();
+
+    let lockfile =
+        fs::read_to_string(workspace.join("pnpm-lock.yaml")).expect("read pnpm-lock.yaml");
+    let consistent = "@pnpm.e2e/peer-diamond-plugin@1.0.0(@pnpm.e2e/peer-diamond-parser@1.0.0(@pnpm.e2e/peer-diamond-ts@1.0.0))(@pnpm.e2e/peer-diamond-ts@1.0.0)";
+    let inconsistent = "@pnpm.e2e/peer-diamond-plugin@1.0.0(@pnpm.e2e/peer-diamond-parser@1.0.0(@pnpm.e2e/peer-diamond-ts@2.0.0))";
+    assert!(
+        lockfile.contains(consistent),
+        "expected the plugin to share ts@1.0.0 with its parser; lockfile:\n{lockfile}",
+    );
+    assert!(
+        !lockfile.contains(inconsistent),
+        "the plugin must not be paired with a ts@2.0.0 parser; lockfile:\n{lockfile}",
+    );
+
+    drop((root, mock_instance)); // cleanup
+}
+
 /// `catalog:` on a direct dep should be dereferenced through
 /// `pnpm-workspace.yaml`'s `catalog` section before the npm resolver
 /// sees it. The fetched virtual-store entry is the catalog's resolved

pnpr/.fixtures/packages/@pnpm.e2e/peer-diamond-app/1.0.0/package.json

@@ -0,0 +1,8 @@
+{
+  "name": "@pnpm.e2e/peer-diamond-app",
+  "version": "1.0.0",
+  "dependencies": {
+    "@pnpm.e2e/peer-diamond-ts": "1.0.0",
+    "@pnpm.e2e/peer-diamond-bundle": "1.0.0"
+  }
+}

pnpr/.fixtures/packages/@pnpm.e2e/peer-diamond-bundle/1.0.0/package.json

@@ -0,0 +1,11 @@
+{
+  "name": "@pnpm.e2e/peer-diamond-bundle",
+  "version": "1.0.0",
+  "dependencies": {
+    "@pnpm.e2e/peer-diamond-plugin": "1.0.0",
+    "@pnpm.e2e/peer-diamond-parser": "1.0.0"
+  },
+  "peerDependencies": {
+    "@pnpm.e2e/peer-diamond-ts": "*"
+  }
+}

pnpr/.fixtures/packages/@pnpm.e2e/peer-diamond-parser/1.0.0/package.json

@@ -0,0 +1,7 @@
+{
+  "name": "@pnpm.e2e/peer-diamond-parser",
+  "version": "1.0.0",
+  "peerDependencies": {
+    "@pnpm.e2e/peer-diamond-ts": "*"
+  }
+}

pnpr/.fixtures/packages/@pnpm.e2e/peer-diamond-plugin/1.0.0/package.json

@@ -0,0 +1,8 @@
+{
+  "name": "@pnpm.e2e/peer-diamond-plugin",
+  "version": "1.0.0",
+  "peerDependencies": {
+    "@pnpm.e2e/peer-diamond-parser": "*",
+    "@pnpm.e2e/peer-diamond-ts": "*"
+  }
+}

pnpr/.fixtures/packages/@pnpm.e2e/peer-diamond-ts/1.0.0/package.json

@@ -0,0 +1,4 @@
+{
+  "name": "@pnpm.e2e/peer-diamond-ts",
+  "version": "1.0.0"
+}

pnpr/.fixtures/packages/@pnpm.e2e/peer-diamond-ts/2.0.0/package.json

@@ -0,0 +1,4 @@
+{
+  "name": "@pnpm.e2e/peer-diamond-ts",
+  "version": "2.0.0"
+}