From e3abdfa379f9fa4621346b546b722da646160218 Mon Sep 17 00:00:00 2001 From: DonIsaac <22823424+DonIsaac@users.noreply.github.com> Date: Tue, 6 Aug 2024 01:00:57 +0000 Subject: [PATCH] perf(linter): reduce String allocations and clones (#4673) --- crates/oxc_linter/src/config/env.rs | 32 ++++++--- .../oxc_linter/src/config/settings/jsdoc.rs | 33 +++++---- crates/oxc_linter/src/config/settings/next.rs | 8 ++- .../import/no_named_as_default_member.rs | 6 +- .../src/rules/jest/consistent_test_it.rs | 71 ++++++++++--------- .../src/rules/jest/expect_expect.rs | 2 +- .../src/rules/jsdoc/check_access.rs | 2 +- .../src/rules/jsdoc/check_property_names.rs | 2 +- .../src/rules/jsdoc/require_param.rs | 2 +- .../src/rules/jsdoc/require_returns.rs | 4 +- .../src/rules/jsdoc/require_yields.rs | 10 +-- .../src/rules/typescript/ban_ts_comment.rs | 2 +- .../no_useless_promise_resolve_reject.rs | 5 +- .../src/rules/unicorn/prefer_node_protocol.rs | 9 ++- crates/oxc_linter/src/utils/jsdoc.rs | 2 +- 15 files changed, 103 insertions(+), 87 deletions(-) diff --git a/crates/oxc_linter/src/config/env.rs b/crates/oxc_linter/src/config/env.rs index a11b32a83cad7..20b516d364857 100644 --- a/crates/oxc_linter/src/config/env.rs +++ b/crates/oxc_linter/src/config/env.rs @@ -1,6 +1,7 @@ use rustc_hash::FxHashMap; use schemars::JsonSchema; use serde::Deserialize; +use std::{borrow::Borrow, hash::Hash}; /// Predefine global variables. // TODO: list the keys we support @@ -9,15 +10,25 @@ use serde::Deserialize; pub struct OxlintEnv(FxHashMap); impl OxlintEnv { - pub fn from_vec(env: Vec) -> Self { - let map = env.into_iter().map(|key| (key, true)).collect(); - - Self(map) + pub fn contains(&self, key: &Q) -> bool + where + String: Borrow, + Q: ?Sized + Hash + Eq, + { + self.0.get(key).is_some_and(|v| *v) } pub fn iter(&self) -> impl Iterator + '_ { // Filter out false values - self.0.iter().filter(|(_, v)| **v).map(|(k, _)| k.as_str()) + self.0.iter().filter_map(|(k, v)| (*v).then_some(k.as_str())) + } +} + +impl FromIterator for OxlintEnv { + fn from_iter>(iter: T) -> Self { + let map = iter.into_iter().map(|key| (key, true)).collect(); + + Self(map) } } @@ -32,7 +43,6 @@ impl Default for OxlintEnv { #[cfg(test)] mod test { - use itertools::Itertools; use serde::Deserialize; use super::OxlintEnv; @@ -44,15 +54,15 @@ mod test { })) .unwrap(); assert_eq!(env.iter().count(), 2); - assert!(env.iter().contains(&"browser")); - assert!(env.iter().contains(&"node")); - assert!(!env.iter().contains(&"es6")); - assert!(!env.iter().contains(&"builtin")); + assert!(env.contains("browser")); + assert!(env.contains("node")); + assert!(!env.contains("es6")); + assert!(!env.contains("builtin")); } #[test] fn test_parse_env_default() { let env = OxlintEnv::default(); assert_eq!(env.iter().count(), 1); - assert!(env.iter().contains(&"builtin")); + assert!(env.contains("builtin")); } } diff --git a/crates/oxc_linter/src/config/settings/jsdoc.rs b/crates/oxc_linter/src/config/settings/jsdoc.rs index 23a7295cd393c..3bf3c590dec2f 100644 --- a/crates/oxc_linter/src/config/settings/jsdoc.rs +++ b/crates/oxc_linter/src/config/settings/jsdoc.rs @@ -1,3 +1,5 @@ +use std::borrow::Cow; + use rustc_hash::FxHashMap; use schemars::JsonSchema; use serde::Deserialize; @@ -95,25 +97,27 @@ impl Default for JSDocPluginSettings { impl JSDocPluginSettings { /// Only for `check-tag-names` rule /// Return `Some(reason)` if blocked - pub fn check_blocked_tag_name(&self, tag_name: &str) -> Option { + pub fn check_blocked_tag_name(&self, tag_name: &str) -> Option> { match self.tag_name_preference.get(tag_name) { - Some(TagNamePreference::FalseOnly(_)) => Some(format!("Unexpected tag `@{tag_name}`.")), - Some(TagNamePreference::ObjectWithMessage { message }) => Some(message.to_string()), + Some(TagNamePreference::FalseOnly(_)) => { + Some(Cow::Owned(format!("Unexpected tag `@{tag_name}`."))) + } + Some(TagNamePreference::ObjectWithMessage { message }) => Some(Cow::Borrowed(message)), _ => None, } } /// Only for `check-tag-names` rule /// Return `Some(reason)` if replacement found or default aliased - pub fn check_preferred_tag_name(&self, original_name: &str) -> Option { - let reason = |preferred_name: &str| -> String { - format!("Replace tag `@{original_name}` with `@{preferred_name}`.") + pub fn check_preferred_tag_name(&self, original_name: &str) -> Option> { + let reason = |preferred_name: &str| -> Cow { + Cow::Owned(format!("Replace tag `@{original_name}` with `@{preferred_name}`.")) }; match self.tag_name_preference.get(original_name) { Some(TagNamePreference::TagNameOnly(preferred_name)) => Some(reason(preferred_name)), Some(TagNamePreference::ObjectWithMessageAndReplacement { message, .. }) => { - Some(message.to_string()) + Some(Cow::Borrowed(message)) } _ => { // https://github.com/gajus/eslint-plugin-jsdoc/blob/main/docs/settings.md#default-preferred-aliases @@ -163,13 +167,13 @@ impl JSDocPluginSettings { /// Resolve original, known tag name to user preferred name /// If not defined, return original name - pub fn resolve_tag_name(&self, original_name: &str) -> String { + pub fn resolve_tag_name<'s>(&'s self, original_name: &'s str) -> &'s str { match self.tag_name_preference.get(original_name) { Some( TagNamePreference::TagNameOnly(replacement) | TagNamePreference::ObjectWithMessageAndReplacement { replacement, .. }, - ) => replacement.to_string(), - _ => original_name.to_string(), + ) => replacement, + _ => original_name, } } } @@ -198,6 +202,7 @@ enum TagNamePreference { #[cfg(test)] mod test { use serde::Deserialize; + use std::borrow::Cow; use super::JSDocPluginSettings; @@ -294,9 +299,9 @@ mod test { .unwrap(); assert_eq!( settings.check_blocked_tag_name("foo"), - Some("Unexpected tag `@foo`.".to_string()) + Some(Cow::Borrowed("Unexpected tag `@foo`.")) ); - assert_eq!(settings.check_blocked_tag_name("bar"), Some("do not use bar".to_string())); + assert_eq!(settings.check_blocked_tag_name("bar"), Some(Cow::Borrowed("do not use bar"))); assert_eq!(settings.check_blocked_tag_name("baz"), None); } @@ -316,10 +321,10 @@ mod test { .unwrap(); assert_eq!(settings.check_preferred_tag_name("foo"), None,); assert_eq!(settings.check_preferred_tag_name("bar"), None); - assert_eq!(settings.check_preferred_tag_name("baz"), Some("baz is noop now".to_string())); + assert_eq!(settings.check_preferred_tag_name("baz"), Some("baz is noop now".into())); assert_eq!( settings.check_preferred_tag_name("qux"), - Some("Replace tag `@qux` with `@quux`.".to_string()) + Some("Replace tag `@qux` with `@quux`.".into()) ); } } diff --git a/crates/oxc_linter/src/config/settings/next.rs b/crates/oxc_linter/src/config/settings/next.rs index 0f9f20234e98c..1e5421152db9f 100644 --- a/crates/oxc_linter/src/config/settings/next.rs +++ b/crates/oxc_linter/src/config/settings/next.rs @@ -1,3 +1,5 @@ +use std::borrow::Cow; + use schemars::JsonSchema; use serde::Deserialize; @@ -9,10 +11,10 @@ pub struct NextPluginSettings { } impl NextPluginSettings { - pub fn get_root_dirs(&self) -> Vec { + pub fn get_root_dirs(&self) -> Cow<'_, [String]> { match &self.root_dir { - OneOrMany::One(val) => vec![val.clone()], - OneOrMany::Many(vec) => vec.clone(), + OneOrMany::One(val) => Cow::Owned(vec![val.clone()]), + OneOrMany::Many(vec) => Cow::Borrowed(vec), } } } diff --git a/crates/oxc_linter/src/rules/import/no_named_as_default_member.rs b/crates/oxc_linter/src/rules/import/no_named_as_default_member.rs index 57819e2b9ea97..73c48c2c8a4a1 100644 --- a/crates/oxc_linter/src/rules/import/no_named_as_default_member.rs +++ b/crates/oxc_linter/src/rules/import/no_named_as_default_member.rs @@ -86,7 +86,7 @@ impl Rule for NoNamedAsDefaultMember { .and_then(|symbol_id| has_members_map.get(&symbol_id)) .and_then(|it| { if it.0.exported_bindings.contains_key(entry_name) { - Some(it.1.to_string()) + Some(&it.1) } else { None } @@ -109,7 +109,7 @@ impl Rule for NoNamedAsDefaultMember { }, &ident.name, prop_str, - &module_name, + module_name, )); }; }; @@ -136,7 +136,7 @@ impl Rule for NoNamedAsDefaultMember { decl.span, &ident.name, &name, - &module_name, + module_name, )); } } diff --git a/crates/oxc_linter/src/rules/jest/consistent_test_it.rs b/crates/oxc_linter/src/rules/jest/consistent_test_it.rs index 926dabd77c469..4bf490c9c8065 100644 --- a/crates/oxc_linter/src/rules/jest/consistent_test_it.rs +++ b/crates/oxc_linter/src/rules/jest/consistent_test_it.rs @@ -1,4 +1,4 @@ -use std::str::FromStr; +use std::{borrow::Cow, str::FromStr}; use oxc_ast::{ast::Expression, AstKind}; use oxc_diagnostics::OxcDiagnostic; @@ -17,18 +17,18 @@ use crate::{ }; fn consistent_method(x1: &str, x2: &str, span0: Span) -> OxcDiagnostic { - OxcDiagnostic::warn("Enforce `test` and `it` usage conventions".to_string()) + OxcDiagnostic::warn("Enforce `test` and `it` usage conventions") .with_help(format!("Prefer using {x1:?} instead of {x2:?}")) .with_label(span0) } fn consistent_method_within_describe(x1: &str, x2: &str, span0: Span) -> OxcDiagnostic { - OxcDiagnostic::warn("Enforce `test` and `it` usage conventions".to_string()) + OxcDiagnostic::warn("Enforce `test` and `it` usage conventions") .with_help(format!("Prefer using {x1:?} instead of {x2:?} within describe")) .with_label(span0) } -#[derive(Debug, Clone, Eq, PartialEq)] +#[derive(Debug, Clone, Copy, Eq, PartialEq)] enum TestCaseName { Fit, IT, @@ -36,16 +36,21 @@ enum TestCaseName { Xit, Xtest, } +impl TestCaseName { + pub fn as_str(self) -> &'static str { + match self { + Self::Fit => "fit", + Self::IT => "it", + Self::Test => "test", + Self::Xit => "xit", + Self::Xtest => "xtest", + } + } +} impl std::fmt::Display for TestCaseName { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - Self::Fit => write!(f, "fit"), - Self::IT => write!(f, "it"), - Self::Test => write!(f, "test"), - Self::Xit => write!(f, "xit"), - Self::Xtest => write!(f, "xtest"), - } + self.as_str().fmt(f) } } @@ -228,69 +233,65 @@ impl ConsistentTestIt { } let is_test = matches!(jest_fn_call.kind, JestFnKind::General(JestGeneralFnKind::Test)); - let fn_to_str = self.within_fn.to_string(); + let fn_to_str = self.within_fn.as_str(); - if is_test && describe_nesting_hash.is_empty() && !jest_fn_call.name.ends_with(&fn_to_str) { - let opposite_test_keyword = Self::get_opposite_test_case(&self.within_fn); + if is_test && describe_nesting_hash.is_empty() && !jest_fn_call.name.ends_with(fn_to_str) { + let opposite_test_keyword = Self::get_opposite_test_case(self.within_fn); if let Some((span, prefer_test_name)) = Self::get_prefer_test_name_and_span( call_expr.callee.get_inner_expression(), &jest_fn_call.name, - &fn_to_str, + fn_to_str, ) { ctx.diagnostic_with_fix( - consistent_method(&fn_to_str, &opposite_test_keyword, span), + consistent_method(fn_to_str, opposite_test_keyword, span), |fixer| fixer.replace(span, prefer_test_name), ); } } - let describe_to_str = self.within_describe.to_string(); + let describe_to_str = self.within_describe.as_str(); if is_test && !describe_nesting_hash.is_empty() - && !jest_fn_call.name.ends_with(&describe_to_str) + && !jest_fn_call.name.ends_with(describe_to_str) { - let opposite_test_keyword = Self::get_opposite_test_case(&self.within_describe); + let opposite_test_keyword = Self::get_opposite_test_case(self.within_describe); if let Some((span, prefer_test_name)) = Self::get_prefer_test_name_and_span( call_expr.callee.get_inner_expression(), &jest_fn_call.name, - &describe_to_str, + describe_to_str, ) { ctx.diagnostic_with_fix( - consistent_method_within_describe( - &describe_to_str, - &opposite_test_keyword, - span, - ), + consistent_method_within_describe(describe_to_str, opposite_test_keyword, span), |fixer| fixer.replace(span, prefer_test_name), ); } } } - fn get_opposite_test_case(test_case_name: &TestCaseName) -> String { + fn get_opposite_test_case(test_case_name: TestCaseName) -> &'static str { if matches!(test_case_name, TestCaseName::Test) { - TestCaseName::IT.to_string() + TestCaseName::IT.as_str() } else { - TestCaseName::Test.to_string() + TestCaseName::Test.as_str() } } - fn get_prefer_test_name_and_span( + fn get_prefer_test_name_and_span<'s>( expr: &Expression, test_name: &str, - fix_jest_name: &str, - ) -> Option<(Span, String)> { + fix_jest_name: &'s str, + ) -> Option<(Span, Cow<'s, str>)> { match expr { Expression::Identifier(ident) => { if ident.name.eq("fit") { - return Some((ident.span, "test.only".to_string())); + return Some((ident.span, Cow::Borrowed("test.only"))); } let prefer_test_name = match test_name.chars().next() { - Some('x') => format!("x{fix_jest_name}"), - Some('f') => format!("f{fix_jest_name}"), - _ => fix_jest_name.to_string(), + Some('x') => Cow::Owned(format!("x{fix_jest_name}")), + Some('f') => Cow::Owned(format!("f{fix_jest_name}")), + _ => Cow::Borrowed(fix_jest_name), }; Some((ident.span(), prefer_test_name)) } diff --git a/crates/oxc_linter/src/rules/jest/expect_expect.rs b/crates/oxc_linter/src/rules/jest/expect_expect.rs index 949375a0002d6..2f58e4af0f290 100644 --- a/crates/oxc_linter/src/rules/jest/expect_expect.rs +++ b/crates/oxc_linter/src/rules/jest/expect_expect.rs @@ -19,7 +19,7 @@ use crate::{ }; fn expect_expect_diagnostic(span0: Span) -> OxcDiagnostic { - OxcDiagnostic::warn("Test has no assertions".to_string()) + OxcDiagnostic::warn("Test has no assertions") .with_help("Add assertion(s) in this Test") .with_label(span0) } diff --git a/crates/oxc_linter/src/rules/jsdoc/check_access.rs b/crates/oxc_linter/src/rules/jsdoc/check_access.rs index 1287b1582b540..1a14edcc905ec 100644 --- a/crates/oxc_linter/src/rules/jsdoc/check_access.rs +++ b/crates/oxc_linter/src/rules/jsdoc/check_access.rs @@ -64,7 +64,7 @@ impl Rule for CheckAccess { let resolved_access_tag_name = settings.resolve_tag_name("access"); let mut access_related_tag_names = FxHashSet::default(); - access_related_tag_names.insert(resolved_access_tag_name.to_string()); + access_related_tag_names.insert(resolved_access_tag_name); for level in &ACCESS_LEVELS { access_related_tag_names.insert(settings.resolve_tag_name(level)); } diff --git a/crates/oxc_linter/src/rules/jsdoc/check_property_names.rs b/crates/oxc_linter/src/rules/jsdoc/check_property_names.rs index 98cc62beff657..213677736d7ea 100644 --- a/crates/oxc_linter/src/rules/jsdoc/check_property_names.rs +++ b/crates/oxc_linter/src/rules/jsdoc/check_property_names.rs @@ -101,7 +101,7 @@ impl Rule for CheckPropertyNames { .map(|span| { LabeledSpan::at( (span.start as usize)..(span.end as usize), - "Duplicated property".to_string(), + "Duplicated property", ) }) .collect::>(); diff --git a/crates/oxc_linter/src/rules/jsdoc/require_param.rs b/crates/oxc_linter/src/rules/jsdoc/require_param.rs index 33ca41095f112..63697e028eae8 100644 --- a/crates/oxc_linter/src/rules/jsdoc/require_param.rs +++ b/crates/oxc_linter/src/rules/jsdoc/require_param.rs @@ -158,7 +158,7 @@ impl Rule for RequireParam { } // Collected JSDoc `@param` tags - let tags_to_check = collect_tags(&jsdocs, &settings.resolve_tag_name("param")); + let tags_to_check = collect_tags(&jsdocs, settings.resolve_tag_name("param")); let shallow_tags = tags_to_check.iter().filter(|(name, _)| !name.contains('.')).collect::>(); diff --git a/crates/oxc_linter/src/rules/jsdoc/require_returns.rs b/crates/oxc_linter/src/rules/jsdoc/require_returns.rs index 7a714d9f05cc7..dbe7f1beda26e 100644 --- a/crates/oxc_linter/src/rules/jsdoc/require_returns.rs +++ b/crates/oxc_linter/src/rules/jsdoc/require_returns.rs @@ -226,12 +226,12 @@ impl Rule for RequireReturns { let jsdoc_tags = jsdocs.iter().flat_map(JSDoc::tags).collect::>(); let resolved_returns_tag_name = settings.resolve_tag_name("returns"); - if is_missing_returns_tag(&jsdoc_tags, &resolved_returns_tag_name) { + if is_missing_returns_tag(&jsdoc_tags, resolved_returns_tag_name) { ctx.diagnostic(missing_returns_diagnostic(*func_span)); continue; } - if let Some(span) = is_duplicated_returns_tag(&jsdoc_tags, &resolved_returns_tag_name) { + if let Some(span) = is_duplicated_returns_tag(&jsdoc_tags, resolved_returns_tag_name) { ctx.diagnostic(duplicate_returns_diagnostic(span)); } } diff --git a/crates/oxc_linter/src/rules/jsdoc/require_yields.rs b/crates/oxc_linter/src/rules/jsdoc/require_yields.rs index 6f32c246e5d9a..f842e3889f176 100644 --- a/crates/oxc_linter/src/rules/jsdoc/require_yields.rs +++ b/crates/oxc_linter/src/rules/jsdoc/require_yields.rs @@ -142,7 +142,7 @@ impl Rule for RequireYields { // Without this option, need to check `yield` value. // Check will be performed in `YieldExpression` branch. if config.force_require_yields - && is_missing_yields_tag(&jsdoc_tags, &resolved_yields_tag_name) + && is_missing_yields_tag(&jsdoc_tags, resolved_yields_tag_name) { ctx.diagnostic(missing_yields(func.span)); return; @@ -150,7 +150,7 @@ impl Rule for RequireYields { // Other checks are always performed - if let Some(span) = is_duplicated_yields_tag(&jsdoc_tags, &resolved_yields_tag_name) + if let Some(span) = is_duplicated_yields_tag(&jsdoc_tags, resolved_yields_tag_name) { ctx.diagnostic(duplicate_yields(span)); return; @@ -161,8 +161,8 @@ impl Rule for RequireYields { if let Some(span) = is_missing_yields_tag_with_generator_tag( &jsdoc_tags, - &resolved_yields_tag_name, - &resolved_generator_tag_name, + resolved_yields_tag_name, + resolved_generator_tag_name, ) { ctx.diagnostic(missing_yields_with_generator(span)); } @@ -229,7 +229,7 @@ impl Rule for RequireYields { let jsdoc_tags = jsdocs.iter().flat_map(JSDoc::tags).collect::>(); let resolved_yields_tag_name = settings.resolve_tag_name("yields"); - if is_missing_yields_tag(&jsdoc_tags, &resolved_yields_tag_name) { + if is_missing_yields_tag(&jsdoc_tags, resolved_yields_tag_name) { ctx.diagnostic(missing_yields(generator_func.span)); } } diff --git a/crates/oxc_linter/src/rules/typescript/ban_ts_comment.rs b/crates/oxc_linter/src/rules/typescript/ban_ts_comment.rs index 1d53581df9c2f..498e2354457e2 100644 --- a/crates/oxc_linter/src/rules/typescript/ban_ts_comment.rs +++ b/crates/oxc_linter/src/rules/typescript/ban_ts_comment.rs @@ -195,7 +195,7 @@ impl Rule for BanTsComment { if !re.is_match(description) { ctx.diagnostic(comment_description_not_match_pattern( directive, - &re.to_string(), + re.as_str(), comm.span, )); } diff --git a/crates/oxc_linter/src/rules/unicorn/no_useless_promise_resolve_reject.rs b/crates/oxc_linter/src/rules/unicorn/no_useless_promise_resolve_reject.rs index 417d171e6ac4e..4fdad3643571b 100644 --- a/crates/oxc_linter/src/rules/unicorn/no_useless_promise_resolve_reject.rs +++ b/crates/oxc_linter/src/rules/unicorn/no_useless_promise_resolve_reject.rs @@ -270,9 +270,8 @@ fn generate_fix<'a>( let mut replace_range = if is_reject { parent.kind().span() } else { call_expr.span() }; let replacement_text = if is_reject { - let mut text = - if arg_text.is_empty() { "undefined".to_string() } else { arg_text.to_string() }; - text = format!("throw {text}"); + let text = if arg_text.is_empty() { "undefined" } else { arg_text }; + let mut text = format!("throw {text}"); if is_yield { replace_range = get_parenthesized_node(parent, ctx).kind().span(); diff --git a/crates/oxc_linter/src/rules/unicorn/prefer_node_protocol.rs b/crates/oxc_linter/src/rules/unicorn/prefer_node_protocol.rs index 772c344502e78..125e92374771e 100644 --- a/crates/oxc_linter/src/rules/unicorn/prefer_node_protocol.rs +++ b/crates/oxc_linter/src/rules/unicorn/prefer_node_protocol.rs @@ -67,15 +67,14 @@ impl Rule for PreferNodeProtocol { let module_name = if let Some((prefix, postfix)) = string_lit_value.split_once('/') { // `e.g. ignore "assert/"` if postfix.is_empty() { - string_lit_value.to_string() + string_lit_value.as_str() } else { - prefix.to_string() + prefix } } else { - string_lit_value.to_string() + string_lit_value.as_str() }; - if module_name.starts_with("node:") - || NODEJS_BUILTINS.binary_search(&module_name.as_str()).is_err() + if module_name.starts_with("node:") || NODEJS_BUILTINS.binary_search(&module_name).is_err() { return; } diff --git a/crates/oxc_linter/src/utils/jsdoc.rs b/crates/oxc_linter/src/utils/jsdoc.rs index 0161251d9b34c..80f2851c1ac56 100644 --- a/crates/oxc_linter/src/utils/jsdoc.rs +++ b/crates/oxc_linter/src/utils/jsdoc.rs @@ -101,7 +101,7 @@ pub fn should_ignore_as_avoid( exempted_tag_names: &[String], ) -> bool { let mut ignore_tag_names = - exempted_tag_names.iter().map(std::convert::Into::into).collect::>(); + exempted_tag_names.iter().map(String::as_str).collect::>(); if settings.ignore_replaces_docs { ignore_tag_names.insert(settings.resolve_tag_name("ignore")); }