Skip to content

Commit

Permalink
feat(linter): implement no-return-assign (#6108)
Browse files Browse the repository at this point in the history
This PR implements the not-recomennded eslint rule `no-return-assign`
#479

---------

Co-authored-by: Cameron <cameron.clark@hey.com>
  • Loading branch information
radu2147 and camc314 authored Sep 27, 2024
1 parent 7bc3988 commit ae539af
Show file tree
Hide file tree
Showing 3 changed files with 312 additions and 0 deletions.
2 changes: 2 additions & 0 deletions crates/oxc_linter/src/rules.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand Down
199 changes: 199 additions & 0 deletions crates/oxc_linter/src/rules/eslint/no_return_assign.rs
Original file line number Diff line number Diff line change
@@ -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();
}
111 changes: 111 additions & 0 deletions crates/oxc_linter/src/snapshots/no_return_assign.snap
Original file line number Diff line number Diff line change
@@ -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]
1function x() { return result = a * b; };
· ──────────────────────
╰────

eslint(no-return-assign): Return statement should not contain an assignment.
╭─[no_return_assign.tsx:1:16]
1function x() { return (result) = (a * b); };
· ──────────────────────────
╰────

eslint(no-return-assign): Return statement should not contain an assignment.
╭─[no_return_assign.tsx:1:16]
1function x() { return result = a * b; };
· ──────────────────────
╰────

eslint(no-return-assign): Return statement should not contain an assignment.
╭─[no_return_assign.tsx:1:16]
1function 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]
1function x() { return (result = a * b); };
· ────────────────────────
╰────

eslint(no-return-assign): Return statement should not contain an assignment.
╭─[no_return_assign.tsx:1:16]
1function x() { return result || (result = a * b); };
· ──────────────────────────────────
╰────

eslint(no-return-assign): Return statement should not contain an assignment.
╭─[no_return_assign.tsx:2:20]
1function foo(){
2return a = b
· ────────────
3 │ }
╰────

eslint(no-return-assign): Return statement should not contain an assignment.
╭─[no_return_assign.tsx:2:20]
1function doSomething() {
2return foo = bar && foo > 0;
· ────────────────────────────
3 │ }
╰────

eslint(no-return-assign): Return statement should not contain an assignment.
╭─[no_return_assign.tsx:2:20]
1function 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]
1function doSomething() {
2return foo = () => a
· ────────────────────
3 │ }
╰────

eslint(no-return-assign): Arrow function should not return an assignment.
╭─[no_return_assign.tsx:2:27]
1 │ function doSomething() {
2return () => a = () => b
· ─────────────────
3 │ }
╰────

eslint(no-return-assign): Return statement should not contain an assignment.
╭─[no_return_assign.tsx:3:24]
2return function bar(b){
3return 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
· ────────────
╰────

0 comments on commit ae539af

Please sign in to comment.