fix(linter): delay merging of oxlintrc configs

Author: camc314

apps/oxlint/fixtures/overrides_with_plugin/.oxlintrc.json

@@ -0,0 +1,13 @@
+{
+    "overrides": [
+      {
+        "files": ["*.test.ts"],
+        "plugins": ["jest", "vitest"],
+        "rules": {
+          "jest/valid-title": "deny",
+          "no-unused-vars": "off"
+        }
+      }
+    ]
+  }
+  

apps/oxlint/fixtures/overrides_with_plugin/index.test.ts

@@ -0,0 +1,7 @@
+describe("", () => {
+  //
+
+  it("", () => {});
+});
+
+const foo = 123;

apps/oxlint/fixtures/overrides_with_plugin/index.ts

@@ -0,0 +1 @@
+const foo = 123;

apps/oxlint/src/lint.rs

@@ -180,7 +180,11 @@ impl Runner for LintRunner {
             FxHashMap::default()
         };
 
-        enable_plugins.apply_overrides(&mut oxlintrc.plugins);
+        {
+            let mut plugins = oxlintrc.plugins.unwrap_or_default();
+            enable_plugins.apply_overrides(&mut plugins);
+            oxlintrc.plugins = Some(plugins);
+        }
 
         let oxlintrc_for_print = if misc_options.print_config || basic_options.init {
             Some(oxlintrc.clone())
@@ -1153,4 +1157,10 @@ mod test {
         let args = &["--import-plugin", "-D", "import/no-cycle"];
         Tester::new().with_cwd("fixtures/import-cycle".into()).test_and_snapshot(args);
     }
+
+    #[test]
+    fn test_plugins_in_overrides_enabled_correctly() {
+        let args = &["-c", ".oxlintrc.json"];
+        Tester::new().with_cwd("fixtures/overrides_with_plugin".into()).test_and_snapshot(args);
+    }
 }

apps/oxlint/src/snapshots/fixtures__extends_config_--config relative_paths__extends_extends_config.json console.js@oxlint.snap

@@ -14,7 +14,7 @@ working directory: fixtures/extends_config
   help: Delete this console statement.
 
 Found 0 warnings and 1 error.
-Finished in <variable>ms on 1 file with 103 rules using 1 threads.
+Finished in <variable>ms on 1 file with 102 rules using 1 threads.
 ----------
 CLI result: LintFoundErrors
 ----------

apps/oxlint/src/snapshots/fixtures__extends_config_overrides@oxlint.snap

@@ -1,6 +1,5 @@
 ---
 source: apps/oxlint/src/tester.rs
-snapshot_kind: text
 ---
 ########## 
 arguments: overrides
@@ -31,7 +30,16 @@ working directory: fixtures/extends_config
    `----
   help: Avoid using ambiguous text like "click here", replace it with more descriptive text that provides context.
 
-Found 0 warnings and 3 errors.
+  ! ]8;;https://oxc.rs/docs/guide/usage/linter/rules/jsx_a11y/anchor-is-valid.html\eslint-plugin-jsx-a11y(anchor-is-valid)]8;;\: Missing `href` attribute for the `a` element.
+   ,-[overrides/test.tsx:2:11]
+ 1 | function component(): any {
+ 2 |   return <a>click here</a>;
+   :           ^
+ 3 | }
+   `----
+  help: Provide an `href` for the `a` element.
+
+Found 1 warning and 3 errors.
 Finished in <variable>ms on 2 files using 1 threads.
 ----------
 CLI result: LintFoundErrors

apps/oxlint/src/snapshots/fixtures__overrides_with_plugin_-c .oxlintrc.json@oxlint.snap

