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

feat(rome_js_analyze): add module and classes support for noRedundantUseStrict #3955

Merged
merged 10 commits into from
Dec 12, 2022
Original file line number Diff line number Diff line change
Expand Up @@ -2,43 +2,61 @@ use crate::JsRuleAction;
use rome_analyze::{context::RuleContext, declare_rule, ActionCategory, Ast, Rule, RuleDiagnostic};
use rome_console::markup;
use rome_diagnostics::Applicability;
use rome_js_syntax::{JsDirective, JsDirectiveList, JsFunctionBody, JsModule, JsScript};
use rome_js_syntax::{
JsClassDeclaration, JsClassExpression, JsDirective, JsDirectiveList, JsFunctionBody,
JsLanguage, JsModule, JsScript,
};

use rome_rowan::{declare_node_union, AstNode, BatchMutationExt};
use rome_rowan::{declare_node_union, AstNode, BatchMutationExt, SyntaxNode};

declare_rule! {
/// Prevents from having redundant `"use strict"`.
///
/// ## Examples
///
/// ### Invalid
/// ```js,expect_diagnostic
/// ```cjs,expect_diagnostic
/// "use strict";
/// function foo() {
/// "use strict";
/// }
/// ```
/// ```js,expect_diagnostic
/// ```cjs,expect_diagnostic
/// "use strict";
/// "use strict";
///
/// function foo() {
///
/// }
/// ```
/// ```js,expect_diagnostic
/// ```cjs,expect_diagnostic
/// function foo() {
/// "use strict";
/// "use strict";
/// }
/// ```
/// ```cjs,expect_diagnostic
/// class C1 {
/// test() {
/// "use strict";
/// }
/// }
/// ```
/// ```cjs,expect_diagnostic
/// const C2 = class {
/// test() {
/// "use strict";
/// }
/// };
///
/// ```
/// ### Valid
/// ```js
/// ```cjs
/// function foo() {
///
/// }
///```
/// ```js
/// ```cjs
/// function foo() {
/// "use strict";
/// }
Expand All @@ -55,44 +73,45 @@ declare_rule! {
}
}

declare_node_union! { AnyNodeWithDirectives = JsFunctionBody | JsModule | JsScript }
declare_node_union! { AnyNodeWithDirectives = JsFunctionBody | JsScript }
impl AnyNodeWithDirectives {
fn directives(&self) -> JsDirectiveList {
match self {
AnyNodeWithDirectives::JsFunctionBody(node) => node.directives(),
AnyNodeWithDirectives::JsScript(script) => script.directives(),
AnyNodeWithDirectives::JsModule(module) => module.directives(),
}
}
}
declare_node_union! { JsModuleOrClass = JsClassDeclaration | JsClassExpression| JsModule }
mzbac marked this conversation as resolved.
Show resolved Hide resolved

