From 10d35e1e9774f07048fc1a58a3d2aeeb3eb08d5c Mon Sep 17 00:00:00 2001 From: Cameron Clark Date: Wed, 28 May 2025 17:01:40 +0100 Subject: [PATCH] fix(linter): correctly inherit categories when plugins are enabled --- .../fixtures/issue_10394/.oxlintrc.json | 9 ++ apps/oxlint/fixtures/issue_10394/foo.test.ts | 3 + .../overrides_with_plugin/index.test.ts | 5 +- .../fixtures/overrides_with_plugin/index.ts | 1 + apps/oxlint/src/lint.rs | 6 ++ ..._issue_10394_-c .oxlintrc.json@oxlint.snap | 21 +++++ ..._with_plugin_-c .oxlintrc.json@oxlint.snap | 16 +++- .../oxc_linter/src/config/config_builder.rs | 32 +++++-- crates/oxc_linter/src/config/config_store.rs | 87 ++++++++++++++----- crates/oxc_linter/src/config/rules.rs | 26 ++++-- crates/oxc_linter/src/tester.rs | 31 ++----- 11 files changed, 176 insertions(+), 61 deletions(-) create mode 100644 apps/oxlint/fixtures/issue_10394/.oxlintrc.json create mode 100644 apps/oxlint/fixtures/issue_10394/foo.test.ts create mode 100644 apps/oxlint/src/snapshots/fixtures__issue_10394_-c .oxlintrc.json@oxlint.snap diff --git a/apps/oxlint/fixtures/issue_10394/.oxlintrc.json b/apps/oxlint/fixtures/issue_10394/.oxlintrc.json new file mode 100644 index 0000000000000..35e0959027184 --- /dev/null +++ b/apps/oxlint/fixtures/issue_10394/.oxlintrc.json @@ -0,0 +1,9 @@ +{ + "overrides": [ + { + "plugins": ["jest", "vitest"], + "files": ["**/*.test.ts"], + "rules": {} + } + ] +} diff --git a/apps/oxlint/fixtures/issue_10394/foo.test.ts b/apps/oxlint/fixtures/issue_10394/foo.test.ts new file mode 100644 index 0000000000000..d6b4696f4f4bb --- /dev/null +++ b/apps/oxlint/fixtures/issue_10394/foo.test.ts @@ -0,0 +1,3 @@ +describe("", () => { + // +}); diff --git a/apps/oxlint/fixtures/overrides_with_plugin/index.test.ts b/apps/oxlint/fixtures/overrides_with_plugin/index.test.ts index b8a63407093bd..e6058a6427796 100644 --- a/apps/oxlint/fixtures/overrides_with_plugin/index.test.ts +++ b/apps/oxlint/fixtures/overrides_with_plugin/index.test.ts @@ -1,7 +1,10 @@ describe("", () => { - // + // ^ jest/no-valid-title error as explicitly set in the `.test.ts` override it("", () => {}); + // ^ jest/no-valid-title error as explicitly set in the `.test.ts` override + // ^ jest/expect-expect as `jest` plugin is enabled and `jest/expect-expect` is a correctness rule }); const foo = 123; +// no no-unused-vars error as override disables this rule for `.test.ts` files diff --git a/apps/oxlint/fixtures/overrides_with_plugin/index.ts b/apps/oxlint/fixtures/overrides_with_plugin/index.ts index 472e4584238a4..493d707cf45f6 100644 --- a/apps/oxlint/fixtures/overrides_with_plugin/index.ts +++ b/apps/oxlint/fixtures/overrides_with_plugin/index.ts @@ -1 +1,2 @@ const foo = 123; +// no-unused-vars error expected as `eslint` plugin and `correctness` categories are on by default (override is not applied.) diff --git a/apps/oxlint/src/lint.rs b/apps/oxlint/src/lint.rs index 1f54645da0d4e..e0e1010e2e341 100644 --- a/apps/oxlint/src/lint.rs +++ b/apps/oxlint/src/lint.rs @@ -1150,4 +1150,10 @@ mod test { let args = &["-c", ".oxlintrc.json"]; Tester::new().with_cwd("fixtures/overrides_with_plugin".into()).test_and_snapshot(args); } + + #[test] + fn test_plugins_inside_overrides_categories_enabled_correctly() { + let args = &["-c", ".oxlintrc.json"]; + Tester::new().with_cwd("fixtures/issue_10394".into()).test_and_snapshot(args); + } } diff --git a/apps/oxlint/src/snapshots/fixtures__issue_10394_-c .oxlintrc.json@oxlint.snap b/apps/oxlint/src/snapshots/fixtures__issue_10394_-c .oxlintrc.json@oxlint.snap new file mode 100644 index 0000000000000..3005b6f9ec811 --- /dev/null +++ b/apps/oxlint/src/snapshots/fixtures__issue_10394_-c .oxlintrc.json@oxlint.snap @@ -0,0 +1,21 @@ +--- +source: apps/oxlint/src/tester.rs +--- +########## +arguments: -c .oxlintrc.json +working directory: fixtures/issue_10394 +---------- + + ! ]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" + ,-[foo.test.ts:1:10] + 1 | describe("", () => { + : ^^ + 2 | // + `---- + help: "Write a meaningful title for your test" + +Found 1 warning and 0 errors. +Finished in ms on 1 file with 87 rules using 1 threads. +---------- +CLI result: LintSucceeded +---------- diff --git a/apps/oxlint/src/snapshots/fixtures__overrides_with_plugin_-c .oxlintrc.json@oxlint.snap b/apps/oxlint/src/snapshots/fixtures__overrides_with_plugin_-c .oxlintrc.json@oxlint.snap index ed2f2165de86f..b2bd915f423fa 100644 --- a/apps/oxlint/src/snapshots/fixtures__overrides_with_plugin_-c .oxlintrc.json@oxlint.snap +++ b/apps/oxlint/src/snapshots/fixtures__overrides_with_plugin_-c .oxlintrc.json@oxlint.snap @@ -10,7 +10,7 @@ working directory: fixtures/overrides_with_plugin ,-[index.test.ts:1:10] 1 | describe("", () => { : ^^ - 2 | // + 2 | // ^ jest/no-valid-title error as explicitly set in the `.test.ts` override `---- help: "Write a meaningful title for your test" @@ -19,6 +19,7 @@ working directory: fixtures/overrides_with_plugin 1 | const foo = 123; : ^|^ : `-- 'foo' is declared here + 2 | // no-unused-vars error expected as `eslint` plugin and `correctness` categories are on by default (override is not applied.) `---- help: Consider removing this declaration. @@ -27,11 +28,20 @@ working directory: fixtures/overrides_with_plugin 3 | 4 | it("", () => {}); : ^^ - 5 | }); + 5 | // ^ jest/no-valid-title error as explicitly set in the `.test.ts` override `---- help: "Write a meaningful title for your test" -Found 1 warning and 2 errors. + ! ]8;;https://oxc.rs/docs/guide/usage/linter/rules/jest/expect-expect.html\eslint-plugin-jest(expect-expect)]8;;\: Test has no assertions + ,-[index.test.ts:4:3] + 3 | + 4 | it("", () => {}); + : ^^ + 5 | // ^ jest/no-valid-title error as explicitly set in the `.test.ts` override + `---- + help: Add assertion(s) in this Test + +Found 2 warnings and 2 errors. Finished in ms on 2 files with 87 rules using 1 threads. ---------- CLI result: LintFoundErrors diff --git a/crates/oxc_linter/src/config/config_builder.rs b/crates/oxc_linter/src/config/config_builder.rs index 68c268b8c6fa4..f4914186d956b 100644 --- a/crates/oxc_linter/src/config/config_builder.rs +++ b/crates/oxc_linter/src/config/config_builder.rs @@ -15,12 +15,13 @@ use crate::{ rules::RULES, }; -use super::Config; +use super::{Config, categories::OxlintCategories}; #[must_use = "You dropped your builder without building a Linter! Did you mean to call .build()?"] pub struct ConfigStoreBuilder { pub(super) rules: FxHashMap, config: LintConfig, + categories: OxlintCategories, overrides: OxlintOverrides, cache: RulesCache, @@ -43,11 +44,12 @@ impl ConfigStoreBuilder { pub fn empty() -> Self { let config = LintConfig::default(); let rules = FxHashMap::default(); + let categories: OxlintCategories = OxlintCategories::default(); let overrides = OxlintOverrides::default(); let cache = RulesCache::new(config.plugins); let extended_paths = Vec::new(); - Self { rules, config, overrides, cache, extended_paths } + Self { rules, config, categories, overrides, cache, extended_paths } } /// Warn on all rules in all plugins and categories, including those in `nursery`. @@ -57,10 +59,11 @@ impl ConfigStoreBuilder { pub fn all() -> Self { let config = LintConfig { plugins: LintPlugins::all(), ..LintConfig::default() }; let overrides = OxlintOverrides::default(); + let categories: OxlintCategories = OxlintCategories::default(); let cache = RulesCache::new(config.plugins); let rules = RULES.iter().map(|rule| (rule.clone(), AllowWarnDeny::Warn)).collect(); let extended_paths = Vec::new(); - Self { rules, config, overrides, cache, extended_paths } + Self { rules, config, categories, overrides, cache, extended_paths } } /// Create a [`ConfigStoreBuilder`] from a loaded or manually built [`Oxlintrc`]. @@ -138,6 +141,12 @@ impl ConfigStoreBuilder { Self::warn_correctness(oxlintrc.plugins.unwrap_or_default()) }; + let mut categories = oxlintrc.categories.clone(); + + if !start_empty { + categories.insert(RuleCategory::Correctness, AllowWarnDeny::Warn); + } + let config = LintConfig { plugins: oxlintrc.plugins.unwrap_or_default(), settings: oxlintrc.settings, @@ -147,8 +156,14 @@ impl ConfigStoreBuilder { }; let cache = RulesCache::new(config.plugins); - let mut builder = - Self { rules, config, overrides: oxlintrc.overrides, cache, extended_paths }; + let mut builder = Self { + rules, + config, + categories, + overrides: oxlintrc.overrides, + cache, + extended_paths, + }; for filter in oxlintrc.categories.filters() { builder = builder.with_filter(&filter); @@ -183,6 +198,11 @@ impl ConfigStoreBuilder { self } + pub fn with_categories(mut self, categories: OxlintCategories) -> Self { + self.categories = categories; + self + } + /// Enable or disable a set of plugins, leaving unrelated plugins alone. /// /// See [`ConfigStoreBuilder::with_plugins`] for details on how plugin configuration affects your @@ -285,7 +305,7 @@ impl ConfigStoreBuilder { self.rules.into_iter().collect::>() }; rules.sort_unstable_by_key(|(r, _)| r.id()); - Config::new(rules, self.config, self.overrides) + Config::new(rules, self.categories, self.config, self.overrides) } /// Warn for all correctness rules in the given set of plugins. diff --git a/crates/oxc_linter/src/config/config_store.rs b/crates/oxc_linter/src/config/config_store.rs index 8f8bf7836d891..7874717a07cdb 100644 --- a/crates/oxc_linter/src/config/config_store.rs +++ b/crates/oxc_linter/src/config/config_store.rs @@ -5,7 +5,7 @@ use std::{ use rustc_hash::FxHashMap; -use super::{LintConfig, LintPlugins, overrides::OxlintOverrides}; +use super::{LintConfig, LintPlugins, categories::OxlintCategories, overrides::OxlintOverrides}; use crate::{ AllowWarnDeny, rules::{RULES, RuleEnum}, @@ -28,8 +28,18 @@ impl Clone for ResolvedLinterState { #[derive(Debug, Clone)] pub struct Config { /// The basic linter state for this configuration. + /// For files that match no overrides, this lint config will be used. pub(crate) base: ResolvedLinterState, + /// Used as the base config for applying overrides + /// NOTE: `AllowWarnDeny::Allow` should exist here to allow us to correctly + /// keep a rule disabled when an override is applied with a different plugin set + pub(crate) base_rules: Vec<(RuleEnum, AllowWarnDeny)>, + + /// Categories specified at the root. This is used to resolve which rules + /// should be enabled when a different plugin is enabled as part of an override. + pub(crate) categories: OxlintCategories, + /// An optional set of overrides to apply to the base state depending on the file being linted. pub(crate) overrides: OxlintOverrides, } @@ -37,14 +47,24 @@ pub struct Config { impl Config { pub fn new( rules: Vec<(RuleEnum, AllowWarnDeny)>, + categories: OxlintCategories, config: LintConfig, overrides: OxlintOverrides, ) -> Self { Config { base: ResolvedLinterState { - rules: Arc::from(rules.into_boxed_slice()), + rules: Arc::from( + rules + .iter() + .filter(|(_, severity)| severity.is_warn_deny()) + .cloned() + .collect::>() + .into_boxed_slice(), + ), config: Arc::new(config), }, + base_rules: rules, + categories, overrides, } } @@ -96,8 +116,7 @@ impl Config { } let mut rules = self - .base - .rules + .base_rules .iter() .filter(|(rule, _)| plugins.contains(LintPlugins::from(rule.plugin_name()))) .cloned() @@ -110,6 +129,18 @@ impl Config { .collect::>(); for override_config in overrides_to_apply { + if let Some(override_plugins) = override_config.plugins { + if override_plugins != plugins { + for (rule, severity) in all_rules.iter().filter_map(|rule| { + self.categories + .get(&rule.category()) + .map(|severity| (rule.clone(), severity)) + }) { + rules.entry(rule).or_insert(*severity); + } + } + } + if !override_config.rules.is_empty() { override_config.rules.override_rules(&mut rules, &all_rules); } @@ -137,7 +168,8 @@ impl Config { Arc::new(config) }; - let rules = rules.into_iter().collect::>(); + let rules = + rules.into_iter().filter(|(_, severity)| severity.is_warn_deny()).collect::>(); ResolvedLinterState { rules: Arc::from(rules.into_boxed_slice()), config } } } @@ -203,7 +235,10 @@ mod test { use super::{ConfigStore, OxlintOverrides}; use crate::{ AllowWarnDeny, LintPlugins, RuleEnum, - config::{LintConfig, OxlintEnv, OxlintGlobals, OxlintSettings, config_store::Config}, + config::{ + LintConfig, OxlintEnv, OxlintGlobals, OxlintSettings, categories::OxlintCategories, + config_store::Config, + }, }; macro_rules! from_json { @@ -226,7 +261,7 @@ mod test { "rules": {} }]); let store = ConfigStore::new( - Config::new(base_rules, LintConfig::default(), overrides), + Config::new(base_rules, OxlintCategories::default(), LintConfig::default(), overrides), FxHashMap::default(), ); @@ -248,7 +283,7 @@ mod test { "rules": {} }]); let store = ConfigStore::new( - Config::new(base_rules, LintConfig::default(), overrides), + Config::new(base_rules, OxlintCategories::default(), LintConfig::default(), overrides), FxHashMap::default(), ); @@ -271,7 +306,7 @@ mod test { }]); let store = ConfigStore::new( - Config::new(base_rules, LintConfig::default(), overrides), + Config::new(base_rules, OxlintCategories::default(), LintConfig::default(), overrides), FxHashMap::default(), ); assert_eq!(store.number_of_rules(), Some(1)); @@ -294,7 +329,7 @@ mod test { }]); let store = ConfigStore::new( - Config::new(base_rules, LintConfig::default(), overrides), + Config::new(base_rules, OxlintCategories::default(), LintConfig::default(), overrides), FxHashMap::default(), ); assert_eq!(store.number_of_rules(), Some(1)); @@ -317,7 +352,7 @@ mod test { }]); let store = ConfigStore::new( - Config::new(base_rules, LintConfig::default(), overrides), + Config::new(base_rules, OxlintCategories::default(), LintConfig::default(), overrides), FxHashMap::default(), ); assert_eq!(store.number_of_rules(), Some(1)); @@ -348,8 +383,10 @@ mod test { "plugins": ["typescript"], }]); - let store = - ConfigStore::new(Config::new(vec![], base_config, overrides), FxHashMap::default()); + let store = ConfigStore::new( + Config::new(vec![], OxlintCategories::default(), base_config, overrides), + FxHashMap::default(), + ); assert_eq!(store.base.base.config.plugins, LintPlugins::IMPORT); let app = store.resolve("other.mjs".as_ref()).config; @@ -382,8 +419,10 @@ mod test { }, }]); - let store = - ConfigStore::new(Config::new(vec![], base_config, overrides), FxHashMap::default()); + let store = ConfigStore::new( + Config::new(vec![], OxlintCategories::default(), base_config, overrides), + FxHashMap::default(), + ); assert!(!store.base.base.config.env.contains("React")); let app = store.resolve("App.tsx".as_ref()).config; @@ -407,8 +446,10 @@ mod test { }, }]); - let store = - ConfigStore::new(Config::new(vec![], base_config, overrides), FxHashMap::default()); + let store = ConfigStore::new( + Config::new(vec![], OxlintCategories::default(), base_config, overrides), + FxHashMap::default(), + ); assert!(store.base.base.config.env.contains("es2024")); let app = store.resolve("App.tsx".as_ref()).config; @@ -433,8 +474,10 @@ mod test { }, }]); - let store = - ConfigStore::new(Config::new(vec![], base_config, overrides), FxHashMap::default()); + let store = ConfigStore::new( + Config::new(vec![], OxlintCategories::default(), base_config, overrides), + FxHashMap::default(), + ); assert!(!store.base.base.config.globals.is_enabled("React")); assert!(!store.base.base.config.globals.is_enabled("Secret")); @@ -464,8 +507,10 @@ mod test { }, }]); - let store = - ConfigStore::new(Config::new(vec![], base_config, overrides), FxHashMap::default()); + let store = ConfigStore::new( + Config::new(vec![], OxlintCategories::default(), base_config, overrides), + FxHashMap::default(), + ); assert!(store.base.base.config.globals.is_enabled("React")); assert!(store.base.base.config.globals.is_enabled("Secret")); diff --git a/crates/oxc_linter/src/config/rules.rs b/crates/oxc_linter/src/config/rules.rs index bf37c6a77fc85..3c706f7262afe 100644 --- a/crates/oxc_linter/src/config/rules.rs +++ b/crates/oxc_linter/src/config/rules.rs @@ -61,7 +61,6 @@ impl OxlintRules { pub(crate) fn override_rules(&self, rules_for_override: &mut RuleSet, all_rules: &[RuleEnum]) { use itertools::Itertools; let mut rules_to_replace: Vec<(RuleEnum, AllowWarnDeny)> = vec![]; - let mut rules_to_remove: Vec = vec![]; // Rules can have the same name but different plugin names let lookup = self.rules.iter().into_group_map_by(|r| r.rule_name.as_str()); @@ -91,7 +90,9 @@ impl OxlintRules { if let Some((rule, _)) = rules_for_override.iter().find(|(r, _)| { r.name() == rule_name && r.plugin_name() == plugin_name }) { - rules_to_remove.push(rule.clone()); + let config = rule_config.config.clone().unwrap_or_default(); + let rule = rule.read_json(config); + rules_to_replace.push((rule, AllowWarnDeny::Allow)); } // If the given rule is not found in the rule list (for example, if all rules are disabled), // then look it up in the entire rules list and add it. @@ -101,7 +102,7 @@ impl OxlintRules { { let config = rule_config.config.clone().unwrap_or_default(); let rule = rule.read_json(config); - rules_to_remove.push(rule); + rules_to_replace.push((rule, AllowWarnDeny::Allow)); } } } @@ -136,16 +137,25 @@ impl OxlintRules { .push((rule.read_json(config), rule_config.severity)); } } else if let Some(&rule) = rules.get(&plugin_name) { - rules_to_remove.push(rule.clone()); + let config = rule_config.config.clone().unwrap_or_default(); + let rule = rule.read_json(config); + rules_to_replace.push((rule, AllowWarnDeny::Allow)); + } + // If the given rule is not found in the rule list (for example, if all rules are disabled), + // then look it up in the entire rules list and add it with Allow severity. + else if let Some(rule) = all_rules + .iter() + .find(|r| r.name() == rule_name && r.plugin_name() == plugin_name) + { + let config = rule_config.config.clone().unwrap_or_default(); + let rule = rule.read_json(config); + rules_to_replace.push((rule, AllowWarnDeny::Allow)); } } } } } - for rule in rules_to_remove { - rules_for_override.remove(&rule); - } for (rule, severity) in rules_to_replace { let _ = rules_for_override.remove(&rule); rules_for_override.insert(rule, severity); @@ -454,7 +464,7 @@ mod test { rules.insert(RuleEnum::EslintNoConsole(Default::default()), AllowWarnDeny::Deny); r#override(&mut rules, &json!({ "eslint/no-console": "off" })); - assert!(rules.is_empty()); + assert!(!rules.iter().any(|(_, severity)| severity.is_warn_deny())); } #[test] diff --git a/crates/oxc_linter/src/tester.rs b/crates/oxc_linter/src/tester.rs index 21b5748e34387..c51d93c9a8249 100644 --- a/crates/oxc_linter/src/tester.rs +++ b/crates/oxc_linter/src/tester.rs @@ -11,7 +11,7 @@ use oxc_allocator::Allocator; use oxc_diagnostics::{GraphicalReportHandler, GraphicalTheme, NamedSource}; use rustc_hash::FxHashMap; use serde::Deserialize; -use serde_json::Value; +use serde_json::{Value, json}; use crate::{ AllowWarnDeny, ConfigStore, ConfigStoreBuilder, LintPlugins, LintService, LintServiceOptions, @@ -414,14 +414,8 @@ impl Tester { fn test_pass(&mut self) { for TestCase { source, rule_config, eslint_config, path } in self.expect_pass.clone() { - let result = self.run( - &source, - rule_config.clone(), - &eslint_config, - path, - ExpectFixKind::None, - 0, - ); + let result = + self.run(&source, rule_config.clone(), eslint_config, path, ExpectFixKind::None, 0); let passed = result == TestResult::Passed; let config = rule_config.map_or_else( || "\n\n------------------------\n".to_string(), @@ -442,14 +436,8 @@ impl Tester { fn test_fail(&mut self) { for TestCase { source, rule_config, eslint_config, path } in self.expect_fail.clone() { - let result = self.run( - &source, - rule_config.clone(), - &eslint_config, - path, - ExpectFixKind::None, - 0, - ); + let result = + self.run(&source, rule_config.clone(), eslint_config, path, ExpectFixKind::None, 0); let failed = result == TestResult::Failed; let config = rule_config.map_or_else( || "\n\n------------------------".to_string(), @@ -485,7 +473,7 @@ impl Tester { let ExpectFixTestCase { source, expected, rule_config: config } = fix; for (index, expect) in expected.iter().enumerate() { let result = - self.run(&source, config.clone(), &None, None, expect.kind, index as u8); + self.run(&source, config.clone(), None, None, expect.kind, index as u8); match result { TestResult::Fixed(fixed_str) => assert_eq!( expect.expected, fixed_str, @@ -499,12 +487,11 @@ impl Tester { } } - #[expect(clippy::ref_option)] fn run( &mut self, source_text: &str, rule_config: Option, - eslint_config: &Option, + eslint_config: Option, path: Option, fix_kind: ExpectFixKind, fix_index: u8, @@ -515,8 +502,8 @@ impl Tester { self.lint_options, ConfigStore::new( eslint_config - .as_ref() - .map_or_else(ConfigStoreBuilder::empty, |v| { + .map_or_else(ConfigStoreBuilder::empty, |mut v| { + v.as_object_mut().unwrap().insert("categories".into(), json!({})); ConfigStoreBuilder::from_oxlintrc(true, Oxlintrc::deserialize(v).unwrap()) .unwrap() })