@@ -0,0 +1,38 @@
+---
+source: apps/oxlint/src/tester.rs
+---
+########## 
+arguments: -c .oxlintrc.json
+working directory: fixtures/overrides_with_plugin
+----------
+
+  x ]8;;https://oxc.rs/docs/guide/usage/linter/rules/jest/valid-title.html\eslint-plugin-jest(valid-title)]8;;\: "Should not have an empty title"
+   ,-[index.test.ts:1:10]
+ 1 | describe("", () => {
+   :          ^^
+ 2 |   //
+   `----
+  help: "Write a meaningful title for your test"
+
+  ! ]8;;https://oxc.rs/docs/guide/usage/linter/rules/eslint/no-unused-vars.html\eslint(no-unused-vars)]8;;\: Variable 'foo' is declared but never used. Unused variables should start with a '_'.
+   ,-[index.ts:1:7]
+ 1 | const foo = 123;
+   :       ^|^
+   :        `-- 'foo' is declared here
+   `----
+  help: Consider removing this declaration.
+
+  x ]8;;https://oxc.rs/docs/guide/usage/linter/rules/jest/valid-title.html\eslint-plugin-jest(valid-title)]8;;\: "Should not have an empty title"
+   ,-[index.test.ts:4:6]
+ 3 | 
+ 4 |   it("", () => {});
+   :      ^^
+ 5 | });
+   `----
+  help: "Write a meaningful title for your test"
+
+Found 1 warning and 2 errors.
+Finished in <variable>ms on 2 files with 101 rules using 1 threads.
+----------
+CLI result: LintFoundErrors
+----------

crates/oxc_linter/src/config/categories.rs

@@ -1,4 +1,7 @@
-use std::{borrow::Cow, ops::Deref};
+use std::{
+    borrow::Cow,
+    ops::{Deref, DerefMut},
+};
 
 use rustc_hash::FxHashMap;
 use schemars::JsonSchema;
@@ -18,6 +21,12 @@ impl Deref for OxlintCategories {
     }
 }
 
+impl DerefMut for OxlintCategories {
+    fn deref_mut(&mut self) -> &mut Self::Target {
+        &mut self.0
+    }
+}
+
 impl OxlintCategories {
     pub fn filters(&self) -> impl Iterator<Item = LintFilter> + '_ {
         self.iter().map(|(category, severity)| LintFilter::new(*severity, *category).unwrap())

crates/oxc_linter/src/config/config_builder.rs

@@ -87,99 +87,70 @@ impl ConfigStoreBuilder {
         start_empty: bool,
         oxlintrc: Oxlintrc,
     ) -> Result<Self, ConfigBuilderError> {
-        // TODO(refactor); can we make this function infallible, and move all the error handling to
-        // the `build` method?
-        let Oxlintrc {
-            plugins,
-            settings,
-            env,
-            globals,
-            categories,
-            rules: oxlintrc_rules,
-            overrides,
-            path,
-            ignore_patterns: _,
-            extends,
-        } = oxlintrc;
-
-        let config = LintConfig { plugins, settings, env, globals, path: Some(path) };
-        let rules =
-            if start_empty { FxHashSet::default() } else { Self::warn_correctness(plugins) };
-        let cache = RulesCache::new(config.plugins);
-        let mut builder = Self { rules, config, overrides, cache };
+        // TODO: this can be cached to avoid re-conputing the same oxlintrc
+        fn resolve_oxlintrc_config(config: Oxlintrc) -> Result<Oxlintrc, ConfigBuilderError> {
+            let path = config.path.clone();
+            let root_path = path.parent();
+            let extends = config.extends.clone();
+
+            let mut oxlintrc = config;
+
+            for path in extends.iter().rev() {
+                if path.starts_with("eslint:") || path.starts_with("plugin:") {
+                    // `eslint:` and `plugin:` named configs are not supported
+                    continue;
+                }
+                // if path does not include a ".", then we will heuristically skip it since it
+                // kind of looks like it might be a named config
+                if !path.to_string_lossy().contains('.') {
+                    continue;
+                }
 
-        for filter in categories.filters() {
-            builder = builder.with_filter(&filter);
-        }
+                let path = match root_path {
+                    Some(p) => &p.join(path),
+                    None => path,
+                };
 
-        {
-            if !extends.is_empty() {
-                let config_path = builder.config.path.clone();
-                let config_path_parent = config_path.as_ref().and_then(|p| p.parent());
-
-                for path in &extends {
-                    if path.starts_with("eslint:") || path.starts_with("plugin:") {
-                        // eslint: and plugin: named configs are not supported
-                        continue;
-                    }
-                    // if path does not include a ".", then we will heuristically skip it since it
-                    // kind of looks like it might be a named config
-                    if !path.to_string_lossy().contains('.') {
-                        continue;
+                let extends_oxlintrc = Oxlintrc::from_file(path).map_err(|e| {
+                    ConfigBuilderError::InvalidConfigFile {
+                        file: path.display().to_string(),
+                        reason: e.to_string(),
                     }
+                })?;
 
-                    // resolve path relative to config path
-                    let path = match config_path_parent {
-                        Some(config_file_path) => &config_file_path.join(path),
-                        None => path,
-                    };
-                    // TODO: throw an error if this is a self-referential extend
-                    // TODO(perf): use a global config cache to avoid re-parsing the same file multiple times
-                    match Oxlintrc::from_file(path) {
-                        Ok(extended_config) => {
-                            // TODO(refactor): can we merge this together? seems redundant to use `override_rules` and then
-                            // use `ConfigStoreBuilder`, but we don't have a better way of loading rules from config files other than that.
-                            // Use `override_rules` to apply rule configurations and add/remove rules as needed
-                            extended_config
-                                .rules
-                                .override_rules(&mut builder.rules, &builder.cache.borrow());
-                            // Use `ConfigStoreBuilder` to load extended config files and then apply rules from those
-                            let mut extended_config_store =
-                                ConfigStoreBuilder::from_oxlintrc(true, extended_config)?;
-                            let rules = std::mem::take(&mut extended_config_store.rules);
-                            builder = builder.with_rules(rules);
-
-                            // Handle plugin inheritance
-                            let parent_plugins = extended_config_store.plugins();
-                            let child_plugins = builder.plugins();
-
-                            if child_plugins == LintPlugins::default() {
-                                // If child has default plugins, inherit from parent
-                                builder = builder.with_plugins(parent_plugins);
-                            } else if child_plugins != LintPlugins::empty() {
-                                // If child specifies plugins, combine with parent's plugins
-                                builder = builder.with_plugins(child_plugins.union(parent_plugins));
-                            }
-
-                            if !extended_config_store.overrides.is_empty() {
-                                let overrides =
-                                    std::mem::take(&mut extended_config_store.overrides);
-                                builder = builder.with_overrides(overrides);
-                            }
-                        }
-                        Err(err) => {
-                            return Err(ConfigBuilderError::InvalidConfigFile {
-                                file: path.display().to_string(),
-                                reason: err.to_string(),
-                            });
-                        }
-                    }
-                }
+                let extends = resolve_oxlintrc_config(extends_oxlintrc)?;
+
+                oxlintrc = oxlintrc.merge(extends);
             }
+            Ok(oxlintrc)
+        }
+
+        let oxlintrc = resolve_oxlintrc_config(oxlintrc)?;
 
+        let rules = if start_empty {
+            FxHashSet::default()
+        } else {
+            Self::warn_correctness(oxlintrc.plugins.unwrap_or_default())
+        };
+
+        let config = LintConfig {
+            plugins: oxlintrc.plugins.unwrap_or_default(),
+            settings: oxlintrc.settings,
+            env: oxlintrc.env,
+            globals: oxlintrc.globals,
+            path: Some(oxlintrc.path),
+        };
+        let cache = RulesCache::new(config.plugins);
+        let mut builder = Self { rules, config, overrides: oxlintrc.overrides, cache };
+
+        for filter in oxlintrc.categories.filters() {
+            builder = builder.with_filter(&filter);
+        }
+
+        {
             let all_rules = builder.cache.borrow();
 
-            oxlintrc_rules.override_rules(&mut builder.rules, all_rules.as_slice());
+            oxlintrc.rules.override_rules(&mut builder.rules, all_rules.as_slice());
         }
 
         Ok(builder)
@@ -227,11 +198,6 @@ impl ConfigStoreBuilder {
         self
     }
 
-    pub(crate) fn with_rules<R: IntoIterator<Item = RuleWithSeverity>>(mut self, rules: R) -> Self {
-        self.rules.extend(rules);
-        self
-    }
-
     /// Appends an override to the end of the current list of overrides.
     pub fn with_overrides<O: IntoIterator<Item = OxlintOverride>>(mut self, overrides: O) -> Self {
         self.overrides.extend(overrides);

crates/oxc_linter/src/config/config_store.rs

@@ -85,6 +85,13 @@ impl Config {
         let mut env = self.base.config.env.clone();
         let mut globals = self.base.config.globals.clone();
         let mut plugins = self.base.config.plugins;
+
+        for override_config in overrides_to_apply.clone() {
+            if let Some(override_plugins) = override_config.plugins {
+                plugins |= override_plugins;
+            }
+        }
+
         let mut rules = self
             .base
             .rules
@@ -104,10 +111,6 @@ impl Config {
                 override_config.rules.override_rules(&mut rules, &all_rules);
             }
 
-            if let Some(override_plugins) = override_config.plugins {
-                plugins |= override_plugins;
-            }
-
             if let Some(override_env) = &override_config.env {
                 override_env.override_envs(&mut env);
             }
@@ -136,11 +139,14 @@ impl Config {
     }
 }
 
-/// Resolves a lint configuration for a given file, by applying overrides based on the file's path.
+/// Stores the configuration state for the linter including:
+/// 1. the root configuration (base)
+/// 2. any nested configurations (`nested_configs`)
+///
+/// If an explicit config has been provided `-c config.json`, then `nested_configs` will be empty
 #[derive(Debug, Clone)]
 pub struct ConfigStore {
     base: Config,
-
     nested_configs: FxHashMap<PathBuf, Config>,
 }