Skip to content

Commit

Permalink
perf(linter): reduce String allocations and clones
Browse files Browse the repository at this point in the history
  • Loading branch information
DonIsaac committed Aug 5, 2024
1 parent e8b662a commit 00bde6e
Show file tree
Hide file tree
Showing 15 changed files with 103 additions and 87 deletions.
32 changes: 21 additions & 11 deletions crates/oxc_linter/src/config/env.rs
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -9,15 +10,25 @@ use serde::Deserialize;
pub struct OxlintEnv(FxHashMap<String, bool>);

impl OxlintEnv {
pub fn from_vec(env: Vec<String>) -> Self {
let map = env.into_iter().map(|key| (key, true)).collect();

Self(map)
pub fn contains<Q>(&self, key: &Q) -> bool
where
String: Borrow<Q>,
Q: ?Sized + Hash + Eq,
{
self.0.get(key).is_some_and(|v| *v)
}

pub fn iter(&self) -> impl Iterator<Item = &str> + '_ {
// 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<String> for OxlintEnv {
fn from_iter<T: IntoIterator<Item = String>>(iter: T) -> Self {
let map = iter.into_iter().map(|key| (key, true)).collect();

Self(map)
}
}

Expand All @@ -32,7 +43,6 @@ impl Default for OxlintEnv {

#[cfg(test)]
mod test {
use itertools::Itertools;
use serde::Deserialize;

use super::OxlintEnv;
Expand All @@ -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"));
}
}
33 changes: 19 additions & 14 deletions crates/oxc_linter/src/config/settings/jsdoc.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use std::borrow::Cow;

use rustc_hash::FxHashMap;
use schemars::JsonSchema;
use serde::Deserialize;
Expand Down Expand Up @@ -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<String> {
pub fn check_blocked_tag_name(&self, tag_name: &str) -> Option<Cow<str>> {
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<String> {
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<Cow<str>> {
let reason = |preferred_name: &str| -> Cow<str> {
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
Expand Down Expand Up @@ -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,
}
}
}
Expand Down Expand Up @@ -198,6 +202,7 @@ enum TagNamePreference {
#[cfg(test)]
mod test {
use serde::Deserialize;
use std::borrow::Cow;

use super::JSDocPluginSettings;

Expand Down Expand Up @@ -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);
}

Expand All @@ -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())
);
}
}
8 changes: 5 additions & 3 deletions crates/oxc_linter/src/config/settings/next.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use std::borrow::Cow;

use schemars::JsonSchema;
use serde::Deserialize;

Expand All @@ -9,10 +11,10 @@ pub struct NextPluginSettings {
}

impl NextPluginSettings {
pub fn get_root_dirs(&self) -> Vec<String> {
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),
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -109,7 +109,7 @@ impl Rule for NoNamedAsDefaultMember {
},
&ident.name,
prop_str,
&module_name,
module_name,
));
};
};
Expand All @@ -136,7 +136,7 @@ impl Rule for NoNamedAsDefaultMember {
decl.span,
&ident.name,
&name,
&module_name,
module_name,
));
}
}
Expand Down
71 changes: 36 additions & 35 deletions crates/oxc_linter/src/rules/jest/consistent_test_it.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::str::FromStr;
use std::{borrow::Cow, str::FromStr};

use oxc_ast::{ast::Expression, AstKind};
use oxc_diagnostics::OxcDiagnostic;
Expand All @@ -17,35 +17,40 @@ 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,
Test,
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)
}
}

Expand Down Expand Up @@ -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))
}
Expand Down
2 changes: 1 addition & 1 deletion crates/oxc_linter/src/rules/jest/expect_expect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
2 changes: 1 addition & 1 deletion crates/oxc_linter/src/rules/jsdoc/check_access.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
Expand Down
2 changes: 1 addition & 1 deletion crates/oxc_linter/src/rules/jsdoc/check_property_names.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::<Vec<_>>();
Expand Down
2 changes: 1 addition & 1 deletion crates/oxc_linter/src/rules/jsdoc/require_param.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::<Vec<_>>();

Expand Down
Loading

0 comments on commit 00bde6e

Please sign in to comment.