Skip to content

Commit 7ad6cf8

Browse files
camchenrycamchenry
committed
refactor(linter): store severity separately, remove RuleWithSeverity (#11051)
Originally I thought this might improve performance by storing the `AllowWarnDeny` separately, since it only occupies 1 byte, while the struct needs to be aligned to 8 bytes (I think?). But it didn't really make any difference. However, I think it's still worth merging as removing the `RuleWithSeverity` struct makes it conceptually simpler, since we don't have yet another rule-like struct in the codebase.
1 parent 126ae75 commit 7ad6cf8

File tree

9 files changed

+129
-197
lines changed

9 files changed

+129
-197
lines changed

‎.clippy.toml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
ignore-interior-mutability = ["oxc_linter::rule::RuleWithSeverity"]
1+
ignore-interior-mutability = ["oxc_linter::rules::RuleEnum"]
22

33
disallowed-methods = [
44
{ path = "str::to_ascii_lowercase", reason = "To avoid memory allocation, use `cow_utils::CowUtils::cow_to_ascii_lowercase` instead." },

‎crates/oxc_linter/src/config/config_builder.rs‎

Lines changed: 65 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -4,14 +4,13 @@ use std::{
44
};
55

66
use itertools::Itertools;
7-
use rustc_hash::FxHashSet;
7+
use rustc_hash::FxHashMap;
88

99
use oxc_diagnostics::OxcDiagnostic;
1010
use oxc_span::{CompactStr, format_compact_str};
1111

1212
use crate::{
1313
AllowWarnDeny, LintConfig, LintFilter, LintFilterKind, Oxlintrc, RuleCategory, RuleEnum,
14-
RuleWithSeverity,
1514
config::{ESLintRule, LintPlugins, OxlintOverrides, OxlintRules, overrides::OxlintOverride},
1615
rules::RULES,
1716
};
@@ -20,7 +19,7 @@ use super::Config;
2019

2120
#[must_use = "You dropped your builder without building a Linter! Did you mean to call .build()?"]
2221
pub struct ConfigStoreBuilder {
23-
pub(super) rules: FxHashSet<RuleWithSeverity>,
22+
pub(super) rules: FxHashMap<RuleEnum, AllowWarnDeny>,
2423
config: LintConfig,
2524
overrides: OxlintOverrides,
2625
cache: RulesCache,
@@ -39,7 +38,7 @@ impl ConfigStoreBuilder {
3938
/// You can think of this as `oxlint -A all`.
4039
pub fn empty() -> Self {
4140
let config = LintConfig::default();
42-
let rules = FxHashSet::default();
41+
let rules = FxHashMap::default();
4342
let overrides = OxlintOverrides::default();
4443
let cache = RulesCache::new(config.plugins);
4544

@@ -54,15 +53,8 @@ impl ConfigStoreBuilder {
5453
let config = LintConfig { plugins: LintPlugins::all(), ..LintConfig::default() };
5554
let overrides = OxlintOverrides::default();
5655
let cache = RulesCache::new(config.plugins);
57-
Self {
58-
rules: RULES
59-
.iter()
60-
.map(|rule| RuleWithSeverity { rule: rule.clone(), severity: AllowWarnDeny::Warn })
61-
.collect(),
62-
config,
63-
overrides,
64-
cache,
65-
}
56+
let rules = RULES.iter().map(|rule| (rule.clone(), AllowWarnDeny::Warn)).collect();
57+
Self { rules, config, overrides, cache }
6658
}
6759

6860
/// Create a [`ConfigStoreBuilder`] from a loaded or manually built [`Oxlintrc`].
@@ -104,7 +96,7 @@ impl ConfigStoreBuilder {
10496

10597
let config = LintConfig { plugins, settings, env, globals, path: Some(path) };
10698
let rules =
107-
if start_empty { FxHashSet::default() } else { Self::warn_correctness(plugins) };
99+
if start_empty { FxHashMap::default() } else { Self::warn_correctness(plugins) };
108100
let cache = RulesCache::new(config.plugins);
109101
let mut builder = Self { rules, config, overrides, cache };
110102

@@ -222,12 +214,15 @@ impl ConfigStoreBuilder {
222214
}
223215

224216
#[cfg(test)]
225-
pub(crate) fn with_rule(mut self, rule: RuleWithSeverity) -> Self {
226-
self.rules.insert(rule);
217+
pub(crate) fn with_rule(mut self, rule: RuleEnum, severity: AllowWarnDeny) -> Self {
218+
self.rules.insert(rule, severity);
227219
self
228220
}
229221

230-
pub(crate) fn with_rules<R: IntoIterator<Item = RuleWithSeverity>>(mut self, rules: R) -> Self {
222+
pub(crate) fn with_rules<R: IntoIterator<Item = (RuleEnum, AllowWarnDeny)>>(
223+
mut self,
224+
rules: R,
225+
) -> Self {
231226
self.rules.extend(rules);
232227
self
233228
}
@@ -263,12 +258,12 @@ impl ConfigStoreBuilder {
263258
},
264259
AllowWarnDeny::Allow => match filter {
265260
LintFilterKind::Category(category) => {
266-
self.rules.retain(|rule| rule.category() != *category);
261+
self.rules.retain(|rule, _| rule.category() != *category);
267262
}
268263
LintFilterKind::Rule(plugin, rule) => {
269-
self.rules.retain(|r| r.plugin_name() != plugin || r.name() != rule);
264+
self.rules.retain(|r, _| r.plugin_name() != plugin || r.name() != rule);
270265
}
271-
LintFilterKind::Generic(name) => self.rules.retain(|rule| rule.name() != name),
266+
LintFilterKind::Generic(name) => self.rules.retain(|rule, _| rule.name() != name),
272267
LintFilterKind::All => self.rules.clear(),
273268
},
274269
}
@@ -287,14 +282,13 @@ impl ConfigStoreBuilder {
287282
// NOTE: we may want to warn users if they're configuring a rule that does not exist.
288283
let rules_to_configure = all_rules.iter().filter(query);
289284
for rule in rules_to_configure {
290-
match self.rules.take(rule) {
291-
Some(mut existing_rule) => {
292-
existing_rule.severity = severity;
293-
self.rules.insert(existing_rule);
294-
}
295-
_ => {
296-
self.rules.insert(RuleWithSeverity::new(rule.clone(), severity));
297-
}
285+
// If the rule is already in the list, just update its severity.
286+
// Otherwise, add it to the map.
287+
288+
if let Some(existing_rule) = self.rules.get_mut(rule) {
289+
*existing_rule = severity;
290+
} else {
291+
self.rules.insert(rule.clone(), severity);
298292
}
299293
}
300294
}
@@ -306,17 +300,20 @@ impl ConfigStoreBuilder {
306300
// to be taken out.
307301
let plugins = self.plugins();
308302
let mut rules = if self.cache.is_stale() {
309-
self.rules.into_iter().filter(|r| plugins.contains(r.plugin_name().into())).collect()
303+
self.rules
304+
.into_iter()
305+
.filter(|(r, _)| plugins.contains(r.plugin_name().into()))
306+
.collect()
310307
} else {
311308
self.rules.into_iter().collect::<Vec<_>>()
312309
};
313-
rules.sort_unstable_by_key(|r| r.id());
310+
rules.sort_unstable_by_key(|(r, _)| r.id());
314311

315312
Ok(Config::new(rules, self.config, self.overrides))
316313
}
317314

318315
/// Warn for all correctness rules in the given set of plugins.
319-
fn warn_correctness(plugins: LintPlugins) -> FxHashSet<RuleWithSeverity> {
316+
fn warn_correctness(plugins: LintPlugins) -> FxHashMap<RuleEnum, AllowWarnDeny> {
320317
RULES
321318
.iter()
322319
.filter(|rule| {
@@ -325,7 +322,7 @@ impl ConfigStoreBuilder {
325322
rule.category() == RuleCategory::Correctness
326323
&& plugins.contains(LintPlugins::from(rule.plugin_name()))
327324
})
328-
.map(|rule| RuleWithSeverity { rule: rule.clone(), severity: AllowWarnDeny::Warn })
325+
.map(|rule| (rule.clone(), AllowWarnDeny::Warn))
329326
.collect()
330327
}
331328

@@ -344,13 +341,13 @@ impl ConfigStoreBuilder {
344341
let new_rules = self
345342
.rules
346343
.iter()
347-
.sorted_by_key(|x| (x.plugin_name(), x.name()))
348-
.map(|r: &RuleWithSeverity| ESLintRule {
344+
.sorted_by_key(|(r, _)| (r.plugin_name(), r.name()))
345+
.map(|(r, severity)| ESLintRule {
349346
plugin_name: r.plugin_name().to_string(),
350-
rule_name: r.rule.name().to_string(),
351-
severity: r.severity,
347+
rule_name: r.name().to_string(),
348+
severity: *severity,
352349
config: rule_name_to_rule
353-
.get(&get_name(r.plugin_name(), r.rule.name()))
350+
.get(&get_name(r.plugin_name(), r.name()))
354351
.and_then(|r| r.config.clone()),
355352
})
356353
.collect();
@@ -519,10 +516,10 @@ mod test {
519516

520517
// populated with all correctness-level ESLint rules at a "warn" severity
521518
assert!(!builder.rules.is_empty());
522-
for rule in &builder.rules {
519+
for (rule, severity) in &builder.rules {
523520
assert_eq!(rule.category(), RuleCategory::Correctness);
524-
assert_eq!(rule.severity, AllowWarnDeny::Warn);
525-
let plugin = rule.rule.plugin_name();
521+
assert_eq!(*severity, AllowWarnDeny::Warn);
522+
let plugin = rule.plugin_name();
526523
let name = rule.name();
527524
assert!(
528525
builder.plugins().contains(plugin.into()),
@@ -551,9 +548,9 @@ mod test {
551548
assert!(!builder.rules.is_empty());
552549
assert_eq!(initial_rule_count, rule_count_after_deny);
553550

554-
for rule in &builder.rules {
551+
for (rule, severity) in &builder.rules {
555552
assert_eq!(rule.category(), RuleCategory::Correctness);
556-
assert_eq!(rule.severity, AllowWarnDeny::Deny);
553+
assert_eq!(*severity, AllowWarnDeny::Deny);
557554

558555
let plugin = rule.plugin_name();
559556
let name = rule.name();
@@ -579,12 +576,12 @@ mod test {
579576
"Changing a single rule from warn to deny should not add a new one, just modify what's already there."
580577
);
581578

582-
let no_const_assign = builder
579+
let (_, severity) = builder
583580
.rules
584581
.iter()
585-
.find(|r| r.plugin_name() == "eslint" && r.name() == "no-const-assign")
582+
.find(|(r, _)| r.plugin_name() == "eslint" && r.name() == "no-const-assign")
586583
.expect("Could not find eslint/no-const-assign after configuring it to 'deny'");
587-
assert_eq!(no_const_assign.severity, AllowWarnDeny::Deny);
584+
assert_eq!(*severity, AllowWarnDeny::Deny);
588585
}
589586
}
590587
// turn on a rule that isn't configured yet and set it to "warn"
@@ -595,15 +592,15 @@ mod test {
595592
let filter = LintFilter::new(AllowWarnDeny::Warn, filter_string).unwrap();
596593
let builder = ConfigStoreBuilder::default();
597594
// sanity check: not already turned on
598-
assert!(!builder.rules.iter().any(|r| r.name() == "no-console"));
595+
assert!(!builder.rules.iter().any(|(r, _)| r.name() == "no-console"));
599596
let builder = builder.with_filter(&filter);
600-
let no_console = builder
597+
let (_, severity) = builder
601598
.rules
602599
.iter()
603-
.find(|r| r.plugin_name() == "eslint" && r.name() == "no-console")
600+
.find(|(r, _)| r.plugin_name() == "eslint" && r.name() == "no-console")
604601
.expect("Could not find eslint/no-console after configuring it to 'warn'");
605602

606-
assert_eq!(no_console.severity, AllowWarnDeny::Warn);
603+
assert_eq!(*severity, AllowWarnDeny::Warn);
607604
}
608605
}
609606

@@ -618,11 +615,11 @@ mod test {
618615
!builder.rules.is_empty(),
619616
"warning on categories after allowing all rules should populate the rules set"
620617
);
621-
for rule in &builder.rules {
618+
for (rule, severity) in &builder.rules {
622619
let plugin = rule.plugin_name();
623620
let name = rule.name();
624621
assert_eq!(
625-
rule.severity,
622+
*severity,
626623
AllowWarnDeny::Warn,
627624
"{plugin}/{name} should have a warning severity"
628625
);
@@ -656,7 +653,7 @@ mod test {
656653
desired_plugins.set(LintPlugins::TYPESCRIPT, false);
657654

658655
let linter = ConfigStoreBuilder::default().with_plugins(desired_plugins).build().unwrap();
659-
for rule in linter.base.rules.iter() {
656+
for (rule, _) in linter.base.rules.iter() {
660657
let name = rule.name();
661658
let plugin = rule.plugin_name();
662659
assert_ne!(
@@ -727,32 +724,28 @@ mod test {
727724
)
728725
.unwrap();
729726
let builder = ConfigStoreBuilder::from_oxlintrc(false, oxlintrc).unwrap();
730-
for rule in &builder.rules {
727+
for (rule, severity) in &builder.rules {
731728
let name = rule.name();
732729
let plugin = rule.plugin_name();
733730
let category = rule.category();
734731
match category {
735732
RuleCategory::Correctness => {
736733
if name == "no-const-assign" {
737734
assert_eq!(
738-
rule.severity,
735+
*severity,
739736
AllowWarnDeny::Deny,
740737
"no-const-assign should be denied",
741738
);
742739
} else {
743740
assert_eq!(
744-
rule.severity,
741+
*severity,
745742
AllowWarnDeny::Warn,
746743
"{plugin}/{name} should be a warning"
747744
);
748745
}
749746
}
750747
RuleCategory::Suspicious => {
751-
assert_eq!(
752-
rule.severity,
753-
AllowWarnDeny::Deny,
754-
"{plugin}/{name} should be denied"
755-
);
748+
assert_eq!(*severity, AllowWarnDeny::Deny, "{plugin}/{name} should be denied");
756749
}
757750
invalid => {
758751
panic!("Found rule {plugin}/{name} with an unexpected category {invalid:?}");
@@ -796,25 +789,26 @@ mod test {
796789
update_rules_config
797790
.rules()
798791
.iter()
799-
.any(|r| r.name() == "no-debugger" && r.severity == AllowWarnDeny::Warn)
792+
.any(|(r, severity)| r.name() == "no-debugger" && *severity == AllowWarnDeny::Warn)
800793
);
801794
assert!(
802795
update_rules_config
803796
.rules()
804797
.iter()
805-
.any(|r| r.name() == "no-console" && r.severity == AllowWarnDeny::Warn)
798+
.any(|(r, severity)| r.name() == "no-console" && *severity == AllowWarnDeny::Warn)
806799
);
807800
assert!(
808801
!update_rules_config
809802
.rules()
810803
.iter()
811-
.any(|r| r.name() == "no-null" && r.severity == AllowWarnDeny::Allow)
804+
.any(|(r, severity)| r.name() == "no-null" && *severity == AllowWarnDeny::Allow)
812805
);
813806
assert!(
814807
update_rules_config
815808
.rules()
816809
.iter()
817-
.any(|r| r.name() == "prefer-as-const" && r.severity == AllowWarnDeny::Warn)
810+
.any(|(r, severity)| r.name() == "prefer-as-const"
811+
&& *severity == AllowWarnDeny::Warn)
818812
);
819813
}
820814

@@ -831,7 +825,7 @@ mod test {
831825
}
832826
"#,
833827
);
834-
assert!(warn_all.rules().iter().all(|r| r.severity == AllowWarnDeny::Warn));
828+
assert!(warn_all.rules().iter().all(|(_, severity)| *severity == AllowWarnDeny::Warn));
835829

836830
let deny_all = config_store_from_str(
837831
r#"
@@ -844,7 +838,7 @@ mod test {
844838
}
845839
"#,
846840
);
847-
assert!(deny_all.rules().iter().all(|r| r.severity == AllowWarnDeny::Deny));
841+
assert!(deny_all.rules().iter().all(|(_, severity)| *severity == AllowWarnDeny::Deny));
848842

849843
let allow_all = config_store_from_str(
850844
r#"
@@ -857,7 +851,7 @@ mod test {
857851
}
858852
"#,
859853
);
860-
assert!(allow_all.rules().iter().all(|r| r.severity == AllowWarnDeny::Allow));
854+
assert!(allow_all.rules().iter().all(|(_, severity)| *severity == AllowWarnDeny::Allow));
861855
assert_eq!(allow_all.number_of_rules(), 0);
862856

863857
let allow_and_override_config = config_store_from_str(
@@ -879,19 +873,20 @@ mod test {
879873
allow_and_override_config
880874
.rules()
881875
.iter()
882-
.any(|r| r.name() == "no-var" && r.severity == AllowWarnDeny::Warn)
876+
.any(|(r, severity)| r.name() == "no-var" && *severity == AllowWarnDeny::Warn)
883877
);
884878
assert!(
885879
allow_and_override_config
886880
.rules()
887881
.iter()
888-
.any(|r| r.name() == "approx-constant" && r.severity == AllowWarnDeny::Deny)
882+
.any(|(r, severity)| r.name() == "approx-constant"
883+
&& *severity == AllowWarnDeny::Deny)
889884
);
890885
assert!(
891886
allow_and_override_config
892887
.rules()
893888
.iter()
894-
.any(|r| r.name() == "no-null" && r.severity == AllowWarnDeny::Deny)
889+
.any(|(r, severity)| r.name() == "no-null" && *severity == AllowWarnDeny::Deny)
895890
);
896891
}
897892

0 commit comments

Comments
 (0)