From be9d728ec0a3f76725d93eff8b0d270747f51497 Mon Sep 17 00:00:00 2001 From: Anchen Li Date: Wed, 16 Nov 2022 01:55:40 +1100 Subject: [PATCH 01/11] fix(semantic_analyzers): fix the false error for noConstAssign --- .../nursery/no_const_assign.rs | 17 +-- .../tests/specs/nursery/noConstAssign.js | 13 +- .../tests/specs/nursery/noConstAssign.js.snap | 131 +++++++++++------- 3 files changed, 102 insertions(+), 59 deletions(-) diff --git a/crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs b/crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs index b5e72b332d4..4b1f015568b 100644 --- a/crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs +++ b/crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs @@ -2,7 +2,7 @@ use crate::semantic_services::Semantic; use rome_analyze::context::RuleContext; use rome_analyze::{declare_rule, Rule, RuleDiagnostic}; use rome_console::markup; -use rome_js_syntax::{JsIdentifierAssignment, JsVariableDeclaration}; +use rome_js_syntax::{JsFormalParameter, JsIdentifierAssignment, JsVariableDeclaration}; use rome_rowan::{AstNode, TextRange}; declare_rule! { @@ -61,13 +61,14 @@ impl Rule for NoConstAssign { let model = ctx.model(); let declared_binding = model.declaration(node)?; - if let Some(variable_declaration) = declared_binding - .syntax() - .ancestors() - .find_map(|ancestor| JsVariableDeclaration::cast_ref(&ancestor)) - { - if variable_declaration.is_const() { - return Some(declared_binding.syntax().text_trimmed_range()); + for node in declared_binding.syntax().ancestors() { + if let Some(_) = JsFormalParameter::cast_ref(&node) { + return None; + } + if let Some(variable_declaration) = JsVariableDeclaration::cast_ref(&node) { + if variable_declaration.is_const() { + return Some(declared_binding.syntax().text_trimmed_range()); + } } } diff --git a/crates/rome_js_analyze/tests/specs/nursery/noConstAssign.js b/crates/rome_js_analyze/tests/specs/nursery/noConstAssign.js index 0f07d27799a..02b383cf555 100644 --- a/crates/rome_js_analyze/tests/specs/nursery/noConstAssign.js +++ b/crates/rome_js_analyze/tests/specs/nursery/noConstAssign.js @@ -1,11 +1,18 @@ const a = 1; a = 2; -const b = 2, c = 43; +const b = 2, + c = 43; b = 4; ++b; b += 45; b--; function f() { - b++; -} \ No newline at end of file + b++; +} +function f(d) { + b++; +} +const fn = (val) => { + val = 0; +}; diff --git a/crates/rome_js_analyze/tests/specs/nursery/noConstAssign.js.snap b/crates/rome_js_analyze/tests/specs/nursery/noConstAssign.js.snap index 459304b621a..def4ebaa0fd 100644 --- a/crates/rome_js_analyze/tests/specs/nursery/noConstAssign.js.snap +++ b/crates/rome_js_analyze/tests/specs/nursery/noConstAssign.js.snap @@ -1,5 +1,6 @@ --- source: crates/rome_js_analyze/tests/spec_tests.rs +assertion_line: 99 expression: noConstAssign.js --- # Input @@ -7,14 +8,22 @@ expression: noConstAssign.js const a = 1; a = 2; -const b = 2, c = 43; +const b = 2, + c = 43; b = 4; ++b; b += 45; b--; function f() { - b++; + b++; } +function f(d) { + b++; +} +const fn = (val) => { + val = 0; +}; + ``` # Diagnostics @@ -27,7 +36,7 @@ noConstAssign.js:2:1 lint/nursery/noConstAssign ━━━━━━━━━━ > 2 │ a = 2; │ ^ 3 │ - 4 │ const b = 2, c = 43; + 4 │ const b = 2, i This is where the variable is defined as constant @@ -40,119 +49,145 @@ noConstAssign.js:2:1 lint/nursery/noConstAssign ━━━━━━━━━━ ``` ``` -noConstAssign.js:5:1 lint/nursery/noConstAssign ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +noConstAssign.js:6:1 lint/nursery/noConstAssign ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ ! Can't assign b because it's a constant - 4 │ const b = 2, c = 43; - > 5 │ b = 4; + 4 │ const b = 2, + 5 │ c = 43; + > 6 │ b = 4; │ ^ - 6 │ ++b; - 7 │ b += 45; + 7 │ ++b; + 8 │ b += 45; i This is where the variable is defined as constant 2 │ a = 2; 3 │ - > 4 │ const b = 2, c = 43; + > 4 │ const b = 2, │ ^ - 5 │ b = 4; - 6 │ ++b; + 5 │ c = 43; + 6 │ b = 4; ``` ``` -noConstAssign.js:6:3 lint/nursery/noConstAssign ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +noConstAssign.js:7:3 lint/nursery/noConstAssign ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ ! Can't assign b because it's a constant - 4 │ const b = 2, c = 43; - 5 │ b = 4; - > 6 │ ++b; + 5 │ c = 43; + 6 │ b = 4; + > 7 │ ++b; │ ^ - 7 │ b += 45; - 8 │ b--; + 8 │ b += 45; + 9 │ b--; i This is where the variable is defined as constant 2 │ a = 2; 3 │ - > 4 │ const b = 2, c = 43; + > 4 │ const b = 2, │ ^ - 5 │ b = 4; - 6 │ ++b; + 5 │ c = 43; + 6 │ b = 4; ``` ``` -noConstAssign.js:7:1 lint/nursery/noConstAssign ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +noConstAssign.js:8:1 lint/nursery/noConstAssign ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ ! Can't assign b because it's a constant - 5 │ b = 4; - 6 │ ++b; - > 7 │ b += 45; - │ ^ - 8 │ b--; - 9 │ function f() { + 6 │ b = 4; + 7 │ ++b; + > 8 │ b += 45; + │ ^ + 9 │ b--; + 10 │ function f() { i This is where the variable is defined as constant 2 │ a = 2; 3 │ - > 4 │ const b = 2, c = 43; + > 4 │ const b = 2, │ ^ - 5 │ b = 4; - 6 │ ++b; + 5 │ c = 43; + 6 │ b = 4; ``` ``` -noConstAssign.js:8:1 lint/nursery/noConstAssign ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +noConstAssign.js:9:1 lint/nursery/noConstAssign ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ ! Can't assign b because it's a constant - 6 │ ++b; - 7 │ b += 45; - > 8 │ b--; + 7 │ ++b; + 8 │ b += 45; + > 9 │ b--; │ ^ - 9 │ function f() { - 10 │ b++; + 10 │ function f() { + 11 │ b++; + + i This is where the variable is defined as constant + + 2 │ a = 2; + 3 │ + > 4 │ const b = 2, + │ ^ + 5 │ c = 43; + 6 │ b = 4; + + +``` + +``` +noConstAssign.js:11:2 lint/nursery/noConstAssign ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Can't assign b because it's a constant + + 9 │ b--; + 10 │ function f() { + > 11 │ b++; + │ ^ + 12 │ } + 13 │ function f(d) { i This is where the variable is defined as constant 2 │ a = 2; 3 │ - > 4 │ const b = 2, c = 43; + > 4 │ const b = 2, │ ^ - 5 │ b = 4; - 6 │ ++b; + 5 │ c = 43; + 6 │ b = 4; ``` ``` -noConstAssign.js:10:5 lint/nursery/noConstAssign ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +noConstAssign.js:14:2 lint/nursery/noConstAssign ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ ! Can't assign b because it's a constant - 8 │ b--; - 9 │ function f() { - > 10 │ b++; - │ ^ - 11 │ } + 12 │ } + 13 │ function f(d) { + > 14 │ b++; + │ ^ + 15 │ } + 16 │ const fn = (val) => { i This is where the variable is defined as constant 2 │ a = 2; 3 │ - > 4 │ const b = 2, c = 43; + > 4 │ const b = 2, │ ^ - 5 │ b = 4; - 6 │ ++b; + 5 │ c = 43; + 6 │ b = 4; ``` From cfca99aecd1db325e6eae2306de953f70721f001 Mon Sep 17 00:00:00 2001 From: Anchen Li Date: Wed, 16 Nov 2022 09:31:09 +1100 Subject: [PATCH 02/11] address comments --- .../src/semantic_analyzers/nursery/no_const_assign.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs b/crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs index 4b1f015568b..e258aba687d 100644 --- a/crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs +++ b/crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs @@ -62,7 +62,7 @@ impl Rule for NoConstAssign { let declared_binding = model.declaration(node)?; for node in declared_binding.syntax().ancestors() { - if let Some(_) = JsFormalParameter::cast_ref(&node) { + if JsFormalParameter::can_cast(node.kind()) { return None; } if let Some(variable_declaration) = JsVariableDeclaration::cast_ref(&node) { From 7dfa68f6ff501b17435c4b1713c8c1b9cc432321 Mon Sep 17 00:00:00 2001 From: Anchen Li Date: Wed, 16 Nov 2022 15:38:44 +1100 Subject: [PATCH 03/11] add hardcoded case checking --- .../nursery/no_const_assign.rs | 12 ++++++++++-- .../tests/specs/nursery/noConstAssign.js | 19 +++++++++++++++++++ .../tests/specs/nursery/noConstAssign.js.snap | 19 +++++++++++++++++++ 3 files changed, 48 insertions(+), 2 deletions(-) diff --git a/crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs b/crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs index e258aba687d..f35d62c64b2 100644 --- a/crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs +++ b/crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs @@ -2,7 +2,10 @@ use crate::semantic_services::Semantic; use rome_analyze::context::RuleContext; use rome_analyze::{declare_rule, Rule, RuleDiagnostic}; use rome_console::markup; -use rome_js_syntax::{JsFormalParameter, JsIdentifierAssignment, JsVariableDeclaration}; +use rome_js_syntax::{ + JsCatchDeclaration, JsClassExpression, JsFunctionExpression, JsIdentifierAssignment, + JsParameterList, JsVariableDeclaration, +}; use rome_rowan::{AstNode, TextRange}; declare_rule! { @@ -61,8 +64,13 @@ impl Rule for NoConstAssign { let model = ctx.model(); let declared_binding = model.declaration(node)?; + for node in declared_binding.syntax().ancestors() { - if JsFormalParameter::can_cast(node.kind()) { + if JsParameterList::can_cast(node.kind()) + || JsCatchDeclaration::can_cast(node.kind()) + || JsClassExpression::can_cast(node.kind()) + || JsFunctionExpression::can_cast(node.kind()) + { return None; } if let Some(variable_declaration) = JsVariableDeclaration::cast_ref(&node) { diff --git a/crates/rome_js_analyze/tests/specs/nursery/noConstAssign.js b/crates/rome_js_analyze/tests/specs/nursery/noConstAssign.js index 02b383cf555..54c5a4eb657 100644 --- a/crates/rome_js_analyze/tests/specs/nursery/noConstAssign.js +++ b/crates/rome_js_analyze/tests/specs/nursery/noConstAssign.js @@ -16,3 +16,22 @@ function f(d) { const fn = (val) => { val = 0; }; + +const e = () => { + try { + foo(); + } catch (err) { + err = 4; + } +}; + +const f = (...rest) => { + rest = 4; +}; + +const g = class bar {}; +bar = 1; + +const h = function foo() { + foo = 1; +}; diff --git a/crates/rome_js_analyze/tests/specs/nursery/noConstAssign.js.snap b/crates/rome_js_analyze/tests/specs/nursery/noConstAssign.js.snap index def4ebaa0fd..c9ab4c61c58 100644 --- a/crates/rome_js_analyze/tests/specs/nursery/noConstAssign.js.snap +++ b/crates/rome_js_analyze/tests/specs/nursery/noConstAssign.js.snap @@ -24,6 +24,25 @@ const fn = (val) => { val = 0; }; +const e = () => { + try { + foo(); + } catch (err) { + err = 4; + } +}; + +const f = (...rest) => { + rest = 4; +}; + +const g = class bar {}; +bar = 1; + +const h = function foo() { + foo = 1; +}; + ``` # Diagnostics From 72e6c1c1095ae6adee66254ce2624d5967e54c6c Mon Sep 17 00:00:00 2001 From: Anchen Li Date: Wed, 16 Nov 2022 23:02:55 +1100 Subject: [PATCH 04/11] change only travesal upward when parent is declarator --- .../nursery/no_const_assign.rs | 28 +++++++++---------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs b/crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs index f35d62c64b2..460e6149d8e 100644 --- a/crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs +++ b/crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs @@ -2,10 +2,7 @@ use crate::semantic_services::Semantic; use rome_analyze::context::RuleContext; use rome_analyze::{declare_rule, Rule, RuleDiagnostic}; use rome_console::markup; -use rome_js_syntax::{ - JsCatchDeclaration, JsClassExpression, JsFunctionExpression, JsIdentifierAssignment, - JsParameterList, JsVariableDeclaration, -}; +use rome_js_syntax::{JsIdentifierAssignment, JsSyntaxKind, JsVariableDeclaration}; use rome_rowan::{AstNode, TextRange}; declare_rule! { @@ -65,19 +62,20 @@ impl Rule for NoConstAssign { let declared_binding = model.declaration(node)?; - for node in declared_binding.syntax().ancestors() { - if JsParameterList::can_cast(node.kind()) - || JsCatchDeclaration::can_cast(node.kind()) - || JsClassExpression::can_cast(node.kind()) - || JsFunctionExpression::can_cast(node.kind()) - { - return None; - } - if let Some(variable_declaration) = JsVariableDeclaration::cast_ref(&node) { - if variable_declaration.is_const() { - return Some(declared_binding.syntax().text_trimmed_range()); + let parent = declared_binding.syntax().parent()?; + + match parent.kind() { + JsSyntaxKind::JS_VARIABLE_DECLARATOR => { + if let Some(variable_declaration) = parent + .ancestors() + .find_map(|ancestor| JsVariableDeclaration::cast_ref(&ancestor)) + { + if variable_declaration.is_const() { + return Some(declared_binding.syntax().text_trimmed_range()); + } } } + _ => return None, } None From 40a4a5b5d650010196827def74deabb535554703 Mon Sep 17 00:00:00 2001 From: Anchen Li Date: Thu, 17 Nov 2022 23:28:15 +1100 Subject: [PATCH 05/11] fix for in and destructuring --- .../nursery/no_const_assign.rs | 32 +++-- .../tests/specs/nursery/noConstAssign.js | 15 ++ .../tests/specs/nursery/noConstAssign.js.snap | 131 ++++++++++++++++++ crates/rome_js_syntax/src/stmt_ext.rs | 18 ++- 4 files changed, 185 insertions(+), 11 deletions(-) diff --git a/crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs b/crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs index 460e6149d8e..fed8ce7e41c 100644 --- a/crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs +++ b/crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs @@ -2,7 +2,11 @@ use crate::semantic_services::Semantic; use rome_analyze::context::RuleContext; use rome_analyze::{declare_rule, Rule, RuleDiagnostic}; use rome_console::markup; -use rome_js_syntax::{JsIdentifierAssignment, JsSyntaxKind, JsVariableDeclaration}; +use rome_js_syntax::{ + JsAnyBindingPattern, JsArrayBindingPatternElementList, JsForVariableDeclaration, + JsIdentifierAssignment, JsObjectBindingPatternShorthandProperty, JsVariableDeclaration, + JsVariableDeclarator, +}; use rome_rowan::{AstNode, TextRange}; declare_rule! { @@ -62,20 +66,28 @@ impl Rule for NoConstAssign { let declared_binding = model.declaration(node)?; - let parent = declared_binding.syntax().parent()?; + let js_any_binding_parent = declared_binding + .syntax() + .ancestors() + .find(|n| !JsAnyBindingPattern::can_cast(n.kind()))?; - match parent.kind() { - JsSyntaxKind::JS_VARIABLE_DECLARATOR => { - if let Some(variable_declaration) = parent - .ancestors() - .find_map(|ancestor| JsVariableDeclaration::cast_ref(&ancestor)) - { - if variable_declaration.is_const() { + let js_any_binding_parent_kind = js_any_binding_parent.kind(); + if JsVariableDeclarator::can_cast(js_any_binding_parent_kind) + || JsArrayBindingPatternElementList::can_cast(js_any_binding_parent_kind) + || JsObjectBindingPatternShorthandProperty::can_cast(js_any_binding_parent_kind) + { + for n in js_any_binding_parent.ancestors() { + if let Some(js_variable_declaration) = JsVariableDeclaration::cast_ref(&n) { + if js_variable_declaration.is_const() { + return Some(declared_binding.syntax().text_trimmed_range()); + } + } + if let Some(js_for_variable_declaration) = JsForVariableDeclaration::cast_ref(&n) { + if js_for_variable_declaration.is_const() { return Some(declared_binding.syntax().text_trimmed_range()); } } } - _ => return None, } None diff --git a/crates/rome_js_analyze/tests/specs/nursery/noConstAssign.js b/crates/rome_js_analyze/tests/specs/nursery/noConstAssign.js index 54c5a4eb657..3408c0d4003 100644 --- a/crates/rome_js_analyze/tests/specs/nursery/noConstAssign.js +++ b/crates/rome_js_analyze/tests/specs/nursery/noConstAssign.js @@ -35,3 +35,18 @@ bar = 1; const h = function foo() { foo = 1; }; + +const { + i, + j: { l }, +} = { i: 1, j: { l: 2 } }; +i = 4; +l = 4; + +for (const k in [1, 2]) { + k = 4; +} + +const [p, { q }] = [1, { q: 2 }]; +p = 3; +q = 4; diff --git a/crates/rome_js_analyze/tests/specs/nursery/noConstAssign.js.snap b/crates/rome_js_analyze/tests/specs/nursery/noConstAssign.js.snap index c9ab4c61c58..ca157d5f38a 100644 --- a/crates/rome_js_analyze/tests/specs/nursery/noConstAssign.js.snap +++ b/crates/rome_js_analyze/tests/specs/nursery/noConstAssign.js.snap @@ -43,6 +43,21 @@ const h = function foo() { foo = 1; }; +const { + i, + j: { l }, +} = { i: 1, j: { l: 2 } }; +i = 4; +l = 4; + +for (const k in [1, 2]) { + k = 4; +} + +const [p, { q }] = [1, { q: 2 }]; +p = 3; +q = 4; + ``` # Diagnostics @@ -211,4 +226,120 @@ noConstAssign.js:14:2 lint/nursery/noConstAssign ━━━━━━━━━━ ``` +``` +noConstAssign.js:43:1 lint/nursery/noConstAssign ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Can't assign i because it's a constant + + 41 │ j: { l }, + 42 │ } = { i: 1, j: { l: 2 } }; + > 43 │ i = 4; + │ ^ + 44 │ l = 4; + 45 │ + + i This is where the variable is defined as constant + + 39 │ const { + > 40 │ i, + │ ^ + 41 │ j: { l }, + 42 │ } = { i: 1, j: { l: 2 } }; + + +``` + +``` +noConstAssign.js:44:1 lint/nursery/noConstAssign ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Can't assign l because it's a constant + + 42 │ } = { i: 1, j: { l: 2 } }; + 43 │ i = 4; + > 44 │ l = 4; + │ ^ + 45 │ + 46 │ for (const k in [1, 2]) { + + i This is where the variable is defined as constant + + 39 │ const { + 40 │ i, + > 41 │ j: { l }, + │ ^ + 42 │ } = { i: 1, j: { l: 2 } }; + 43 │ i = 4; + + +``` + +``` +noConstAssign.js:47:2 lint/nursery/noConstAssign ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Can't assign k because it's a constant + + 46 │ for (const k in [1, 2]) { + > 47 │ k = 4; + │ ^ + 48 │ } + 49 │ + + i This is where the variable is defined as constant + + 44 │ l = 4; + 45 │ + > 46 │ for (const k in [1, 2]) { + │ ^ + 47 │ k = 4; + 48 │ } + + +``` + +``` +noConstAssign.js:51:1 lint/nursery/noConstAssign ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Can't assign p because it's a constant + + 50 │ const [p, { q }] = [1, { q: 2 }]; + > 51 │ p = 3; + │ ^ + 52 │ q = 4; + 53 │ + + i This is where the variable is defined as constant + + 48 │ } + 49 │ + > 50 │ const [p, { q }] = [1, { q: 2 }]; + │ ^ + 51 │ p = 3; + 52 │ q = 4; + + +``` + +``` +noConstAssign.js:52:1 lint/nursery/noConstAssign ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Can't assign q because it's a constant + + 50 │ const [p, { q }] = [1, { q: 2 }]; + 51 │ p = 3; + > 52 │ q = 4; + │ ^ + 53 │ + + i This is where the variable is defined as constant + + 48 │ } + 49 │ + > 50 │ const [p, { q }] = [1, { q: 2 }]; + │ ^ + 51 │ p = 3; + 52 │ q = 4; + + +``` + diff --git a/crates/rome_js_syntax/src/stmt_ext.rs b/crates/rome_js_syntax/src/stmt_ext.rs index a4f3168c2fb..2e96ef651eb 100644 --- a/crates/rome_js_syntax/src/stmt_ext.rs +++ b/crates/rome_js_syntax/src/stmt_ext.rs @@ -1,6 +1,6 @@ //! Extended AST node definitions for statements which are unique and special enough to generate code for manually -use crate::{JsVariableDeclaration, T}; +use crate::{JsForVariableDeclaration, JsVariableDeclaration, T}; use rome_rowan::SyntaxResult; #[derive(Debug, Copy, Clone, Eq, PartialEq, Hash)] @@ -37,7 +37,23 @@ impl JsVariableDeclaration { }) } } +impl JsForVariableDeclaration { + /// Whether the declaration is a const declaration + pub fn is_const(&self) -> bool { + self.variable_kind() == Ok(JsVariableKind::Const) + } + pub fn variable_kind(&self) -> SyntaxResult { + let token_kind = self.kind_token().map(|t| t.kind())?; + + Ok(match token_kind { + T![const] => JsVariableKind::Const, + T![let] => JsVariableKind::Let, + T![var] => JsVariableKind::Var, + _ => unreachable!(), + }) + } +} #[cfg(test)] mod tests { use rome_js_factory::syntax::{JsSyntaxKind::*, JsVariableDeclaration}; From 9d0b0f9aedd53f3bb262f2d3b5eb7307ebb7b9fe Mon Sep 17 00:00:00 2001 From: Anchen Li Date: Thu, 17 Nov 2022 23:40:06 +1100 Subject: [PATCH 06/11] fix format --- crates/rome_js_syntax/src/stmt_ext.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/rome_js_syntax/src/stmt_ext.rs b/crates/rome_js_syntax/src/stmt_ext.rs index 80a0a5fa968..007b022a515 100644 --- a/crates/rome_js_syntax/src/stmt_ext.rs +++ b/crates/rome_js_syntax/src/stmt_ext.rs @@ -108,4 +108,4 @@ mod tests { assert!(var_decl.is_var()); } -} \ No newline at end of file +} From 389a562e734316bedc10135b35d2a567d16d6fbe Mon Sep 17 00:00:00 2001 From: Anchen Li Date: Fri, 18 Nov 2022 01:28:41 +1100 Subject: [PATCH 07/11] address comments --- .../nursery/no_const_assign.rs | 38 ++++++++++++------- 1 file changed, 24 insertions(+), 14 deletions(-) diff --git a/crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs b/crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs index fed8ce7e41c..d180b257510 100644 --- a/crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs +++ b/crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs @@ -3,8 +3,7 @@ use rome_analyze::context::RuleContext; use rome_analyze::{declare_rule, Rule, RuleDiagnostic}; use rome_console::markup; use rome_js_syntax::{ - JsAnyBindingPattern, JsArrayBindingPatternElementList, JsForVariableDeclaration, - JsIdentifierAssignment, JsObjectBindingPatternShorthandProperty, JsVariableDeclaration, + JsForVariableDeclaration, JsIdentifierAssignment, JsSyntaxKind::*, JsVariableDeclaration, JsVariableDeclarator, }; use rome_rowan::{AstNode, TextRange}; @@ -66,23 +65,34 @@ impl Rule for NoConstAssign { let declared_binding = model.declaration(node)?; - let js_any_binding_parent = declared_binding - .syntax() - .ancestors() - .find(|n| !JsAnyBindingPattern::can_cast(n.kind()))?; + if let Some(possible_declarator) = declared_binding.syntax().ancestors().find(|node| { + !matches!( + node.kind(), + JS_OBJECT_BINDING_PATTERN_SHORTHAND_PROPERTY + | JS_OBJECT_BINDING_PATTERN_PROPERTY_LIST + | JS_OBJECT_BINDING_PATTERN_PROPERTY + | JS_OBJECT_BINDING_PATTERN + | JS_ARRAY_BINDING_PATTERN_ELEMENT_LIST + | JS_ARRAY_BINDING_PATTERN + | JS_IDENTIFIER_BINDING + ) + }) { + if JsVariableDeclarator::can_cast(possible_declarator.kind()) { + let mut possible_declaration = possible_declarator.parent()?; + if possible_declaration.kind().is_list() { + possible_declaration = possible_declaration.parent()?; + } - let js_any_binding_parent_kind = js_any_binding_parent.kind(); - if JsVariableDeclarator::can_cast(js_any_binding_parent_kind) - || JsArrayBindingPatternElementList::can_cast(js_any_binding_parent_kind) - || JsObjectBindingPatternShorthandProperty::can_cast(js_any_binding_parent_kind) - { - for n in js_any_binding_parent.ancestors() { - if let Some(js_variable_declaration) = JsVariableDeclaration::cast_ref(&n) { + if let Some(js_variable_declaration) = + JsVariableDeclaration::cast_ref(&possible_declaration) + { if js_variable_declaration.is_const() { return Some(declared_binding.syntax().text_trimmed_range()); } } - if let Some(js_for_variable_declaration) = JsForVariableDeclaration::cast_ref(&n) { + if let Some(js_for_variable_declaration) = + JsForVariableDeclaration::cast_ref(&possible_declaration) + { if js_for_variable_declaration.is_const() { return Some(declared_binding.syntax().text_trimmed_range()); } From 6ebc51b80dd4a669c56e0f7656d4a4d78930721c Mon Sep 17 00:00:00 2001 From: Anchen Li Date: Fri, 18 Nov 2022 12:59:24 +1100 Subject: [PATCH 08/11] remove hardcoded binding check --- .../nursery/no_const_assign.rs | 19 ++++++------- .../tests/specs/nursery/noConstAssign.js | 3 ++ .../tests/specs/nursery/noConstAssign.js.snap | 28 ++++++++++++++++++- 3 files changed, 38 insertions(+), 12 deletions(-) diff --git a/crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs b/crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs index d180b257510..bb1fdc41cef 100644 --- a/crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs +++ b/crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs @@ -3,7 +3,9 @@ use rome_analyze::context::RuleContext; use rome_analyze::{declare_rule, Rule, RuleDiagnostic}; use rome_console::markup; use rome_js_syntax::{ - JsForVariableDeclaration, JsIdentifierAssignment, JsSyntaxKind::*, JsVariableDeclaration, + JsAnyArrayBindingPatternElement, JsAnyObjectBindingPatternMember, + JsArrayBindingPatternElementList, JsForVariableDeclaration, JsIdentifierAssignment, + JsIdentifierBinding, JsObjectBindingPatternPropertyList, JsVariableDeclaration, JsVariableDeclarator, }; use rome_rowan::{AstNode, TextRange}; @@ -66,16 +68,11 @@ impl Rule for NoConstAssign { let declared_binding = model.declaration(node)?; if let Some(possible_declarator) = declared_binding.syntax().ancestors().find(|node| { - !matches!( - node.kind(), - JS_OBJECT_BINDING_PATTERN_SHORTHAND_PROPERTY - | JS_OBJECT_BINDING_PATTERN_PROPERTY_LIST - | JS_OBJECT_BINDING_PATTERN_PROPERTY - | JS_OBJECT_BINDING_PATTERN - | JS_ARRAY_BINDING_PATTERN_ELEMENT_LIST - | JS_ARRAY_BINDING_PATTERN - | JS_IDENTIFIER_BINDING - ) + !JsAnyObjectBindingPatternMember::can_cast(node.kind()) + && !JsObjectBindingPatternPropertyList::can_cast(node.kind()) + && !JsAnyArrayBindingPatternElement::can_cast(node.kind()) + && !JsArrayBindingPatternElementList::can_cast(node.kind()) + && !JsIdentifierBinding::can_cast(node.kind()) }) { if JsVariableDeclarator::can_cast(possible_declarator.kind()) { let mut possible_declaration = possible_declarator.parent()?; diff --git a/crates/rome_js_analyze/tests/specs/nursery/noConstAssign.js b/crates/rome_js_analyze/tests/specs/nursery/noConstAssign.js index 3408c0d4003..a50191929be 100644 --- a/crates/rome_js_analyze/tests/specs/nursery/noConstAssign.js +++ b/crates/rome_js_analyze/tests/specs/nursery/noConstAssign.js @@ -50,3 +50,6 @@ for (const k in [1, 2]) { const [p, { q }] = [1, { q: 2 }]; p = 3; q = 4; + +const { r, ...rest } = s; +r = 4; diff --git a/crates/rome_js_analyze/tests/specs/nursery/noConstAssign.js.snap b/crates/rome_js_analyze/tests/specs/nursery/noConstAssign.js.snap index ca157d5f38a..8b9e129d69c 100644 --- a/crates/rome_js_analyze/tests/specs/nursery/noConstAssign.js.snap +++ b/crates/rome_js_analyze/tests/specs/nursery/noConstAssign.js.snap @@ -1,6 +1,6 @@ --- source: crates/rome_js_analyze/tests/spec_tests.rs -assertion_line: 99 +assertion_line: 73 expression: noConstAssign.js --- # Input @@ -58,6 +58,9 @@ const [p, { q }] = [1, { q: 2 }]; p = 3; q = 4; +const { r, ...rest } = s; +r = 4; + ``` # Diagnostics @@ -329,6 +332,7 @@ noConstAssign.js:52:1 lint/nursery/noConstAssign ━━━━━━━━━━ > 52 │ q = 4; │ ^ 53 │ + 54 │ const { r, ...rest } = s; i This is where the variable is defined as constant @@ -342,4 +346,26 @@ noConstAssign.js:52:1 lint/nursery/noConstAssign ━━━━━━━━━━ ``` +``` +noConstAssign.js:55:1 lint/nursery/noConstAssign ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Can't assign r because it's a constant + + 54 │ const { r, ...rest } = s; + > 55 │ r = 4; + │ ^ + 56 │ + + i This is where the variable is defined as constant + + 52 │ q = 4; + 53 │ + > 54 │ const { r, ...rest } = s; + │ ^ + 55 │ r = 4; + 56 │ + + +``` + From f420392500fdaa471a702b843daa1cc607420e42 Mon Sep 17 00:00:00 2001 From: Anchen Li Date: Fri, 18 Nov 2022 20:20:08 +1100 Subject: [PATCH 09/11] refactor variable declaration look up logic --- .../nursery/no_const_assign.rs | 23 +++++++++---------- 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs b/crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs index bb1fdc41cef..94a9fdd6320 100644 --- a/crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs +++ b/crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs @@ -6,7 +6,7 @@ use rome_js_syntax::{ JsAnyArrayBindingPatternElement, JsAnyObjectBindingPatternMember, JsArrayBindingPatternElementList, JsForVariableDeclaration, JsIdentifierAssignment, JsIdentifierBinding, JsObjectBindingPatternPropertyList, JsVariableDeclaration, - JsVariableDeclarator, + JsVariableDeclarator, JsVariableDeclaratorList, }; use rome_rowan::{AstNode, TextRange}; @@ -75,22 +75,21 @@ impl Rule for NoConstAssign { && !JsIdentifierBinding::can_cast(node.kind()) }) { if JsVariableDeclarator::can_cast(possible_declarator.kind()) { - let mut possible_declaration = possible_declarator.parent()?; - if possible_declaration.kind().is_list() { - possible_declaration = possible_declaration.parent()?; - } - - if let Some(js_variable_declaration) = - JsVariableDeclaration::cast_ref(&possible_declaration) + let possible_declaration = possible_declarator.parent()?; + if let Some(js_for_variable_declaration) = + JsForVariableDeclaration::cast_ref(&possible_declaration) { - if js_variable_declaration.is_const() { + if js_for_variable_declaration.is_const() { return Some(declared_binding.syntax().text_trimmed_range()); } } - if let Some(js_for_variable_declaration) = - JsForVariableDeclaration::cast_ref(&possible_declaration) + + if let Some(js_variable_declaration) = + JsVariableDeclaratorList::cast_ref(&possible_declaration) + .and_then(|declaration| declaration.syntax().parent()) + .and_then(JsVariableDeclaration::cast) { - if js_for_variable_declaration.is_const() { + if js_variable_declaration.is_const() { return Some(declared_binding.syntax().text_trimmed_range()); } } From ab91ea40616522e9150e7cd0a0c1dbf6987117b0 Mon Sep 17 00:00:00 2001 From: Anchen Date: Fri, 18 Nov 2022 20:52:25 +1100 Subject: [PATCH 10/11] Update crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs Co-authored-by: Micha Reiser --- .../src/semantic_analyzers/nursery/no_const_assign.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs b/crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs index 94a9fdd6320..f4add0c8d73 100644 --- a/crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs +++ b/crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs @@ -84,7 +84,7 @@ impl Rule for NoConstAssign { } } - if let Some(js_variable_declaration) = + else if let Some(js_variable_declaration) = JsVariableDeclaratorList::cast_ref(&possible_declaration) .and_then(|declaration| declaration.syntax().parent()) .and_then(JsVariableDeclaration::cast) From 7d921b47ecb58b362a05311f180a0d2fe11c0ff6 Mon Sep 17 00:00:00 2001 From: Anchen Li Date: Fri, 18 Nov 2022 20:55:40 +1100 Subject: [PATCH 11/11] fix lint --- .../src/semantic_analyzers/nursery/no_const_assign.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs b/crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs index f4add0c8d73..8368e57691d 100644 --- a/crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs +++ b/crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs @@ -82,9 +82,7 @@ impl Rule for NoConstAssign { if js_for_variable_declaration.is_const() { return Some(declared_binding.syntax().text_trimmed_range()); } - } - - else if let Some(js_variable_declaration) = + } else if let Some(js_variable_declaration) = JsVariableDeclaratorList::cast_ref(&possible_declaration) .and_then(|declaration| declaration.syntax().parent()) .and_then(JsVariableDeclaration::cast)