Skip to content

Commit

Permalink
perf(linter/no-unused-vars): do not construct Regex for default ign…
Browse files Browse the repository at this point in the history
…ore 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('_')`.

<img width="1439" alt="image" src="https://github.com/user-attachments/assets/7a4abb8c-2c0e-4252-8df4-e5b1880e8209">

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.
  • Loading branch information
camchenry committed Oct 16, 2024
1 parent 9f555d7 commit ee73f56
Show file tree
Hide file tree
Showing 3 changed files with 117 additions and 32 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Expand Down Expand Up @@ -115,11 +117,14 @@ impl NoUnusedVars {
}

fn get_unused_var_name(&self, symbol: &Symbol<'_, '_>) -> Option<CompactStr> {
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,
};

Expand Down
10 changes: 7 additions & 3 deletions crates/oxc_linter/src/rules/eslint/no_unused_vars/ignored.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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('_'),
}
}
}

Expand Down
122 changes: 99 additions & 23 deletions crates/oxc_linter/src/rules/eslint/no_unused_vars/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ pub struct NoUnusedVarsOptions {
/// var b = 10;
/// console.log(b);
/// ```
pub vars_ignore_pattern: Option<Regex>,
pub vars_ignore_pattern: IgnorePattern<Regex>,

/// Controls how unused arguments are checked.
///
Expand Down Expand Up @@ -65,7 +65,7 @@ pub struct NoUnusedVarsOptions {
/// }
/// foo(1, 2);
/// ```
pub args_ignore_pattern: Option<Regex>,
pub args_ignore_pattern: IgnorePattern<Regex>,

/// Using a Rest property it is possible to "omit" properties from an
/// object, but by default the sibling properties are marked as "unused".
Expand Down Expand Up @@ -108,7 +108,7 @@ pub struct NoUnusedVarsOptions {
/// console.error("Error caught in catch block");
/// }
/// ```
pub caught_errors_ignore_pattern: Option<Regex>,
pub caught_errors_ignore_pattern: IgnorePattern<Regex>,

/// This option specifies exceptions within destructuring patterns that will
/// not be checked for usage. Variables declared within array destructuring
Expand All @@ -132,7 +132,7 @@ pub struct NoUnusedVarsOptions {
/// console.log(n);
/// });
/// ```
pub destructured_array_ignore_pattern: Option<Regex>,
pub destructured_array_ignore_pattern: IgnorePattern<Regex>,

/// The `ignoreClassWithStaticInitBlock` option is a boolean (default:
/// `false`). Static initialization blocks allow you to initialize static
Expand Down Expand Up @@ -207,18 +207,92 @@ pub struct NoUnusedVarsOptions {
pub report_used_ignore_pattern: bool,
}

/// Represents an `Option<Regex>` 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<R> {
/// 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<R> IgnorePattern<R> {
/// 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<Option<Regex>> for IgnorePattern<Regex> {
#[inline]
fn from(value: Option<Regex>) -> 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,
}
Expand Down Expand Up @@ -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<Regex> {
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<Regex> {
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<Value> for NoUnusedVarsOptions {
type Error = OxcDiagnostic;
Expand All @@ -426,7 +502,7 @@ impl TryFrom<Value> 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<Regex> =
let vars_ignore_pattern=
parse_unicode_rule(config.get("varsIgnorePattern"), "varsIgnorePattern");

let args: ArgsOption = config
Expand All @@ -437,7 +513,7 @@ impl TryFrom<Value> for NoUnusedVarsOptions {
.transpose()?
.unwrap_or_default();

let args_ignore_pattern: Option<Regex> =
let args_ignore_pattern =
parse_unicode_rule(config.get("argsIgnorePattern"), "argsIgnorePattern");

let caught_errors: CaughtErrors = config
Expand All @@ -453,7 +529,7 @@ impl TryFrom<Value> for NoUnusedVarsOptions {
"caughtErrorsIgnorePattern",
);

let destructured_array_ignore_pattern: Option<Regex> = parse_unicode_rule(
let destructured_array_ignore_pattern = parse_unicode_rule(
config.get("destructuredArrayIgnorePattern"),
"destructuredArrayIgnorePattern",
);
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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());
Expand Down

0 comments on commit ee73f56

Please sign in to comment.