install: fail a download that a non-optional peer needs, whichever dependency owns the slot

src/install/hoisted_install.rs

@@ -76,7 +76,7 @@ pub(crate) fn install_hoisted_packages(
         .hoisted_dependencies
         .clone_from(&original_tree_dep_ids);
 
-    {
+    let tree_required_packages = {
         // `lockfile` is `Box<Lockfile>` so the heap object
         // is disjoint from the `PackageManager` struct; snapshot raw `*mut
         // Lockfile` and `*mut Log` first so `filter` can hold `&mut Lockfile`
@@ -95,9 +95,9 @@ pub(crate) fn install_hoisted_packages(
                 install_root_dependencies,
                 workspace_filters,
                 packages_to_install,
-            )?;
+            )?
         }
-    }
+    };
     // Re-derive after `filter()` so every subsequent `this` use (progress
     // setup through the install loop) is a fresh child of `mgr_ptr` under
     // Stacked Borrows — `&mut *mgr_ptr` inside the block above popped the
@@ -384,7 +384,8 @@ pub(crate) fn install_hoisted_packages(
                 summary: &mut summary,
                 force_install,
                 successfully_installed: Bitset::init_empty(pkg_len)?,
-                required_packages: tree::RequiredPackages::new(
+                required_packages: tree::RequiredPackages::from_tree(
+                    tree_required_packages,
                     workspace_filters,
                     install_root_dependencies,
                     packages_to_install,

src/install/lockfile.rs

@@ -1375,29 +1375,34 @@ impl<'a> Cloner<'a> {
 impl Lockfile {
     /// Re-hoists while a pass bound an optional peer late; a reload has that binding up front.
     pub(crate) fn resolve(&mut self, log: &mut bun_ast::Log) -> Result<(), tree::SubtreeError> {
-        while self.hoist::<{ tree::BuilderMethod::Resolvable }>(log, None, true, &[], None)? {}
+        while self
+            .hoist::<{ tree::BuilderMethod::Resolvable }>(log, None, true, &[], None)?
+            .late_bound_optional_peer
+        {}
         Ok(())
     }
 
+    /// Returns the packages that the install tree binds a non-optional dependency to.
     pub(crate) fn filter(
         &mut self,
         log: &mut bun_ast::Log,
         manager: &mut PackageManager,
         install_root_dependencies: bool,
         workspace_filters: &[WorkspaceFilter],
         packages_to_install: Option<&[PackageID]>,
-    ) -> Result<(), tree::SubtreeError> {
-        self.hoist::<{ tree::BuilderMethod::Filter }>(
-            log,
-            Some(manager),
-            install_root_dependencies,
-            workspace_filters,
-            packages_to_install,
-        )?;
-        Ok(())
-    }
-
-    /// Sets `buffers.trees`/`hoisted_dependencies`; returns `Builder::late_bound_optional_peer`.
+    ) -> Result<DynamicBitSet, tree::SubtreeError> {
+        Ok(self
+            .hoist::<{ tree::BuilderMethod::Filter }>(
+                log,
+                Some(manager),
+                install_root_dependencies,
+                workspace_filters,
+                packages_to_install,
+            )?
+            .required_packages)
+    }
+
+    /// Sets `buffers.trees`/`hoisted_dependencies`.
     pub(crate) fn hoist<const METHOD: tree::BuilderMethod>(
         &mut self,
         log: &mut bun_ast::Log,
@@ -1407,7 +1412,7 @@ impl Lockfile {
         install_root_dependencies: bool,
         workspace_filters: &[WorkspaceFilter],
         packages_to_install: Option<&[PackageID]>,
-    ) -> Result<bool, tree::SubtreeError> {
+    ) -> Result<HoistResult, tree::SubtreeError> {
         let slice = self.packages.slice();
 
         // Only the install applies the barrier, so the saved tree does not depend on it.
@@ -1440,6 +1445,11 @@ impl Lockfile {
             late_bound_optional_peer: false,
             list: Default::default(),
             sort_buf: Default::default(),
+            required_packages: if METHOD == tree::BuilderMethod::Filter {
+                DynamicBitSet::init_empty(slice.len())?
+            } else {
+                DynamicBitSet::default()
+            },
         };
 
         Tree::default().process_subtree(tree::ROOT_DEP_ID, tree::INVALID_ID, &mut builder)?;
@@ -1459,10 +1469,19 @@ impl Lockfile {
         let late_bound_optional_peer = builder.late_bound_optional_peer;
         self.buffers.trees = cleaned.trees;
         self.buffers.hoisted_dependencies = cleaned.dep_ids;
-        Ok(late_bound_optional_peer)
+        Ok(HoistResult {
+            late_bound_optional_peer,
+            required_packages: cleaned.required_packages,
+        })
     }
 }
 
+pub(crate) struct HoistResult {
+    pub(crate) late_bound_optional_peer: bool,
+    /// `Builder::required_packages`. Empty unless `METHOD` is `Filter`.
+    pub(crate) required_packages: DynamicBitSet,
+}
+
 #[derive(Clone, Copy)]
 pub struct PendingResolution {
     pub(crate) old_resolution: PackageID,

src/install/lockfile/Tree.rs

@@ -123,7 +123,8 @@ impl Tree {
 
 enum HoistDependencyResult {
     DependencyLoop,
-    Hoisted,
+    /// Deduplicated onto a placed dependency on this package.
+    Hoisted(PackageID),
     Resolve(PackageID),
     ResolveReplace(ResolveReplace),
     ResolveLater,
@@ -443,6 +444,8 @@ pub struct Builder<'a, const METHOD: BuilderMethod> {
     pub(crate) packages_to_install: Option<&'a [PackageID]>,
     /// Workspace package ids that are hoisting barriers (self-contained node_modules).
     pub(crate) self_contained: Vec<PackageID>,
+    /// The packages that a dependency without `Behavior::OPTIONAL` is bound to. Only `Filter`.
+    pub(crate) required_packages: DynamicBitSet,
 }
 
 pub struct BuilderEntry {
@@ -460,6 +463,7 @@ bun_collections::multi_array_columns! {
 pub(crate) struct CleanResult {
     pub trees: Vec<Tree>,
     pub dep_ids: Vec<DependencyID>,
+    pub required_packages: DynamicBitSet,
 }
 
 impl<'a, const METHOD: BuilderMethod> Builder<'a, METHOD> {
@@ -487,6 +491,32 @@ impl<'a, const METHOD: BuilderMethod> Builder<'a, METHOD> {
         self.lockfile().buffers.string_bytes.as_slice()
     }
 
+    /// `dependency`, one of `parent_range`, is linked, bound to `pkg_id`.
+    fn mark_required(
+        &mut self,
+        dependency: &Dependency,
+        parent_range: DependencyIDSlice,
+        pkg_id: PackageID,
+    ) {
+        if METHOD != BuilderMethod::Filter
+            || dependency
+                .behavior
+                .contains(crate::dependency::Behavior::OPTIONAL)
+            || (pkg_id as usize) >= self.required_packages.bit_length()
+        {
+            return;
+        }
+        // A peer whose parent also lists the name in `optionalDependencies` is an optional peer.
+        if dependency.behavior.is_peer()
+            && self.dependencies[parent_range.begin() as usize..parent_range.end() as usize]
+                .iter()
+                .any(|dep| dep.name_hash == dependency.name_hash && dep.behavior.is_optional())
+        {
+            return;
+        }
+        self.required_packages.set(pkg_id as usize);
+    }
+
     /// Flatten the multi-dimensional ArrayList of package IDs into a single easily serializable array
     pub(crate) fn clean(&mut self) -> Result<CleanResult, AllocError> {
         let mut total: u32 = 0;
@@ -529,7 +559,11 @@ impl<'a, const METHOD: BuilderMethod> Builder<'a, METHOD> {
 
         slice.deinit_owned();
 
-        Ok(CleanResult { trees, dep_ids })
+        Ok(CleanResult {
+            trees,
+            dep_ids,
+            required_packages: core::mem::take(&mut self.required_packages),
+        })
     }
 }
 
@@ -620,6 +654,8 @@ pub(crate) struct RequiredPackages<'a> {
     workspace_filters: &'a [WorkspaceFilter],
     install_root_dependencies: bool,
     packages_to_install: Option<&'a [PackageID]>,
+    /// `Builder::required_packages` of the hoisted install tree. Peers are bound there.
+    tree: DynamicBitSet,
     /// Walked on the first question that the asking dependency cannot answer.
     packages: Option<DynamicBitSet>,
 }
@@ -629,11 +665,27 @@ impl<'a> RequiredPackages<'a> {
         workspace_filters: &'a [WorkspaceFilter],
         install_root_dependencies: bool,
         packages_to_install: Option<&'a [PackageID]>,
+    ) -> Self {
+        Self::from_tree(
+            DynamicBitSet::default(),
+            workspace_filters,
+            install_root_dependencies,
+            packages_to_install,
+        )
+    }
+
+    /// `tree` is what `Lockfile::filter` returns.
+    pub(crate) fn from_tree(
+        tree: DynamicBitSet,
+        workspace_filters: &'a [WorkspaceFilter],
+        install_root_dependencies: bool,
+        packages_to_install: Option<&'a [PackageID]>,
     ) -> Self {
         Self {
             workspace_filters,
             install_root_dependencies,
             packages_to_install,
+            tree,
             packages: None,
         }
     }
@@ -652,6 +704,9 @@ impl<'a> RequiredPackages<'a> {
         {
             return true;
         }
+        if (package_id as usize) < self.tree.bit_length() {
+            return self.tree.is_set(package_id as usize);
+        }
         // Walk again if the install appended a package since.
         let packages = match &mut self.packages {
             Some(packages) if packages.bit_length() == lockfile.packages.len() => packages,
@@ -894,20 +949,18 @@ impl Tree {
                 if pkg_resolutions[pkg_id as usize].tag == crate::resolution::Tag::Folder {
                     // A peer an ancestor edge already provides dedupes instead of nesting
                     // a second copy of the folder (#40561).
-                    if dependency.behavior.is_peer()
-                        && matches!(
-                            Tree::hoist_dependency::<true, METHOD>(
-                                next_id,
-                                hoist_root_id,
-                                pkg_id,
-                                dep_id,
-                                resolution_list,
-                                builder,
-                            ),
-                            HoistDependencyResult::Hoisted
-                        )
-                    {
-                        break 'hoisted HoistDependencyResult::Hoisted;
+                    if dependency.behavior.is_peer() {
+                        let hoisted = Tree::hoist_dependency::<true, METHOD>(
+                            next_id,
+                            hoist_root_id,
+                            pkg_id,
+                            dep_id,
+                            resolution_list,
+                            builder,
+                        );
+                        if matches!(hoisted, HoistDependencyResult::Hoisted(_)) {
+                            break 'hoisted hoisted;
+                        }
                     }
 
                     // Folder packages never hoist, so a cycle between them would nest forever.
@@ -941,7 +994,11 @@ impl Tree {
             };
 
             match hoisted {
-                HoistDependencyResult::DependencyLoop | HoistDependencyResult::Hoisted => continue,
+                HoistDependencyResult::DependencyLoop => continue,
+                HoistDependencyResult::Hoisted(bound_pkg_id) => {
+                    builder.mark_required(dependency, resolution_list, bound_pkg_id);
+                    continue;
+                }
 
                 HoistDependencyResult::Resolve(res_id) => {
                     debug_assert!(pkg_id == invalid_package_id);
@@ -972,6 +1029,7 @@ impl Tree {
                 }
                 HoistDependencyResult::ResolveReplace(replace) => {
                     debug_assert!(pkg_id != invalid_package_id);
+                    builder.mark_required(dependency, resolution_list, pkg_id);
                     builder.late_bound_optional_peer = true;
                     builder.resolutions[replace.dep_id as usize] = pkg_id;
                     if let Some(entry) = builder
@@ -1026,6 +1084,7 @@ impl Tree {
                     entry.value_ptr.put(dep_id, ())?;
                 }
                 HoistDependencyResult::Placement(dest) => {
+                    builder.mark_required(dependency, resolution_list, pkg_id);
                     {
                         // Go through ListExt
                         // accessors sequentially so the &mut borrows do not overlap.
@@ -1124,12 +1183,12 @@ impl Tree {
 
             if res_id == package_id {
                 // this dependency is the same package as the other, hoist
-                return HoistDependencyResult::Hoisted; // 1
+                return HoistDependencyResult::Hoisted(res_id); // 1
             }
 
             if input_dep_range.contains(dep_id) {
                 // same package lists this name in another dependency group
-                return HoistDependencyResult::Hoisted; // 1
+                return HoistDependencyResult::Hoisted(res_id); // 1
             }
 
             // now we either keep the dependency at this place in the tree,
@@ -1142,7 +1201,7 @@ impl Tree {
                     {
                         HoistDependencyResult::Rebind(res_id)
                     } else {
-                        HoistDependencyResult::Hoisted
+                        HoistDependencyResult::Hoisted(res_id)
                     }
                 };