From 5450932c065d3268efd923b420cafbfcf1df2cce Mon Sep 17 00:00:00 2001 From: poyoho Date: Sat, 20 Jul 2024 17:52:00 +0800 Subject: [PATCH 1/8] feat: init --- crates/oxc_linter/src/rules.rs | 2 + .../src/rules/eslint/no_useless_call.rs | 86 +++++++++++++++++++ 2 files changed, 88 insertions(+) create mode 100644 crates/oxc_linter/src/rules/eslint/no_useless_call.rs diff --git a/crates/oxc_linter/src/rules.rs b/crates/oxc_linter/src/rules.rs index 5e38ded477a7a..aab37cf6217c8 100644 --- a/crates/oxc_linter/src/rules.rs +++ b/crates/oxc_linter/src/rules.rs @@ -106,6 +106,7 @@ mod eslint { pub mod no_unsafe_optional_chaining; pub mod no_unused_labels; pub mod no_unused_private_class_members; + pub mod no_useless_call; pub mod no_useless_catch; pub mod no_useless_concat; pub mod no_useless_constructor; @@ -519,6 +520,7 @@ oxc_macros::declare_all_lint_rules! { eslint::no_unsafe_optional_chaining, eslint::no_unused_labels, eslint::no_unused_private_class_members, + eslint::no_useless_call, eslint::no_useless_catch, eslint::no_useless_escape, eslint::no_useless_rename, diff --git a/crates/oxc_linter/src/rules/eslint/no_useless_call.rs b/crates/oxc_linter/src/rules/eslint/no_useless_call.rs new file mode 100644 index 0000000000000..ebae48cdfa72b --- /dev/null +++ b/crates/oxc_linter/src/rules/eslint/no_useless_call.rs @@ -0,0 +1,86 @@ +use oxc_diagnostics::OxcDiagnostic; +use oxc_macros::declare_oxc_lint; +use oxc_span::Span; + +use crate::{context::LintContext, rule::Rule, AstNode}; + +#[derive(Debug, Default, Clone)] +pub struct NoUselessCall; + +declare_oxc_lint!( + /// ### What it does + /// + /// + /// ### Why is this bad? + /// + /// + /// ### Example + /// ```javascript + /// ``` + NoUselessCall, + nursery, // TODO: change category to `correctness`, `suspicious`, `pedantic`, `perf`, `restriction`, or `style` + // See for details +); + +impl Rule for NoUselessCall { + fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) {} +} + +#[test] +fn test() { + use crate::tester::Tester; + + let pass = vec![ + "foo.apply(obj, 1, 2);", + "obj.foo.apply(null, 1, 2);", + "obj.foo.apply(otherObj, 1, 2);", + "a.b(x, y).c.foo.apply(a.b(x, z).c, 1, 2);", + "foo.apply(obj, [1, 2]);", + "obj.foo.apply(null, [1, 2]);", + "obj.foo.apply(otherObj, [1, 2]);", + "a.b(x, y).c.foo.apply(a.b(x, z).c, [1, 2]);", + "a.b.foo.apply(a.b.c, [1, 2]);", + "foo.apply(null, args);", + "obj.foo.apply(obj, args);", + "var call; foo[call](null, 1, 2);", + "var apply; foo[apply](null, [1, 2]);", + "foo.call();", + "obj.foo.call();", + "foo.apply();", + "obj.foo.apply();", + "obj?.foo.bar.call(obj.foo, 1, 2);", // { "ecmaVersion": 2020 }, + "class C { #call; wrap(foo) { foo.#call(undefined, 1, 2); } }", // { "ecmaVersion": 2022 } + ]; + + let fail = vec![ + "foo.call(undefined, 1, 2);", + "foo.call(void 0, 1, 2);", + "foo.call(null, 1, 2);", + "obj.foo.call(obj, 1, 2);", + "a.b.c.foo.call(a.b.c, 1, 2);", + "a.b(x, y).c.foo.call(a.b(x, y).c, 1, 2);", + "foo.apply(undefined, [1, 2]);", + "foo.apply(void 0, [1, 2]);", + "foo.apply(null, [1, 2]);", + "obj.foo.apply(obj, [1, 2]);", + "a.b.c.foo.apply(a.b.c, [1, 2]);", + "a.b(x, y).c.foo.apply(a.b(x, y).c, [1, 2]);", + "[].concat.apply([ ], [1, 2]);", + "[].concat.apply([ + /*empty*/ + ], [1, 2]);", + r#"abc.get("foo", 0).concat.apply(abc . get("foo", 0 ), [1, 2]);"#, + "foo.call?.(undefined, 1, 2);", // { "ecmaVersion": 2020 }, + "foo?.call(undefined, 1, 2);", // { "ecmaVersion": 2020 }, + "(foo?.call)(undefined, 1, 2);", // { "ecmaVersion": 2020 }, + "obj.foo.call?.(obj, 1, 2);", // { "ecmaVersion": 2020 }, + "obj?.foo.call(obj, 1, 2);", // { "ecmaVersion": 2020 }, + "(obj?.foo).call(obj, 1, 2);", // { "ecmaVersion": 2020 }, + "(obj?.foo.call)(obj, 1, 2);", // { "ecmaVersion": 2020 }, + "obj?.foo.bar.call(obj?.foo, 1, 2);", // { "ecmaVersion": 2020 }, + "(obj?.foo).bar.call(obj?.foo, 1, 2);", // { "ecmaVersion": 2020 }, + "obj.foo?.bar.call(obj.foo, 1, 2);", // { "ecmaVersion": 2020 } + ]; + + Tester::new(NoUselessCall::NAME, pass, fail).test_and_snapshot(); +} From 0d17bbcb2064633005d1f28bec05786993c3342d Mon Sep 17 00:00:00 2001 From: poyoho Date: Sun, 21 Jul 2024 22:55:11 +0800 Subject: [PATCH 2/8] feat: no useless call --- crates/oxc_linter/src/ast_util.rs | 90 +++++++++++ .../src/rules/eslint/no_useless_call.rs | 80 ++++++++- .../src/snapshots/no_useless_call.snap | 153 ++++++++++++++++++ crates/oxc_linter/src/utils/unicorn.rs | 81 +++++++++- 4 files changed, 397 insertions(+), 7 deletions(-) create mode 100644 crates/oxc_linter/src/snapshots/no_useless_call.snap diff --git a/crates/oxc_linter/src/ast_util.rs b/crates/oxc_linter/src/ast_util.rs index 7f2f4c40108d4..3e24ccaad1ea2 100644 --- a/crates/oxc_linter/src/ast_util.rs +++ b/crates/oxc_linter/src/ast_util.rs @@ -16,6 +16,68 @@ use oxc_ast::ast::*; use crate::context::LintContext; +pub enum SkipChainExpression<'a> { + ChainElement(&'a ChainElement<'a>), + Expression(&'a Expression<'a>), +} + +pub fn skip_parenthesized_expression<'a>(expr: &'a Expression<'a>) -> &Expression<'a> { + match expr { + Expression::ParenthesizedExpression(paren_expr) => &paren_expr.expression, + _ => expr + } +} + +pub fn skip_chain_expression<'a>(expr: &'a Expression<'a>) -> SkipChainExpression<'a> { + match expr { + Expression::ParenthesizedExpression(paren_expr) => skip_chain_expression(&paren_expr.expression), + Expression::ChainExpression(chain_expr) => SkipChainExpression::ChainElement(&chain_expr.expression), + _ => SkipChainExpression::Expression(expr) + } +} + +pub fn get_skip_chain_expression_static_member_expression(skip_expr_callee: SkipChainExpression) -> Option<&StaticMemberExpression> { + match skip_expr_callee { + SkipChainExpression::ChainElement(expr) => { + match expr { + ChainElement::StaticMemberExpression(member_expr) => { + Some(member_expr) + }, + _ => None, + } + }, + SkipChainExpression::Expression(expr) => { + match expr { + Expression::StaticMemberExpression(member_expr) => { + Some(member_expr) + }, + _ => None, + } + } + } +} + +pub fn get_skip_chain_expr_member_expr(skip_expr_callee: SkipChainExpression) -> Option<&MemberExpression> { + match skip_expr_callee { + SkipChainExpression::ChainElement(expr) => { + match expr { + match_member_expression!(ChainElement) => { + Some(expr.to_member_expression()) + }, + _ => None, + } + }, + SkipChainExpression::Expression(expr) => { + match expr { + match_member_expression!(Expression) => { + Some(expr.to_member_expression()) + }, + _ => None, + } + } + } +} + /// Test if an AST node is a boolean value that never changes. Specifically we /// test for: /// 1. Literal booleans (`true` or `false`) @@ -74,6 +136,14 @@ pub trait IsConstant<'a, 'b> { fn is_constant(&self, in_boolean_position: bool, ctx: &LintContext<'a>) -> bool; } +pub trait IsArray { + fn is_array(&self) -> bool; +} + +pub trait IsNullOrUnderfined { + fn is_null_or_undefined(&self) -> bool; +} + impl<'a, 'b> IsConstant<'a, 'b> for Expression<'a> { fn is_constant(&self, in_boolean_position: bool, ctx: &LintContext<'a>) -> bool { match self { @@ -170,6 +240,26 @@ impl<'a, 'b> IsConstant<'a, 'b> for Argument<'a> { } } +impl IsArray for Argument<'_> { + fn is_array(&self) -> bool { + match self { + Argument::ArrayExpression(_) => true, + _ => false + } + } +} + +impl IsNullOrUnderfined for Argument<'_> { + fn is_null_or_undefined(&self) -> bool { + match self { + Argument::NullLiteral(_) => true, + Argument::Identifier(ident) => ident.name == "undefined", + Argument::UnaryExpression(unary_expr) => unary_expr.operator == UnaryOperator::Void, + _ => false + } + } +} + impl<'a, 'b> IsConstant<'a, 'b> for ArrayExpressionElement<'a> { fn is_constant(&self, in_boolean_position: bool, ctx: &LintContext<'a>) -> bool { match self { diff --git a/crates/oxc_linter/src/rules/eslint/no_useless_call.rs b/crates/oxc_linter/src/rules/eslint/no_useless_call.rs index ebae48cdfa72b..67f154f13c724 100644 --- a/crates/oxc_linter/src/rules/eslint/no_useless_call.rs +++ b/crates/oxc_linter/src/rules/eslint/no_useless_call.rs @@ -1,18 +1,21 @@ +use oxc_ast::{ast::{Argument, CallExpression, ChainElement, Expression, StaticMemberExpression}, match_member_expression, visit::walk::walk_expression, AstKind}; use oxc_diagnostics::OxcDiagnostic; use oxc_macros::declare_oxc_lint; -use oxc_span::Span; +use oxc_span::{GetSpan, Span}; -use crate::{context::LintContext, rule::Rule, AstNode}; +use crate::{ast_util::*, context::LintContext, rule::Rule, rules::jest::expect_expect, utils::is_same_reference, AstNode}; #[derive(Debug, Default, Clone)] pub struct NoUselessCall; declare_oxc_lint!( /// ### What it does - /// + /// + /// Disallow unnecessary calls to `.call()` and `.apply()` /// /// ### Why is this bad? - /// + /// + /// This rule is aimed to flag usage of Function.prototype.call() and Function.prototype.apply() that can be replaced with the normal function invocation. /// /// ### Example /// ```javascript @@ -22,8 +25,74 @@ declare_oxc_lint!( // See for details ); +fn no_useless_call_diagnostic(span1: Span) -> OxcDiagnostic { + OxcDiagnostic::warn("Disallow the use of undeclared variables.") + .with_label(span1) +} + +fn is_array_argument(_expr: Option<&Argument>) -> bool { + let Some(expr) = _expr else { + return false; + }; + + expr.is_array() +} + +fn is_apply_member(call_expr: &CallExpression, member_expr: &StaticMemberExpression) -> bool { + (member_expr.property.name == "call" && call_expr.arguments.len() >= 1) || + (member_expr.property.name == "apply" && call_expr.arguments.len() == 2 && is_array_argument(call_expr.arguments.get(1))) +} + +fn is_call_or_non_variadic_apply(call_expr: &CallExpression, ctx: &LintContext) -> bool { + let skip_expr_callee = skip_chain_expression(&call_expr.callee); + let Some(static_member_expr) = get_skip_chain_expression_static_member_expression(skip_expr_callee) else { + return false; + }; + + is_apply_member(call_expr, static_member_expr) +} + +fn is_validate_this_arg(ctx: &LintContext, expected_this: Option<&Expression>, this_arg: &Argument) -> bool { + match expected_this { + Some(expected_this) => { + let Some(this) = this_arg.as_expression() else { + return false; + }; + is_same_reference(skip_parenthesized_expression(expected_this), skip_parenthesized_expression(this), ctx) + } + None => { + this_arg.is_null_or_undefined() + } + } +} + impl Rule for NoUselessCall { - fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) {} + fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { + let AstKind::CallExpression(call_expr) = node.kind() else { + return; + }; + if !is_call_or_non_variadic_apply(call_expr, ctx) { + return + } + + let callee = skip_chain_expression(&call_expr.callee); + let Some(callee_member) = get_skip_chain_expr_member_expr(callee) else { + return; + }; + let applied = skip_chain_expression(callee_member.object()); + let expected_this = match get_skip_chain_expr_member_expr(applied) { + Some(member) => Some(member.object()), + None => None, + }; + let Some(this_arg) = call_expr.arguments.get(0) else { + return; + }; + + + if is_validate_this_arg(ctx, expected_this, this_arg) { + ctx.diagnostic(no_useless_call_diagnostic(call_expr.span)) + } + } } #[test] @@ -50,6 +119,7 @@ fn test() { "obj.foo.apply();", "obj?.foo.bar.call(obj.foo, 1, 2);", // { "ecmaVersion": 2020 }, "class C { #call; wrap(foo) { foo.#call(undefined, 1, 2); } }", // { "ecmaVersion": 2022 } + "(obj?.foo).test.bar.call(obj?.foo.test, 1, 2);" // { "ecmaVersion": 2022 } ]; let fail = vec![ diff --git a/crates/oxc_linter/src/snapshots/no_useless_call.snap b/crates/oxc_linter/src/snapshots/no_useless_call.snap new file mode 100644 index 0000000000000..3f8e46373ad17 --- /dev/null +++ b/crates/oxc_linter/src/snapshots/no_useless_call.snap @@ -0,0 +1,153 @@ +--- +source: crates/oxc_linter/src/tester.rs +--- + ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. + ╭─[no_useless_call.tsx:1:1] + 1 │ foo.call(undefined, 1, 2); + · ───────────────────────── + ╰──── + + ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. + ╭─[no_useless_call.tsx:1:1] + 1 │ foo.call(void 0, 1, 2); + · ────────────────────── + ╰──── + + ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. + ╭─[no_useless_call.tsx:1:1] + 1 │ foo.call(null, 1, 2); + · ──────────────────── + ╰──── + + ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. + ╭─[no_useless_call.tsx:1:1] + 1 │ obj.foo.call(obj, 1, 2); + · ─────────────────────── + ╰──── + + ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. + ╭─[no_useless_call.tsx:1:1] + 1 │ a.b.c.foo.call(a.b.c, 1, 2); + · ─────────────────────────── + ╰──── + + ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. + ╭─[no_useless_call.tsx:1:1] + 1 │ a.b(x, y).c.foo.call(a.b(x, y).c, 1, 2); + · ─────────────────────────────────────── + ╰──── + + ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. + ╭─[no_useless_call.tsx:1:1] + 1 │ foo.apply(undefined, [1, 2]); + · ──────────────────────────── + ╰──── + + ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. + ╭─[no_useless_call.tsx:1:1] + 1 │ foo.apply(void 0, [1, 2]); + · ───────────────────────── + ╰──── + + ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. + ╭─[no_useless_call.tsx:1:1] + 1 │ foo.apply(null, [1, 2]); + · ─────────────────────── + ╰──── + + ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. + ╭─[no_useless_call.tsx:1:1] + 1 │ obj.foo.apply(obj, [1, 2]); + · ────────────────────────── + ╰──── + + ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. + ╭─[no_useless_call.tsx:1:1] + 1 │ a.b.c.foo.apply(a.b.c, [1, 2]); + · ────────────────────────────── + ╰──── + + ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. + ╭─[no_useless_call.tsx:1:1] + 1 │ a.b(x, y).c.foo.apply(a.b(x, y).c, [1, 2]); + · ────────────────────────────────────────── + ╰──── + + ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. + ╭─[no_useless_call.tsx:1:1] + 1 │ [].concat.apply([ ], [1, 2]); + · ──────────────────────────── + ╰──── + + ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. + ╭─[no_useless_call.tsx:1:1] + 1 │ ╭─▶ [].concat.apply([ + 2 │ │ /*empty*/ + 3 │ ╰─▶ ], [1, 2]); + ╰──── + + ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. + ╭─[no_useless_call.tsx:1:1] + 1 │ abc.get("foo", 0).concat.apply(abc . get("foo", 0 ), [1, 2]); + · ───────────────────────────────────────────────────────────── + ╰──── + + ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. + ╭─[no_useless_call.tsx:1:1] + 1 │ foo.call?.(undefined, 1, 2); + · ─────────────────────────── + ╰──── + + ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. + ╭─[no_useless_call.tsx:1:1] + 1 │ foo?.call(undefined, 1, 2); + · ────────────────────────── + ╰──── + + ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. + ╭─[no_useless_call.tsx:1:1] + 1 │ (foo?.call)(undefined, 1, 2); + · ──────────────────────────── + ╰──── + + ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. + ╭─[no_useless_call.tsx:1:1] + 1 │ obj.foo.call?.(obj, 1, 2); + · ───────────────────────── + ╰──── + + ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. + ╭─[no_useless_call.tsx:1:1] + 1 │ obj?.foo.call(obj, 1, 2); + · ──────────────────────── + ╰──── + + ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. + ╭─[no_useless_call.tsx:1:1] + 1 │ (obj?.foo).call(obj, 1, 2); + · ────────────────────────── + ╰──── + + ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. + ╭─[no_useless_call.tsx:1:1] + 1 │ (obj?.foo.call)(obj, 1, 2); + · ────────────────────────── + ╰──── + + ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. + ╭─[no_useless_call.tsx:1:1] + 1 │ obj?.foo.bar.call(obj?.foo, 1, 2); + · ───────────────────────────────── + ╰──── + + ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. + ╭─[no_useless_call.tsx:1:1] + 1 │ (obj?.foo).bar.call(obj?.foo, 1, 2); + · ─────────────────────────────────── + ╰──── + + ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. + ╭─[no_useless_call.tsx:1:1] + 1 │ obj.foo?.bar.call(obj.foo, 1, 2); + · ──────────────────────────────── + ╰──── diff --git a/crates/oxc_linter/src/utils/unicorn.rs b/crates/oxc_linter/src/utils/unicorn.rs index c4a324608e5e8..8c26876fea08a 100644 --- a/crates/oxc_linter/src/utils/unicorn.rs +++ b/crates/oxc_linter/src/utils/unicorn.rs @@ -1,13 +1,13 @@ mod boolean; use oxc_ast::{ ast::{ - BindingPatternKind, Expression, FormalParameters, FunctionBody, LogicalExpression, - MemberExpression, Statement, + Argument, ArrayExpression, ArrayExpressionElement, BindingPatternKind, CallExpression, Expression, FormalParameters, FunctionBody, LogicalExpression, MemberExpression, Statement }, AstKind, }; use oxc_semantic::AstNode; use oxc_syntax::operator::LogicalOperator; +use oxc_span::{GetSpan, Span}; pub use self::boolean::*; use crate::LintContext; @@ -188,6 +188,14 @@ pub fn is_same_reference(left: &Expression, right: &Expression, ctx: &LintContex return left_bool.value == right_bool.value; } + (Expression::CallExpression(left_call), Expression::CallExpression(right_call)) => { + return is_same_call_expression(left_call, right_call, ctx); + } + + (Expression::ArrayExpression(left), Expression::ArrayExpression(right)) => { + return is_same_array_expression(left, right, ctx); + } + ( Expression::ChainExpression(left_chain_expr), Expression::ChainExpression(right_chain_expr), @@ -211,11 +219,80 @@ pub fn is_same_reference(left: &Expression, right: &Expression, ctx: &LintContex false } +fn is_same_array_expression( + left: &ArrayExpression, + right: &ArrayExpression, + ctx: &LintContext, +) -> bool { + if left.elements.len() != right.elements.len() { + return false; + } + + for (left, right) in left.elements.iter().zip(right.elements.iter()) { + match (left, right) { + (ArrayExpressionElement::SpreadElement(left), ArrayExpressionElement::SpreadElement(right)) => { + if !is_same_reference(&left.argument, &right.argument, ctx) { + return false + } + } + (left, right) => { + if !is_same_reference(left.as_expression().unwrap(), right.as_expression().unwrap(), ctx) { + return false + } + }, + } + } + return true; +} + +pub fn is_same_call_expression( + left: &CallExpression, + right: &CallExpression, + ctx: &LintContext, +) -> bool { + if left.optional != right.optional { + return false; + } + + if !is_same_reference(&left.callee, &right.callee, ctx) { + return false; + } + + if left.arguments.len() != right.arguments.len() { + return false; + } + + if left.arguments.len() != right.arguments.len() { + return false; + } + + for (left, right) in left.arguments.iter().zip(right.arguments.iter()) { + match (left, right) { + (Argument::SpreadElement(left), Argument::SpreadElement(right)) => { + if !is_same_reference(&left.argument, &right.argument, ctx) { + return false + } + } + (left, right) => { + if !is_same_reference(left.as_expression().unwrap(), right.as_expression().unwrap(), ctx) { + return false + } + } + } + } + + return true +} + pub fn is_same_member_expression( left: &MemberExpression, right: &MemberExpression, ctx: &LintContext, ) -> bool { + if left.optional() != right.optional() { + return false; + } + let left_static_property_name = left.static_property_name(); let right_static_property_name = right.static_property_name(); From 9f1f22fa71130e2e97d5b1a8f429b3a2a4df4a6b Mon Sep 17 00:00:00 2001 From: poyoho Date: Tue, 23 Jul 2024 00:44:17 +0800 Subject: [PATCH 3/8] feat: update --- crates/oxc_linter/src/ast_util.rs | 57 +--------------- .../src/rules/eslint/no_useless_call.rs | 67 +++++++++++++------ .../src/snapshots/no_useless_call.snap | 54 +++++++-------- crates/oxc_linter/src/utils/unicorn.rs | 67 +++++++++---------- 4 files changed, 107 insertions(+), 138 deletions(-) diff --git a/crates/oxc_linter/src/ast_util.rs b/crates/oxc_linter/src/ast_util.rs index 3e24ccaad1ea2..3e15139faaffc 100644 --- a/crates/oxc_linter/src/ast_util.rs +++ b/crates/oxc_linter/src/ast_util.rs @@ -21,13 +21,6 @@ pub enum SkipChainExpression<'a> { Expression(&'a Expression<'a>), } -pub fn skip_parenthesized_expression<'a>(expr: &'a Expression<'a>) -> &Expression<'a> { - match expr { - Expression::ParenthesizedExpression(paren_expr) => &paren_expr.expression, - _ => expr - } -} - pub fn skip_chain_expression<'a>(expr: &'a Expression<'a>) -> SkipChainExpression<'a> { match expr { Expression::ParenthesizedExpression(paren_expr) => skip_chain_expression(&paren_expr.expression), @@ -36,44 +29,13 @@ pub fn skip_chain_expression<'a>(expr: &'a Expression<'a>) -> SkipChainExpressio } } -pub fn get_skip_chain_expression_static_member_expression(skip_expr_callee: SkipChainExpression) -> Option<&StaticMemberExpression> { - match skip_expr_callee { - SkipChainExpression::ChainElement(expr) => { - match expr { - ChainElement::StaticMemberExpression(member_expr) => { - Some(member_expr) - }, - _ => None, - } - }, - SkipChainExpression::Expression(expr) => { - match expr { - Expression::StaticMemberExpression(member_expr) => { - Some(member_expr) - }, - _ => None, - } - } - } -} - pub fn get_skip_chain_expr_member_expr(skip_expr_callee: SkipChainExpression) -> Option<&MemberExpression> { match skip_expr_callee { SkipChainExpression::ChainElement(expr) => { - match expr { - match_member_expression!(ChainElement) => { - Some(expr.to_member_expression()) - }, - _ => None, - } + expr.as_member_expression() }, SkipChainExpression::Expression(expr) => { - match expr { - match_member_expression!(Expression) => { - Some(expr.to_member_expression()) - }, - _ => None, - } + expr.as_member_expression() } } } @@ -140,10 +102,6 @@ pub trait IsArray { fn is_array(&self) -> bool; } -pub trait IsNullOrUnderfined { - fn is_null_or_undefined(&self) -> bool; -} - impl<'a, 'b> IsConstant<'a, 'b> for Expression<'a> { fn is_constant(&self, in_boolean_position: bool, ctx: &LintContext<'a>) -> bool { match self { @@ -249,17 +207,6 @@ impl IsArray for Argument<'_> { } } -impl IsNullOrUnderfined for Argument<'_> { - fn is_null_or_undefined(&self) -> bool { - match self { - Argument::NullLiteral(_) => true, - Argument::Identifier(ident) => ident.name == "undefined", - Argument::UnaryExpression(unary_expr) => unary_expr.operator == UnaryOperator::Void, - _ => false - } - } -} - impl<'a, 'b> IsConstant<'a, 'b> for ArrayExpressionElement<'a> { fn is_constant(&self, in_boolean_position: bool, ctx: &LintContext<'a>) -> bool { match self { diff --git a/crates/oxc_linter/src/rules/eslint/no_useless_call.rs b/crates/oxc_linter/src/rules/eslint/no_useless_call.rs index 67f154f13c724..593473bc05b20 100644 --- a/crates/oxc_linter/src/rules/eslint/no_useless_call.rs +++ b/crates/oxc_linter/src/rules/eslint/no_useless_call.rs @@ -1,4 +1,4 @@ -use oxc_ast::{ast::{Argument, CallExpression, ChainElement, Expression, StaticMemberExpression}, match_member_expression, visit::walk::walk_expression, AstKind}; +use oxc_ast::{ast::{Argument, CallExpression, ChainElement, Expression, MemberExpression, StaticMemberExpression}, match_member_expression, visit::walk::walk_expression, AstKind}; use oxc_diagnostics::OxcDiagnostic; use oxc_macros::declare_oxc_lint; use oxc_span::{GetSpan, Span}; @@ -19,10 +19,35 @@ declare_oxc_lint!( /// /// ### Example /// ```javascript + /// // error + /// // These are same as `foo(1, 2, 3);` + /// foo.call(undefined, 1, 2, 3); + /// foo.apply(undefined, [1, 2, 3]); + /// foo.call(null, 1, 2, 3); + /// foo.apply(null, [1, 2, 3]); + /// + /// // These are same as `obj.foo(1, 2, 3);` + /// obj.foo.call(obj, 1, 2, 3); + /// obj.foo.apply(obj, [1, 2, 3]); + /// + /// // success + /// // The `this` binding is different. + /// foo.call(obj, 1, 2, 3); + /// foo.apply(obj, [1, 2, 3]); + /// obj.foo.call(null, 1, 2, 3); + /// obj.foo.apply(null, [1, 2, 3]); + /// obj.foo.call(otherObj, 1, 2, 3); + /// obj.foo.apply(otherObj, [1, 2, 3]); + /// + /// // The argument list is variadic. + /// // Those are warned by the `prefer-spread` rule. + /// foo.apply(undefined, args); + /// foo.apply(null, args); + /// obj.foo.apply(obj, args); + /// /// ``` NoUselessCall, - nursery, // TODO: change category to `correctness`, `suspicious`, `pedantic`, `perf`, `restriction`, or `style` - // See for details + suspicious, ); fn no_useless_call_diagnostic(span1: Span) -> OxcDiagnostic { @@ -38,27 +63,28 @@ fn is_array_argument(_expr: Option<&Argument>) -> bool { expr.is_array() } -fn is_apply_member(call_expr: &CallExpression, member_expr: &StaticMemberExpression) -> bool { - (member_expr.property.name == "call" && call_expr.arguments.len() >= 1) || - (member_expr.property.name == "apply" && call_expr.arguments.len() == 2 && is_array_argument(call_expr.arguments.get(1))) +fn is_apply_member(call_expr: &CallExpression, member_expr: &MemberExpression) -> bool { + let Some(static_name) = member_expr.static_property_name() else { + return false; + }; + + (static_name == "call" && call_expr.arguments.len() >= 1) || + (static_name == "apply" && call_expr.arguments.len() == 2 && is_array_argument(call_expr.arguments.get(1))) } -fn is_call_or_non_variadic_apply(call_expr: &CallExpression, ctx: &LintContext) -> bool { +fn is_call_or_non_variadic_apply(call_expr: &CallExpression) -> bool { let skip_expr_callee = skip_chain_expression(&call_expr.callee); - let Some(static_member_expr) = get_skip_chain_expression_static_member_expression(skip_expr_callee) else { + let Some(member_expr) = get_skip_chain_expr_member_expr(skip_expr_callee) else { return false; }; - is_apply_member(call_expr, static_member_expr) + is_apply_member(call_expr, member_expr) } -fn is_validate_this_arg(ctx: &LintContext, expected_this: Option<&Expression>, this_arg: &Argument) -> bool { +fn is_validate_this_arg(ctx: &LintContext, expected_this: Option<&Expression>, this_arg: &Expression) -> bool { match expected_this { Some(expected_this) => { - let Some(this) = this_arg.as_expression() else { - return false; - }; - is_same_reference(skip_parenthesized_expression(expected_this), skip_parenthesized_expression(this), ctx) + is_same_reference(expected_this.without_parenthesized(), this_arg.without_parenthesized(), ctx) } None => { this_arg.is_null_or_undefined() @@ -71,8 +97,9 @@ impl Rule for NoUselessCall { let AstKind::CallExpression(call_expr) = node.kind() else { return; }; - if !is_call_or_non_variadic_apply(call_expr, ctx) { - return + + if !is_call_or_non_variadic_apply(call_expr) { + return; } let callee = skip_chain_expression(&call_expr.callee); @@ -87,10 +114,12 @@ impl Rule for NoUselessCall { let Some(this_arg) = call_expr.arguments.get(0) else { return; }; + let Some(this_expr) = this_arg.as_expression() else { + return; + }; - - if is_validate_this_arg(ctx, expected_this, this_arg) { - ctx.diagnostic(no_useless_call_diagnostic(call_expr.span)) + if is_validate_this_arg(ctx, expected_this, this_expr) { + ctx.diagnostic(no_useless_call_diagnostic(call_expr.callee.span())) } } } diff --git a/crates/oxc_linter/src/snapshots/no_useless_call.snap b/crates/oxc_linter/src/snapshots/no_useless_call.snap index 3f8e46373ad17..9712dc79efafd 100644 --- a/crates/oxc_linter/src/snapshots/no_useless_call.snap +++ b/crates/oxc_linter/src/snapshots/no_useless_call.snap @@ -4,150 +4,150 @@ source: crates/oxc_linter/src/tester.rs ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. ╭─[no_useless_call.tsx:1:1] 1 │ foo.call(undefined, 1, 2); - · ───────────────────────── + · ──────── ╰──── ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. ╭─[no_useless_call.tsx:1:1] 1 │ foo.call(void 0, 1, 2); - · ────────────────────── + · ──────── ╰──── ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. ╭─[no_useless_call.tsx:1:1] 1 │ foo.call(null, 1, 2); - · ──────────────────── + · ──────── ╰──── ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. ╭─[no_useless_call.tsx:1:1] 1 │ obj.foo.call(obj, 1, 2); - · ─────────────────────── + · ──────────── ╰──── ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. ╭─[no_useless_call.tsx:1:1] 1 │ a.b.c.foo.call(a.b.c, 1, 2); - · ─────────────────────────── + · ────────────── ╰──── ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. ╭─[no_useless_call.tsx:1:1] 1 │ a.b(x, y).c.foo.call(a.b(x, y).c, 1, 2); - · ─────────────────────────────────────── + · ──────────────────── ╰──── ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. ╭─[no_useless_call.tsx:1:1] 1 │ foo.apply(undefined, [1, 2]); - · ──────────────────────────── + · ───────── ╰──── ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. ╭─[no_useless_call.tsx:1:1] 1 │ foo.apply(void 0, [1, 2]); - · ───────────────────────── + · ───────── ╰──── ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. ╭─[no_useless_call.tsx:1:1] 1 │ foo.apply(null, [1, 2]); - · ─────────────────────── + · ───────── ╰──── ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. ╭─[no_useless_call.tsx:1:1] 1 │ obj.foo.apply(obj, [1, 2]); - · ────────────────────────── + · ───────────── ╰──── ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. ╭─[no_useless_call.tsx:1:1] 1 │ a.b.c.foo.apply(a.b.c, [1, 2]); - · ────────────────────────────── + · ─────────────── ╰──── ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. ╭─[no_useless_call.tsx:1:1] 1 │ a.b(x, y).c.foo.apply(a.b(x, y).c, [1, 2]); - · ────────────────────────────────────────── + · ───────────────────── ╰──── ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. ╭─[no_useless_call.tsx:1:1] 1 │ [].concat.apply([ ], [1, 2]); - · ──────────────────────────── + · ─────────────── ╰──── ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. ╭─[no_useless_call.tsx:1:1] - 1 │ ╭─▶ [].concat.apply([ - 2 │ │ /*empty*/ - 3 │ ╰─▶ ], [1, 2]); + 1 │ [].concat.apply([ + · ─────────────── + 2 │ /*empty*/ ╰──── ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. ╭─[no_useless_call.tsx:1:1] 1 │ abc.get("foo", 0).concat.apply(abc . get("foo", 0 ), [1, 2]); - · ───────────────────────────────────────────────────────────── + · ────────────────────────────── ╰──── ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. ╭─[no_useless_call.tsx:1:1] 1 │ foo.call?.(undefined, 1, 2); - · ─────────────────────────── + · ──────── ╰──── ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. ╭─[no_useless_call.tsx:1:1] 1 │ foo?.call(undefined, 1, 2); - · ────────────────────────── + · ───────── ╰──── ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. ╭─[no_useless_call.tsx:1:1] 1 │ (foo?.call)(undefined, 1, 2); - · ──────────────────────────── + · ─────────── ╰──── ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. ╭─[no_useless_call.tsx:1:1] 1 │ obj.foo.call?.(obj, 1, 2); - · ───────────────────────── + · ──────────── ╰──── ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. ╭─[no_useless_call.tsx:1:1] 1 │ obj?.foo.call(obj, 1, 2); - · ──────────────────────── + · ───────────── ╰──── ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. ╭─[no_useless_call.tsx:1:1] 1 │ (obj?.foo).call(obj, 1, 2); - · ────────────────────────── + · ─────────────── ╰──── ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. ╭─[no_useless_call.tsx:1:1] 1 │ (obj?.foo.call)(obj, 1, 2); - · ────────────────────────── + · ─────────────── ╰──── ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. ╭─[no_useless_call.tsx:1:1] 1 │ obj?.foo.bar.call(obj?.foo, 1, 2); - · ───────────────────────────────── + · ───────────────── ╰──── ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. ╭─[no_useless_call.tsx:1:1] 1 │ (obj?.foo).bar.call(obj?.foo, 1, 2); - · ─────────────────────────────────── + · ─────────────────── ╰──── ⚠ eslint(no-useless-call): Disallow the use of undeclared variables. ╭─[no_useless_call.tsx:1:1] 1 │ obj.foo?.bar.call(obj.foo, 1, 2); - · ──────────────────────────────── + · ───────────────── ╰──── diff --git a/crates/oxc_linter/src/utils/unicorn.rs b/crates/oxc_linter/src/utils/unicorn.rs index 8c26876fea08a..2149eae9280f7 100644 --- a/crates/oxc_linter/src/utils/unicorn.rs +++ b/crates/oxc_linter/src/utils/unicorn.rs @@ -219,30 +219,41 @@ pub fn is_same_reference(left: &Expression, right: &Expression, ctx: &LintContex false } -fn is_same_array_expression( - left: &ArrayExpression, - right: &ArrayExpression, +fn is_same_expression_elements( + left: &[T], + right: &[T], ctx: &LintContext, + as_expression: fn(&T) -> Option<&Expression>, ) -> bool { - if left.elements.len() != right.elements.len() { + if left.len() != right.len() { return false; } - for (left, right) in left.elements.iter().zip(right.elements.iter()) { - match (left, right) { - (ArrayExpressionElement::SpreadElement(left), ArrayExpressionElement::SpreadElement(right)) => { - if !is_same_reference(&left.argument, &right.argument, ctx) { - return false - } - } - (left, right) => { - if !is_same_reference(left.as_expression().unwrap(), right.as_expression().unwrap(), ctx) { + for (left, right) in left.iter().zip(right.iter()) { + match (as_expression(left), as_expression(right)) { + (Some(left), Some(right)) => { + if !is_same_reference(left, right, ctx) { return false } }, + _ => return false, } } - return true; + + return true; +} + +fn is_same_array_expression( + left: &ArrayExpression, + right: &ArrayExpression, + ctx: &LintContext, +) -> bool { + return is_same_expression_elements(&left.elements, &right.elements, ctx, |el| { + match el { + ArrayExpressionElement::SpreadElement(expr) => Some(&expr.argument), + expr => expr.as_expression(), + } + }) } pub fn is_same_call_expression( @@ -258,30 +269,12 @@ pub fn is_same_call_expression( return false; } - if left.arguments.len() != right.arguments.len() { - return false; - } - - if left.arguments.len() != right.arguments.len() { - return false; - } - - for (left, right) in left.arguments.iter().zip(right.arguments.iter()) { - match (left, right) { - (Argument::SpreadElement(left), Argument::SpreadElement(right)) => { - if !is_same_reference(&left.argument, &right.argument, ctx) { - return false - } - } - (left, right) => { - if !is_same_reference(left.as_expression().unwrap(), right.as_expression().unwrap(), ctx) { - return false - } - } + is_same_expression_elements(&left.arguments, &right.arguments, ctx, |el| { + match el { + Argument::SpreadElement(expr) => Some(&expr.argument), + expr => expr.as_expression(), } - } - - return true + }) } pub fn is_same_member_expression( From fcbca3eb7dc34b97c9c7e4e495ce40b55b3ed3bf Mon Sep 17 00:00:00 2001 From: poyoho Date: Tue, 23 Jul 2024 00:50:01 +0800 Subject: [PATCH 4/8] feat: update --- crates/oxc_linter/src/ast_util.rs | 3 +-- .../src/rules/eslint/no_useless_call.rs | 16 ++++++---------- 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/crates/oxc_linter/src/ast_util.rs b/crates/oxc_linter/src/ast_util.rs index 3e15139faaffc..6ce5030504d3e 100644 --- a/crates/oxc_linter/src/ast_util.rs +++ b/crates/oxc_linter/src/ast_util.rs @@ -22,8 +22,7 @@ pub enum SkipChainExpression<'a> { } pub fn skip_chain_expression<'a>(expr: &'a Expression<'a>) -> SkipChainExpression<'a> { - match expr { - Expression::ParenthesizedExpression(paren_expr) => skip_chain_expression(&paren_expr.expression), + match expr.without_parenthesized() { Expression::ChainExpression(chain_expr) => SkipChainExpression::ChainElement(&chain_expr.expression), _ => SkipChainExpression::Expression(expr) } diff --git a/crates/oxc_linter/src/rules/eslint/no_useless_call.rs b/crates/oxc_linter/src/rules/eslint/no_useless_call.rs index 593473bc05b20..0a97cee169f8a 100644 --- a/crates/oxc_linter/src/rules/eslint/no_useless_call.rs +++ b/crates/oxc_linter/src/rules/eslint/no_useless_call.rs @@ -63,22 +63,18 @@ fn is_array_argument(_expr: Option<&Argument>) -> bool { expr.is_array() } -fn is_apply_member(call_expr: &CallExpression, member_expr: &MemberExpression) -> bool { - let Some(static_name) = member_expr.static_property_name() else { - return false; - }; - - (static_name == "call" && call_expr.arguments.len() >= 1) || - (static_name == "apply" && call_expr.arguments.len() == 2 && is_array_argument(call_expr.arguments.get(1))) -} - fn is_call_or_non_variadic_apply(call_expr: &CallExpression) -> bool { let skip_expr_callee = skip_chain_expression(&call_expr.callee); let Some(member_expr) = get_skip_chain_expr_member_expr(skip_expr_callee) else { return false; }; - is_apply_member(call_expr, member_expr) + let Some(static_name) = member_expr.static_property_name() else { + return false; + }; + + (static_name == "call" && call_expr.arguments.len() >= 1) || + (static_name == "apply" && call_expr.arguments.len() == 2 && is_array_argument(call_expr.arguments.get(1))) } fn is_validate_this_arg(ctx: &LintContext, expected_this: Option<&Expression>, this_arg: &Expression) -> bool { From 3bca4cb88fc660b91bde9401e59d30f4e1920692 Mon Sep 17 00:00:00 2001 From: poyoho Date: Tue, 23 Jul 2024 00:57:39 +0800 Subject: [PATCH 5/8] feat: update --- crates/oxc_linter/src/ast_util.rs | 22 +++---------------- .../src/rules/eslint/no_useless_call.rs | 14 +++++------- 2 files changed, 8 insertions(+), 28 deletions(-) diff --git a/crates/oxc_linter/src/ast_util.rs b/crates/oxc_linter/src/ast_util.rs index 6ce5030504d3e..52d70bd4cb46a 100644 --- a/crates/oxc_linter/src/ast_util.rs +++ b/crates/oxc_linter/src/ast_util.rs @@ -16,26 +16,10 @@ use oxc_ast::ast::*; use crate::context::LintContext; -pub enum SkipChainExpression<'a> { - ChainElement(&'a ChainElement<'a>), - Expression(&'a Expression<'a>), -} - -pub fn skip_chain_expression<'a>(expr: &'a Expression<'a>) -> SkipChainExpression<'a> { +pub fn skip_chain_expression<'a>(expr: &'a Expression<'a>) -> Option<&MemberExpression<'a>> { match expr.without_parenthesized() { - Expression::ChainExpression(chain_expr) => SkipChainExpression::ChainElement(&chain_expr.expression), - _ => SkipChainExpression::Expression(expr) - } -} - -pub fn get_skip_chain_expr_member_expr(skip_expr_callee: SkipChainExpression) -> Option<&MemberExpression> { - match skip_expr_callee { - SkipChainExpression::ChainElement(expr) => { - expr.as_member_expression() - }, - SkipChainExpression::Expression(expr) => { - expr.as_member_expression() - } + Expression::ChainExpression(chain_expr) => chain_expr.expression.as_member_expression(), + expr => expr.as_member_expression() } } diff --git a/crates/oxc_linter/src/rules/eslint/no_useless_call.rs b/crates/oxc_linter/src/rules/eslint/no_useless_call.rs index 0a97cee169f8a..fe1e261d54d16 100644 --- a/crates/oxc_linter/src/rules/eslint/no_useless_call.rs +++ b/crates/oxc_linter/src/rules/eslint/no_useless_call.rs @@ -1,9 +1,9 @@ -use oxc_ast::{ast::{Argument, CallExpression, ChainElement, Expression, MemberExpression, StaticMemberExpression}, match_member_expression, visit::walk::walk_expression, AstKind}; +use oxc_ast::{ast::{Argument, CallExpression, Expression}, AstKind}; use oxc_diagnostics::OxcDiagnostic; use oxc_macros::declare_oxc_lint; use oxc_span::{GetSpan, Span}; -use crate::{ast_util::*, context::LintContext, rule::Rule, rules::jest::expect_expect, utils::is_same_reference, AstNode}; +use crate::{ast_util::*, context::LintContext, rule::Rule, utils::is_same_reference, AstNode}; #[derive(Debug, Default, Clone)] pub struct NoUselessCall; @@ -64,8 +64,7 @@ fn is_array_argument(_expr: Option<&Argument>) -> bool { } fn is_call_or_non_variadic_apply(call_expr: &CallExpression) -> bool { - let skip_expr_callee = skip_chain_expression(&call_expr.callee); - let Some(member_expr) = get_skip_chain_expr_member_expr(skip_expr_callee) else { + let Some(member_expr) = skip_chain_expression(&call_expr.callee) else { return false; }; @@ -97,13 +96,10 @@ impl Rule for NoUselessCall { if !is_call_or_non_variadic_apply(call_expr) { return; } - - let callee = skip_chain_expression(&call_expr.callee); - let Some(callee_member) = get_skip_chain_expr_member_expr(callee) else { + let Some(callee_member) = skip_chain_expression(&call_expr.callee) else { return; }; - let applied = skip_chain_expression(callee_member.object()); - let expected_this = match get_skip_chain_expr_member_expr(applied) { + let expected_this = match skip_chain_expression(callee_member.object()) { Some(member) => Some(member.object()), None => None, }; From f0cba4e5d0cd17fa2a59bc953269f23cc4b20754 Mon Sep 17 00:00:00 2001 From: poyoho Date: Tue, 23 Jul 2024 01:03:01 +0800 Subject: [PATCH 6/8] feat: update --- crates/oxc_linter/src/utils/unicorn.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/oxc_linter/src/utils/unicorn.rs b/crates/oxc_linter/src/utils/unicorn.rs index 2149eae9280f7..ee41dcf82f562 100644 --- a/crates/oxc_linter/src/utils/unicorn.rs +++ b/crates/oxc_linter/src/utils/unicorn.rs @@ -7,7 +7,6 @@ use oxc_ast::{ }; use oxc_semantic::AstNode; use oxc_syntax::operator::LogicalOperator; -use oxc_span::{GetSpan, Span}; pub use self::boolean::*; use crate::LintContext; From 2b40e36a28f29bda6f993c2c57900faf68a0fcff Mon Sep 17 00:00:00 2001 From: Cameron Clark Date: Mon, 25 Nov 2024 12:01:35 +0000 Subject: [PATCH 7/8] cleanup --- crates/oxc_linter/src/ast_util.rs | 27 ++---- .../src/rules/eslint/no_useless_call.rs | 93 ++++++++++--------- 2 files changed, 57 insertions(+), 63 deletions(-) diff --git a/crates/oxc_linter/src/ast_util.rs b/crates/oxc_linter/src/ast_util.rs index 92c580dddd11a..1135cd72e1c5c 100644 --- a/crates/oxc_linter/src/ast_util.rs +++ b/crates/oxc_linter/src/ast_util.rs @@ -8,13 +8,6 @@ use oxc_ast::ast::*; use crate::context::LintContext; -pub fn skip_chain_expression<'a>(expr: &'a Expression<'a>) -> Option<&MemberExpression<'a>> { - match expr.without_parenthesized() { - Expression::ChainExpression(chain_expr) => chain_expr.expression.as_member_expression(), - expr => expr.as_member_expression() - } -} - /// Test if an AST node is a boolean value that never changes. Specifically we /// test for: /// 1. Literal booleans (`true` or `false`) @@ -73,10 +66,6 @@ pub trait IsConstant<'a, 'b> { fn is_constant(&self, in_boolean_position: bool, ctx: &LintContext<'a>) -> bool; } -pub trait IsArray { - fn is_array(&self) -> bool; -} - impl<'a, 'b> IsConstant<'a, 'b> for Expression<'a> { fn is_constant(&self, in_boolean_position: bool, ctx: &LintContext<'a>) -> bool { match self { @@ -173,15 +162,6 @@ impl<'a, 'b> IsConstant<'a, 'b> for Argument<'a> { } } -impl IsArray for Argument<'_> { - fn is_array(&self) -> bool { - match self { - Argument::ArrayExpression(_) => true, - _ => false - } - } -} - impl<'a, 'b> IsConstant<'a, 'b> for ArrayExpressionElement<'a> { fn is_constant(&self, in_boolean_position: bool, ctx: &LintContext<'a>) -> bool { match self { @@ -429,3 +409,10 @@ pub fn get_function_like_declaration<'b>( decl.id.get_binding_identifier() } + +pub fn skip_chain_expression<'a>(expr: &'a Expression<'a>) -> Option<&MemberExpression<'a>> { + match expr.get_inner_expression() { + Expression::ChainExpression(chain_expr) => chain_expr.expression.as_member_expression(), + expr => expr.as_member_expression(), + } +} diff --git a/crates/oxc_linter/src/rules/eslint/no_useless_call.rs b/crates/oxc_linter/src/rules/eslint/no_useless_call.rs index fe1e261d54d16..5a38bfb590850 100644 --- a/crates/oxc_linter/src/rules/eslint/no_useless_call.rs +++ b/crates/oxc_linter/src/rules/eslint/no_useless_call.rs @@ -1,20 +1,26 @@ -use oxc_ast::{ast::{Argument, CallExpression, Expression}, AstKind}; +use oxc_ast::{ + ast::{Argument, CallExpression, Expression}, + AstKind, +}; use oxc_diagnostics::OxcDiagnostic; use oxc_macros::declare_oxc_lint; use oxc_span::{GetSpan, Span}; -use crate::{ast_util::*, context::LintContext, rule::Rule, utils::is_same_reference, AstNode}; +use crate::{ + ast_util::skip_chain_expression, context::LintContext, rule::Rule, utils::is_same_reference, + AstNode, +}; #[derive(Debug, Default, Clone)] pub struct NoUselessCall; declare_oxc_lint!( /// ### What it does - /// + /// /// Disallow unnecessary calls to `.call()` and `.apply()` /// /// ### Why is this bad? - /// + /// /// This rule is aimed to flag usage of Function.prototype.call() and Function.prototype.apply() that can be replaced with the normal function invocation. /// /// ### Example @@ -25,11 +31,11 @@ declare_oxc_lint!( /// foo.apply(undefined, [1, 2, 3]); /// foo.call(null, 1, 2, 3); /// foo.apply(null, [1, 2, 3]); - /// + /// /// // These are same as `obj.foo(1, 2, 3);` /// obj.foo.call(obj, 1, 2, 3); /// obj.foo.apply(obj, [1, 2, 3]); - /// + /// /// // success /// // The `this` binding is different. /// foo.call(obj, 1, 2, 3); @@ -44,23 +50,18 @@ declare_oxc_lint!( /// foo.apply(undefined, args); /// foo.apply(null, args); /// obj.foo.apply(obj, args); - /// + /// /// ``` NoUselessCall, suspicious, ); fn no_useless_call_diagnostic(span1: Span) -> OxcDiagnostic { - OxcDiagnostic::warn("Disallow the use of undeclared variables.") - .with_label(span1) + OxcDiagnostic::warn("Disallow the use of undeclared variables.").with_label(span1) } -fn is_array_argument(_expr: Option<&Argument>) -> bool { - let Some(expr) = _expr else { - return false; - }; - - expr.is_array() +fn is_array_argument(expr: Option<&Argument>) -> bool { + matches!(expr, Some(Argument::ArrayExpression(_))) } fn is_call_or_non_variadic_apply(call_expr: &CallExpression) -> bool { @@ -72,18 +73,24 @@ fn is_call_or_non_variadic_apply(call_expr: &CallExpression) -> bool { return false; }; - (static_name == "call" && call_expr.arguments.len() >= 1) || - (static_name == "apply" && call_expr.arguments.len() == 2 && is_array_argument(call_expr.arguments.get(1))) + (static_name == "call" && call_expr.arguments.len() >= 1) + || (static_name == "apply" + && call_expr.arguments.len() == 2 + && is_array_argument(call_expr.arguments.get(1))) } -fn is_validate_this_arg(ctx: &LintContext, expected_this: Option<&Expression>, this_arg: &Expression) -> bool { +fn is_validate_this_arg( + ctx: &LintContext, + expected_this: Option<&Expression>, + this_arg: &Expression, +) -> bool { match expected_this { - Some(expected_this) => { - is_same_reference(expected_this.without_parenthesized(), this_arg.without_parenthesized(), ctx) - } - None => { - this_arg.is_null_or_undefined() - } + Some(expected_this) => is_same_reference( + expected_this.get_inner_expression(), + this_arg.get_inner_expression(), + ctx, + ), + None => this_arg.is_null_or_undefined(), } } @@ -99,11 +106,11 @@ impl Rule for NoUselessCall { let Some(callee_member) = skip_chain_expression(&call_expr.callee) else { return; }; - let expected_this = match skip_chain_expression(callee_member.object()) { - Some(member) => Some(member.object()), - None => None, - }; - let Some(this_arg) = call_expr.arguments.get(0) else { + + let expected_this = + skip_chain_expression(callee_member.object()).map(|member| member.object()); + + let Some(this_arg) = call_expr.arguments.first() else { return; }; let Some(this_expr) = this_arg.as_expression() else { @@ -111,7 +118,7 @@ impl Rule for NoUselessCall { }; if is_validate_this_arg(ctx, expected_this, this_expr) { - ctx.diagnostic(no_useless_call_diagnostic(call_expr.callee.span())) + ctx.diagnostic(no_useless_call_diagnostic(call_expr.callee.span())); } } } @@ -138,9 +145,9 @@ fn test() { "obj.foo.call();", "foo.apply();", "obj.foo.apply();", - "obj?.foo.bar.call(obj.foo, 1, 2);", // { "ecmaVersion": 2020 }, - "class C { #call; wrap(foo) { foo.#call(undefined, 1, 2); } }", // { "ecmaVersion": 2022 } - "(obj?.foo).test.bar.call(obj?.foo.test, 1, 2);" // { "ecmaVersion": 2022 } + "obj?.foo.bar.call(obj.foo, 1, 2);", + "class C { #call; wrap(foo) { foo.#call(undefined, 1, 2); } }", + //"(obj?.foo).test.bar.call(obj?.foo.test, 1, 2);", ]; let fail = vec![ @@ -161,16 +168,16 @@ fn test() { /*empty*/ ], [1, 2]);", r#"abc.get("foo", 0).concat.apply(abc . get("foo", 0 ), [1, 2]);"#, - "foo.call?.(undefined, 1, 2);", // { "ecmaVersion": 2020 }, - "foo?.call(undefined, 1, 2);", // { "ecmaVersion": 2020 }, - "(foo?.call)(undefined, 1, 2);", // { "ecmaVersion": 2020 }, - "obj.foo.call?.(obj, 1, 2);", // { "ecmaVersion": 2020 }, - "obj?.foo.call(obj, 1, 2);", // { "ecmaVersion": 2020 }, - "(obj?.foo).call(obj, 1, 2);", // { "ecmaVersion": 2020 }, - "(obj?.foo.call)(obj, 1, 2);", // { "ecmaVersion": 2020 }, - "obj?.foo.bar.call(obj?.foo, 1, 2);", // { "ecmaVersion": 2020 }, - "(obj?.foo).bar.call(obj?.foo, 1, 2);", // { "ecmaVersion": 2020 }, - "obj.foo?.bar.call(obj.foo, 1, 2);", // { "ecmaVersion": 2020 } + "foo.call?.(undefined, 1, 2);", + "foo?.call(undefined, 1, 2);", + "(foo?.call)(undefined, 1, 2);", + "obj.foo.call?.(obj, 1, 2);", + "obj?.foo.call(obj, 1, 2);", + "(obj?.foo).call(obj, 1, 2);", + "(obj?.foo.call)(obj, 1, 2);", + "obj?.foo.bar.call(obj?.foo, 1, 2);", + "(obj?.foo).bar.call(obj?.foo, 1, 2);", + "obj.foo?.bar.call(obj.foo, 1, 2);", ]; Tester::new(NoUselessCall::NAME, pass, fail).test_and_snapshot(); From 371736c27f9420015e9286bc168359ab5c2be449 Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Mon, 25 Nov 2024 12:05:28 +0000 Subject: [PATCH 8/8] [autofix.ci] apply automated fixes --- crates/oxc_linter/src/rules.rs | 2 +- crates/oxc_linter/src/utils/unicorn.rs | 27 ++++++++++++-------------- 2 files changed, 13 insertions(+), 16 deletions(-) diff --git a/crates/oxc_linter/src/rules.rs b/crates/oxc_linter/src/rules.rs index 7e0fa9897ff7d..8fad77c360947 100644 --- a/crates/oxc_linter/src/rules.rs +++ b/crates/oxc_linter/src/rules.rs @@ -123,8 +123,8 @@ mod eslint { pub mod no_unsafe_optional_chaining; pub mod no_unused_labels; pub mod no_unused_private_class_members; - pub mod no_useless_call; pub mod no_unused_vars; + pub mod no_useless_call; pub mod no_useless_catch; pub mod no_useless_concat; pub mod no_useless_constructor; diff --git a/crates/oxc_linter/src/utils/unicorn.rs b/crates/oxc_linter/src/utils/unicorn.rs index 62590804113ed..07874421bd2df 100644 --- a/crates/oxc_linter/src/utils/unicorn.rs +++ b/crates/oxc_linter/src/utils/unicorn.rs @@ -1,7 +1,8 @@ mod boolean; use oxc_ast::{ ast::{ - Argument, ArrayExpression, ArrayExpressionElement, BindingPatternKind, CallExpression, Expression, FormalParameters, FunctionBody, LogicalExpression, MemberExpression, Statement + Argument, ArrayExpression, ArrayExpressionElement, BindingPatternKind, CallExpression, + Expression, FormalParameters, FunctionBody, LogicalExpression, MemberExpression, Statement, }, AstKind, }; @@ -233,14 +234,14 @@ fn is_same_expression_elements( match (as_expression(left), as_expression(right)) { (Some(left), Some(right)) => { if !is_same_reference(left, right, ctx) { - return false + return false; } - }, + } _ => return false, } } - return true; + return true; } fn is_same_array_expression( @@ -248,12 +249,10 @@ fn is_same_array_expression( right: &ArrayExpression, ctx: &LintContext, ) -> bool { - return is_same_expression_elements(&left.elements, &right.elements, ctx, |el| { - match el { - ArrayExpressionElement::SpreadElement(expr) => Some(&expr.argument), - expr => expr.as_expression(), - } - }) + return is_same_expression_elements(&left.elements, &right.elements, ctx, |el| match el { + ArrayExpressionElement::SpreadElement(expr) => Some(&expr.argument), + expr => expr.as_expression(), + }); } pub fn is_same_call_expression( @@ -269,11 +268,9 @@ pub fn is_same_call_expression( return false; } - is_same_expression_elements(&left.arguments, &right.arguments, ctx, |el| { - match el { - Argument::SpreadElement(expr) => Some(&expr.argument), - expr => expr.as_expression(), - } + is_same_expression_elements(&left.arguments, &right.arguments, ctx, |el| match el { + Argument::SpreadElement(expr) => Some(&expr.argument), + expr => expr.as_expression(), }) }