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

fix(rome_js_analyze): only emit control flow side-effects for variable declarators with an initializer #2870

Merged
merged 1 commit into from
Jul 13, 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
2 changes: 2 additions & 0 deletions crates/rome_js_analyze/src/control_flow/nodes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ mod statement;
mod switch_stmt;
mod throw_stmt;
mod try_catch;
mod variable;
mod while_stmt;

pub(super) use block::*;
Expand All @@ -26,4 +27,5 @@ pub(super) use statement::*;
pub(super) use switch_stmt::*;
pub(super) use throw_stmt::*;
pub(super) use try_catch::*;
pub(super) use variable::*;
pub(super) use while_stmt::*;
6 changes: 2 additions & 4 deletions crates/rome_js_analyze/src/control_flow/nodes/statement.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
use rome_js_syntax::{
JsDebuggerStatement, JsEmptyStatement, JsExpressionStatement, JsVariableStatement,
};
use rome_js_syntax::{JsDebuggerStatement, JsEmptyStatement, JsExpressionStatement};
use rome_rowan::{declare_node_union, AstNode, SyntaxResult};

use crate::control_flow::{
Expand All @@ -9,7 +7,7 @@ use crate::control_flow::{
};

declare_node_union! {
pub(in crate::control_flow) JsSimpleStatement = JsDebuggerStatement | JsEmptyStatement | JsExpressionStatement | JsVariableStatement
pub(in crate::control_flow) JsSimpleStatement = JsDebuggerStatement | JsEmptyStatement | JsExpressionStatement
}

pub(in crate::control_flow) struct StatementVisitor;
Expand Down
29 changes: 29 additions & 0 deletions crates/rome_js_analyze/src/control_flow/nodes/variable.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
use rome_js_syntax::JsVariableStatement;
use rome_rowan::{AstNode, SyntaxResult};

use crate::control_flow::{
visitor::{NodeVisitor, StatementStack},
FunctionBuilder,
};

pub(in crate::control_flow) struct VariableVisitor;

impl<B> NodeVisitor<B> for VariableVisitor {
type Node = JsVariableStatement;

fn enter(
node: Self::Node,
builder: &mut FunctionBuilder,
_: StatementStack,
) -> SyntaxResult<Self> {
let declaration = node.declaration()?;
for declarator in declaration.declarators() {
if let Some(initializer) = declarator?.initializer() {
let expr = initializer.expression()?;
builder.append_statement().with_node(expr.into_syntax());
}
}

Ok(Self)
}
}
1 change: 1 addition & 0 deletions crates/rome_js_analyze/src/control_flow/visitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ declare_visitor! {
continue_stmt: ContinueVisitor,
return_stmt: ReturnVisitor,
throw: ThrowVisitor,
variable: VariableVisitor,
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
function JsVariableStatement1() {
return;
var variable;
}

function JsVariableStatement2() {
return;
var variable = initializer();
}

function JsVariableStatement3() {
return;
let variable;
}

function JsVariableStatement4() {
return;
let variable = initializer();
}

function JsVariableStatement5() {
return;
const variable = initializer();
}

function JsVariableStatement6() {
return;
var variable1 = initializer(),
variable2 = initializer();
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
---
source: crates/rome_js_analyze/tests/spec_tests.rs
assertion_line: 98
expression: JsVariableStatement.js
---
# Input
```js
function JsVariableStatement1() {
return;
var variable;
}

function JsVariableStatement2() {
return;
var variable = initializer();
}

function JsVariableStatement3() {
return;
let variable;
}
Comment on lines +18 to +21
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This case should throw a diagnostic, even when it gets assigned. Same for const.

Examples

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned in the PR description, I expect a diagnostic for this case will be emitted by a future noUnusedVariable lint rule based on the semantic analysis instead. Any access to the variable before the return statement is invalid and will emit an error (since reading or writing a let variable before its declared is not permitted), while any access to the variable coming after the declaration will also be marked as unreachable and should be ignored by the noUnusedVariable rule. Since the variable will be marked as unused by the semantic analysis I wanted to avoid a duplicate diagnostic flagging it as unreachable again in the control flow analysis, so the control flow diagnostic only covers the initializer expression

Copy link
Contributor

@ematipico ematipico Jul 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought you were just covering the var declarations, because also the let don't need an initializers. Thank you.

Although also const would be affected by the noUnusedVariable, because it requires a initializer, but they are not hoisted to the gloabl scope. Which means that we could have double diagnostics on the same line.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while any access to the variable coming after the declaration will also be marked as unreachable and should be ignored by the noUnusedVariable rule.

Do we want that? This would imply a rule that needs the SemanticModeland the CFG. This means that we need to check if every reference is reachable or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which means that we could have double diagnostics on the same line.

There would indeed be two diagnostics on the same line but they would not necessarily overlap: noDeadCode will only highlight the initializer expression, and noUnusedVariable could highlight only the identifier in the declaration:

const UNUSED = "value";
      ------   ------- noDeadCode
      |
      noUnusedVariable

Do we want that? This would imply a rule that needs the SemanticModeland the CFG. This means that we need to check if every reference is reachable or not.

I don't think it's a requirement for the initial version of noUnusedVariable but it does seem like something we'll want to have eventually (and an good use case to exercise the capability of the analyzer infrastructure to orchestrate the execution of rules to fulfill complex query requirements)


function JsVariableStatement4() {
return;
let variable = initializer();
}

function JsVariableStatement5() {
return;
const variable = initializer();
}

function JsVariableStatement6() {
return;
var variable1 = initializer(),
variable2 = initializer();
}

```

# Diagnostics
```
warning[noDeadCode]: This code is unreachable
┌─ JsVariableStatement.js:8:20
7 │ return;
│ ------- This statement will return from the function ...
8 │ var variable = initializer();
│ ------------- ... before it can reach this code


```

```
warning[noDeadCode]: This code is unreachable
┌─ JsVariableStatement.js:18:20
17 │ return;
│ ------- This statement will return from the function ...
18 │ let variable = initializer();
│ ------------- ... before it can reach this code


```

```
warning[noDeadCode]: This code is unreachable
┌─ JsVariableStatement.js:23:22
22 │ return;
│ ------- This statement will return from the function ...
23 │ const variable = initializer();
│ ------------- ... before it can reach this code


```

```
warning[noDeadCode]: This code is unreachable
┌─ JsVariableStatement.js:28:21
27 │ return;
│ ------- This statement will return from the function ...
28 │ var variable1 = initializer(),
│ ------------- ... before it can reach this code


```

```
warning[noDeadCode]: This code is unreachable
┌─ JsVariableStatement.js:29:21
27 │ return;
│ ------- This statement will return from the function ...
28 │ var variable1 = initializer(),
29 │ variable2 = initializer();
│ ------------- ... before it can reach this code
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was expecting this PR to do something like:

warning[noDeadCode]: This code is unreachable
   ┌─ JsVariableStatement.js:29:21
   │
27 │     return;
   │     ------- This statement will return from the function ...
28 │     var variable1 = initializer(),
   │                     ------------- ... before it can reach this code
             variable2,
29 │         variable3 = initializer();
   │                     ------------- ... before it can reach this code

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is due to a limitation in the Language Server Protocol: when the diagnostic is displayed in the editor only the primary range will be rendered as "unnecessary", and since diagnostics can only have a single primary label the linter needs to emit a new diagnostic for each non-contiguous range of code that's unreachable



```