fix: incremental make rebuild removed entry module (#9209)

* test: add test for make rebuild removed entry module

* fix: incremental make rebuild removed entry module

* fix: incremental make rebuild removed entry module

---------

Co-authored-by: ahabhgk <ahabhgk@gmail.com>

Author: jerrykingxyz

crates/rspack_core/src/cache/persistent/occasion/make/module_graph.rs

@@ -187,7 +187,7 @@ pub async fn recovery_module_graph(
       if module_exist {
         None
       } else {
-        mg.revoke_connection(dep_id, false)
+        mg.revoke_dependency(dep_id, false)
       }
     })
     .collect::<HashSet<_>>();

crates/rspack_core/src/compiler/compilation.rs

@@ -821,7 +821,6 @@ impl Compilation {
   #[instrument("Compilation:make", skip_all)]
   pub async fn make(&mut self) -> Result<()> {
     self.make_artifact.reset_dependencies_incremental_info();
-    //        self.module_executor.
     // run module_executor
     if let Some(module_executor) = &mut self.module_executor {
       let mut module_executor = std::mem::take(module_executor);

crates/rspack_core/src/compiler/make/cutout/mod.rs

@@ -3,7 +3,7 @@ mod fix_build_meta;
 mod fix_issuers;
 mod has_module_graph_change;
 
-use rspack_collections::IdentifierSet;
+use rspack_collections::{IdentifierSet, UkeySet};
 use rustc_hash::FxHashSet as HashSet;
 
 use self::{
@@ -27,33 +27,22 @@ impl Cutout {
     artifact: &mut MakeArtifact,
     params: Vec<MakeParam>,
   ) -> HashSet<BuildDependency> {
-    let mut entry_dependencies = std::mem::take(&mut artifact.entry_dependencies);
+    let mut next_entry_dependencies = HashSet::default();
+    let mut clean_entry_dependencies = false;
+    let mut useless_entry_dependencies = UkeySet::default();
     let mut force_build_modules = IdentifierSet::default();
     let mut force_build_deps = HashSet::default();
-    let mut remove_entry_deps = HashSet::default();
 
     let module_graph = artifact.get_module_graph();
 
     for item in params {
       match item {
         MakeParam::BuildEntry(deps) => {
-          for dep_id in deps {
-            if !entry_dependencies.contains(&dep_id) {
-              force_build_deps.insert((dep_id, None));
-              entry_dependencies.insert(dep_id);
-            }
-          }
+          next_entry_dependencies = deps;
         }
         MakeParam::BuildEntryAndClean(deps) => {
-          remove_entry_deps.extend(std::mem::take(&mut entry_dependencies));
-          entry_dependencies = deps;
-          for dep_id in &entry_dependencies {
-            if remove_entry_deps.contains(dep_id) {
-              remove_entry_deps.remove(dep_id);
-            } else {
-              force_build_deps.insert((*dep_id, None));
-            }
-          }
+          next_entry_dependencies = deps;
+          clean_entry_dependencies = true;
         }
         MakeParam::CheckNeedBuild => {
           force_build_modules.extend(module_graph.modules().values().filter_map(|module| {
@@ -65,32 +54,30 @@ impl Cutout {
           }));
         }
         MakeParam::ModifiedFiles(files) => {
-          force_build_modules.extend(module_graph.modules().values().filter_map(|module| {
+          for module in module_graph.modules().values() {
             // check has dependencies modified
             if module.depends_on(&files) {
-              Some(module.identifier())
-            } else {
-              None
+              // add module id
+              force_build_modules.insert(module.identifier());
             }
-          }))
+          }
         }
         MakeParam::RemovedFiles(files) => {
-          force_build_modules.extend(module_graph.modules().values().flat_map(|module| {
-            let mut res = vec![];
-
+          for module in module_graph.modules().values() {
             // check has dependencies modified
             if module.depends_on(&files) {
               // add module id
-              res.push(module.identifier());
-              // add parent module id
-              res.extend(
-                module_graph
-                  .get_incoming_connections(&module.identifier())
-                  .filter_map(|connect| connect.original_module_identifier),
-              )
+              force_build_modules.insert(module.identifier());
+              // process parent module id
+              for connect in module_graph.get_incoming_connections(&module.identifier()) {
+                if let Some(original_module_identifier) = connect.original_module_identifier {
+                  force_build_modules.insert(original_module_identifier);
+                } else {
+                  useless_entry_dependencies.insert(connect.dependency_id);
+                }
+              }
             }
-            res
-          }))
+          }
         }
         MakeParam::ForceBuildDeps(deps) => {
           for item in deps {
@@ -128,23 +115,55 @@ impl Cutout {
       force_build_deps.extend(artifact.revoke_module(&id));
     }
 
+    let mut entry_dependencies = std::mem::take(&mut artifact.entry_dependencies);
     let mut module_graph = artifact.get_module_graph_mut();
-    for dep_id in remove_entry_deps {
+    // remove useless entry dependencies
+    let mut remove_entry_dependencies = HashSet::default();
+    for dep_id in useless_entry_dependencies {
+      if !next_entry_dependencies.contains(&dep_id) {
+        remove_entry_dependencies.insert(dep_id);
+      }
+    }
+    if clean_entry_dependencies {
+      remove_entry_dependencies.extend(entry_dependencies.difference(&next_entry_dependencies));
+    }
+    for dep_id in remove_entry_dependencies {
       // connection may have been deleted by revoke module
       if let Some(con) = module_graph.connection_by_dependency_id(&dep_id) {
+        // need clean_isolated_module to check whether the module is still used by other deps
         self
           .clean_isolated_module
           .add_need_check_module(*con.module_identifier());
-        module_graph.revoke_connection(&dep_id, true);
       }
-      force_build_deps.remove(&(dep_id, None));
+      module_graph.revoke_dependency(&dep_id, true);
+      entry_dependencies.remove(&dep_id);
+    }
+
+    // add entry dependencies
+    for dep in next_entry_dependencies
+      .difference(&entry_dependencies)
+      .copied()
+      .collect::<Vec<_>>()
+    {
+      force_build_deps.insert((dep, None));
+      entry_dependencies.insert(dep);
     }
 
     artifact.entry_dependencies = entry_dependencies;
 
     self.has_module_graph_change.analyze_artifact(artifact);
 
+    // only return available force_build_deps
+    let module_graph = artifact.get_module_graph();
     force_build_deps
+      .into_iter()
+      .filter(|(dep_id, _)| {
+        let Some(dep) = module_graph.dependency_by_id(dep_id) else {
+          return false;
+        };
+        dep.as_module_dependency().is_some() || dep.as_context_dependency().is_some()
+      })
+      .collect()
   }
 
   pub fn fix_artifact(self, artifact: &mut MakeArtifact) {

crates/rspack_core/src/compiler/make/repair/mod.rs

@@ -102,22 +102,13 @@ pub async fn repair(
   let module_graph = artifact.get_module_graph_mut();
   let init_tasks = build_dependencies
     .into_iter()
-    .filter_map::<Box<dyn Task<MakeTaskContext>>, _>(|(id, parent_module_identifier)| {
+    .map::<Box<dyn Task<MakeTaskContext>>, _>(|(id, parent_module_identifier)| {
       let dependency = module_graph
         .dependency_by_id(&id)
         .expect("dependency not found");
-      // filter module_dependency and context_dependency
-      if dependency.as_module_dependency().is_none() && dependency.as_context_dependency().is_none()
-      {
-        return None;
-      }
 
-      // filter parent module existed dependency
       let parent_module =
         parent_module_identifier.and_then(|id| module_graph.module_by_identifier(&id));
-      if parent_module_identifier.is_some() && parent_module.is_none() {
-        return None;
-      }
 
       let current_profile = compilation
         .options
@@ -134,7 +125,7 @@ pub async fn repair(
             None
           }
         });
-      Some(Box::new(factorize::FactorizeTask {
+      Box::new(factorize::FactorizeTask {
         compilation_id: compilation.id(),
         module_factory: compilation.get_dependency_factory(dependency),
         original_module_identifier: parent_module_identifier,
@@ -149,7 +140,7 @@ pub async fn repair(
         options: compilation.options.clone(),
         current_profile,
         resolver_factory: compilation.resolver_factory.clone(),
-      }))
+      })
     })
     .collect::<Vec<_>>();
 

crates/rspack_core/src/module_graph/mod.rs

@@ -213,35 +213,41 @@ impl<'a> ModuleGraph<'a> {
     map
   }
 
-  /// Remove a connection and return connection origin module identifier and dependency
+  /// Remove dependency in mgm and target connection, return dependency_id and origin module identifier
   ///
   /// force will completely remove dependency, and you will not regenerate it from dependency_id
-  pub fn revoke_connection(
+  pub fn revoke_dependency(
     &mut self,
     dep_id: &DependencyId,
     force: bool,
   ) -> Option<BuildDependency> {
-    let connection = self.connection_by_dependency_id(dep_id)?;
-    let module_identifier = *connection.module_identifier();
-    let original_module_identifier = connection.original_module_identifier;
+    let connection_info = self.connection_by_dependency_id(dep_id).map(|connection| {
+      (
+        connection.original_module_identifier,
+        *connection.module_identifier(),
+      )
+    });
 
     let Some(active_partial) = &mut self.active else {
       panic!("should have active partial");
     };
-    active_partial.connections.insert(*dep_id, None);
+    if connection_info.is_some() {
+      active_partial.connections.insert(*dep_id, None);
+    }
     if force {
       active_partial.dependencies.insert(*dep_id, None);
       active_partial
         .dependency_id_to_parents
         .insert(*dep_id, None);
     }
 
+    let (original_module_identifier, module_identifier) = connection_info?;
     // remove outgoing from original module graph module
     if let Some(original_module_identifier) = &original_module_identifier {
       if let Some(mgm) = self.module_graph_module_by_identifier_mut(original_module_identifier) {
         mgm.remove_outgoing_connection(dep_id);
-        // Because of mgm.dependencies is set when original module build success
-        // it does not need to remove dependency in mgm.dependencies.
+        // Because of mgm.all_dependencies is set when original module build success
+        // it does not need to remove dependency in mgm.all_dependencies.
       }
     }
     // remove incoming from module graph module
@@ -258,12 +264,12 @@ impl<'a> ModuleGraph<'a> {
       .map(|m| Vec::from(m.get_blocks()))
       .unwrap_or_default();
 
-    let (outgoing_connections, incoming_connections) = self
+    let (incoming_connections, all_dependencies) = self
       .module_graph_module_by_identifier(module_id)
       .map(|mgm| {
         (
-          mgm.outgoing_connections().clone(),
           mgm.incoming_connections().clone(),
+          mgm.all_dependencies.clone(),
         )
       })
       .unwrap_or_default();
@@ -279,13 +285,13 @@ impl<'a> ModuleGraph<'a> {
       active_partial.blocks.insert(block, None);
     }
 
-    for cid in outgoing_connections {
-      self.revoke_connection(&cid, true);
+    for dep_id in all_dependencies {
+      self.revoke_dependency(&dep_id, true);
     }
 
     incoming_connections
       .iter()
-      .filter_map(|cid| self.revoke_connection(cid, false))
+      .filter_map(|dep_id| self.revoke_dependency(dep_id, false))
       .collect()
   }
 

crates/rspack_plugin_library/src/modern_module_library_plugin.rs

@@ -273,7 +273,7 @@ async fn finish_modules(&self, compilation: &mut Compilation) -> Result<()> {
       let boxed_dep = Box::new(new_dep.clone()) as BoxDependency;
       block.add_dependency_id(*new_dep.id());
       mg.add_dependency(boxed_dep);
-      mg.revoke_connection(connection_id, true);
+      mg.revoke_dependency(connection_id, true);
     }
   }
 
@@ -380,7 +380,7 @@ async fn finish_modules(&self, compilation: &mut Compilation) -> Result<()> {
         .expect("should have module");
 
       importer.remove_dependency_id(*connection);
-      mg.revoke_connection(connection, true);
+      mg.revoke_dependency(connection, true);
     }
   }
 

tests/e2e/cases/make/remove-dynamic-entry/index.test.ts

@@ -0,0 +1,19 @@
+import { expect, test } from "@/fixtures";
+
+test("should compile", async ({ page, fileAction, rspack }) => {
+	await expect(page.getByText("index1")).toBeVisible();
+	await expect(page.getByText("index2")).toBeVisible();
+
+	fileAction.deleteFile("src/index2.js");
+	fileAction.updateFile("src/index1.js", content => content.replace("div.innerText = \"index1\";", "div.innerText = \"index1 updated\";"));
+
+	await expect(async () => {
+		await page.reload();
+		expect(await page.locator("#index1").innerText()).toBe("index1 updated");
+	}).toPass();
+	await expect(page.locator("#index2")).toHaveCount(0);
+	await expect(page.locator("#webpack-dev-server-client-overlay")).toHaveCount(0);
+
+	const stats = rspack.compiler._lastCompilation?.getStats().toJson({ all: false, errors: true });
+	expect(stats?.errors?.length).toBe(0);
+});

tests/e2e/cases/make/remove-dynamic-entry/rspack.config.js

@@ -0,0 +1,25 @@
+const path = require("path")
+const fs = require("fs");
+const rspack = require("@rspack/core");
+
+/** @type {import("@rspack/core").Configuration} */
+module.exports = {
+	entry: async () => {
+		const context = path.resolve(__dirname, "src");
+		const files = await fs.promises.readdir(context);
+		let entries = files.filter(f => f.startsWith('index'));
+		entries.sort();
+		return entries.reduce((acc, e, i) => {
+			acc[`index${i + 1}`] = path.resolve(context, e);
+			return acc;
+		}, {})
+	},
+	context: __dirname,
+	mode: "development",
+	plugins: [
+		new rspack.HtmlRspackPlugin(),
+	],
+	devServer: {
+		hot: true
+	},
+};

tests/e2e/cases/make/remove-dynamic-entry/src/index1.js

@@ -0,0 +1,6 @@
+Promise.resolve().then(() => {
+  const div = document.createElement("div");
+  div.innerText = "index1";
+  div.id = "index1";
+  document.body.appendChild(div);  
+})

tests/e2e/cases/make/remove-dynamic-entry/src/index2.js

@@ -0,0 +1,4 @@
+const div = document.createElement("div");
+div.innerText = "index2";
+div.id = "index2";
+document.body.appendChild(div);