refactor: module diagnostic action move to Diagnosable trait (#9264)

* refactor: Diagnosable trait use mutable self

* refactor: update Diagnosable trait

Author: jerrykingxyz

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

@@ -145,7 +145,7 @@ mod t {
 
   use rspack_cacheable::{cacheable, cacheable_dyn, with::Skip};
   use rspack_collections::Identifiable;
-  use rspack_error::{impl_empty_diagnosable_trait, Diagnostic, Result};
+  use rspack_error::{impl_empty_diagnosable_trait, Result};
   use rspack_macros::impl_source_map_config;
   use rspack_sources::Source;
   use rspack_util::{atom::Atom, source_map::SourceMapKind};
@@ -285,10 +285,6 @@ mod t {
       todo!()
     }
 
-    fn get_diagnostics(&self) -> Vec<Diagnostic> {
-      todo!()
-    }
-
     fn original_source(&self) -> Option<&dyn Source> {
       todo!()
     }

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

@@ -61,8 +61,9 @@ impl Task<MakeTaskContext> for BuildTask {
 
     let build_result = result.map(|t| {
       let diagnostics = module
-        .clone_diagnostics()
-        .into_iter()
+        .diagnostics()
+        .iter()
+        .cloned()
         .map(|d| d.with_module_identifier(Some(module.identifier())))
         .collect();
       t.with_diagnostic(diagnostics)

crates/rspack_core/src/concatenated_module.rs

@@ -3,7 +3,7 @@ use std::{
   collections::hash_map::Entry,
   fmt::Debug,
   hash::{BuildHasherDefault, Hasher},
-  sync::{Arc, LazyLock, Mutex},
+  sync::{Arc, LazyLock},
 };
 
 use dashmap::DashMap;
@@ -372,7 +372,7 @@ pub struct ConcatenatedModule {
   #[cacheable(with=AsMap)]
   cached_source_sizes: DashMap<SourceType, f64, BuildHasherDefault<FxHasher>>,
   #[cacheable(with=Skip)]
-  diagnostics: Mutex<Vec<Diagnostic>>,
+  diagnostics: Vec<Diagnostic>,
   build_info: BuildInfo,
 }
 
@@ -394,7 +394,7 @@ impl ConcatenatedModule {
       dependencies: vec![],
       blocks: vec![],
       cached_source_sizes: DashMap::default(),
-      diagnostics: Mutex::new(vec![]),
+      diagnostics: vec![],
       build_info: BuildInfo {
         cacheable: true,
         hash: None,
@@ -490,11 +490,6 @@ impl Module for ConcatenatedModule {
     &ModuleType::JsEsm
   }
 
-  fn get_diagnostics(&self) -> Vec<Diagnostic> {
-    let guard = self.diagnostics.lock().expect("should have diagnostics");
-    guard.clone()
-  }
-
   fn factory_meta(&self) -> Option<&FactoryMeta> {
     self.root_module_ctxt.factory_meta.as_ref()
   }
@@ -556,9 +551,6 @@ impl Module for ConcatenatedModule {
     compilation: Option<&Compilation>,
   ) -> Result<BuildResult> {
     let compilation = compilation.expect("should pass compilation");
-    // https://github.com/webpack/webpack/blob/1f99ad6367f2b8a6ef17cce0e058f7a67fb7db18/lib/optimize/ConcatenatedModule.js#L774-L784
-    // Some fields does not exists in rspack
-    self.clear_diagnostics();
 
     let module_graph = compilation.get_module_graph();
     let modules = self
@@ -592,12 +584,8 @@ impl Module for ConcatenatedModule {
       for b in module.get_blocks() {
         self.blocks.push(*b);
       }
-      let mut diagnostics_guard = self.diagnostics.lock().expect("should have diagnostics");
       // populate diagnostic
-      diagnostics_guard.extend(module.get_diagnostics());
-
-      // release guard ASAP
-      drop(diagnostics_guard);
+      self.diagnostics.extend(module.diagnostics().into_owned());
 
       // populate topLevelDeclarations
       let module_build_info = module.build_info();
@@ -1411,42 +1399,20 @@ impl Module for ConcatenatedModule {
 }
 
 impl Diagnosable for ConcatenatedModule {
-  fn add_diagnostic(&self, diagnostic: Diagnostic) {
-    self
-      .diagnostics
-      .lock()
-      .expect("should be able to lock diagnostics")
-      .push(diagnostic);
-  }
-
-  fn add_diagnostics(&self, mut diagnostics: Vec<Diagnostic>) {
-    self
-      .diagnostics
-      .lock()
-      .expect("should be able to lock diagnostics")
-      .append(&mut diagnostics);
-  }
-
-  fn clone_diagnostics(&self) -> Vec<Diagnostic> {
-    self
-      .diagnostics
-      .lock()
-      .expect("should be able to lock diagnostics")
-      .iter()
-      .cloned()
-      .collect()
+  fn add_diagnostic(&mut self, diagnostic: Diagnostic) {
+    self.diagnostics.push(diagnostic);
   }
-}
 
-impl ConcatenatedModule {
-  fn clear_diagnostics(&mut self) {
-    self
-      .diagnostics
-      .lock()
-      .expect("should be able to lock diagnostics")
-      .clear()
+  fn add_diagnostics(&mut self, mut diagnostics: Vec<Diagnostic>) {
+    self.diagnostics.append(&mut diagnostics);
   }
 
+  fn diagnostics(&self) -> Cow<[Diagnostic]> {
+    Cow::Borrowed(&self.diagnostics)
+  }
+}
+
+impl ConcatenatedModule {
   // TODO: replace self.modules with indexmap or linkedhashset
   fn get_modules_with_info(
     &self,

crates/rspack_core/src/context_module.rs

@@ -10,7 +10,7 @@ use rspack_cacheable::{
   with::{AsOption, AsPreset, AsVec, Unsupported},
 };
 use rspack_collections::{Identifiable, Identifier};
-use rspack_error::{impl_empty_diagnosable_trait, Diagnostic, Result};
+use rspack_error::{impl_empty_diagnosable_trait, Result};
 use rspack_macros::impl_source_map_config;
 use rspack_paths::{ArcPath, Utf8PathBuf};
 use rspack_regex::RspackRegex;
@@ -847,10 +847,6 @@ impl Module for ContextModule {
     &[SourceType::JavaScript]
   }
 
-  fn get_diagnostics(&self) -> Vec<Diagnostic> {
-    vec![]
-  }
-
   fn original_source(&self) -> Option<&dyn rspack_sources::Source> {
     None
   }

crates/rspack_core/src/external_module.rs

@@ -2,7 +2,7 @@ use std::{borrow::Cow, hash::Hash, iter};
 
 use rspack_cacheable::{cacheable, cacheable_dyn};
 use rspack_collections::{Identifiable, Identifier};
-use rspack_error::{error, impl_empty_diagnosable_trait, Diagnostic, Result};
+use rspack_error::{error, impl_empty_diagnosable_trait, Result};
 use rspack_hash::RspackHash;
 use rspack_macros::impl_source_map_config;
 use rspack_util::{ext::DynHash, json_stringify, source_map::SourceMapKind};
@@ -490,10 +490,6 @@ impl Module for ExternalModule {
     &ModuleType::JsAuto
   }
 
-  fn get_diagnostics(&self) -> Vec<Diagnostic> {
-    vec![]
-  }
-
   fn source_types(&self) -> &[SourceType] {
     if self.external_type == "css-import" {
       EXTERNAL_MODULE_CSS_SOURCE_TYPES

crates/rspack_core/src/module.rs

@@ -11,7 +11,7 @@ use rspack_cacheable::{
   with::{AsOption, AsVec},
 };
 use rspack_collections::{Identifiable, Identifier, IdentifierSet};
-use rspack_error::{Diagnosable, Diagnostic, Result};
+use rspack_error::{Diagnosable, Result};
 use rspack_fs::ReadableFileSystem;
 use rspack_hash::RspackHashDigest;
 use rspack_paths::ArcPath;
@@ -218,7 +218,7 @@ pub trait Module:
 
   /// Defines what kind of code generation results this module can generate.
   fn source_types(&self) -> &[SourceType];
-  fn get_diagnostics(&self) -> Vec<Diagnostic>;
+
   /// The original source of the module. This could be optional, modules like the `NormalModule` can have the corresponding original source.
   /// However, modules that is created from "nowhere" (e.g. `ExternalModule` and `MissingModule`) does not have its original source.
   fn original_source(&self) -> Option<&dyn Source>;
@@ -606,7 +606,7 @@ mod test {
 
   use rspack_cacheable::cacheable;
   use rspack_collections::{Identifiable, Identifier};
-  use rspack_error::{Diagnosable, Diagnostic, Result};
+  use rspack_error::{impl_empty_diagnosable_trait, Result};
   use rspack_sources::Source;
   use rspack_util::source_map::{ModuleSourceMapConfig, SourceMapKind};
 
@@ -633,7 +633,7 @@ mod test {
         }
       }
 
-      impl Diagnosable for $ident {}
+      impl_empty_diagnosable_trait!($ident);
 
       impl DependenciesBlock for $ident {
         fn add_block_id(&mut self, _: AsyncDependenciesBlockIdentifier) {
@@ -692,10 +692,6 @@ mod test {
           unreachable!()
         }
 
-        fn get_diagnostics(&self) -> Vec<Diagnostic> {
-          vec![]
-        }
-
         fn update_hash(
           &self,
           _hasher: &mut dyn std::hash::Hasher,

crates/rspack_core/src/normal_module.rs

@@ -4,7 +4,7 @@ use std::{
   ptr::NonNull,
   sync::{
     atomic::{AtomicUsize, Ordering},
-    Arc, Mutex,
+    Arc,
   },
 };
 
@@ -143,7 +143,7 @@ pub struct NormalModule {
   #[cacheable(with=AsMap)]
   cached_source_sizes: DashMap<SourceType, f64, BuildHasherDefault<FxHasher>>,
   #[cacheable(with=Skip)]
-  diagnostics: Mutex<Vec<Diagnostic>>,
+  diagnostics: Vec<Diagnostic>,
 
   code_generation_dependencies: Option<Vec<Box<dyn ModuleDependency>>>,
   presentational_dependencies: Option<Vec<Box<dyn DependencyTemplate>>>,
@@ -231,7 +231,7 @@ impl NormalModule {
       debug_id: DEBUG_ID.fetch_add(1, Ordering::Relaxed),
 
       cached_source_sizes: DashMap::default(),
-      diagnostics: Mutex::new(Default::default()),
+      diagnostics: Default::default(),
       code_generation_dependencies: None,
       presentational_dependencies: None,
       factory_meta: None,
@@ -371,11 +371,6 @@ impl Module for NormalModule {
     &self.module_type
   }
 
-  fn get_diagnostics(&self) -> Vec<Diagnostic> {
-    let guard = self.diagnostics.lock().expect("should have diagnostics");
-    guard.clone()
-  }
-
   fn source_types(&self) -> &[SourceType] {
     self.parser_and_generator.source_types()
   }
@@ -408,8 +403,6 @@ impl Module for NormalModule {
     build_context: BuildContext,
     _compilation: Option<&Compilation>,
   ) -> Result<BuildResult> {
-    self.clear_diagnostics();
-
     // so does webpack
     self.parsed = true;
 
@@ -545,7 +538,7 @@ impl Module for NormalModule {
     if no_parse {
       self.parsed = false;
       self.original_source = Some(original_source.clone());
-      self.source = NormalModuleSource::new_built(original_source, self.clone_diagnostics());
+      self.source = NormalModuleSource::new_built(original_source, self.diagnostics.clone());
       self.code_generation_dependencies = Some(Vec::new());
       self.presentational_dependencies = Some(Vec::new());
 
@@ -607,7 +600,7 @@ impl Module for NormalModule {
     // Only side effects used in code_generate can stay here
     // Other side effects should be set outside use_cache
     self.original_source = Some(source.clone());
-    self.source = NormalModuleSource::new_built(source, self.clone_diagnostics());
+    self.source = NormalModuleSource::new_built(source, self.diagnostics.clone());
     self.code_generation_dependencies = Some(code_generation_dependencies);
     self.presentational_dependencies = Some(presentational_dependencies);
 
@@ -798,30 +791,16 @@ impl Module for NormalModule {
 }
 
 impl Diagnosable for NormalModule {
-  fn add_diagnostic(&self, diagnostic: Diagnostic) {
-    self
-      .diagnostics
-      .lock()
-      .expect("should be able to lock diagnostics")
-      .push(diagnostic);
+  fn add_diagnostic(&mut self, diagnostic: Diagnostic) {
+    self.diagnostics.push(diagnostic);
   }
 
-  fn add_diagnostics(&self, mut diagnostics: Vec<Diagnostic>) {
-    self
-      .diagnostics
-      .lock()
-      .expect("should be able to lock diagnostics")
-      .append(&mut diagnostics);
+  fn add_diagnostics(&mut self, mut diagnostics: Vec<Diagnostic>) {
+    self.diagnostics.append(&mut diagnostics);
   }
 
-  fn clone_diagnostics(&self) -> Vec<Diagnostic> {
-    self
-      .diagnostics
-      .lock()
-      .expect("should be able to lock diagnostics")
-      .iter()
-      .cloned()
-      .collect()
+  fn diagnostics(&self) -> Cow<[Diagnostic]> {
+    Cow::Borrowed(&self.diagnostics)
   }
 }
 
@@ -851,12 +830,4 @@ impl NormalModule {
     }
     Ok(RawStringSource::from(content.into_string_lossy()).boxed())
   }
-
-  fn clear_diagnostics(&mut self) {
-    self
-      .diagnostics
-      .lock()
-      .expect("should be able to lock diagnostics")
-      .clear()
-  }
 }

crates/rspack_core/src/raw_module.rs

@@ -2,7 +2,7 @@ use std::borrow::Cow;
 
 use rspack_cacheable::{cacheable, cacheable_dyn, with::AsPreset};
 use rspack_collections::Identifiable;
-use rspack_error::{impl_empty_diagnosable_trait, Diagnostic, Result};
+use rspack_error::{impl_empty_diagnosable_trait, Result};
 use rspack_macros::impl_source_map_config;
 use rspack_sources::{BoxSource, RawStringSource, Source, SourceExt};
 use rspack_util::source_map::SourceMapKind;
@@ -92,10 +92,6 @@ impl DependenciesBlock for RawModule {
 impl Module for RawModule {
   impl_module_meta_info!();
 
-  fn get_diagnostics(&self) -> Vec<Diagnostic> {
-    vec![]
-  }
-
   fn module_type(&self) -> &ModuleType {
     &ModuleType::JsAuto
   }

crates/rspack_core/src/self_module.rs

@@ -3,7 +3,7 @@ use std::borrow::Cow;
 use async_trait::async_trait;
 use rspack_cacheable::{cacheable, cacheable_dyn};
 use rspack_collections::{Identifiable, Identifier};
-use rspack_error::{impl_empty_diagnosable_trait, Diagnostic, Result};
+use rspack_error::{impl_empty_diagnosable_trait, Result};
 use rspack_macros::impl_source_map_config;
 use rspack_sources::Source;
 use rspack_util::source_map::SourceMapKind;
@@ -79,10 +79,6 @@ impl DependenciesBlock for SelfModule {
 impl Module for SelfModule {
   impl_module_meta_info!();
 
-  fn get_diagnostics(&self) -> Vec<Diagnostic> {
-    vec![]
-  }
-
   fn size(&self, _source_type: Option<&SourceType>, _compilation: Option<&Compilation>) -> f64 {
     self.identifier.len() as f64
   }