From 9a6f450d37993b37bdb0c07b966ee238df999207 Mon Sep 17 00:00:00 2001 From: therewillbecode Date: Mon, 3 Mar 2025 10:00:50 +0000 Subject: [PATCH 01/26] Init rule for promise no-return-wrap --- crates/oxc_linter/src/rules.rs | 2 + .../src/rules/promise/no_return_wrap.rs | 203 ++++++++++++++++++ 2 files changed, 205 insertions(+) create mode 100644 crates/oxc_linter/src/rules/promise/no_return_wrap.rs diff --git a/crates/oxc_linter/src/rules.rs b/crates/oxc_linter/src/rules.rs index f09dd7e8f3635..205a38696d942 100644 --- a/crates/oxc_linter/src/rules.rs +++ b/crates/oxc_linter/src/rules.rs @@ -526,6 +526,7 @@ mod promise { 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; pub mod prefer_await_to_callbacks; pub mod prefer_await_to_then; @@ -857,6 +858,7 @@ oxc_macros::declare_all_lint_rules! { oxc::uninvoked_array_callback, promise::avoid_new, promise::catch_or_return, + promise::no_return_wrap, promise::no_nesting, promise::no_promise_in_callback, promise::no_callback_in_promise, 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..a7683983e2d5d --- /dev/null +++ b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs @@ -0,0 +1,203 @@ +use oxc_diagnostics::OxcDiagnostic; +use oxc_macros::declare_oxc_lint; +use oxc_span::Span; + +use crate::{ + AstNode, + context::LintContext, + fixer::{RuleFix, RuleFixer}, + rule::Rule, +}; + +fn no_return_wrap_diagnostic(span: Span) -> OxcDiagnostic { + // See for details + OxcDiagnostic::warn("Should be an imperative statement about what is wrong") + .with_help("Should be a command-like statement that tells the user how to fix the issue") + .with_label(span) +} + +#[derive(Debug, Default, Clone)] +pub struct NoReturnWrap; + +declare_oxc_lint!( + /// ### What it does + /// + /// + /// ### Why is this bad? + /// + /// + /// ### Examples + /// + /// Examples of **incorrect** code for this rule: + /// ```js + /// FIXME: Tests will fail if examples are missing or syntactically incorrect. + /// ``` + /// + /// Examples of **correct** code for this rule: + /// ```js + /// FIXME: Tests will fail if examples are missing or syntactically incorrect. + /// ``` + NoReturnWrap, + promise, + nursery, // TODO: change category to `correctness`, `suspicious`, `pedantic`, `perf`, `restriction`, or `style` + // See for details + + pending // TODO: describe fix capabilities. Remove if no fix can be done, + // keep at 'pending' if you think one could be added but don't know how. + // Options are 'fix', 'fix_dangerous', 'suggestion', and 'conditional_fix_suggestion' +); + +impl Rule for NoReturnWrap { + fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) {} +} + +#[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, + ), + ("doThing().then(() => Promise.resolve(4))", None), + ("doThing().then(() => Promise.reject(4))", None), + ]; + + Tester::new(NoReturnWrap::NAME, NoReturnWrap::PLUGIN, pass, fail).test_and_snapshot(); +} From 86b3b2d34206cda0fad3d95ab8183e82f4c4af5c Mon Sep 17 00:00:00 2001 From: therewillbecode Date: Mon, 3 Mar 2025 12:40:36 +0000 Subject: [PATCH 02/26] Add initial docs --- .../src/rules/promise/no_return_wrap.rs | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs index a7683983e2d5d..c633042a3437c 100644 --- a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs +++ b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs @@ -22,9 +22,15 @@ pub struct NoReturnWrap; declare_oxc_lint!( /// ### What it does /// + /// Prevents unnecessary wrapping of results in `Promise.resolve` or `Promise.reject` as the + /// Promise will do that for us. /// /// ### Why is this bad? /// + /// `Promise.resolve` and Promise.reject` are used to convert raw values to promises. This + /// conversion is unnecessary for return values inside promises as such return values are + /// already converted into promises. This is why some take the opinion that returning values + /// such as `Promise.resolve(1)` or `Promise.reject(err)` is syntactic noise. /// /// ### Examples /// @@ -37,14 +43,13 @@ declare_oxc_lint!( /// ```js /// FIXME: Tests will fail if examples are missing or syntactically incorrect. /// ``` + /// + /// ### Options + /// NoReturnWrap, promise, - nursery, // TODO: change category to `correctness`, `suspicious`, `pedantic`, `perf`, `restriction`, or `style` - // See for details - - pending // TODO: describe fix capabilities. Remove if no fix can be done, - // keep at 'pending' if you think one could be added but don't know how. - // Options are 'fix', 'fix_dangerous', 'suggestion', and 'conditional_fix_suggestion' + style, + pending ); impl Rule for NoReturnWrap { From f200c9f8358409ceb42992fec2a0007be3619b95 Mon Sep 17 00:00:00 2001 From: therewillbecode Date: Mon, 3 Mar 2025 13:11:27 +0000 Subject: [PATCH 03/26] Add inside then or catch check --- .../src/rules/promise/no_return_wrap.rs | 51 ++++++++++++++++--- 1 file changed, 45 insertions(+), 6 deletions(-) diff --git a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs index c633042a3437c..e03a5d4e5ab6b 100644 --- a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs +++ b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs @@ -1,13 +1,15 @@ -use oxc_diagnostics::OxcDiagnostic; -use oxc_macros::declare_oxc_lint; -use oxc_span::Span; - use crate::{ AstNode, context::LintContext, fixer::{RuleFix, RuleFixer}, rule::Rule, + utils::is_promise, }; +use oxc_ast::AstKind; +use oxc_ast::ast::{CallExpression, MemberExpression}; +use oxc_diagnostics::OxcDiagnostic; +use oxc_macros::declare_oxc_lint; +use oxc_span::Span; fn no_return_wrap_diagnostic(span: Span) -> OxcDiagnostic { // See for details @@ -17,7 +19,9 @@ fn no_return_wrap_diagnostic(span: Span) -> OxcDiagnostic { } #[derive(Debug, Default, Clone)] -pub struct NoReturnWrap; +pub struct NoReturnWrap { + allow_reject: bool, +} declare_oxc_lint!( /// ### What it does @@ -53,7 +57,42 @@ declare_oxc_lint!( ); impl Rule for NoReturnWrap { - fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) {} + fn from_configuration(value: serde_json::Value) -> Self { + let allow_reject = value + .get(0) + .and_then(|v| v.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>) { + let AstKind::CallExpression(call_expr) = node.kind() else { + return; + }; + let d = ctx.source_text(); + let in_prom_cb = inside_then_or_catch(node, ctx); + println!("Is indise then or catch: {0:?}, {1:?}", in_prom_cb, d); + } +} + +/// Return true if this node is inside a `then` or `catch` promise callback. Will return `true` +/// for `node` in both `prom.then(null, () => node)` and `prom.then(() => node)`. +fn inside_then_or_catch<'a, 'b>(node: &'a AstNode<'b>, ctx: &'a LintContext<'b>) -> bool { + ctx.nodes().ancestors(node.id()).any(|node| { + node.kind() + .as_call_expression() + .is_some_and(|call_expr| { + matches!( + call_expr + .callee + .as_member_expression() + .and_then(MemberExpression::static_property_name), + Some("then" | "catch") + ) + }) + }) } #[test] From 0b0c74773f7eb83bede3228da39b10c7c2a4b3ef Mon Sep 17 00:00:00 2001 From: therewillbecode Date: Mon, 3 Mar 2025 13:40:38 +0000 Subject: [PATCH 04/26] More docs --- .../src/rules/promise/no_return_wrap.rs | 24 ++++++++++++++----- 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs index e03a5d4e5ab6b..3dac3b0865a0e 100644 --- a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs +++ b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs @@ -26,15 +26,27 @@ pub struct NoReturnWrap { declare_oxc_lint!( /// ### What it does /// - /// Prevents unnecessary wrapping of results in `Promise.resolve` or `Promise.reject` as the - /// Promise will do that for us. + /// Prevents unnecessary wrapping of return values in promises with either `Promise.resolve` + /// or `Promise.reject`. + /// + /// This rule enforces the following stances: + /// + /// 1. When a promise is to be resolved, instead of returning `Promise.resolve(value)` it is + /// better to return the raw value with `return value` instead. + /// + /// 2. When a promise is to be rejected, instead of returning `Promise.reject(error)`, instead + /// the raw error value should be thrown as in `throw error`. + /// + /// There is an option to turn off the enforcing of 2, see the options section below. /// /// ### Why is this bad? /// - /// `Promise.resolve` and Promise.reject` are used to convert raw values to promises. This - /// conversion is unnecessary for return values inside promises as such return values are - /// already converted into promises. This is why some take the opinion that returning values - /// such as `Promise.resolve(1)` or `Promise.reject(err)` is syntactic noise. + /// It is unnecessary to use `Promise.resolve` and Promise.reject` for converting raw values + /// to promises in the return statements of `then` and `catch` callbacks. Using these + /// operations to convert raw values to promises is unnecessary as simply returning the raw + /// value for the success case and throwing the raw error value in the failure case have the + /// same effect. This is why some take the opinion that returning values such as + /// `Promise.resolve(1)` or `Promise.reject(err)` is syntactic noise. /// /// ### Examples /// From 9d19d55a81fbdbf586e4589b244001dd79efae55 Mon Sep 17 00:00:00 2001 From: therewillbecode Date: Mon, 3 Mar 2025 19:09:15 +0000 Subject: [PATCH 05/26] Handle call expression inside then cb --- .../src/rules/promise/no_return_wrap.rs | 101 ++++++++++++++---- 1 file changed, 83 insertions(+), 18 deletions(-) diff --git a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs index 3dac3b0865a0e..71101543a7b51 100644 --- a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs +++ b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs @@ -1,3 +1,12 @@ +use oxc_allocator::Box as OBox; +use oxc_ast::{ + AstKind, + ast::{CallExpression, Expression, FunctionBody, MemberExpression, ReturnStatement, Statement}, +}; +use oxc_diagnostics::OxcDiagnostic; +use oxc_macros::declare_oxc_lint; +use oxc_span::Span; + use crate::{ AstNode, context::LintContext, @@ -5,11 +14,6 @@ use crate::{ rule::Rule, utils::is_promise, }; -use oxc_ast::AstKind; -use oxc_ast::ast::{CallExpression, MemberExpression}; -use oxc_diagnostics::OxcDiagnostic; -use oxc_macros::declare_oxc_lint; -use oxc_span::Span; fn no_return_wrap_diagnostic(span: Span) -> OxcDiagnostic { // See for details @@ -64,7 +68,7 @@ declare_oxc_lint!( /// NoReturnWrap, promise, - style, + nursery, pending ); @@ -83,27 +87,86 @@ impl Rule for NoReturnWrap { let AstKind::CallExpression(call_expr) = node.kind() else { return; }; + let d = ctx.source_text(); let in_prom_cb = inside_then_or_catch(node, ctx); - println!("Is indise then or catch: {0:?}, {1:?}", in_prom_cb, d); + + // let args = &call_expr.arguments; + // let resolve_cb = todo!(); + // let reject_cb = todo!(); + + // let ret = get_return(state); + // println!("return then or catch: {0:?}, {1:?}", args , d); + + for argument in &call_expr.arguments { + let Some(arg_expr) = argument.as_expression().map(|a| a.without_parentheses()) else { + println!("noe"); + + 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); + } + Expression::CallExpression(call) => { + let Expression::StaticMemberExpression(s) = call.callee.get_inner_expression() + else { + continue; + }; + match &s.object.get_inner_expression() { + 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, + } + continue; + } + _ => 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_wrap_diagnostic(stmt.span)); +} + /// Return true if this node is inside a `then` or `catch` promise callback. Will return `true` /// for `node` in both `prom.then(null, () => node)` and `prom.then(() => node)`. fn inside_then_or_catch<'a, 'b>(node: &'a AstNode<'b>, ctx: &'a LintContext<'b>) -> bool { ctx.nodes().ancestors(node.id()).any(|node| { - node.kind() - .as_call_expression() - .is_some_and(|call_expr| { - matches!( - call_expr - .callee - .as_member_expression() - .and_then(MemberExpression::static_property_name), - Some("then" | "catch") - ) - }) + node.kind().as_call_expression().is_some_and(|call_expr| { + matches!( + call_expr + .callee + .as_member_expression() + .and_then(MemberExpression::static_property_name), + Some("then" | "catch") + ) + }) }) } @@ -152,6 +215,7 @@ fn test() { ]; 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), @@ -253,6 +317,7 @@ fn test() { ), ("doThing().then(() => Promise.resolve(4))", None), ("doThing().then(() => Promise.reject(4))", None), + */ ]; Tester::new(NoReturnWrap::NAME, NoReturnWrap::PLUGIN, pass, fail).test_and_snapshot(); From 07033069b2fe2bb1441e480f215a46895b5dd94d Mon Sep 17 00:00:00 2001 From: therewillbecode Date: Mon, 3 Mar 2025 19:10:52 +0000 Subject: [PATCH 06/26] Rename func --- crates/oxc_linter/src/rules/promise/no_return_wrap.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs index 71101543a7b51..1eff78a884b6d 100644 --- a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs +++ b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs @@ -107,13 +107,13 @@ impl Rule for NoReturnWrap { match arg_expr { Expression::ArrowFunctionExpression(arrow_expr) => { - find_return_statement(&arrow_expr.body, ctx); + find_first_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); + find_first_return_statement(func_body, ctx); } Expression::CallExpression(call) => { let Expression::StaticMemberExpression(s) = call.callee.get_inner_expression() @@ -122,13 +122,13 @@ impl Rule for NoReturnWrap { }; match &s.object.get_inner_expression() { Expression::ArrowFunctionExpression(arrow_expr) => { - find_return_statement(&arrow_expr.body, ctx); + find_first_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); + find_first_return_statement(func_body, ctx); } _ => continue, } @@ -140,7 +140,7 @@ impl Rule for NoReturnWrap { } } -fn find_return_statement<'a>(func_body: &OBox<'_, FunctionBody<'a>>, ctx: &LintContext<'a>) { +fn find_first_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 { From 2670be5125414a5c2b2d6be23aeda8d16f89a7bf Mon Sep 17 00:00:00 2001 From: therewillbecode Date: Mon, 3 Mar 2025 21:03:28 +0000 Subject: [PATCH 07/26] Examine return arg --- .../src/rules/promise/no_return_wrap.rs | 24 ++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs index 1eff78a884b6d..fbe0934b77197 100644 --- a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs +++ b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs @@ -151,7 +151,27 @@ fn find_first_return_statement<'a>(func_body: &OBox<'_, FunctionBody<'a>>, ctx: return; }; - ctx.diagnostic(no_return_wrap_diagnostic(stmt.span)); + let Some(Expression::CallExpression(call_expr)) = &stmt.argument else { + return; + }; + + let Expression::StaticMemberExpression(stat_expr) = &call_expr.callee else { + return; + }; + + let Some(obj_call_ident) = stat_expr.object.get_identifier_reference() else { + return; + }; + + if !ctx.semantic().is_reference_to_global_variable(obj_call_ident) { + return; + } + + if obj_call_ident.name == "Promise" { + ctx.diagnostic(no_return_wrap_diagnostic(call_expr.span)); + } + + ctx.diagnostic(no_return_wrap_diagnostic(call_expr.span)); } /// Return true if this node is inside a `then` or `catch` promise callback. Will return `true` @@ -215,7 +235,6 @@ fn test() { ]; 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), @@ -317,7 +336,6 @@ fn test() { ), ("doThing().then(() => Promise.resolve(4))", None), ("doThing().then(() => Promise.reject(4))", None), - */ ]; Tester::new(NoReturnWrap::NAME, NoReturnWrap::PLUGIN, pass, fail).test_and_snapshot(); From c907cc8db7fe488f15ea75d8e3adfdaed9c63b03 Mon Sep 17 00:00:00 2001 From: therewillbecode Date: Mon, 3 Mar 2025 21:06:12 +0000 Subject: [PATCH 08/26] Format test strings --- .../src/rules/promise/no_return_wrap.rs | 102 ++++++++---------- 1 file changed, 46 insertions(+), 56 deletions(-) diff --git a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs index fbe0934b77197..52303a96434e3 100644 --- a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs +++ b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs @@ -255,83 +255,73 @@ fn test() { None, ), ( - " - fn(function() { - doThing().then(function() { - return Promise.resolve(4) - }) - return - })", + "fn(function() { + doThing().then(function() { + return Promise.resolve(4) + }) + return + })", None, ), ( - " - fn(function() { - doThing().then(function nm() { - return Promise.resolve(4) - }) - return - })", + "fn(function() { + doThing().then(function nm() { + return Promise.resolve(4) + }) + return + })", None, ), ( - " - fn(function() { - fn2(function() { - doThing().then(function() { - return Promise.resolve(4) - }) - }) - })", + "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) - }) - }) - })", + "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); - }); - }, - } - ", + "const o = { + fn: function() { + return doThing().then(function() { + return Promise.resolve(5); + }); + }, + }", None, ), ( - " - fn( - doThing().then(function() { - return Promise.resolve(5); - }) - ); - ", + "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) - }) - } - ", + "function a () { + return p.then(function(val) { + return Promise.resolve(val * 4) + }) + }", None, ), ("doThing().then(() => Promise.resolve(4))", None), From 0f5d3b89cb9f959a085d7ac0a1232eba327928a5 Mon Sep 17 00:00:00 2001 From: therewillbecode Date: Mon, 3 Mar 2025 21:21:50 +0000 Subject: [PATCH 09/26] wip --- .../src/rules/promise/no_return_wrap.rs | 64 +++++++++++-------- 1 file changed, 37 insertions(+), 27 deletions(-) diff --git a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs index 52303a96434e3..f3783d739061f 100644 --- a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs +++ b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs @@ -5,7 +5,7 @@ use oxc_ast::{ }; use oxc_diagnostics::OxcDiagnostic; use oxc_macros::declare_oxc_lint; -use oxc_span::Span; +use oxc_span::{Span, format_compact_str}; use crate::{ AstNode, @@ -89,7 +89,10 @@ impl Rule for NoReturnWrap { }; let d = ctx.source_text(); - let in_prom_cb = inside_then_or_catch(node, ctx); + //let in_prom_cb = + if !inside_then_or_catch(node, ctx) { + return; + } // let args = &call_expr.arguments; // let resolve_cb = todo!(); @@ -107,40 +110,45 @@ impl Rule for NoReturnWrap { match arg_expr { Expression::ArrowFunctionExpression(arrow_expr) => { - find_first_return_statement(&arrow_expr.body, ctx); + check_first_return_statement(&arrow_expr.body, ctx, self.allow_reject); } Expression::FunctionExpression(func_expr) => { let Some(func_body) = &func_expr.body else { continue; }; - find_first_return_statement(func_body, ctx); - } - Expression::CallExpression(call) => { - let Expression::StaticMemberExpression(s) = call.callee.get_inner_expression() - else { - continue; - }; - match &s.object.get_inner_expression() { - Expression::ArrowFunctionExpression(arrow_expr) => { - find_first_return_statement(&arrow_expr.body, ctx); - } - Expression::FunctionExpression(func_expr) => { - let Some(func_body) = &func_expr.body else { - continue; - }; - find_first_return_statement(func_body, ctx); - } - _ => continue, - } - continue; + check_first_return_statement(func_body, ctx, self.allow_reject); } + // Expression::CallExpression(call) => { + // let Expression::StaticMemberExpression(s) = call.callee.get_inner_expression() + // else { + // continue; + // }; + // match &s.object.get_inner_expression() { + // Expression::ArrowFunctionExpression(arrow_expr) => { + // check_first_return_statement(&arrow_expr.body, ctx, self.allow_reject); + // } + // Expression::FunctionExpression(func_expr) => { + // let Some(func_body) = &func_expr.body else { + // continue; + // }; + // check_first_return_statement(func_body, ctx, self.allow_reject); + // } + // _ => continue, + // } + // continue; + // } _ => continue, } } } } -fn find_first_return_statement<'a>(func_body: &OBox<'_, FunctionBody<'a>>, ctx: &LintContext<'a>) { +/// Checks for `return Promise.resolve()` or `return Promise.reject()`. +fn check_first_return_statement<'a>( + func_body: &OBox<'_, FunctionBody<'a>>, + ctx: &LintContext<'a>, + allow_reject: bool, +) { let Some(return_stmt) = func_body.statements.iter().find(|stmt| matches!(stmt, Statement::ReturnStatement(_))) else { @@ -168,10 +176,12 @@ fn find_first_return_statement<'a>(func_body: &OBox<'_, FunctionBody<'a>>, ctx: } if obj_call_ident.name == "Promise" { - ctx.diagnostic(no_return_wrap_diagnostic(call_expr.span)); + if stat_expr.property.name == "resolve" { + ctx.diagnostic(no_return_wrap_diagnostic(call_expr.span)); + } else if stat_expr.property.name == "reject" && !allow_reject { + ctx.diagnostic(no_return_wrap_diagnostic(call_expr.span)); + } } - - ctx.diagnostic(no_return_wrap_diagnostic(call_expr.span)); } /// Return true if this node is inside a `then` or `catch` promise callback. Will return `true` From d35692a13d1ab65588b2054329e488cb43aa57e5 Mon Sep 17 00:00:00 2001 From: therewillbecode Date: Mon, 3 Mar 2025 21:49:38 +0000 Subject: [PATCH 10/26] wip --- .../src/rules/promise/no_return_wrap.rs | 52 ++++++++++++++++--- 1 file changed, 46 insertions(+), 6 deletions(-) diff --git a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs index f3783d739061f..0d414c93cad61 100644 --- a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs +++ b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs @@ -149,17 +149,57 @@ fn check_first_return_statement<'a>( ctx: &LintContext<'a>, allow_reject: bool, ) { - let Some(return_stmt) = - func_body.statements.iter().find(|stmt| matches!(stmt, Statement::ReturnStatement(_))) - else { - return; + let top_level_statements = func_body + .statements + .iter() + .find(|stmt| matches!(stmt, Statement::ReturnStatement(_) | Statement::IfStatement(_))); + + let maybe_return_stmt = match top_level_statements { + Some(Statement::ReturnStatement(r)) => Some(r), + Some(Statement::IfStatement(if_stmt)) => match &if_stmt.consequent { + Statement::BlockStatement(block_stmt) => { + // Find first return statement in if { // here } else { } + let res = block_stmt.body.iter().find_map(|stmt| { + if let Statement::ReturnStatement(r) = stmt { + return Some(r); + } else { + return None; + } + }); + + match res { + None => { + // No return found so now look if { } else { // here} + block_stmt.body.iter().find_map(|stmt| { + if let Statement::ReturnStatement(r) = stmt { + return Some(r); + } else { + return None; + } + }) + } + res => res, + } + } + Statement::ReturnStatement(r) => Some(r), + _ => None, + }, + _ => None, }; - let Statement::ReturnStatement(stmt) = return_stmt else { + let Some(return_stmt) = maybe_return_stmt else { return; }; - let Some(Expression::CallExpression(call_expr)) = &stmt.argument else { + // else { + // return; + // }; + + //let Statement::ReturnStatement(stmt) = return_stmt else { + // return; + //}; + + let Some(Expression::CallExpression(call_expr)) = &return_stmt.argument else { return; }; From 3dd0a20e92c4ce99b43bedfcff10d5762a259f52 Mon Sep 17 00:00:00 2001 From: therewillbecode Date: Mon, 3 Mar 2025 21:51:40 +0000 Subject: [PATCH 11/26] wip --- .../src/rules/promise/no_return_wrap.rs | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs index 0d414c93cad61..bc0121e4b16e4 100644 --- a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs +++ b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs @@ -143,7 +143,8 @@ impl Rule for NoReturnWrap { } } -/// Checks for `return Promise.resolve()` or `return Promise.reject()`. +/// Checks for `return Promise.resolve()` or `return Promise.reject()` at top level statements and +/// will look inside if no return is found as a top level statement in the function body. fn check_first_return_statement<'a>( func_body: &OBox<'_, FunctionBody<'a>>, ctx: &LintContext<'a>, @@ -158,7 +159,7 @@ fn check_first_return_statement<'a>( Some(Statement::ReturnStatement(r)) => Some(r), Some(Statement::IfStatement(if_stmt)) => match &if_stmt.consequent { Statement::BlockStatement(block_stmt) => { - // Find first return statement in if { // here } else { } + // Find first return statement in `if { // here } else { }` let res = block_stmt.body.iter().find_map(|stmt| { if let Statement::ReturnStatement(r) = stmt { return Some(r); @@ -169,7 +170,7 @@ fn check_first_return_statement<'a>( match res { None => { - // No return found so now look if { } else { // here} + // No return found so now look `if { } else { // here }` block_stmt.body.iter().find_map(|stmt| { if let Statement::ReturnStatement(r) = stmt { return Some(r); @@ -191,14 +192,6 @@ fn check_first_return_statement<'a>( return; }; - // else { - // return; - // }; - - //let Statement::ReturnStatement(stmt) = return_stmt else { - // return; - //}; - let Some(Expression::CallExpression(call_expr)) = &return_stmt.argument else { return; }; From eb9312f5d0b223b0f3a73798b180771c21894187 Mon Sep 17 00:00:00 2001 From: therewillbecode Date: Mon, 3 Mar 2025 21:56:31 +0000 Subject: [PATCH 12/26] Add test case --- crates/oxc_linter/src/rules/promise/no_return_wrap.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs index bc0121e4b16e4..92ee9d098fe92 100644 --- a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs +++ b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs @@ -275,6 +275,11 @@ fn test() { ("doThing().then(() => { return Promise.a() })", None), ("doThing().then(() => a())", None), ("doThing().then(() => Promise.a())", None), + ( + "class Promise { constructor(){} resolve(){} }; + doThing().then(function() { return Promise.resolve(4) })", + None, + ), ]; let fail = vec![ From 940b0c263d0d1858f7b99c9a1ac24737939ab193 Mon Sep 17 00:00:00 2001 From: therewillbecode Date: Mon, 3 Mar 2025 22:15:57 +0000 Subject: [PATCH 13/26] Handle bind(this) --- .../src/rules/promise/no_return_wrap.rs | 107 ++++++++++-------- 1 file changed, 59 insertions(+), 48 deletions(-) diff --git a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs index 92ee9d098fe92..4e6f0330dbcf8 100644 --- a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs +++ b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs @@ -5,15 +5,9 @@ use oxc_ast::{ }; use oxc_diagnostics::OxcDiagnostic; use oxc_macros::declare_oxc_lint; -use oxc_span::{Span, format_compact_str}; +use oxc_span::Span; -use crate::{ - AstNode, - context::LintContext, - fixer::{RuleFix, RuleFixer}, - rule::Rule, - utils::is_promise, -}; +use crate::{AstNode, context::LintContext, rule::Rule}; fn no_return_wrap_diagnostic(span: Span) -> OxcDiagnostic { // See for details @@ -88,23 +82,12 @@ impl Rule for NoReturnWrap { return; }; - let d = ctx.source_text(); - //let in_prom_cb = if !inside_then_or_catch(node, ctx) { return; } - // let args = &call_expr.arguments; - // let resolve_cb = todo!(); - // let reject_cb = todo!(); - - // let ret = get_return(state); - // println!("return then or catch: {0:?}, {1:?}", args , d); - for argument in &call_expr.arguments { let Some(arg_expr) = argument.as_expression().map(|a| a.without_parentheses()) else { - println!("noe"); - continue; }; @@ -118,31 +101,67 @@ impl Rule for NoReturnWrap { }; check_first_return_statement(func_body, ctx, self.allow_reject); } - // Expression::CallExpression(call) => { - // let Expression::StaticMemberExpression(s) = call.callee.get_inner_expression() - // else { - // continue; - // }; - // match &s.object.get_inner_expression() { - // Expression::ArrowFunctionExpression(arrow_expr) => { - // check_first_return_statement(&arrow_expr.body, ctx, self.allow_reject); - // } - // Expression::FunctionExpression(func_expr) => { - // let Some(func_body) = &func_expr.body else { - // continue; - // }; - // check_first_return_statement(func_body, ctx, self.allow_reject); - // } - // _ => continue, - // } - // continue; - // } + Expression::CallExpression(call) => { + let Expression::StaticMemberExpression(static_memb_expr) = + call.callee.get_inner_expression() + else { + continue; + }; + + // `.bind(this)` is true but `.bind(foo)` is false. + let is_this_arg = call.arguments.first().is_some_and(|arg| { + matches!(arg.as_expression(), Some(Expression::ThisExpression(_))) + }); + + let property_name = static_memb_expr.property.name; + + if is_this_arg && property_name == "bind" { + } else { + // We only examine the return statement inside func when the call expression on + // the func is a `this` binding for example `func.bind.this()` or + // `func.bind.this().bind.this()`. + continue; + }; + + let inner_obj = &static_memb_expr.object.get_inner_expression(); + + if let Expression::CallExpression(nested_call) = inner_obj { + // if not a chained .bind(this) then skip + let Expression::StaticMemberExpression(nested_expr) = + nested_call.callee.get_inner_expression() + else { + continue; + }; + check_callback_fn( + ctx, + self.allow_reject, + nested_expr.object.without_parentheses(), + ); + } else { + check_callback_fn(ctx, self.allow_reject, inner_obj); + }; + } _ => continue, } } } } +fn check_callback_fn<'a>(ctx: &LintContext<'a>, allow_reject: bool, expr: &Expression<'a>) { + match expr { + Expression::ArrowFunctionExpression(arrow_expr) => { + check_first_return_statement(&arrow_expr.body, ctx, allow_reject); + } + Expression::FunctionExpression(func_expr) => { + let Some(func_body) = &func_expr.body else { + return; + }; + check_first_return_statement(func_body, ctx, allow_reject); + } + _ => (), + } +} + /// Checks for `return Promise.resolve()` or `return Promise.reject()` at top level statements and /// will look inside if no return is found as a top level statement in the function body. fn check_first_return_statement<'a>( @@ -161,22 +180,14 @@ fn check_first_return_statement<'a>( Statement::BlockStatement(block_stmt) => { // Find first return statement in `if { // here } else { }` let res = block_stmt.body.iter().find_map(|stmt| { - if let Statement::ReturnStatement(r) = stmt { - return Some(r); - } else { - return None; - } + if let Statement::ReturnStatement(r) = stmt { Some(r) } else { None } }); match res { None => { // No return found so now look `if { } else { // here }` block_stmt.body.iter().find_map(|stmt| { - if let Statement::ReturnStatement(r) = stmt { - return Some(r); - } else { - return None; - } + if let Statement::ReturnStatement(r) = stmt { Some(r) } else { None } }) } res => res, From 3c1f108fbef875110936305740dbcfa6e2ae46f8 Mon Sep 17 00:00:00 2001 From: therewillbecode Date: Tue, 4 Mar 2025 10:24:18 +0000 Subject: [PATCH 14/26] Fomat test case strings --- .../src/rules/promise/no_return_wrap.rs | 22 +++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs index 4e6f0330dbcf8..267dcab3dec90 100644 --- a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs +++ b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs @@ -262,8 +262,8 @@ fn test() { ("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().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), @@ -301,7 +301,14 @@ fn test() { ("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" } })"#, + 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), @@ -310,7 +317,14 @@ fn test() { None, ), ( - "doThing().catch(function(x) {if (x) { return Promise.resolve(4) } else { return Promise.reject() } })", + "doThing().catch( + function(x) { + if (x) { + return Promise.resolve(4) + } else { + return Promise.reject() + } + })", None, ), ( From 3bb771ac0b2c7cc93e9048663729413846d7e3a3 Mon Sep 17 00:00:00 2001 From: therewillbecode Date: Tue, 4 Mar 2025 11:19:59 +0000 Subject: [PATCH 15/26] Tests passing --- .../src/rules/promise/no_return_wrap.rs | 48 +++++++++++++++++-- 1 file changed, 45 insertions(+), 3 deletions(-) diff --git a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs index 267dcab3dec90..4aaf2fc21f8a3 100644 --- a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs +++ b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs @@ -93,7 +93,42 @@ impl Rule for NoReturnWrap { match arg_expr { Expression::ArrowFunctionExpression(arrow_expr) => { - check_first_return_statement(&arrow_expr.body, ctx, self.allow_reject); + println!("ddddddoooooo "); + if arrow_expr.body.statements.len() == 1 { + let Some(only_stmt) = &arrow_expr.body.statements.first() else { + return; + }; + + if let Statement::BlockStatement(_) = only_stmt { + check_first_return_statement(&arrow_expr.body, ctx, self.allow_reject); + }; + + if let Statement::ReturnStatement(r) = only_stmt { + println!("zoooooo {only_stmt:?}"); + if let Some(Expression::CallExpression(returned_call_expr)) = + &r.argument + { + check_for_resolve_reject( + ctx, + self.allow_reject, + &returned_call_expr, + ) + } + }; + + let Statement::ExpressionStatement(expr_stmt) = only_stmt else { + return; + }; + + let Expression::CallExpression(ref returned_call_expr) = + expr_stmt.expression + else { + return; + }; + check_for_resolve_reject(ctx, self.allow_reject, &returned_call_expr); + } else { + check_first_return_statement(&arrow_expr.body, ctx, self.allow_reject); + } } Expression::FunctionExpression(func_expr) => { let Some(func_body) = &func_expr.body else { @@ -148,6 +183,8 @@ impl Rule for NoReturnWrap { } fn check_callback_fn<'a>(ctx: &LintContext<'a>, allow_reject: bool, expr: &Expression<'a>) { + println!("sssss {expr:?}"); + match expr { Expression::ArrowFunctionExpression(arrow_expr) => { check_first_return_statement(&arrow_expr.body, ctx, allow_reject); @@ -162,7 +199,7 @@ fn check_callback_fn<'a>(ctx: &LintContext<'a>, allow_reject: bool, expr: &Expre } } -/// Checks for `return Promise.resolve()` or `return Promise.reject()` at top level statements and +/// Checks for `return` at top level statements and /// will look inside if no return is found as a top level statement in the function body. fn check_first_return_statement<'a>( func_body: &OBox<'_, FunctionBody<'a>>, @@ -203,10 +240,15 @@ fn check_first_return_statement<'a>( return; }; - let Some(Expression::CallExpression(call_expr)) = &return_stmt.argument else { + let Some(Expression::CallExpression(returned_call_expr)) = &return_stmt.argument else { return; }; + check_for_resolve_reject(ctx, allow_reject, &returned_call_expr) +} + +/// Checks for `return Promise.resolve()` or `return Promise.reject()` +fn check_for_resolve_reject(ctx: &LintContext, allow_reject: bool, call_expr: &CallExpression) { let Expression::StaticMemberExpression(stat_expr) = &call_expr.callee else { return; }; From d53d643da4dc61141db6a9201e30d4b83d00458d Mon Sep 17 00:00:00 2001 From: therewillbecode Date: Tue, 4 Mar 2025 11:26:27 +0000 Subject: [PATCH 16/26] Add another test case --- .../oxc_linter/src/rules/promise/no_return_wrap.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs index 4aaf2fc21f8a3..5923c48cbd706 100644 --- a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs +++ b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs @@ -430,6 +430,7 @@ fn test() { ), ("doThing().then((function() { return Promise.resolve(4) }).bind(this))", None), ("doThing().then((function() { return Promise.resolve(4) }).bind(this).bind(this))", None), + ("doThing().then(null, (function() { return Promise.resolve(4) }).bind(this))", None), ("doThing().then(() => { return Promise.resolve(4) })", None), ( "function a () { @@ -441,6 +442,19 @@ fn test() { ), ("doThing().then(() => Promise.resolve(4))", None), ("doThing().then(() => Promise.reject(4))", None), + ( + "fn((() => { + fn2(function() { + doThing().then(function() { + fn3(function() { + return Promise.resolve(4) + }) + return Promise.resolve(4) + }) + }).bind(this) + }))", + None, + ) ]; Tester::new(NoReturnWrap::NAME, NoReturnWrap::PLUGIN, pass, fail).test_and_snapshot(); From 0ea0aa07bf41f0b0c0d2fe26f68e739d38ba2244 Mon Sep 17 00:00:00 2001 From: therewillbecode Date: Tue, 4 Mar 2025 11:45:59 +0000 Subject: [PATCH 17/26] Fix clippys --- .../src/rules/promise/no_return_wrap.rs | 31 ++++++++----------- 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs index 5923c48cbd706..481dae3249f38 100644 --- a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs +++ b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs @@ -1,7 +1,7 @@ use oxc_allocator::Box as OBox; use oxc_ast::{ AstKind, - ast::{CallExpression, Expression, FunctionBody, MemberExpression, ReturnStatement, Statement}, + ast::{CallExpression, Expression, FunctionBody, MemberExpression, Statement}, }; use oxc_diagnostics::OxcDiagnostic; use oxc_macros::declare_oxc_lint; @@ -87,13 +87,13 @@ impl Rule for NoReturnWrap { } for argument in &call_expr.arguments { - let Some(arg_expr) = argument.as_expression().map(|a| a.without_parentheses()) else { + let Some(arg_expr) = argument.as_expression().map(Expression::without_parentheses) + else { continue; }; match arg_expr { Expression::ArrowFunctionExpression(arrow_expr) => { - println!("ddddddoooooo "); if arrow_expr.body.statements.len() == 1 { let Some(only_stmt) = &arrow_expr.body.statements.first() else { return; @@ -104,15 +104,14 @@ impl Rule for NoReturnWrap { }; if let Statement::ReturnStatement(r) = only_stmt { - println!("zoooooo {only_stmt:?}"); if let Some(Expression::CallExpression(returned_call_expr)) = &r.argument { check_for_resolve_reject( ctx, self.allow_reject, - &returned_call_expr, - ) + returned_call_expr, + ); } }; @@ -125,7 +124,7 @@ impl Rule for NoReturnWrap { else { return; }; - check_for_resolve_reject(ctx, self.allow_reject, &returned_call_expr); + check_for_resolve_reject(ctx, self.allow_reject, returned_call_expr); } else { check_first_return_statement(&arrow_expr.body, ctx, self.allow_reject); } @@ -183,8 +182,6 @@ impl Rule for NoReturnWrap { } fn check_callback_fn<'a>(ctx: &LintContext<'a>, allow_reject: bool, expr: &Expression<'a>) { - println!("sssss {expr:?}"); - match expr { Expression::ArrowFunctionExpression(arrow_expr) => { check_first_return_statement(&arrow_expr.body, ctx, allow_reject); @@ -244,7 +241,7 @@ fn check_first_return_statement<'a>( return; }; - check_for_resolve_reject(ctx, allow_reject, &returned_call_expr) + check_for_resolve_reject(ctx, allow_reject, returned_call_expr); } /// Checks for `return Promise.resolve()` or `return Promise.reject()` @@ -261,12 +258,10 @@ fn check_for_resolve_reject(ctx: &LintContext, allow_reject: bool, call_expr: &C return; } - if obj_call_ident.name == "Promise" { - if stat_expr.property.name == "resolve" { - ctx.diagnostic(no_return_wrap_diagnostic(call_expr.span)); - } else if stat_expr.property.name == "reject" && !allow_reject { - ctx.diagnostic(no_return_wrap_diagnostic(call_expr.span)); - } + if obj_call_ident.name == "Promise" && stat_expr.property.name == "resolve" + || (stat_expr.property.name == "reject" && !allow_reject) + { + ctx.diagnostic(no_return_wrap_diagnostic(call_expr.span)); } } @@ -437,7 +432,7 @@ fn test() { return p.then(function(val) { return Promise.resolve(val * 4) }) - }", + }", None, ), ("doThing().then(() => Promise.resolve(4))", None), @@ -454,7 +449,7 @@ fn test() { }).bind(this) }))", None, - ) + ), ]; Tester::new(NoReturnWrap::NAME, NoReturnWrap::PLUGIN, pass, fail).test_and_snapshot(); From ffb01c35cfef73b4a413f6a16fdabe339fd1663d Mon Sep 17 00:00:00 2001 From: therewillbecode Date: Tue, 4 Mar 2025 12:14:41 +0000 Subject: [PATCH 18/26] Better msg for user --- .../src/rules/promise/no_return_wrap.rs | 39 ++++++++++++++----- 1 file changed, 29 insertions(+), 10 deletions(-) diff --git a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs index 481dae3249f38..bccdb8333854b 100644 --- a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs +++ b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs @@ -5,15 +5,24 @@ use oxc_ast::{ }; use oxc_diagnostics::OxcDiagnostic; use oxc_macros::declare_oxc_lint; -use oxc_span::Span; +use oxc_span::{CompactStr, Span}; use crate::{AstNode, context::LintContext, rule::Rule}; -fn no_return_wrap_diagnostic(span: Span) -> OxcDiagnostic { - // See for details - OxcDiagnostic::warn("Should be an imperative statement about what is wrong") - .with_help("Should be a command-like statement that tells the user how to fix the issue") - .with_label(span) +fn no_return_wrap_diagnostic(span: Span, issue: &ReturnWrapper) -> OxcDiagnostic { + let warn_msg: CompactStr = match issue { + ReturnWrapper::Resolve => "Avoid wrapping return values in Promise.resolve".into(), + ReturnWrapper::Reject => "Expected throw instead of Promise.reject".into(), + }; + + let help_msg: CompactStr = match issue { + ReturnWrapper::Resolve => { + "Return the value being passed into Promise.resolve instead".into() + } + ReturnWrapper::Reject => "Throw the value being passed into Promise.reject instead".into(), + }; + + OxcDiagnostic::warn(warn_msg).with_help(help_msg).with_label(span) } #[derive(Debug, Default, Clone)] @@ -21,6 +30,12 @@ pub struct NoReturnWrap { allow_reject: bool, } +#[derive(Debug, PartialEq)] +enum ReturnWrapper { + Resolve, + Reject, +} + declare_oxc_lint!( /// ### What it does /// @@ -258,10 +273,14 @@ fn check_for_resolve_reject(ctx: &LintContext, allow_reject: bool, call_expr: &C return; } - if obj_call_ident.name == "Promise" && stat_expr.property.name == "resolve" - || (stat_expr.property.name == "reject" && !allow_reject) - { - ctx.diagnostic(no_return_wrap_diagnostic(call_expr.span)); + if !(obj_call_ident.name == "Promise") { + return; + } + + if stat_expr.property.name == "resolve" { + ctx.diagnostic(no_return_wrap_diagnostic(call_expr.span, &ReturnWrapper::Resolve)); + } else if stat_expr.property.name == "reject" && !allow_reject { + ctx.diagnostic(no_return_wrap_diagnostic(call_expr.span, &ReturnWrapper::Reject)); } } From c80a9b4ed119663c6c2e4d36d43b3dc17148abd4 Mon Sep 17 00:00:00 2001 From: therewillbecode Date: Tue, 4 Mar 2025 12:26:32 +0000 Subject: [PATCH 19/26] Add doc examples --- .../src/rules/promise/no_return_wrap.rs | 26 +++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs index bccdb8333854b..501eb28d9d166 100644 --- a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs +++ b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs @@ -65,12 +65,34 @@ declare_oxc_lint!( /// /// Examples of **incorrect** code for this rule: /// ```js - /// FIXME: Tests will fail if examples are missing or syntactically incorrect. + /// myPromise().then(() => Promise.resolve(4)) + /// myPromise().then(function() { return Promise.resolve(4) }) + /// + /// myPromise().then(() => Promise.reject("err")) + /// myPromise().then(function() { return Promise.reject("err") }) + /// ``` + /// + /// ```js + /// myPromise().catch( + /// function() { + /// return Promise.reject("err") + /// }) /// ``` /// /// Examples of **correct** code for this rule: /// ```js - /// FIXME: Tests will fail if examples are missing or syntactically incorrect. + /// myPromise().then(() => 4) + /// myPromise().then(function() { return 4 }) + /// + /// myPromise().then(() => throw "err") + /// myPromise().then(function() { throw "err" }) + /// ``` + /// + /// ```js + /// myPromise().catch( + /// function() { + /// throw "err" + /// }) /// ``` /// /// ### Options From 1e971ecaac26e28308fda6cd009501a419055b34 Mon Sep 17 00:00:00 2001 From: therewillbecode Date: Tue, 4 Mar 2025 12:41:48 +0000 Subject: [PATCH 20/26] Handle finally --- .../src/rules/promise/no_return_wrap.rs | 29 ++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs index 501eb28d9d166..68fc64a6c05ce 100644 --- a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs +++ b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs @@ -79,6 +79,17 @@ declare_oxc_lint!( /// }) /// ``` /// + /// ```js + /// myPromise().finally( + /// function() { + /// return Promise.reject("err") + /// }) + /// ``` + /// + /// ```js + /// myPromise().finally(() => Promise.resolve(4)) + /// ``` + /// /// Examples of **correct** code for this rule: /// ```js /// myPromise().then(() => 4) @@ -95,6 +106,10 @@ declare_oxc_lint!( /// }) /// ``` /// + /// ```js + /// myPromise().finally(() => 4) + /// ``` + /// /// ### Options /// NoReturnWrap, @@ -316,7 +331,7 @@ fn inside_then_or_catch<'a, 'b>(node: &'a AstNode<'b>, ctx: &'a LintContext<'b>) .callee .as_member_expression() .and_then(MemberExpression::static_property_name), - Some("then" | "catch") + Some("then" | "catch" | "finally") ) }) }) @@ -337,6 +352,8 @@ fn test() { ("doThing().then(null, function() { throw 4 })", None), ("doThing().catch(null, function() { return 4 })", None), ("doThing().catch(null, function() { throw 4 })", None), + ("doThing().then(() => {}).finally(() => 4)", None), + (r#"doThing().then(() => {}).finally(() => { throw "err" })"#, None), ("doThing().then(function() { return Promise.all([a,b,c]) })", None), ("doThing().then(() => 4)", None), ("doThing().then(() => { throw 4 })", None), @@ -353,6 +370,11 @@ fn test() { "doThing().then(function() { return Promise.reject(4) })", Some(serde_json::json!([{ "allowReject": true }])), ), + (r#"doThing().then(function () {}).finally(function () { Promise.reject("err") })"#, None), + ( + r#"doThing().then(function () {}).finally(function () { return Promise.reject("err") })"#, + Some(serde_json::json!([{ "allowReject": true }])), + ), ("doThing().then((function() { return Promise.resolve(4) }).toString())", None), ( "doThing().then(() => Promise.reject(4))", @@ -378,6 +400,11 @@ fn test() { ("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), + ("doThing().finally(() => Promise.resolve(4))", None), + ( + r#"doThing().then(function () {}).finally(function () { return Promise.reject("err") })"#, + None, + ), ( r#"doThing().then( function(x) { From 462b217083f72738f942ecbf7621faa973011d42 Mon Sep 17 00:00:00 2001 From: therewillbecode Date: Tue, 4 Mar 2025 12:46:38 +0000 Subject: [PATCH 21/26] Use existing is_promise from utils --- .../src/rules/promise/no_return_wrap.rs | 21 ++++++------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs index 68fc64a6c05ce..b5c3d096c2cae 100644 --- a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs +++ b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs @@ -1,13 +1,13 @@ use oxc_allocator::Box as OBox; use oxc_ast::{ AstKind, - ast::{CallExpression, Expression, FunctionBody, MemberExpression, Statement}, + ast::{CallExpression, Expression, FunctionBody, Statement}, }; use oxc_diagnostics::OxcDiagnostic; use oxc_macros::declare_oxc_lint; use oxc_span::{CompactStr, Span}; -use crate::{AstNode, context::LintContext, rule::Rule}; +use crate::{AstNode, context::LintContext, rule::Rule, utils::is_promise}; fn no_return_wrap_diagnostic(span: Span, issue: &ReturnWrapper) -> OxcDiagnostic { let warn_msg: CompactStr = match issue { @@ -134,7 +134,7 @@ impl Rule for NoReturnWrap { return; }; - if !inside_then_or_catch(node, ctx) { + if !inside_promise_cb(node, ctx) { return; } @@ -321,19 +321,10 @@ fn check_for_resolve_reject(ctx: &LintContext, allow_reject: bool, call_expr: &C } } -/// Return true if this node is inside a `then` or `catch` promise callback. Will return `true` -/// for `node` in both `prom.then(null, () => node)` and `prom.then(() => node)`. -fn inside_then_or_catch<'a, 'b>(node: &'a AstNode<'b>, ctx: &'a LintContext<'b>) -> bool { +/// Return true if this node is inside a `then` or `catch` or `finally` promise callback. +fn inside_promise_cb<'a, 'b>(node: &'a AstNode<'b>, ctx: &'a LintContext<'b>) -> bool { ctx.nodes().ancestors(node.id()).any(|node| { - node.kind().as_call_expression().is_some_and(|call_expr| { - matches!( - call_expr - .callee - .as_member_expression() - .and_then(MemberExpression::static_property_name), - Some("then" | "catch" | "finally") - ) - }) + node.kind().as_call_expression().is_some_and(|call_expr| is_promise(call_expr).is_some()) }) } From d8d551cda7837a333a74e698400c88d263e2a280 Mon Sep 17 00:00:00 2001 From: therewillbecode Date: Tue, 4 Mar 2025 12:55:03 +0000 Subject: [PATCH 22/26] Switch arg order --- .../src/rules/promise/no_return_wrap.rs | 81 ++++++++++--------- 1 file changed, 41 insertions(+), 40 deletions(-) diff --git a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs index b5c3d096c2cae..c646935c1ec6b 100644 --- a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs +++ b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs @@ -1,7 +1,7 @@ use oxc_allocator::Box as OBox; use oxc_ast::{ AstKind, - ast::{CallExpression, Expression, FunctionBody, Statement}, + ast::{ArrowFunctionExpression, CallExpression, Expression, FunctionBody, Statement}, }; use oxc_diagnostics::OxcDiagnostic; use oxc_macros::declare_oxc_lint; @@ -145,47 +145,14 @@ impl Rule for NoReturnWrap { }; match arg_expr { - Expression::ArrowFunctionExpression(arrow_expr) => { - if arrow_expr.body.statements.len() == 1 { - let Some(only_stmt) = &arrow_expr.body.statements.first() else { - return; - }; - - if let Statement::BlockStatement(_) = only_stmt { - check_first_return_statement(&arrow_expr.body, ctx, self.allow_reject); - }; - - if let Statement::ReturnStatement(r) = only_stmt { - if let Some(Expression::CallExpression(returned_call_expr)) = - &r.argument - { - check_for_resolve_reject( - ctx, - self.allow_reject, - returned_call_expr, - ); - } - }; - - let Statement::ExpressionStatement(expr_stmt) = only_stmt else { - return; - }; - - let Expression::CallExpression(ref returned_call_expr) = - expr_stmt.expression - else { - return; - }; - check_for_resolve_reject(ctx, self.allow_reject, returned_call_expr); - } else { - check_first_return_statement(&arrow_expr.body, ctx, self.allow_reject); - } + Expression::ArrowFunctionExpression(arrow) => { + check_arrow_cb_arg(ctx, self.allow_reject, arrow); } Expression::FunctionExpression(func_expr) => { let Some(func_body) = &func_expr.body else { continue; }; - check_first_return_statement(func_body, ctx, self.allow_reject); + check_first_return_statement(ctx, func_body, self.allow_reject); } Expression::CallExpression(call) => { let Expression::StaticMemberExpression(static_memb_expr) = @@ -233,16 +200,50 @@ impl Rule for NoReturnWrap { } } +/// Look for issues in the arrow callback `cb` in `myProm().then(cb)`. +fn check_arrow_cb_arg<'a>( + ctx: &LintContext<'a>, + allow_reject: bool, + arrow_expr: &ArrowFunctionExpression<'a>, +) { + if arrow_expr.body.statements.len() == 1 { + let Some(only_stmt) = &arrow_expr.body.statements.first() else { + return; + }; + + if let Statement::BlockStatement(_) = only_stmt { + check_first_return_statement(ctx, &arrow_expr.body, allow_reject); + }; + + if let Statement::ReturnStatement(r) = only_stmt { + if let Some(Expression::CallExpression(returned_call_expr)) = &r.argument { + check_for_resolve_reject(ctx, allow_reject, returned_call_expr); + } + }; + + let Statement::ExpressionStatement(expr_stmt) = only_stmt else { + return; + }; + + let Expression::CallExpression(ref returned_call_expr) = expr_stmt.expression else { + return; + }; + check_for_resolve_reject(ctx, allow_reject, returned_call_expr); + } else { + check_first_return_statement(ctx, &arrow_expr.body, allow_reject); + } +} + fn check_callback_fn<'a>(ctx: &LintContext<'a>, allow_reject: bool, expr: &Expression<'a>) { match expr { Expression::ArrowFunctionExpression(arrow_expr) => { - check_first_return_statement(&arrow_expr.body, ctx, allow_reject); + check_first_return_statement(ctx, &arrow_expr.body, allow_reject); } Expression::FunctionExpression(func_expr) => { let Some(func_body) = &func_expr.body else { return; }; - check_first_return_statement(func_body, ctx, allow_reject); + check_first_return_statement(ctx, func_body, allow_reject); } _ => (), } @@ -251,8 +252,8 @@ fn check_callback_fn<'a>(ctx: &LintContext<'a>, allow_reject: bool, expr: &Expre /// Checks for `return` at top level statements and /// will look inside if no return is found as a top level statement in the function body. fn check_first_return_statement<'a>( - func_body: &OBox<'_, FunctionBody<'a>>, ctx: &LintContext<'a>, + func_body: &OBox<'_, FunctionBody<'a>>, allow_reject: bool, ) { let top_level_statements = func_body From 810688f45dda6e45b162d3868da474a9fdcd35f8 Mon Sep 17 00:00:00 2001 From: therewillbecode Date: Tue, 4 Mar 2025 13:05:07 +0000 Subject: [PATCH 23/26] Document option --- .../src/rules/promise/no_return_wrap.rs | 27 ++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs index c646935c1ec6b..153c5e3cb0ff7 100644 --- a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs +++ b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs @@ -112,6 +112,27 @@ declare_oxc_lint!( /// /// ### Options /// + /// + /// #### allowReject + /// + /// `{ type: boolean, default: false }` + /// + /// The `allowReject` turns off the checking of returning a call `Promise.reject` inside a + /// promise handler. + /// + /// With `allowReject` set to `true` the following are examples of correct code: + /// + /// ```js + /// myPromise().then( + /// function() { + /// return Promise.reject(0) + /// }) + /// ``` + /// + /// ```js + /// myPromise().then().catch(() => Promise.reject("err")) + /// ``` + /// NoReturnWrap, promise, nursery, @@ -359,10 +380,14 @@ fn test() { ("doThing(function(x) { return Promise.reject(x) })", None), ("doThing().then(function() { return })", None), ( - "doThing().then(function() { return Promise.reject(4) })", + "doThing().then(function() { return Promise.reject(0) })", Some(serde_json::json!([{ "allowReject": true }])), ), (r#"doThing().then(function () {}).finally(function () { Promise.reject("err") })"#, None), + ( + r#"doThing().then().catch(() => Promise.reject("err"))"#, + Some(serde_json::json!([{ "allowReject": true }])), + ), ( r#"doThing().then(function () {}).finally(function () { return Promise.reject("err") })"#, Some(serde_json::json!([{ "allowReject": true }])), From d03a02ad54caf617d7612ffbdf4b4d3f03ad76d5 Mon Sep 17 00:00:00 2001 From: therewillbecode Date: Tue, 4 Mar 2025 13:06:58 +0000 Subject: [PATCH 24/26] Add test snapshot --- .../src/snapshots/promise_no_return_wrap.snap | 222 ++++++++++++++++++ 1 file changed, 222 insertions(+) create mode 100644 crates/oxc_linter/src/snapshots/promise_no_return_wrap.snap diff --git a/crates/oxc_linter/src/snapshots/promise_no_return_wrap.snap b/crates/oxc_linter/src/snapshots/promise_no_return_wrap.snap new file mode 100644 index 0000000000000..ca499465738a5 --- /dev/null +++ b/crates/oxc_linter/src/snapshots/promise_no_return_wrap.snap @@ -0,0 +1,222 @@ +--- +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:36] + 1 │ doThing().then(function() { return Promise.resolve(4) }) + · ────────────────── + ╰──── + help: Return the value being passed into Promise.resolve instead + + ⚠ eslint-plugin-promise(no-return-wrap): Avoid wrapping return values in Promise.resolve + ╭─[no_return_wrap.tsx:1:42] + 1 │ doThing().then(null, function() { return Promise.resolve(4) }) + · ────────────────── + ╰──── + help: Return the value being passed into Promise.resolve instead + + ⚠ eslint-plugin-promise(no-return-wrap): Avoid wrapping return values in Promise.resolve + ╭─[no_return_wrap.tsx:1:37] + 1 │ doThing().catch(function() { return Promise.resolve(4) }) + · ────────────────── + ╰──── + help: Return the value being passed into Promise.resolve instead + + ⚠ eslint-plugin-promise(no-return-wrap): Expected throw instead of Promise.reject + ╭─[no_return_wrap.tsx:1:36] + 1 │ doThing().then(function() { return Promise.reject(4) }) + · ───────────────── + ╰──── + help: Throw the value being passed into Promise.reject instead + + ⚠ eslint-plugin-promise(no-return-wrap): Expected throw instead of Promise.reject + ╭─[no_return_wrap.tsx:1:42] + 1 │ doThing().then(null, function() { return Promise.reject(4) }) + · ───────────────── + ╰──── + help: Throw the value being passed into Promise.reject instead + + ⚠ eslint-plugin-promise(no-return-wrap): Expected throw instead of Promise.reject + ╭─[no_return_wrap.tsx:1:37] + 1 │ doThing().catch(function() { return Promise.reject(4) }) + · ───────────────── + ╰──── + help: Throw the value being passed into Promise.reject instead + + ⚠ eslint-plugin-promise(no-return-wrap): Avoid wrapping return values in Promise.resolve + ╭─[no_return_wrap.tsx:1:25] + 1 │ doThing().finally(() => Promise.resolve(4)) + · ────────────────── + ╰──── + help: Return the value being passed into Promise.resolve instead + + ⚠ eslint-plugin-promise(no-return-wrap): Expected throw instead of Promise.reject + ╭─[no_return_wrap.tsx:1:61] + 1 │ doThing().then(function () {}).finally(function () { return Promise.reject("err") }) + · ───────────────────── + ╰──── + help: Throw the value being passed into Promise.reject instead + + ⚠ eslint-plugin-promise(no-return-wrap): Avoid wrapping return values in Promise.resolve + ╭─[no_return_wrap.tsx:4:29] + 3 │ if (x>1) { + 4 │ return Promise.resolve(4) + · ────────────────── + 5 │ } else { + ╰──── + help: Return the value being passed into Promise.resolve instead + + ⚠ eslint-plugin-promise(no-return-wrap): Expected throw instead of Promise.reject + ╭─[no_return_wrap.tsx:1:48] + 1 │ doThing().then(function(x) { if (x>1) { return Promise.reject(4) } }) + · ───────────────── + ╰──── + help: Throw the value being passed into Promise.reject instead + + ⚠ eslint-plugin-promise(no-return-wrap): Avoid wrapping return values in Promise.resolve + ╭─[no_return_wrap.tsx:1:63] + 1 │ doThing().then(null, function() { if (true && false) { return Promise.resolve() } }) + · ───────────────── + ╰──── + help: Return the value being passed into Promise.resolve instead + + ⚠ eslint-plugin-promise(no-return-wrap): Avoid wrapping return values in Promise.resolve + ╭─[no_return_wrap.tsx:4:26] + 3 │ if (x) { + 4 │ return Promise.resolve(4) + · ────────────────── + 5 │ } else { + ╰──── + help: Return the value being passed into Promise.resolve instead + + ⚠ eslint-plugin-promise(no-return-wrap): Avoid wrapping return values in Promise.resolve + ╭─[no_return_wrap.tsx:3:16] + 2 │ doThing().then(function() { + 3 │ return Promise.resolve(4) + · ────────────────── + 4 │ }) + ╰──── + help: Return the value being passed into Promise.resolve instead + + ⚠ eslint-plugin-promise(no-return-wrap): Avoid wrapping return values in Promise.resolve + ╭─[no_return_wrap.tsx:3:16] + 2 │ doThing().then(function nm() { + 3 │ return Promise.resolve(4) + · ────────────────── + 4 │ }) + ╰──── + help: Return the value being passed into Promise.resolve instead + + ⚠ eslint-plugin-promise(no-return-wrap): Avoid wrapping return values in Promise.resolve + ╭─[no_return_wrap.tsx:4:18] + 3 │ doThing().then(function() { + 4 │ return Promise.resolve(4) + · ────────────────── + 5 │ }) + ╰──── + help: Return the value being passed into Promise.resolve instead + + ⚠ eslint-plugin-promise(no-return-wrap): Avoid wrapping return values in Promise.resolve + ╭─[no_return_wrap.tsx:7:18] + 6 │ }) + 7 │ return Promise.resolve(4) + · ────────────────── + 8 │ }) + ╰──── + help: Return the value being passed into Promise.resolve instead + + ⚠ eslint-plugin-promise(no-return-wrap): Avoid wrapping return values in Promise.resolve + ╭─[no_return_wrap.tsx:5:20] + 4 │ fn3(function() { + 5 │ return Promise.resolve(4) + · ────────────────── + 6 │ }) + ╰──── + help: Return the value being passed into Promise.resolve instead + + ⚠ eslint-plugin-promise(no-return-wrap): Avoid wrapping return values in Promise.resolve + ╭─[no_return_wrap.tsx:4:18] + 3 │ return doThing().then(function() { + 4 │ return Promise.resolve(5); + · ────────────────── + 5 │ }); + ╰──── + help: Return the value being passed into Promise.resolve instead + + ⚠ eslint-plugin-promise(no-return-wrap): Avoid wrapping return values in Promise.resolve + ╭─[no_return_wrap.tsx:3:16] + 2 │ doThing().then(function() { + 3 │ return Promise.resolve(5); + · ────────────────── + 4 │ }) + ╰──── + help: Return the value being passed into Promise.resolve instead + + ⚠ eslint-plugin-promise(no-return-wrap): Avoid wrapping return values in Promise.resolve + ╭─[no_return_wrap.tsx:1:37] + 1 │ doThing().then((function() { return Promise.resolve(4) }).bind(this)) + · ────────────────── + ╰──── + help: Return the value being passed into Promise.resolve instead + + ⚠ eslint-plugin-promise(no-return-wrap): Avoid wrapping return values in Promise.resolve + ╭─[no_return_wrap.tsx:1:37] + 1 │ doThing().then((function() { return Promise.resolve(4) }).bind(this).bind(this)) + · ────────────────── + ╰──── + help: Return the value being passed into Promise.resolve instead + + ⚠ eslint-plugin-promise(no-return-wrap): Avoid wrapping return values in Promise.resolve + ╭─[no_return_wrap.tsx:1:43] + 1 │ doThing().then(null, (function() { return Promise.resolve(4) }).bind(this)) + · ────────────────── + ╰──── + help: Return the value being passed into Promise.resolve instead + + ⚠ eslint-plugin-promise(no-return-wrap): Avoid wrapping return values in Promise.resolve + ╭─[no_return_wrap.tsx:1:31] + 1 │ doThing().then(() => { return Promise.resolve(4) }) + · ────────────────── + ╰──── + help: Return the value being passed into Promise.resolve instead + + ⚠ eslint-plugin-promise(no-return-wrap): Avoid wrapping return values in Promise.resolve + ╭─[no_return_wrap.tsx:3:16] + 2 │ return p.then(function(val) { + 3 │ return Promise.resolve(val * 4) + · ──────────────────────── + 4 │ }) + ╰──── + help: Return the value being passed into Promise.resolve instead + + ⚠ eslint-plugin-promise(no-return-wrap): Avoid wrapping return values in Promise.resolve + ╭─[no_return_wrap.tsx:1:22] + 1 │ doThing().then(() => Promise.resolve(4)) + · ────────────────── + ╰──── + help: Return the value being passed into Promise.resolve instead + + ⚠ eslint-plugin-promise(no-return-wrap): Expected throw instead of Promise.reject + ╭─[no_return_wrap.tsx:1:22] + 1 │ doThing().then(() => Promise.reject(4)) + · ───────────────── + ╰──── + help: Throw the value being passed into Promise.reject instead + + ⚠ eslint-plugin-promise(no-return-wrap): Avoid wrapping return values in Promise.resolve + ╭─[no_return_wrap.tsx:7:18] + 6 │ }) + 7 │ return Promise.resolve(4) + · ────────────────── + 8 │ }) + ╰──── + help: Return the value being passed into Promise.resolve instead + + ⚠ eslint-plugin-promise(no-return-wrap): Avoid wrapping return values in Promise.resolve + ╭─[no_return_wrap.tsx:5:20] + 4 │ fn3(function() { + 5 │ return Promise.resolve(4) + · ────────────────── + 6 │ }) + ╰──── + help: Return the value being passed into Promise.resolve instead From f7122ac7d2148ec7fddcfdab889007621b819821 Mon Sep 17 00:00:00 2001 From: therewillbecode Date: Sat, 8 Mar 2025 20:13:51 +0000 Subject: [PATCH 25/26] Implement PR feedback --- .../src/rules/promise/no_return_wrap.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs index 153c5e3cb0ff7..274dbabb2e9e1 100644 --- a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs +++ b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs @@ -5,21 +5,21 @@ use oxc_ast::{ }; use oxc_diagnostics::OxcDiagnostic; use oxc_macros::declare_oxc_lint; -use oxc_span::{CompactStr, Span}; +use oxc_span::Span; use crate::{AstNode, context::LintContext, rule::Rule, utils::is_promise}; fn no_return_wrap_diagnostic(span: Span, issue: &ReturnWrapper) -> OxcDiagnostic { - let warn_msg: CompactStr = match issue { - ReturnWrapper::Resolve => "Avoid wrapping return values in Promise.resolve".into(), - ReturnWrapper::Reject => "Expected throw instead of Promise.reject".into(), + let warn_msg = match issue { + ReturnWrapper::Resolve => "Avoid wrapping return values in Promise.resolve", + ReturnWrapper::Reject => "Expected throw instead of Promise.reject", }; - let help_msg: CompactStr = match issue { + let help_msg = match issue { ReturnWrapper::Resolve => { - "Return the value being passed into Promise.resolve instead".into() + "Return the value being passed into Promise.resolve instead" } - ReturnWrapper::Reject => "Throw the value being passed into Promise.reject instead".into(), + ReturnWrapper::Reject => "Throw the value being passed into Promise.reject instead", }; OxcDiagnostic::warn(warn_msg).with_help(help_msg).with_label(span) @@ -135,7 +135,7 @@ declare_oxc_lint!( /// NoReturnWrap, promise, - nursery, + style, pending ); From d744441a4b4e42ea1b9259fde47a5bf4152b98e0 Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Sat, 8 Mar 2025 20:14:55 +0000 Subject: [PATCH 26/26] [autofix.ci] apply automated fixes --- crates/oxc_linter/src/rules/promise/no_return_wrap.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs index 274dbabb2e9e1..6c1f566ca0a52 100644 --- a/crates/oxc_linter/src/rules/promise/no_return_wrap.rs +++ b/crates/oxc_linter/src/rules/promise/no_return_wrap.rs @@ -16,9 +16,7 @@ fn no_return_wrap_diagnostic(span: Span, issue: &ReturnWrapper) -> OxcDiagnostic }; let help_msg = match issue { - ReturnWrapper::Resolve => { - "Return the value being passed into Promise.resolve instead" - } + ReturnWrapper::Resolve => "Return the value being passed into Promise.resolve instead", ReturnWrapper::Reject => "Throw the value being passed into Promise.reject instead", };