From 5992b7575e67671690aefcc0cf2be681f4be1e35 Mon Sep 17 00:00:00 2001 From: Jelle van der Waa Date: Sat, 10 Aug 2024 04:41:29 +0200 Subject: [PATCH] feat(linter): implement `eslint-plugin-promise/no-return-in-finally, prefer-await-to-then` rule (#4318) Part of https://github.com/oxc-project/oxc/pull/4252 --------- Co-authored-by: wenzhe --- crates/oxc_linter/src/rules.rs | 4 + .../src/rules/promise/no_return_in_finally.rs | 109 ++++++++++++++++++ .../src/rules/promise/prefer_await_to_then.rs | 94 +++++++++++++++ .../src/snapshots/no_return_in_finally.snap | 30 +++++ .../src/snapshots/prefer_await_to_then.snap | 68 +++++++++++ 5 files changed, 305 insertions(+) create mode 100644 crates/oxc_linter/src/rules/promise/no_return_in_finally.rs create mode 100644 crates/oxc_linter/src/rules/promise/prefer_await_to_then.rs create mode 100644 crates/oxc_linter/src/snapshots/no_return_in_finally.snap create mode 100644 crates/oxc_linter/src/snapshots/prefer_await_to_then.snap diff --git a/crates/oxc_linter/src/rules.rs b/crates/oxc_linter/src/rules.rs index 97b466c5ad2d2..344112dce1395 100644 --- a/crates/oxc_linter/src/rules.rs +++ b/crates/oxc_linter/src/rules.rs @@ -444,7 +444,9 @@ mod tree_shaking { mod promise { pub mod avoid_new; pub mod no_new_statics; + pub mod no_return_in_finally; pub mod param_names; + pub mod prefer_await_to_then; pub mod valid_params; } @@ -857,6 +859,8 @@ oxc_macros::declare_all_lint_rules! { promise::no_new_statics, promise::param_names, promise::valid_params, + promise::no_return_in_finally, + promise::prefer_await_to_then, vitest::no_import_node_test, vitest::prefer_to_be_falsy, vitest::prefer_to_be_truthy, diff --git a/crates/oxc_linter/src/rules/promise/no_return_in_finally.rs b/crates/oxc_linter/src/rules/promise/no_return_in_finally.rs new file mode 100644 index 0000000000000..35f5d3ee2c963 --- /dev/null +++ b/crates/oxc_linter/src/rules/promise/no_return_in_finally.rs @@ -0,0 +1,109 @@ +use oxc_allocator::Box as OBox; +use oxc_ast::{ + ast::{Expression, FunctionBody, Statement}, + AstKind, +}; +use oxc_diagnostics::OxcDiagnostic; +use oxc_macros::declare_oxc_lint; +use oxc_span::Span; + +use crate::{context::LintContext, rule::Rule, utils::is_promise, AstNode}; + +fn no_return_in_finally_diagnostic(span0: Span) -> OxcDiagnostic { + OxcDiagnostic::warn("Don't return in a finally callback") + .with_help("Remove the return statement as nothing can consume the return value") + .with_label(span0) +} + +#[derive(Debug, Default, Clone)] +pub struct NoReturnInFinally; + +declare_oxc_lint!( + /// ### What it does + /// + /// Disallow return statements in a finally() callback of a promise. + /// + /// ### Why is this bad? + /// + /// Disallow return statements inside a callback passed to finally(), since nothing would + /// consume what's returned. + /// + /// ### Example + /// ```javascript + /// myPromise.finally(function (val) { + /// return val + /// }) + /// ``` + NoReturnInFinally, + nursery, +); + +impl Rule for NoReturnInFinally { + fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { + let AstKind::CallExpression(call_expr) = node.kind() else { + return; + }; + + let Some(prop_name) = is_promise(call_expr) else { + return; + }; + + if prop_name != "finally" { + return; + } + + for argument in &call_expr.arguments { + let Some(arg_expr) = argument.as_expression() else { + continue; + }; + match arg_expr { + Expression::ArrowFunctionExpression(arrow_expr) => { + find_return_statement(&arrow_expr.body, ctx); + } + Expression::FunctionExpression(func_expr) => { + let Some(func_body) = &func_expr.body else { + continue; + }; + find_return_statement(func_body, ctx); + } + _ => continue, + } + } + } +} + +fn find_return_statement<'a>(func_body: &OBox<'_, FunctionBody<'a>>, ctx: &LintContext<'a>) { + let Some(return_stmt) = + func_body.statements.iter().find(|stmt| matches!(stmt, Statement::ReturnStatement(_))) + else { + return; + }; + + let Statement::ReturnStatement(stmt) = return_stmt else { + return; + }; + + ctx.diagnostic(no_return_in_finally_diagnostic(stmt.span)); +} + +#[test] +fn test() { + use crate::tester::Tester; + + let pass = vec![ + "Promise.resolve(1).finally(() => { console.log(2) })", + "Promise.reject(4).finally(() => { console.log(2) })", + "Promise.reject(4).finally(() => {})", + "myPromise.finally(() => {});", + "Promise.resolve(1).finally(function () { })", + ]; + + let fail = vec![ + "Promise.resolve(1).finally(() => { return 2 })", + "Promise.reject(0).finally(() => { return 2 })", + "myPromise.finally(() => { return 2 });", + "Promise.resolve(1).finally(function () { return 2 })", + ]; + + Tester::new(NoReturnInFinally::NAME, pass, fail).test_and_snapshot(); +} diff --git a/crates/oxc_linter/src/rules/promise/prefer_await_to_then.rs b/crates/oxc_linter/src/rules/promise/prefer_await_to_then.rs new file mode 100644 index 0000000000000..4705cad013ea3 --- /dev/null +++ b/crates/oxc_linter/src/rules/promise/prefer_await_to_then.rs @@ -0,0 +1,94 @@ +use oxc_ast::AstKind; +use oxc_diagnostics::OxcDiagnostic; +use oxc_macros::declare_oxc_lint; +use oxc_span::Span; + +fn prefer_wait_to_then_diagnostic(span0: Span) -> OxcDiagnostic { + OxcDiagnostic::warn("Prefer await to then()/catch()/finally()").with_label(span0) +} + +use crate::{context::LintContext, rule::Rule, utils::is_promise, AstNode}; + +#[derive(Debug, Default, Clone)] +pub struct PreferAwaitToThen; + +declare_oxc_lint!( + /// ### What it does + /// + /// Prefer `await` to `then()`/`catch()`/`finally()` for reading Promise values + /// + /// ### Why is this bad? + /// + /// Async/await syntax can be seen as more readable. + /// + /// ### Example + /// ```javascript + /// myPromise.then(doSomething) + /// ``` + PreferAwaitToThen, + style, +); + +fn is_inside_yield_or_await(node: &AstNode) -> bool { + matches!(node.kind(), AstKind::YieldExpression(_) | AstKind::AwaitExpression(_)) +} + +impl Rule for PreferAwaitToThen { + fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { + let AstKind::CallExpression(call_expr) = node.kind() else { + return; + }; + + if is_promise(call_expr).is_none() { + return; + } + + // Already inside a yield or await + if ctx + .nodes() + .ancestors(node.id()) + .any(|node_id| is_inside_yield_or_await(ctx.nodes().get_node(node_id))) + { + return; + } + + ctx.diagnostic(prefer_wait_to_then_diagnostic(call_expr.span)); + } +} + +#[test] +fn test() { + use crate::tester::Tester; + + let pass = vec![ + "async function hi() { await thing() }", + "async function hi() { await thing().then() }", + "async function hi() { await thing().catch() }", + "a = async () => (await something())", + "a = async () => { + try { await something() } catch (error) { somethingElse() } + }", + // + // Top level await is allowed now, so comment this out + // "something().then(async () => await somethingElse())", + "function foo() { hey.somethingElse(x => {}) }", + "const isThenable = (obj) => { + return obj && typeof obj.then === 'function'; + };", + "function isThenable(obj) { + return obj && typeof obj.then === 'function'; + }", + ]; + + let fail = vec![ + "function foo() { hey.then(x => {}) }", + "function foo() { hey.then(function() { }).then() }", + "function foo() { hey.then(function() { }).then(x).catch() }", + "async function a() { hey.then(function() { }).then(function() { }) }", + "function foo() { hey.catch(x => {}) }", + "function foo() { hey.finally(x => {}) }", + "something().then(async () => await somethingElse())", + ]; + + Tester::new(PreferAwaitToThen::NAME, pass, fail).test_and_snapshot(); +} diff --git a/crates/oxc_linter/src/snapshots/no_return_in_finally.snap b/crates/oxc_linter/src/snapshots/no_return_in_finally.snap new file mode 100644 index 0000000000000..59c520cd9aade --- /dev/null +++ b/crates/oxc_linter/src/snapshots/no_return_in_finally.snap @@ -0,0 +1,30 @@ +--- +source: crates/oxc_linter/src/tester.rs +--- + ⚠ eslint-plugin-promise(no-return-in-finally): Don't return in a finally callback + ╭─[no_return_in_finally.tsx:1:36] + 1 │ Promise.resolve(1).finally(() => { return 2 }) + · ──────── + ╰──── + help: Remove the return statement as nothing can consume the return value + + ⚠ eslint-plugin-promise(no-return-in-finally): Don't return in a finally callback + ╭─[no_return_in_finally.tsx:1:35] + 1 │ Promise.reject(0).finally(() => { return 2 }) + · ──────── + ╰──── + help: Remove the return statement as nothing can consume the return value + + ⚠ eslint-plugin-promise(no-return-in-finally): Don't return in a finally callback + ╭─[no_return_in_finally.tsx:1:27] + 1 │ myPromise.finally(() => { return 2 }); + · ──────── + ╰──── + help: Remove the return statement as nothing can consume the return value + + ⚠ eslint-plugin-promise(no-return-in-finally): Don't return in a finally callback + ╭─[no_return_in_finally.tsx:1:42] + 1 │ Promise.resolve(1).finally(function () { return 2 }) + · ──────── + ╰──── + help: Remove the return statement as nothing can consume the return value diff --git a/crates/oxc_linter/src/snapshots/prefer_await_to_then.snap b/crates/oxc_linter/src/snapshots/prefer_await_to_then.snap new file mode 100644 index 0000000000000..40651ad2ea5b1 --- /dev/null +++ b/crates/oxc_linter/src/snapshots/prefer_await_to_then.snap @@ -0,0 +1,68 @@ +--- +source: crates/oxc_linter/src/tester.rs +--- + ⚠ eslint-plugin-promise(prefer-await-to-then): Prefer await to then()/catch()/finally() + ╭─[prefer_await_to_then.tsx:1:18] + 1 │ function foo() { hey.then(x => {}) } + · ───────────────── + ╰──── + + ⚠ eslint-plugin-promise(prefer-await-to-then): Prefer await to then()/catch()/finally() + ╭─[prefer_await_to_then.tsx:1:18] + 1 │ function foo() { hey.then(function() { }).then() } + · ─────────────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(prefer-await-to-then): Prefer await to then()/catch()/finally() + ╭─[prefer_await_to_then.tsx:1:18] + 1 │ function foo() { hey.then(function() { }).then() } + · ──────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(prefer-await-to-then): Prefer await to then()/catch()/finally() + ╭─[prefer_await_to_then.tsx:1:18] + 1 │ function foo() { hey.then(function() { }).then(x).catch() } + · ──────────────────────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(prefer-await-to-then): Prefer await to then()/catch()/finally() + ╭─[prefer_await_to_then.tsx:1:18] + 1 │ function foo() { hey.then(function() { }).then(x).catch() } + · ──────────────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(prefer-await-to-then): Prefer await to then()/catch()/finally() + ╭─[prefer_await_to_then.tsx:1:18] + 1 │ function foo() { hey.then(function() { }).then(x).catch() } + · ──────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(prefer-await-to-then): Prefer await to then()/catch()/finally() + ╭─[prefer_await_to_then.tsx:1:22] + 1 │ async function a() { hey.then(function() { }).then(function() { }) } + · ───────────────────────────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(prefer-await-to-then): Prefer await to then()/catch()/finally() + ╭─[prefer_await_to_then.tsx:1:22] + 1 │ async function a() { hey.then(function() { }).then(function() { }) } + · ──────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(prefer-await-to-then): Prefer await to then()/catch()/finally() + ╭─[prefer_await_to_then.tsx:1:18] + 1 │ function foo() { hey.catch(x => {}) } + · ────────────────── + ╰──── + + ⚠ eslint-plugin-promise(prefer-await-to-then): Prefer await to then()/catch()/finally() + ╭─[prefer_await_to_then.tsx:1:18] + 1 │ function foo() { hey.finally(x => {}) } + · ──────────────────── + ╰──── + + ⚠ eslint-plugin-promise(prefer-await-to-then): Prefer await to then()/catch()/finally() + ╭─[prefer_await_to_then.tsx:1:1] + 1 │ something().then(async () => await somethingElse()) + · ─────────────────────────────────────────────────── + ╰────