Skip to content

Commit

Permalink
fix(noUselessCatch): fix code action panics
Browse files Browse the repository at this point in the history
  • Loading branch information
@Conaclos
Conaclos committed Aug 19, 2024
1 parent 3446093 commit 6c97a5f
Show file tree
Hide file tree
Showing 5 changed files with 161 additions and 160 deletions.
107 changes: 26 additions & 81 deletions crates/biome_js_analyze/src/lint/complexity/no_useless_catch.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -78,21 +75,19 @@ declare_lint_rule! {
}

impl Rule for NoUselessCatch {
type Query = Ast<AnyJsStatementAndModule>;
type Query = Ast<AnyJsTryStatement>;
type State = TextRange;
type Signals = Option<Self::State>;
type Options = ();

fn run(ctx: &RuleContext<Self>) -> 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;
}

Expand All @@ -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()
Expand All @@ -126,93 +121,43 @@ impl Rule for NoUselessCatch {
RuleDiagnostic::new(
rule_category!(),
text_range,
markup!("The "<Emphasis>"catch"</Emphasis>" clause that only rethrows the original error is redundant."),
markup!("The "<Emphasis>"catch"</Emphasis>" clause that only rethrows the original error is useless."),
)
.note(markup!(
"These unnecessary "<Emphasis>"catch"</Emphasis>" clauses can be confusing. It is recommended to remove them."
"An unnecessary "<Emphasis>"catch"</Emphasis>" clause can be confusing."
)),
)
}

fn action(ctx: &RuleContext<Self>, _: &Self::State) -> Option<JsRuleAction> {
let node = ctx.query();

let body = node.body()?;
let stmts = body.statements();
let expressions: Vec<JsExpressionStatement> = 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::<JsStatementList>()?;
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::<Vec<_>>();
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 "<Emphasis>{note}</Emphasis>" clause.").to_owned(),
mutation,
));
}
}

declare_node_union! {
pub AnyJsStatementAndModule = JsTryStatement | JsTryFinallyStatement | JsModule
}

impl AnyJsStatementAndModule {
pub fn catch_clause(&self) -> Option<JsCatchClause> {
match self {
AnyJsStatementAndModule::JsTryStatement(node) => node.catch_clause().ok(),
AnyJsStatementAndModule::JsTryFinallyStatement(node) => node.catch_clause(),
AnyJsStatementAndModule::JsModule(_) => None,
}
}

pub fn finally_clause(&self) -> Option<JsTryFinallyStatement> {
match self {
AnyJsStatementAndModule::JsTryStatement(_) => None,
AnyJsStatementAndModule::JsTryFinallyStatement(node) => Some(node.clone()),
AnyJsStatementAndModule::JsModule(_) => None,
}
}

pub fn body(&self) -> Option<JsBlockStatement> {
match self {
AnyJsStatementAndModule::JsTryStatement(node) => node.body().ok(),
AnyJsStatementAndModule::JsTryFinallyStatement(node) => node.body().ok(),
AnyJsStatementAndModule::JsModule(_) => None,
}
}

pub fn try_token(&self) -> Option<JsSyntaxToken> {
match self {
AnyJsStatementAndModule::JsTryStatement(node) => node.try_token().ok(),
AnyJsStatementAndModule::JsTryFinallyStatement(node) => node.try_token().ok(),
AnyJsStatementAndModule::JsModule(_) => None,
}
))
}
}
8 changes: 2 additions & 6 deletions crates/biome_js_analyze/src/services/control_flow/nodes/try_catch.rs
View file
Original file line number Diff line number Diff line change
@@ -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<BlockId>,
finally_block: Option<BlockId>,
Expand Down
27 changes: 18 additions & 9 deletions crates/biome_js_analyze/tests/specs/complexity/noUselessCatch/invalid.js
View file
Original file line number Diff line number Diff line change
@@ -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 {
Expand Down
140 changes: 78 additions & 62 deletions crates/biome_js_analyze/tests/specs/complexity/noUselessCatch/invalid.js.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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 │ }
```
Loading

0 comments on commit 6c97a5f

Please sign in to comment.