From c0b2cf1731572d5ac010895d226155dcec16bbaf Mon Sep 17 00:00:00 2001 From: Jelle van der Waa Date: Sat, 13 Jul 2024 20:30:51 +0200 Subject: [PATCH] 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/no_new_statics.rs | 8 +- .../src/rules/promise/valid_params.rs | 189 ++++++++++++++++++ .../src/snapshots/valid_params.snap | 170 ++++++++++++++++ crates/oxc_linter/src/utils/mod.rs | 4 +- crates/oxc_linter/src/utils/promise.rs | 30 +++ 6 files changed, 396 insertions(+), 7 deletions(-) create mode 100644 crates/oxc_linter/src/rules/promise/valid_params.rs create mode 100644 crates/oxc_linter/src/snapshots/valid_params.snap create mode 100644 crates/oxc_linter/src/utils/promise.rs diff --git a/crates/oxc_linter/src/rules.rs b/crates/oxc_linter/src/rules.rs index 56e650eb547d8..48ee7702bba30 100644 --- a/crates/oxc_linter/src/rules.rs +++ b/crates/oxc_linter/src/rules.rs @@ -445,6 +445,7 @@ mod promise { pub mod avoid_new; pub mod no_new_statics; pub mod param_names; + pub mod valid_params; } mod vitest { @@ -854,6 +855,7 @@ oxc_macros::declare_all_lint_rules! { promise::avoid_new, promise::no_new_statics, promise::param_names, + promise::valid_params, vitest::no_import_node_test, vitest::prefer_to_be_truthy, } diff --git a/crates/oxc_linter/src/rules/promise/no_new_statics.rs b/crates/oxc_linter/src/rules/promise/no_new_statics.rs index 2154fe36a8582..8aaba0ac6d5db 100644 --- a/crates/oxc_linter/src/rules/promise/no_new_statics.rs +++ b/crates/oxc_linter/src/rules/promise/no_new_statics.rs @@ -3,7 +3,7 @@ use oxc_diagnostics::OxcDiagnostic; use oxc_macros::declare_oxc_lint; use oxc_span::Span; -use crate::{context::LintContext, rule::Rule, AstNode}; +use crate::{context::LintContext, rule::Rule, utils::PROMISE_STATIC_METHODS, AstNode}; fn static_promise_diagnostic(x0: &str, span0: Span) -> OxcDiagnostic { OxcDiagnostic::warn(format!("Disallow calling `new` on a `Promise.{x0}`")).with_label(span0) @@ -52,11 +52,7 @@ impl Rule for NoNewStatics { 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" - ) { + if PROMISE_STATIC_METHODS.contains(prop_name) { ctx.diagnostic_with_fix( static_promise_diagnostic( prop_name, 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..40c0c1fe319c0 --- /dev/null +++ b/crates/oxc_linter/src/rules/promise/valid_params.rs @@ -0,0 +1,189 @@ +use oxc_ast::AstKind; +use oxc_diagnostics::OxcDiagnostic; +use oxc_macros::declare_oxc_lint; +use oxc_span::Span; + +use crate::{context::LintContext, rule::Rule, utils::is_promise, AstNode}; + +fn zero_or_one_argument_required_diagnostic( + span0: Span, + prop_name: &str, + args_len: usize, +) -> OxcDiagnostic { + OxcDiagnostic::warn(format!( + "Promise.{prop_name}() requires 0 or 1 arguments, but received {args_len}" + )) + .with_label(span0) +} + +fn one_or_two_argument_required_diagnostic( + span0: Span, + prop_name: &str, + args_len: usize, +) -> OxcDiagnostic { + OxcDiagnostic::warn(format!( + "Promise.{prop_name}() requires 1 or 2 arguments, but received {args_len}" + )) + .with_label(span0) +} + +fn one_argument_required_diagnostic( + span0: Span, + prop_name: &str, + args_len: usize, +) -> OxcDiagnostic { + OxcDiagnostic::warn(format!( + "Promise.{prop_name}() requires 1 argument, but received {args_len}" + )) + .with_label(span0) +} + +fn valid_params_diagnostic(span0: Span, x0: &str) -> OxcDiagnostic { + OxcDiagnostic::warn(x0.to_string()).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, +); + +impl Rule for ValidParams { + fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { + let AstKind::CallExpression(call_expr) = node.kind() else { + return; + }; + + let Some(prop_name) = is_promise(call_expr) else { + return; + }; + + let args_len = call_expr.arguments.len(); + + match prop_name.as_str() { + "resolve" | "reject" => { + if args_len > 1 { + ctx.diagnostic(zero_or_one_argument_required_diagnostic( + call_expr.span, + &prop_name, + args_len, + )); + } + } + "then" => { + if args_len != 1 && args_len != 2 { + ctx.diagnostic(one_or_two_argument_required_diagnostic( + call_expr.span, + &prop_name, + 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(one_argument_required_diagnostic( + call_expr.span, + &prop_name, + 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..67989442e9dff --- /dev/null +++ b/crates/oxc_linter/src/snapshots/valid_params.snap @@ -0,0 +1,170 @@ +--- +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 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 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 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.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(() => {}, () => {}) + · ──────────────────────────────────────────── + ╰──── diff --git a/crates/oxc_linter/src/utils/mod.rs b/crates/oxc_linter/src/utils/mod.rs index 7e65a155456f0..e8a2fb2fba40c 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; @@ -8,7 +9,8 @@ mod unicorn; mod vitest; pub use self::{ - jest::*, jsdoc::*, nextjs::*, react::*, react_perf::*, tree_shaking::*, unicorn::*, vitest::*, + jest::*, jsdoc::*, nextjs::*, promise::*, react::*, react_perf::*, tree_shaking::*, unicorn::*, + vitest::*, }; /// 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..76a61dfecf79f --- /dev/null +++ b/crates/oxc_linter/src/utils/promise.rs @@ -0,0 +1,30 @@ +use oxc_ast::ast::CallExpression; +use phf::{phf_set, Set}; + +// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise +pub const PROMISE_STATIC_METHODS: Set<&'static str> = phf_set! { + "resolve", + "reject", + "all", + "allSettled", + "race", + "any", + "withResolvers", +}; + +pub fn is_promise(call_expr: &CallExpression) -> Option { + let member_expr = call_expr.callee.get_member_expr()?; + let prop_name = member_expr.static_property_name()?; + + // hello.then(), hello.catch(), hello.finally() + if matches!(prop_name, "then" | "catch" | "finally") { + return Some(prop_name.into()); + } + + if member_expr.object().is_specific_id("Promise") && PROMISE_STATIC_METHODS.contains(prop_name) + { + return Some(prop_name.into()); + } + + None +}