From aaecb333ff540a8c62fc03d449e9880cb742afda Mon Sep 17 00:00:00 2001 From: Jelle van der Waa Date: Sat, 13 Jul 2024 18:22:54 +0200 Subject: [PATCH 01/13] feat(tasks/rulegen): support creating eslint-plugin-promise rules --- apps/oxlint/src/command/lint.rs | 4 ++++ apps/oxlint/src/lint/mod.rs | 3 ++- crates/oxc_linter/src/options.rs | 9 +++++++++ justfile | 3 +++ tasks/rulegen/src/main.rs | 7 +++++++ tasks/rulegen/src/template.rs | 1 + tasks/website/src/linter/snapshots/cli.snap | 2 ++ tasks/website/src/linter/snapshots/cli_terminal.snap | 1 + 8 files changed, 29 insertions(+), 1 deletion(-) diff --git a/apps/oxlint/src/command/lint.rs b/apps/oxlint/src/command/lint.rs index de29b4720481a..672e5a77fcf93 100644 --- a/apps/oxlint/src/command/lint.rs +++ b/apps/oxlint/src/command/lint.rs @@ -223,6 +223,10 @@ pub struct EnablePlugins { /// Enable the React performance plugin and detect rendering performance problems #[bpaf(switch, hide_usage)] pub react_perf_plugin: bool, + + /// Enable the promise plugin and detect promise usage problems + #[bpaf(switch, hide_usage)] + pub promise_plugin: bool, } #[cfg(test)] diff --git a/apps/oxlint/src/lint/mod.rs b/apps/oxlint/src/lint/mod.rs index 777fdfee31d8b..609239796c192 100644 --- a/apps/oxlint/src/lint/mod.rs +++ b/apps/oxlint/src/lint/mod.rs @@ -104,7 +104,8 @@ impl Runner for LintRunner { .with_vitest_plugin(enable_plugins.vitest_plugin) .with_jsx_a11y_plugin(enable_plugins.jsx_a11y_plugin) .with_nextjs_plugin(enable_plugins.nextjs_plugin) - .with_react_perf_plugin(enable_plugins.react_perf_plugin); + .with_react_perf_plugin(enable_plugins.react_perf_plugin) + .with_promise_plugin(enable_plugins.promise_plugin); let linter = match Linter::from_options(lint_options) { Ok(lint_service) => lint_service, diff --git a/crates/oxc_linter/src/options.rs b/crates/oxc_linter/src/options.rs index a894dcdc2510a..3bb9c4c0bc62f 100644 --- a/crates/oxc_linter/src/options.rs +++ b/crates/oxc_linter/src/options.rs @@ -29,6 +29,7 @@ pub struct LintOptions { pub jsx_a11y_plugin: bool, pub nextjs_plugin: bool, pub react_perf_plugin: bool, + pub promise_plugin: bool, } impl Default for LintOptions { @@ -48,6 +49,7 @@ impl Default for LintOptions { jsx_a11y_plugin: false, nextjs_plugin: false, react_perf_plugin: false, + promise_plugin: false, } } } @@ -138,6 +140,12 @@ impl LintOptions { self.react_perf_plugin = yes; self } + + #[must_use] + pub fn with_promise_plugin(mut self, yes: bool) -> Self { + self.promise_plugin = yes; + self + } } #[derive(Debug, Clone, Copy, Eq, PartialEq)] @@ -342,6 +350,7 @@ impl LintOptions { "react_perf" => self.react_perf_plugin, "oxc" => self.oxc_plugin, "eslint" | "tree_shaking" => true, + "promise" => self.promise_plugin, name => panic!("Unhandled plugin: {name}"), }) .cloned() diff --git a/justfile b/justfile index 6ee3f7c3f1dcb..fb751ce7a1f88 100755 --- a/justfile +++ b/justfile @@ -145,6 +145,9 @@ new-react-perf-rule name: new-n-rule name: cargo run -p rulegen {{name}} n +new-promise-rule name: + cargo run -p rulegen {{name}} promise + clone-submodule dir url sha: git clone --depth=1 {{url}} {{dir}} || true cd {{dir}} && git fetch origin {{sha}} && git reset --hard {{sha}} diff --git a/tasks/rulegen/src/main.rs b/tasks/rulegen/src/main.rs index 75e518e09fdf9..183310845b6fa 100644 --- a/tasks/rulegen/src/main.rs +++ b/tasks/rulegen/src/main.rs @@ -55,6 +55,9 @@ const NODE_TEST_PATH: &str = const TREE_SHAKING_PATH: &str = "https://raw.githubusercontent.com/lukastaegert/eslint-plugin-tree-shaking/master/src/rules"; +const PROMISE_TEST_PATH: &str = + "https://raw.githubusercontent.com/eslint-community/eslint-plugin-promise/main/__tests__"; + struct TestCase { source_text: String, code: Option, @@ -570,6 +573,7 @@ pub enum RuleKind { JSDoc, Node, TreeShaking, + Promise, } impl RuleKind { @@ -586,6 +590,7 @@ impl RuleKind { "jsdoc" => Self::JSDoc, "n" => Self::Node, "tree-shaking" => Self::TreeShaking, + "promise" => Self::Promise, _ => Self::ESLint, } } @@ -606,6 +611,7 @@ impl Display for RuleKind { Self::JSDoc => write!(f, "eslint-plugin-jsdoc"), Self::Node => write!(f, "eslint-plugin-n"), Self::TreeShaking => write!(f, "eslint-plugin-tree-shaking"), + Self::Promise => write!(f, "eslint-plugin-promise"), } } } @@ -632,6 +638,7 @@ fn main() { RuleKind::JSDoc => format!("{JSDOC_TEST_PATH}/{camel_rule_name}.js"), RuleKind::Node => format!("{NODE_TEST_PATH}/{kebab_rule_name}.js"), RuleKind::TreeShaking => format!("{TREE_SHAKING_PATH}/{kebab_rule_name}.test.ts"), + RuleKind::Promise => format!("{PROMISE_TEST_PATH}/{kebab_rule_name}.js"), RuleKind::Oxc => String::new(), }; diff --git a/tasks/rulegen/src/template.rs b/tasks/rulegen/src/template.rs index 047ff5acb6153..117aaf6142113 100644 --- a/tasks/rulegen/src/template.rs +++ b/tasks/rulegen/src/template.rs @@ -42,6 +42,7 @@ impl<'a> Template<'a> { RuleKind::JSDoc => Path::new("crates/oxc_linter/src/rules/jsdoc"), RuleKind::Node => Path::new("crates/oxc_linter/src/rules/node"), RuleKind::TreeShaking => Path::new("crates/oxc_linter/src/rules/tree_shaking"), + RuleKind::Promise => Path::new("crates/oxc_linter/src/rules/promise"), }; std::fs::create_dir_all(path)?; diff --git a/tasks/website/src/linter/snapshots/cli.snap b/tasks/website/src/linter/snapshots/cli.snap index 23397ef38ef75..cab0aa8056c8e 100644 --- a/tasks/website/src/linter/snapshots/cli.snap +++ b/tasks/website/src/linter/snapshots/cli.snap @@ -66,6 +66,8 @@ Arguments: Enable the Next.js plugin and detect Next.js problems - **` --react-perf-plugin`** — Enable the React performance plugin and detect rendering performance problems +- **` --promise-plugin`** — + Enable the promise plugin and detect promise usage problems diff --git a/tasks/website/src/linter/snapshots/cli_terminal.snap b/tasks/website/src/linter/snapshots/cli_terminal.snap index 42246aa3a568c..c8102ba9f7474 100644 --- a/tasks/website/src/linter/snapshots/cli_terminal.snap +++ b/tasks/website/src/linter/snapshots/cli_terminal.snap @@ -40,6 +40,7 @@ Enable Plugins --nextjs-plugin Enable the Next.js plugin and detect Next.js problems --react-perf-plugin Enable the React performance plugin and detect rendering performance problems + --promise-plugin Enable the promise plugin and detect promise usage problems Fix Problems --fix Fix as many issues as possible. Only unfixed issues are reported in From f7b004f84a69ca5b24c36715bdc1e4987d10b219 Mon Sep 17 00:00:00 2001 From: Jelle van der Waa Date: Sat, 13 Jul 2024 18:36:04 +0200 Subject: [PATCH 02/13] feat(linter/eslint-plugin-promise): implement avoid-new Rule Detail: [link](https://github.com/eslint-community/eslint-plugin-promise/blob/main/docs/rules/avoid-new.md) --- crates/oxc_linter/src/rules.rs | 5 ++ .../oxc_linter/src/rules/promise/avoid_new.rs | 69 +++++++++++++++++++ .../oxc_linter/src/snapshots/avoid_new.snap | 20 ++++++ 3 files changed, 94 insertions(+) create mode 100644 crates/oxc_linter/src/rules/promise/avoid_new.rs create mode 100644 crates/oxc_linter/src/snapshots/avoid_new.snap diff --git a/crates/oxc_linter/src/rules.rs b/crates/oxc_linter/src/rules.rs index de384d52c8c04..dc829daefbaa1 100644 --- a/crates/oxc_linter/src/rules.rs +++ b/crates/oxc_linter/src/rules.rs @@ -432,6 +432,10 @@ mod tree_shaking { pub mod no_side_effects_in_initialization; } +mod promise { + pub mod avoid_new; +} + oxc_macros::declare_all_lint_rules! { eslint::array_callback_return, eslint::constructor_super, @@ -822,4 +826,5 @@ oxc_macros::declare_all_lint_rules! { jsdoc::require_returns_type, jsdoc::require_yields, tree_shaking::no_side_effects_in_initialization, + promise::avoid_new, } diff --git a/crates/oxc_linter/src/rules/promise/avoid_new.rs b/crates/oxc_linter/src/rules/promise/avoid_new.rs new file mode 100644 index 0000000000000..3ecfbd8b16833 --- /dev/null +++ b/crates/oxc_linter/src/rules/promise/avoid_new.rs @@ -0,0 +1,69 @@ +use oxc_ast::{ast::Expression, AstKind}; +use oxc_diagnostics::OxcDiagnostic; +use oxc_macros::declare_oxc_lint; +use oxc_span::Span; + +use crate::{context::LintContext, rule::Rule, AstNode}; + +fn avoid_new_promise_diagnostic(span0: Span) -> OxcDiagnostic { + OxcDiagnostic::warn("eslint-plugin-promise(avoid-new): Avoid creating new promises") + .with_label(span0) +} + +#[derive(Debug, Default, Clone)] +pub struct AvoidNew; + +declare_oxc_lint!( + /// ### What it does + /// + /// Disallow creating new promises outside of utility libs. + /// + /// ### Why is this bad? + /// + /// If you dislike the new promise style promises. + /// + /// ### Example + /// ```javascript + /// new Promise((resolve, reject) => { ... }); + /// ``` + AvoidNew, + style, +); + +impl Rule for AvoidNew { + fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { + let AstKind::NewExpression(expr) = node.kind() else { + return; + }; + + let Expression::Identifier(ident) = &expr.callee else { + return; + }; + + if ident.name == "Promise" && ctx.semantic().is_reference_to_global_variable(ident) { + ctx.diagnostic(avoid_new_promise_diagnostic(expr.span)); + } + } +} + +#[test] +fn test() { + use crate::tester::Tester; + + let pass = vec![ + "Promise.resolve()", + "Promise.reject()", + "Promise.all()", + "new Horse()", + "new PromiseLikeThing()", + "new Promise.resolve()", + ]; + + let fail = vec![ + "var x = new Promise(function (x, y) {})", + "new Promise()", + "Thing(new Promise(() => {}))", + ]; + + Tester::new(AvoidNew::NAME, pass, fail).test_and_snapshot(); +} diff --git a/crates/oxc_linter/src/snapshots/avoid_new.snap b/crates/oxc_linter/src/snapshots/avoid_new.snap new file mode 100644 index 0000000000000..d023f7b19141d --- /dev/null +++ b/crates/oxc_linter/src/snapshots/avoid_new.snap @@ -0,0 +1,20 @@ +--- +source: crates/oxc_linter/src/tester.rs +--- + ⚠ eslint-plugin-promise(avoid-new): Avoid creating new promises + ╭─[avoid_new.tsx:1:9] + 1 │ var x = new Promise(function (x, y) {}) + · ─────────────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(avoid-new): Avoid creating new promises + ╭─[avoid_new.tsx:1:1] + 1 │ new Promise() + · ───────────── + ╰──── + + ⚠ eslint-plugin-promise(avoid-new): Avoid creating new promises + ╭─[avoid_new.tsx:1:7] + 1 │ Thing(new Promise(() => {})) + · ───────────────────── + ╰──── From 0d4d3a44b0a99f1640515206f9cd860d3411e9a8 Mon Sep 17 00:00:00 2001 From: Jelle van der Waa Date: Sat, 13 Jul 2024 19:49:11 +0200 Subject: [PATCH 03/13] feat(linter/eslint-plugin-promise): implement no-new-statics Rule Detail: [link](https://github.com/eslint-community/eslint-plugin-promise/blob/main/docs/rules/no-new-statics.md) --- crates/oxc_linter/src/rules.rs | 2 + .../src/rules/promise/no_new_statics.rs | 106 ++++++++++++++++++ .../src/snapshots/no_new_statics.snap | 46 ++++++++ 3 files changed, 154 insertions(+) create mode 100644 crates/oxc_linter/src/rules/promise/no_new_statics.rs create mode 100644 crates/oxc_linter/src/snapshots/no_new_statics.snap diff --git a/crates/oxc_linter/src/rules.rs b/crates/oxc_linter/src/rules.rs index dc829daefbaa1..d6ce56904305a 100644 --- a/crates/oxc_linter/src/rules.rs +++ b/crates/oxc_linter/src/rules.rs @@ -434,6 +434,7 @@ mod tree_shaking { mod promise { pub mod avoid_new; + pub mod no_new_statics; } oxc_macros::declare_all_lint_rules! { @@ -827,4 +828,5 @@ oxc_macros::declare_all_lint_rules! { jsdoc::require_yields, tree_shaking::no_side_effects_in_initialization, promise::avoid_new, + promise::no_new_statics, } diff --git a/crates/oxc_linter/src/rules/promise/no_new_statics.rs b/crates/oxc_linter/src/rules/promise/no_new_statics.rs new file mode 100644 index 0000000000000..95976a0fdb2b0 --- /dev/null +++ b/crates/oxc_linter/src/rules/promise/no_new_statics.rs @@ -0,0 +1,106 @@ +use oxc_ast::{ast::Expression, AstKind}; +use oxc_diagnostics::OxcDiagnostic; +use oxc_macros::declare_oxc_lint; +use oxc_span::Span; + +use crate::{context::LintContext, rule::Rule, AstNode}; + +fn static_promise_diagnostic(span0: Span) -> OxcDiagnostic { + OxcDiagnostic::warn( + "eslint-plugin-promise(no-new-statics): Disallow calling `new` on a Promise static method", + ) + .with_label(span0) +} + +#[derive(Debug, Default, Clone)] +pub struct NoNewStatics; + +declare_oxc_lint!( + /// ### What it does + /// + /// Disallow calling new on a Promise static method. + /// + /// ### Why is this bad? + /// + /// Calling a Promise static method with new is invalid, resulting in a TypeError at runtime. + /// + /// ### Example + /// ```javascript + /// new Promise.resolve(value); + /// ``` + NoNewStatics, + correctness +); + +impl Rule for NoNewStatics { + fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { + let AstKind::NewExpression(new_expr) = node.kind() else { + return; + }; + + let Some(member_expr) = &new_expr.callee.get_member_expr() else { + return; + }; + + let Expression::Identifier(ident) = &member_expr.object() else { + return; + }; + + if ident.name != "Promise" || !ctx.semantic().is_reference_to_global_variable(ident) { + return; + } + + let Some(prop_name) = member_expr.static_property_name() else { + return; + }; + + // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise + if matches!( + prop_name, + "resolve" | "reject" | "all" | "allSettled" | "race" | "any" | "withResolvers" + ) { + ctx.diagnostic_with_fix(static_promise_diagnostic(new_expr.span), |fixer| { + fixer.delete_range(Span::new(new_expr.span.start, ident.span.start)) + }); + } + } +} + +#[test] +fn test() { + use crate::tester::Tester; + + let pass = vec![ + "Promise.resolve()", + "Promise.reject()", + "Promise.all()", + "Promise.race()", + "new Promise(function (resolve, reject) {})", + "new SomeClass()", + "SomeClass.resolve()", + "new SomeClass.resolve()", + ]; + + let fail = vec![ + "new Promise.resolve()", + "new Promise.reject()", + "new Promise.all()", + "new Promise.allSettled()", + "new Promise.any()", + "new Promise.race()", + "function foo() { + var a = getA() + return new Promise.resolve(a) + }", + ]; + + let fix = vec![ + ("new Promise.resolve()", "Promise.resolve()", None), + ("new Promise.reject()", "Promise.reject()", None), + ("new Promise.all()", "Promise.all()", None), + ("new Promise.allSettled()", "Promise.allSettled()", None), + ("new Promise.any()", "Promise.any()", None), + ("new Promise.race()", "Promise.race()", None), + ]; + Tester::new(NoNewStatics::NAME, pass, fail).expect_fix(fix).test_and_snapshot(); +} diff --git a/crates/oxc_linter/src/snapshots/no_new_statics.snap b/crates/oxc_linter/src/snapshots/no_new_statics.snap new file mode 100644 index 0000000000000..6daad9a6900a5 --- /dev/null +++ b/crates/oxc_linter/src/snapshots/no_new_statics.snap @@ -0,0 +1,46 @@ +--- +source: crates/oxc_linter/src/tester.rs +--- + ⚠ eslint-plugin-promise(no-new-statics): Disallow calling `new` on a Promise static method + ╭─[no_new_statics.tsx:1:1] + 1 │ new Promise.resolve() + · ───────────────────── + ╰──── + + ⚠ eslint-plugin-promise(no-new-statics): Disallow calling `new` on a Promise static method + ╭─[no_new_statics.tsx:1:1] + 1 │ new Promise.reject() + · ──────────────────── + ╰──── + + ⚠ eslint-plugin-promise(no-new-statics): Disallow calling `new` on a Promise static method + ╭─[no_new_statics.tsx:1:1] + 1 │ new Promise.all() + · ───────────────── + ╰──── + + ⚠ eslint-plugin-promise(no-new-statics): Disallow calling `new` on a Promise static method + ╭─[no_new_statics.tsx:1:1] + 1 │ new Promise.allSettled() + · ──────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(no-new-statics): Disallow calling `new` on a Promise static method + ╭─[no_new_statics.tsx:1:1] + 1 │ new Promise.any() + · ───────────────── + ╰──── + + ⚠ eslint-plugin-promise(no-new-statics): Disallow calling `new` on a Promise static method + ╭─[no_new_statics.tsx:1:1] + 1 │ new Promise.race() + · ────────────────── + ╰──── + + ⚠ eslint-plugin-promise(no-new-statics): Disallow calling `new` on a Promise static method + ╭─[no_new_statics.tsx:3:13] + 2 │ var a = getA() + 3 │ return new Promise.resolve(a) + · ────────────────────── + 4 │ } + ╰──── From 38883c0f07597767b984cf4a05d280eb6e8f64bf Mon Sep 17 00:00:00 2001 From: Jelle van der Waa Date: Sat, 13 Jul 2024 23:18:54 +0200 Subject: [PATCH 04/13] feat(linter/eslint-plugin-promise): implement param-names Rule detail: [link](https://github.com/eslint-community/eslint-plugin-promise/blob/main/docs/rules/param-names.md) --- crates/oxc_linter/src/rules.rs | 2 + .../src/rules/promise/param_names.rs | 179 ++++++++++++++++++ .../oxc_linter/src/snapshots/param_names.snap | 50 +++++ 3 files changed, 231 insertions(+) create mode 100644 crates/oxc_linter/src/rules/promise/param_names.rs create mode 100644 crates/oxc_linter/src/snapshots/param_names.snap diff --git a/crates/oxc_linter/src/rules.rs b/crates/oxc_linter/src/rules.rs index d6ce56904305a..ad727107d6f06 100644 --- a/crates/oxc_linter/src/rules.rs +++ b/crates/oxc_linter/src/rules.rs @@ -435,6 +435,7 @@ mod tree_shaking { mod promise { pub mod avoid_new; pub mod no_new_statics; + pub mod param_names; } oxc_macros::declare_all_lint_rules! { @@ -829,4 +830,5 @@ oxc_macros::declare_all_lint_rules! { tree_shaking::no_side_effects_in_initialization, promise::avoid_new, promise::no_new_statics, + promise::param_names, } diff --git a/crates/oxc_linter/src/rules/promise/param_names.rs b/crates/oxc_linter/src/rules/promise/param_names.rs new file mode 100644 index 0000000000000..9902acaed3467 --- /dev/null +++ b/crates/oxc_linter/src/rules/promise/param_names.rs @@ -0,0 +1,179 @@ +use oxc_ast::{ + ast::{BindingPatternKind, Expression, FormalParameter, FormalParameters}, + AstKind, +}; +use oxc_diagnostics::OxcDiagnostic; +use oxc_macros::declare_oxc_lint; +use oxc_span::Span; +use regex::Regex; + +use crate::{context::LintContext, rule::Rule, AstNode}; + +fn param_names_diagnostic(span0: Span, x0: &str) -> OxcDiagnostic { + OxcDiagnostic::warn(format!("eslint-plugin-promise(param-names): Promise constructor parameters must be named to match `{x0}`")).with_label(span0) +} + +#[derive(Debug, Default, Clone)] +pub struct ParamNames(Box); + +#[derive(Debug, Default, Clone)] +pub struct ParamNamesConfig { + resolve_pattern: Option, + reject_pattern: Option, +} + +impl std::ops::Deref for ParamNames { + type Target = ParamNamesConfig; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +enum ParamType { + Resolve, + Reject, +} + +declare_oxc_lint!( + /// ### What it does + /// + /// Enforce standard parameter names for Promise constructors. + /// + /// ### Why is this bad? + /// + /// Ensures that new Promise() is instantiated with the parameter names resolve, reject to + /// avoid confusion with order such as reject, resolve. The Promise constructor uses the + /// RevealingConstructor pattern. Using the same parameter names as the language specification + /// makes code more uniform and easier to understand. + /// + /// ### Example + /// ```javascript + /// new Promise(function (reject, resolve) { ... }) // incorrect order + /// new Promise(function (ok, fail) { ... }) // non-standard parameter names + /// ``` + ParamNames, + style, +); + +impl Rule for ParamNames { + fn from_configuration(value: serde_json::Value) -> Self { + let mut cfg = ParamNamesConfig::default(); + + if let Some(config) = value.get(0) { + if let Some(val) = config.get("resolvePattern").and_then(serde_json::Value::as_str) { + cfg.resolve_pattern = Regex::new(val).ok(); + } + if let Some(val) = config.get("rejectPattern").and_then(serde_json::Value::as_str) { + cfg.reject_pattern = Regex::new(val).ok(); + } + } + + Self(Box::new(cfg)) + } + + fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { + let AstKind::NewExpression(new_expr) = node.kind() else { + return; + }; + + if !new_expr.callee.is_specific_id("Promise") || new_expr.arguments.len() != 1 { + return; + } + + for argument in &new_expr.arguments { + let Some(arg_expr) = argument.as_expression() else { + continue; + }; + match arg_expr { + Expression::ArrowFunctionExpression(arrow_expr) => { + self.check_parameter_names(&arrow_expr.params, ctx); + } + Expression::FunctionExpression(func_expr) => { + self.check_parameter_names(&func_expr.params, ctx); + } + _ => continue, + } + } + } +} + +impl ParamNames { + fn check_parameter_names(&self, params: &FormalParameters, ctx: &LintContext) { + if params.items.len() < 1 { + return; + } + + self.check_parameter(¶ms.items[0], &ParamType::Resolve, ctx); + + if params.items.len() > 1 { + self.check_parameter(¶ms.items[1], &ParamType::Reject, ctx); + } + } + + fn check_parameter(&self, param: &FormalParameter, param_type: &ParamType, ctx: &LintContext) { + let BindingPatternKind::BindingIdentifier(param_ident) = ¶m.pattern.kind else { + return; + }; + + let param_pattern = if matches!(param_type, ParamType::Reject) { + &self.reject_pattern + } else { + &self.resolve_pattern + }; + + match param_pattern { + Some(pattern) => { + if !pattern.is_match(param_ident.name.as_str()) { + ctx.diagnostic(param_names_diagnostic(param_ident.span, pattern.as_str())); + } + } + None => { + if matches!(param_type, ParamType::Resolve) + && !matches!(param_ident.name.as_str(), "_resolve" | "resolve") + { + ctx.diagnostic(param_names_diagnostic(param_ident.span, "^_?resolve$")); + } else if matches!(param_type, ParamType::Reject) + && !matches!(param_ident.name.as_str(), "_reject" | "reject") + { + ctx.diagnostic(param_names_diagnostic(param_ident.span, "^_?reject$")); + } + } + } + } +} + +#[test] +fn test() { + use crate::tester::Tester; + + let pass = vec![ + ("new Promise(function(resolve, reject) {})", None), + ("new Promise(function(resolve, _reject) {})", None), + ("new Promise(function(_resolve, reject) {})", None), + ("new Promise(function(_resolve, _reject) {})", None), + ("new Promise(function(resolve) {})", None), + ("new Promise(function(_resolve) {})", None), + ("new Promise(resolve => {})", None), + ("new Promise((resolve, reject) => {})", None), + ("new Promise(() => {})", None), + ("new NonPromise()", None), + ( + "new Promise((yes, no) => {})", + Some(serde_json::json!([{ "resolvePattern": "^yes$", "rejectPattern": "^no$" }])), + ), + ]; + + let fail = vec![ + ("new Promise(function(reject, resolve) {})", None), + ("new Promise(function(resolve, rej) {})", None), + ("new Promise(yes => {})", None), + ("new Promise((yes, no) => {})", None), + ( + "new Promise(function(resolve, reject) { config(); })", + Some(serde_json::json!([{ "resolvePattern": "^yes$", "rejectPattern": "^no$" }])), + ), + ]; + + Tester::new(ParamNames::NAME, pass, fail).test_and_snapshot(); +} diff --git a/crates/oxc_linter/src/snapshots/param_names.snap b/crates/oxc_linter/src/snapshots/param_names.snap new file mode 100644 index 0000000000000..577e2a7049a2d --- /dev/null +++ b/crates/oxc_linter/src/snapshots/param_names.snap @@ -0,0 +1,50 @@ +--- +source: crates/oxc_linter/src/tester.rs +--- + ⚠ eslint-plugin-promise(param-names): Promise constructor parameters must be named to match `^_?resolve$` + ╭─[param_names.tsx:1:22] + 1 │ new Promise(function(reject, resolve) {}) + · ────── + ╰──── + + ⚠ eslint-plugin-promise(param-names): Promise constructor parameters must be named to match `^_?reject$` + ╭─[param_names.tsx:1:30] + 1 │ new Promise(function(reject, resolve) {}) + · ─────── + ╰──── + + ⚠ eslint-plugin-promise(param-names): Promise constructor parameters must be named to match `^_?reject$` + ╭─[param_names.tsx:1:31] + 1 │ new Promise(function(resolve, rej) {}) + · ─── + ╰──── + + ⚠ eslint-plugin-promise(param-names): Promise constructor parameters must be named to match `^_?resolve$` + ╭─[param_names.tsx:1:13] + 1 │ new Promise(yes => {}) + · ─── + ╰──── + + ⚠ eslint-plugin-promise(param-names): Promise constructor parameters must be named to match `^_?resolve$` + ╭─[param_names.tsx:1:14] + 1 │ new Promise((yes, no) => {}) + · ─── + ╰──── + + ⚠ eslint-plugin-promise(param-names): Promise constructor parameters must be named to match `^_?reject$` + ╭─[param_names.tsx:1:19] + 1 │ new Promise((yes, no) => {}) + · ── + ╰──── + + ⚠ eslint-plugin-promise(param-names): Promise constructor parameters must be named to match `^yes$` + ╭─[param_names.tsx:1:22] + 1 │ new Promise(function(resolve, reject) { config(); }) + · ─────── + ╰──── + + ⚠ eslint-plugin-promise(param-names): Promise constructor parameters must be named to match `^no$` + ╭─[param_names.tsx:1:31] + 1 │ new Promise(function(resolve, reject) { config(); }) + · ────── + ╰──── From 9d0ba5168cc64495bd38b1f3d25172b29268b90f Mon Sep 17 00:00:00 2001 From: Jelle van der Waa Date: Sat, 13 Jul 2024 20:30:51 +0200 Subject: [PATCH 05/13] feat(linter/eslint-plugin-promise): implement valid-params Rule detail: [link](https://github.com/eslint-community/eslint-plugin-promise/blob/main/docs/rules/valid-params.md) --- crates/oxc_linter/src/rules.rs | 2 + .../src/rules/promise/valid_params.rs | 190 ++++++++++++++++++ .../src/snapshots/valid_params.snap | 146 ++++++++++++++ 3 files changed, 338 insertions(+) create mode 100644 crates/oxc_linter/src/rules/promise/valid_params.rs create mode 100644 crates/oxc_linter/src/snapshots/valid_params.snap diff --git a/crates/oxc_linter/src/rules.rs b/crates/oxc_linter/src/rules.rs index ad727107d6f06..2100cf5280b41 100644 --- a/crates/oxc_linter/src/rules.rs +++ b/crates/oxc_linter/src/rules.rs @@ -436,6 +436,7 @@ mod promise { pub mod avoid_new; pub mod no_new_statics; pub mod param_names; + pub mod valid_params; } oxc_macros::declare_all_lint_rules! { @@ -831,4 +832,5 @@ oxc_macros::declare_all_lint_rules! { promise::avoid_new, promise::no_new_statics, promise::param_names, + promise::valid_params, } diff --git a/crates/oxc_linter/src/rules/promise/valid_params.rs b/crates/oxc_linter/src/rules/promise/valid_params.rs new file mode 100644 index 0000000000000..e3bc177a26356 --- /dev/null +++ b/crates/oxc_linter/src/rules/promise/valid_params.rs @@ -0,0 +1,190 @@ +use oxc_ast::{ + ast::{CallExpression, Expression}, + AstKind, +}; +use oxc_diagnostics::OxcDiagnostic; +use oxc_macros::declare_oxc_lint; +use oxc_span::Span; + +use crate::{context::LintContext, rule::Rule, AstNode}; + +fn valid_params_diagnostic(span0: Span, x0: &str) -> OxcDiagnostic { + OxcDiagnostic::warn(format!("eslint-plugin-promise(valid-params): {x0}")).with_label(span0) +} + +#[derive(Debug, Default, Clone)] +pub struct ValidParams; + +declare_oxc_lint!( + /// ### What it does + /// + /// Enforces the proper number of arguments are passed to Promise functions. + /// + /// ### Why is this bad? + /// + /// Calling a Promise function with the incorrect number of arguments can lead to unexpected + /// behavior or hard to spot bugs. + /// + /// ### Example + /// ```javascript + /// Promise.resolve(1, 2) + /// ``` + ValidParams, + correctness, +); + +fn is_promise(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; + }; + + // hello.then(), hello.catch(), hello.finally() + if matches!(prop_name, "then" | "catch" | "finally") { + return true; + } + + // foo().then(foo => {}), needed? + if let Expression::CallExpression(inner_call_expr) = member_expr.object() { + return is_promise(inner_call_expr); + } + + if member_expr.object().is_specific_id("Promise") + && matches!( + prop_name, + "resolve" | "reject" | "all" | "allSettled" | "race" | "any" | "withResolvers" + ) + { + return true; + } + + false +} + +impl Rule for ValidParams { + fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { + let AstKind::CallExpression(call_expr) = node.kind() else { + return; + }; + + if !is_promise(call_expr) { + return; + } + + let Some(member_expr) = call_expr.callee.get_member_expr() else { + return; + }; + + let Some(prop_name) = member_expr.static_property_name() else { + return; + }; + + let args_len = call_expr.arguments.len(); + + match prop_name { + "resolve" | "reject" => { + if args_len > 1 { + ctx.diagnostic(valid_params_diagnostic(call_expr.span, &format!("Promise.{prop_name}() requires 0 or 1 arguments, but received {args_len}"))); + } + } + "then" => { + if !(1..=2).contains(&args_len) { + ctx.diagnostic(valid_params_diagnostic(call_expr.span, &format!("Promise.{prop_name}() requires 1 or 2 arguments, but received {args_len}"))); + } + } + "race" | "all" | "allSettled" | "any" | "catch" | "finally" => { + if args_len != 1 { + ctx.diagnostic(valid_params_diagnostic( + call_expr.span, + &format!( + "Promise.{prop_name}() requires 1 argument, but received {args_len}" + ), + )); + } + } + _ => {} + } + } +} + +#[test] +fn test() { + use crate::tester::Tester; + + let pass = vec![ + "Promise.resolve()", + "Promise.resolve(1)", + "Promise.resolve({})", + "Promise.resolve(referenceToSomething)", + "Promise.reject()", + "Promise.reject(1)", + "Promise.reject({})", + "Promise.reject(referenceToSomething)", + "Promise.reject(Error())", + "Promise.race([])", + "Promise.race(iterable)", + "Promise.race([one, two, three])", + "Promise.all([])", + "Promise.all(iterable)", + "Promise.all([one, two, three])", + "Promise.allSettled([])", + "Promise.allSettled(iterable)", + "Promise.allSettled([one, two, three])", + "Promise.any([])", + "Promise.any(iterable)", + "Promise.any([one, two, three])", + "somePromise().then(success)", + "somePromise().then(success, failure)", + "promiseReference.then(() => {})", + "promiseReference.then(() => {}, () => {})", + "somePromise().catch(callback)", + "somePromise().catch(err => {})", + "promiseReference.catch(callback)", + "promiseReference.catch(err => {})", + "somePromise().finally(callback)", + "somePromise().finally(() => {})", + "promiseReference.finally(callback)", + "promiseReference.finally(() => {})", + "Promise.all([ + Promise.resolve(1), + Promise.resolve(2), + Promise.reject(Error()), + ]) + .then(console.log) + .catch(console.error) + .finally(console.log) + ", + ]; + + let fail = vec![ + "Promise.resolve(1, 2)", + "Promise.resolve({}, function() {}, 1, 2, 3)", + "Promise.reject(1, 2, 3)", + "Promise.reject({}, function() {}, 1, 2)", + "Promise.race(1, 2)", + "Promise.race({}, function() {}, 1, 2, 3)", + "Promise.all(1, 2, 3)", + "Promise.all({}, function() {}, 1, 2)", + "Promise.allSettled(1, 2, 3)", + "Promise.allSettled({}, function() {}, 1, 2)", + "Promise.any(1, 2, 3)", + "Promise.any({}, function() {}, 1, 2)", + "somePromise().then()", + "somePromise().then(() => {}, () => {}, () => {})", + "promiseReference.then()", + "promiseReference.then(() => {}, () => {}, () => {})", + "somePromise().catch()", + "somePromise().catch(() => {}, () => {})", + "promiseReference.catch()", + "promiseReference.catch(() => {}, () => {})", + "somePromise().finally()", + "somePromise().finally(() => {}, () => {})", + "promiseReference.finally()", + "promiseReference.finally(() => {}, () => {})", + ]; + + Tester::new(ValidParams::NAME, pass, fail).test_and_snapshot(); +} diff --git a/crates/oxc_linter/src/snapshots/valid_params.snap b/crates/oxc_linter/src/snapshots/valid_params.snap new file mode 100644 index 0000000000000..4ca7277c11896 --- /dev/null +++ b/crates/oxc_linter/src/snapshots/valid_params.snap @@ -0,0 +1,146 @@ +--- +source: crates/oxc_linter/src/tester.rs +--- + ⚠ eslint-plugin-promise(valid-params): Promise.resolve() requires 0 or 1 arguments, but received 2 + ╭─[valid_params.tsx:1:1] + 1 │ Promise.resolve(1, 2) + · ───────────────────── + ╰──── + + ⚠ eslint-plugin-promise(valid-params): Promise.resolve() requires 0 or 1 arguments, but received 5 + ╭─[valid_params.tsx:1:1] + 1 │ Promise.resolve({}, function() {}, 1, 2, 3) + · ─────────────────────────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(valid-params): Promise.reject() requires 0 or 1 arguments, but received 3 + ╭─[valid_params.tsx:1:1] + 1 │ Promise.reject(1, 2, 3) + · ─────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(valid-params): Promise.reject() requires 0 or 1 arguments, but received 4 + ╭─[valid_params.tsx:1:1] + 1 │ Promise.reject({}, function() {}, 1, 2) + · ─────────────────────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(valid-params): Promise.race() requires 1 argument, but received 2 + ╭─[valid_params.tsx:1:1] + 1 │ Promise.race(1, 2) + · ────────────────── + ╰──── + + ⚠ eslint-plugin-promise(valid-params): Promise.race() requires 1 argument, but received 5 + ╭─[valid_params.tsx:1:1] + 1 │ Promise.race({}, function() {}, 1, 2, 3) + · ──────────────────────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(valid-params): Promise.all() requires 1 argument, but received 3 + ╭─[valid_params.tsx:1:1] + 1 │ Promise.all(1, 2, 3) + · ──────────────────── + ╰──── + + ⚠ eslint-plugin-promise(valid-params): Promise.all() requires 1 argument, but received 4 + ╭─[valid_params.tsx:1:1] + 1 │ Promise.all({}, function() {}, 1, 2) + · ──────────────────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(valid-params): Promise.allSettled() requires 1 argument, but received 3 + ╭─[valid_params.tsx:1:1] + 1 │ Promise.allSettled(1, 2, 3) + · ─────────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(valid-params): Promise.allSettled() requires 1 argument, but received 4 + ╭─[valid_params.tsx:1:1] + 1 │ Promise.allSettled({}, function() {}, 1, 2) + · ─────────────────────────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(valid-params): Promise.any() requires 1 argument, but received 3 + ╭─[valid_params.tsx:1:1] + 1 │ Promise.any(1, 2, 3) + · ──────────────────── + ╰──── + + ⚠ eslint-plugin-promise(valid-params): Promise.any() requires 1 argument, but received 4 + ╭─[valid_params.tsx:1:1] + 1 │ Promise.any({}, function() {}, 1, 2) + · ──────────────────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(valid-params): Promise.then() requires 1 or 2 arguments, but received 0 + ╭─[valid_params.tsx:1:1] + 1 │ somePromise().then() + · ──────────────────── + ╰──── + + ⚠ eslint-plugin-promise(valid-params): Promise.then() requires 1 or 2 arguments, but received 3 + ╭─[valid_params.tsx:1:1] + 1 │ somePromise().then(() => {}, () => {}, () => {}) + · ──────────────────────────────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(valid-params): Promise.then() requires 1 or 2 arguments, but received 0 + ╭─[valid_params.tsx:1:1] + 1 │ promiseReference.then() + · ─────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(valid-params): Promise.then() requires 1 or 2 arguments, but received 3 + ╭─[valid_params.tsx:1:1] + 1 │ promiseReference.then(() => {}, () => {}, () => {}) + · ─────────────────────────────────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(valid-params): Promise.catch() requires 1 argument, but received 0 + ╭─[valid_params.tsx:1:1] + 1 │ somePromise().catch() + · ───────────────────── + ╰──── + + ⚠ eslint-plugin-promise(valid-params): Promise.catch() requires 1 argument, but received 2 + ╭─[valid_params.tsx:1:1] + 1 │ somePromise().catch(() => {}, () => {}) + · ─────────────────────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(valid-params): Promise.catch() requires 1 argument, but received 0 + ╭─[valid_params.tsx:1:1] + 1 │ promiseReference.catch() + · ──────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(valid-params): Promise.catch() requires 1 argument, but received 2 + ╭─[valid_params.tsx:1:1] + 1 │ promiseReference.catch(() => {}, () => {}) + · ────────────────────────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(valid-params): Promise.finally() requires 1 argument, but received 0 + ╭─[valid_params.tsx:1:1] + 1 │ somePromise().finally() + · ─────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(valid-params): Promise.finally() requires 1 argument, but received 2 + ╭─[valid_params.tsx:1:1] + 1 │ somePromise().finally(() => {}, () => {}) + · ───────────────────────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(valid-params): Promise.finally() requires 1 argument, but received 0 + ╭─[valid_params.tsx:1:1] + 1 │ promiseReference.finally() + · ────────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(valid-params): Promise.finally() requires 1 argument, but received 2 + ╭─[valid_params.tsx:1:1] + 1 │ promiseReference.finally(() => {}, () => {}) + · ──────────────────────────────────────────── + ╰──── From 271bbec7ce1cf3d6508018505c69f8957aacfac4 Mon Sep 17 00:00:00 2001 From: Jelle van der Waa Date: Sat, 13 Jul 2024 20:34:38 +0200 Subject: [PATCH 06/13] refactor(linter): make `is_promise` re-usable for other future rules --- crates/oxc_linter/src/ast_util.rs | 31 ++++++++++++++++ .../src/rules/promise/valid_params.rs | 37 +------------------ 2 files changed, 33 insertions(+), 35 deletions(-) diff --git a/crates/oxc_linter/src/ast_util.rs b/crates/oxc_linter/src/ast_util.rs index 7f2f4c40108d4..776808db5c2bc 100644 --- a/crates/oxc_linter/src/ast_util.rs +++ b/crates/oxc_linter/src/ast_util.rs @@ -403,3 +403,34 @@ pub fn is_function_node(node: &AstNode) -> bool { _ => false, } } + +pub fn is_promise(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; + }; + + // hello.then(), hello.catch(), hello.finally() + if matches!(prop_name, "then" | "catch" | "finally") { + return true; + } + + // foo().then(foo => {}), needed? + if let Expression::CallExpression(inner_call_expr) = member_expr.object() { + return is_promise(inner_call_expr); + } + + if member_expr.object().is_specific_id("Promise") + && matches!( + prop_name, + "resolve" | "reject" | "all" | "allSettled" | "race" | "any" | "withResolvers" + ) + { + return true; + } + + false +} diff --git a/crates/oxc_linter/src/rules/promise/valid_params.rs b/crates/oxc_linter/src/rules/promise/valid_params.rs index e3bc177a26356..b4437f777a4f1 100644 --- a/crates/oxc_linter/src/rules/promise/valid_params.rs +++ b/crates/oxc_linter/src/rules/promise/valid_params.rs @@ -1,7 +1,5 @@ -use oxc_ast::{ - ast::{CallExpression, Expression}, - AstKind, -}; +use crate::ast_util::is_promise; +use oxc_ast::AstKind; use oxc_diagnostics::OxcDiagnostic; use oxc_macros::declare_oxc_lint; use oxc_span::Span; @@ -33,37 +31,6 @@ declare_oxc_lint!( correctness, ); -fn is_promise(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; - }; - - // hello.then(), hello.catch(), hello.finally() - if matches!(prop_name, "then" | "catch" | "finally") { - return true; - } - - // foo().then(foo => {}), needed? - if let Expression::CallExpression(inner_call_expr) = member_expr.object() { - return is_promise(inner_call_expr); - } - - if member_expr.object().is_specific_id("Promise") - && matches!( - prop_name, - "resolve" | "reject" | "all" | "allSettled" | "race" | "any" | "withResolvers" - ) - { - return true; - } - - false -} - impl Rule for ValidParams { fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { let AstKind::CallExpression(call_expr) = node.kind() else { From 82457268af50a8d77e51e57f6356d95e1bc17b6d Mon Sep 17 00:00:00 2001 From: Jelle van der Waa Date: Sat, 13 Jul 2024 21:03:02 +0200 Subject: [PATCH 07/13] feat(linter/eslint-plugin-promise): implement no-return-in-finally Rule detail: [link](https://github.com/eslint-community/eslint-plugin-promise/blob/main/docs/rules/no-return-in-finally.md) --- crates/oxc_linter/src/rules.rs | 2 + .../src/rules/promise/no_return_in_finally.rs | 118 ++++++++++++++++++ .../src/snapshots/no_return_in_finally.snap | 26 ++++ 3 files changed, 146 insertions(+) create mode 100644 crates/oxc_linter/src/rules/promise/no_return_in_finally.rs create mode 100644 crates/oxc_linter/src/snapshots/no_return_in_finally.snap diff --git a/crates/oxc_linter/src/rules.rs b/crates/oxc_linter/src/rules.rs index 2100cf5280b41..cd8a5c8de5b28 100644 --- a/crates/oxc_linter/src/rules.rs +++ b/crates/oxc_linter/src/rules.rs @@ -435,6 +435,7 @@ 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 valid_params; } @@ -833,4 +834,5 @@ oxc_macros::declare_all_lint_rules! { promise::no_new_statics, promise::param_names, promise::valid_params, + promise::no_return_in_finally, } 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..dbf78062812fa --- /dev/null +++ b/crates/oxc_linter/src/rules/promise/no_return_in_finally.rs @@ -0,0 +1,118 @@ +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::{ast_util::is_promise, context::LintContext, rule::Rule, AstNode}; + +fn no_return_in_finally_diagnostic(span0: Span) -> OxcDiagnostic { + OxcDiagnostic::warn( + "eslint-plugin-promise(no-return-in-finally): Disallow return statements in finally", + ) + .with_label(span0) +} + +#[derive(Debug, Default, Clone)] +pub struct NoReturnInFinally; + +declare_oxc_lint!( + /// ### What it does + /// + /// Disallow return statements in finally(). + /// + /// ### 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, + correctness, +); + +impl Rule for NoReturnInFinally { + fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { + let AstKind::CallExpression(call_expr) = node.kind() else { + return; + }; + + if !is_promise(call_expr) { + return; + } + + let Some(member_expr) = call_expr.callee.get_member_expr() else { + return; + }; + + let Some(prop_name) = member_expr.static_property_name() 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/snapshots/no_return_in_finally.snap b/crates/oxc_linter/src/snapshots/no_return_in_finally.snap new file mode 100644 index 0000000000000..7d57955e88adc --- /dev/null +++ b/crates/oxc_linter/src/snapshots/no_return_in_finally.snap @@ -0,0 +1,26 @@ +--- +source: crates/oxc_linter/src/tester.rs +--- + ⚠ eslint-plugin-promise(no-return-in-finally): Disallow return statements in finally + ╭─[no_return_in_finally.tsx:1:36] + 1 │ Promise.resolve(1).finally(() => { return 2 }) + · ──────── + ╰──── + + ⚠ eslint-plugin-promise(no-return-in-finally): Disallow return statements in finally + ╭─[no_return_in_finally.tsx:1:35] + 1 │ Promise.reject(0).finally(() => { return 2 }) + · ──────── + ╰──── + + ⚠ eslint-plugin-promise(no-return-in-finally): Disallow return statements in finally + ╭─[no_return_in_finally.tsx:1:27] + 1 │ myPromise.finally(() => { return 2 }); + · ──────── + ╰──── + + ⚠ eslint-plugin-promise(no-return-in-finally): Disallow return statements in finally + ╭─[no_return_in_finally.tsx:1:42] + 1 │ Promise.resolve(1).finally(function () { return 2 }) + · ──────── + ╰──── From 65cf0ed37fb43ad193bf4ef729f076cef2f3c104 Mon Sep 17 00:00:00 2001 From: Jelle van der Waa Date: Sun, 14 Jul 2024 01:52:41 +0200 Subject: [PATCH 08/13] feat(linter/eslint-plugin-promise): implement no-return-wrap Rule detail: [link](https://github.com/eslint-community/eslint-plugin-promise/blob/main/docs/rules/no-return-wrap.md) --- crates/oxc_linter/src/rules.rs | 2 + .../src/rules/promise/no_return_wrap.rs | 291 ++++++++++++++++++ .../src/snapshots/no_return_wrap.snap | 154 +++++++++ 3 files changed, 447 insertions(+) create mode 100644 crates/oxc_linter/src/rules/promise/no_return_wrap.rs create mode 100644 crates/oxc_linter/src/snapshots/no_return_wrap.snap diff --git a/crates/oxc_linter/src/rules.rs b/crates/oxc_linter/src/rules.rs index cd8a5c8de5b28..5c5d89e68017b 100644 --- a/crates/oxc_linter/src/rules.rs +++ b/crates/oxc_linter/src/rules.rs @@ -436,6 +436,7 @@ mod promise { pub mod avoid_new; pub mod no_new_statics; pub mod no_return_in_finally; + pub mod no_return_wrap; pub mod param_names; pub mod valid_params; } @@ -835,4 +836,5 @@ oxc_macros::declare_all_lint_rules! { promise::param_names, promise::valid_params, promise::no_return_in_finally, + promise::no_return_wrap, } diff --git a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs new file mode 100644 index 0000000000000..fcb803b74f762 --- /dev/null +++ b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs @@ -0,0 +1,291 @@ +use oxc_ast::{ + ast::{CallExpression, Expression, Statement}, + AstKind, +}; +use oxc_diagnostics::OxcDiagnostic; +use oxc_macros::declare_oxc_lint; +use oxc_span::Span; + +use crate::{ + ast_util::{get_enclosing_function, is_method_call, is_promise}, + context::LintContext, + rule::Rule, + AstNode, +}; + +fn no_return_wrap_diagnostic(span0: Span, x0: &str) -> OxcDiagnostic { + OxcDiagnostic::warn(format!("eslint-plugin-promise(no-return-wrap): {x0}")).with_label(span0) +} + +#[derive(Debug, Default, Clone)] +pub struct NoReturnWrap { + // + allow_reject: bool, +} + +declare_oxc_lint!( + /// ### What it does + /// + /// Disallow wrapping values in Promise.resolve or Promise.reject when not needed + /// (promise/no-return-wrap). + /// + /// ### Why is this bad? + /// + /// Ensure that inside a then() or a catch() we always return or throw a raw value instead of + /// wrapping in Promise.resolve or Promise.reject + /// + /// ### Example + /// ```javascript + /// myPromise.then(function (val) { + /// return Promise.resolve(val * 2) + /// }) + /// myPromise.then(function (val) { + /// return Promise.reject('bad thing') + /// }) + /// ``` + NoReturnWrap, + correctness, +); + +impl Rule for NoReturnWrap { + fn from_configuration(value: serde_json::Value) -> Self { + let allow_reject = value + .get(0) + .and_then(|config| config.get("allowReject")) + .and_then(serde_json::Value::as_bool) + .unwrap_or(false); + + Self { allow_reject } + } + + fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { + match node.kind() { + AstKind::ArrowFunctionExpression(arrowfunc_expr) => { + if arrowfunc_expr.body.statements.len() != 1 { + return; + } + + let Statement::ExpressionStatement(expr_stmt) = &arrowfunc_expr.body.statements[0] + else { + return; + }; + + let Expression::CallExpression(call_expr) = &expr_stmt.expression else { + return; + }; + + if !self.is_promise_call(call_expr) { + return; + } + + is_in_promise(call_expr, node, call_expr.span, ctx); + } + AstKind::ReturnStatement(stmt) => { + let Some(Expression::CallExpression(call_expr)) = &stmt.argument else { + return; + }; + if !self.is_promise_call(call_expr) { + return; + } + + is_in_promise(call_expr, node, stmt.span, ctx); + } + _ => {} + } + } +} + +impl NoReturnWrap { + fn is_promise_call(&self, call_expr: &CallExpression) -> bool { + let Some(member_expr) = call_expr.callee.get_member_expr() else { + return false; + }; + + if !member_expr.object().is_specific_id("Promise") { + return false; + } + + let Some(prop_name) = member_expr.static_property_name() else { + return false; + }; + + if prop_name != "resolve" && prop_name != "reject" { + return false; + } + + if self.allow_reject && prop_name == "reject" { + return false; + } + + true + } +} + +fn is_in_promise<'a>( + call_expr: &CallExpression, + node: &AstNode<'a>, + span: Span, + ctx: &LintContext<'a>, +) { + let Some(member_expr) = call_expr.callee.get_member_expr() else { + return; + }; + + let Some(prop_name) = member_expr.static_property_name() else { + return; + }; + + let Some(func_node) = get_enclosing_function(node, ctx) else { return }; + + // Rename to get_enclosing_call_expr?? + // We are only interested in the first CallExpression from the enclosing function scope but + // not in + for node_id in ctx.nodes().ancestors(func_node.id()) { + let kind = ctx.nodes().kind(node_id); + let AstKind::CallExpression(outer_call_expr) = kind else { continue }; + + // Ignore .bind(this) + if !call_expr.optional && is_method_call(outer_call_expr, None, Some(&["bind"]), None, None) + { + continue; + } + + if is_promise(outer_call_expr) { + if prop_name == "resolve" { + ctx.diagnostic(no_return_wrap_diagnostic( + span, + "Avoid wrapping return values in Promise.resolve", + )); + } + if prop_name == "reject" { + ctx.diagnostic(no_return_wrap_diagnostic( + span, + "Expected throw instead of Promise.reject", + )); + } + } + + return; + } +} + +#[test] +fn test() { + use crate::tester::Tester; + + let pass = vec![ + ("Promise.resolve(4).then(function(x) { return x })", None), + ("Promise.reject(4).then(function(x) { return x })", None), + ("Promise.resolve(4).then(function() {})", None), + ("Promise.reject(4).then(function() {})", None), + ("doThing().then(function() { return 4 })", None), + ("doThing().then(function() { throw 4 })", None), + ("doThing().then(null, function() { return 4 })", None), + ("doThing().then(null, function() { throw 4 })", None), + ("doThing().catch(null, function() { return 4 })", None), + ("doThing().catch(null, function() { throw 4 })", None), + ("doThing().then(function() { return Promise.all([a,b,c]) })", None), + ("doThing().then(() => 4)", None), + ("doThing().then(() => { throw 4 })", None), + ("doThing().then(()=>{}, () => 4)", None), + ("doThing().then(()=>{}, () => { throw 4 })", None), + ("doThing().catch(() => 4)", None), + ("doThing().catch(() => { throw 4 })", None), + ("var x = function() { return Promise.resolve(4) }", None), + ("function y() { return Promise.resolve(4) }", None), + ("function then() { return Promise.reject() }", None), + ("doThing(function(x) { return Promise.reject(x) })", None), + ("doThing().then(function() { return })", None), + ( + "doThing().then(function() { return Promise.reject(4) })", + Some(serde_json::json!([{ "allowReject": true }])), + ), + ("doThing().then((function() { return Promise.resolve(4) }).toString())", None), + ( + "doThing().then(() => Promise.reject(4))", + Some(serde_json::json!([{ "allowReject": true }])), + ), + ("doThing().then(function() { return a() })", None), + ("doThing().then(function() { return Promise.a() })", None), + ("doThing().then(() => { return a() })", None), + ("doThing().then(() => { return Promise.a() })", None), + ("doThing().then(() => a())", None), + ("doThing().then(() => Promise.a())", None), + ]; + + let fail = vec![ + ("doThing().then(function() { return Promise.resolve(4) })", None), +("doThing().then(null, function() { return Promise.resolve(4) })", None), +("doThing().catch(function() { return Promise.resolve(4) })", None), +("doThing().then(function() { return Promise.reject(4) })", None), +("doThing().then(null, function() { return Promise.reject(4) })", None), +("doThing().catch(function() { return Promise.reject(4) })", None), +(r#"doThing().then(function(x) { if (x>1) { return Promise.resolve(4) } else { throw "bad" } })"#, None), +("doThing().then(function(x) { if (x>1) { return Promise.reject(4) } })", None), +("doThing().then(null, function() { if (true && false) { return Promise.resolve() } })", None), +("doThing().catch(function(x) {if (x) { return Promise.resolve(4) } else { return Promise.reject() } })", None), +(" + fn(function() { + doThing().then(function() { + return Promise.resolve(4) + }) + return + })", None), +(" + fn(function() { + doThing().then(function nm() { + return Promise.resolve(4) + }) + return + })", None), +(" + fn(function() { + fn2(function() { + doThing().then(function() { + return Promise.resolve(4) + }) + }) + })", None), +(" + fn(function() { + fn2(function() { + doThing().then(function() { + fn3(function() { + return Promise.resolve(4) + }) + return Promise.resolve(4) + }) + }) + })", None), +(" + const o = { + fn: function() { + return doThing().then(function() { + return Promise.resolve(5); + }); + }, + } + ", None), +(" + fn( + doThing().then(function() { + return Promise.resolve(5); + }) + ); + ", None), +("doThing().then((function() { return Promise.resolve(4) }).bind(this))", None), +("doThing().then((function() { return Promise.resolve(4) }).bind(this).bind(this))", None), +("doThing().then(() => { return Promise.resolve(4) })", None), +(" + function a () { + return p.then(function(val) { + return Promise.resolve(val * 4) + }) + } + ", None), +("doThing1().then(() => Promise.resolve(9))", None), +("doThing().then(() => Promise.reject(4))", None) + ]; + + Tester::new(NoReturnWrap::NAME, pass, fail).test_and_snapshot(); +} diff --git a/crates/oxc_linter/src/snapshots/no_return_wrap.snap b/crates/oxc_linter/src/snapshots/no_return_wrap.snap new file mode 100644 index 0000000000000..1ae2555dde685 --- /dev/null +++ b/crates/oxc_linter/src/snapshots/no_return_wrap.snap @@ -0,0 +1,154 @@ +--- +source: crates/oxc_linter/src/tester.rs +--- + ⚠ eslint-plugin-promise(no-return-wrap): Avoid wrapping return values in Promise.resolve + ╭─[no_return_wrap.tsx:1:29] + 1 │ doThing().then(function() { return Promise.resolve(4) }) + · ───────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(no-return-wrap): Avoid wrapping return values in Promise.resolve + ╭─[no_return_wrap.tsx:1:35] + 1 │ doThing().then(null, function() { return Promise.resolve(4) }) + · ───────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(no-return-wrap): Avoid wrapping return values in Promise.resolve + ╭─[no_return_wrap.tsx:1:30] + 1 │ doThing().catch(function() { return Promise.resolve(4) }) + · ───────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(no-return-wrap): Expected throw instead of Promise.reject + ╭─[no_return_wrap.tsx:1:29] + 1 │ doThing().then(function() { return Promise.reject(4) }) + · ──────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(no-return-wrap): Expected throw instead of Promise.reject + ╭─[no_return_wrap.tsx:1:35] + 1 │ doThing().then(null, function() { return Promise.reject(4) }) + · ──────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(no-return-wrap): Expected throw instead of Promise.reject + ╭─[no_return_wrap.tsx:1:30] + 1 │ doThing().catch(function() { return Promise.reject(4) }) + · ──────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(no-return-wrap): Avoid wrapping return values in Promise.resolve + ╭─[no_return_wrap.tsx:1:41] + 1 │ doThing().then(function(x) { if (x>1) { return Promise.resolve(4) } else { throw "bad" } }) + · ───────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(no-return-wrap): Expected throw instead of Promise.reject + ╭─[no_return_wrap.tsx:1:41] + 1 │ doThing().then(function(x) { if (x>1) { return Promise.reject(4) } }) + · ──────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(no-return-wrap): Avoid wrapping return values in Promise.resolve + ╭─[no_return_wrap.tsx:1:56] + 1 │ doThing().then(null, function() { if (true && false) { return Promise.resolve() } }) + · ──────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(no-return-wrap): Avoid wrapping return values in Promise.resolve + ╭─[no_return_wrap.tsx:1:39] + 1 │ doThing().catch(function(x) {if (x) { return Promise.resolve(4) } else { return Promise.reject() } }) + · ───────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(no-return-wrap): Expected throw instead of Promise.reject + ╭─[no_return_wrap.tsx:1:74] + 1 │ doThing().catch(function(x) {if (x) { return Promise.resolve(4) } else { return Promise.reject() } }) + · ─────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(no-return-wrap): Avoid wrapping return values in Promise.resolve + ╭─[no_return_wrap.tsx:4:14] + 3 │ doThing().then(function() { + 4 │ return Promise.resolve(4) + · ───────────────────────── + 5 │ }) + ╰──── + + ⚠ eslint-plugin-promise(no-return-wrap): Avoid wrapping return values in Promise.resolve + ╭─[no_return_wrap.tsx:4:14] + 3 │ doThing().then(function nm() { + 4 │ return Promise.resolve(4) + · ───────────────────────── + 5 │ }) + ╰──── + + ⚠ eslint-plugin-promise(no-return-wrap): Avoid wrapping return values in Promise.resolve + ╭─[no_return_wrap.tsx:5:16] + 4 │ doThing().then(function() { + 5 │ return Promise.resolve(4) + · ───────────────────────── + 6 │ }) + ╰──── + + ⚠ eslint-plugin-promise(no-return-wrap): Avoid wrapping return values in Promise.resolve + ╭─[no_return_wrap.tsx:8:16] + 7 │ }) + 8 │ return Promise.resolve(4) + · ───────────────────────── + 9 │ }) + ╰──── + + ⚠ eslint-plugin-promise(no-return-wrap): Avoid wrapping return values in Promise.resolve + ╭─[no_return_wrap.tsx:5:16] + 4 │ return doThing().then(function() { + 5 │ return Promise.resolve(5); + · ────────────────────────── + 6 │ }); + ╰──── + + ⚠ eslint-plugin-promise(no-return-wrap): Avoid wrapping return values in Promise.resolve + ╭─[no_return_wrap.tsx:4:14] + 3 │ doThing().then(function() { + 4 │ return Promise.resolve(5); + · ────────────────────────── + 5 │ }) + ╰──── + + ⚠ eslint-plugin-promise(no-return-wrap): Avoid wrapping return values in Promise.resolve + ╭─[no_return_wrap.tsx:1:30] + 1 │ doThing().then((function() { return Promise.resolve(4) }).bind(this)) + · ───────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(no-return-wrap): Avoid wrapping return values in Promise.resolve + ╭─[no_return_wrap.tsx:1:30] + 1 │ doThing().then((function() { return Promise.resolve(4) }).bind(this).bind(this)) + · ───────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(no-return-wrap): Avoid wrapping return values in Promise.resolve + ╭─[no_return_wrap.tsx:1:24] + 1 │ doThing().then(() => { return Promise.resolve(4) }) + · ───────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(no-return-wrap): Avoid wrapping return values in Promise.resolve + ╭─[no_return_wrap.tsx:4:14] + 3 │ return p.then(function(val) { + 4 │ return Promise.resolve(val * 4) + · ─────────────────────────────── + 5 │ }) + ╰──── + + ⚠ eslint-plugin-promise(no-return-wrap): Avoid wrapping return values in Promise.resolve + ╭─[no_return_wrap.tsx:1:23] + 1 │ doThing1().then(() => Promise.resolve(9)) + · ────────────────── + ╰──── + + ⚠ eslint-plugin-promise(no-return-wrap): Expected throw instead of Promise.reject + ╭─[no_return_wrap.tsx:1:22] + 1 │ doThing().then(() => Promise.reject(4)) + · ───────────────── + ╰──── From 86c8dfae8e10be2f1983600ba5daf919eb9a2fd6 Mon Sep 17 00:00:00 2001 From: Jelle van der Waa Date: Sun, 14 Jul 2024 15:28:43 +0200 Subject: [PATCH 09/13] feat(linter/eslint-plugin-promise): implement no-promise-in-callback Rule detail: [link](https://github.com/eslint-community/eslint-plugin-promise/blob/main/docs/rules/no-promise-in-callback.md) --- crates/oxc_linter/src/rules.rs | 2 + .../rules/promise/no_promise_in_callback.rs | 176 ++++++++++++++++++ .../src/snapshots/no_promise_in_callback.snap | 68 +++++++ 3 files changed, 246 insertions(+) create mode 100644 crates/oxc_linter/src/rules/promise/no_promise_in_callback.rs create mode 100644 crates/oxc_linter/src/snapshots/no_promise_in_callback.snap diff --git a/crates/oxc_linter/src/rules.rs b/crates/oxc_linter/src/rules.rs index 5c5d89e68017b..8334f0dcd75e6 100644 --- a/crates/oxc_linter/src/rules.rs +++ b/crates/oxc_linter/src/rules.rs @@ -435,6 +435,7 @@ mod tree_shaking { mod promise { pub mod avoid_new; pub mod no_new_statics; + pub mod no_promise_in_callback; pub mod no_return_in_finally; pub mod no_return_wrap; pub mod param_names; @@ -837,4 +838,5 @@ oxc_macros::declare_all_lint_rules! { promise::valid_params, promise::no_return_in_finally, promise::no_return_wrap, + promise::no_promise_in_callback, } diff --git a/crates/oxc_linter/src/rules/promise/no_promise_in_callback.rs b/crates/oxc_linter/src/rules/promise/no_promise_in_callback.rs new file mode 100644 index 0000000000000..d7b3e0474bc4b --- /dev/null +++ b/crates/oxc_linter/src/rules/promise/no_promise_in_callback.rs @@ -0,0 +1,176 @@ +use oxc_ast::{ + ast::{BindingPatternKind, FormalParameter}, + AstKind, +}; +use oxc_diagnostics::OxcDiagnostic; +use oxc_macros::declare_oxc_lint; +use oxc_span::Span; + +use crate::{ast_util::is_promise, context::LintContext, rule::Rule, AstNode}; + +fn no_promise_in_callback_diagnostic(span0: Span) -> OxcDiagnostic { + OxcDiagnostic::warn( + "eslint-plugin-promise(no-promise-in-callback): Avoid using promises inside of callbacks", + ) + .with_label(span0) +} + +#[derive(Debug, Default, Clone)] +pub struct NoPromiseInCallback; + +declare_oxc_lint!( + /// ### What it does + /// + /// Disallow using promises inside of callbacks. + /// + /// ### Why is this bad? + /// + /// + /// ### Example + /// ```javascript + /// a(function(err) { doThing().then(a) }) + /// function x(err) { Promise.all() } + /// ``` + NoPromiseInCallback, + style, +); + +fn is_inside_callback<'a>(node: &AstNode<'a>, ctx: &LintContext<'a>) -> bool { + if !matches!(node.kind(), AstKind::ArrowFunctionExpression(_) | AstKind::Function(_)) { + return false; + } + + // is callback with "err" or "error" as first parameter + if !is_callback(node) { + return false; + } + + // Don't warn about valid chained promises + if is_inside_promise(node, ctx) { + return false; + } + + true +} + +impl Rule for NoPromiseInCallback { + fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { + let AstKind::CallExpression(call_expr) = node.kind() else { + return; + }; + + if !is_promise(call_expr) { + return; + } + + // Don't warn about returning promises, it's likely not really a callback function. + let Some(parent) = ctx.nodes().parent_node(node.id()) else { + return; + }; + + if matches!(parent.kind(), AstKind::ReturnStatement(_)) { + return; + } + + if ctx + .nodes() + .ancestors(node.id()) + .any(|node_id| is_inside_callback(ctx.nodes().get_node(node_id), ctx)) + { + ctx.diagnostic(no_promise_in_callback_diagnostic(call_expr.span)); + } + } +} + +fn is_inside_promise<'a>(node: &AstNode<'a>, ctx: &LintContext<'a>) -> bool { + // kind: Argument(FunctionExpression) + let Some(func_argument_expr) = ctx.nodes().parent_node(node.id()) else { + return false; + }; + + let Some(call_expr_node) = ctx.nodes().parent_node(func_argument_expr.id()) else { + return false; + }; + + let AstKind::CallExpression(call_expr) = call_expr_node.kind() else { + return false; + }; + + 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; + }; + + if !matches!(prop_name, "catch" | "then") { + return false; + } + + true +} + +fn get_first_parameter<'a>(node: &AstNode<'a>) -> Option<&'a FormalParameter<'a>> { + return match node.kind() { + AstKind::Function(func) => func.params.items.first(), + AstKind::ArrowFunctionExpression(arrow) => arrow.params.items.first(), + _ => None, + }; +} + +fn is_callback(node: &AstNode) -> bool { + let Some(first_param) = get_first_parameter(node) else { return false }; + + let BindingPatternKind::BindingIdentifier(first_param_ident) = &first_param.pattern.kind else { + return false; + }; + + if !matches!(first_param_ident.name.as_str(), "error" | "err") { + return false; + } + + true +} + +#[test] +fn test() { + use crate::tester::Tester; + + let pass = vec![ + "go(function() { return Promise.resolve(4) })", + "go(function() { return a.then(b) })", + "go(function() { b.catch(c) })", + "go(function() { b.then(c, d) })", + "go(() => Promise.resolve(4))", + "go((errrr) => a.then(b))", + "go((helpers) => { b.catch(c) })", + "go((e) => { b.then(c, d) })", + "a.catch((err) => { b.then(c, d) })", + "var x = function() { return Promise.resolve(4) }", + "function y() { return Promise.resolve(4) }", + "function then() { return Promise.reject() }", + "doThing(function(x) { return Promise.reject(x) })", + "doThing().then(function() { return Promise.all([a,b,c]) })", + "doThing().then(function() { return Promise.resolve(4) })", + "doThing().then(() => Promise.resolve(4))", + "doThing().then(() => Promise.all([a]))", + "a(function(err) { return doThing().then(a) })", + ]; + + let fail = vec![ + "a(function(err) { doThing().then(a) })", + "a(function(error, zup, supa) { doThing().then(a) })", + "a(function(error) { doThing().then(a) })", + "a((error) => { doThing().then(a) })", + "a((error) => doThing().then(a))", + "a((err, data) => { doThing().then(a) })", + "a((err, data) => doThing().then(a))", + "function x(err) { Promise.all() }", + "function x(err) { Promise.allSettled() }", + "function x(err) { Promise.any() }", + "let x = (err) => doThingWith(err).then(a)", + ]; + + Tester::new(NoPromiseInCallback::NAME, pass, fail).test_and_snapshot(); +} diff --git a/crates/oxc_linter/src/snapshots/no_promise_in_callback.snap b/crates/oxc_linter/src/snapshots/no_promise_in_callback.snap new file mode 100644 index 0000000000000..e69785a091300 --- /dev/null +++ b/crates/oxc_linter/src/snapshots/no_promise_in_callback.snap @@ -0,0 +1,68 @@ +--- +source: crates/oxc_linter/src/tester.rs +--- + ⚠ eslint-plugin-promise(no-promise-in-callback): Avoid using promises inside of callbacks + ╭─[no_promise_in_callback.tsx:1:19] + 1 │ a(function(err) { doThing().then(a) }) + · ───────────────── + ╰──── + + ⚠ eslint-plugin-promise(no-promise-in-callback): Avoid using promises inside of callbacks + ╭─[no_promise_in_callback.tsx:1:32] + 1 │ a(function(error, zup, supa) { doThing().then(a) }) + · ───────────────── + ╰──── + + ⚠ eslint-plugin-promise(no-promise-in-callback): Avoid using promises inside of callbacks + ╭─[no_promise_in_callback.tsx:1:21] + 1 │ a(function(error) { doThing().then(a) }) + · ───────────────── + ╰──── + + ⚠ eslint-plugin-promise(no-promise-in-callback): Avoid using promises inside of callbacks + ╭─[no_promise_in_callback.tsx:1:16] + 1 │ a((error) => { doThing().then(a) }) + · ───────────────── + ╰──── + + ⚠ eslint-plugin-promise(no-promise-in-callback): Avoid using promises inside of callbacks + ╭─[no_promise_in_callback.tsx:1:14] + 1 │ a((error) => doThing().then(a)) + · ───────────────── + ╰──── + + ⚠ eslint-plugin-promise(no-promise-in-callback): Avoid using promises inside of callbacks + ╭─[no_promise_in_callback.tsx:1:20] + 1 │ a((err, data) => { doThing().then(a) }) + · ───────────────── + ╰──── + + ⚠ eslint-plugin-promise(no-promise-in-callback): Avoid using promises inside of callbacks + ╭─[no_promise_in_callback.tsx:1:18] + 1 │ a((err, data) => doThing().then(a)) + · ───────────────── + ╰──── + + ⚠ eslint-plugin-promise(no-promise-in-callback): Avoid using promises inside of callbacks + ╭─[no_promise_in_callback.tsx:1:19] + 1 │ function x(err) { Promise.all() } + · ───────────── + ╰──── + + ⚠ eslint-plugin-promise(no-promise-in-callback): Avoid using promises inside of callbacks + ╭─[no_promise_in_callback.tsx:1:19] + 1 │ function x(err) { Promise.allSettled() } + · ──────────────────── + ╰──── + + ⚠ eslint-plugin-promise(no-promise-in-callback): Avoid using promises inside of callbacks + ╭─[no_promise_in_callback.tsx:1:19] + 1 │ function x(err) { Promise.any() } + · ───────────── + ╰──── + + ⚠ eslint-plugin-promise(no-promise-in-callback): Avoid using promises inside of callbacks + ╭─[no_promise_in_callback.tsx:1:18] + 1 │ let x = (err) => doThingWith(err).then(a) + · ──────────────────────── + ╰──── From ffa143ce55310575670b888f005b2d6cfce61208 Mon Sep 17 00:00:00 2001 From: Jelle van der Waa Date: Sun, 14 Jul 2024 16:13:49 +0200 Subject: [PATCH 10/13] feat(eslint-plugin-promise): implement prefer-await-to-then Rule detail: [link](https://github.com/eslint-community/eslint-plugin-promise/blob/main/docs/rules/prefer-await-to-then.md) --- crates/oxc_linter/src/rules.rs | 2 + .../src/rules/promise/prefer_await_to_then.rs | 107 ++++++++++++++++++ .../src/snapshots/prefer_await_to_then.snap | 62 ++++++++++ 3 files changed, 171 insertions(+) create mode 100644 crates/oxc_linter/src/rules/promise/prefer_await_to_then.rs 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 8334f0dcd75e6..1cd42c99ac679 100644 --- a/crates/oxc_linter/src/rules.rs +++ b/crates/oxc_linter/src/rules.rs @@ -439,6 +439,7 @@ mod promise { pub mod no_return_in_finally; pub mod no_return_wrap; pub mod param_names; + pub mod prefer_await_to_then; pub mod valid_params; } @@ -839,4 +840,5 @@ oxc_macros::declare_all_lint_rules! { promise::no_return_in_finally, promise::no_return_wrap, promise::no_promise_in_callback, + promise::prefer_await_to_then, } 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..fc0732472bb91 --- /dev/null +++ b/crates/oxc_linter/src/rules/promise/prefer_await_to_then.rs @@ -0,0 +1,107 @@ +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( + "eslint-plugin-promise(prefer-await-to-then): Prefer await to then()/catch()/finally()", + ) + .with_label(span0) +} + +use crate::{context::LintContext, rule::Rule, 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; + }; + + let Some(member_expr) = call_expr.callee.get_member_expr() else { + return; + }; + + let Some(prop_name) = member_expr.static_property_name() else { + return; + }; + + if !matches!(prop_name, "then" | "catch" | "finally") { + return; + } + + // Await statements cannot be added to the top level scope + if ctx.scopes().get_flags(node.scope_id()).is_top() { + 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() } + }", + "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 => {}) }", + ]; + + Tester::new(PreferAwaitToThen::NAME, pass, fail).test_and_snapshot(); +} 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..990f057288bac --- /dev/null +++ b/crates/oxc_linter/src/snapshots/prefer_await_to_then.snap @@ -0,0 +1,62 @@ +--- +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 => {}) } + · ──────────────────── + ╰──── From 13510f37ddacb6c6895b786eb92b046ec87f509d Mon Sep 17 00:00:00 2001 From: Jelle van der Waa Date: Sun, 14 Jul 2024 17:53:43 +0200 Subject: [PATCH 11/13] refactor(linter): introduce promise utility module --- crates/oxc_linter/src/utils/mod.rs | 3 +- crates/oxc_linter/src/utils/promise.rs | 49 ++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) create mode 100644 crates/oxc_linter/src/utils/promise.rs diff --git a/crates/oxc_linter/src/utils/mod.rs b/crates/oxc_linter/src/utils/mod.rs index 331c67ad05ecc..5ec14ba37d90a 100644 --- a/crates/oxc_linter/src/utils/mod.rs +++ b/crates/oxc_linter/src/utils/mod.rs @@ -1,6 +1,7 @@ mod jest; mod jsdoc; mod nextjs; +mod promise; mod react; mod react_perf; mod tree_shaking; @@ -9,7 +10,7 @@ mod unicorn; use crate::LintContext; pub use self::{ - jest::*, jsdoc::*, nextjs::*, react::*, react_perf::*, tree_shaking::*, unicorn::*, + jest::*, jsdoc::*, nextjs::*, promise::*, react::*, react_perf::*, tree_shaking::*, unicorn::*, }; /// Check if the Jest rule is adapted to Vitest. diff --git a/crates/oxc_linter/src/utils/promise.rs b/crates/oxc_linter/src/utils/promise.rs new file mode 100644 index 0000000000000..83ff951cc3596 --- /dev/null +++ b/crates/oxc_linter/src/utils/promise.rs @@ -0,0 +1,49 @@ +use oxc_ast::{ast::CallExpression, AstKind}; +use oxc_semantic::AstNode; + +use crate::LintContext; + +pub fn is_promise_callback(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; + }; + + if !matches!(prop_name, "catch" | "then") { + return false; + } + + true +} + +pub fn is_inside_promise<'a>(node: &AstNode<'a>, ctx: &LintContext<'a>) -> bool { + // kind: Argument(FunctionExpression) + let Some(func_argument_expr) = ctx.nodes().parent_node(node.id()) else { + return false; + }; + + let Some(call_expr_node) = ctx.nodes().parent_node(func_argument_expr.id()) else { + return false; + }; + + let AstKind::CallExpression(call_expr) = call_expr_node.kind() else { + return false; + }; + + 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; + }; + + if !matches!(prop_name, "catch" | "then") { + return false; + } + + true +} From 1ffd302e29eed126b8fd0ed3e58b4e8e91a25514 Mon Sep 17 00:00:00 2001 From: Jelle van der Waa Date: Sun, 14 Jul 2024 17:54:22 +0200 Subject: [PATCH 12/13] feat(linter/eslint-plugin-promise): implement no-callback-in-promise Detail: [link](https://github.com/eslint-community/eslint-plugin-promise/blob/main/docs/rules/no-callback-in-promise.md) --- crates/oxc_linter/src/rules.rs | 2 + .../rules/promise/no_callback_in_promise.rs | 158 ++++++++++++++++++ .../src/snapshots/no_callback_in_promise.snap | 74 ++++++++ 3 files changed, 234 insertions(+) create mode 100644 crates/oxc_linter/src/rules/promise/no_callback_in_promise.rs create mode 100644 crates/oxc_linter/src/snapshots/no_callback_in_promise.snap diff --git a/crates/oxc_linter/src/rules.rs b/crates/oxc_linter/src/rules.rs index 1cd42c99ac679..b60603e32cff3 100644 --- a/crates/oxc_linter/src/rules.rs +++ b/crates/oxc_linter/src/rules.rs @@ -434,6 +434,7 @@ mod tree_shaking { mod promise { pub mod avoid_new; + pub mod no_callback_in_promise; pub mod no_new_statics; pub mod no_promise_in_callback; pub mod no_return_in_finally; @@ -841,4 +842,5 @@ oxc_macros::declare_all_lint_rules! { promise::no_return_wrap, promise::no_promise_in_callback, promise::prefer_await_to_then, + promise::no_callback_in_promise, } diff --git a/crates/oxc_linter/src/rules/promise/no_callback_in_promise.rs b/crates/oxc_linter/src/rules/promise/no_callback_in_promise.rs new file mode 100644 index 0000000000000..6f5be34b43125 --- /dev/null +++ b/crates/oxc_linter/src/rules/promise/no_callback_in_promise.rs @@ -0,0 +1,158 @@ +use oxc_ast::{ast::Expression, AstKind}; +use oxc_diagnostics::OxcDiagnostic; +use oxc_macros::declare_oxc_lint; +use oxc_span::Span; +use phf::phf_set; + +use crate::{ + context::LintContext, + rule::Rule, + utils::{is_inside_promise, is_promise_callback}, + AstNode, +}; + +fn no_callback_in_promise_diagnostic(span0: Span) -> OxcDiagnostic { + OxcDiagnostic::warn( + "eslint-plugin-promise(no-callback-in-promise): Avoid calling back inside of a promise", + ) + .with_label(span0) +} + +#[derive(Debug, Default, Clone)] +pub struct NoCallbackInPromise(Box); + +#[derive(Debug, Default, Clone)] +pub struct NoCallbackInPromiseConfig { + exceptions: Vec, +} + +impl std::ops::Deref for NoCallbackInPromise { + type Target = NoCallbackInPromiseConfig; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +declare_oxc_lint!( + /// ### What it does + /// + /// Disallow calling cb() inside of a then() (use nodeify instead). + /// + /// ### Why is this bad? + /// + /// As a general rule, callbacks should never be directly invoked inside a + /// Promise.prototype.then() or Promise.prototype.catch() method. That's because your callback + /// may be unintentionally be invoked twice. It also can be confusing to mix paradigms. + /// + /// ### Example + /// ```javascript + /// Promise.resolve() + /// .then(() => callback(null, 'data')) + /// .catch((err) => callback(err.message, null)) + /// ``` + NoCallbackInPromise, + style, +); + +pub const CALLBACK_NAMES: phf::Set<&'static str> = phf_set! { + "done", + "cb", + "callback", + "next", +}; + +impl Rule for NoCallbackInPromise { + fn from_configuration(value: serde_json::Value) -> Self { + let obj = value.get(0); + + Self(Box::new(NoCallbackInPromiseConfig { + exceptions: obj + .and_then(|v| v.get("exceptions")) + .and_then(serde_json::Value::as_array) + .unwrap_or(&vec![]) + .iter() + .map(serde_json::Value::as_str) + .filter(Option::is_some) + .map(|x| x.unwrap().into()) + .collect::>(), + })) + } + + fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { + let AstKind::CallExpression(call_expr) = node.kind() else { + return; + }; + + if call_expr.callee_name().is_some_and(|name| { + CALLBACK_NAMES.contains(name) && !self.exceptions.contains(&name.to_string()) + }) { + if ctx + .nodes() + .ancestors(node.id()) + .any(|node_id| is_inside_callback(ctx.nodes().get_node(node_id), ctx)) + { + ctx.diagnostic(no_callback_in_promise_diagnostic(call_expr.span)); + } + } else if is_promise_callback(call_expr) { + if call_expr.arguments.is_empty() { + return; + } + + let Some(Expression::Identifier(first_arg)) = &call_expr.arguments[0].as_expression() + else { + return; + }; + + if !self.exceptions.contains(&first_arg.name.to_string()) + && CALLBACK_NAMES.contains(first_arg.name.as_ref()) + { + ctx.diagnostic(no_callback_in_promise_diagnostic(first_arg.span)); + } + } + } +} + +fn is_inside_callback<'a>(node: &AstNode<'a>, ctx: &LintContext<'a>) -> bool { + if !matches!(node.kind(), AstKind::ArrowFunctionExpression(_) | AstKind::Function(_)) { + return false; + } + + is_inside_promise(node, ctx) +} + +#[test] +fn test() { + use crate::tester::Tester; + + let pass = vec![ + ("function thing(cb) { cb() }", None), + ("doSomething(function(err) { cb(err) })", None), + ("function thing(callback) { callback() }", None), + ("doSomething(function(err) { callback(err) })", None), + ("let thing = (cb) => cb()", None), + ("doSomething(err => cb(err))", None), + ("a.then(() => next())", Some(serde_json::json!([{ "exceptions": ["next"] }]))), + ( + "a.then(() => next()).catch((err) => next(err))", + Some(serde_json::json!([{ "exceptions": ["next"] }])), + ), + ("a.then(next)", Some(serde_json::json!([{ "exceptions": ["next"] }]))), + ("a.then(next).catch(next)", Some(serde_json::json!([{ "exceptions": ["next"] }]))), + ]; + + let fail = vec![ + ("a.then(cb)", None), + ("a.then(() => cb())", None), + ("a.then(function(err) { cb(err) })", None), + ("a.then(function(data) { cb(data) }, function(err) { cb(err) })", None), + ("a.catch(function(err) { cb(err) })", None), + ("a.then(callback)", None), + ("a.then(() => callback())", None), + ("a.then(function(err) { callback(err) })", None), + ("a.then(function(data) { callback(data) }, function(err) { callback(err) })", None), + ("a.catch(function(err) { callback(err) })", None), + ]; + + Tester::new(NoCallbackInPromise::NAME, pass, fail).test_and_snapshot(); +} diff --git a/crates/oxc_linter/src/snapshots/no_callback_in_promise.snap b/crates/oxc_linter/src/snapshots/no_callback_in_promise.snap new file mode 100644 index 0000000000000..5382440b571fd --- /dev/null +++ b/crates/oxc_linter/src/snapshots/no_callback_in_promise.snap @@ -0,0 +1,74 @@ +--- +source: crates/oxc_linter/src/tester.rs +--- + ⚠ eslint-plugin-promise(no-callback-in-promise): Avoid calling back inside of a promise + ╭─[no_callback_in_promise.tsx:1:8] + 1 │ a.then(cb) + · ── + ╰──── + + ⚠ eslint-plugin-promise(no-callback-in-promise): Avoid calling back inside of a promise + ╭─[no_callback_in_promise.tsx:1:14] + 1 │ a.then(() => cb()) + · ──── + ╰──── + + ⚠ eslint-plugin-promise(no-callback-in-promise): Avoid calling back inside of a promise + ╭─[no_callback_in_promise.tsx:1:24] + 1 │ a.then(function(err) { cb(err) }) + · ─────── + ╰──── + + ⚠ eslint-plugin-promise(no-callback-in-promise): Avoid calling back inside of a promise + ╭─[no_callback_in_promise.tsx:1:25] + 1 │ a.then(function(data) { cb(data) }, function(err) { cb(err) }) + · ──────── + ╰──── + + ⚠ eslint-plugin-promise(no-callback-in-promise): Avoid calling back inside of a promise + ╭─[no_callback_in_promise.tsx:1:53] + 1 │ a.then(function(data) { cb(data) }, function(err) { cb(err) }) + · ─────── + ╰──── + + ⚠ eslint-plugin-promise(no-callback-in-promise): Avoid calling back inside of a promise + ╭─[no_callback_in_promise.tsx:1:25] + 1 │ a.catch(function(err) { cb(err) }) + · ─────── + ╰──── + + ⚠ eslint-plugin-promise(no-callback-in-promise): Avoid calling back inside of a promise + ╭─[no_callback_in_promise.tsx:1:8] + 1 │ a.then(callback) + · ──────── + ╰──── + + ⚠ eslint-plugin-promise(no-callback-in-promise): Avoid calling back inside of a promise + ╭─[no_callback_in_promise.tsx:1:14] + 1 │ a.then(() => callback()) + · ────────── + ╰──── + + ⚠ eslint-plugin-promise(no-callback-in-promise): Avoid calling back inside of a promise + ╭─[no_callback_in_promise.tsx:1:24] + 1 │ a.then(function(err) { callback(err) }) + · ───────────── + ╰──── + + ⚠ eslint-plugin-promise(no-callback-in-promise): Avoid calling back inside of a promise + ╭─[no_callback_in_promise.tsx:1:25] + 1 │ a.then(function(data) { callback(data) }, function(err) { callback(err) }) + · ────────────── + ╰──── + + ⚠ eslint-plugin-promise(no-callback-in-promise): Avoid calling back inside of a promise + ╭─[no_callback_in_promise.tsx:1:59] + 1 │ a.then(function(data) { callback(data) }, function(err) { callback(err) }) + · ───────────── + ╰──── + + ⚠ eslint-plugin-promise(no-callback-in-promise): Avoid calling back inside of a promise + ╭─[no_callback_in_promise.tsx:1:25] + 1 │ a.catch(function(err) { callback(err) }) + · ───────────── + ╰──── From c01a85dab207442da082ee8037bcdb68108b8c8a Mon Sep 17 00:00:00 2001 From: Jelle van der Waa Date: Sun, 14 Jul 2024 22:58:04 +0200 Subject: [PATCH 13/13] feat(linter/eslint-plugin-promise): implement catch-or-return Rule detail: [link](https://github.com/eslint-community/eslint-plugin-promise/blob/main/docs/rules/catch-or-return.md) --- crates/oxc_linter/src/rules.rs | 2 + .../src/rules/promise/catch_or_return.rs | 293 ++++++++++++++++++ .../src/snapshots/catch_or_return.snap | 146 +++++++++ 3 files changed, 441 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 b60603e32cff3..2eaa17a21f0bd 100644 --- a/crates/oxc_linter/src/rules.rs +++ b/crates/oxc_linter/src/rules.rs @@ -434,6 +434,7 @@ mod tree_shaking { mod promise { pub mod avoid_new; + pub mod catch_or_return; pub mod no_callback_in_promise; pub mod no_new_statics; pub mod no_promise_in_callback; @@ -843,4 +844,5 @@ oxc_macros::declare_all_lint_rules! { promise::no_promise_in_callback, promise::prefer_await_to_then, promise::no_callback_in_promise, + promise::catch_or_return, } 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..c1bab0dddb3bc --- /dev/null +++ b/crates/oxc_linter/src/rules/promise/catch_or_return.rs @@ -0,0 +1,293 @@ +use oxc_ast::{ + ast::{CallExpression, Expression}, + AstKind, +}; +use oxc_diagnostics::OxcDiagnostic; +use oxc_macros::declare_oxc_lint; +use oxc_span::Span; + +use crate::{ast_util::is_promise, context::LintContext, rule::Rule, 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_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!["catch".to_string()], + } + } +} + +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 + /// ```javascript + /// myPromise.then(doSomething) + /// myPromise.then(doSomething, catchErrors) // catch() may be a little better + /// 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(ToString::to_string) + .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![termination_string_config.to_string()]; + } + + 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; + }; + + if !is_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) && self.is_allowed_promise_termination(object_call_expr) + { + return true; + } + } + + // somePromise.catch() + if self.termination_method.contains(&prop_name.to_string()) { + return true; + } + + // somePromise['catch']() + if prop_name == "catch" + && matches!(call_expr.callee.get_inner_expression(), Expression::StringLiteral(_)) + { + return true; + } + + false + } +} + +#[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), + ]; + + 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..b12afdb32a225 --- /dev/null +++ b/crates/oxc_linter/src/snapshots/catch_or_return.snap @@ -0,0 +1,146 @@ +--- +source: crates/oxc_linter/src/tester.rs +--- + ⚠ eslint-plugin-promise(catch-or-return): Expected catch or return + ╭─[catch_or_return.tsx:1:37] + 1 │ function callPromise(promise, cb) { promise.then(cb) } + · ──────────────── + ╰──── + + ⚠ 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)) + · ───────────────────────────────────────────────────────────── + ╰──── + + ⚠ 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; }) + · ───────────────────────────────────────────────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(catch-or-return): Expected catch or return + ╭─[catch_or_return.tsx:1:1] + 1 │ Promise.resolve(frank) + · ────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(catch-or-return): Expected catch or return + ╭─[catch_or_return.tsx:1:1] + 1 │ Promise.all([]) + · ─────────────── + ╰──── + + ⚠ eslint-plugin-promise(catch-or-return): Expected catch or return + ╭─[catch_or_return.tsx:1:1] + 1 │ Promise.allSettled([]) + · ────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(catch-or-return): Expected catch or return + ╭─[catch_or_return.tsx:1:1] + 1 │ Promise.any([]) + · ─────────────── + ╰──── + + ⚠ eslint-plugin-promise(catch-or-return): Expected catch or return + ╭─[catch_or_return.tsx:1:1] + 1 │ Promise.race([]) + · ──────────────── + ╰──── + + ⚠ eslint-plugin-promise(catch-or-return): Expected catch or return + ╭─[catch_or_return.tsx:1:1] + 1 │ frank().then(to).catch(fn).then(foo) + · ──────────────────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(catch-or-return): Expected catch or return + ╭─[catch_or_return.tsx:1:1] + 1 │ frank().finally(fn) + · ─────────────────── + ╰──── + + ⚠ eslint-plugin-promise(catch-or-return): Expected catch or return + ╭─[catch_or_return.tsx:1:1] + 1 │ frank().then(to).finally(fn) + · ──────────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(catch-or-return): Expected catch or return + ╭─[catch_or_return.tsx:1:1] + 1 │ frank().then(go).catch(doIt).finally(fn) + · ──────────────────────────────────────── + ╰──── + + ⚠ 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) + · ───────────────────────────────────────────────────────────── + ╰──── + + ⚠ 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) + · ────────────────────────────────────────────────────────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(catch-or-return): Expected catch or return + ╭─[catch_or_return.tsx:1:16] + 1 │ function a() { frank().then(go) } + · ──────────────── + ╰──── + + ⚠ 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() } + · ───────────────────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(catch-or-return): Expected catch or return + ╭─[catch_or_return.tsx:1:16] + 1 │ function a() { frank().then(go).then()} + · ─────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(catch-or-return): Expected catch or return + ╭─[catch_or_return.tsx:1:16] + 1 │ function a() { frank.then(go).then(to) } + · ─────────────────────── + ╰──── + + ⚠ 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) + · ────────────────────────────────────────────────── + ╰──── + + ⚠ 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) + · ──────────────────────────────────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(catch-or-return): Expected done or return + ╭─[catch_or_return.tsx:1:1] + 1 │ frank().then(go) + · ──────────────── + ╰──── + + ⚠ eslint-plugin-promise(catch-or-return): Expected done or return + ╭─[catch_or_return.tsx:1:1] + 1 │ frank().catch(go) + · ───────────────── + ╰──── + + ⚠ eslint-plugin-promise(catch-or-return): Expected catch or return + ╭─[catch_or_return.tsx:1:1] + 1 │ frank().catch(go).someOtherMethod() + · ─────────────────────────────────── + ╰──── + + ⚠ eslint-plugin-promise(catch-or-return): Expected catch or return + ╭─[catch_or_return.tsx:1:1] + 1 │ frank()['catch'](go).someOtherMethod() + · ────────────────────────────────────── + ╰────