From 058f5f2f8b63dedfb970af85d63efe07fdef6ca8 Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Mon, 25 May 2026 01:35:23 +0200 Subject: [PATCH] fix(package-manager): port pnpm's lockfile-pruner BFS to re-derive transitive optional (#11919) The resolver's per-node AND-fold updates only the directly-revisited package, so descendants walked first via an `optionalDependencies` edge stay stuck at `optional: true` even when a later non-optional path reaches them transitively. Upstream hides this from users by re-deriving the flag in `copyDependencySubGraph`; pacquet had no equivalent pass, so a fetch or build failure on a transitively-required package was silently tolerated as if it were optional. Port the BFS into `dependencies_graph_to_lockfile`: walk from the importer's direct deps (classified by manifest dep-group), recurse through each node's children with parent-inherited optional for regular edges and forced `optional: true` for `optionalDependencies` edges, then override `SnapshotEntry.optional` to `false` for any package reached by an all-non-optional path. Refs https://github.com/pnpm/pnpm/issues/11916. --- .../src/dependencies_graph_to_lockfile.rs | 125 ++++++++++- .../dependencies_graph_to_lockfile/tests.rs | 200 ++++++++++++++++++ 2 files changed, 315 insertions(+), 10 deletions(-) diff --git a/pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs b/pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs index 28414bfab6..884142d256 100644 --- a/pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs +++ b/pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs @@ -73,7 +73,9 @@ pub fn dependencies_graph_to_lockfile(opts: GraphToLockfileOptions<'_>) -> Lockf let importer = build_root_importer(manifest, resolved); - let (packages, snapshots) = build_packages_and_snapshots(&resolved.peers_result.graph); + let optional_overrides = compute_corrected_optional(manifest, resolved); + let (packages, snapshots) = + build_packages_and_snapshots(&resolved.peers_result.graph, &optional_overrides); let mut importers: HashMap = HashMap::with_capacity(1); importers.insert(Lockfile::ROOT_IMPORTER_KEY.to_string(), importer); @@ -243,8 +245,13 @@ fn real_name(result: &ResolveResult) -> Option { /// /// Multiple snapshot entries (peer variants) share one packages entry, /// so the loop dedupes by peer-stripped key. +/// +/// `optional_overrides` carries the corrected `optional` flag per +/// depPath produced by [`compute_corrected_optional`]; a missing +/// entry falls back to [`DependenciesGraphNode::optional`]. fn build_packages_and_snapshots( graph: &DependenciesGraph, + optional_overrides: &HashMap, ) -> (HashMap, HashMap) { let mut packages: HashMap = HashMap::new(); let mut snapshots: HashMap = HashMap::new(); @@ -253,7 +260,7 @@ fn build_packages_and_snapshots( let Ok(snapshot_key) = node.dep_path.as_str().parse::() else { continue }; let metadata_key = snapshot_key.without_peer(); - let snapshot = build_snapshot_entry(node, graph); + let snapshot = build_snapshot_entry(node, graph, optional_overrides); snapshots.insert(snapshot_key, snapshot); packages.entry(metadata_key).or_insert_with(|| build_package_metadata(node)); @@ -372,14 +379,19 @@ fn build_peer_dep_blocks(node: &DependenciesGraphNode) -> PeerDepBlocks { /// [`toLockfileDependency`](https://github.com/pnpm/pnpm/blob/097983fbca/installing/deps-resolver/src/updateLockfile.ts#L49-L156): /// the `dependencies` / `optionalDependencies` partition follows the /// node's own `optionalDependencies` set and peer-optional flag; -/// `transitivePeerDependencies` is sorted; `optional` is copied from -/// the resolver's [`DependenciesGraphNode::optional`] (AND-folded -/// across every visit so a snapshot is marked `optional: true` only -/// when every path from any importer to it goes through an -/// `optionalDependencies` edge). `BuildModules` consults this flag to -/// decide whether a build failure is fatal or should be reported via +/// `transitivePeerDependencies` is sorted; `optional` is sourced from +/// [`compute_corrected_optional`] (the lockfile-pruner BFS port, +/// which re-derives the flag from the importer graph because the +/// resolver's per-node fold misses transitive descendants on +/// revisits — see ). +/// `BuildModules` consults this flag to decide whether a build +/// failure is fatal or should be reported via /// `pnpm:skipped-optional-dependency`. -fn build_snapshot_entry(node: &DependenciesGraphNode, graph: &DependenciesGraph) -> SnapshotEntry { +fn build_snapshot_entry( + node: &DependenciesGraphNode, + graph: &DependenciesGraph, + optional_overrides: &HashMap, +) -> SnapshotEntry { let optional_children = optional_children_of(node); let mut dependencies: HashMap = HashMap::new(); @@ -400,13 +412,106 @@ fn build_snapshot_entry(node: &DependenciesGraphNode, graph: &DependenciesGraph) list }; + let optional = optional_overrides.get(&node.dep_path).copied().unwrap_or(node.optional); + SnapshotEntry { id: None, dependencies: (!dependencies.is_empty()).then_some(dependencies), optional_dependencies: (!optional_dependencies.is_empty()).then_some(optional_dependencies), transitive_peer_dependencies: (!transitive.is_empty()).then_some(transitive), patched: None, - optional: node.optional, + optional, + } +} + +/// Re-derive each snapshot's `optional` flag by walking the graph +/// from the importer's direct deps, classifying each starting edge by +/// the dep-group it lives in on the manifest. A package ends up +/// `optional: false` iff every visit didn't land on it through an +/// `optionalDependencies` edge — i.e. there exists at least one path +/// from any importer to it whose edges are all non-optional. +/// +/// Ports upstream pnpm's +/// [`copyDependencySubGraph`](https://github.com/pnpm/pnpm/blob/b9de85dcb6/lockfile/pruner/src/index.ts#L160-L205) +/// BFS, which exists for exactly this reason: the resolver's +/// per-node AND-fold at +/// [`resolveDependencies.ts:1630`](https://github.com/pnpm/pnpm/blob/b9de85dcb6/installing/deps-resolver/src/resolveDependencies.ts#L1627-L1648) +/// updates only the directly-revisited package, so the +/// already-walked descendants stay stuck at whatever `optional` they +/// were tagged with on the first visit. See +/// for the scenario. +/// +/// A missing entry in the returned map means the node was never +/// reachable from any importer dep — [`build_snapshot_entry`] falls +/// back to [`DependenciesGraphNode::optional`] for those, matching +/// upstream's "untouched depLockfile keeps its existing flag" arm. +fn compute_corrected_optional( + manifest: &PackageManifest, + resolved: &ResolveImporterResult, +) -> HashMap { + let graph = &resolved.peers_result.graph; + let direct = &resolved.peers_result.direct_dependencies_by_alias; + let alias_to_group = manifest_alias_to_group(manifest); + + // Partition importer deps by group, mirroring the + // `(devDepPaths, optionalDepPaths, prodDepPaths)` split upstream + // hands to `copyDependencySubGraph`. + let mut dev_seeds: Vec<&DepPath> = Vec::new(); + let mut optional_seeds: Vec<&DepPath> = Vec::new(); + let mut prod_seeds: Vec<&DepPath> = Vec::new(); + for (alias, dep_path) in direct { + let group = alias_to_group.get(alias).copied().unwrap_or(DependencyGroup::Prod); + match group { + DependencyGroup::Dev => dev_seeds.push(dep_path), + DependencyGroup::Optional => optional_seeds.push(dep_path), + DependencyGroup::Prod | DependencyGroup::Peer => prod_seeds.push(dep_path), + } + } + + let mut walked: HashSet<(&DepPath, bool)> = HashSet::new(); + let mut visited: HashSet<&DepPath> = HashSet::new(); + let mut non_optional: HashSet<&DepPath> = HashSet::new(); + + walk_subgraph(graph, &mut walked, &mut visited, &mut non_optional, dev_seeds, false); + walk_subgraph(graph, &mut walked, &mut visited, &mut non_optional, optional_seeds, true); + walk_subgraph(graph, &mut walked, &mut visited, &mut non_optional, prod_seeds, false); + + let mut out: HashMap = HashMap::with_capacity(visited.len()); + for dep_path in visited { + out.insert(dep_path.clone(), !non_optional.contains(dep_path)); + } + out +} + +/// Iterative half of [`compute_corrected_optional`]. Pushes +/// `(dep_path, optional)` pairs onto an explicit stack rather than +/// recursing so deep graphs can't blow the call stack. Children +/// declared by the parent's `optionalDependencies` (or by a +/// peer-deps-meta `optional: true`) always recurse with +/// `optional: true`; the rest inherit the parent's `optional`. +fn walk_subgraph<'g>( + graph: &'g DependenciesGraph, + walked: &mut HashSet<(&'g DepPath, bool)>, + visited: &mut HashSet<&'g DepPath>, + non_optional: &mut HashSet<&'g DepPath>, + seeds: Vec<&'g DepPath>, + optional: bool, +) { + let mut stack: Vec<(&'g DepPath, bool)> = seeds.into_iter().map(|dp| (dp, optional)).collect(); + while let Some((dep_path, optional)) = stack.pop() { + if !walked.insert((dep_path, optional)) { + continue; + } + let Some(node) = graph.get(dep_path) else { continue }; + visited.insert(dep_path); + if !optional { + non_optional.insert(dep_path); + } + let opt_children = optional_children_of(node); + for (alias, child_dep_path) in &node.children { + let child_optional = optional || opt_children.contains(alias.as_str()); + stack.push((child_dep_path, child_optional)); + } } } diff --git a/pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rs b/pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rs index 9301e94fc8..7c9e35abe8 100644 --- a/pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rs +++ b/pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rs @@ -523,3 +523,203 @@ fn snapshot_optional_flag_round_trips_from_dependencies_graph_node() { "snapshot marked optional in the graph propagates to the lockfile", ); } + +/// Scenario from +/// [pnpm/pnpm#11916](https://github.com/pnpm/pnpm/issues/11916): root +/// declares `optionalDependencies.a` and `dependencies.b`; `a.dependencies = {c}` +/// and `b.dependencies = {a}`. The resolver's [`DependenciesGraphNode::optional`] +/// field is stale on `c` — the tree walker marks it `optional: true` +/// on the `optional → a → c` descent and then misses the AND-fold on +/// the `prod → b → a` revisit because the lazy-children path doesn't +/// re-traverse `a`'s subtree. The lockfile-pruner BFS re-derives the +/// flag from the importer-rooted graph, so `c` ends up `optional: +/// false` (reachable through the all-non-optional path `prod → b → +/// a → c`). +#[test] +fn transitive_optional_is_recomputed_for_packages_reachable_via_a_non_optional_path() { + let (_tmp, manifest) = write_manifest(json!({ + "name": "fixture", + "version": "1.0.0", + "dependencies": { "b": "^1.0.0" }, + "optionalDependencies": { "a": "^1.0.0" }, + })); + + // `c` was first reached via the optional path, so the resolver + // left `node.optional = true`. The pruner should flip it back to + // false. + let c = make_node_with_optional( + "c", + "1.0.0", + json!({ "name": "c", "version": "1.0.0" }), + BTreeMap::new(), + BTreeMap::new(), + HashSet::new(), + true, + ); + + // `a` was revisited from `b` so the resolver AND-folded its flag + // to false — that part already works. The bug is purely in the + // descendants. + let mut a_children = BTreeMap::new(); + a_children.insert("c".to_string(), DepPath::from("c@1.0.0".to_string())); + let a = make_node_with_optional( + "a", + "1.0.0", + json!({ "name": "a", "version": "1.0.0", "dependencies": { "c": "^1.0.0" } }), + a_children, + BTreeMap::new(), + HashSet::new(), + false, + ); + + let mut b_children = BTreeMap::new(); + b_children.insert("a".to_string(), DepPath::from("a@1.0.0".to_string())); + let b = make_node_with_optional( + "b", + "1.0.0", + json!({ "name": "b", "version": "1.0.0", "dependencies": { "a": "^1.0.0" } }), + b_children, + BTreeMap::new(), + HashSet::new(), + false, + ); + + let mut graph = DependenciesGraph::new(); + graph.insert(a.dep_path.clone(), a); + graph.insert(b.dep_path.clone(), b); + graph.insert(c.dep_path.clone(), c); + + let mut direct = BTreeMap::new(); + direct.insert("a".to_string(), DepPath::from("a@1.0.0".to_string())); + direct.insert("b".to_string(), DepPath::from("b@1.0.0".to_string())); + + let resolved = ResolveImporterResult { + resolved_tree: ResolvedTree::default(), + peers_result: ResolvePeersResult { + graph, + direct_dependencies_by_alias: direct, + peer_dependency_issues: PeerDependencyIssues::default(), + }, + }; + + let lockfile = dependencies_graph_to_lockfile(GraphToLockfileOptions { + manifest: &manifest, + resolved: &resolved, + auto_install_peers: false, + exclude_links_from_lockfile: false, + overrides: None, + ignored_optional_dependencies: None, + }); + + let snapshots = lockfile.snapshots.as_ref().expect("snapshots map"); + let a_key: PackageKey = "a@1.0.0".parse().unwrap(); + let b_key: PackageKey = "b@1.0.0".parse().unwrap(); + let c_key: PackageKey = "c@1.0.0".parse().unwrap(); + assert!(!snapshots[&b_key].optional, "b is a direct prod dep"); + assert!(!snapshots[&a_key].optional, "a is reachable via prod → b → a"); + assert!(!snapshots[&c_key].optional, "c is reachable via prod → b → a → c"); +} + +/// Ported from upstream pnpm's +/// [`'subdependency is both optional and dev'`](https://github.com/pnpm/pnpm/blob/b9de85dcb6/lockfile/pruner/test/index.ts#L378-L449) +/// pruner test: when one shared subdep is reached via a dev parent's +/// `optionalDependencies` and a prod parent's `dependencies`, only +/// the strictly-optional subdep ends up `optional: true`. +#[test] +fn shared_subdep_reached_through_dev_optional_and_prod_paths_is_marked_non_optional() { + let (_tmp, manifest) = write_manifest(json!({ + "name": "fixture", + "version": "1.0.0", + "dependencies": { "prod-parent": "^1.0.0" }, + "devDependencies": { "parent": "^1.0.0" }, + })); + + let subdep = make_node_with_optional( + "subdep", + "1.0.0", + json!({ "name": "subdep", "version": "1.0.0" }), + BTreeMap::new(), + BTreeMap::new(), + HashSet::new(), + true, + ); + let subdep2 = make_node_with_optional( + "subdep2", + "1.0.0", + json!({ "name": "subdep2", "version": "1.0.0" }), + BTreeMap::new(), + BTreeMap::new(), + HashSet::new(), + true, + ); + + let mut parent_children = BTreeMap::new(); + parent_children.insert("subdep".to_string(), DepPath::from("subdep@1.0.0".to_string())); + parent_children.insert("subdep2".to_string(), DepPath::from("subdep2@1.0.0".to_string())); + let parent = make_node_with_optional( + "parent", + "1.0.0", + json!({ + "name": "parent", + "version": "1.0.0", + "optionalDependencies": { "subdep": "^1.0.0", "subdep2": "^1.0.0" }, + }), + parent_children, + BTreeMap::new(), + HashSet::new(), + false, + ); + + let mut prod_children = BTreeMap::new(); + prod_children.insert("subdep2".to_string(), DepPath::from("subdep2@1.0.0".to_string())); + let prod_parent = make_node_with_optional( + "prod-parent", + "1.0.0", + json!({ + "name": "prod-parent", + "version": "1.0.0", + "dependencies": { "subdep2": "^1.0.0" }, + }), + prod_children, + BTreeMap::new(), + HashSet::new(), + false, + ); + + let mut graph = DependenciesGraph::new(); + graph.insert(parent.dep_path.clone(), parent); + graph.insert(prod_parent.dep_path.clone(), prod_parent); + graph.insert(subdep.dep_path.clone(), subdep); + graph.insert(subdep2.dep_path.clone(), subdep2); + + let mut direct = BTreeMap::new(); + direct.insert("parent".to_string(), DepPath::from("parent@1.0.0".to_string())); + direct.insert("prod-parent".to_string(), DepPath::from("prod-parent@1.0.0".to_string())); + + let resolved = ResolveImporterResult { + resolved_tree: ResolvedTree::default(), + peers_result: ResolvePeersResult { + graph, + direct_dependencies_by_alias: direct, + peer_dependency_issues: PeerDependencyIssues::default(), + }, + }; + + let lockfile = dependencies_graph_to_lockfile(GraphToLockfileOptions { + manifest: &manifest, + resolved: &resolved, + auto_install_peers: false, + exclude_links_from_lockfile: false, + overrides: None, + ignored_optional_dependencies: None, + }); + + let snapshots = lockfile.snapshots.as_ref().unwrap(); + let subdep_key: PackageKey = "subdep@1.0.0".parse().unwrap(); + let subdep2_key: PackageKey = "subdep2@1.0.0".parse().unwrap(); + assert!(snapshots[&subdep_key].optional, "subdep only reachable via dev → optional path"); + assert!( + !snapshots[&subdep2_key].optional, + "subdep2 is reachable via prod-parent → subdep2 (all non-optional)", + ); +}