From 8cebdc8129872f4ef4b32d09913f69f7055ebb2e Mon Sep 17 00:00:00 2001 From: camchenry <1514176+camchenry@users.noreply.github.com> Date: Thu, 21 Nov 2024 01:46:28 +0000 Subject: [PATCH] feat(linter): allow appending plugins in override (#7379) follow up to https://github.com/oxc-project/oxc/issues/6896 for improved compatibility with ESLint, this tries to match the behavior of plugin overrides so that plugins can be enabled for certain paths. this does not allow disabling plugins. --- crates/oxc_linter/src/config/flat.rs | 79 +++++++++++++---------- crates/oxc_linter/src/config/overrides.rs | 28 ++++++++ crates/oxc_linter/src/config/plugins.rs | 27 ++++---- 3 files changed, 85 insertions(+), 49 deletions(-) diff --git a/crates/oxc_linter/src/config/flat.rs b/crates/oxc_linter/src/config/flat.rs index f80020883391d..1dfd48d5991e9 100644 --- a/crates/oxc_linter/src/config/flat.rs +++ b/crates/oxc_linter/src/config/flat.rs @@ -113,12 +113,7 @@ impl ConfigStore { /// NOTE: this function must not borrow any entries from `self.cache` or DashMap will deadlock. fn apply_overrides(&self, override_ids: &[OverrideId]) -> ResolvedLinterState { - let plugins = self - .overrides - .iter() - .rev() - .find_map(|cfg| cfg.plugins) - .unwrap_or(self.base.config.plugins); + let mut plugins = self.base.config.plugins; let all_rules = RULES .iter() @@ -135,10 +130,14 @@ impl ConfigStore { let overrides = override_ids.iter().map(|id| &self.overrides[*id]); for override_config in overrides { - if override_config.rules.is_empty() { - continue; + if !override_config.rules.is_empty() { + override_config.rules.override_rules(&mut rules, &all_rules); + } + + // Append the override's plugins to the base list of enabled plugins. + if let Some(override_plugins) = override_config.plugins { + plugins |= override_plugins; } - override_config.rules.override_rules(&mut rules, &all_rules); } let rules = rules.into_iter().collect::>(); @@ -158,7 +157,10 @@ impl ConfigStore { #[cfg(test)] mod test { use super::{ConfigStore, OxlintOverrides}; - use crate::{config::LintConfig, AllowWarnDeny, LintPlugins, RuleEnum, RuleWithSeverity}; + use crate::{ + config::{LintConfig, OxlintEnv, OxlintGlobals, OxlintSettings}, + AllowWarnDeny, LintPlugins, RuleEnum, RuleWithSeverity, + }; macro_rules! from_json { ($json:tt) => { @@ -171,11 +173,6 @@ mod test { RuleWithSeverity::new(RuleEnum::NoExplicitAny(Default::default()), AllowWarnDeny::Warn) } - #[allow(clippy::default_trait_access)] - fn no_cycle() -> RuleWithSeverity { - RuleWithSeverity::new(RuleEnum::NoCycle(Default::default()), AllowWarnDeny::Warn) - } - /// an empty ruleset is a no-op #[test] fn test_no_rules() { @@ -219,26 +216,6 @@ mod test { ); } - /// removing plugins strips rules from those plugins, even if no rules are - /// added/removed explicitly - #[test] - fn test_no_rules_and_remove_plugins() { - let base_rules = vec![no_cycle()]; - let overrides = from_json!([{ - "files": ["*.test.{ts,tsx}"], - "plugins": ["jest"], - "rules": {} - }]); - let config = LintConfig { - plugins: LintPlugins::default() | LintPlugins::IMPORT, - ..LintConfig::default() - }; - let store = ConfigStore::new(base_rules, config, overrides); - - assert_eq!(store.resolve("App.tsx".as_ref()).rules.len(), 1); - assert_eq!(store.resolve("App.test.tsx".as_ref()).rules.len(), 0); - } - #[test] fn test_remove_rule() { let base_rules = vec![no_explicit_any()]; @@ -300,4 +277,36 @@ mod test { assert_eq!(src_app.len(), 1); assert_eq!(src_app[0].severity, AllowWarnDeny::Deny); } + + #[test] + fn test_add_plugins() { + let base_config = LintConfig { + plugins: LintPlugins::IMPORT, + env: OxlintEnv::default(), + settings: OxlintSettings::default(), + globals: OxlintGlobals::default(), + }; + let overrides = from_json!([{ + "files": ["*.jsx", "*.tsx"], + "plugins": ["react"], + }, { + "files": ["*.ts", "*.tsx"], + "plugins": ["typescript"], + }]); + + let store = ConfigStore::new(vec![], base_config, overrides); + assert_eq!(store.base.config.plugins, LintPlugins::IMPORT); + + let app = store.resolve("other.mjs".as_ref()).config; + assert_eq!(app.plugins, LintPlugins::IMPORT); + + let app = store.resolve("App.jsx".as_ref()).config; + assert_eq!(app.plugins, LintPlugins::IMPORT | LintPlugins::REACT); + + let app = store.resolve("App.ts".as_ref()).config; + assert_eq!(app.plugins, LintPlugins::IMPORT | LintPlugins::TYPESCRIPT); + + let app = store.resolve("App.tsx".as_ref()).config; + assert_eq!(app.plugins, LintPlugins::IMPORT | LintPlugins::REACT | LintPlugins::TYPESCRIPT); + } } diff --git a/crates/oxc_linter/src/config/overrides.rs b/crates/oxc_linter/src/config/overrides.rs index 576466cd30fae..dc4ca11acfb22 100644 --- a/crates/oxc_linter/src/config/overrides.rs +++ b/crates/oxc_linter/src/config/overrides.rs @@ -151,3 +151,31 @@ impl JsonSchema for GlobSet { gen.subschema_for::>() } } + +mod test { + #[test] + fn test_parsing_plugins() { + use super::*; + use serde_json::{from_value, json}; + + let config: OxlintOverride = from_value(json!({ + "files": ["*.tsx"], + })) + .unwrap(); + assert_eq!(config.plugins, None); + + let config: OxlintOverride = from_value(json!({ + "files": ["*.tsx"], + "plugins": [], + })) + .unwrap(); + assert_eq!(config.plugins, Some(LintPlugins::empty())); + + let config: OxlintOverride = from_value(json!({ + "files": ["*.tsx"], + "plugins": ["typescript", "react"], + })) + .unwrap(); + assert_eq!(config.plugins, Some(LintPlugins::REACT | LintPlugins::TYPESCRIPT)); + } +} diff --git a/crates/oxc_linter/src/config/plugins.rs b/crates/oxc_linter/src/config/plugins.rs index b507b32a351d2..85cedb9e43306 100644 --- a/crates/oxc_linter/src/config/plugins.rs +++ b/crates/oxc_linter/src/config/plugins.rs @@ -149,7 +149,7 @@ impl> FromIterator for LintPlugins { fn from_iter>(iter: T) -> Self { iter.into_iter() .map(|plugin| plugin.as_ref().into()) - .fold(LintPlugins::default(), LintPlugins::union) + .fold(LintPlugins::empty(), LintPlugins::union) } } @@ -184,18 +184,17 @@ impl<'de> Deserialize<'de> for LintPlugins { // serde_json::from_value provides a String. The former is // used in almost all cases, but the latter is more // convenient for test cases. - match seq.next_element::<&str>() { + match seq.next_element::() { Ok(Some(next)) => { - plugins |= next.into(); + plugins |= next.as_str().into(); } Ok(None) => break, - Err(_) => { - if let Some(next) = seq.next_element::()? { - plugins |= next.as_str().into(); - } else { - break; + Err(_) => match seq.next_element::<&str>() { + Ok(Some(next)) => { + plugins |= next.into(); } - } + Ok(None) | Err(_) => break, + }, }; } @@ -316,7 +315,7 @@ impl LintPluginOptions { impl> FromIterator<(S, bool)> for LintPluginOptions { fn from_iter>(iter: I) -> Self { - let mut options = Self::default(); + let mut options = Self::none(); for (s, enabled) in iter { let flags = LintPlugins::from(s.as_ref()); match flags { @@ -381,11 +380,11 @@ mod test { fn test_collect_empty() { let empty: &[&str] = &[]; let plugins: LintPluginOptions = empty.iter().copied().collect(); - assert_eq!(plugins, LintPluginOptions::default()); + assert_eq!(plugins, LintPluginOptions::none()); let empty: Vec<(String, bool)> = vec![]; let plugins: LintPluginOptions = empty.into_iter().collect(); - assert_eq!(plugins, LintPluginOptions::default()); + assert_eq!(plugins, LintPluginOptions::none()); } #[test] @@ -394,9 +393,9 @@ mod test { let plugins: LintPluginOptions = enabled.into_iter().collect(); let expected = LintPluginOptions { react: true, - unicorn: true, + unicorn: false, typescript: true, - oxc: true, + oxc: false, import: false, jsdoc: false, jest: true,