diff --git a/crates/oxc_linter/src/generated/rule_runner_impls.rs b/crates/oxc_linter/src/generated/rule_runner_impls.rs index 0a77e098a3797..663a1ced58284 100644 --- a/crates/oxc_linter/src/generated/rule_runner_impls.rs +++ b/crates/oxc_linter/src/generated/rule_runner_impls.rs @@ -612,6 +612,12 @@ impl RuleRunner for crate::rules::eslint::no_plusplus::NoPlusplus { const RUN_FUNCTIONS: RuleRunFunctionsImplemented = RuleRunFunctionsImplemented::Run; } +impl RuleRunner for crate::rules::eslint::no_promise_executor_return::NoPromiseExecutorReturn { + const NODE_TYPES: Option<&AstTypesBitset> = + Some(&AstTypesBitset::from_types(&[AstType::NewExpression])); + const RUN_FUNCTIONS: RuleRunFunctionsImplemented = RuleRunFunctionsImplemented::Run; +} + impl RuleRunner for crate::rules::eslint::no_proto::NoProto { const NODE_TYPES: Option<&AstTypesBitset> = Some(&AstTypesBitset::from_types(&[ AstType::ComputedMemberExpression, diff --git a/crates/oxc_linter/src/rules.rs b/crates/oxc_linter/src/rules.rs index 5367f55d8a540..1a6496432e778 100644 --- a/crates/oxc_linter/src/rules.rs +++ b/crates/oxc_linter/src/rules.rs @@ -132,6 +132,7 @@ pub(crate) mod eslint { pub mod no_object_constructor; pub mod no_param_reassign; pub mod no_plusplus; + pub mod no_promise_executor_return; pub mod no_proto; pub mod no_prototype_builtins; pub mod no_redeclare; @@ -776,6 +777,7 @@ oxc_macros::declare_all_lint_rules! { eslint::no_nonoctal_decimal_escape, eslint::no_obj_calls, eslint::no_plusplus, + eslint::no_promise_executor_return, eslint::no_proto, eslint::no_prototype_builtins, eslint::no_redeclare, diff --git a/crates/oxc_linter/src/rules/eslint/no_promise_executor_return.rs b/crates/oxc_linter/src/rules/eslint/no_promise_executor_return.rs new file mode 100644 index 0000000000000..dfece55be99b6 --- /dev/null +++ b/crates/oxc_linter/src/rules/eslint/no_promise_executor_return.rs @@ -0,0 +1,289 @@ +use oxc_ast::{ + AstKind, + ast::{Argument, Expression, FunctionBody}, +}; +use oxc_ast_visit::Visit; +use oxc_diagnostics::OxcDiagnostic; +use oxc_macros::declare_oxc_lint; +use oxc_span::Span; +use oxc_syntax::operator::UnaryOperator; +use schemars::JsonSchema; +use serde::Deserialize; + +use crate::{ + AstNode, + context::LintContext, + rule::{DefaultRuleConfig, Rule}, +}; + +fn no_promise_executor_return_diagnostic(span: Span) -> OxcDiagnostic { + OxcDiagnostic::warn("Return statement should not be used in Promise executor.") + .with_help("Use `resolve()` or `reject()` instead of returning a value.") + .with_label(span) +} + +#[derive(Debug, Default, Clone, Deserialize)] +pub struct NoPromiseExecutorReturn(Box); + +#[derive(Debug, Default, Clone, JsonSchema, Deserialize)] +#[serde(rename_all = "camelCase", default)] +pub struct NoPromiseExecutorReturnConfig { + /// If `true`, allows returning `void` expressions (e.g., `return void resolve()`). + allow_void: bool, +} + +impl std::ops::Deref for NoPromiseExecutorReturn { + type Target = NoPromiseExecutorReturnConfig; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +declare_oxc_lint!( + /// ### What it does + /// + /// Disallow returning values from Promise executor functions. + /// + /// ### Why is this bad? + /// + /// The `new Promise` constructor accepts an executor function as an argument, + /// which has `resolve` and `reject` parameters that can be used to control the + /// state of the created Promise. + /// + /// The return value of the executor is ignored. Returning a value from an executor + /// function is a possible error because the returned value cannot be used and it + /// doesn't affect the promise in any way. + /// + /// ### Examples + /// + /// Examples of **incorrect** code for this rule: + /// ```javascript + /// new Promise((resolve, reject) => { + /// if (someCondition) { + /// return defaultResult; + /// } + /// getSomething((err, result) => { + /// if (err) { + /// reject(err); + /// } else { + /// resolve(result); + /// } + /// }); + /// }); + /// + /// new Promise((resolve, reject) => getSomething((err, data) => { + /// if (err) { + /// reject(err); + /// } else { + /// resolve(data); + /// } + /// })); + /// + /// new Promise(() => { + /// return 1; + /// }); + /// ``` + /// + /// Examples of **correct** code for this rule: + /// ```javascript + /// new Promise((resolve, reject) => { + /// if (someCondition) { + /// resolve(defaultResult); + /// return; + /// } + /// getSomething((err, result) => { + /// if (err) { + /// reject(err); + /// } else { + /// resolve(result); + /// } + /// }); + /// }); + /// + /// new Promise((resolve, reject) => { + /// getSomething((err, data) => { + /// if (err) { + /// reject(err); + /// } else { + /// resolve(data); + /// } + /// }); + /// }); + /// + /// new Promise(r => { r(1) }); + /// ``` + NoPromiseExecutorReturn, + eslint, + pedantic, + config = NoPromiseExecutorReturnConfig, +); + +impl Rule for NoPromiseExecutorReturn { + fn from_configuration(value: serde_json::Value) -> Self { + serde_json::from_value::>(value) + .unwrap_or_default() + .into_inner() + } + + 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") { + return; + } + + let Some(first_arg) = new_expr.arguments.first().and_then(Argument::as_expression) else { + return; + }; + + let inner_expr = first_arg.get_inner_expression(); + + match inner_expr { + Expression::ArrowFunctionExpression(arrow) => { + // Check for implicit return (expression body without braces) + if let Some(expr) = arrow.get_expression() { + // Arrow function with expression body: `new Promise(r => r(1))` + // This is an implicit return, report it unless allowVoid and it's a void expression + if self.allow_void + && let Expression::UnaryExpression(unary) = expr.get_inner_expression() + && unary.operator == UnaryOperator::Void + { + return; + } + ctx.diagnostic(no_promise_executor_return_diagnostic(arrow.body.span)); + } else { + // Arrow function with block body: check for return statements + self.check_function_body(&arrow.body, ctx); + } + } + Expression::FunctionExpression(func) => { + if let Some(body) = &func.body { + self.check_function_body(body, ctx); + } + } + _ => {} + } + } +} + +impl NoPromiseExecutorReturn { + fn check_function_body(&self, body: &FunctionBody, ctx: &LintContext) { + let mut finder = ReturnStatementFinder::new(self.allow_void); + finder.visit_function_body(body); + + for span in finder.return_spans { + ctx.diagnostic(no_promise_executor_return_diagnostic(span)); + } + } +} + +struct ReturnStatementFinder { + return_spans: Vec, + allow_void: bool, +} + +impl ReturnStatementFinder { + fn new(allow_void: bool) -> Self { + Self { return_spans: Vec::new(), allow_void } + } +} + +impl Visit<'_> for ReturnStatementFinder { + fn visit_return_statement(&mut self, it: &oxc_ast::ast::ReturnStatement<'_>) { + // Empty return is allowed + let Some(argument) = &it.argument else { + return; + }; + + // Check for void expression if allowVoid is true + if self.allow_void + && let Expression::UnaryExpression(unary) = argument.get_inner_expression() + && unary.operator == UnaryOperator::Void + { + return; + } + + self.return_spans.push(it.span); + } + + fn visit_function( + &mut self, + _it: &oxc_ast::ast::Function<'_>, + _flags: oxc_semantic::ScopeFlags, + ) { + // Skip visiting nested functions - they have their own scope + } + + fn visit_arrow_function_expression(&mut self, _it: &oxc_ast::ast::ArrowFunctionExpression<'_>) { + // Skip visiting nested arrow functions - they have their own scope + } +} + +#[test] +fn test() { + use crate::tester::Tester; + + let pass = vec![ + // Empty return is allowed + ("new Promise((resolve, reject) => { return; })", None), + ("new Promise(function (resolve, reject) { return; })", None), + // No return at all + ("new Promise((resolve, reject) => { resolve(1); })", None), + ("new Promise(function (resolve, reject) { resolve(1); })", None), + // Arrow with block body, no return + ("new Promise(r => { r(1) })", None), + // Not a Promise + ("new Foo((resolve, reject) => { return 1; })", None), + ("new Foo(r => r(1))", None), + // Nested function with return is ok + ("new Promise((resolve) => { function inner() { return 1; } inner(); resolve(); })", None), + ("new Promise((resolve) => { const inner = () => 1; resolve(inner()); })", None), + // void is allowed with allowVoid option (explicit return) + ( + "new Promise((resolve) => { return void resolve(1); })", + Some(serde_json::json!([{ "allowVoid": true }])), + ), + ( + "new Promise((resolve) => { return void 0; })", + Some(serde_json::json!([{ "allowVoid": true }])), + ), + // void is allowed with allowVoid option (implicit return in arrow expression body) + ("new Promise(r => void r(1))", Some(serde_json::json!([{ "allowVoid": true }]))), + ]; + + let fail = vec![ + // Arrow with expression body (implicit return) + ("new Promise(r => r(1))", None), + ("new Promise((resolve, reject) => resolve(1))", None), + // Explicit return with value + ("new Promise((resolve, reject) => { return 1; })", None), + ("new Promise(function (resolve, reject) { return 1; })", None), + // Return in control flow + ("new Promise((resolve, reject) => { if (true) { return 1; } })", None), + ("new Promise((resolve, reject) => { if (true) return 1; })", None), + // Return variable + ("new Promise((resolve, reject) => { return resolve; })", None), + // Return function call result (not resolve/reject call itself) + ("new Promise((resolve, reject) => { return foo(); })", None), + // Return resolve/reject call result - these should NOT be special-cased as valid + ("new Promise((resolve, reject) => { return resolve(1); })", None), + ("new Promise((resolve, reject) => { return reject(new Error('fail')); })", None), + // Nested blocks + ("new Promise((resolve, reject) => { { { return 1; } } })", None), + // In try-catch + ("new Promise((resolve, reject) => { try { return 1; } catch(e) {} })", None), + // void is not allowed by default + ("new Promise((resolve) => { return void resolve(1); })", None), + // In switch + ("new Promise((resolve) => { switch(x) { case 1: return 1; } })", None), + // In loops + ("new Promise((resolve) => { while(true) { return 1; } })", None), + ("new Promise((resolve) => { for(;;) { return 1; } })", None), + ]; + + Tester::new(NoPromiseExecutorReturn::NAME, NoPromiseExecutorReturn::PLUGIN, pass, fail) + .test_and_snapshot(); +} diff --git a/crates/oxc_linter/src/snapshots/eslint_no_promise_executor_return.snap b/crates/oxc_linter/src/snapshots/eslint_no_promise_executor_return.snap new file mode 100644 index 0000000000000..dcab9a58b77cd --- /dev/null +++ b/crates/oxc_linter/src/snapshots/eslint_no_promise_executor_return.snap @@ -0,0 +1,114 @@ +--- +source: crates/oxc_linter/src/tester.rs +--- + ⚠ eslint(no-promise-executor-return): Return statement should not be used in Promise executor. + ╭─[no_promise_executor_return.tsx:1:18] + 1 │ new Promise(r => r(1)) + · ──── + ╰──── + help: Use `resolve()` or `reject()` instead of returning a value. + + ⚠ eslint(no-promise-executor-return): Return statement should not be used in Promise executor. + ╭─[no_promise_executor_return.tsx:1:34] + 1 │ new Promise((resolve, reject) => resolve(1)) + · ────────── + ╰──── + help: Use `resolve()` or `reject()` instead of returning a value. + + ⚠ eslint(no-promise-executor-return): Return statement should not be used in Promise executor. + ╭─[no_promise_executor_return.tsx:1:36] + 1 │ new Promise((resolve, reject) => { return 1; }) + · ───────── + ╰──── + help: Use `resolve()` or `reject()` instead of returning a value. + + ⚠ eslint(no-promise-executor-return): Return statement should not be used in Promise executor. + ╭─[no_promise_executor_return.tsx:1:42] + 1 │ new Promise(function (resolve, reject) { return 1; }) + · ───────── + ╰──── + help: Use `resolve()` or `reject()` instead of returning a value. + + ⚠ eslint(no-promise-executor-return): Return statement should not be used in Promise executor. + ╭─[no_promise_executor_return.tsx:1:48] + 1 │ new Promise((resolve, reject) => { if (true) { return 1; } }) + · ───────── + ╰──── + help: Use `resolve()` or `reject()` instead of returning a value. + + ⚠ eslint(no-promise-executor-return): Return statement should not be used in Promise executor. + ╭─[no_promise_executor_return.tsx:1:46] + 1 │ new Promise((resolve, reject) => { if (true) return 1; }) + · ───────── + ╰──── + help: Use `resolve()` or `reject()` instead of returning a value. + + ⚠ eslint(no-promise-executor-return): Return statement should not be used in Promise executor. + ╭─[no_promise_executor_return.tsx:1:36] + 1 │ new Promise((resolve, reject) => { return resolve; }) + · ─────────────── + ╰──── + help: Use `resolve()` or `reject()` instead of returning a value. + + ⚠ eslint(no-promise-executor-return): Return statement should not be used in Promise executor. + ╭─[no_promise_executor_return.tsx:1:36] + 1 │ new Promise((resolve, reject) => { return foo(); }) + · ───────────── + ╰──── + help: Use `resolve()` or `reject()` instead of returning a value. + + ⚠ eslint(no-promise-executor-return): Return statement should not be used in Promise executor. + ╭─[no_promise_executor_return.tsx:1:36] + 1 │ new Promise((resolve, reject) => { return resolve(1); }) + · ────────────────── + ╰──── + help: Use `resolve()` or `reject()` instead of returning a value. + + ⚠ eslint(no-promise-executor-return): Return statement should not be used in Promise executor. + ╭─[no_promise_executor_return.tsx:1:36] + 1 │ new Promise((resolve, reject) => { return reject(new Error('fail')); }) + · ───────────────────────────────── + ╰──── + help: Use `resolve()` or `reject()` instead of returning a value. + + ⚠ eslint(no-promise-executor-return): Return statement should not be used in Promise executor. + ╭─[no_promise_executor_return.tsx:1:40] + 1 │ new Promise((resolve, reject) => { { { return 1; } } }) + · ───────── + ╰──── + help: Use `resolve()` or `reject()` instead of returning a value. + + ⚠ eslint(no-promise-executor-return): Return statement should not be used in Promise executor. + ╭─[no_promise_executor_return.tsx:1:42] + 1 │ new Promise((resolve, reject) => { try { return 1; } catch(e) {} }) + · ───────── + ╰──── + help: Use `resolve()` or `reject()` instead of returning a value. + + ⚠ eslint(no-promise-executor-return): Return statement should not be used in Promise executor. + ╭─[no_promise_executor_return.tsx:1:28] + 1 │ new Promise((resolve) => { return void resolve(1); }) + · ─────────────────────── + ╰──── + help: Use `resolve()` or `reject()` instead of returning a value. + + ⚠ eslint(no-promise-executor-return): Return statement should not be used in Promise executor. + ╭─[no_promise_executor_return.tsx:1:48] + 1 │ new Promise((resolve) => { switch(x) { case 1: return 1; } }) + · ───────── + ╰──── + help: Use `resolve()` or `reject()` instead of returning a value. + + ⚠ eslint(no-promise-executor-return): Return statement should not be used in Promise executor. + ╭─[no_promise_executor_return.tsx:1:42] + 1 │ new Promise((resolve) => { while(true) { return 1; } }) + · ───────── + ╰──── + help: Use `resolve()` or `reject()` instead of returning a value. + + ⚠ eslint(no-promise-executor-return): Return statement should not be used in Promise executor. + ╭─[no_promise_executor_return.tsx:1:38] + 1 │ new Promise((resolve) => { for(;;) { return 1; } }) + · ───────── + ╰──── + help: Use `resolve()` or `reject()` instead of returning a value.