diff --git a/crates/oxc_linter/src/rules.rs b/crates/oxc_linter/src/rules.rs index b1fbdff96088a..4680894cb2a6c 100644 --- a/crates/oxc_linter/src/rules.rs +++ b/crates/oxc_linter/src/rules.rs @@ -471,6 +471,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_return_in_finally; pub mod param_names; @@ -765,6 +766,7 @@ oxc_macros::declare_all_lint_rules! { oxc::uninvoked_array_callback, promise::avoid_new, promise::catch_or_return, + promise::no_callback_in_promise, promise::no_new_statics, promise::no_return_in_finally, promise::param_names, 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..507eda7786b10 --- /dev/null +++ b/crates/oxc_linter/src/rules/promise/no_callback_in_promise.rs @@ -0,0 +1,186 @@ +use oxc_ast::{ + ast::{CallExpression, Expression, MemberExpression}, + AstKind, +}; +use oxc_diagnostics::OxcDiagnostic; +use oxc_macros::declare_oxc_lint; +use oxc_span::{CompactStr, GetSpan, Span}; + +use crate::{context::LintContext, rule::Rule, AstNode}; + +fn no_callback_in_promise_diagnostic(span: Span) -> OxcDiagnostic { + OxcDiagnostic::warn("Avoid calling back inside of a promise") + .with_help("Use `then` and `catch` directly") + .with_label(span) +} + +#[derive(Debug, Default, Clone)] +pub struct NoCallbackInPromise(Box); + +#[derive(Debug, Clone)] +pub struct NoCallbackInPromiseConfig { + callbacks: Vec, +} + +impl Default for NoCallbackInPromiseConfig { + fn default() -> Self { + Self { callbacks: vec!["callback".into(), "cb".into(), "done".into(), "next".into()] } + } +} + +impl std::ops::Deref for NoCallbackInPromise { + type Target = NoCallbackInPromiseConfig; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +declare_oxc_lint!( + /// ### What it does + /// + /// Disallows calling a callback function (`cb()`) inside a `Promise.prototype.then()` + /// or `Promise.prototype.catch()`. + /// + /// ### Why is this bad? + /// + /// Directly invoking a callback inside a `then()` or `catch()` method can lead to + /// unexpected behavior, such as the callback being called multiple times. Additionally, + /// mixing the callback and promise paradigms in this way can make the code confusing + /// and harder to maintain. + /// + /// ### Examples + /// + /// Examples of **incorrect** code for this rule: + /// ```js + /// function callback(err, data) { + /// console.log('Callback got called with:', err, data) + /// throw new Error('My error') + /// } + /// + /// Promise.resolve() + /// .then(() => callback(null, 'data')) + /// .catch((err) => callback(err.message, null)) + /// ``` + /// + /// Examples of **correct** code for this rule: + /// ```js + /// Promise.resolve() + /// .then((data) => { console.log(data) }) + /// .catch((err) => { console.error(err) }) + /// ``` + NoCallbackInPromise, + correctness, +); + +impl Rule for NoCallbackInPromise { + fn from_configuration(value: serde_json::Value) -> Self { + let mut default_config = NoCallbackInPromiseConfig::default(); + + let exceptions: Vec = value + .get(0) + .and_then(|v| v.get("exceptions")) + .and_then(serde_json::Value::as_array) + .map(|v| { + v.iter().filter_map(serde_json::Value::as_str).map(ToString::to_string).collect() + }) + .unwrap_or_default(); + + default_config.callbacks.retain(|item| !exceptions.contains(&item.to_string())); + + Self(Box::new(default_config)) + } + + fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { + let Some(call_expr) = node.kind().as_call_expression() else { + return; + }; + + let is_not_callback = call_expr + .callee + .get_identifier_reference() + .map_or(true, |id| self.callbacks.binary_search(&id.name.as_str().into()).is_err()); + + if is_not_callback { + if Self::has_promise_callback(call_expr) { + let Some(id) = call_expr.arguments.first().and_then(|arg| { + arg.as_expression().and_then(Expression::get_identifier_reference) + }) else { + return; + }; + + let name = id.name.as_str(); + if self.callbacks.binary_search(&name.into()).is_ok() { + ctx.diagnostic(no_callback_in_promise_diagnostic(id.span)); + } + } + } else if ctx + .nodes() + .iter_parents(node.id()) + .skip(1) + .any(|node| Self::is_inside_promise(node, ctx)) + { + ctx.diagnostic(no_callback_in_promise_diagnostic(node.span())); + } + } +} + +impl NoCallbackInPromise { + fn is_inside_promise(node: &AstNode, ctx: &LintContext) -> bool { + if !matches!(node.kind(), AstKind::Function(_) | AstKind::ArrowFunctionExpression(_)) + || !matches!(ctx.nodes().parent_kind(node.id()), Some(AstKind::Argument(_))) + { + return false; + } + + ctx.nodes().iter_parents(node.id()).nth(2).is_some_and(|node| { + node.kind().as_call_expression().is_some_and(Self::has_promise_callback) + }) + } + + fn has_promise_callback(call_expr: &CallExpression) -> bool { + matches!( + call_expr + .callee + .as_member_expression() + .and_then(MemberExpression::static_property_name), + Some("then" | "catch") + ) + } +} + +#[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..6306f9b89fe0c --- /dev/null +++ b/crates/oxc_linter/src/snapshots/no_callback_in_promise.snap @@ -0,0 +1,86 @@ +--- +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) + · ── + ╰──── + help: Use `then` and `catch` directly + + ⚠ 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()) + · ──── + ╰──── + help: Use `then` and `catch` directly + + ⚠ 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) }) + · ─────── + ╰──── + help: Use `then` and `catch` directly + + ⚠ 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) }) + · ──────── + ╰──── + help: Use `then` and `catch` directly + + ⚠ 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) }) + · ─────── + ╰──── + help: Use `then` and `catch` directly + + ⚠ 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) }) + · ─────── + ╰──── + help: Use `then` and `catch` directly + + ⚠ 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) + · ──────── + ╰──── + help: Use `then` and `catch` directly + + ⚠ 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()) + · ────────── + ╰──── + help: Use `then` and `catch` directly + + ⚠ 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) }) + · ───────────── + ╰──── + help: Use `then` and `catch` directly + + ⚠ 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) }) + · ────────────── + ╰──── + help: Use `then` and `catch` directly + + ⚠ 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) }) + · ───────────── + ╰──── + help: Use `then` and `catch` directly + + ⚠ 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) }) + · ───────────── + ╰──── + help: Use `then` and `catch` directly