Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf(es/lints): Avoid needless allocations in no-dupe-args #9041

Merged
merged 14 commits into from
Jun 14, 2024
86 changes: 48 additions & 38 deletions crates/swc_ecma_lints/src/rules/no_dupe_args.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
use swc_atoms::JsWord;
use swc_common::{collections::AHashMap, errors::HANDLER, Span};
use swc_common::errors::HANDLER;
use swc_ecma_ast::*;
use swc_ecma_utils::find_pat_ids;
use swc_ecma_utils::for_each_binding_ident;
use swc_ecma_visit::{noop_visit_type, Visit, VisitWith};

use crate::rule::{visitor_rule, Rule};
Expand All @@ -13,54 +12,65 @@ pub fn no_dupe_args() -> Box<dyn Rule> {
#[derive(Debug, Default)]
struct NoDupeArgs {}

impl NoDupeArgs {
fn check(&self, param_list: Vec<(JsWord, Span)>) {
let mut variables_map = AHashMap::<JsWord, Span>::default();

param_list.into_iter().for_each(|(js_word, span)| {
if let Some(old_span) = variables_map.insert(js_word.clone(), span) {
HANDLER.with(|handler| {
handler
.struct_span_err(
span,
&format!(
"the name `{}` is bound more than once in this parameter list",
js_word
),
)
.span_label(old_span, "previous definition here".to_string())
.span_label(span, &"used as parameter more than once".to_string())
.emit();
});
}
});
}
/// This has time complexity of O(n^2), but it's fine as the number of paramters
/// is usually small.
macro_rules! check {
Comment on lines +33 to +34
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably holds true for 99.9%, but maybe we can add another implementation that is used for n > 100 based on a hashmap.

($node:expr) => {{
// This allocates only if there are duplicate parameters.
let mut done = vec![];
kdy1 marked this conversation as resolved.
Show resolved Hide resolved

let mut i1 = 0;
for_each_binding_ident($node, |id1| {
i1 += 1;

let mut i2 = 0;
for_each_binding_ident($node, |id2| {
i2 += 1;

if i1 >= i2 || done.contains(&i1) {
return;
kdy1 marked this conversation as resolved.
Show resolved Hide resolved
}

done.push(i1);

kdy1 marked this conversation as resolved.
Show resolved Hide resolved
if id1.span.ctxt == id2.span.ctxt && id1.sym == id2.sym {
HANDLER.with(|handler| {
handler
.struct_span_err(
id2.span,
&format!(
"the name `{}` is bound more than once in this parameter list",
id1.sym
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a binding is defined 3 times this error will be reported multiple times. That was already the case with the old implementation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's handled by done. Actually I added done just to match the original behavior.

),
)
.span_label(id1.span, "previous definition here".to_string())
.span_label(id2.span, &"used as parameter more than once".to_string())
.emit();
});
}
})
})
}};
}

impl Visit for NoDupeArgs {
noop_visit_type!();

fn visit_function(&mut self, f: &Function) {
let variables: Vec<(JsWord, Span)> = find_pat_ids(&f.params);

self.check(variables);
check!(&f.params);

f.visit_children_with(self);
}

fn visit_arrow_expr(&mut self, arrow_fn: &ArrowExpr) {
let variables: Vec<(JsWord, Span)> = find_pat_ids(&arrow_fn.params);

self.check(variables);
fn visit_arrow_expr(&mut self, f: &ArrowExpr) {
check!(&f.params);

arrow_fn.visit_children_with(self);
f.visit_children_with(self);
}

fn visit_constructor(&mut self, n: &Constructor) {
let variables: Vec<(JsWord, Span)> = find_pat_ids(&n.params);
fn visit_constructor(&mut self, f: &Constructor) {
check!(&f.params);

self.check(variables);

n.visit_children_with(self);
f.visit_children_with(self);
}
}
35 changes: 35 additions & 0 deletions crates/swc_ecma_utils/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2373,6 +2373,8 @@ pub struct DestructuringFinder<I: IdentLike> {
}

/// Finds all **binding** idents of `node`.
///
/// If you want to avoid allocation, use [`for_each_binding_ident`] instead.
pub fn find_pat_ids<T, I: IdentLike>(node: &T) -> Vec<I>
where
T: VisitWith<DestructuringFinder<I>>,
Expand All @@ -2397,6 +2399,39 @@ impl<I: IdentLike> Visit for DestructuringFinder<I> {
fn visit_prop_name(&mut self, _: &PropName) {}
}

/// Finds all **binding** idents of variables.
pub struct BindingIdentifierVisitor<F>
where
F: for<'a> FnMut(&'a Ident),
{
op: F,
}

/// Finds all **binding** idents of `node`. **Any nested identifiers in
/// expressions are ignored**.
pub fn for_each_binding_ident<T, F>(node: &T, op: F)
where
T: VisitWith<BindingIdentifierVisitor<F>>,
F: for<'a> FnMut(&'a Ident),
{
let mut v = BindingIdentifierVisitor { op };
node.visit_with(&mut v);
}

impl<F> Visit for BindingIdentifierVisitor<F>
where
F: for<'a> FnMut(&'a Ident),
{
noop_visit_type!();

/// No-op (we don't care about expressions)
fn visit_expr(&mut self, _: &Expr) {}

fn visit_binding_ident(&mut self, i: &BindingIdent) {
(self.op)(i);
}
}

pub fn is_valid_ident(s: &JsWord) -> bool {
if s.len() == 0 {
return false;
Expand Down
Loading