fix: cargo lint error

web-infra-dev · Aug 9, 2024 · 506e804 · 506e804
1 parent 09888c8
commit 506e804
Show file tree

Hide file tree

Showing 5 changed files with 88 additions and 60 deletions.
diff --git a/crates/node_binding/src/lib.rs b/crates/node_binding/src/lib.rs
@@ -165,7 +165,7 @@ impl Rspack {
       Ok(unsafe { s.compiler.as_mut().get_unchecked_mut() })
     })?;
 
-    self.cleanup_last_compilation(env, &compiler.compilation);
+    self.cleanup_last_compilation(&compiler.compilation);
 
     // SAFETY:
     // 1. `Compiler` is pinned and stored on the heap.
@@ -176,8 +176,8 @@ impl Rspack {
     )
   }
 
-  fn cleanup_last_compilation(&self, env: Env, compilation: &Compilation) {
-    JsCompilationWrapper::cleanup(env.raw(), compilation.id());
+  fn cleanup_last_compilation(&self, compilation: &Compilation) {
+    JsCompilationWrapper::cleanup(compilation.id());
   }
 }
 

diff --git a/crates/rspack_binding_values/src/compilation/mod.rs b/crates/rspack_binding_values/src/compilation/mod.rs
@@ -30,7 +30,6 @@ use crate::utils::callbackify;
 use crate::JsStatsOptimizationBailout;
 use crate::LocalJsFilename;
 use crate::ModuleDTOWrapper;
-use crate::MODULE_INSTANCE_REFS;
 use crate::{
   chunk::JsChunk, CompatSource, JsAsset, JsAssetInfo, JsChunkGroup, JsCompatSource, JsPathData,
   JsStats, ToJsCompatSource,
@@ -563,8 +562,21 @@ impl JsCompilation {
   }
 }
 
+#[derive(Default)]
+struct CompilationInstanceRefs(RefCell<HashMap<CompilationId, (Ref, napi_env)>>);
+
+impl Drop for CompilationInstanceRefs {
+  fn drop(&mut self) {
+    // cleanup references to be executed in cases of panic or unexpected termination
+    let mut refs = self.0.borrow_mut();
+    for (_, (mut r, env)) in refs.drain() {
+      let _ = r.unref(env);
+    }
+  }
+}
+
 thread_local! {
-  pub static COMPILATION_INSTANCE_REFS: RefCell<HashMap<CompilationId, Ref>> = Default::default();
+  static COMPILATION_INSTANCE_REFS: CompilationInstanceRefs = Default::default();
 }
 
 // The difference between JsCompilationWrapper and JsCompilation is:
@@ -586,52 +598,42 @@ impl JsCompilationWrapper {
     })
   }
 
