From 9977d00961cc62491416beb03f1b30bb5ff91d47 Mon Sep 17 00:00:00 2001 From: l3ops Date: Tue, 20 Dec 2022 16:48:48 +0100 Subject: [PATCH] improve the diagnostics --- .../analyzers/nursery/no_unreachable_super.rs | 69 ++++++---- .../noUnreachableSuper/duplicateSuper.js.snap | 125 ++++++++++++++---- .../noUnreachableSuper/missingSuper.js.snap | 28 +++- .../thisBeforeSuper.js.snap | 61 ++++++++- 4 files changed, 216 insertions(+), 67 deletions(-) diff --git a/crates/rome_js_analyze/src/analyzers/nursery/no_unreachable_super.rs b/crates/rome_js_analyze/src/analyzers/nursery/no_unreachable_super.rs index 3464ec35293f..e14458569d0c 100644 --- a/crates/rome_js_analyze/src/analyzers/nursery/no_unreachable_super.rs +++ b/crates/rome_js_analyze/src/analyzers/nursery/no_unreachable_super.rs @@ -235,46 +235,59 @@ impl Rule for NoUnreachableSuper { RuleState::ThisBeforeSuper { this, super_ } => Some( RuleDiagnostic::new( rule_category!(), - *this, - markup! { "`""this""` is accessed before `""super()""` is called." }, + ctx.query().node.text_trimmed_range(), + markup! { "This constructor has code paths accessing `""this""` before `""super()""` is called." }, ) - .detail(super_, markup! { "`""super()""` is only called here" }), + .detail(this, markup! { "`""this""` is accessed here:" }) + .detail(super_, markup! { "`""super()""` is only called here:" }) + .note("If this is intentional, add an explicit throw statement in unsupported paths."), ), - + + RuleState::ThisWithoutSuper { this } => Some( + RuleDiagnostic::new( + rule_category!(), + ctx.query().node.text_trimmed_range(), + markup! { "This constructor has code paths accessing `""this""` without calling `""super()""` first." }, + ) + .detail(this, markup! { "`""this""` is accessed here:" }) + .note("If this is intentional, add an explicit throw statement in unsupported paths."), + ), + RuleState::DuplicateSuper { first, second } if *first == *second => Some( RuleDiagnostic::new( rule_category!(), - second, - markup! { "`""super()""` is called in a loop." }, - ), + ctx.query().node.text_trimmed_range(), + markup! { "This constructor calls `""super()""` in a loop." }, + ) + .detail(first, markup! { "`""super()""` is called here:" }), ), RuleState::DuplicateSuper { first, second } => Some( RuleDiagnostic::new( rule_category!(), - second, - markup! { "`""super()""` is called more than once." }, + ctx.query().node.text_trimmed_range(), + markup! { "This constructor has code paths where `""super()""` is called more than once." }, ) - .detail(first, markup! { "`""super()""` has already been called here" }), + .detail(first, markup! { "`""super()""` is first called here:" }) + .detail(second, markup! { "`""super()""` is then called again here:" }), ), - RuleState::ThisWithoutSuper { this } => Some(RuleDiagnostic::new( - rule_category!(), - *this, - markup! { "`""this""` is accessed without calling `""super()""` first." }, - )), - - RuleState::ReturnWithoutSuper { - return_: Some(range), - } => Some(RuleDiagnostic::new( - rule_category!(), - *range, - markup! { "This statement returns from the constructor without having called `""super()""` first." }, - )), - RuleState::ReturnWithoutSuper { return_: None } => Some(RuleDiagnostic::new( - rule_category!(), - ctx.query().node.text_trimmed_range(), - markup! { "This constructor returns without calling `""super()""`." }, - )), + RuleState::ReturnWithoutSuper { return_: Some(range) } => Some( + RuleDiagnostic::new( + rule_category!(), + ctx.query().node.text_trimmed_range(), + markup! { "This constructor has code paths that return without calling `""super()""` first." }, + ) + .detail(range, markup! { "This statement returns from the constructor before `""super()""` has been called:" }) + .note("If this is intentional, add an explicit throw statement in unsupported paths."), + ), + RuleState::ReturnWithoutSuper { return_: None } => Some( + RuleDiagnostic::new( + rule_category!(), + ctx.query().node.text_trimmed_range(), + markup! { "This constructor has code paths that return without calling `""super()""`." }, + ) + .note("If this is intentional, add an explicit throw statement in unsupported paths."), + ), } } } diff --git a/crates/rome_js_analyze/tests/specs/nursery/noUnreachableSuper/duplicateSuper.js.snap b/crates/rome_js_analyze/tests/specs/nursery/noUnreachableSuper/duplicateSuper.js.snap index 91e924eab70b..871f031f6d06 100644 --- a/crates/rome_js_analyze/tests/specs/nursery/noUnreachableSuper/duplicateSuper.js.snap +++ b/crates/rome_js_analyze/tests/specs/nursery/noUnreachableSuper/duplicateSuper.js.snap @@ -70,18 +70,22 @@ class G extends A { # Diagnostics ``` -duplicateSuper.js:16:9 lint/nursery/noUnreachableSuper ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +duplicateSuper.js:14:5 lint/nursery/noUnreachableSuper ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! `super()` is called more than once. + ! This constructor has code paths where `super()` is called more than once. - 14 │ constructor() { - 15 │ super(1); + 12 │ // invalid + 13 │ class C extends A { + > 14 │ constructor() { + │ ^^^^^^^^^^^^^^^ + > 15 │ super(1); > 16 │ super(2); - │ ^^^^^ - 17 │ } + > 17 │ } + │ ^ 18 │ } + 19 │ - i `super()` has already been called here + i `super()` is first called here: 13 │ class C extends A { 14 │ constructor() { @@ -90,22 +94,36 @@ duplicateSuper.js:16:9 lint/nursery/noUnreachableSuper ━━━━━━━━ 16 │ super(2); 17 │ } + i `super()` is then called again here: + + 14 │ constructor() { + 15 │ super(1); + > 16 │ super(2); + │ ^^^^^ + 17 │ } + 18 │ } + ``` ``` -duplicateSuper.js:27:9 lint/nursery/noUnreachableSuper ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +duplicateSuper.js:22:5 lint/nursery/noUnreachableSuper ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! `super()` is called more than once. + ! This constructor has code paths where `super()` is called more than once. - 25 │ } - 26 │ + 20 │ // invalid + 21 │ class D extends A { + > 22 │ constructor(cond) { + │ ^^^^^^^^^^^^^^^^^^^ + > 23 │ if (cond) { + ... > 27 │ super(); - │ ^^^^^ - 28 │ } + > 28 │ } + │ ^ 29 │ } + 30 │ - i `super()` has already been called here + i `super()` is first called here: 22 │ constructor(cond) { 23 │ if (cond) { @@ -114,13 +132,36 @@ duplicateSuper.js:27:9 lint/nursery/noUnreachableSuper ━━━━━━━━ 25 │ } 26 │ + i `super()` is then called again here: + + 25 │ } + 26 │ + > 27 │ super(); + │ ^^^^^ + 28 │ } + 29 │ } + ``` ``` -duplicateSuper.js:35:13 lint/nursery/noUnreachableSuper ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +duplicateSuper.js:33:5 lint/nursery/noUnreachableSuper ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! `super()` is called in a loop. + ! This constructor calls `super()` in a loop. + + 31 │ // invalid + 32 │ class E extends A { + > 33 │ constructor(cond) { + │ ^^^^^^^^^^^^^^^^^^^ + > 34 │ do { + > 35 │ super(); + > 36 │ } while (cond); + > 37 │ } + │ ^ + 38 │ } + 39 │ + + i `super()` is called here: 33 │ constructor(cond) { 34 │ do { @@ -133,18 +174,23 @@ duplicateSuper.js:35:13 lint/nursery/noUnreachableSuper ━━━━━━━━ ``` ``` -duplicateSuper.js:47:13 lint/nursery/noUnreachableSuper ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +duplicateSuper.js:42:5 lint/nursery/noUnreachableSuper ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! `super()` is called more than once. + ! This constructor has code paths where `super()` is called more than once. - 45 │ } - 46 │ if (condB) { - > 47 │ super(true); - │ ^^^^^ - 48 │ } - 49 │ } + 40 │ // invalid + 41 │ class F extends A { + > 42 │ constructor(condA, condB) { + │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + > 43 │ if (condA) { + ... + > 48 │ } + > 49 │ } + │ ^ + 50 │ } + 51 │ - i `super()` has already been called here + i `super()` is first called here: 42 │ constructor(condA, condB) { 43 │ if (condA) { @@ -153,13 +199,36 @@ duplicateSuper.js:47:13 lint/nursery/noUnreachableSuper ━━━━━━━━ 45 │ } 46 │ if (condB) { + i `super()` is then called again here: + + 45 │ } + 46 │ if (condB) { + > 47 │ super(true); + │ ^^^^^ + 48 │ } + 49 │ } + ``` ``` -duplicateSuper.js:57:17 lint/nursery/noUnreachableSuper ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - - ! `super()` is called in a loop. +duplicateSuper.js:54:5 lint/nursery/noUnreachableSuper ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! This constructor calls `super()` in a loop. + + 52 │ // invalid + 53 │ class G extends A { + > 54 │ constructor(condA, condB) { + │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + > 55 │ while (condA) { + ... + > 59 │ } + > 60 │ } + │ ^ + 61 │ } + 62 │ + + i `super()` is called here: 55 │ while (condA) { 56 │ if (condB) { diff --git a/crates/rome_js_analyze/tests/specs/nursery/noUnreachableSuper/missingSuper.js.snap b/crates/rome_js_analyze/tests/specs/nursery/noUnreachableSuper/missingSuper.js.snap index 3cdabb99c171..c619bac8d870 100644 --- a/crates/rome_js_analyze/tests/specs/nursery/noUnreachableSuper/missingSuper.js.snap +++ b/crates/rome_js_analyze/tests/specs/nursery/noUnreachableSuper/missingSuper.js.snap @@ -84,7 +84,7 @@ class G extends A { ``` missingSuper.js:19:5 lint/nursery/noUnreachableSuper ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! This constructor returns without calling `super()`. + ! This constructor has code paths that return without calling `super()`. 17 │ // invalid 18 │ class C extends A { @@ -98,13 +98,15 @@ missingSuper.js:19:5 lint/nursery/noUnreachableSuper ━━━━━━━━━ 24 │ } 25 │ + i If this is intentional, add an explicit throw statement in unsupported paths. + ``` ``` missingSuper.js:28:5 lint/nursery/noUnreachableSuper ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! This constructor returns without calling `super()`. + ! This constructor has code paths that return without calling `super()`. 26 │ // invalid 27 │ class D extends A { @@ -118,13 +120,29 @@ missingSuper.js:28:5 lint/nursery/noUnreachableSuper ━━━━━━━━━ 37 │ } 38 │ + i If this is intentional, add an explicit throw statement in unsupported paths. + ``` ``` -missingSuper.js:43:13 lint/nursery/noUnreachableSuper ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +missingSuper.js:41:5 lint/nursery/noUnreachableSuper ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! This statement returns from the constructor without having called `super()` first. + ! This constructor has code paths that return without calling `super()` first. + + 39 │ // invalid + 40 │ class E extends A { + > 41 │ constructor(cond) { + │ ^^^^^^^^^^^^^^^^^^^ + > 42 │ if (cond) { + ... + > 46 │ super(true); + > 47 │ } + │ ^ + 48 │ } + 49 │ + + i This statement returns from the constructor before `super()` has been called: 41 │ constructor(cond) { 42 │ if (cond) { @@ -133,6 +151,8 @@ missingSuper.js:43:13 lint/nursery/noUnreachableSuper ━━━━━━━━ 44 │ } 45 │ + i If this is intentional, add an explicit throw statement in unsupported paths. + ``` diff --git a/crates/rome_js_analyze/tests/specs/nursery/noUnreachableSuper/thisBeforeSuper.js.snap b/crates/rome_js_analyze/tests/specs/nursery/noUnreachableSuper/thisBeforeSuper.js.snap index ca90b4e1ca36..adac8f29a40b 100644 --- a/crates/rome_js_analyze/tests/specs/nursery/noUnreachableSuper/thisBeforeSuper.js.snap +++ b/crates/rome_js_analyze/tests/specs/nursery/noUnreachableSuper/thisBeforeSuper.js.snap @@ -61,9 +61,22 @@ class F extends A { # Diagnostics ``` -thisBeforeSuper.js:25:9 lint/nursery/noUnreachableSuper ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +thisBeforeSuper.js:24:5 lint/nursery/noUnreachableSuper ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! `this` is accessed before `super()` is called. + ! This constructor has code paths accessing `this` before `super()` is called. + + 22 │ // invalid + 23 │ class D extends A { + > 24 │ constructor() { + │ ^^^^^^^^^^^^^^^ + > 25 │ this.field = "value"; + > 26 │ super(); + > 27 │ } + │ ^ + 28 │ } + 29 │ + + i `this` is accessed here: 23 │ class D extends A { 24 │ constructor() { @@ -72,7 +85,7 @@ thisBeforeSuper.js:25:9 lint/nursery/noUnreachableSuper ━━━━━━━━ 26 │ super(); 27 │ } - i `super()` is only called here + i `super()` is only called here: 24 │ constructor() { 25 │ this.field = "value"; @@ -81,13 +94,29 @@ thisBeforeSuper.js:25:9 lint/nursery/noUnreachableSuper ━━━━━━━━ 27 │ } 28 │ } + i If this is intentional, add an explicit throw statement in unsupported paths. + ``` ``` -thisBeforeSuper.js:33:9 lint/nursery/noUnreachableSuper ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +thisBeforeSuper.js:32:5 lint/nursery/noUnreachableSuper ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! `this` is accessed without calling `super()` first. + ! This constructor has code paths accessing `this` without calling `super()` first. + + 30 │ // invalid + 31 │ class E extends A { + > 32 │ constructor(cond) { + │ ^^^^^^^^^^^^^^^^^^^ + > 33 │ this.field = "value"; + ... + > 39 │ } + > 40 │ } + │ ^ + 41 │ } + 42 │ + + i `this` is accessed here: 31 │ class E extends A { 32 │ constructor(cond) { @@ -96,13 +125,29 @@ thisBeforeSuper.js:33:9 lint/nursery/noUnreachableSuper ━━━━━━━━ 34 │ 35 │ if (cond) { + i If this is intentional, add an explicit throw statement in unsupported paths. + ``` ``` -thisBeforeSuper.js:50:9 lint/nursery/noUnreachableSuper ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +thisBeforeSuper.js:45:5 lint/nursery/noUnreachableSuper ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - ! `this` is accessed without calling `super()` first. + ! This constructor has code paths accessing `this` without calling `super()` first. + + 43 │ // invalid + 44 │ class F extends A { + > 45 │ constructor(cond) { + │ ^^^^^^^^^^^^^^^^^^^ + > 46 │ if (cond) { + ... + > 50 │ this.field = "value"; + > 51 │ } + │ ^ + 52 │ } + 53 │ + + i `this` is accessed here: 48 │ } 49 │ @@ -111,6 +156,8 @@ thisBeforeSuper.js:50:9 lint/nursery/noUnreachableSuper ━━━━━━━━ 51 │ } 52 │ } + i If this is intentional, add an explicit throw statement in unsupported paths. + ```