diff --git a/crates/biome_js_analyze/src/lint/complexity/no_useless_catch.rs b/crates/biome_js_analyze/src/lint/complexity/no_useless_catch.rs index ddcad733fb52..1577c329d4a8 100644 --- a/crates/biome_js_analyze/src/lint/complexity/no_useless_catch.rs +++ b/crates/biome_js_analyze/src/lint/complexity/no_useless_catch.rs @@ -4,11 +4,8 @@ use biome_analyze::{ }; use biome_console::markup; use biome_js_factory::make; -use biome_js_syntax::{ - AnyJsModuleItem, AnyJsStatement, JsBlockStatement, JsCatchClause, JsExpressionStatement, - JsModule, JsSyntaxToken, JsTryFinallyStatement, JsTryStatement, TextRange, -}; -use biome_rowan::{declare_node_union, AstNode, AstNodeList, BatchMutationExt}; +use biome_js_syntax::{AnyJsTryStatement, JsStatementList, TextRange}; +use biome_rowan::{AstNode, AstNodeList, BatchMutationExt}; use crate::JsRuleAction; @@ -78,21 +75,19 @@ declare_lint_rule! { } impl Rule for NoUselessCatch { - type Query = Ast; + type Query = Ast; type State = TextRange; type Signals = Option; type Options = (); fn run(ctx: &RuleContext) -> Self::Signals { - let binding = ctx.query(); - - let catch_clause = binding.catch_clause()?; - + let node = ctx.query(); + let catch_clause = node.catch_clause()?; let catch_body = catch_clause.body().ok()?; - let body_statements = catch_body.statements(); + let catch_body_statements = catch_body.statements(); // We need guarantees that body_statements has only one `throw` statement. - if body_statements.len() != 1 { + if catch_body_statements.len() > 1 { return None; } @@ -106,7 +101,7 @@ impl Rule for NoUselessCatch { .ok()?; let catch_err_name = catch_binding_err.text(); - let first_statement = body_statements.first()?; + let first_statement = catch_body_statements.first()?; let js_throw_statement = first_statement.as_js_throw_statement()?; let throw_ident = js_throw_statement .argument() @@ -126,93 +121,43 @@ impl Rule for NoUselessCatch { RuleDiagnostic::new( rule_category!(), text_range, - markup!("The ""catch"" clause that only rethrows the original error is redundant."), + markup!("The ""catch"" clause that only rethrows the original error is useless."), ) .note(markup!( - "These unnecessary ""catch"" clauses can be confusing. It is recommended to remove them." + "An unnecessary ""catch"" clause can be confusing." )), ) } fn action(ctx: &RuleContext, _: &Self::State) -> Option { let node = ctx.query(); - - let body = node.body()?; - let stmts = body.statements(); - let expressions: Vec = stmts - .iter() - .map(|stmt| stmt.as_js_expression_statement().unwrap().clone()) - .collect(); - - let catch_clause = node.catch_clause()?; - let finally_clause = node.finally_clause(); - let mut mutation = ctx.root().begin(); - let note = if finally_clause.is_some() { + let note = if node.finally_clause().is_some() { + let catch_clause = node.catch_clause()?; mutation.remove_node(catch_clause); "catch" } else { - let module = make::js_module( - make::js_directive_list(None), - make::js_module_item_list(expressions.into_iter().map(|expression| { - AnyJsModuleItem::AnyJsStatement(AnyJsStatement::JsExpressionStatement( - expression.clone(), - )) - })), - make::eof(), - ) - .build(); - mutation.replace_node_discard_trivia( - node.clone(), - AnyJsStatementAndModule::JsModule(module), - ); + let try_stmts = node.body().ok()?.statements(); + let stmts_list = node.parent::()?; + let node = node.syntax(); + let try_pos = stmts_list.iter().position(|x| x.syntax() == node)?; + let prev_stmts = stmts_list.iter().take(try_pos); + let next_stmts = stmts_list.iter().skip(try_pos + 1); + let new_stmts = prev_stmts + .chain(try_stmts) + .chain(next_stmts) + .collect::>(); + let new_stmts_list = make::js_statement_list(new_stmts); + mutation.replace_node_discard_trivia(stmts_list, new_stmts_list); "try/catch" }; - return Some(JsRuleAction::new( + Some(JsRuleAction::new( ActionCategory::QuickFix, ctx.metadata().applicability(), markup!("Remove the "{note}" clause.").to_owned(), mutation, - )); - } -} - -declare_node_union! { - pub AnyJsStatementAndModule = JsTryStatement | JsTryFinallyStatement | JsModule -} - -impl AnyJsStatementAndModule { - pub fn catch_clause(&self) -> Option { - match self { - AnyJsStatementAndModule::JsTryStatement(node) => node.catch_clause().ok(), - AnyJsStatementAndModule::JsTryFinallyStatement(node) => node.catch_clause(), - AnyJsStatementAndModule::JsModule(_) => None, - } - } - - pub fn finally_clause(&self) -> Option { - match self { - AnyJsStatementAndModule::JsTryStatement(_) => None, - AnyJsStatementAndModule::JsTryFinallyStatement(node) => Some(node.clone()), - AnyJsStatementAndModule::JsModule(_) => None, - } - } - - pub fn body(&self) -> Option { - match self { - AnyJsStatementAndModule::JsTryStatement(node) => node.body().ok(), - AnyJsStatementAndModule::JsTryFinallyStatement(node) => node.body().ok(), - AnyJsStatementAndModule::JsModule(_) => None, - } - } - - pub fn try_token(&self) -> Option { - match self { - AnyJsStatementAndModule::JsTryStatement(node) => node.try_token().ok(), - AnyJsStatementAndModule::JsTryFinallyStatement(node) => node.try_token().ok(), - AnyJsStatementAndModule::JsModule(_) => None, - } + )) } } diff --git a/crates/biome_js_analyze/src/services/control_flow/nodes/try_catch.rs b/crates/biome_js_analyze/src/services/control_flow/nodes/try_catch.rs index aeac6ecb842a..ecaed4d8ed1e 100644 --- a/crates/biome_js_analyze/src/services/control_flow/nodes/try_catch.rs +++ b/crates/biome_js_analyze/src/services/control_flow/nodes/try_catch.rs @@ -1,16 +1,12 @@ use biome_control_flow::{builder::BlockId, ExceptionHandlerKind}; -use biome_js_syntax::{JsCatchClause, JsFinallyClause, JsTryFinallyStatement, JsTryStatement}; -use biome_rowan::{declare_node_union, SyntaxResult}; +use biome_js_syntax::{AnyJsTryStatement, JsCatchClause, JsFinallyClause}; +use biome_rowan::SyntaxResult; use crate::services::control_flow::{ visitor::{NodeVisitor, StatementStack}, FunctionBuilder, }; -declare_node_union! { - pub(in crate::services::control_flow) AnyJsTryStatement = JsTryStatement | JsTryFinallyStatement -} - pub(in crate::services::control_flow) struct TryVisitor { catch_block: Option, finally_block: Option, diff --git a/crates/biome_js_analyze/tests/specs/complexity/noUselessCatch/invalid.js b/crates/biome_js_analyze/tests/specs/complexity/noUselessCatch/invalid.js index 5c1ff7a50ff9..82723d754542 100644 --- a/crates/biome_js_analyze/tests/specs/complexity/noUselessCatch/invalid.js +++ b/crates/biome_js_analyze/tests/specs/complexity/noUselessCatch/invalid.js @@ -1,14 +1,23 @@ -try { - doSomethingThatMightThrow(); -} catch (e) { - throw e; +function foo () { + f(); + try { + doSomethingThatMightThrowA(); + doSomethingThatMightThrowB(); + } catch (e) { + throw e; + } + g(); } -try { - doSomethingThatMightThrowA(); - doSomethingThatMightThrowB(); -} catch (e) { - throw e; +for(let i = 0; i < 5; i++) { + try { + doSomethingThatMightThrowA(); + if (true) { + doSomethingThatMightThrowB(); + } + } catch (e) { + throw e; + } } try { diff --git a/crates/biome_js_analyze/tests/specs/complexity/noUselessCatch/invalid.js.snap b/crates/biome_js_analyze/tests/specs/complexity/noUselessCatch/invalid.js.snap index 377378b2b98c..c7881e21d9dc 100644 --- a/crates/biome_js_analyze/tests/specs/complexity/noUselessCatch/invalid.js.snap +++ b/crates/biome_js_analyze/tests/specs/complexity/noUselessCatch/invalid.js.snap @@ -4,17 +4,26 @@ expression: invalid.js --- # Input ```jsx -try { - doSomethingThatMightThrow(); -} catch (e) { - throw e; +function foo () { + f(); + try { + doSomethingThatMightThrowA(); + doSomethingThatMightThrowB(); + } catch (e) { + throw e; + } + g(); } -try { - doSomethingThatMightThrowA(); - doSomethingThatMightThrowB(); -} catch (e) { - throw e; +for(let i = 0; i < 5; i++) { + try { + doSomethingThatMightThrowA(); + if (true) { + doSomethingThatMightThrowB(); + } + } catch (e) { + throw e; + } } try { @@ -29,88 +38,95 @@ try { # Diagnostics ``` -invalid.js:4:2 lint/complexity/noUselessCatch FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +invalid.js:7:3 lint/complexity/noUselessCatch FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! The catch clause that only rethrows the original error is redundant. + ! The catch clause that only rethrows the original error is useless. - 2 │ doSomethingThatMightThrow(); - 3 │ } catch (e) { - > 4 │ throw e; - │ ^^^^^^^^ - 5 │ } - 6 │ + 5 │ doSomethingThatMightThrowB(); + 6 │ } catch (e) { + > 7 │ throw e; + │ ^^^^^^^^ + 8 │ } + 9 │ g(); - i These unnecessary catch clauses can be confusing. It is recommended to remove them. + i An unnecessary catch clause can be confusing. i Unsafe fix: Remove the try/catch clause. - 1 │ - try·{ - 1 │ + - 2 2 │ doSomethingThatMightThrow(); - 3 │ - }·catch·(e)·{ - 4 │ - → throw·e; - 5 │ - } - 6 3 │ - 7 4 │ try { + 1 1 │ function foo () { + 2 2 │ f(); + 3 │ - → try·{ + 4 │ - → → doSomethingThatMightThrowA(); + 5 │ - → → doSomethingThatMightThrowB(); + 6 │ - → }·catch·(e)·{ + 7 │ - → → throw·e; + 8 │ - → } + 3 │ + → → doSomethingThatMightThrowA(); + 4 │ + → → doSomethingThatMightThrowB(); + 9 5 │ g(); + 10 6 │ } ``` ``` -invalid.js:11:2 lint/complexity/noUselessCatch FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +invalid.js:19:3 lint/complexity/noUselessCatch FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! The catch clause that only rethrows the original error is redundant. + ! The catch clause that only rethrows the original error is useless. - 9 │ doSomethingThatMightThrowB(); - 10 │ } catch (e) { - > 11 │ throw e; - │ ^^^^^^^^ - 12 │ } - 13 │ + 17 │ } + 18 │ } catch (e) { + > 19 │ throw e; + │ ^^^^^^^^ + 20 │ } + 21 │ } - i These unnecessary catch clauses can be confusing. It is recommended to remove them. + i An unnecessary catch clause can be confusing. i Unsafe fix: Remove the try/catch clause. - 4 4 │ throw e; - 5 5 │ } - 6 │ - - 7 │ - try·{ - 8 6 │ doSomethingThatMightThrowA(); - 9 7 │ doSomethingThatMightThrowB(); - 10 │ - }·catch·(e)·{ - 11 │ - → throw·e; - 12 │ - } - 13 8 │ - 14 9 │ try { + 11 11 │ + 12 12 │ for(let i = 0; i < 5; i++) { + 13 │ - → try·{ + 14 │ - → → doSomethingThatMightThrowA(); + 13 │ + → → doSomethingThatMightThrowA(); + 15 14 │ if (true) { + 16 15 │ doSomethingThatMightThrowB(); + 17 │ - → → } + 18 │ - → }·catch·(e)·{ + 19 │ - → → throw·e; + 20 │ - → } + 16 │ + → → } + 21 17 │ } + 22 18 │ ``` ``` -invalid.js:17:2 lint/complexity/noUselessCatch FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +invalid.js:26:2 lint/complexity/noUselessCatch FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! The catch clause that only rethrows the original error is redundant. + ! The catch clause that only rethrows the original error is useless. - 15 │ doSomethingThatMightThrow(); - 16 │ } catch (e) { - > 17 │ throw e; + 24 │ doSomethingThatMightThrow(); + 25 │ } catch (e) { + > 26 │ throw e; │ ^^^^^^^^ - 18 │ } finally { - 19 │ cleanUp(); + 27 │ } finally { + 28 │ cleanUp(); - i These unnecessary catch clauses can be confusing. It is recommended to remove them. + i An unnecessary catch clause can be confusing. i Unsafe fix: Remove the catch clause. - 14 14 │ try { - 15 15 │ doSomethingThatMightThrow(); - 16 │ - }·catch·(e)·{ - 17 │ - → throw·e; - 18 │ - }·finally·{ - 16 │ + }·finally·{ - 19 17 │ cleanUp(); - 20 18 │ } + 23 23 │ try { + 24 24 │ doSomethingThatMightThrow(); + 25 │ - }·catch·(e)·{ + 26 │ - → throw·e; + 27 │ - }·finally·{ + 25 │ + }·finally·{ + 28 26 │ cleanUp(); + 29 27 │ } ``` diff --git a/crates/biome_js_syntax/src/stmt_ext.rs b/crates/biome_js_syntax/src/stmt_ext.rs index 181049422c14..129ba8ee65c7 100644 --- a/crates/biome_js_syntax/src/stmt_ext.rs +++ b/crates/biome_js_syntax/src/stmt_ext.rs @@ -2,8 +2,9 @@ use crate::{ AnyJsArrayAssignmentPatternElement, AnyJsAssignmentPattern, AnyJsSwitchClause, - JsBreakStatement, JsContinueStatement, JsForVariableDeclaration, JsLabeledStatement, - JsStatementList, JsSyntaxKind, JsSyntaxToken as SyntaxToken, JsVariableDeclaration, + JsBlockStatement, JsBreakStatement, JsCatchClause, JsContinueStatement, JsFinallyClause, + JsForVariableDeclaration, JsLabeledStatement, JsStatementList, JsSyntaxKind, + JsSyntaxToken as SyntaxToken, JsTryFinallyStatement, JsTryStatement, JsVariableDeclaration, JsVariableDeclarator, TsModuleDeclaration, T, }; use biome_rowan::{declare_node_union, AstNode, SyntaxResult}; @@ -31,6 +32,40 @@ impl AnyJsSwitchClause { } } +declare_node_union! { + pub AnyJsTryStatement = JsTryStatement | JsTryFinallyStatement +} + +impl AnyJsTryStatement { + pub fn try_token(&self) -> SyntaxResult { + match self { + Self::JsTryStatement(node) => node.try_token(), + Self::JsTryFinallyStatement(node) => node.try_token(), + } + } + + pub fn body(&self) -> SyntaxResult { + match self { + Self::JsTryStatement(node) => node.body(), + Self::JsTryFinallyStatement(node) => node.body(), + } + } + + pub fn catch_clause(&self) -> Option { + match self { + Self::JsTryStatement(node) => node.catch_clause().ok(), + Self::JsTryFinallyStatement(node) => node.catch_clause(), + } + } + + pub fn finally_clause(&self) -> Option { + match self { + Self::JsTryStatement(_) => None, + Self::JsTryFinallyStatement(node) => node.finally_clause().ok(), + } + } +} + #[derive(Debug, Copy, Clone, Eq, PartialEq, Hash)] pub enum JsVariableKind { Const,