-  pub fn cleanup(env: napi_env, compilation_id: CompilationId) {
+  pub fn cleanup(compilation_id: CompilationId) {
     COMPILATION_INSTANCE_REFS.with(|ref_cell| {
-      let mut refs = ref_cell.borrow_mut();
-      if let Some(mut r) = refs.remove(&compilation_id) {
+      let mut refs = ref_cell.0.borrow_mut();
+      if let Some((mut r, env)) = refs.remove(&compilation_id) {
         let _ = r.unref(env);
       }
     });
-
-    MODULE_INSTANCE_REFS.with(|refs| {
-      let mut refs_by_compilation_id = refs.borrow_mut();
-      let refs = refs_by_compilation_id.remove(&compilation_id);
-      if let Some(mut refs) = refs {
-        for (_, mut r) in refs.drain() {
-          let _ = r.unref(env);
-        }
-      }
-    });
+    ModuleDTOWrapper::cleanup(compilation_id);
   }
 }
 
 impl ToNapiValue for JsCompilationWrapper {
   unsafe fn to_napi_value(env: sys::napi_env, val: Self) -> Result<sys::napi_value> {
     COMPILATION_INSTANCE_REFS.with(|ref_cell| {
       let mut env_wrapper = Env::from_raw(env);
-      let mut refs = ref_cell.borrow_mut();
+      let mut refs = ref_cell.0.borrow_mut();
       let compilation_id = val.0.id();
       let mut vacant = false;
       let napi_value = match refs.entry(compilation_id) {
         std::collections::hash_map::Entry::Occupied(entry) => {
           let r = entry.get();
-          ToNapiValue::to_napi_value(env, r)
+          ToNapiValue::to_napi_value(env, &r.0)
         }
         std::collections::hash_map::Entry::Vacant(entry) => {
           vacant = true;
           let instance = JsCompilation(val.0).into_instance(env_wrapper)?;
           let napi_value = ToNapiValue::to_napi_value(env, instance)?;
           let r = Ref::new(env, napi_value, 1)?;
-          let r = entry.insert(r);
-          ToNapiValue::to_napi_value(env, r)
+          let r = entry.insert((r, env));
+          ToNapiValue::to_napi_value(env, &r.0)
         }
       };
       if vacant {
-        // cleanup references to be executed in cases of panic or unexpected termination
-        let _ = env_wrapper.add_env_cleanup_hook((), move |_| {
-          JsCompilationWrapper::cleanup(env, compilation_id)
-        });
+        // cleanup references to be executed when the JS thread exits normally
+        let _ = env_wrapper
+          .add_env_cleanup_hook((), move |_| JsCompilationWrapper::cleanup(compilation_id));
       }
       napi_value
     })

diff --git a/crates/rspack_binding_values/src/module.rs b/crates/rspack_binding_values/src/module.rs
@@ -8,6 +8,7 @@ use rspack_core::{
 };
 use rspack_napi::{napi::bindgen_prelude::*, Ref};
 use rustc_hash::FxHashMap as HashMap;
+use sys::napi_env;
 
 use super::{JsCompatSource, ToJsCompatSource};
 use crate::{DependencyDTO, JsChunk, JsCodegenerationResults};
@@ -217,16 +218,29 @@ impl ModuleDTO {
   #[napi]
   pub fn size(&self, ty: Option<String>) -> f64 {
     let module = self.module();
-    let ty = match ty {
-      Some(s) => Some(SourceType::from(s.as_str())),
-      None => None,
-    };
+    let ty = ty.map(|s| SourceType::from(s.as_str()));
     module.size(ty.as_ref(), self.compilation)
   }
 }
 
+type ModuleInstanceRefs = HashMap<Identifier, (Ref, napi_env)>;
+
+#[derive(Default)]
+struct ModuleInstanceRefsByCompilationId(RefCell<HashMap<CompilationId, ModuleInstanceRefs>>);
+
+impl Drop for ModuleInstanceRefsByCompilationId {
+  fn drop(&mut self) {
+    let mut refs_by_compilation_id = self.0.borrow_mut();
+    for (_, mut refs) in refs_by_compilation_id.drain() {
+      for (_, (mut r, env)) in refs.drain() {
+        let _ = r.unref(env);
+      }
+    }
+  }
+}
+
 thread_local! {
-  pub static MODULE_INSTANCE_REFS: RefCell<HashMap<CompilationId, HashMap<Identifier, Ref>>> = Default::default();
+  static MODULE_INSTANCE_REFS: ModuleInstanceRefsByCompilationId = Default::default();
 }
 
 // The difference between ModuleDTOWrapper and ModuleDTO is:
@@ -254,12 +268,23 @@ impl ModuleDTOWrapper {
       compilation,
     }
   }
+
+  pub fn cleanup(compilation_id: CompilationId) {
+    MODULE_INSTANCE_REFS.with(|refs| {
+      let mut refs_by_compilation_id = refs.0.borrow_mut();
+      if let Some(mut refs) = refs_by_compilation_id.remove(&compilation_id) {
+        for (_, (mut r, env)) in refs.drain() {
+          let _ = r.unref(env);
+        }
+      }
+    });
+  }
 }
 
 impl ToNapiValue for ModuleDTOWrapper {
   unsafe fn to_napi_value(env: sys::napi_env, val: Self) -> Result<sys::napi_value> {
     MODULE_INSTANCE_REFS.with(|refs| {
-      let mut refs_by_compilation_id = refs.borrow_mut();
+      let mut refs_by_compilation_id = refs.0.borrow_mut();
       let entry = refs_by_compilation_id.entry(val.compilation.id());
       let refs = match entry {
         std::collections::hash_map::Entry::Occupied(entry) => entry.into_mut(),
@@ -271,15 +296,15 @@ impl ToNapiValue for ModuleDTOWrapper {
       match refs.entry(val.module_id) {
         std::collections::hash_map::Entry::Occupied(entry) => {
           let r = entry.get();
-          ToNapiValue::to_napi_value(env, r)
+          ToNapiValue::to_napi_value(env, &r.0)
         }
         std::collections::hash_map::Entry::Vacant(entry) => {
           let instance =
             ModuleDTO::new(val.module_id, val.compilation).into_instance(Env::from_raw(env))?;
           let napi_value = ToNapiValue::to_napi_value(env, instance)?;
           let r = Ref::new(env, napi_value, 1)?;
-          let r = entry.insert(r);
-          ToNapiValue::to_napi_value(env, r)
+          let r = entry.insert((r, env));
+          ToNapiValue::to_napi_value(env, &r.0)
         }
       }
     })

diff --git a/packages/rspack-test-tools/tests/configCases/code-generation/path-ends-with-star/entry.mjs b/packages/rspack-test-tools/tests/configCases/code-generation/path-ends-with-star/entry.mjs
@@ -1,4 +1,4 @@
 import * as a from "./star*/a.js";
 
 export const staticA = a;
-export const dynamicA = await import("./star*/a.js")
+export const dynamicA = await import("./star*/a.js")
diff --git a/.../rspack-test-tools/tests/configCases/code-generation/path-ends-with-star/rspack.config.js b/.../rspack-test-tools/tests/configCases/code-generation/path-ends-with-star/rspack.config.js
@@ -1,29 +1,30 @@
 const isWindows = process.platform === "win32";
 
-const entry = `it("should generate valid code", async () => {${isWindows
-        ? `expect("skip windows").toBe("skip windows");`
-        : `const { staticA, dynamicA } = await import("./entry.mjs"); expect(staticA.a).toBe(1); expect(dynamicA.a).toBe(1);`
-    }});`;
+const entry = `it("should generate valid code", async () => {${
+	isWindows
+		? `expect("skip windows").toBe("skip windows");`
+		: `const { staticA, dynamicA } = await import("./entry.mjs"); expect(staticA.a).toBe(1); expect(dynamicA.a).toBe(1);`
+	}});`;
 
 /** @type {import("@rspack/core").Configuration} */
 module.exports = {
-    entry: `data:text/javascript,${entry}`,
-    plugins: [
-        function skipWindows(compiler) {
-            // windows' path can't include *
-            if (!isWindows) {
-                const fs = require("fs");
-                const path = require("path");
-                const dir = path.resolve(__dirname, "star*");
-                if (!fs.existsSync(dir)) {
-                    fs.mkdirSync(dir);
-                }
-                fs.writeFileSync(path.resolve(dir, "a.js"), "export const a = 1;");
-                // cleanup
-                compiler.hooks.done.tap("skipWindows", () => {
-                    fs.rmSync(dir, { recursive: true, force: true });
-                });
-            }
-        }
-    ]
-};
+	entry: `data:text/javascript,${entry}`,
+	plugins: [
+		function skipWindows(compiler) {
+			// windows' path can't include *
+			if (!isWindows) {
+				const fs = require("fs");
+				const path = require("path");
+				const dir = path.resolve(__dirname, "star*");
+				if (!fs.existsSync(dir)) {
+					fs.mkdirSync(dir);
+				}
+				fs.writeFileSync(path.resolve(dir, "a.js"), "export const a = 1;");
+				// cleanup
+				compiler.hooks.done.tap("skipWindows", () => {
+					fs.rmSync(dir, { recursive: true, force: true });
+				});
+			}
+		}
+	]
+};