refactor(oxfmt): Refactor walk.rs and format.rs relationship (#14795)

Also a pure refactor, but using `filter_entry()` theoretically improves performance.

Author: leaysgur

apps/oxfmt/src/format.rs

@@ -6,8 +6,6 @@ use std::{
     time::Instant,
 };
 
-use ignore::overrides::OverrideBuilder;
-
 use oxc_diagnostics::DiagnosticService;
 use oxc_formatter::{FormatOptions, Oxfmtrc};
 
@@ -62,22 +60,16 @@ impl FormatRunner {
             }
         };
 
-        // Normalize user input paths
-        let (target_paths, exclude_patterns) = normalize_paths(&cwd, &paths);
-
-        // Build exclude patterns if any exist
-        let override_builder = (!exclude_patterns.is_empty())
-            .then(|| {
-                let mut builder = OverrideBuilder::new(&cwd);
-                for pattern_str in exclude_patterns {
-                    builder.add(&pattern_str).ok()?;
-                }
-                builder.build().ok()
-            })
-            .flatten();
-
-        // TODO: Support ignoring files
-        let walker = Walk::new(&target_paths, override_builder, ignore_options.with_node_modules);
+        let walker = match Walk::build(&cwd, &paths, ignore_options.with_node_modules) {
+            Ok(walker) => walker,
+            Err(err) => {
+                print_and_flush_stdout(
+                    stdout,
+                    &format!("Failed to parse target paths or ignore settings.\n{err}\n"),
+                );
+                return CliRunResult::InvalidOptionConfig;
+            }
+        };
 
         // Get the receiver for streaming entries
         let rx_entry = walker.stream_entries();
@@ -169,81 +161,32 @@ impl FormatRunner {
     }
 }
 
-const DEFAULT_OXFMTRC: &str = ".oxfmtrc.json";
-
 /// # Errors
 ///
 /// Returns error if:
 /// - Config file is specified but not found or invalid
 /// - Config file parsing fails
