From ae539af6758752684b712b46944cde555185962d Mon Sep 17 00:00:00 2001 From: Radu Baston <65868820+radu2147@users.noreply.github.com> Date: Fri, 27 Sep 2024 23:52:18 +0300 Subject: [PATCH] feat(linter): implement no-return-assign (#6108) This PR implements the not-recomennded eslint rule `no-return-assign` https://github.com/oxc-project/oxc/issues/479 --------- Co-authored-by: Cameron --- crates/oxc_linter/src/rules.rs | 2 + .../src/rules/eslint/no_return_assign.rs | 199 ++++++++++++++++++ .../src/snapshots/no_return_assign.snap | 111 ++++++++++ 3 files changed, 312 insertions(+) create mode 100644 crates/oxc_linter/src/rules/eslint/no_return_assign.rs create mode 100644 crates/oxc_linter/src/snapshots/no_return_assign.snap diff --git a/crates/oxc_linter/src/rules.rs b/crates/oxc_linter/src/rules.rs index 9e8996a03b156..84482610c6259 100644 --- a/crates/oxc_linter/src/rules.rs +++ b/crates/oxc_linter/src/rules.rs @@ -97,6 +97,7 @@ mod eslint { pub mod no_redeclare; pub mod no_regex_spaces; pub mod no_restricted_globals; + pub mod no_return_assign; pub mod no_script_url; pub mod no_self_assign; pub mod no_self_compare; @@ -563,6 +564,7 @@ oxc_macros::declare_all_lint_rules! { eslint::no_redeclare, eslint::no_regex_spaces, eslint::no_restricted_globals, + eslint::no_return_assign, eslint::no_script_url, eslint::no_self_assign, eslint::no_self_compare, diff --git a/crates/oxc_linter/src/rules/eslint/no_return_assign.rs b/crates/oxc_linter/src/rules/eslint/no_return_assign.rs new file mode 100644 index 0000000000000..3952bfcd80d53 --- /dev/null +++ b/crates/oxc_linter/src/rules/eslint/no_return_assign.rs @@ -0,0 +1,199 @@ +use oxc_ast::AstKind; +use oxc_diagnostics::OxcDiagnostic; +use oxc_macros::declare_oxc_lint; +use oxc_span::{GetSpan, Span}; +use serde_json::Value; + +use crate::{context::LintContext, rule::Rule, AstNode}; + +fn no_return_assign_diagnostic(span: Span, message: &str) -> OxcDiagnostic { + OxcDiagnostic::warn(message.to_string()).with_label(span) +} + +#[derive(Debug, Default, Clone)] +pub struct NoReturnAssign { + always_disallow_assignment_in_return: bool, +} + +declare_oxc_lint!( + /// ### What it does + /// Disallows assignment operators in return statements + /// + /// ### Why is this bad? + /// Assignment is allowed by js in return expressions, but usually, an expression with only one equal sign is intended to be a comparison. + /// However, because of the missing equal sign, this turns to assignment, which is valid js code + /// Because of this ambiguity, it’s considered a best practice to not use assignment in return statements. + /// + /// ### Examples + /// + /// Examples of **incorrect** code for this rule: + /// ```js + /// () => a = b; + /// function x() { return a = b; } + /// ``` + /// + /// Examples of **correct** code for this rule: + /// ```js + /// () => (a = b) + /// function x() { var result = a = b; return result; } + /// ``` + NoReturnAssign, + style, + suggestion +); + +fn is_sentinel_node(ast_kind: AstKind) -> bool { + (ast_kind.is_statement() && !matches!(&ast_kind, AstKind::ExpressionStatement(_))) + || matches!( + &ast_kind, + AstKind::ArrowFunctionExpression(_) | AstKind::Function(_) | AstKind::Class(_) + ) +} + +impl Rule for NoReturnAssign { + fn from_configuration(value: Value) -> Self { + let always_disallow_assignment_in_return = value + .get(0) + .and_then(Value::as_str) + .map_or_else(|| false, |value| value != "except-parens"); + Self { always_disallow_assignment_in_return } + } + + fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { + if let AstKind::AssignmentExpression(_) = node.kind() { + if !self.always_disallow_assignment_in_return + && ctx + .nodes() + .parent_node(node.id()) + .is_some_and(|node| node.kind().as_parenthesized_expression().is_some()) + { + return; + } + let mut parent_node = ctx.nodes().parent_node(node.id()); + while parent_node.is_some_and(|parent| !is_sentinel_node(parent.kind())) { + parent_node = ctx.nodes().parent_node(parent_node.unwrap().id()); + } + if let Some(parent) = parent_node { + match parent.kind() { + AstKind::ReturnStatement(_) => { + ctx.diagnostic(no_return_assign_diagnostic( + parent.span(), + "Return statement should not contain an assignment.", + )); + } + AstKind::ArrowFunctionExpression(_) => { + ctx.diagnostic(no_return_assign_diagnostic( + parent.span(), + "Arrow function should not return an assignment.", + )); + } + _ => (), + } + } + } + } +} + +#[test] +fn test() { + use crate::tester::Tester; + + let pass = vec![ + ("module.exports = {'a': 1};", None), // { "sourceType": "module" }, + ("var result = a * b;", None), + ("function x() { var result = a * b; return result; }", None), + ("function x() { return (result = a * b); }", None), + ( + "function x() { var result = a * b; return result; }", + Some(serde_json::json!(["except-parens"])), + ), + ("function x() { return (result = a * b); }", Some(serde_json::json!(["except-parens"]))), + ( + "function x() { var result = a * b; return result; }", + Some(serde_json::json!(["always"])), + ), + ( + "function x() { return function y() { result = a * b }; }", + Some(serde_json::json!(["always"])), + ), + ("() => { return (result = a * b); }", Some(serde_json::json!(["except-parens"]))), + ("() => (result = a * b)", Some(serde_json::json!(["except-parens"]))), + ("const foo = (a,b,c) => ((a = b), c)", None), + ( + "function foo(){ + return (a = b) + }", + None, + ), + ( + "function bar(){ + return function foo(){ + return (a = b) && c + } + }", + None, + ), + ("const foo = (a) => (b) => (a = b)", None), // { "ecmaVersion": 6 } + ]; + + let fail = vec![ + ("function x() { return result = a * b; };", None), + ("function x() { return (result) = (a * b); };", None), + ("function x() { return result = a * b; };", Some(serde_json::json!(["except-parens"]))), + ( + "function x() { return (result) = (a * b); };", + Some(serde_json::json!(["except-parens"])), + ), + ("() => { return result = a * b; }", None), + ("() => result = a * b", None), + ("function x() { return result = a * b; };", Some(serde_json::json!(["always"]))), + ("function x() { return (result = a * b); };", Some(serde_json::json!(["always"]))), + ( + "function x() { return result || (result = a * b); };", + Some(serde_json::json!(["always"])), + ), + ( + "function foo(){ + return a = b + }", + None, + ), + ( + "function doSomething() { + return foo = bar && foo > 0; + }", + None, + ), + ( + "function doSomething() { + return foo = function(){ + return (bar = bar1) + } + }", + None, + ), + ( + "function doSomething() { + return foo = () => a + }", + None, + ), // { "ecmaVersion": 6 }, + ( + "function doSomething() { + return () => a = () => b + }", + None, + ), // { "ecmaVersion": 6 }, + ( + "function foo(a){ + return function bar(b){ + return a = b + } + }", + None, + ), + ("const foo = (a) => (b) => a = b", None), // { "ecmaVersion": 6 } + ]; + + Tester::new(NoReturnAssign::NAME, pass, fail).test_and_snapshot(); +} diff --git a/crates/oxc_linter/src/snapshots/no_return_assign.snap b/crates/oxc_linter/src/snapshots/no_return_assign.snap new file mode 100644 index 0000000000000..c014e8f4a9f0b --- /dev/null +++ b/crates/oxc_linter/src/snapshots/no_return_assign.snap @@ -0,0 +1,111 @@ +--- +source: crates/oxc_linter/src/tester.rs +--- + ⚠ eslint(no-return-assign): Return statement should not contain an assignment. + ╭─[no_return_assign.tsx:1:16] + 1 │ function x() { return result = a * b; }; + · ────────────────────── + ╰──── + + ⚠ eslint(no-return-assign): Return statement should not contain an assignment. + ╭─[no_return_assign.tsx:1:16] + 1 │ function x() { return (result) = (a * b); }; + · ────────────────────────── + ╰──── + + ⚠ eslint(no-return-assign): Return statement should not contain an assignment. + ╭─[no_return_assign.tsx:1:16] + 1 │ function x() { return result = a * b; }; + · ────────────────────── + ╰──── + + ⚠ eslint(no-return-assign): Return statement should not contain an assignment. + ╭─[no_return_assign.tsx:1:16] + 1 │ function x() { return (result) = (a * b); }; + · ────────────────────────── + ╰──── + + ⚠ eslint(no-return-assign): Return statement should not contain an assignment. + ╭─[no_return_assign.tsx:1:9] + 1 │ () => { return result = a * b; } + · ────────────────────── + ╰──── + + ⚠ eslint(no-return-assign): Arrow function should not return an assignment. + ╭─[no_return_assign.tsx:1:1] + 1 │ () => result = a * b + · ──────────────────── + ╰──── + + ⚠ eslint(no-return-assign): Return statement should not contain an assignment. + ╭─[no_return_assign.tsx:1:16] + 1 │ function x() { return result = a * b; }; + · ────────────────────── + ╰──── + + ⚠ eslint(no-return-assign): Return statement should not contain an assignment. + ╭─[no_return_assign.tsx:1:16] + 1 │ function x() { return (result = a * b); }; + · ──────────────────────── + ╰──── + + ⚠ eslint(no-return-assign): Return statement should not contain an assignment. + ╭─[no_return_assign.tsx:1:16] + 1 │ function x() { return result || (result = a * b); }; + · ────────────────────────────────── + ╰──── + + ⚠ eslint(no-return-assign): Return statement should not contain an assignment. + ╭─[no_return_assign.tsx:2:20] + 1 │ function foo(){ + 2 │ return a = b + · ──────────── + 3 │ } + ╰──── + + ⚠ eslint(no-return-assign): Return statement should not contain an assignment. + ╭─[no_return_assign.tsx:2:20] + 1 │ function doSomething() { + 2 │ return foo = bar && foo > 0; + · ──────────────────────────── + 3 │ } + ╰──── + + ⚠ eslint(no-return-assign): Return statement should not contain an assignment. + ╭─[no_return_assign.tsx:2:20] + 1 │ function doSomething() { + 2 │ ╭─▶ return foo = function(){ + 3 │ │ return (bar = bar1) + 4 │ ╰─▶ } + 5 │ } + ╰──── + + ⚠ eslint(no-return-assign): Return statement should not contain an assignment. + ╭─[no_return_assign.tsx:2:20] + 1 │ function doSomething() { + 2 │ return foo = () => a + · ──────────────────── + 3 │ } + ╰──── + + ⚠ eslint(no-return-assign): Arrow function should not return an assignment. + ╭─[no_return_assign.tsx:2:27] + 1 │ function doSomething() { + 2 │ return () => a = () => b + · ───────────────── + 3 │ } + ╰──── + + ⚠ eslint(no-return-assign): Return statement should not contain an assignment. + ╭─[no_return_assign.tsx:3:24] + 2 │ return function bar(b){ + 3 │ return a = b + · ──────────── + 4 │ } + ╰──── + + ⚠ eslint(no-return-assign): Arrow function should not return an assignment. + ╭─[no_return_assign.tsx:1:20] + 1 │ const foo = (a) => (b) => a = b + · ──────────── + ╰────