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

Commit

Permalink
feature(rome_js_analyze): no function reassign lint rule (#2895)
Browse files Browse the repository at this point in the history
* hoisting function declaration for no function reassign lint rule
  • Loading branch information
xunilrj authored Jul 19, 2022
1 parent f520c9c commit b97d5e1
Show file tree
Hide file tree
Showing 9 changed files with 544 additions and 13 deletions.
3 changes: 2 additions & 1 deletion crates/rome_js_analyze/src/semantic_analyzers/js.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
use rome_analyze::declare_group;
mod no_arguments;
mod no_catch_assign;
mod no_function_assign;
mod no_label_var;
mod no_shouty_constants;
declare_group! { pub (crate) Js { name : "js" , rules : [no_arguments :: NoArguments , no_catch_assign :: NoCatchAssign , no_label_var :: NoLabelVar , no_shouty_constants :: NoShoutyConstants ,] } }
declare_group! { pub (crate) Js { name : "js" , rules : [no_arguments :: NoArguments , no_catch_assign :: NoCatchAssign , no_function_assign :: NoFunctionAssign , no_label_var :: NoLabelVar , no_shouty_constants :: NoShoutyConstants ,] } }
164 changes: 164 additions & 0 deletions crates/rome_js_analyze/src/semantic_analyzers/js/no_function_assign.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,164 @@
use crate::semantic_services::Semantic;
use rome_analyze::{context::RuleContext, declare_rule, Rule, RuleCategory, RuleDiagnostic};
use rome_console::markup;
use rome_js_semantic::{AllReferencesExtensions, Reference};
use rome_js_syntax::{JsFunctionDeclaration, JsIdentifierBinding};
use rome_rowan::AstNode;

declare_rule! {
/// Disallow reassigning function declarations.
///
/// ## Examples
///
/// ### Invalid
///
/// ```js,expect_diagnostic
/// function foo() { };
/// foo = bar;
/// ```
///
/// ```js,expect_diagnostic
/// function foo() {
/// foo = bar;
/// }
/// ```
///
/// ```js,expect_diagnostic
/// foo = bar;
/// function foo() { };
/// ```
///
/// ```js,expect_diagnostic
/// [foo] = bar;
/// function foo() { };
/// ```
///
/// ```js,expect_diagnostic
/// ({ x: foo = 0 } = bar);
/// function foo() { };
/// ```
///
/// ```js,expect_diagnostic
/// function foo() {
/// [foo] = bar;
/// }
/// ```
/// ```js,expect_diagnostic
/// (function () {
/// ({ x: foo = 0 } = bar);
/// function foo() { };
/// })();
/// ```
///
/// ## Valid
///
/// ```js
/// function foo() {
/// var foo = bar;
/// }
/// ```
///
/// ```js
/// function foo(foo) {
/// foo = bar;
/// }
/// ```
///
/// ```js
/// function foo() {
/// var foo;
/// foo = bar;
/// }
/// ```
///
/// ```js
/// var foo = () => {};
/// foo = bar;
/// ```
///
/// ```js
/// var foo = function() {};
/// foo = bar;
/// ```
///
/// ```js
/// var foo = function() {
/// foo = bar;
/// };
/// ```
///
/// ```js
/// import bar from 'bar';
/// function foo() {
/// var foo = bar;
/// }
/// ```
pub(crate) NoFunctionAssign {
version: "0.7.0",
name: "noFunctionAssign"
}
}

pub struct State {
id: JsIdentifierBinding,
all_writes: Vec<Reference>,
}

impl Rule for NoFunctionAssign {
const CATEGORY: RuleCategory = RuleCategory::Lint;

type Query = Semantic<JsFunctionDeclaration>;
type State = State;
type Signals = Option<Self::State>;

fn run(ctx: &RuleContext<Self>) -> Option<Self::State> {
let declaration = ctx.query();
let model = ctx.model();

let id = declaration.id().ok()?;
let id = id.as_js_identifier_binding()?;
let all_writes: Vec<Reference> = id.all_writes(model).collect();

if all_writes.is_empty() {
None
} else {
Some(State {
id: id.clone(),
all_writes,
})
}
}

fn diagnostic(_: &RuleContext<Self>, state: &Self::State) -> Option<RuleDiagnostic> {
let mut diag = RuleDiagnostic::warning(
state.id.syntax().text_trimmed_range(),
markup! {
"Do not reassign a function declaration."
},
);

let mut hoisted_quantity = 0;
for reference in state.all_writes.iter() {
let node = reference.node();
diag = diag.secondary(node.text_trimmed_range(), "Reassigned here.");

hoisted_quantity += if reference.is_using_hoisted_declaration() {
1
} else {
0
};
}

let diag = if hoisted_quantity > 0 {
diag.footer_note(
markup! {"Reassignment happens here because the function declaration is hoisted."},
)
} else {
diag
};

let diag = diag.footer_note(markup! {"Use a local variable instead."});

Some(diag)
}
}
25 changes: 25 additions & 0 deletions crates/rome_js_analyze/tests/specs/js/noFunctionAssign.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
function foo() { };
foo = bar;

function foo2() { foo2 = bar; }

foo3 = bar;
function foo3() { };

[foo4] = bar;
function foo4() { };

({ x: foo5 = 0 } = bar);
function foo5() { };

function foo6() { [foo6] = bar; }

(function () { ({ x: foo7 = 0 } = bar); function foo7() { }; })();

// Valid
function foo8() { var foo8 = bar; }
function foo9(foo9) { foo9 = bar; }
function foo10() { var foo10; foo10 = bar; }
var foo11 = () => { }; foo11 = bar;
var foo12 = function () { }; foo12 = bar;
var foo13 = function () { foo13 = bar; };
133 changes: 133 additions & 0 deletions crates/rome_js_analyze/tests/specs/js/noFunctionAssign.js.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
---
source: crates/rome_js_analyze/tests/spec_tests.rs
expression: noFunctionAssign.js
---
# Input
```js
function foo() { };
foo = bar;
function foo2() { foo2 = bar; }
foo3 = bar;
function foo3() { };
[foo4] = bar;
function foo4() { };
({ x: foo5 = 0 } = bar);
function foo5() { };
function foo6() { [foo6] = bar; }
(function () { ({ x: foo7 = 0 } = bar); function foo7() { }; })();
// Valid
function foo8() { var foo8 = bar; }
function foo9(foo9) { foo9 = bar; }
function foo10() { var foo10; foo10 = bar; }
var foo11 = () => { }; foo11 = bar;
var foo12 = function () { }; foo12 = bar;
var foo13 = function () { foo13 = bar; };
```

# Diagnostics
```
warning[js/noFunctionAssign]: Do not reassign a function declaration.
┌─ noFunctionAssign.js:1:10
1 │ function foo() { };
│ ---
2 │ foo = bar;
│ --- Reassigned here.
= note: Use a local variable instead.
```

```
warning[js/noFunctionAssign]: Do not reassign a function declaration.
┌─ noFunctionAssign.js:4:10
4 │ function foo2() { foo2 = bar; }
│ ---- ---- Reassigned here.
= note: Use a local variable instead.
```

```
warning[js/noFunctionAssign]: Do not reassign a function declaration.
┌─ noFunctionAssign.js:7:10
6 │ foo3 = bar;
│ ---- Reassigned here.
7 │ function foo3() { };
│ ----
= note: Reassignment happens here because the function declaration is hoisted.
= note: Use a local variable instead.
```

```
warning[js/noFunctionAssign]: Do not reassign a function declaration.
┌─ noFunctionAssign.js:10:10
9 │ [foo4] = bar;
│ ---- Reassigned here.
10 │ function foo4() { };
│ ----
= note: Reassignment happens here because the function declaration is hoisted.
= note: Use a local variable instead.
```

```
warning[js/noFunctionAssign]: Do not reassign a function declaration.
┌─ noFunctionAssign.js:13:10
12 │ ({ x: foo5 = 0 } = bar);
│ ---- Reassigned here.
13 │ function foo5() { };
│ ----
= note: Reassignment happens here because the function declaration is hoisted.
= note: Use a local variable instead.
```

```
warning[js/noFunctionAssign]: Do not reassign a function declaration.
┌─ noFunctionAssign.js:15:10
15 │ function foo6() { [foo6] = bar; }
│ ---- ---- Reassigned here.
= note: Use a local variable instead.
```

```
warning[js/noFunctionAssign]: Do not reassign a function declaration.
┌─ noFunctionAssign.js:17:50
17 │ (function () { ({ x: foo7 = 0 } = bar); function foo7() { }; })();
│ ---- ----
│ │
│ Reassigned here.
= note: Reassignment happens here because the function declaration is hoisted.
= note: Use a local variable instead.
```


4 changes: 4 additions & 0 deletions crates/rome_js_semantic/src/events.rs
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,10 @@ impl SemanticEventExtractor {
self.push_binding_into_scope(scope_idx, &name_token);
};
}
Some(JS_FUNCTION_DECLARATION) => {
let scope_idx = self.scope_index_to_hoist_declarations();
self.push_binding_into_scope(scope_idx, &name_token);
}
Some(_) => {
let scope_idx = self.scopes.len() - 1;
self.push_binding_into_scope(scope_idx, &name_token);
Expand Down
Loading

0 comments on commit b97d5e1

Please sign in to comment.