From 5b116103b7bd5d308efd575cf93d309559f1acd3 Mon Sep 17 00:00:00 2001 From: camc314 <18101008+camc314@users.noreply.github.com> Date: Thu, 29 Jan 2026 14:36:26 +0000 Subject: [PATCH] refactor(linter): extract config searching functions to their own file (#18673) move the funcitons that search for config files out of sever_linter.rs/lint.rs into config_loader so that they're all in the same place --- apps/oxlint/src/config_loader.rs | 117 ++++++++++++++++++++++++++- apps/oxlint/src/lint.rs | 53 +++--------- apps/oxlint/src/lsp/config_walker.rs | 87 -------------------- apps/oxlint/src/lsp/mod.rs | 1 - apps/oxlint/src/lsp/server_linter.rs | 6 +- 5 files changed, 131 insertions(+), 133 deletions(-) delete mode 100644 apps/oxlint/src/lsp/config_walker.rs diff --git a/apps/oxlint/src/config_loader.rs b/apps/oxlint/src/config_loader.rs index f30e565e035a1..0315c4004e9b5 100644 --- a/apps/oxlint/src/config_loader.rs +++ b/apps/oxlint/src/config_loader.rs @@ -1,9 +1,124 @@ -use std::path::{Path, PathBuf}; +use std::{ + ffi::OsStr, + path::{Path, PathBuf}, + sync::{Arc, mpsc}, +}; +use ignore::DirEntry; use oxc_diagnostics::OxcDiagnostic; use oxc_linter::{ Config, ConfigStoreBuilder, ExternalLinter, ExternalPluginStore, LintFilter, Oxlintrc, }; +use rustc_hash::FxHashSet; + +use crate::DEFAULT_OXLINTRC_NAME; + +/// Discover config files by walking UP from each file's directory to ancestors. +/// +/// Used by CLI where we have specific files to lint and need to find configs +/// that apply to them. +/// +/// Example: For files `/project/src/foo.js` and `/project/src/bar/baz.js`: +/// - Checks `/project/src/bar/`, `/project/src/`, `/project/`, `/` +/// - Returns paths to any `.oxlintrc.json` files found +pub fn discover_configs_in_ancestors>( + files: &[P], +) -> impl IntoIterator { + let mut config_paths = FxHashSet::::default(); + let mut visited_dirs = FxHashSet::default(); + + for file in files { + let path = file.as_ref(); + // Start from the file's parent directory and walk up the tree + let mut current = path.parent(); + while let Some(dir) = current { + // Stop if we've already checked this directory (and its ancestors) + let inserted = visited_dirs.insert(dir.to_path_buf()); + if !inserted { + break; + } + if let Some(config_path) = find_config_in_directory(dir) { + config_paths.insert(config_path); + } + current = dir.parent(); + } + } + + config_paths.into_iter() +} + +/// Discover config files by walking DOWN from a root directory. +/// +/// Used by LSP where we have a workspace root and need to discover all configs +/// upfront for file watching and diagnostics. +pub fn discover_configs_in_tree(root: &Path) -> impl IntoIterator { + let walker = ignore::WalkBuilder::new(root) + .hidden(false) // don't skip hidden files + .parents(false) // disable gitignore from parent dirs + .ignore(false) // disable .ignore files + .git_global(false) // disable global gitignore + .follow_links(true) + .build_parallel(); + + let (sender, receiver) = mpsc::channel::>>(); + let mut builder = ConfigWalkBuilder { sender }; + walker.visit(&mut builder); + drop(builder); + + receiver.into_iter().flatten().map(|p| PathBuf::from(p.as_ref())) +} + +/// Check if a directory contains an oxlint config file. +fn find_config_in_directory(dir: &Path) -> Option { + let config_path = dir.join(DEFAULT_OXLINTRC_NAME); + if config_path.is_file() { Some(config_path) } else { None } +} + +// Helper types for parallel directory walking +struct ConfigWalkBuilder { + sender: mpsc::Sender>>, +} + +impl<'s> ignore::ParallelVisitorBuilder<'s> for ConfigWalkBuilder { + fn build(&mut self) -> Box { + Box::new(ConfigWalkCollector { paths: vec![], sender: self.sender.clone() }) + } +} + +struct ConfigWalkCollector { + paths: Vec>, + sender: mpsc::Sender>>, +} + +impl Drop for ConfigWalkCollector { + fn drop(&mut self) { + let paths = std::mem::take(&mut self.paths); + self.sender.send(paths).unwrap(); + } +} + +impl ignore::ParallelVisitor for ConfigWalkCollector { + fn visit(&mut self, entry: Result) -> ignore::WalkState { + match entry { + Ok(entry) => { + if is_config_file(&entry) { + self.paths.push(entry.path().as_os_str().into()); + } + ignore::WalkState::Continue + } + Err(_) => ignore::WalkState::Skip, + } + } +} + +fn is_config_file(entry: &DirEntry) -> bool { + let Some(file_type) = entry.file_type() else { return false }; + if file_type.is_dir() { + return false; + } + let Some(file_name) = entry.path().file_name() else { return false }; + file_name == DEFAULT_OXLINTRC_NAME +} pub struct LoadedConfig { /// The directory this config applies to diff --git a/apps/oxlint/src/lint.rs b/apps/oxlint/src/lint.rs index 549c81ab4caf0..53f3c1b8d3691 100644 --- a/apps/oxlint/src/lint.rs +++ b/apps/oxlint/src/lint.rs @@ -9,7 +9,7 @@ use std::{ use cow_utils::CowUtils; use ignore::{gitignore::Gitignore, overrides::OverrideBuilder}; -use rustc_hash::{FxBuildHasher, FxHashMap, FxHashSet}; +use rustc_hash::{FxBuildHasher, FxHashMap}; use oxc_diagnostics::{DiagnosticSender, DiagnosticService, GraphicalReportHandler, OxcDiagnostic}; use oxc_linter::{ @@ -20,7 +20,7 @@ use oxc_linter::{ use crate::{ DEFAULT_OXLINTRC_NAME, cli::{CliRunResult, LintCommand, MiscOptions, ReportUnusedDirectives, WarningOptions}, - config_loader::{ConfigLoadError, ConfigLoader}, + config_loader::{ConfigLoadError, ConfigLoader, discover_configs_in_ancestors}, output_formatter::{LintCommandInfo, OutputFormatter}, walk::Walk, }; @@ -474,40 +474,18 @@ impl CliRunner { external_plugin_store: &mut ExternalPluginStore, nested_ignore_patterns: &mut Vec<(Vec, PathBuf)>, ) -> Result, CliRunResult> { - // TODO(perf): benchmark whether or not it is worth it to store the configurations on a - // per-file or per-directory basis, to avoid calling `.parent()` on every path. - let mut nested_oxlintrc = FxHashSet::::default(); - // get all of the unique directories among the paths to use for search for - // oxlint config files in those directories and their ancestors - // e.g. `/some/file.js` will check `/some` and `/` - // `/some/other/file.js` will check `/some/other`, `/some`, and `/` - let mut directories = FxHashSet::default(); - for path in paths { - let path = Path::new(path); - // Start from the file's parent directory and walk up the tree - let mut current = path.parent(); - while let Some(dir) = current { - // NOTE: Initial benchmarking showed that it was faster to iterate over the directories twice - // rather than constructing the configs in one iteration. It's worth re-benchmarking that though. - let inserted = directories.insert(dir); - if !inserted { - break; - } - current = dir.parent(); - } - } - for directory in directories { - if let Some(path) = Self::find_oxlint_config_path_in_directory(directory) { - nested_oxlintrc.insert(path); - } - } + // Discover config files by walking up from each file's directory + let config_paths: Vec<_> = + paths.iter().map(|p| Path::new(p.as_ref()).to_path_buf()).collect(); + let discovered_configs = discover_configs_in_ancestors(&config_paths); // Load all discovered configs let mut loader = ConfigLoader::new(external_linter, external_plugin_store, filters); - let (configs, errors) = loader.load_many(nested_oxlintrc); + let (configs, errors) = loader.load_many(discovered_configs); - if let Some(error) = errors.first() { - let message = match error { + // Fail on first error (CLI requires all configs to be valid) + if let Some(error) = errors.into_iter().next() { + let message = match &error { ConfigLoadError::Parse { path, error } => { format!( "Failed to parse oxlint configuration file at {}.\n{}\n", @@ -517,17 +495,17 @@ impl CliRunner { } ConfigLoadError::Build { path, error } => { format!( - "Failed to build oxlint configuration file at {}.\n{}\n", + "Failed to build configuration from {}.\n{}\n", path.to_string_lossy().cow_replace('\\', "/"), render_report(handler, &OxcDiagnostic::error(error.clone())) ) } }; - print_and_flush_stdout(stdout, &message); return Err(CliRunResult::InvalidOptionConfig); } + // Convert loaded configs to nested config format let mut nested_configs = FxHashMap::::with_capacity_and_hasher(configs.len(), FxBuildHasher); for loaded in configs { @@ -551,13 +529,6 @@ impl CliRunner { } Ok(Oxlintrc::default()) } - - /// Looks in a directory for an oxlint config file and returns the path if it exists. - /// Does not validate the file or apply the default config file. - fn find_oxlint_config_path_in_directory(dir: &Path) -> Option { - let possible_config_path = dir.join(DEFAULT_OXLINTRC_NAME); - if possible_config_path.is_file() { Some(possible_config_path) } else { None } - } } pub fn print_and_flush_stdout(stdout: &mut dyn Write, message: &str) { diff --git a/apps/oxlint/src/lsp/config_walker.rs b/apps/oxlint/src/lsp/config_walker.rs deleted file mode 100644 index fd14f6033ef8e..0000000000000 --- a/apps/oxlint/src/lsp/config_walker.rs +++ /dev/null @@ -1,87 +0,0 @@ -use std::{ - ffi::OsStr, - path::Path, - sync::{Arc, mpsc}, -}; - -use ignore::DirEntry; - -use crate::DEFAULT_OXLINTRC_NAME; - -pub struct ConfigWalker { - inner: ignore::WalkParallel, -} - -struct WalkBuilder { - sender: mpsc::Sender>>, -} - -impl<'s> ignore::ParallelVisitorBuilder<'s> for WalkBuilder { - fn build(&mut self) -> Box { - Box::new(WalkCollector { paths: vec![], sender: self.sender.clone() }) - } -} - -struct WalkCollector { - paths: Vec>, - sender: mpsc::Sender>>, -} - -impl Drop for WalkCollector { - fn drop(&mut self) { - let paths = std::mem::take(&mut self.paths); - self.sender.send(paths).unwrap(); - } -} - -impl ignore::ParallelVisitor for WalkCollector { - fn visit(&mut self, entry: Result) -> ignore::WalkState { - match entry { - Ok(entry) => { - if Self::is_wanted_entry(&entry) { - self.paths.push(entry.path().as_os_str().into()); - } - ignore::WalkState::Continue - } - Err(_err) => ignore::WalkState::Skip, - } - } -} - -impl WalkCollector { - fn is_wanted_entry(entry: &DirEntry) -> bool { - let Some(file_type) = entry.file_type() else { return false }; - if file_type.is_dir() { - return false; - } - let Some(file_name) = entry.path().file_name() else { return false }; - - file_name == DEFAULT_OXLINTRC_NAME - } -} - -impl ConfigWalker { - /// Will not canonicalize paths. - /// # Panics - pub fn new(path: &Path) -> Self { - let inner: ignore::WalkParallel = ignore::WalkBuilder::new(path) - // disable skip hidden, which will not not search for files starting with a dot - .hidden(false) - // disable all gitignore features - .parents(false) - .ignore(false) - .git_global(false) - .follow_links(true) - .build_parallel(); - - Self { inner } - } - - pub fn paths(self) -> Vec> { - let (sender, receiver) = mpsc::channel::>>(); - let mut builder = WalkBuilder { sender }; - self.inner.visit(&mut builder); - drop(builder); - receiver.into_iter().flatten().collect() - } -} diff --git a/apps/oxlint/src/lsp/mod.rs b/apps/oxlint/src/lsp/mod.rs index 8c710fc6f3d81..d52e777f85b3b 100644 --- a/apps/oxlint/src/lsp/mod.rs +++ b/apps/oxlint/src/lsp/mod.rs @@ -1,6 +1,5 @@ mod code_actions; mod commands; -mod config_walker; mod error_with_position; mod lsp_file_system; mod options; diff --git a/apps/oxlint/src/lsp/server_linter.rs b/apps/oxlint/src/lsp/server_linter.rs index 70a85a900037b..571c6d2fad496 100644 --- a/apps/oxlint/src/lsp/server_linter.rs +++ b/apps/oxlint/src/lsp/server_linter.rs @@ -29,13 +29,13 @@ use oxc_language_server::{ use crate::{ DEFAULT_OXLINTRC_NAME, config_loader::ConfigLoader, + config_loader::discover_configs_in_tree, lsp::{ code_actions::{ CODE_ACTION_KIND_SOURCE_FIX_ALL_OXC, apply_all_fix_code_action, apply_fix_code_actions, fix_all_text_edit, }, commands::{FIX_ALL_COMMAND_ID, FixAllCommandArgs}, - config_walker::ConfigWalker, error_with_position::{ DiagnosticReport, LinterCodeAction, create_unused_directives_messages, generate_inverted_diagnostics, message_to_lsp_diagnostic, @@ -276,10 +276,10 @@ impl ServerLinterBuilder { nested_ignore_patterns: &mut Vec<(Vec, PathBuf)>, extended_paths: &mut FxHashSet, ) -> FxHashMap { - let paths = ConfigWalker::new(root_path).paths(); + let config_paths = discover_configs_in_tree(root_path); let mut loader = ConfigLoader::new(external_linter, external_plugin_store, &[]); - let (configs, errors) = loader.load_many(paths.iter().map(Path::new)); + let (configs, errors) = loader.load_many(config_paths); for error in errors { warn!("Skipping config file {}: {:?}", error.path().display(), error);