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

Commit

Permalink
improve the formatting of diagnostics and add additional test cases
Browse files Browse the repository at this point in the history
  • Loading branch information
@leops
leops committed Dec 19, 2022
1 parent baa91e8 commit d27c898
Show file tree
Hide file tree
Showing 5 changed files with 87 additions and 38 deletions.
35 changes: 26 additions & 9 deletions crates/rome_js_analyze/src/analyzers/nursery/no_unreachable_super.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,11 @@ use std::{collections::VecDeque, iter, slice};

use roaring::bitmap::RoaringBitmap;
use rome_analyze::{context::RuleContext, declare_rule, Rule, RuleDiagnostic};
use rome_console::markup;
use rome_control_flow::InstructionKind;
use rome_js_syntax::{
AnyJsClass, AnyJsExpression, JsConstructorClassMember, JsSuperExpression, JsSyntaxElement,
JsThisExpression, TextRange, WalkEvent,
JsThisExpression, JsThrowStatement, TextRange, WalkEvent,
};
use rome_rowan::AstNode;

Expand Down Expand Up @@ -235,34 +236,44 @@ impl Rule for NoUnreachableSuper {
RuleDiagnostic::new(
rule_category!(),
*this,
"`this` is accessed before `super()` is called",
markup! { "`"<Emphasis>"this"</Emphasis>"` is accessed before `"<Emphasis>"super()"</Emphasis>"` is called." },
)
.detail(super_, "`super()` is only called here"),
.detail(super_, markup! { "`"<Emphasis>"super()"</Emphasis>"` is only called here" }),
),

RuleState::DuplicateSuper { first, second } if *first == *second => Some(
RuleDiagnostic::new(
rule_category!(),
second,
markup! { "`"<Emphasis>"super()"</Emphasis>"` is called in a loop." },
),
),
RuleState::DuplicateSuper { first, second } => Some(
RuleDiagnostic::new(
rule_category!(),
second,
"`super()` is called more than once",
markup! { "`"<Emphasis>"super()"</Emphasis>"` is called more than once." },
)
.detail(first, "`super()` has already been called here"),
.detail(first, markup! { "`"<Emphasis>"super()"</Emphasis>"` has already been called here" }),
),

RuleState::ThisWithoutSuper { this } => Some(RuleDiagnostic::new(
rule_category!(),
*this,
"`this` is accessed without calling `super()` first",
markup! { "`"<Emphasis>"this"</Emphasis>"` is accessed without calling `"<Emphasis>"super()"</Emphasis>"` first." },
)),

