fix(linter): category filters not re-configuring already-enabled rules

crates/oxc_linter/src/builder.rs

@@ -13,7 +13,7 @@ use crate::{
 
 #[must_use = "You dropped your builder without building a Linter! Did you mean to call .build()?"]
 pub struct LinterBuilder {
-    rules: FxHashSet<RuleWithSeverity>,
+    pub(super) rules: FxHashSet<RuleWithSeverity>,
     options: LintOptions,
     config: LintConfig,
     cache: RulesCache,
@@ -119,6 +119,11 @@ impl LinterBuilder {
         self
     }
 
+    #[inline]
+    pub fn plugins(&self) -> LintPlugins {
+        self.options.plugins
+    }
+
     #[cfg(test)]
     pub(crate) fn with_rule(mut self, rule: RuleWithSeverity) -> Self {
         self.rules.insert(rule);
@@ -139,12 +144,15 @@ impl LinterBuilder {
         match severity {
             AllowWarnDeny::Deny | AllowWarnDeny::Warn => match filter {
                 LintFilterKind::Category(category) => {
-                    self.rules.extend(
-                        all_rules
-                            .iter()
-                            .filter(|rule| rule.category() == category)
-                            .map(|rule| RuleWithSeverity::new(rule.clone(), severity)),
-                    );
+                    let rules_to_configure = all_rules.iter().filter(|r| r.category() == category);
+                    for rule in rules_to_configure {
+                        if let Some(mut existing_rule) = self.rules.take(rule) {
+                            existing_rule.severity = severity;
+                            self.rules.insert(existing_rule);
+                        } else {
+                            self.rules.insert(RuleWithSeverity::new(rule.clone(), severity));
+                        }
+                    }
                 }
                 LintFilterKind::Rule(_, name) => {
                     self.rules.extend(
@@ -292,3 +300,140 @@ impl RulesCache {
         *self.0.borrow_mut() = Some(all_rules);
     }
 }
+
+#[cfg(test)]
+mod test {
+    use super::*;
+
+    #[test]
+    fn test_builder_default() {
+        let builder = LinterBuilder::default();
+        assert_eq!(builder.plugins(), LintPlugins::default());
+
+        // populated with all correctness-level ESLint rules at a "warn" severity
+        assert!(!builder.rules.is_empty());
+        for rule in &builder.rules {
+            assert_eq!(rule.category(), RuleCategory::Correctness);
+            assert_eq!(rule.severity, AllowWarnDeny::Warn);
+            let plugin = rule.rule.plugin_name();
+            let name = rule.name();
+            assert!(
+                builder.plugins().contains(plugin.into()),
+                "{plugin}/{name} is in the default rule set but its plugin is not enabled"
+            );
+        }
+    }
+
+    #[test]
+    fn test_builder_empty() {
+        let builder = LinterBuilder::empty();
+        assert_eq!(builder.plugins(), LintPlugins::default());
+        assert!(builder.rules.is_empty());
+    }
+
+    #[test]
+    fn test_filter_deny_on_default() {
+        let builder = LinterBuilder::default();
+        let initial_rule_count = builder.rules.len();
+
+        let builder = builder.with_filters([LintFilter::deny(RuleCategory::Correctness)]);
+        let rule_count_after_deny = builder.rules.len();
+
+        // By default, all correctness rules are set to warn. the above filter should only
+        // re-configure those rules, and not add/remove any others.
+        assert!(!builder.rules.is_empty());
+        assert_eq!(initial_rule_count, rule_count_after_deny);
+
+        for rule in &builder.rules {
+            assert_eq!(rule.category(), RuleCategory::Correctness);
+            assert_eq!(rule.severity, AllowWarnDeny::Deny);
+
+            let plugin = rule.plugin_name();
+            let name = rule.name();
+            assert!(
+                builder.plugins().contains(plugin.into()),
+                "{plugin}/{name} is in the default rule set but its plugin is not enabled"
+            );
+        }
+    }
+
+    #[test]
+    fn test_filter_allow_all_then_warn() {
+        let builder =
+            LinterBuilder::default()
+                .with_filters([LintFilter::new(AllowWarnDeny::Allow, "all").unwrap()]);
+        assert!(builder.rules.is_empty(), "Allowing all rules should empty out the rules list");
+
+        let builder = builder.with_filters([LintFilter::warn(RuleCategory::Correctness)]);
+        assert!(
+            !builder.rules.is_empty(),
+            "warning on categories after allowing all rules should populate the rules set"
+        );
+        for rule in &builder.rules {
+            let plugin = rule.plugin_name();
+            let name = rule.name();
+            assert_eq!(
+                rule.severity,
+                AllowWarnDeny::Warn,
+                "{plugin}/{name} should have a warning severity"
+            );
+            assert_eq!(
+                rule.category(),
+                RuleCategory::Correctness,
+                "{plugin}/{name} should not be enabled b/c we only enabled correctness rules"
+            );
+        }
+    }
+
+    #[test]
+    fn test_rules_after_plugin_added() {
+        let builder = LinterBuilder::default();
+        let initial_rule_count = builder.rules.len();
+
+        let builder = builder.and_plugins(LintPlugins::IMPORT, true);
+        assert_eq!(initial_rule_count, builder.rules.len(), "Enabling a plugin should not add any rules, since we don't know which categories to turn on.");
+    }
+
+    #[test]
+    fn test_plugin_configuration() {
+        let builder = LinterBuilder::default();
+        let initial_plugins = builder.plugins();
+
+        // ==========================================================================================
+        // Test LinterBuilder::and_plugins, which deltas the plugin list instead of overriding it
+        // ==========================================================================================
+
+        // Enable eslint plugin. Since it's already enabled, this does nothing.
+        assert!(initial_plugins.contains(LintPlugins::ESLINT)); // sanity check that eslint is
+                                                                // enabled
+        let builder = builder.and_plugins(LintPlugins::ESLINT, true);
+        assert_eq!(initial_plugins, builder.plugins());
+
+        // Disable import plugin. Since it's not already enabled, this is also a no-op.
+        assert!(!builder.plugins().contains(LintPlugins::IMPORT)); // sanity check that it's not
+                                                                   // already enabled
+        let builder = builder.and_plugins(LintPlugins::IMPORT, false);
+        assert_eq!(initial_plugins, builder.plugins());
+
+        // Enable import plugin. Since it's not already enabled, this turns it on.
+        let builder = builder.and_plugins(LintPlugins::IMPORT, true);
+        assert_eq!(LintPlugins::default().union(LintPlugins::IMPORT), builder.plugins());
+        assert_ne!(initial_plugins, builder.plugins());
+
+        // Turn import back off, resetting plugins to the initial state
+        let builder = builder.and_plugins(LintPlugins::IMPORT, false);
+        assert_eq!(initial_plugins, builder.plugins());
+
+        // ==========================================================================================
+        // Test LinterBuilder::with_plugins, which _does_ override plugins
+        // ==========================================================================================
+
+        let builder = builder.with_plugins(LintPlugins::ESLINT);
+        assert_eq!(LintPlugins::ESLINT, builder.plugins());
+
+        let expected_plugins =
+            LintPlugins::ESLINT.union(LintPlugins::TYPESCRIPT).union(LintPlugins::NEXTJS);
+        let builder = builder.with_plugins(expected_plugins);
+        assert_eq!(expected_plugins, builder.plugins());
+    }
+}

crates/oxc_linter/src/rule.rs

@@ -1,5 +1,5 @@
 use std::{
-    borrow::Cow,
+    borrow::{Borrow, Cow},
     fmt,
     hash::{Hash, Hasher},
     ops::Deref,
@@ -261,6 +261,12 @@ impl Deref for RuleWithSeverity {
     }
 }
 
+impl Borrow<RuleEnum> for RuleWithSeverity {
+    fn borrow(&self) -> &RuleEnum {
+        &self.rule
+    }
+}
+
 impl RuleWithSeverity {
     pub fn new(rule: RuleEnum, severity: AllowWarnDeny) -> Self {
         Self { rule, severity }