feat(linter): implement eslint-plugin-promise/no-return-in-finally, prefer-await-to-then rule (#4318)

Part of #4252

---------

Co-authored-by: wenzhe <[email protected]>

Author: jelly

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,

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();
+}

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() }
+			    }",
+        // <https://github.com/tc39/proposal-top-level-await>
+        // 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();
+}

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

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())
+   · ───────────────────────────────────────────────────
+   ╰────