From ee73f568b3116ccd8dbb861f2cda3e4465b6c080 Mon Sep 17 00:00:00 2001 From: camchenry <1514176+camchenry@users.noreply.github.com> Date: Wed, 16 Oct 2024 19:59:29 +0000 Subject: [PATCH] perf(linter/no-unused-vars): do not construct `Regex` for default ignore pattern (#6590) Profiling shows that constructing the `Regex` for `no-unused-vars` takes up a good chunk of time for small files. Since the default pattern for ignored variables is just `^_`, we can more simply implement this with `.starts_with('_')`. image I decided to create a new `Option`-like enum that has three states: `Some` and `None` which are like the typical `Option` states, and another `Default` state which is a special case. The `Default` state represents that matching code should use the default pattern (if applicable). I decided not to keep using `Option`, because I didn't want to overload the `None` state by confusing whether or not it was the default state, or the user explicitly overrided it as `None`. The theory here is that most users are not going to pass a custom ignore pattern, so we can avoid needing to create a `Regex` in most cases, which greatly speeds up the initialization for this rule. --- .../eslint/no_unused_vars/fixers/fix_vars.rs | 17 ++- .../rules/eslint/no_unused_vars/ignored.rs | 10 +- .../rules/eslint/no_unused_vars/options.rs | 122 ++++++++++++++---- 3 files changed, 117 insertions(+), 32 deletions(-) diff --git a/crates/oxc_linter/src/rules/eslint/no_unused_vars/fixers/fix_vars.rs b/crates/oxc_linter/src/rules/eslint/no_unused_vars/fixers/fix_vars.rs index 95f37d2a62c59..27cfda717defd 100644 --- a/crates/oxc_linter/src/rules/eslint/no_unused_vars/fixers/fix_vars.rs +++ b/crates/oxc_linter/src/rules/eslint/no_unused_vars/fixers/fix_vars.rs @@ -4,10 +4,12 @@ use oxc_ast::{ }; use oxc_semantic::{AstNode, NodeId}; use oxc_span::CompactStr; -use regex::Regex; use super::{count_whitespace_or_commas, BindingInfo, NoUnusedVars, Symbol}; -use crate::fixer::{RuleFix, RuleFixer}; +use crate::{ + fixer::{RuleFix, RuleFixer}, + rules::eslint::no_unused_vars::options::IgnorePattern, +}; impl NoUnusedVars { /// Delete a variable declaration or rename it to match `varsIgnorePattern`. @@ -115,11 +117,14 @@ impl NoUnusedVars { } fn get_unused_var_name(&self, symbol: &Symbol<'_, '_>) -> Option { - let pat = self.vars_ignore_pattern.as_ref().map(Regex::as_str)?; - - let ignored_name: String = match pat { + let ignored_name: String = match self.vars_ignore_pattern.as_ref() { // TODO: support more patterns - "^_" => format!("_{}", symbol.name()), + IgnorePattern::Default => { + format!("_{}", symbol.name()) + } + IgnorePattern::Some(re) if re.as_str() == "^_" => { + format!("_{}", symbol.name()) + } _ => return None, }; diff --git a/crates/oxc_linter/src/rules/eslint/no_unused_vars/ignored.rs b/crates/oxc_linter/src/rules/eslint/no_unused_vars/ignored.rs index 0a8f812fce6a8..fd7674a682a9c 100644 --- a/crates/oxc_linter/src/rules/eslint/no_unused_vars/ignored.rs +++ b/crates/oxc_linter/src/rules/eslint/no_unused_vars/ignored.rs @@ -8,7 +8,7 @@ use oxc_ast::{ }; use regex::Regex; -use super::{NoUnusedVars, Symbol}; +use super::{options::IgnorePattern, NoUnusedVars, Symbol}; #[derive(Debug, Default, Clone, Copy)] pub(super) enum FoundStatus { @@ -336,8 +336,12 @@ impl NoUnusedVars { } #[inline] - fn is_none_or_match(re: Option<&Regex>, haystack: &str) -> bool { - re.map_or(false, |pat| pat.is_match(haystack)) + fn is_none_or_match(re: IgnorePattern<&Regex>, haystack: &str) -> bool { + match re { + IgnorePattern::None => false, + IgnorePattern::Some(re) => re.is_match(haystack), + IgnorePattern::Default => haystack.starts_with('_'), + } } } diff --git a/crates/oxc_linter/src/rules/eslint/no_unused_vars/options.rs b/crates/oxc_linter/src/rules/eslint/no_unused_vars/options.rs index ddfe83ec71974..46e41303478e0 100644 --- a/crates/oxc_linter/src/rules/eslint/no_unused_vars/options.rs +++ b/crates/oxc_linter/src/rules/eslint/no_unused_vars/options.rs @@ -34,7 +34,7 @@ pub struct NoUnusedVarsOptions { /// var b = 10; /// console.log(b); /// ``` - pub vars_ignore_pattern: Option, + pub vars_ignore_pattern: IgnorePattern, /// Controls how unused arguments are checked. /// @@ -65,7 +65,7 @@ pub struct NoUnusedVarsOptions { /// } /// foo(1, 2); /// ``` - pub args_ignore_pattern: Option, + pub args_ignore_pattern: IgnorePattern, /// Using a Rest property it is possible to "omit" properties from an /// object, but by default the sibling properties are marked as "unused". @@ -108,7 +108,7 @@ pub struct NoUnusedVarsOptions { /// console.error("Error caught in catch block"); /// } /// ``` - pub caught_errors_ignore_pattern: Option, + pub caught_errors_ignore_pattern: IgnorePattern, /// This option specifies exceptions within destructuring patterns that will /// not be checked for usage. Variables declared within array destructuring @@ -132,7 +132,7 @@ pub struct NoUnusedVarsOptions { /// console.log(n); /// }); /// ``` - pub destructured_array_ignore_pattern: Option, + pub destructured_array_ignore_pattern: IgnorePattern, /// The `ignoreClassWithStaticInitBlock` option is a boolean (default: /// `false`). Static initialization blocks allow you to initialize static @@ -207,18 +207,92 @@ pub struct NoUnusedVarsOptions { pub report_used_ignore_pattern: bool, } +/// Represents an `Option` with an additional `Default` variant, +/// which represents the default ignore pattern for when no pattern is +/// explicitly provided. +#[derive(Debug, Clone, Copy)] +pub enum IgnorePattern { + /// No ignore pattern was provided, use the default pattern. This + /// means that the pattern is `^_`. + Default, + /// The ignore pattern is explicitly none. + None, + /// The ignore pattern is a regex. + Some(R), +} + +impl IgnorePattern { + /// Returns `true` if the pattern is [`IgnorePattern::Default`]. + #[inline] + pub fn is_default(&self) -> bool { + matches!(self, Self::Default) + } + + /// Returns `true` if the pattern is [`IgnorePattern::None`]. + #[inline] + pub fn is_none(&self) -> bool { + matches!(self, Self::None) + } + + /// Returns `true` if the pattern is [`IgnorePattern::Some`]. + #[inline] + pub fn is_some(&self) -> bool { + matches!(self, Self::Some(_)) + } + + /// Returns the inner value if it is [`IgnorePattern::Some`], otherwise + /// panics with a default message. + /// + /// See also [`Option::unwrap`]. + #[inline] + pub fn unwrap(self) -> R { + self.expect("called `IgnorePattern::unwrap()` on a non-`Some` value") + } + + /// Returns the inner value if it is [`IgnorePattern::Some`], otherwise + /// panics with a custom message provided by `msg`. + /// + /// See also [`Option::expect`]. + #[inline] + pub fn expect(self, msg: &str) -> R { + if let Self::Some(r) = self { + r + } else { + panic!("{}", msg) + } + } + + #[inline] + pub fn as_ref(&self) -> IgnorePattern<&R> { + match self { + Self::Default => IgnorePattern::Default, + Self::None => IgnorePattern::None, + Self::Some(ref r) => IgnorePattern::Some(r), + } + } +} + +impl From> for IgnorePattern { + #[inline] + fn from(value: Option) -> Self { + match value { + None => Self::None, + Some(regex) => Self::Some(regex), + } + } +} + impl Default for NoUnusedVarsOptions { fn default() -> Self { - let underscore = Some(Regex::new("^_").unwrap()); Self { vars: VarsOption::default(), - vars_ignore_pattern: underscore.clone(), + vars_ignore_pattern: IgnorePattern::Default, args: ArgsOption::default(), - args_ignore_pattern: underscore, + args_ignore_pattern: IgnorePattern::Default, ignore_rest_siblings: false, caught_errors: CaughtErrors::default(), - caught_errors_ignore_pattern: None, - destructured_array_ignore_pattern: None, + caught_errors_ignore_pattern: IgnorePattern::None, + destructured_array_ignore_pattern: IgnorePattern::None, ignore_class_with_static_init_block: false, report_used_ignore_pattern: false, } @@ -397,13 +471,15 @@ impl TryFrom<&Value> for CaughtErrors { } /// Parses a potential pattern into a [`Regex`] that accepts unicode characters. -fn parse_unicode_rule(value: Option<&Value>, name: &str) -> Option { - value - .and_then(Value::as_str) - .map(|pattern| regex::RegexBuilder::new(pattern).unicode(true).build()) - .transpose() - .map_err(|err| panic!("Invalid '{name}' option for no-unused-vars: {err}")) - .unwrap() +fn parse_unicode_rule(value: Option<&Value>, name: &str) -> IgnorePattern { + IgnorePattern::from( + value + .and_then(Value::as_str) + .map(|pattern| regex::RegexBuilder::new(pattern).unicode(true).build()) + .transpose() + .map_err(|err| panic!("Invalid '{name}' option for no-unused-vars: {err}")) + .unwrap(), + ) } impl TryFrom for NoUnusedVarsOptions { type Error = OxcDiagnostic; @@ -426,7 +502,7 @@ impl TryFrom for NoUnusedVarsOptions { // NOTE: when a configuration object is provided, do not provide // a default ignore pattern here. They've opted into configuring // this rule, and we'll give them full control over it. - let vars_ignore_pattern: Option = + let vars_ignore_pattern= parse_unicode_rule(config.get("varsIgnorePattern"), "varsIgnorePattern"); let args: ArgsOption = config @@ -437,7 +513,7 @@ impl TryFrom for NoUnusedVarsOptions { .transpose()? .unwrap_or_default(); - let args_ignore_pattern: Option = + let args_ignore_pattern = parse_unicode_rule(config.get("argsIgnorePattern"), "argsIgnorePattern"); let caught_errors: CaughtErrors = config @@ -453,7 +529,7 @@ impl TryFrom for NoUnusedVarsOptions { "caughtErrorsIgnorePattern", ); - let destructured_array_ignore_pattern: Option = parse_unicode_rule( + let destructured_array_ignore_pattern = parse_unicode_rule( config.get("destructuredArrayIgnorePattern"), "destructuredArrayIgnorePattern", ); @@ -504,9 +580,9 @@ mod tests { fn test_options_default() { let rule = NoUnusedVarsOptions::default(); assert_eq!(rule.vars, VarsOption::All); - assert!(rule.vars_ignore_pattern.is_some_and(|v| v.as_str() == "^_")); + assert!(rule.vars_ignore_pattern.is_default()); assert_eq!(rule.args, ArgsOption::AfterUsed); - assert!(rule.args_ignore_pattern.is_some_and(|v| v.as_str() == "^_")); + assert!(rule.args_ignore_pattern.is_default()); assert_eq!(rule.caught_errors, CaughtErrors::all()); assert!(rule.caught_errors_ignore_pattern.is_none()); assert!(rule.destructured_array_ignore_pattern.is_none()); @@ -601,10 +677,10 @@ mod tests { let opts = NoUnusedVarsOptions::try_from(json).unwrap(); let default = NoUnusedVarsOptions::default(); assert_eq!(opts.vars, default.vars); - assert!(default.vars_ignore_pattern.unwrap().as_str() == "^_"); + assert!(default.vars_ignore_pattern.is_default()); assert_eq!(opts.args, default.args); - assert!(default.args_ignore_pattern.unwrap().as_str() == "^_"); + assert!(default.args_ignore_pattern.is_default()); assert_eq!(opts.caught_errors, default.caught_errors); assert!(opts.caught_errors_ignore_pattern.is_none());