-fn load_config(cwd: &Path, config: Option<&PathBuf>) -> Result<FormatOptions, String> {
-    // If `--config` is explicitly specified, use that path directly
-    if let Some(config_path) = config {
-        let full_path = if config_path.is_absolute() {
+fn load_config(cwd: &Path, config_path: Option<&PathBuf>) -> Result<FormatOptions, String> {
+    let config_path = if let Some(config_path) = config_path {
+        // If `--config` is explicitly specified, use that path
+        Some(if config_path.is_absolute() {
             PathBuf::from(config_path)
         } else {
             cwd.join(config_path)
-        };
-
-        // This will error if the file does not exist or is invalid
-        let oxfmtrc = Oxfmtrc::from_file(&full_path)?;
-        return oxfmtrc.into_format_options();
-    }
-
-    // If `--config` is not specified, search the nearest config file from cwd upwards
-    for dir in cwd.ancestors() {
-        let config_path = dir.join(DEFAULT_OXFMTRC);
-        if config_path.exists() {
-            let oxfmtrc = Oxfmtrc::from_file(&config_path)?;
-            return oxfmtrc.into_format_options();
-        }
+        })
+    } else {
+        // If `--config` is not specified, search the nearest config file from cwd upwards
+        cwd.ancestors().find_map(|dir| {
+            let config_path = dir.join(".oxfmtrc.json");
+            if config_path.exists() { Some(config_path) } else { None }
+        })
+    };
+
+    match config_path {
+        Some(ref path) => Oxfmtrc::from_file(path)?.into_format_options(),
+        // Default if not specified and not found
+        None => Ok(FormatOptions::default()),
     }
-
-    // No config file found, use defaults
-    Ok(FormatOptions::default())
-}
-
-/// Normalize user input paths into `target_paths` and `exclude_patterns`.
-/// - `target_paths`: Absolute paths to format
-/// - `exclude_patterns`: Pattern strings to exclude (with `!` prefix)
-fn normalize_paths(cwd: &Path, input_paths: &[PathBuf]) -> (Vec<PathBuf>, Vec<String>) {
-    let mut target_paths = vec![];
-    let mut exclude_patterns = vec![];
-
-    for path in input_paths {
-        let path_str = path.to_string_lossy();
-
-        // Instead of `oxlint`'s `--ignore-pattern=PAT`,
-        // `oxfmt` supports `!` prefix in paths like Prettier.
-        if path_str.starts_with('!') {
-            exclude_patterns.push(path_str.to_string());
-            continue;
-        }
-
-        // Otherwise, treat as target path
-
-        if path.is_absolute() {
-            target_paths.push(path.clone());
-            continue;
-        }
-
-        // NOTE: `.` and cwd behaves differently, need to normalize
-        let path = if path_str == "." {
-            cwd.to_path_buf()
-        } else if let Some(stripped) = path_str.strip_prefix("./") {
-            cwd.join(stripped)
-        } else {
-            cwd.join(path)
-        };
-        target_paths.push(path);
-    }
-
-    // Default to cwd if no `target_paths` are provided
-    if target_paths.is_empty() {
-        target_paths.push(cwd.into());
-    }
-
-    (target_paths, exclude_patterns)
 }
 
 fn print_and_flush_stdout(stdout: &mut dyn Write, message: &str) {

apps/oxfmt/src/walk.rs

@@ -1,54 +1,109 @@
-use std::{ffi::OsStr, path::PathBuf, sync::mpsc};
+use std::{
+    path::{Path, PathBuf},
+    sync::mpsc,
+};
 
-use ignore::overrides::Override;
+use ignore::overrides::OverrideBuilder;
 
 use oxc_formatter::get_supported_source_type;
 use oxc_span::SourceType;
 
 pub struct Walk {
     inner: ignore::WalkParallel,
-    with_node_modules: bool,
 }
 
 impl Walk {
-    /// Will not canonicalize paths.
-    /// # Panics
-    pub fn new(
+    pub fn build(
+        cwd: &PathBuf,
         paths: &[PathBuf],
-        override_builder: Option<Override>,
         with_node_modules: bool,
-    ) -> Self {
+    ) -> Result<Self, String> {
+        let (target_paths, exclude_patterns) = normalize_paths(cwd, paths);
+
+        // Add all non-`!` prefixed paths to the walker base
         let mut inner = ignore::WalkBuilder::new(
-            paths
-                .iter()
-                .next()
-                .expect("Expected paths parameter to Walk::new() to contain at least one path."),
+            target_paths
+                .first()
+                .expect("Expected paths parameter to Walk::build() to contain at least one path."),
         );
-
-        if let Some(paths) = paths.get(1..) {
+        if let Some(paths) = target_paths.get(1..) {
             for path in paths {
                 inner.add(path);
             }
         }
 
-        if let Some(override_builder) = override_builder {
-            inner.overrides(override_builder);
+        // Treat all `!` prefixed patterns as overrides to exclude
+        if !exclude_patterns.is_empty() {
+            let mut builder = OverrideBuilder::new(cwd);
+            for pattern_str in exclude_patterns {
+                builder
+                    .add(&pattern_str)
+                    .map_err(|_| format!("{pattern_str} is not a valid glob for override."))?;
+            }
+            let overrides = builder.build().map_err(|_| "Failed to build overrides".to_string())?;
+            inner.overrides(overrides);
         }
 
-        // Do not follow symlinks like Prettier does.
-        // See https://github.com/prettier/prettier/pull/14627
-        let inner = inner.hidden(false).ignore(false).git_global(false).build_parallel();
-        Self { inner, with_node_modules }
+        // TODO: Support ignoring files
+        // --ignore-path PATH1 --ignore-path PATH2
+        // or default cwd/{.gitignore,.prettierignore}
+        // if let Some(err) = inner.add_ignore(path) {
+        //     return Err(format!("Failed to add ignore file: {}", err));
+        // }
+
+        // NOTE: If return `false` here, it will not be `visit()`ed at all
+        inner.filter_entry(move |entry| {
+            // Skip stdin for now
+            let Some(file_type) = entry.file_type() else {
+                return false;
+            };
+
+            if file_type.is_dir() {
+                // We are setting `.hidden(false)` on the `WalkBuilder` below,
+                // it means we want to include hidden files and directories.
+                // However, we (and also Prettier) still skip traversing certain directories.
+                // https://prettier.io/docs/ignore#ignoring-files-prettierignore
+                let is_skipped_dir = {
+                    let dir_name = entry.file_name();
+                    dir_name == ".git"
+                        || dir_name == ".jj"
+                        || dir_name == ".sl"
+                        || dir_name == ".svn"
+                        || dir_name == ".hg"
+                        || (!with_node_modules && dir_name == "node_modules")
+                };
+                if is_skipped_dir {
+                    return false;
+                }
+            }
+
+            // NOTE: We can also check `get_supported_source_type()` here to skip.
+            // But we want to pass parsed `SourceType` to `FormatService`,
+            // so we do it later in the visitor instead.
+
+            true
+        });
+
+        let inner = inner
+            // Do not follow symlinks like Prettier does.
+            // See https://github.com/prettier/prettier/pull/14627
+            .follow_links(false)
+            // Include hidden files and directories except those we explicitly skip above
+            .hidden(false)
+            // Do not respect `.gitignore` automatically, we handle it manually
+            .ignore(false)
+            .git_global(false)
+            .build_parallel();
+        Ok(Self { inner })
     }
 
     /// Stream entries through a channel as they are discovered
     pub fn stream_entries(self) -> mpsc::Receiver<WalkEntry> {
         let (sender, receiver) = mpsc::channel::<WalkEntry>();
-        let with_node_modules = self.with_node_modules;
 
         // Spawn the walk operation in a separate thread
         rayon::spawn(move || {
-            let mut builder = WalkBuilder { sender, with_node_modules };
+            let mut builder = WalkBuilder { sender };
             self.inner.visit(&mut builder);
             // Channel will be closed when builder is dropped
         });
@@ -57,43 +112,68 @@ impl Walk {
     }
 }
 
+/// Normalize user input paths into `target_paths` and `exclude_patterns`.
+/// - `target_paths`: Absolute paths to format
+/// - `exclude_patterns`: Pattern strings to exclude (with `!` prefix)
+fn normalize_paths(cwd: &Path, input_paths: &[PathBuf]) -> (Vec<PathBuf>, Vec<String>) {
+    let mut target_paths = vec![];
+    let mut exclude_patterns = vec![];
+
+    for path in input_paths {
+        let path_str = path.to_string_lossy();
+
+        // Instead of `oxlint`'s `--ignore-pattern=PAT`,
+        // `oxfmt` supports `!` prefix in paths like Prettier.
+        if path_str.starts_with('!') {
+            exclude_patterns.push(path_str.to_string());
+            continue;
+        }
+
+        // Otherwise, treat as target path
+
+        if path.is_absolute() {
+            target_paths.push(path.clone());
+            continue;
+        }
+
+        // NOTE: `.` and cwd behaves differently, need to normalize
+        let path = if path_str == "." {
+            cwd.to_path_buf()
+        } else if let Some(stripped) = path_str.strip_prefix("./") {
+            cwd.join(stripped)
+        } else {
+            cwd.join(path)
+        };
+        target_paths.push(path);
+    }
+
+    // Default to cwd if no `target_paths` are provided
+    if target_paths.is_empty() {
+        target_paths.push(cwd.into());
+    }
+
+    (target_paths, exclude_patterns)
+}
+
+// ---
+
 pub struct WalkEntry {
     pub path: PathBuf,
     pub source_type: SourceType,
 }
 
 struct WalkBuilder {
     sender: mpsc::Sender<WalkEntry>,
-    with_node_modules: bool,
 }
 
 impl<'s> ignore::ParallelVisitorBuilder<'s> for WalkBuilder {
     fn build(&mut self) -> Box<dyn ignore::ParallelVisitor + 's> {
-        Box::new(WalkVisitor {
-            sender: self.sender.clone(),
-            with_node_modules: self.with_node_modules,
-        })
+        Box::new(WalkVisitor { sender: self.sender.clone() })
     }
 }
 
 struct WalkVisitor {
     sender: mpsc::Sender<WalkEntry>,
-    with_node_modules: bool,
-}
-
-impl WalkVisitor {
-    // We are setting `.hidden(false)` on the `WalkBuilder`,
-    // it means we want to include hidden files and directories.
-    // However, we (and also Prettier) still skip traversing certain directories.
-    // https://prettier.io/docs/ignore#ignoring-files-prettierignore
-    fn is_skipped_dir(&self, dir_name: &OsStr) -> bool {
-        dir_name == ".git"
-            || dir_name == ".jj"
-            || dir_name == ".sl"
-            || dir_name == ".svn"
-            || dir_name == ".hg"
-            || (!self.with_node_modules && dir_name == "node_modules")
-    }
 }
 
 impl ignore::ParallelVisitor for WalkVisitor {
@@ -104,14 +184,9 @@ impl ignore::ParallelVisitor for WalkVisitor {
                     return ignore::WalkState::Continue;
                 };
 
-                if file_type.is_dir() {
-                    if self.is_skipped_dir(entry.file_name()) {
-                        return ignore::WalkState::Skip;
-                    }
-                    return ignore::WalkState::Continue;
-                }
-
-                if let Some(source_type) = get_supported_source_type(entry.path()) {
+                if !file_type.is_dir()
+                    && let Some(source_type) = get_supported_source_type(entry.path())
+                {
                     let walk_entry = WalkEntry { path: entry.path().to_path_buf(), source_type };
                     // Send each entry immediately through the channel
                     // If send fails, the receiver has been dropped, so stop walking