Restructure logger to enable logger_srcfile (#104)

The overall goal of this change is to support a "sub-trait" of `logger` that adds some src-file logging methods associated against a given logging. This will be used to trace issues with src files while e.g. parsing or interpreting the structure of source files. As part of this change, the `Sync` requirement of logger had to be dropped. This is because the src file informatio nwe get from `swc` is necessarially non-`Sync`/`Send`. --------- Co-authored-by: Max Huang-Hobbs <[email protected]>
Adjective-Object · Dec 2, 2024 · 29cc6f5 · 29cc6f5
1 parent e3a2a58
commit 29cc6f5
Show file tree

Hide file tree

Showing 8 changed files with 129 additions and 13 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/change/@good-fences-api-6ffba5db-a75b-4958-8cba-3e2dea22fa8b.json b/change/@good-fences-api-6ffba5db-a75b-4958-8cba-3e2dea22fa8b.json
@@ -0,0 +1,7 @@
+{
+  "type": "none",
+  "comment": "internal refactoring of loggers",
+  "packageName": "@good-fences/api",
+  "email": "[email protected]",
+  "dependentChangeType": "none"
+}
diff --git a/crates/logger/src/lib.rs b/crates/logger/src/lib.rs
@@ -2,8 +2,14 @@ use std::sync::Mutex;
 
 use anyhow::anyhow;
 
-pub trait Logger: Send + Sync + Clone {
+pub trait Logger: Clone {
     fn log(&self, message: impl Into<String>);
+    fn warn(&self, message: impl Into<String>) {
+        self.log(format!("WARN: {}", message.into()));
+    }
+    fn error(&self, message: impl Into<String>) {
+        self.log(format!("ERROR: {}", message.into()));
+    }
 }
 
 impl<T: Logger> Logger for &T {

diff --git a/crates/logger_srcfile/Cargo.toml b/crates/logger_srcfile/Cargo.toml
@@ -0,0 +1,12 @@
+[package]
+name = "logger_srcfile"
+version = "0.2.0"
+authors = ["Maxwell Huang-Hobbs <[email protected]>"]
+edition = "2018"
+
+[lib]
+crate-type = ["lib"]
+
+[dependencies]
+logger = { path = "../logger" }
+swc_common.workspace = true
diff --git a/crates/logger_srcfile/src/lib.rs b/crates/logger_srcfile/src/lib.rs
@@ -0,0 +1,81 @@
+use logger::Logger;
+use swc_common::{Loc, SourceMap, Span};
+
+pub trait SrcLogger: Logger {
+    fn src_warn(&self, loc: &Loc, message: impl Into<String>) {
+        self.warn(format!(
+            "{}:{}:{} :: {}",
+            loc.file.name,
+            loc.line,
+            loc.col_display,
+            message.into()
+        ));
+    }
+    fn src_error(&self, loc: &Loc, message: impl Into<String>) {
+        self.error(format!(
+            "{}:{}:{} :: {}",
+            loc.file.name,
+            loc.line,
+            loc.col_display,
+            message.into()
+        ));
+    }
+}
+
+pub trait HasSourceMap {
+    fn source_map(&self) -> &SourceMap;
+}
+
+pub trait SrcFileLogger: Logger + HasSourceMap {
+    fn src_warn(&self, location: &Span, message: impl Into<String>) {
+        let loc = self.source_map().lookup_char_pos(location.lo);
+        self.warn(format!(
+            "{}:{}:{} :: {}",
+            loc.file.name,
+            loc.line,
+            loc.col_display,
+            message.into()
+        ));
+    }
+    fn src_error(&self, location: &Span, message: impl Into<String>) {
+        let loc = self.source_map().lookup_char_pos(location.lo);
+        self.error(format!(
+            "{}:{}:{} :: {}",
+            loc.file.name,
+            loc.line,
+            loc.col_display,
+            message.into()
+        ));
+    }
+}
+
+#[derive(Clone)]
+pub struct WrapFileLogger<'a, TLogger: Logger> {
+    source_map: &'a SourceMap,
+    inner_logger: TLogger,
+}
+impl<'a, TLogger: Logger> WrapFileLogger<'a, TLogger> {
+    pub fn new(source_map: &'a SourceMap, inner_logger: TLogger) -> Self {
+        Self {
+            source_map,
+            inner_logger,
+        }
+    }
+}
+impl<TLogger: Logger> Logger for WrapFileLogger<'_, TLogger> {
+    fn log(&self, message: impl Into<String>) {
+        self.inner_logger.log(message);
+    }
+    fn error(&self, message: impl Into<String>) {
+        self.inner_logger.error(message);
+    }
+    fn warn(&self, message: impl Into<String>) {
+        self.inner_logger.warn(message);
+    }
+}
+impl<TLogger: Logger> HasSourceMap for WrapFileLogger<'_, TLogger> {
+    fn source_map(&self) -> &SourceMap {
+        self.source_map
+    }
+}
+impl<TLogger: Logger> SrcFileLogger for WrapFileLogger<'_, TLogger> {}
diff --git a/crates/unused_finder/src/lib.rs b/crates/unused_finder/src/lib.rs
@@ -31,7 +31,7 @@ pub use tag::UsedTagEnum;
 pub use unused_finder::{UnusedFinder, UnusedFinderResult};
 
 pub fn find_unused_items(
-    logger: impl logger::Logger,
+    logger: impl logger::Logger + Sync,
     config: UnusedFinderJSONConfig,
 ) -> Result<UnusedFinderReport, js_err::JsErr> {
     let mut finder = UnusedFinder::new_from_json_config(&logger, config)

diff --git a/crates/unused_finder/src/unused_finder.rs b/crates/unused_finder/src/unused_finder.rs
@@ -158,14 +158,17 @@ fn resolver_for_packages(root_dir: PathBuf, packages: &RepoPackages) -> impl Res
 
 impl UnusedFinder {
     pub fn new_from_json_config(
-        logger: impl Logger,
+        logger: impl Logger + Sync,
         json_config: UnusedFinderJSONConfig,
     ) -> Result<Self, JsErr> {
         let config = UnusedFinderConfig::try_from(json_config).map_err(JsErr::invalid_arg)?;
         Self::new_from_cfg(logger, config).map_err(JsErr::generic_failure)
     }
 
-    pub fn new_from_cfg(logger: impl Logger, config: UnusedFinderConfig) -> Result<Self, JsErr> {
+    pub fn new_from_cfg(
+        logger: impl Logger + Sync,
+        config: UnusedFinderConfig,
+    ) -> Result<Self, JsErr> {
         if config.repo_root.is_empty() {
             return Err(JsErr::invalid_arg(anyhow!(
                 "repoRoot must be set in config"
@@ -215,7 +218,7 @@ impl UnusedFinder {
 
     // Helper method used before taking a snapshot of the file tree for graph computation.
     // performs either a partial or full refresh of the file tree, depending on the value of `files_to_check`
-    fn update_dirty_files(&mut self, logger: impl Logger) -> Result<(), JsErr> {
+    fn update_dirty_files(&mut self, logger: impl Logger + Sync) -> Result<(), JsErr> {
         match &self.dirty_files {
             DirtyFiles::All => {
                 logger.log("Refreshing all files");
@@ -230,7 +233,7 @@ impl UnusedFinder {
                 let scanned_files = files
                     .par_iter()
                     .map(|file_path| {
-                        self.update_single_file(file_path, logger.clone())
+                        self.update_single_file(file_path, &logger)
                             .map_err(JsErr::generic_failure)
                     })
                     .collect::<Result<Vec<_>, _>>()?;
@@ -296,7 +299,7 @@ impl UnusedFinder {
     /// Walks and parses all source files in the repo, returning a WalkFileResult
     /// with
     fn walk_and_resolve_all(
-        logger: impl Logger,
+        logger: impl Logger + Sync,
         config: &UnusedFinderConfig,
     ) -> Result<SourceFiles, JsErr> {
         // Note: this silently ignores any errors that occur during the walk
@@ -320,7 +323,7 @@ impl UnusedFinder {
 
     // Gets a report by performing a graph traversal on the current in-memory state of the repo,
     // from the last time the file tree was scanned.
-    pub fn find_unused(&mut self, logger: impl Logger) -> Result<UnusedFinderResult, JsErr> {
+    pub fn find_unused(&mut self, logger: impl Logger + Sync) -> Result<UnusedFinderResult, JsErr> {
         // Scan the file-system for changed files
         self.update_dirty_files(&logger)?;
 
@@ -394,7 +397,7 @@ impl UnusedFinder {
 
     /// helper to get the list of files that are "entrypoints" to the used
     /// symbol graph (ignored files)
-    fn get_entrypoints(&self, logger: impl Logger) -> Vec<&Path> {
+    fn get_entrypoints(&self, logger: impl Logger + Sync) -> Vec<&Path> {
         // get all package exports.
         self.last_walk_result
             .source_files

diff --git a/crates/unused_finder/src/walk.rs b/crates/unused_finder/src/walk.rs
@@ -136,7 +136,7 @@ pub struct WalkedFiles {
 
 /// Walks the root paths of a project and returns a list of source files and packages
 pub fn walk_src_files(
-    logger: impl Logger,
+    logger: impl Logger + Sync,
     root_paths: &[impl AsRef<Path> + Debug],
     repo_root_path: impl AsRef<Path>,
     ignored_filenames: &[impl AsRef<str> + Debug],
@@ -150,15 +150,14 @@ pub fn walk_src_files(
     // safely borrow &Logger, because the scope guarantees the threads
     // will be joined after the scope ends.
     std::thread::scope(|scope| -> Result<(), anyhow::Error> {
-        let logger_clone = logger.clone();
-        let collector_thread = scope.spawn(move || {
+        let collector_thread = scope.spawn(|| {
             for file in rx {
                 match file {
                     Ok(file) => {
                         all_walked_ref.push(file);
                     }
                     Err(e) => {
-                        logger_clone.log(format!("Error during walk: {:?}", e));
+                        logger.log(format!("Error during walk: {:?}", e));
                     }
                 }
             }