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

Commit a69f962

Browse files
leopsleops
authored
feat(rome_js_analyze): implement the noUnreachableSuper rule (#4017)
* feat(rome_js_analyze): implement the noUnreachableSuper rule * address PR review and fix tests * run lintdoc * improve the formatting of diagnostics and add additional test cases * improve the diagnostics * run lintdoc
1 parent bd4c3e4 commit a69f962

File tree

28 files changed

+1541
-159
lines changed

28 files changed

+1541
-159
lines changed

‎crates/rome_cli/tests/commands/check.rs‎

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -65,9 +65,7 @@ const JS_ERRORS_AFTER: &str = "try {
6565
}
6666
";
6767

68-
const UPGRADE_SEVERITY_CODE: &str = r#"class A extends B {
69-
constructor() {}
70-
}"#;
68+
const UPGRADE_SEVERITY_CODE: &str = r#"if(!cond) { exprA(); } else { exprB() }"#;
7169

7270
const NURSERY_UNSTABLE: &str = r#"if(a = b) {}"#;
7371

@@ -579,16 +577,19 @@ fn upgrade_severity() {
579577

580578
let messages = &console.out_buffer;
581579

580+
let error_count = messages
581+
.iter()
582+
.filter(|m| m.level == LogLevel::Error)
583+
.filter(|m| {
584+
let content = format!("{:?}", m.content);
585+
content.contains("style/noNegationElse")
586+
})
587+
.count();
588+
582589
assert_eq!(
583-
messages
584-
.iter()
585-
.filter(|m| m.level == LogLevel::Error)
586-
.filter(|m| {
587-
let content = format!("{:?}", m.content);
588-
content.contains("nursery/noInvalidConstructorSuper")
589-
})
590-
.count(),
591-
1
590+
error_count, 1,
591+
"expected 1 error-level message in console buffer, found {error_count:?}:\n{:?}",
592+
console.out_buffer
592593
);
593594

594595
assert_cli_snapshot(SnapshotPayload::new(

‎crates/rome_cli/tests/configs.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -145,8 +145,8 @@ pub const CONFIG_LINTER_UPGRADE_DIAGNOSTIC: &str = r#"{
145145
"linter": {
146146
"rules": {
147147
"recommended": true,
148-
"nursery": {
149-
"noInvalidConstructorSuper": "error"
148+
"style": {
149+
"noNegationElse": "error"
150150
}
151151
}
152152
}

‎crates/rome_cli/tests/snapshots/main_commands_check/upgrade_severity.snap‎

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,8 @@ expression: content
99
"linter": {
1010
"rules": {
1111
"recommended": true,
12-
"nursery": {
13-
"noInvalidConstructorSuper": "error"
12+
"style": {
13+
"noNegationElse": "error"
1414
}
1515
}
1616
}
@@ -20,9 +20,7 @@ expression: content
2020
## `file.js`
2121

2222
```js
23-
class A extends B {
24-
constructor() {}
25-
}
23+
if(!cond) { exprA(); } else { exprB() }
2624
```
2725

2826
# Termination Message
@@ -34,14 +32,17 @@ some errors were emitted while running checks
3432
# Emitted Messages
3533

3634
```block
37-
file.js:1:9 lint/nursery/noInvalidConstructorSuper ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
35+
file.js:1:1 lint/style/noNegationElse FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
3836
39-
× This class extends another class and a super() call is expected.
37+
× Invert blocks when performing a negation test.
38+
39+
> 1 │ if(!cond) { exprA(); } else { exprB() }
40+
│ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
41+
42+
i Suggested fix: Exchange alternate and consequent of the node
4043
41-
> 1 │ class A extends B {
42-
│ ^^^^^^^^^
43-
2 │ constructor() {}
44-
3 │ }
44+
- if(!cond)·{·exprA();·}·else·{·exprB()·}
45+
+ if(cond)·{·exprB()·}·else·{·exprA();·}
4546
4647
4748
```

‎crates/rome_control_flow/src/builder.rs‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
use rome_rowan::{Language, SyntaxElement};
1+
use rome_rowan::{Language, SyntaxElement, SyntaxNode};
22

33
use crate::{
44
BasicBlock, ControlFlowGraph, ExceptionHandler, ExceptionHandlerKind, Instruction,
@@ -27,19 +27,21 @@ pub struct FunctionBuilder<L: Language> {
2727
block_cursor: BlockId,
2828
}
2929

30-
impl<L: Language> Default for FunctionBuilder<L> {
31-
fn default() -> Self {
30+
impl<L: Language> FunctionBuilder<L> {
31+
/// Create a new [FunctionBuilder] instance from a function node
32+
pub fn new(node: SyntaxNode<L>) -> Self {
3233
Self {
33-
result: ControlFlowGraph::new(),
34+
result: ControlFlowGraph::new(node),
3435
exception_target: Vec::new(),
3536
block_cursor: BlockId { index: 0 },
3637
}
3738
}
38-
}
3939

40-
impl<L: Language> FunctionBuilder<L> {
4140
/// Finishes building the function
42-
pub fn finish(self) -> ControlFlowGraph<L> {
41+
pub fn finish(mut self) -> ControlFlowGraph<L> {
42+
// Append the implicit return instruction that resumes execution of the
43+
// parent procedure when control flow reaches the end of a function
44+
self.append_return();
4345
self.result
4446
}
4547

‎crates/rome_control_flow/src/lib.rs‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ use std::{
55
fmt::{self, Display, Formatter},
66
};
77

8-
use rome_rowan::{Language, SyntaxElement};
8+
use rome_rowan::{Language, SyntaxElement, SyntaxNode};
99

1010
pub mod builder;
1111

@@ -18,12 +18,15 @@ use crate::builder::BlockId;
1818
pub struct ControlFlowGraph<L: Language> {
1919
/// List of blocks that make up this function
2020
pub blocks: Vec<BasicBlock<L>>,
21+
/// The function node this CFG was built for in the syntax tree
22+
pub node: SyntaxNode<L>,
2123
}
2224

2325
impl<L: Language> ControlFlowGraph<L> {
24-
fn new() -> Self {
26+
fn new(node: SyntaxNode<L>) -> Self {
2527
ControlFlowGraph {
2628
blocks: vec![BasicBlock::new(None, None)],
29+
node,
2730
}
2831
}
2932
}

‎crates/rome_diagnostics_categories/src/categories.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,7 @@ define_dategories! {
6666
"lint/nursery/noSelfCompare": "https://docs.rome.tools/lint/rules/noSelfCompare",
6767
"lint/nursery/noSetterReturn": "https://docs.rome.tools/lint/rules/noSetterReturn",
6868
"lint/nursery/noStringCaseMismatch": "https://docs.rome.tools/lint/rules/noStringCaseMismatch",
69+
"lint/nursery/noUnreachableSuper": "https://rome.tools/docs/lint/rules/noUnreachableSuper",
6970
"lint/nursery/noUnsafeFinally": "https://docs.rome.tools/lint/rules/noUnsafeFinally",
7071
"lint/nursery/noUselessSwitchCase": "https://docs.rome.tools/lint/rules/noUselessSwitchCase",
7172
"lint/nursery/noVar": "https://docs.rome.tools/lint/rules/noVar",

‎crates/rome_js_analyze/src/analyzers/nursery.rs‎

Lines changed: 2 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎crates/rome_js_analyze/src/analyzers/nursery/no_invalid_constructor_super.rs‎

Lines changed: 5 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -15,13 +15,15 @@ declare_rule! {
1515
/// ### Invalid
1616
///
1717
/// ```js,expect_diagnostic
18-
/// class A extends B {
19-
/// constructor() {}
18+
/// class A {
19+
/// constructor() {
20+
/// super();
21+
/// }
2022
/// }
2123
/// ```
2224
///
2325
/// ```js,expect_diagnostic
24-
/// class A {
26+
/// class A extends undefined {
2527
/// constructor() {
2628
/// super();
2729
/// }
@@ -52,7 +54,6 @@ declare_rule! {
5254
}
5355

5456
pub(crate) enum NoInvalidConstructorSuperState {
55-
MissingSuper(TextRange),
5657
UnexpectedSuper(TextRange),
5758
BadExtends {
5859
extends_range: TextRange,
@@ -63,17 +64,13 @@ pub(crate) enum NoInvalidConstructorSuperState {
6364
impl NoInvalidConstructorSuperState {
6465
fn range(&self) -> &TextRange {
6566
match self {
66-
NoInvalidConstructorSuperState::MissingSuper(range) => range,
6767
NoInvalidConstructorSuperState::UnexpectedSuper(range) => range,
6868
NoInvalidConstructorSuperState::BadExtends { super_range, .. } => super_range,
6969
}
7070
}
7171

7272
fn message(&self) -> MarkupBuf {
7373
match self {
74-
NoInvalidConstructorSuperState::MissingSuper(_) => {
75-
(markup! { "This class extends another class and a "<Emphasis>"super()"</Emphasis>" call is expected." }).to_owned()
76-
}
7774
NoInvalidConstructorSuperState::UnexpectedSuper(_) => {
7875
(markup! { "This class should not have a "<Emphasis>"super()"</Emphasis>" call. You should remove it." }).to_owned()
7976
}
@@ -136,16 +133,6 @@ impl Rule for NoInvalidConstructorSuper {
136133
None
137134
}
138135
}
139-
(None, Some(extends_clause)) => {
140-
let super_class = extends_clause.super_class().ok()?;
141-
if !matches!(super_class, AnyJsExpression::AnyJsLiteralExpression(_,)) {
142-
Some(NoInvalidConstructorSuperState::MissingSuper(
143-
extends_clause.syntax().text_trimmed_range(),
144-
))
145-
} else {
146-
None
147-
}
148-
}
149136
(Some(super_expression), None) => Some(
150137
NoInvalidConstructorSuperState::UnexpectedSuper(super_expression.range()),
151138
),

0 commit comments

Comments
 (0)