RuleState::ReturnWithoutSuper {
return_: Some(range),
} => Some(RuleDiagnostic::new(
rule_category!(),
*range,
"this statement returns from the constructor without having called `super()` first",
markup! { "This statement returns from the constructor without having called `"<Emphasis>"super()"</Emphasis>"` first." },
)),
RuleState::ReturnWithoutSuper { return_: None } => Some(RuleDiagnostic::new(
rule_category!(),
ctx.query().node.text_trimmed_range(),
"this constructor returns without calling `super()`",
markup! { "This constructor returns without calling `"<Emphasis>"super()"</Emphasis>"`." },
)),
}
}
Expand Down Expand Up @@ -356,7 +367,13 @@ fn inspect_block(

// If the instruction is a return, store its optional text range and stop analyzing the block
InstructionKind::Return => {
has_return = Some(inst.node.as_ref().map(|node| node.text_trimmed_range()));
if let Some(node) = &inst.node {
if !JsThrowStatement::can_cast(node.kind()) {
has_return = Some(Some(node.text_trimmed_range()));
}
} else {
has_return = Some(None);
}
break;
}
}
Expand Down
28 changes: 5 additions & 23 deletions crates/rome_js_analyze/tests/specs/nursery/noUnreachableSuper/duplicateSuper.js.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ class G extends A {
```
duplicateSuper.js:16:9 lint/nursery/noUnreachableSuper ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! `super()` is called more than once
! `super()` is called more than once.
14 │ constructor() {
15 │ super(1);
Expand All @@ -96,7 +96,7 @@ duplicateSuper.js:16:9 lint/nursery/noUnreachableSuper ━━━━━━━━
```
duplicateSuper.js:27:9 lint/nursery/noUnreachableSuper ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! `super()` is called more than once
! `super()` is called more than once.
25 │ }
26 │
Expand All @@ -120,16 +120,7 @@ duplicateSuper.js:27:9 lint/nursery/noUnreachableSuper ━━━━━━━━
```
duplicateSuper.js:35:13 lint/nursery/noUnreachableSuper ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! `super()` is called more than once
33 │ constructor(cond) {
34 │ do {
> 35 │ super();
│ ^^^^^
36 │ } while (cond);
37 │ }
i `super()` has already been called here
! `super()` is called in a loop.
33 │ constructor(cond) {
34 │ do {
Expand All @@ -144,7 +135,7 @@ duplicateSuper.js:35:13 lint/nursery/noUnreachableSuper ━━━━━━━━
```
duplicateSuper.js:47:13 lint/nursery/noUnreachableSuper ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! `super()` is called more than once
! `super()` is called more than once.
45 │ }
46 │ if (condB) {
Expand All @@ -168,16 +159,7 @@ duplicateSuper.js:47:13 lint/nursery/noUnreachableSuper ━━━━━━━━
```
duplicateSuper.js:57:17 lint/nursery/noUnreachableSuper ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! `super()` is called more than once
55 │ while (condA) {
56 │ if (condB) {
> 57 │ super();
│ ^^^^^
58 │ }
59 │ }
i `super()` has already been called here
! `super()` is called in a loop.
55 │ while (condA) {
56 │ if (condB) {
Expand Down
25 changes: 25 additions & 0 deletions crates/rome_js_analyze/tests/specs/nursery/noUnreachableSuper/missingSuper.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,3 +46,28 @@ class E extends A {
super(true);
}
}

// valid
class F extends A {
constructor(variant) {
switch (variant) {
case 0:
default:
super();
break;
}
}
}

// valid
class G extends A {
constructor(cond) {
if (cond) {
super(true);
} else {
throw new Error();
}

this.field = "value";
}
}
31 changes: 28 additions & 3 deletions crates/rome_js_analyze/tests/specs/nursery/noUnreachableSuper/missingSuper.js.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,13 +53,38 @@ class E extends A {
}
}

// valid
class F extends A {
constructor(variant) {
switch (variant) {
case 0:
default:
super();
break;
}
}
}

// valid
class G extends A {
constructor(cond) {
if (cond) {
super(true);
} else {
throw new Error();
}

this.field = "value";
}
}

```

# Diagnostics
```
missingSuper.js:19:5 lint/nursery/noUnreachableSuper ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! this constructor returns without calling `super()`
! This constructor returns without calling `super()`.
17 │ // invalid
18 │ class C extends A {
Expand All @@ -79,7 +104,7 @@ missingSuper.js:19:5 lint/nursery/noUnreachableSuper ━━━━━━━━━
```
missingSuper.js:28:5 lint/nursery/noUnreachableSuper ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! this constructor returns without calling `super()`
! This constructor returns without calling `super()`.
26 │ // invalid
27 │ class D extends A {
Expand All @@ -99,7 +124,7 @@ missingSuper.js:28:5 lint/nursery/noUnreachableSuper ━━━━━━━━━
```
missingSuper.js:43:13 lint/nursery/noUnreachableSuper ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! this statement returns from the constructor without having called `super()` first
! This statement returns from the constructor without having called `super()` first.
41 │ constructor(cond) {
42 │ if (cond) {
Expand Down
6 changes: 3 additions & 3 deletions crates/rome_js_analyze/tests/specs/nursery/noUnreachableSuper/thisBeforeSuper.js.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ class F extends A {
```
thisBeforeSuper.js:25:9 lint/nursery/noUnreachableSuper ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! `this` is accessed before `super()` is called
! `this` is accessed before `super()` is called.
23 │ class D extends A {
24 │ constructor() {
Expand All @@ -87,7 +87,7 @@ thisBeforeSuper.js:25:9 lint/nursery/noUnreachableSuper ━━━━━━━━
```
thisBeforeSuper.js:33:9 lint/nursery/noUnreachableSuper ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! `this` is accessed without calling `super()` first
! `this` is accessed without calling `super()` first.
31 │ class E extends A {
32 │ constructor(cond) {
Expand All @@ -102,7 +102,7 @@ thisBeforeSuper.js:33:9 lint/nursery/noUnreachableSuper ━━━━━━━━
```
thisBeforeSuper.js:50:9 lint/nursery/noUnreachableSuper ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! `this` is accessed without calling `super()` first
! `this` is accessed without calling `super()` first.
48 │ }
49 │
Expand Down

0 comments on commit d27c898

Please sign in to comment.