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

feat(rome_js_analyze): implement the noUnreachableSuper rule #4017

Merged
merged 6 commits into from
Dec 21, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 13 additions & 12 deletions crates/rome_cli/tests/commands/check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,7 @@ const JS_ERRORS_AFTER: &str = "try {
}
";

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

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

Expand Down Expand Up @@ -579,16 +577,19 @@ fn upgrade_severity() {

let messages = &console.out_buffer;

let error_count = messages
.iter()
.filter(|m| m.level == LogLevel::Error)
.filter(|m| {
let content = format!("{:?}", m.content);
content.contains("style/noNegationElse")
})
.count();

assert_eq!(
messages
.iter()
.filter(|m| m.level == LogLevel::Error)
.filter(|m| {
let content = format!("{:?}", m.content);
content.contains("nursery/noInvalidConstructorSuper")
})
.count(),
1
error_count, 1,
"expected 1 error-level message in console buffer, found {error_count:?}:\n{:?}",
console.out_buffer
);

assert_cli_snapshot(SnapshotPayload::new(
Expand Down
4 changes: 2 additions & 2 deletions crates/rome_cli/tests/configs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,8 +145,8 @@ pub const CONFIG_LINTER_UPGRADE_DIAGNOSTIC: &str = r#"{
"linter": {
"rules": {
"recommended": true,
"nursery": {
"noInvalidConstructorSuper": "error"
"style": {
"noNegationElse": "error"
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ expression: content
"linter": {
"rules": {
"recommended": true,
"nursery": {
"noInvalidConstructorSuper": "error"
"style": {
"noNegationElse": "error"
}
}
}
Expand All @@ -20,9 +20,7 @@ expression: content
## `file.js`

```js
class A extends B {
constructor() {}
}
if(!cond) { exprA(); } else { exprB() }
```

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

```block
file.js:1:9 lint/nursery/noInvalidConstructorSuper ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
file.js:1:1 lint/style/noNegationElse FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

× This class extends another class and a super() call is expected.
× Invert blocks when performing a negation test.

> 1 │ if(!cond) { exprA(); } else { exprB() }
│ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

i Suggested fix: Exchange alternate and consequent of the node

> 1 │ class A extends B {
│ ^^^^^^^^^
2 │ constructor() {}
3 │ }
- if(!cond)·{·exprA();·}·else·{·exprB()·}
+ if(cond)·{·exprB()·}·else·{·exprA();·}


```
Expand Down
16 changes: 9 additions & 7 deletions crates/rome_control_flow/src/builder.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use rome_rowan::{Language, SyntaxElement};
use rome_rowan::{Language, SyntaxElement, SyntaxNode};

use crate::{
BasicBlock, ControlFlowGraph, ExceptionHandler, ExceptionHandlerKind, Instruction,
Expand Down Expand Up @@ -27,19 +27,21 @@ pub struct FunctionBuilder<L: Language> {
block_cursor: BlockId,
}

impl<L: Language> Default for FunctionBuilder<L> {
fn default() -> Self {
impl<L: Language> FunctionBuilder<L> {
/// Create a new [FunctionBuilder] instance from a function node
pub fn new(node: SyntaxNode<L>) -> Self {
Self {
result: ControlFlowGraph::new(),
result: ControlFlowGraph::new(node),
exception_target: Vec::new(),
block_cursor: BlockId { index: 0 },
}
}
}

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

Expand Down
7 changes: 5 additions & 2 deletions crates/rome_control_flow/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use std::{
fmt::{self, Display, Formatter},
};

use rome_rowan::{Language, SyntaxElement};
use rome_rowan::{Language, SyntaxElement, SyntaxNode};

pub mod builder;

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

impl<L: Language> ControlFlowGraph<L> {
fn new() -> Self {
fn new(node: SyntaxNode<L>) -> Self {
ControlFlowGraph {
blocks: vec![BasicBlock::new(None, None)],
node,
}
}
}
Expand Down
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 @@ -66,6 +66,7 @@ define_dategories! {
"lint/nursery/noSelfCompare": "https://docs.rome.tools/lint/rules/noSelfCompare",
"lint/nursery/noSetterReturn": "https://docs.rome.tools/lint/rules/noSetterReturn",
"lint/nursery/noStringCaseMismatch": "https://docs.rome.tools/lint/rules/noStringCaseMismatch",
"lint/nursery/noUnreachableSuper": "https://rome.tools/docs/lint/rules/noUnreachableSuper",
"lint/nursery/noUnsafeFinally": "https://docs.rome.tools/lint/rules/noUnsafeFinally",
"lint/nursery/noUselessSwitchCase": "https://docs.rome.tools/lint/rules/noUselessSwitchCase",
"lint/nursery/noVar": "https://docs.rome.tools/lint/rules/noVar",
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.

Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,15 @@ declare_rule! {
/// ### Invalid
///
/// ```js,expect_diagnostic
/// class A extends B {
/// constructor() {}
/// class A {
/// constructor() {
/// super();
/// }
/// }
/// ```
///
/// ```js,expect_diagnostic
/// class A {
/// class A extends undefined {
/// constructor() {
/// super();
/// }
Expand Down Expand Up @@ -52,7 +54,6 @@ declare_rule! {
}

pub(crate) enum NoInvalidConstructorSuperState {
MissingSuper(TextRange),
UnexpectedSuper(TextRange),
BadExtends {
extends_range: TextRange,
Expand All @@ -63,17 +64,13 @@ pub(crate) enum NoInvalidConstructorSuperState {
impl NoInvalidConstructorSuperState {
fn range(&self) -> &TextRange {
match self {
NoInvalidConstructorSuperState::MissingSuper(range) => range,
NoInvalidConstructorSuperState::UnexpectedSuper(range) => range,
NoInvalidConstructorSuperState::BadExtends { super_range, .. } => super_range,
}
}

fn message(&self) -> MarkupBuf {
match self {
NoInvalidConstructorSuperState::MissingSuper(_) => {
(markup! { "This class extends another class and a "<Emphasis>"super()"</Emphasis>" call is expected." }).to_owned()
}
NoInvalidConstructorSuperState::UnexpectedSuper(_) => {
(markup! { "This class should not have a "<Emphasis>"super()"</Emphasis>" call. You should remove it." }).to_owned()
}
Expand Down Expand Up @@ -136,16 +133,6 @@ impl Rule for NoInvalidConstructorSuper {
None
}
}
(None, Some(extends_clause)) => {
let super_class = extends_clause.super_class().ok()?;
if !matches!(super_class, AnyJsExpression::AnyJsLiteralExpression(_,)) {
Some(NoInvalidConstructorSuperState::MissingSuper(
extends_clause.syntax().text_trimmed_range(),
))
} else {
None
}
}
(Some(super_expression), None) => Some(
NoInvalidConstructorSuperState::UnexpectedSuper(super_expression.range()),
),
Expand Down
Loading