From b867e5f16b204d0fc0204e1d4e8f4e3b51d29a55 Mon Sep 17 00:00:00 2001 From: Jelle van der Waa Date: Sat, 31 Aug 2024 18:04:06 +0200 Subject: [PATCH] feat(linter/eslint-plugin-promise): implement catch-or-return (#5121) Rule detail: [link](https://github.com/eslint-community/eslint-plugin-promise/blob/main/docs/rules/catch-or-return.md) Co-authored-by: Don Isaac --- crates/oxc_linter/src/rules.rs | 2 + .../src/rules/promise/catch_or_return.rs | 343 ++++++++++++++++++ .../src/snapshots/catch_or_return.snap | 170 +++++++++ 3 files changed, 515 insertions(+) create mode 100644 crates/oxc_linter/src/rules/promise/catch_or_return.rs create mode 100644 crates/oxc_linter/src/snapshots/catch_or_return.snap diff --git a/crates/oxc_linter/src/rules.rs b/crates/oxc_linter/src/rules.rs index 8c3aef2e2ae36..57fb4f819d7dc 100644 --- a/crates/oxc_linter/src/rules.rs +++ b/crates/oxc_linter/src/rules.rs @@ -450,6 +450,7 @@ mod tree_shaking { mod promise { pub mod avoid_new; + pub mod catch_or_return; pub mod no_new_statics; pub mod no_return_in_finally; pub mod param_names; @@ -879,6 +880,7 @@ oxc_macros::declare_all_lint_rules! { promise::valid_params, promise::no_return_in_finally, promise::prefer_await_to_then, + promise::catch_or_return, promise::spec_only, vitest::no_import_node_test, vitest::prefer_each, diff --git a/crates/oxc_linter/src/rules/promise/catch_or_return.rs b/crates/oxc_linter/src/rules/promise/catch_or_return.rs new file mode 100644 index 0000000000000..8a0effb9ce3f9 --- /dev/null +++ b/crates/oxc_linter/src/rules/promise/catch_or_return.rs @@ -0,0 +1,343 @@ +use oxc_ast::{ + ast::{CallExpression, Expression}, + AstKind, +}; +use oxc_diagnostics::OxcDiagnostic; +use oxc_macros::declare_oxc_lint; +use oxc_span::{CompactStr, Span}; + +use crate::{ + ast_util::is_method_call, context::LintContext, rule::Rule, utils::is_promise, AstNode, +}; + +fn catch_or_return_diagnostic(x0: &str, span0: Span) -> OxcDiagnostic { + OxcDiagnostic::warn(format!("eslint-plugin-promise(catch-or-return): Expected {x0} or return")) + .with_help(format!("Return the promise or chain a {x0}()")) + .with_label(span0) +} + +#[derive(Debug, Default, Clone)] +pub struct CatchOrReturn(Box); + +#[derive(Debug, Clone)] +pub struct CatchOrReturnConfig { + allow_finally: bool, + allow_then: bool, + termination_method: Vec, +} + +impl Default for CatchOrReturnConfig { + fn default() -> Self { + Self { + allow_finally: false, + allow_then: false, + termination_method: vec![CompactStr::new("catch")], + } + } +} + +impl std::ops::Deref for CatchOrReturn { + type Target = CatchOrReturnConfig; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +declare_oxc_lint!( + /// ### What it does + /// + /// Ensure that each time a then() is applied to a promise, a catch() is applied as well. + /// Exceptions are made if you are returning that promise. + /// + /// ### Why is this bad? + /// + /// Not catching errors in a promise can cause hard to debug problems or missing handling of + /// error conditions. + /// + /// ### Example + /// + /// Examples of **incorrect** code for this rule: + /// ```javascript + /// myPromise.then(doSomething) + /// myPromise.then(doSomething, catchErrors) // catch() may be a little better + /// ``` + /// + /// Examples of **correct** code for this rule: + /// ```javascript + /// myPromise.then(doSomething).catch(errors) + /// function doSomethingElse() { + /// return myPromise.then(doSomething) + /// } + /// ``` + CatchOrReturn, + restriction, +); + +impl Rule for CatchOrReturn { + fn from_configuration(value: serde_json::Value) -> Self { + let mut config = CatchOrReturnConfig::default(); + + if let Some(termination_array_config) = value + .get(0) + .and_then(|v| v.get("terminationMethod")) + .and_then(serde_json::Value::as_array) + { + config.termination_method = termination_array_config + .iter() + .filter_map(serde_json::Value::as_str) + .map(CompactStr::from) + .collect(); + } + + if let Some(termination_string_config) = value + .get(0) + .and_then(|v| v.get("terminationMethod")) + .and_then(serde_json::Value::as_str) + { + config.termination_method = vec![CompactStr::new(termination_string_config)]; + } + + if let Some(allow_finally_config) = + value.get(0).and_then(|v| v.get("allowFinally")).and_then(serde_json::Value::as_bool) + { + config.allow_finally = allow_finally_config; + } + + if let Some(allow_then_config) = + value.get(0).and_then(|v| v.get("allowThen")).and_then(serde_json::Value::as_bool) + { + config.allow_then = allow_then_config; + } + + Self(Box::new(config)) + } + + fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { + let AstKind::ExpressionStatement(expr_stmt) = node.kind() else { + return; + }; + + let Expression::CallExpression(call_expr) = &expr_stmt.expression else { + return; + }; + + // Check for a promise or a method call at the end of a promise for example: + // foo().catch().randomFunc() + if is_promise(call_expr).is_none() && !is_part_of_promise(call_expr) { + return; + } + + if self.is_allowed_promise_termination(call_expr) { + return; + } + + let termination_method = &self.termination_method[0]; + ctx.diagnostic(catch_or_return_diagnostic(termination_method, call_expr.span)); + } +} + +impl CatchOrReturn { + fn is_allowed_promise_termination(&self, call_expr: &CallExpression) -> bool { + let Some(member_expr) = call_expr.callee.get_member_expr() else { + return false; + }; + + let Some(prop_name) = member_expr.static_property_name() else { + return false; + }; + + // somePromise.then(a, b) + if self.allow_then && prop_name == "then" && call_expr.arguments.len() == 2 { + return true; + } + + // somePromise.catch().finally(fn) + if self.allow_finally && prop_name == "finally" { + let Expression::CallExpression(object_call_expr) = member_expr.object() else { + return false; + }; + + if is_promise(object_call_expr).is_some() + && self.is_allowed_promise_termination(object_call_expr) + { + return true; + } + } + + // somePromise.catch() + if self.termination_method.contains(&CompactStr::from(prop_name)) { + return true; + } + + // somePromise['catch']() + if prop_name == "catch" + && matches!(call_expr.callee.get_inner_expression(), Expression::StringLiteral(_)) + { + return true; + } + + let Expression::CallExpression(object_call_expr) = member_expr.object() else { + return false; + }; + + // cy.get().then(a, b) + is_cypress_call(object_call_expr) + } +} + +fn is_part_of_promise(call_expr: &CallExpression) -> bool { + let Some(member_expr) = call_expr.callee.get_member_expr() else { + return false; + }; + + let Expression::CallExpression(object_call_expr) = member_expr.object() else { + return false; + }; + + is_promise(object_call_expr).is_some() +} + +fn is_cypress_call(call_expr: &CallExpression) -> bool { + if is_method_call(call_expr, Some(&["cy"]), None, None, None) { + return true; + } + + let Some(member_expr) = call_expr.callee.get_member_expr() else { + return false; + }; + + let Expression::CallExpression(object_call_expr) = member_expr.object() else { + return false; + }; + + is_cypress_call(object_call_expr) +} + +#[test] +fn test() { + use crate::tester::Tester; + + let pass = vec![ + ("frank().then(go).catch(doIt)", None), + ("frank().then(go).then().then().then().catch(doIt)", None), + ("frank().then(go).then().catch(function() { /* why bother */ })", None), + ("frank.then(go).then(to).catch(jail)", None), + ("Promise.resolve(frank).catch(jail)", None), + (r#"Promise.resolve(frank)["catch"](jail)"#, None), + ("frank.then(to).finally(fn).catch(jail)", None), + ( + r#"postJSON("/smajobber/api/reportJob.json") + .then(()=>this.setState()) + .catch(()=>this.setState())"#, + None, + ), + ("function a() { return frank().then(go) }", None), + ("function a() { return frank().then(go).then().then().then() }", None), + ("function a() { return frank().then(go).then()}", None), + ("function a() { return frank.then(go).then(to) }", None), + ("frank().then(go).then(null, doIt)", Some(serde_json::json!([{ "allowThen": true }]))), + ( + "frank().then(go).then().then().then().then(null, doIt)", + Some(serde_json::json!([{ "allowThen": true }])), + ), + ( + "frank().then(go).then().then(null, function() { /* why bother */ })", + Some(serde_json::json!([{ "allowThen": true }])), + ), + ( + "frank.then(go).then(to).then(null, jail)", + Some(serde_json::json!([{ "allowThen": true }])), + ), + ("frank().then(a, b)", Some(serde_json::json!([{ "allowThen": true }]))), + ("frank().then(a).then(b).then(null, c)", Some(serde_json::json!([{ "allowThen": true }]))), + ("frank().then(a).then(b).then(c, d)", Some(serde_json::json!([{ "allowThen": true }]))), + ( + "frank().then(a).then(b).then().then().then(null, doIt)", + Some(serde_json::json!([{ "allowThen": true }])), + ), + ( + "frank().then(a).then(b).then(null, function() { /* why bother */ })", + Some(serde_json::json!([{ "allowThen": true }])), + ), + ("frank().then(go).then(zam, doIt)", Some(serde_json::json!([{ "allowThen": true }]))), + ( + "frank().then(go).then().then().then().then(wham, doIt)", + Some(serde_json::json!([{ "allowThen": true }])), + ), + ( + "frank().then(go).then().then(function() {}, function() { /* why bother */ })", + Some(serde_json::json!([{ "allowThen": true }])), + ), + ( + "frank.then(go).then(to).then(pewPew, jail)", + Some(serde_json::json!([{ "allowThen": true }])), + ), + ( + "frank().then(go).catch(doIt).finally(fn)", + Some(serde_json::json!([{ "allowFinally": true }])), + ), + ( + "frank().then(go).then().then().then().catch(doIt).finally(fn)", + Some(serde_json::json!([{ "allowFinally": true }])), + ), + ( + "frank().then(go).then().catch(function() { /* why bother */ }).finally(fn)", + Some(serde_json::json!([{ "allowFinally": true }])), + ), + ("frank().then(goo).done()", Some(serde_json::json!([{ "terminationMethod": "done" }]))), + ( + "frank().then(go).catch()", + Some(serde_json::json!([{ "terminationMethod": ["catch", "done"] }])), + ), + ( + "frank().then(go).done()", + Some(serde_json::json!([{ "terminationMethod": ["catch", "done"] }])), + ), + ( + "frank().then(go).finally()", + Some(serde_json::json!([{ "terminationMethod": ["catch", "finally"] }])), + ), + ("nonPromiseExpressionStatement();", None), + ("frank().then(go)['catch']", None), + ("await foo().then(bar)", None), + // Cypress + ("cy.get('.myClass').then(go)", None), + ("cy.get('button').click().then()", None), + ]; + + let fail = vec![ + ("function callPromise(promise, cb) { promise.then(cb) }", None), + (r#"fetch("http://www.yahoo.com").then(console.log.bind(console))"#, None), + (r#"a.then(function() { return "x"; }).then(function(y) { throw y; })"#, None), + ("Promise.resolve(frank)", None), + ("Promise.all([])", None), + ("Promise.allSettled([])", None), + ("Promise.any([])", None), + ("Promise.race([])", None), + ("frank().then(to).catch(fn).then(foo)", None), + ("frank().finally(fn)", None), + ("frank().then(to).finally(fn)", None), + ("frank().then(go).catch(doIt).finally(fn)", None), + ("frank().then(go).then().then().then().catch(doIt).finally(fn)", None), + ("frank().then(go).then().catch(function() { /* why bother */ }).finally(fn)", None), + ("function a() { frank().then(go) }", None), + ("function a() { frank().then(go).then().then().then() }", None), + ("function a() { frank().then(go).then()}", None), + ("function a() { frank.then(go).then(to) }", None), + ( + "frank().then(go).catch(doIt).finally(fn).then(foo)", + Some(serde_json::json!([{ "allowFinally": true }])), + ), + ( + "frank().then(go).catch(doIt).finally(fn).foobar(foo)", + Some(serde_json::json!([{ "allowFinally": true }])), + ), + ("frank().then(go)", Some(serde_json::json!([{ "terminationMethod": "done" }]))), + ("frank().catch(go)", Some(serde_json::json!([{ "terminationMethod": "done" }]))), + ("frank().catch(go).someOtherMethod()", None), + ("frank()['catch'](go).someOtherMethod()", None), + ]; + + Tester::new(CatchOrReturn::NAME, pass, fail).test_and_snapshot(); +} diff --git a/crates/oxc_linter/src/snapshots/catch_or_return.snap b/crates/oxc_linter/src/snapshots/catch_or_return.snap new file mode 100644 index 0000000000000..279b77ebe3813 --- /dev/null +++ b/crates/oxc_linter/src/snapshots/catch_or_return.snap @@ -0,0 +1,170 @@ +--- +source: crates/oxc_linter/src/tester.rs +--- + ⚠ eslint-plugin-promise(catch-or-return): eslint-plugin-promise(catch-or-return): Expected catch or return + ╭─[catch_or_return.tsx:1:37] + 1 │ function callPromise(promise, cb) { promise.then(cb) } + · ──────────────── + ╰──── + help: Return the promise or chain a catch() + + ⚠ eslint-plugin-promise(catch-or-return): eslint-plugin-promise(catch-or-return): Expected catch or return + ╭─[catch_or_return.tsx:1:1] + 1 │ fetch("http://www.yahoo.com").then(console.log.bind(console)) + · ───────────────────────────────────────────────────────────── + ╰──── + help: Return the promise or chain a catch() + + ⚠ eslint-plugin-promise(catch-or-return): eslint-plugin-promise(catch-or-return): Expected catch or return + ╭─[catch_or_return.tsx:1:1] + 1 │ a.then(function() { return "x"; }).then(function(y) { throw y; }) + · ───────────────────────────────────────────────────────────────── + ╰──── + help: Return the promise or chain a catch() + + ⚠ eslint-plugin-promise(catch-or-return): eslint-plugin-promise(catch-or-return): Expected catch or return + ╭─[catch_or_return.tsx:1:1] + 1 │ Promise.resolve(frank) + · ────────────────────── + ╰──── + help: Return the promise or chain a catch() + + ⚠ eslint-plugin-promise(catch-or-return): eslint-plugin-promise(catch-or-return): Expected catch or return + ╭─[catch_or_return.tsx:1:1] + 1 │ Promise.all([]) + · ─────────────── + ╰──── + help: Return the promise or chain a catch() + + ⚠ eslint-plugin-promise(catch-or-return): eslint-plugin-promise(catch-or-return): Expected catch or return + ╭─[catch_or_return.tsx:1:1] + 1 │ Promise.allSettled([]) + · ────────────────────── + ╰──── + help: Return the promise or chain a catch() + + ⚠ eslint-plugin-promise(catch-or-return): eslint-plugin-promise(catch-or-return): Expected catch or return + ╭─[catch_or_return.tsx:1:1] + 1 │ Promise.any([]) + · ─────────────── + ╰──── + help: Return the promise or chain a catch() + + ⚠ eslint-plugin-promise(catch-or-return): eslint-plugin-promise(catch-or-return): Expected catch or return + ╭─[catch_or_return.tsx:1:1] + 1 │ Promise.race([]) + · ──────────────── + ╰──── + help: Return the promise or chain a catch() + + ⚠ eslint-plugin-promise(catch-or-return): eslint-plugin-promise(catch-or-return): Expected catch or return + ╭─[catch_or_return.tsx:1:1] + 1 │ frank().then(to).catch(fn).then(foo) + · ──────────────────────────────────── + ╰──── + help: Return the promise or chain a catch() + + ⚠ eslint-plugin-promise(catch-or-return): eslint-plugin-promise(catch-or-return): Expected catch or return + ╭─[catch_or_return.tsx:1:1] + 1 │ frank().finally(fn) + · ─────────────────── + ╰──── + help: Return the promise or chain a catch() + + ⚠ eslint-plugin-promise(catch-or-return): eslint-plugin-promise(catch-or-return): Expected catch or return + ╭─[catch_or_return.tsx:1:1] + 1 │ frank().then(to).finally(fn) + · ──────────────────────────── + ╰──── + help: Return the promise or chain a catch() + + ⚠ eslint-plugin-promise(catch-or-return): eslint-plugin-promise(catch-or-return): Expected catch or return + ╭─[catch_or_return.tsx:1:1] + 1 │ frank().then(go).catch(doIt).finally(fn) + · ──────────────────────────────────────── + ╰──── + help: Return the promise or chain a catch() + + ⚠ eslint-plugin-promise(catch-or-return): eslint-plugin-promise(catch-or-return): Expected catch or return + ╭─[catch_or_return.tsx:1:1] + 1 │ frank().then(go).then().then().then().catch(doIt).finally(fn) + · ───────────────────────────────────────────────────────────── + ╰──── + help: Return the promise or chain a catch() + + ⚠ eslint-plugin-promise(catch-or-return): eslint-plugin-promise(catch-or-return): Expected catch or return + ╭─[catch_or_return.tsx:1:1] + 1 │ frank().then(go).then().catch(function() { /* why bother */ }).finally(fn) + · ────────────────────────────────────────────────────────────────────────── + ╰──── + help: Return the promise or chain a catch() + + ⚠ eslint-plugin-promise(catch-or-return): eslint-plugin-promise(catch-or-return): Expected catch or return + ╭─[catch_or_return.tsx:1:16] + 1 │ function a() { frank().then(go) } + · ──────────────── + ╰──── + help: Return the promise or chain a catch() + + ⚠ eslint-plugin-promise(catch-or-return): eslint-plugin-promise(catch-or-return): Expected catch or return + ╭─[catch_or_return.tsx:1:16] + 1 │ function a() { frank().then(go).then().then().then() } + · ───────────────────────────────────── + ╰──── + help: Return the promise or chain a catch() + + ⚠ eslint-plugin-promise(catch-or-return): eslint-plugin-promise(catch-or-return): Expected catch or return + ╭─[catch_or_return.tsx:1:16] + 1 │ function a() { frank().then(go).then()} + · ─────────────────────── + ╰──── + help: Return the promise or chain a catch() + + ⚠ eslint-plugin-promise(catch-or-return): eslint-plugin-promise(catch-or-return): Expected catch or return + ╭─[catch_or_return.tsx:1:16] + 1 │ function a() { frank.then(go).then(to) } + · ─────────────────────── + ╰──── + help: Return the promise or chain a catch() + + ⚠ eslint-plugin-promise(catch-or-return): eslint-plugin-promise(catch-or-return): Expected catch or return + ╭─[catch_or_return.tsx:1:1] + 1 │ frank().then(go).catch(doIt).finally(fn).then(foo) + · ────────────────────────────────────────────────── + ╰──── + help: Return the promise or chain a catch() + + ⚠ eslint-plugin-promise(catch-or-return): eslint-plugin-promise(catch-or-return): Expected catch or return + ╭─[catch_or_return.tsx:1:1] + 1 │ frank().then(go).catch(doIt).finally(fn).foobar(foo) + · ──────────────────────────────────────────────────── + ╰──── + help: Return the promise or chain a catch() + + ⚠ eslint-plugin-promise(catch-or-return): eslint-plugin-promise(catch-or-return): Expected done or return + ╭─[catch_or_return.tsx:1:1] + 1 │ frank().then(go) + · ──────────────── + ╰──── + help: Return the promise or chain a done() + + ⚠ eslint-plugin-promise(catch-or-return): eslint-plugin-promise(catch-or-return): Expected done or return + ╭─[catch_or_return.tsx:1:1] + 1 │ frank().catch(go) + · ───────────────── + ╰──── + help: Return the promise or chain a done() + + ⚠ eslint-plugin-promise(catch-or-return): eslint-plugin-promise(catch-or-return): Expected catch or return + ╭─[catch_or_return.tsx:1:1] + 1 │ frank().catch(go).someOtherMethod() + · ─────────────────────────────────── + ╰──── + help: Return the promise or chain a catch() + + ⚠ eslint-plugin-promise(catch-or-return): eslint-plugin-promise(catch-or-return): Expected catch or return + ╭─[catch_or_return.tsx:1:1] + 1 │ frank()['catch'](go).someOtherMethod() + · ────────────────────────────────────── + ╰──── + help: Return the promise or chain a catch()