impl Rule for NoRedundantUseStrict {
type Query = Ast<JsDirective>;
type State = JsDirective;
type State = SyntaxNode<JsLanguage>;
type Signals = Option<Self::State>;
type Options = ();

fn run(ctx: &RuleContext<Self>) -> Self::Signals {
let node = ctx.query();
let mut outer_most: Option<JsDirective> = None;
for parent in node
.syntax()
.ancestors()
.filter_map(AnyNodeWithDirectives::cast)
{
for directive in parent.directives() {
if directive.value_token().map_or(false, |t| {
matches!(t.text_trimmed(), "'use strict'" | "\"use strict\"")
}) {
outer_most = Some(directive);
break; // continue with next parent
let mut outer_most: Option<SyntaxNode<JsLanguage>> = None;
for n in node.syntax().ancestors() {
if let Some(parent) = AnyNodeWithDirectives::cast_ref(&n) {
for directive in parent.directives() {
if directive.value_token().map_or(false, |t| {
matches!(t.text_trimmed(), "'use strict'" | "\"use strict\"")
}) {
outer_most = Some(directive.into());
break; // continue with next parent
}
}
}
if let Some(module_or_class) = JsModuleOrClass::cast_ref(&n) {
mzbac marked this conversation as resolved.
Show resolved Hide resolved
outer_most = Some(module_or_class.into());
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm undecided if we should break if we find a class because it doesn't matter if any outer node has a use strict directive .

Copy link
Contributor Author

@mzbac mzbac Dec 5, 2022

Choose a reason for hiding this comment

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

I'm not sure if I understand this correctly. Are you saying that the 'use strict' is scoped within the class, so the outer node would have its own 'use strict' directive?

}
mzbac marked this conversation as resolved.
Show resolved Hide resolved
}

if let Some(outer_most) = outer_most {
// skip itself
if &outer_most == node {
if &outer_most == node.syntax() {
return None;
}
return Some(outer_most);
Expand All @@ -101,18 +120,33 @@ impl Rule for NoRedundantUseStrict {
None
}
fn diagnostic(ctx: &RuleContext<Self>, state: &Self::State) -> Option<RuleDiagnostic> {
let diag = RuleDiagnostic::new(
let mut diag = RuleDiagnostic::new(
rule_category!(),
ctx.query().range(),
markup! {
"Redundant "<Emphasis>{"use strict"}</Emphasis>" directive."
},
)
.detail(
state.range(),
markup! {"This outer "<Emphasis>{"use strict"}</Emphasis>" directive already enables strict mode."},
);

if JsModule::can_cast(state.kind()) {
diag= diag.note(
markup! {"The entire contents of "<Emphasis>{"JavaScript modules are automatically"}</Emphasis>" in strict mode, with no statement needed to initiate it."},
mzbac marked this conversation as resolved.
Show resolved Hide resolved
);
} else if JsClassDeclaration::can_cast(state.kind())
|| JsClassExpression::can_cast(state.kind())
{
diag = diag.detail(
state.text_range(),
markup! {"All parts of a class's body are already in strict mode."},
);
} else {
// for redundant directive
diag= diag.detail(
state.text_range(),
markup! {"This outer "<Emphasis>{"use strict"}</Emphasis>" directive already enables strict mode."},
);
}

Some(diag)
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
"use strict";
"use strict";

function test() {
"use strict";
function inner_a() {
"use strict"; // redundant directive
}
function inner_b() {
function inner_inner() {
"use strict"; // additional redundant directive
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,149 @@
---
source: crates/rome_js_analyze/tests/spec_tests.rs
assertion_line: 86
expression: invalid.cjs
---
# Input
```js
"use strict";
"use strict";

function test() {
"use strict";
function inner_a() {
"use strict"; // redundant directive
}
function inner_b() {
function inner_inner() {
"use strict"; // additional redundant directive
}
}
}

```

# Diagnostics
```
invalid.cjs:2:1 lint/nursery/noRedundantUseStrict FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

! Redundant use strict directive.

1 │ "use strict";
> 2 │ "use strict";
│ ^^^^^^^^^^^^^
3 │
4 │ function test() {

i This outer use strict directive already enables strict mode.

> 1 │ "use strict";
│ ^^^^^^^^^^^^^
2 │ "use strict";
3 │

i Safe fix: Remove the redundant "use strict" directive

1 1 │ "use strict";
2 │ - "use·strict";
3 2 │
4 3 │ function test() {


```

```
invalid.cjs:5:2 lint/nursery/noRedundantUseStrict FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

! Redundant use strict directive.

4 │ function test() {
> 5 │ "use strict";
│ ^^^^^^^^^^^^^
6 │ function inner_a() {
7 │ "use strict"; // redundant directive

i This outer use strict directive already enables strict mode.

> 1 │ "use strict";
│ ^^^^^^^^^^^^^
2 │ "use strict";
3 │

i Safe fix: Remove the redundant "use strict" directive

3 3 │
4 4 │ function test() {
5 │ - → "use·strict";
6 │ - → function·inner_a()·{
5 │ + → function·inner_a()·{
7 6 │ "use strict"; // redundant directive
8 7 │ }


```

```
invalid.cjs:7:3 lint/nursery/noRedundantUseStrict FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

! Redundant use strict directive.

5 │ "use strict";
6 │ function inner_a() {
> 7 │ "use strict"; // redundant directive
│ ^^^^^^^^^^^^^
8 │ }
9 │ function inner_b() {

i This outer use strict directive already enables strict mode.

> 1 │ "use strict";
│ ^^^^^^^^^^^^^
2 │ "use strict";
3 │

i Safe fix: Remove the redundant "use strict" directive

5 5 │ "use strict";
6 6 │ function inner_a() {
7 │ - → → "use·strict";·//·redundant·directive
8 │ - → }
7 │ + → }
9 8 │ function inner_b() {
10 9 │ function inner_inner() {


```

```
invalid.cjs:11:4 lint/nursery/noRedundantUseStrict FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

! Redundant use strict directive.

9 │ function inner_b() {
10 │ function inner_inner() {
> 11 │ "use strict"; // additional redundant directive
│ ^^^^^^^^^^^^^
12 │ }
13 │ }

i This outer use strict directive already enables strict mode.

> 1 │ "use strict";
│ ^^^^^^^^^^^^^
2 │ "use strict";
3 │

i Safe fix: Remove the redundant "use strict" directive

9 9 │ function inner_b() {
10 10 │ function inner_inner() {
11 │ - → → → "use·strict";·//·additional·redundant·directive
12 │ - → → }
11 │ + → → }
13 12 │ }
14 13 │ }


```


Original file line number Diff line number Diff line change
@@ -1,14 +1,18 @@
"use strict";
"use strict";

function test() {
// js module
function foo() {
"use strict";
function inner_a() {
"use strict"; // redundant directive
}
function inner_b() {
function inner_inner() {
"use strict"; // additional redundant directive
}
}

class C1 {
// All code here is evaluated in strict mode
test() {
"use strict";
}
}

const C2 = class {
// All code here is evaluated in strict mode
test() {
"use strict";
}
};
Loading