From fecfe8334b47f0acb742a4d78f8cc9e50c64c52b Mon Sep 17 00:00:00 2001 From: "C. Spencer Beggs" Date: Thu, 9 Jul 2026 16:00:20 -0400 Subject: [PATCH] fix(resolver): keep hoisted peer providers out of the root peer context (#12847) With autoInstallPeers, peers resolved inside a dependency subtree are attached to the root importer's direct dependencies so other subtrees can reuse them. Those entries alias tree nodes that already have a position deep in the graph, and the peer-resolution pass walked them a second time as root children. The two walks raced on the shared per-node dep-path state, so a package inside a self-contained closure could get its peers bound to the root project's incompatible version of a peer instead of the provider next to it in the tree, producing a lockfile that mixes both versions and a peer mismatch at run time. The peer-resolution pass now keeps the attached providers visible as root-level peer providers but resolves their own peers only at their true tree position, falling back to the root context only when that position was pruned by the peers cache. All pruned providers are resolved in a single fallback pass, because a pass only detects peer cycles among its own children and mutually peer-depending providers would otherwise await each other's dep path forever. The fix lands in both stacks: the TypeScript `@pnpm/installing.deps-resolver` and pacquet's `resolving-deps-resolver` crate. Fixes https://github.com/pnpm/pnpm/issues/4993 --------- Signed-off-by: C. Spencer Beggs Co-authored-by: Zoltan Kochan --- .changeset/peer-closures-root-context.md | 6 + .../package-manager/src/install/tests.rs | 97 +++++++++ .../src/resolve_importer.rs | 17 +- .../src/resolve_peers.rs | 68 +++++- .../src/resolve_peers/tests.rs | 200 +++++++++++++++++- .../src/resolve_workspace.rs | 6 +- .../resolving-deps-resolver/src/tests.rs | 2 +- .../with-git-protocol-dep/pnpm-lock.yaml | 5 +- .../test/install/autoInstallPeers.ts | 29 +++ pnpm11/installing/deps-resolver/src/index.ts | 2 + .../deps-resolver/src/resolveDependencies.ts | 12 +- .../src/resolveDependencyTree.ts | 2 + .../deps-resolver/src/resolvePeers.ts | 49 ++++- .../deps-resolver/test/resolvePeers.ts | 144 +++++++++++++ .../closure-lib-a/1.0.0/package.json | 8 + .../closure-lib-b/1.0.0/package.json | 8 + .../closure-peer-x/1.0.0/package.json | 4 + .../closure-peer-x/2.0.0/package.json | 4 + .../closure-plugins/1.0.0/package.json | 9 + 19 files changed, 653 insertions(+), 19 deletions(-) create mode 100644 .changeset/peer-closures-root-context.md create mode 100644 pnpr/.fixtures/packages/@pnpm.e2e/closure-lib-a/1.0.0/package.json create mode 100644 pnpr/.fixtures/packages/@pnpm.e2e/closure-lib-b/1.0.0/package.json create mode 100644 pnpr/.fixtures/packages/@pnpm.e2e/closure-peer-x/1.0.0/package.json create mode 100644 pnpr/.fixtures/packages/@pnpm.e2e/closure-peer-x/2.0.0/package.json create mode 100644 pnpr/.fixtures/packages/@pnpm.e2e/closure-plugins/1.0.0/package.json diff --git a/.changeset/peer-closures-root-context.md b/.changeset/peer-closures-root-context.md new file mode 100644 index 0000000000..d682c8f092 --- /dev/null +++ b/.changeset/peer-closures-root-context.md @@ -0,0 +1,6 @@ +--- +"@pnpm/installing.deps-resolver": patch +"pnpm": patch +--- + +Fixed peer dependency resolution with `autoInstallPeers` when a workspace package depends on a version of a package that a transitive dependency's self-contained closure also provides for itself. The peer providers that are attached to the root project for reuse are no longer peer-resolved a second time in the root context, so packages inside such a closure no longer get their peers bound to the root project's incompatible version [#4993](https://github.com/pnpm/pnpm/issues/4993). diff --git a/pacquet/crates/package-manager/src/install/tests.rs b/pacquet/crates/package-manager/src/install/tests.rs index 7235ad8684..5a48e3d4ad 100644 --- a/pacquet/crates/package-manager/src/install/tests.rs +++ b/pacquet/crates/package-manager/src/install/tests.rs @@ -1720,6 +1720,103 @@ async fn auto_install_peers_skips_meta_only_optional_peers() { drop((dir, mock_instance)); } +/// Mirror of the TS test "a root dependency does not override the +/// peers provided inside a self-contained subtree" +/// (`deps-installer/test/install/autoInstallPeers.ts`). +/// +/// `@pnpm.e2e/closure-plugins` provides every peer of its own subtree +/// (`closure-lib-a` and `closure-lib-b` peer-depend on each other and +/// on `closure-peer-x`, and all three are its regular dependencies). +/// The root project additionally depends on the incompatible +/// `closure-peer-x@2.0.0`. With `autoInstallPeers` (the default), the +/// peers resolved inside the subtree are attached to the root project +/// for reuse — but they must not be peer-resolved again in the root +/// context, where `closure-peer-x@2.0.0` is the nearest provider, or +/// the subtree's peer graph gets a mix of both versions. +#[tokio::test] +async fn root_dependency_does_not_override_peers_of_self_contained_subtree() { + let mock_instance = TestRegistry::start(); + + let dir = tempdir().unwrap(); + let store_dir = dir.path().join("pacquet-store"); + let project_root = dir.path().join("project"); + let modules_dir = project_root.join("node_modules"); + let virtual_store_dir = modules_dir.join(".pacquet"); + + let manifest_path = dir.path().join("package.json"); + let mut manifest = PackageManifest::create_if_needed(manifest_path.clone()).unwrap(); + manifest.add_dependency("@pnpm.e2e/closure-plugins", "1.0.0", DependencyGroup::Prod).unwrap(); + manifest.add_dependency("@pnpm.e2e/closure-peer-x", "2.0.0", DependencyGroup::Prod).unwrap(); + manifest.save().unwrap(); + + let mut config = Config::new(); + config.store_dir = store_dir.into(); + config.modules_dir = modules_dir.clone(); + config.virtual_store_dir = virtual_store_dir; + config.registry = mock_instance.url(); + let config = config.leak(); + + Install { + tarball_mem_cache: Default::default(), + http_client: &Default::default(), + http_client_arc: std::sync::Arc::new(Default::default()), + config, + manifest: &manifest, + emit_initial_manifest: true, + lockfile: MaybeLazyLockfile::Loaded(None), + lockfile_path: None, + dependency_groups: [DependencyGroup::Prod, DependencyGroup::Dev, DependencyGroup::Optional], + frozen_lockfile: false, + prefer_frozen_lockfile: None, + ignore_manifest_check: false, + skip_runtimes: false, + trust_lockfile: false, + update_checksums: false, + is_full_install: true, + supported_architectures: None, + node_linker: pacquet_config::NodeLinker::default(), + lockfile_only: false, + dry_run: false, + resolved_packages: &Default::default(), + update_seed_policy: crate::UpdateSeedPolicy::KeepAll, + auth_override: None, + resolution_observer: None, + catalogs_override: None, + disable_optimistic_repeat_install: false, + pnpmfile_hook_override: None, + workspace_projects_override: None, + } + .run::() + .await + .expect("install should succeed"); + + let content = + std::fs::read_to_string(dir.path().join(Lockfile::FILE_NAME)).expect("read pnpm-lock.yaml"); + assert!( + !content.contains("(@pnpm.e2e/closure-peer-x@2.0.0)"), + "no peer inside the self-contained subtree may bind to the root's \ + closure-peer-x@2.0.0; lockfile:\n{content}", + ); + assert!( + content.contains("(@pnpm.e2e/closure-peer-x@1.0.0)"), + "the subtree's peers must bind to its own closure-peer-x@1.0.0; lockfile:\n{content}", + ); + + // The root keeps its own explicitly declared version. + let lockfile: Lockfile = serde_saphyr::from_str(&content).expect("parse pnpm-lock.yaml"); + let root_deps = lockfile + .root_project() + .expect("root importer recorded") + .dependencies + .as_ref() + .expect("dependencies map"); + let peer_x_key = pacquet_lockfile::PkgName::parse("@pnpm.e2e/closure-peer-x").unwrap(); + let root_peer_x = root_deps.get(&peer_x_key).expect("closure-peer-x recorded at root"); + assert_eq!(root_peer_x.version.to_string(), "2.0.0"); + + drop((dir, mock_instance)); +} + /// A v9 lockfile fixture pinned to a placeholder package whose /// integrity is bogus on purpose. Pacquet enforces tarball integrity /// on the install path, so any test that lets the install reach the diff --git a/pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs b/pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs index 697a9d2e87..67c9bbca46 100644 --- a/pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs +++ b/pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs @@ -285,6 +285,12 @@ pub(crate) struct ImporterHoistState { importer_id: String, ctx: TreeCtx, direct: Vec, + /// `NodeIds` appended to `direct` by + /// [`Self::append_resolved_peer_providers`]. Threaded into + /// [`ResolvePeersOptions::hoisted_peer_provider_node_ids`] so the + /// peer walk resolves them at their tree position instead of the + /// importer root. + hoisted_peer_provider_node_ids: HashSet, parent_pkg_aliases: HashSet, all_missing_optional_peers: BTreeMap>, all_preferred_versions: PreferredVersions, @@ -357,6 +363,7 @@ impl ImporterHoistState { importer_id: importer_id.to_string(), ctx, direct, + hoisted_peer_provider_node_ids: HashSet::new(), parent_pkg_aliases, all_missing_optional_peers: BTreeMap::new(), all_preferred_versions, @@ -379,6 +386,7 @@ impl ImporterHoistState { lockfile_dir: self.lockfile_dir.clone(), modules_dir: self.modules_dir.clone(), hoist_missing_scope: None, + hoisted_peer_provider_node_ids: self.hoisted_peer_provider_node_ids.clone(), } } @@ -517,6 +525,7 @@ impl ImporterHoistState { node_id: node_id.clone(), id: tree_node.resolved_package_id.clone(), }); + self.hoisted_peer_provider_node_ids.insert(node_id.clone()); self.parent_pkg_aliases.insert(alias.clone()); } } @@ -558,9 +567,11 @@ impl ImporterHoistState { /// The importer's direct-dep envelopes for the workspace-wide peer /// pass (which recomputes peers across importers; the per-importer - /// pass would be discarded). - pub(crate) fn into_direct(self) -> Vec { - self.direct + /// pass would be discarded), together with the `NodeIds` of the peer + /// providers among them (see + /// [`ResolvePeersOptions::hoisted_peer_provider_node_ids`]). + pub(crate) fn into_direct(self) -> (Vec, HashSet) { + (self.direct, self.hoisted_peer_provider_node_ids) } /// Run the final per-importer peer pass and emit the result. Used diff --git a/pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs b/pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs index ee3d2c281a..c97ce21e9e 100644 --- a/pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs +++ b/pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs @@ -137,6 +137,19 @@ pub struct ResolvePeersOptions { /// per-importer snapshots shared across every hoist-loop /// iteration. pub hoist_missing_scope: Option>, + + /// `NodeIds` of the peer providers the auto-install-peers loop + /// attached to importers' direct dependencies for reuse by other + /// subtrees. Each such node keeps its original position inside + /// the dependency tree, so the walk resolves its peers there; + /// walking it a second time as a root child would bind its peers + /// against the importer's own dependencies instead of the + /// providers next to it in the tree, and the root-context result + /// would overwrite the in-place one in `node_dep_paths`. The walk + /// therefore skips these direct entries unless the tree position + /// was pruned (an ancestor hit the peers cache) and nothing else + /// resolved them. + pub hoisted_peer_provider_node_ids: std::collections::HashSet, } /// See [`ResolvePeersOptions::hoist_missing_scope`]. @@ -184,6 +197,7 @@ impl Default for ResolvePeersOptions { lockfile_dir: None, modules_dir: None, hoist_missing_scope: None, + hoisted_peer_provider_node_ids: std::collections::HashSet::new(), } } } @@ -331,12 +345,35 @@ pub fn resolve_peers_workspace( let parent_node_ids: Vec = Vec::new(); let parent_pkg_ids_chain: Vec = Vec::new(); let importer_parent_dep_paths = walker.parent_dep_paths_from_refs(&importer_parents); - for dep in &importer.direct { + let (own_direct, provider_direct): (Vec<&DirectDep>, Vec<&DirectDep>) = importer + .direct + .iter() + .partition(|dep| !walker.opts.hoisted_peer_provider_node_ids.contains(&dep.node_id)); + for dep in &own_direct { walker .parent_pkgs_of_node .insert(dep.node_id.clone(), importer_parent_dep_paths.clone()); } - for dep in &importer.direct { + for dep in &own_direct { + walker.resolve_node( + dep.node_id.clone(), + &importer_parents, + &parent_chain_names, + &parent_node_ids, + &parent_pkg_ids_chain, + ); + } + // See ResolvePeersOptions::hoisted_peer_provider_node_ids — a + // provider is normally resolved at its tree position during the + // walk above; only one whose position was pruned still needs the + // root-context fallback. + for dep in &provider_direct { + if walker.node_dep_paths.contains_key(&dep.node_id) { + continue; + } + walker + .parent_pkgs_of_node + .insert(dep.node_id.clone(), importer_parent_dep_paths.clone()); walker.resolve_node( dep.node_id.clone(), &importer_parents, @@ -605,10 +642,33 @@ impl Walker<'_> { // `self.tree.direct`. let direct: Vec = self.tree.direct.clone(); let importer_parent_dep_paths = self.parent_dep_paths_from_refs(&importer_parents); - for dep in &direct { + let (own_direct, provider_direct): (Vec<&DirectDep>, Vec<&DirectDep>) = direct + .iter() + .partition(|dep| !self.opts.hoisted_peer_provider_node_ids.contains(&dep.node_id)); + for dep in &own_direct { self.parent_pkgs_of_node.insert(dep.node_id.clone(), importer_parent_dep_paths.clone()); } - for dep in &direct { + for dep in &own_direct { + let output = self.resolve_node( + dep.node_id.clone(), + &importer_parents, + &parent_chain_names, + &parent_node_ids, + &parent_pkg_ids_chain, + ); + for (peer_alias, peer_node_id) in output.auto_install_resolved_peers { + self.resolved_peer_providers_by_alias.insert(peer_alias, peer_node_id); + } + } + // See ResolvePeersOptions::hoisted_peer_provider_node_ids — a + // provider is normally resolved at its tree position during the + // walk above; only one whose position was pruned still needs the + // root-context fallback. + for dep in &provider_direct { + if self.node_dep_paths.contains_key(&dep.node_id) { + continue; + } + self.parent_pkgs_of_node.insert(dep.node_id.clone(), importer_parent_dep_paths.clone()); let output = self.resolve_node( dep.node_id.clone(), &importer_parents, diff --git a/pacquet/crates/resolving-deps-resolver/src/resolve_peers/tests.rs b/pacquet/crates/resolving-deps-resolver/src/resolve_peers/tests.rs index 210dac3361..f3dfec1944 100644 --- a/pacquet/crates/resolving-deps-resolver/src/resolve_peers/tests.rs +++ b/pacquet/crates/resolving-deps-resolver/src/resolve_peers/tests.rs @@ -1,4 +1,7 @@ -use super::{NodeRecord, ResolvePeersOptions, Walker, resolve_peers, satisfies_with_prereleases}; +use super::{ + ImporterPeerInput, NodeRecord, ResolvePeersOptions, Walker, resolve_peers, + resolve_peers_workspace, satisfies_with_prereleases, +}; use crate::{ dependencies_graph::{DependenciesGraph, PeerDependencyIssues}, node_id::NodeId, @@ -1203,6 +1206,201 @@ fn shared_package_optional_transitive_peer_resolves_deterministically() { } } +/// A hoisted peer provider whose tree position was never visited (nothing in +/// the walk enumerates its node) must still be resolved by the root-context +/// fallback so consumers that bound it get a depPath. +#[test] +fn pruned_hoisted_provider_falls_back_to_root_resolution() { + let prov = NodeId::leaf("prov@1.0.0"); + let consumer = NodeId::next(); + + let mut tree = ResolvedTree { + direct: vec![ + DirectDep { + alias: "consumer".to_string(), + node_id: consumer.clone(), + id: "consumer@1.0.0".to_string(), + }, + DirectDep { + alias: "prov".to_string(), + node_id: prov.clone(), + id: "prov@1.0.0".to_string(), + }, + ], + packages: HashMap::from([ + ("prov@1.0.0".to_string(), package("prov", "1.0.0", &[], true)), + ("consumer@1.0.0".to_string(), package("consumer", "1.0.0", &[("prov", "*")], false)), + ]), + dependencies_tree: HashMap::from([ + (prov.clone(), tree_node("prov@1.0.0", BTreeMap::new(), 1)), + (consumer, tree_node("consumer@1.0.0", BTreeMap::new(), 0)), + ]), + all_peer_dep_names: HashSet::from(["prov".to_string()]), + policy_violations: Vec::new(), + applied_patches: HashSet::new(), + children_by_id: HashMap::new(), + }; + + let result = resolve_peers( + &mut tree, + ResolvePeersOptions { + hoisted_peer_provider_node_ids: HashSet::from([prov]), + ..ResolvePeersOptions::default() + }, + ); + + assert_eq!( + result.direct_dependencies_by_alias.get("prov"), + Some(&DepPath::from("prov@1.0.0")), + "the pruned provider must get a depPath from the fallback", + ); + assert!( + result.graph.contains_key(&DepPath::from("consumer@1.0.0(prov@1.0.0)")), + "the consumer must bind the fallback-resolved provider: {:#?}", + result.graph.keys().collect::>(), + ); +} + +/// Same as [`pruned_hoisted_provider_falls_back_to_root_resolution`] but +/// through the multi-importer entry point. +#[test] +fn pruned_hoisted_provider_falls_back_in_workspace_pass() { + let prov = NodeId::leaf("prov@1.0.0"); + let consumer = NodeId::next(); + + let importer = ImporterPeerInput { + id: ".".to_string(), + direct: vec![ + DirectDep { + alias: "consumer".to_string(), + node_id: consumer.clone(), + id: "consumer@1.0.0".to_string(), + }, + DirectDep { + alias: "prov".to_string(), + node_id: prov.clone(), + id: "prov@1.0.0".to_string(), + }, + ], + root_dir: std::path::PathBuf::from("/repo"), + modules_dir: None, + }; + let mut tree = ResolvedTree { + direct: Vec::new(), + packages: HashMap::from([ + ("prov@1.0.0".to_string(), package("prov", "1.0.0", &[], true)), + ("consumer@1.0.0".to_string(), package("consumer", "1.0.0", &[("prov", "*")], false)), + ]), + dependencies_tree: HashMap::from([ + (prov.clone(), tree_node("prov@1.0.0", BTreeMap::new(), 1)), + (consumer, tree_node("consumer@1.0.0", BTreeMap::new(), 0)), + ]), + all_peer_dep_names: HashSet::from(["prov".to_string()]), + policy_violations: Vec::new(), + applied_patches: HashSet::new(), + children_by_id: HashMap::new(), + }; + + let result = resolve_peers_workspace( + &mut tree, + &[importer], + std::path::Path::new("/repo"), + false, + false, + false, + ResolvePeersOptions { + hoisted_peer_provider_node_ids: HashSet::from([prov]), + ..ResolvePeersOptions::default() + }, + ); + + assert_eq!( + result.direct_dependencies_by_importer.get(".").and_then(|deps| deps.get("prov")), + Some(&DepPath::from("prov@1.0.0")), + "the pruned provider must get a depPath from the fallback", + ); + assert!( + result.graph.contains_key(&DepPath::from("consumer@1.0.0(prov@1.0.0)")), + "the consumer must bind the fallback-resolved provider: {:#?}", + result.graph.keys().collect::>(), + ); +} + +/// Mirror of the TS test "pruned hoisted peer providers that peer-depend on +/// each other are resolved together" (`deps-resolver/test/resolvePeers.ts`): +/// two pruned providers form a peer cycle, so each one's suffix depends on +/// the other's. Both must come out of the fallback with the cycle collapsed +/// to `name@version`, matching the in-place cycle handling. +#[test] +fn pruned_hoisted_providers_with_mutual_peers_resolve() { + let lib_a = NodeId::leaf("lib-a@1.0.0"); + let lib_b = NodeId::leaf("lib-b@1.0.0"); + let consumer = NodeId::next(); + + let mut tree = ResolvedTree { + direct: vec![ + DirectDep { + alias: "consumer".to_string(), + node_id: consumer.clone(), + id: "consumer@1.0.0".to_string(), + }, + DirectDep { + alias: "lib-a".to_string(), + node_id: lib_a.clone(), + id: "lib-a@1.0.0".to_string(), + }, + DirectDep { + alias: "lib-b".to_string(), + node_id: lib_b.clone(), + id: "lib-b@1.0.0".to_string(), + }, + ], + packages: HashMap::from([ + ("lib-a@1.0.0".to_string(), package("lib-a", "1.0.0", &[("lib-b", "^1.0.0")], true)), + ("lib-b@1.0.0".to_string(), package("lib-b", "1.0.0", &[("lib-a", "^1.0.0")], true)), + ( + "consumer@1.0.0".to_string(), + package("consumer", "1.0.0", &[("lib-a", "^1.0.0"), ("lib-b", "^1.0.0")], false), + ), + ]), + dependencies_tree: HashMap::from([ + (lib_a.clone(), tree_node("lib-a@1.0.0", BTreeMap::new(), 1)), + (lib_b.clone(), tree_node("lib-b@1.0.0", BTreeMap::new(), 1)), + (consumer, tree_node("consumer@1.0.0", BTreeMap::new(), 0)), + ]), + all_peer_dep_names: HashSet::from(["lib-a".to_string(), "lib-b".to_string()]), + policy_violations: Vec::new(), + applied_patches: HashSet::new(), + children_by_id: HashMap::new(), + }; + + let result = resolve_peers( + &mut tree, + ResolvePeersOptions { + hoisted_peer_provider_node_ids: HashSet::from([lib_a, lib_b]), + ..ResolvePeersOptions::default() + }, + ); + + assert_eq!( + result.direct_dependencies_by_alias.get("lib-a"), + Some(&DepPath::from("lib-a@1.0.0(lib-b@1.0.0)")), + "graph keys: {:#?}", + result.graph.keys().collect::>(), + ); + assert_eq!( + result.direct_dependencies_by_alias.get("lib-b"), + Some(&DepPath::from("lib-b@1.0.0(lib-a@1.0.0)")), + "graph keys: {:#?}", + result.graph.keys().collect::>(), + ); + assert!( + result.graph.contains_key(&DepPath::from("consumer@1.0.0(lib-a@1.0.0)(lib-b@1.0.0)")), + "the consumer must bind both fallback-resolved providers: {:#?}", + result.graph.keys().collect::>(), + ); +} + fn tree_node(pkg_id: &str, children: BTreeMap, depth: i32) -> DependenciesTreeNode { DependenciesTreeNode { resolved_package_id: pkg_id.to_string(), diff --git a/pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rs b/pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rs index ffbe662017..f92d1d5609 100644 --- a/pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rs +++ b/pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rs @@ -253,12 +253,15 @@ where } } let mut per_importer_inputs: Vec = Vec::with_capacity(importers.len()); + let mut hoisted_peer_provider_node_ids = std::collections::HashSet::new(); for ((importer, state), (project_dir, modules_dir)) in importers.iter().zip(states).zip(input_dirs) { + let (direct, importer_provider_node_ids) = state.into_direct(); + hoisted_peer_provider_node_ids.extend(importer_provider_node_ids); per_importer_inputs.push(ImporterPeerInput { id: importer.id.clone(), - direct: state.into_direct(), + direct, root_dir: project_dir, modules_dir, }); @@ -284,6 +287,7 @@ where // importer's walk. modules_dir: None, hoist_missing_scope: None, + hoisted_peer_provider_node_ids, }; let peers = resolve_peers_workspace( &mut merged_tree, diff --git a/pacquet/crates/resolving-deps-resolver/src/tests.rs b/pacquet/crates/resolving-deps-resolver/src/tests.rs index 8e5cbfbde3..ddf5162d82 100644 --- a/pacquet/crates/resolving-deps-resolver/src/tests.rs +++ b/pacquet/crates/resolving-deps-resolver/src/tests.rs @@ -2152,7 +2152,7 @@ mod peers { exclude_links_from_lockfile: true, lockfile_dir: Some(lockfile_dir), modules_dir: Some(modules_dir), - hoist_missing_scope: None, + ..ResolvePeersOptions::default() }, ); diff --git a/pnpm11/__fixtures__/with-git-protocol-dep/pnpm-lock.yaml b/pnpm11/__fixtures__/with-git-protocol-dep/pnpm-lock.yaml index fcab7a6117..cd3fc01ec0 100644 --- a/pnpm11/__fixtures__/with-git-protocol-dep/pnpm-lock.yaml +++ b/pnpm11/__fixtures__/with-git-protocol-dep/pnpm-lock.yaml @@ -15,11 +15,10 @@ importers: packages: is-negative@https://codeload.github.com/kevva/is-negative/tar.gz/1d7e288222b53a0cab90a331f1865220ec29560c: - resolution: {tarball: https://codeload.github.com/kevva/is-negative/tar.gz/1d7e288222b53a0cab90a331f1865220ec29560c} + resolution: {gitHosted: true, tarball: https://codeload.github.com/kevva/is-negative/tar.gz/1d7e288222b53a0cab90a331f1865220ec29560c} version: 2.1.0 engines: {node: '>=0.10.0'} snapshots: - is-negative@https://codeload.github.com/kevva/is-negative/tar.gz/1d7e288222b53a0cab90a331f1865220ec29560c: - dev: false + is-negative@https://codeload.github.com/kevva/is-negative/tar.gz/1d7e288222b53a0cab90a331f1865220ec29560c: {} diff --git a/pnpm11/installing/deps-installer/test/install/autoInstallPeers.ts b/pnpm11/installing/deps-installer/test/install/autoInstallPeers.ts index add2697fd5..c2dc509664 100644 --- a/pnpm11/installing/deps-installer/test/install/autoInstallPeers.ts +++ b/pnpm11/installing/deps-installer/test/install/autoInstallPeers.ts @@ -758,3 +758,32 @@ test('a locked optional peer version is not rewritten when a sibling workspace p expect(optionalPeerVersion).not.toContain('(@pnpm.e2e/peer-c@1.0.0)') } }) + +test('a root dependency does not override the peers provided inside a self-contained subtree', async () => { + // Regression test for the closure-poisoning bug: @pnpm.e2e/closure-plugins + // provides every peer of its own subtree (closure-lib-a and closure-lib-b + // peer-depend on each other and on closure-peer-x, and all of them are + // regular dependencies of closure-plugins). The root project additionally + // depends on the incompatible closure-peer-x@2.0.0. With autoInstallPeers + // enabled, the peers resolved inside the subtree are also attached to the + // root project so other subtrees can reuse them — but they must not be + // peer-resolved again in the root context, where closure-peer-x@2.0.0 is + // the nearest provider, or the subtree's peer graph gets a mix of both + // versions. + const project = prepareEmpty() + await addDependenciesToPackage({}, [ + '@pnpm.e2e/closure-plugins@1.0.0', + '@pnpm.e2e/closure-peer-x@2.0.0', + ], testDefaults({ autoInstallPeers: true })) + const lockfile = project.readLockfile() + const pluginsSnapshot = lockfile.snapshots['@pnpm.e2e/closure-plugins@1.0.0'] + expect(pluginsSnapshot.dependencies?.['@pnpm.e2e/closure-peer-x']).toBe('1.0.0') + expect(pluginsSnapshot.dependencies?.['@pnpm.e2e/closure-lib-a']).not.toContain('closure-peer-x@2.0.0') + expect(pluginsSnapshot.dependencies?.['@pnpm.e2e/closure-lib-b']).not.toContain('closure-peer-x@2.0.0') + for (const depPath of Object.keys(lockfile.snapshots)) { + if (!depPath.startsWith('@pnpm.e2e/closure-lib')) continue + expect(depPath).not.toContain('closure-peer-x@2.0.0') + } + // The root keeps its own explicitly declared version. + expect(lockfile.importers['.'].dependencies?.['@pnpm.e2e/closure-peer-x']?.version).toBe('2.0.0') +}) diff --git a/pnpm11/installing/deps-resolver/src/index.ts b/pnpm11/installing/deps-resolver/src/index.ts index c78212b6c4..5cf5888235 100644 --- a/pnpm11/installing/deps-resolver/src/index.ts +++ b/pnpm11/installing/deps-resolver/src/index.ts @@ -77,6 +77,7 @@ interface ProjectToLink { binsDir: string declaredDirectDependencies: Set directNodeIdsByAlias: Map + hoistedPeerProviderNodeIds: Set explicitlyRequestedDirectDependencies: Set id: ProjectId linkedDependencies: LinkedDependency[] @@ -254,6 +255,7 @@ export async function resolveDependencies ( ...project.wantedDependencies.flatMap(({ alias, isNew }) => isNew && alias != null ? [alias] : []), ]), directNodeIdsByAlias: resolvedImporter.directNodeIdsByAlias, + hoistedPeerProviderNodeIds: resolvedImporter.hoistedPeerProviderNodeIds, explicitlyRequestedDirectDependencies: new Set( project.wantedDependencies.flatMap(({ alias, bareSpecifier, isNew, prevSpecifier, updateSpec }) => alias != null && (isNew === true || updateSpec === true || (prevSpecifier != null && bareSpecifier !== prevSpecifier)) diff --git a/pnpm11/installing/deps-resolver/src/resolveDependencies.ts b/pnpm11/installing/deps-resolver/src/resolveDependencies.ts index c5293238f8..1a8273d70e 100644 --- a/pnpm11/installing/deps-resolver/src/resolveDependencies.ts +++ b/pnpm11/installing/deps-resolver/src/resolveDependencies.ts @@ -281,6 +281,13 @@ export interface PkgAddress extends PkgAddressOrLinkBase { saveCatalogName?: string lockedPeerContext?: LockedPeerContext previousDepPath?: DepPath + /** + * A peer dependency provider attached to the root importer so that other + * subtrees can reuse it. Its `nodeId` keeps pointing at the provider's + * original position inside the dependency tree, so the node must be + * peer-resolved there — not in the root context. + */ + hoistedPeerProvider?: boolean } export type PkgAddressOrLink = PkgAddress | LinkedDependency @@ -431,7 +438,10 @@ export async function resolveRootDependencies ( // even those peers should be hoisted that are not autoinstalled for (const [resolvedPeerName, resolvedPeerAddress] of Object.entries(importerResolutionResult.resolvedPeers ?? {})) { if (!parentPkgAliases[resolvedPeerName]) { - importerResolutionResult.pkgAddresses.push(resolvedPeerAddress) + importerResolutionResult.pkgAddresses.push({ + ...resolvedPeerAddress, + hoistedPeerProvider: true, + }) } } } diff --git a/pnpm11/installing/deps-resolver/src/resolveDependencyTree.ts b/pnpm11/installing/deps-resolver/src/resolveDependencyTree.ts index ee0b291478..d78aeefe6b 100644 --- a/pnpm11/installing/deps-resolver/src/resolveDependencyTree.ts +++ b/pnpm11/installing/deps-resolver/src/resolveDependencyTree.ts @@ -47,6 +47,7 @@ export interface ResolvedImporters { [id: string]: { directDependencies: ResolvedDirectDependency[] directNodeIdsByAlias: Map + hoistedPeerProviderNodeIds: Set linkedDependencies: LinkedDependency[] } } @@ -357,6 +358,7 @@ export async function resolveDependencyTree ( } }), directNodeIdsByAlias: new Map(directNonLinkedDeps.map(({ alias, nodeId }) => [alias, nodeId])), + hoistedPeerProviderNodeIds: new Set(directNonLinkedDeps.filter((dep) => dep.hoistedPeerProvider).map(({ nodeId }) => nodeId)), linkedDependencies, } } diff --git a/pnpm11/installing/deps-resolver/src/resolvePeers.ts b/pnpm11/installing/deps-resolver/src/resolvePeers.ts index b015cee82e..ca3032f607 100644 --- a/pnpm11/installing/deps-resolver/src/resolvePeers.ts +++ b/pnpm11/installing/deps-resolver/src/resolvePeers.ts @@ -70,6 +70,8 @@ export interface GenericDependenciesGraphWithResolvedChildren + // See PkgAddress.hoistedPeerProvider in resolveDependencies.ts + hoistedPeerProviderNodeIds?: Set declaredDirectDependencies?: Set explicitlyRequestedDirectDependencies?: Set // only the top dependencies that were already installed @@ -121,7 +123,7 @@ export async function resolvePeers ( const finishingList: FinishingResolutionPromise[] = [] const peersCache = new Map() const purePkgs = new Set() - for (const { directNodeIdsByAlias, declaredDirectDependencies, explicitlyRequestedDirectDependencies, topParents, rootDir, id } of opts.projects) { + for (const { directNodeIdsByAlias, hoistedPeerProviderNodeIds, declaredDirectDependencies, explicitlyRequestedDirectDependencies, topParents, rootDir, id } of opts.projects) { const currentProviderSources: CurrentProviderSource[] = [{ directNodeIdsByAlias, declaredDirectDependencies: declaredDirectDependencies ?? new Set(), @@ -145,10 +147,25 @@ export async function resolvePeers ( } } - // eslint-disable-next-line no-await-in-loop - const { finishing } = await resolvePeersOfChildren(Object.fromEntries(directNodeIdsByAlias.entries()), pkgsByName, { + // Hoisted peer providers stay visible as providers (via pkgsByName) but are + // not traversed as direct children: their nodeIds point into subtrees, and + // resolving them a second time in the project's root context would bind + // their peers to the project's own dependencies instead of the providers + // next to them in the tree, racing with the in-place resolution on + // pathsByNodeId and producing peer graphs that mix both contexts. + const ownDirectChildren: Record = {} + const hoistedProviderChildren: Record = {} + for (const [alias, nodeId] of directNodeIdsByAlias.entries()) { + if (hoistedPeerProviderNodeIds?.has(nodeId)) { + hoistedProviderChildren[alias] = nodeId + } else { + ownDirectChildren[alias] = nodeId + } + } + const parentPkgsOfNode: ParentPkgsOfNode = new Map() + const projectPeersContext = { allPeerDepNames: opts.allPeerDepNames, - parentPkgsOfNode: new Map(), + parentPkgsOfNode, dependenciesTree: opts.dependenciesTree, depGraph, lockfileDir: opts.lockfileDir, @@ -168,10 +185,32 @@ export async function resolvePeers ( rootDir, virtualStoreDir: opts.virtualStoreDir, virtualStoreDirMaxLength: opts.virtualStoreDirMaxLength, - }) + } + // eslint-disable-next-line no-await-in-loop + const { finishing } = await resolvePeersOfChildren(ownDirectChildren, pkgsByName, projectPeersContext) if (finishing) { finishingList.push(finishing) } + // A provider whose tree position was pruned from the traversal (its parent + // hit peersCache, so its children were never visited) still has consumers + // awaiting its dep path, so resolve it here as a last resort. Providers + // visited by the traversal above are recorded in parentPkgsOfNode. + // All pruned providers go into a single resolvePeersOfChildren call: + // its cycle analysis only sees the children of one call, and providers + // frequently peer-depend on each other, so resolving them one by one + // would leave their dep path calculations awaiting each other forever. + const prunedProviderChildren: Record = {} + for (const [alias, nodeId] of Object.entries(hoistedProviderChildren)) { + if (parentPkgsOfNode.has(nodeId)) continue + prunedProviderChildren[alias] = nodeId + } + if (Object.keys(prunedProviderChildren).length > 0) { + // eslint-disable-next-line no-await-in-loop + const { finishing } = await resolvePeersOfChildren(prunedProviderChildren, pkgsByName, projectPeersContext) + if (finishing) { + finishingList.push(finishing) + } + } if (Object.keys(peerDependencyIssues.bad).length > 0 || Object.keys(peerDependencyIssues.missing).length > 0) { peerDependencyIssuesByProjects[id] = { ...peerDependencyIssues, diff --git a/pnpm11/installing/deps-resolver/test/resolvePeers.ts b/pnpm11/installing/deps-resolver/test/resolvePeers.ts index 154a3b77d8..9b15477f40 100644 --- a/pnpm11/installing/deps-resolver/test/resolvePeers.ts +++ b/pnpm11/installing/deps-resolver/test/resolvePeers.ts @@ -1717,3 +1717,147 @@ describe('dedupePeers', () => { expect(peerDependencyIssuesByProjects['']?.missing?.['real-pkg']).toBeUndefined() }) }) + +test('pruned hoisted peer providers that peer-depend on each other are resolved together', async () => { + // Hoisted peer providers whose tree position was never visited are resolved + // by a root-context fallback. Providers frequently peer-depend on each + // other, and each resolvePeersOfChildren call only detects peer cycles + // among its own children — so all pruned providers must be resolved in one + // call, or their dep path calculations await each other forever. + const libAPkg = { + name: 'lib-a', + pkgIdWithPatchHash: 'lib-a@1.0.0' as PkgIdWithPatchHash, + version: '1.0.0', + peerDependencies: { + 'lib-b': { version: '^1.0.0' }, + }, + id: '' as PkgResolutionId, + } + const libBPkg = { + name: 'lib-b', + pkgIdWithPatchHash: 'lib-b@1.0.0' as PkgIdWithPatchHash, + version: '1.0.0', + peerDependencies: { + 'lib-a': { version: '^1.0.0' }, + }, + id: '' as PkgResolutionId, + } + const consumerPkg = { + name: 'consumer', + pkgIdWithPatchHash: 'consumer@1.0.0' as PkgIdWithPatchHash, + version: '1.0.0', + peerDependencies: { + 'lib-a': { version: '^1.0.0' }, + 'lib-b': { version: '^1.0.0' }, + }, + id: '' as PkgResolutionId, + } + const { dependenciesGraph, dependenciesByProjectId } = await resolvePeers({ + allPeerDepNames: new Set(['lib-a', 'lib-b']), + projects: [ + { + directNodeIdsByAlias: new Map([ + ['consumer', '>consumer@1.0.0>' as NodeId], + ['lib-a', '>lib-a@1.0.0>' as NodeId], + ['lib-b', '>lib-b@1.0.0>' as NodeId], + ]), + hoistedPeerProviderNodeIds: new Set(['>lib-a@1.0.0>' as NodeId, '>lib-b@1.0.0>' as NodeId]), + topParents: [], + rootDir: '' as ProjectRootDir, + id: '.', + }, + ], + resolvedImporters: {}, + dependenciesTree: new Map>([ + ['>consumer@1.0.0>' as NodeId, { + children: {}, + installable: true, + resolvedPackage: consumerPkg, + depth: 0, + }], + ['>lib-a@1.0.0>' as NodeId, { + children: {}, + installable: true, + resolvedPackage: libAPkg, + depth: 1, + }], + ['>lib-b@1.0.0>' as NodeId, { + children: {}, + installable: true, + resolvedPackage: libBPkg, + depth: 1, + }], + ]), + virtualStoreDir: '', + lockfileDir: '', + virtualStoreDirMaxLength: 120, + peersSuffixMaxLength: 1000, + workspaceProjectIds: new Set(), + }) + expect(Object.keys(dependenciesGraph).sort()).toStrictEqual([ + 'consumer@1.0.0(lib-a@1.0.0)(lib-b@1.0.0)', + 'lib-a@1.0.0(lib-b@1.0.0)', + 'lib-b@1.0.0(lib-a@1.0.0)', + ]) + expect(dependenciesByProjectId['.'].get('lib-a')).toBe('lib-a@1.0.0(lib-b@1.0.0)') + expect(dependenciesByProjectId['.'].get('lib-b')).toBe('lib-b@1.0.0(lib-a@1.0.0)') +}) + +test('a pruned hoisted peer provider is resolved by the root-context fallback', async () => { + // Mirror of the pacquet test `pruned_hoisted_provider_falls_back_to_root_resolution`: + // a hoisted peer provider whose tree position was never visited must still + // get a dep path so the consumers that bound it can finish. + const provPkg = { + name: 'prov', + pkgIdWithPatchHash: 'prov@1.0.0' as PkgIdWithPatchHash, + version: '1.0.0', + peerDependencies: {} as PeerDependencies, + id: '' as PkgResolutionId, + } + const consumerPkg = { + name: 'consumer', + pkgIdWithPatchHash: 'consumer@1.0.0' as PkgIdWithPatchHash, + version: '1.0.0', + peerDependencies: { + prov: { version: '*' }, + }, + id: '' as PkgResolutionId, + } + const { dependenciesGraph, dependenciesByProjectId } = await resolvePeers({ + allPeerDepNames: new Set(['prov']), + projects: [ + { + directNodeIdsByAlias: new Map([ + ['consumer', '>consumer@1.0.0>' as NodeId], + ['prov', '>prov@1.0.0>' as NodeId], + ]), + hoistedPeerProviderNodeIds: new Set(['>prov@1.0.0>' as NodeId]), + topParents: [], + rootDir: '' as ProjectRootDir, + id: '.', + }, + ], + resolvedImporters: {}, + dependenciesTree: new Map>([ + ['>consumer@1.0.0>' as NodeId, { + children: {}, + installable: true, + resolvedPackage: consumerPkg, + depth: 0, + }], + ['>prov@1.0.0>' as NodeId, { + children: {}, + installable: true, + resolvedPackage: provPkg, + depth: 1, + }], + ]), + virtualStoreDir: '', + lockfileDir: '', + virtualStoreDirMaxLength: 120, + peersSuffixMaxLength: 1000, + workspaceProjectIds: new Set(), + }) + expect(dependenciesByProjectId['.'].get('prov')).toBe('prov@1.0.0') + expect(Object.keys(dependenciesGraph)).toContain('consumer@1.0.0(prov@1.0.0)') +}) diff --git a/pnpr/.fixtures/packages/@pnpm.e2e/closure-lib-a/1.0.0/package.json b/pnpr/.fixtures/packages/@pnpm.e2e/closure-lib-a/1.0.0/package.json new file mode 100644 index 0000000000..605634df33 --- /dev/null +++ b/pnpr/.fixtures/packages/@pnpm.e2e/closure-lib-a/1.0.0/package.json @@ -0,0 +1,8 @@ +{ + "name": "@pnpm.e2e/closure-lib-a", + "version": "1.0.0", + "peerDependencies": { + "@pnpm.e2e/closure-lib-b": "^1.0.0", + "@pnpm.e2e/closure-peer-x": "^1.0.0" + } +} diff --git a/pnpr/.fixtures/packages/@pnpm.e2e/closure-lib-b/1.0.0/package.json b/pnpr/.fixtures/packages/@pnpm.e2e/closure-lib-b/1.0.0/package.json new file mode 100644 index 0000000000..4c777eb10e --- /dev/null +++ b/pnpr/.fixtures/packages/@pnpm.e2e/closure-lib-b/1.0.0/package.json @@ -0,0 +1,8 @@ +{ + "name": "@pnpm.e2e/closure-lib-b", + "version": "1.0.0", + "peerDependencies": { + "@pnpm.e2e/closure-lib-a": "^1.0.0", + "@pnpm.e2e/closure-peer-x": "^1.0.0" + } +} diff --git a/pnpr/.fixtures/packages/@pnpm.e2e/closure-peer-x/1.0.0/package.json b/pnpr/.fixtures/packages/@pnpm.e2e/closure-peer-x/1.0.0/package.json new file mode 100644 index 0000000000..da833c9c6b --- /dev/null +++ b/pnpr/.fixtures/packages/@pnpm.e2e/closure-peer-x/1.0.0/package.json @@ -0,0 +1,4 @@ +{ + "name": "@pnpm.e2e/closure-peer-x", + "version": "1.0.0" +} diff --git a/pnpr/.fixtures/packages/@pnpm.e2e/closure-peer-x/2.0.0/package.json b/pnpr/.fixtures/packages/@pnpm.e2e/closure-peer-x/2.0.0/package.json new file mode 100644 index 0000000000..85535e2d1c --- /dev/null +++ b/pnpr/.fixtures/packages/@pnpm.e2e/closure-peer-x/2.0.0/package.json @@ -0,0 +1,4 @@ +{ + "name": "@pnpm.e2e/closure-peer-x", + "version": "2.0.0" +} diff --git a/pnpr/.fixtures/packages/@pnpm.e2e/closure-plugins/1.0.0/package.json b/pnpr/.fixtures/packages/@pnpm.e2e/closure-plugins/1.0.0/package.json new file mode 100644 index 0000000000..addcf13e29 --- /dev/null +++ b/pnpr/.fixtures/packages/@pnpm.e2e/closure-plugins/1.0.0/package.json @@ -0,0 +1,9 @@ +{ + "name": "@pnpm.e2e/closure-plugins", + "version": "1.0.0", + "dependencies": { + "@pnpm.e2e/closure-lib-a": "1.0.0", + "@pnpm.e2e/closure-lib-b": "1.0.0", + "@pnpm.e2e/closure-peer-x": "1.0.0" + } +}