Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

Commit

Permalink
feat(rome_js_analyze): noUselessCatch (#4236)
Browse files Browse the repository at this point in the history
* feat(rome_js_analyze): noUselessCatch

* Update crates/rome_js_analyze/src/analyzers/nursery/no_useless_catch.rs

Co-authored-by: Victorien ELVINGER <victorien@elvinger.fr>

* Apply suggestions from code review

Co-authored-by: Victorien ELVINGER <victorien@elvinger.fr>

* test: update the test snapshot & rule docs

* fix(rome_js_analyzers): use ? instead of unwrap.

* perf: added some annotations

* perf: update annotation

* fix: resolve the conflicts due to rebase

---------

Co-authored-by: Victorien ELVINGER <victorien@elvinger.fr>
  • Loading branch information
GiveMe-A-Name and Conaclos committed Feb 26, 2023
1 parent 996775c commit 979d302
Show file tree
Hide file tree
Showing 14 changed files with 419 additions and 52 deletions.
1 change: 1 addition & 0 deletions crates/rome_diagnostics_categories/src/categories.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ define_dategories! {
"lint/nursery/noGlobalObjectCalls": "https://docs.rome.tools/lint/rules/noGlobalObjectCalls",
"lint/nursery/noPrototypeBuiltins": "https://docs.rome.tools/lint/rules/noPrototypeBuiltins",
"lint/nursery/noSvgWithoutTitle": "https://docs.rome.tools/lint/rules/noSvgWithoutTitle",
"lint/nursery/noUselessCatch": "https://docs.rome.tools/lint/rules/noUselessCatch",
// Insert new nursery rule here

// performance
Expand Down
3 changes: 2 additions & 1 deletion crates/rome_js_analyze/src/analyzers/nursery.rs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

116 changes: 116 additions & 0 deletions crates/rome_js_analyze/src/analyzers/nursery/no_useless_catch.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
use rome_analyze::{context::RuleContext, declare_rule, Ast, Rule, RuleDiagnostic};
use rome_console::markup;
use rome_js_syntax::{JsCatchClause, TextRange};
use rome_rowan::{AstNode, AstNodeList};

declare_rule! {
/// Disallow unnecessary `catch` clauses.
///
/// A `catch` clause that only rethrows the original error is redundant,
/// and has no effect on the runtime behavior of the program.
/// These redundant clauses can be a source of confusion and code bloat,
/// so it’s better to disallow these unnecessary `catch` clauses.
///
/// Source: https://eslint.org/docs/latest/rules/no-useless-catch
///
/// ## Examples
///
/// ### Invalid
///
/// ```js,expect_diagnostic
/// try {
/// doSomething();
/// } catch(e) {
/// throw e;
/// }
/// ```
/// ```js,expect_diagnostic
/// try {
/// doSomething();
/// } catch(e) {
/// throw e;
/// } finally {
/// doCleanUp();
/// }
/// ```
/// ## Valid
///
/// ```js
/// try {
/// doSomething();
/// } catch(e) {
/// doSomethingWhenCatch();
/// throw e;
/// }
/// ```
///
/// ```js
/// try {
/// doSomething();
/// } catch(e) {
/// handleError(e);
/// }
/// ```
///
pub(crate) NoUselessCatch {
version: "next",
name: "noUselessCatch",
recommended: true,
}
}

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

fn run(ctx: &RuleContext<Self>) -> Self::Signals {
let binding = ctx.query();

let catch_body = binding.body().ok()?;
let body_statements = catch_body.statements();

// We need guarantees that body_statements has only one `throw` statement.
if body_statements.len() != 1 {
return None;
}

let catch_declaration = binding.declaration()?;
let catch_binding_err = catch_declaration
.binding()
.ok()?
.as_any_js_binding()?
.as_js_identifier_binding()?
.name_token()
.ok()?;
let catch_err_name = catch_binding_err.text();

let first_statement = body_statements.first()?;
let js_throw_statement = first_statement.as_js_throw_statement()?;
let throw_ident = js_throw_statement
.argument()
.ok()?
.as_js_identifier_expression()?
.text();

if throw_ident.eq(catch_err_name) {
Some(js_throw_statement.syntax().text_trimmed_range())
} else {
None
}
}

fn diagnostic(_: &RuleContext<Self>, text_range: &Self::State) -> Option<RuleDiagnostic> {
Some(
RuleDiagnostic::new(
rule_category!(),
text_range,
markup!("The "<Emphasis>"catch"</Emphasis>" clause that only rethrows the original error is redundant."),
)
.note(markup!(
"These unnecessary "<Emphasis>"catch"</Emphasis>" clauses can be confusing. It is recommended to remove them."
)),
)
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
try {
doSomethingThatMightThrow();
} catch (e) {
throw e;
}

try {
doSomethingThatMightThrow();
} catch (e) {
throw e;
} finally {
cleanUp();
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
---
source: crates/rome_js_analyze/tests/spec_tests.rs
expression: invalid.js
---
# Input
```js
try {
doSomethingThatMightThrow();
} catch (e) {
throw e;
}

try {
doSomethingThatMightThrow();
} catch (e) {
throw e;
} finally {
cleanUp();
}

```

# Diagnostics
```
invalid.js:4:2 lint/nursery/noUselessCatch ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! The catch clause that only rethrows the original error is redundant.
2 │ doSomethingThatMightThrow();
3 │ } catch (e) {
> 4 │ throw e;
│ ^^^^^^^^
5 │ }
6 │
i These unnecessary catch clauses can be confusing. It is recommended to remove them.
```

```
invalid.js:10:2 lint/nursery/noUselessCatch ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! The catch clause that only rethrows the original error is redundant.
8 │ doSomethingThatMightThrow();
9 │ } catch (e) {
> 10 │ throw e;
│ ^^^^^^^^
11 │ } finally {
12 │ cleanUp();
i These unnecessary catch clauses can be confusing. It is recommended to remove them.
```


Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
/* should not generate diagnostics */

try {
doSomethingThatMightThrow();
} catch (e) {
doSomethingBeforeRethrow();
throw e;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
---
source: crates/rome_js_analyze/tests/spec_tests.rs
expression: valid.js
---
# Input
```js
/* should not generate diagnostics */

try {
doSomethingThatMightThrow();
} catch (e) {
doSomethingBeforeRethrow();
throw e;
}

```


Loading

0 comments on commit 979d302

Please sign in to comment.