Repository navigation
feat(analyze/js): implement noShadow
#5761
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| --- | ||
| "@biomejs/biome": minor | ||
| --- | ||
|
|
||
| Added new lint rule [`noShadow`](http://biome.dev/linter/rules/no-shadow), a port of eslint's `no-shadow`. | ||
|
|
||
| This rule disallows variable declarations from shadowing variables declared in an outer scope. For example: | ||
|
|
||
| ```js | ||
| const foo = 1; | ||
|
|
||
| function bar() { | ||
| const foo = 2; // This variable shadows the outer foo | ||
| } | ||
| ``` |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,227 @@ | ||
| use biome_analyze::{ | ||
| QueryMatch, Rule, RuleDiagnostic, RuleSource, context::RuleContext, declare_lint_rule, | ||
| }; | ||
| use biome_console::markup; | ||
| use biome_js_semantic::{Binding, SemanticModel}; | ||
| use biome_js_syntax::{ | ||
| JsClassExpression, JsFunctionExpression, JsIdentifierBinding, JsVariableDeclarator, | ||
| TsIdentifierBinding, TsTypeAliasDeclaration, | ||
| }; | ||
| use biome_rowan::{AstNode, SyntaxNodeCast, TokenText, declare_node_union}; | ||
|
|
||
| use crate::services::semantic::SemanticServices; | ||
|
|
||
| declare_lint_rule! { | ||
| /// Disallow variable declarations from shadowing variables declared in the outer scope. | ||
| /// | ||
| /// Shadowing is the process by which a local variable shares the same name as a variable in its containing scope. This can cause confusion while reading the code and make it impossible to access the global variable. | ||
| /// | ||
| /// See also: [`noShadowRestrictedNames`](http://biome.dev/linter/rules/no-shadow-restricted-names) | ||
| /// | ||
| /// ## Examples | ||
| /// | ||
| /// ### Invalid | ||
| /// | ||
| /// ```js,expect_diagnostic | ||
| /// const foo = "bar"; | ||
| /// if (true) { | ||
| /// const foo = "baz"; | ||
| /// } | ||
| /// ``` | ||
| /// | ||
| /// Variable declarations in functions can shadow variables in the outer scope: | ||
| /// | ||
| /// ```js,expect_diagnostic | ||
| /// const foo = "bar"; | ||
| /// const bar = function () { | ||
| /// const foo = 10; | ||
| /// } | ||
| /// ``` | ||
| /// | ||
| /// Function argument names can shadow variables in the outer scope: | ||
| /// | ||
| /// ```js,expect_diagnostic | ||
| /// const foo = "bar"; | ||
| /// function bar(foo) { | ||
| /// foo = 10; | ||
| /// } | ||
| /// ``` | ||
| /// | ||
| /// ### Valid | ||
| /// | ||
| /// ```js | ||
| /// const foo = "bar"; | ||
| /// if (true) { | ||
| /// const qux = "baz"; | ||
| /// } | ||
| /// ``` | ||
| /// | ||
| pub NoShadow { | ||
| version: "next", | ||
| name: "noShadow", | ||
| language: "js", | ||
| recommended: false, | ||
| sources: &[ | ||
| RuleSource::Eslint("no-shadow"), | ||
| // uncomment when we can handle the test cases from typescript-eslint | ||
| // RuleSource::EslintTypeScript("no-shadow"), | ||
| ], | ||
|
Comment on lines
+65
to
+68
Contributor
Author
There was a problem hiding this comment. I have excluded the unit tests from |
||
| } | ||
| } | ||
|
|
||
| pub struct ShadowedBinding { | ||
| /// The binding that is violating the rule. | ||
| binding: Binding, | ||
| /// The binding that is shadowed. | ||
| shadowed_binding: Binding, | ||
| } | ||
|
|
||
| impl Rule for NoShadow { | ||
| type Query = SemanticServices; | ||
| type State = ShadowedBinding; | ||
| type Signals = Box<[Self::State]>; | ||
| type Options = (); | ||
|
|
||
| fn run(ctx: &RuleContext<Self>) -> Self::Signals { | ||
| let mut shadowed_bindings = Vec::new(); | ||
| let model = ctx.query(); | ||
|
|
||
| for binding in ctx.query().all_bindings() { | ||
| if let Some(shadowed_binding) = check_shadowing(model, binding) { | ||
| shadowed_bindings.push(shadowed_binding); | ||
| } | ||
| } | ||
|
|
||
| shadowed_bindings.into_boxed_slice() | ||
| } | ||
|
|
||
| fn diagnostic(_ctx: &RuleContext<Self>, state: &Self::State) -> Option<RuleDiagnostic> { | ||
| // | ||
| // Read our guidelines to write great diagnostics: | ||
| // https://docs.rs/biome_analyze/latest/biome_analyze/#what-a-rule-should-say-to-the-user | ||
| // | ||
| Some( | ||
| RuleDiagnostic::new( | ||
| rule_category!(), | ||
| state.binding.tree().range(), | ||
| markup! { | ||
| "This variable shadows another variable with the same name in the outer scope." | ||
| }, | ||
| ) | ||
| .detail( | ||
| state.shadowed_binding.tree().range(), | ||
| markup!( | ||
| "This is the shadowed variable, which is now inaccessible in the inner scope." | ||
| ), | ||
| ) | ||
| .note(markup! { | ||
| "Consider renaming this variable. It's easy to confuse the origin of variables if they share the same name." | ||
| }), | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| fn check_shadowing(model: &SemanticModel, binding: Binding) -> Option<ShadowedBinding> { | ||
| let name = get_binding_name(&binding)?; | ||
|
|
||
| for upper in binding.scope().ancestors().skip(1) { | ||
| if let Some(upper_binding) = upper.get_binding(name.clone()) { | ||
| if binding.syntax() == upper_binding.syntax() { | ||
| // a binding can't shadow itself | ||
| continue; | ||
| } | ||
| if is_on_initializer(&binding, &upper_binding) { | ||
| continue; | ||
| } | ||
| if is_redeclaration(model, &binding, &upper_binding) { | ||
| // redeclarations are not shadowing, they get caught by `noRedeclare` | ||
| continue; | ||
| } | ||
| if is_declaration(&binding) | ||
| && is_declaration(&upper_binding) | ||
| && upper_binding.syntax().text_range().start() >= binding.scope().range().end() | ||
| { | ||
| // the shadowed binding must be declared before the shadowing one | ||
| continue; | ||
| } | ||
|
Comment on lines
+140
to
+146
Contributor
Author
There was a problem hiding this comment. This check is probably the most dubious one here because I don't have a source for it. I'm not entirely sure which part of the JS spec would be relevant. |
||
| return Some(ShadowedBinding { | ||
| binding, | ||
| shadowed_binding: upper_binding, | ||
| }); | ||
| } | ||
| } | ||
| None | ||
| } | ||
|
|
||
| fn get_binding_name(binding: &Binding) -> Option<TokenText> { | ||
| let node = binding.syntax(); | ||
| if let Some(ident) = node.clone().cast::<JsIdentifierBinding>() { | ||
| let name = ident.name_token().ok()?; | ||
| return Some(name.token_text_trimmed()); | ||
| } | ||
| if let Some(ident) = node.clone().cast::<TsIdentifierBinding>() { | ||
| let name = ident.name_token().ok()?; | ||
| return Some(name.token_text_trimmed()); | ||
| } | ||
| None | ||
| } | ||
|
|
||
| declare_node_union! { | ||
| pub(crate) AnyIdentifiableExpression = JsFunctionExpression | JsClassExpression | ||
| } | ||
|
|
||
| /// Checks if a variable `a` is inside the initializer of variable `b`. | ||
| /// | ||
| /// This is used to avoid false positives in cases like this: | ||
| /// ```js | ||
| /// const c = function c() {} | ||
| /// ``` | ||
| /// | ||
| /// But the rule should still trigger on these cases: | ||
| /// ```js | ||
| /// var a = function(a) {}; | ||
| /// ``` | ||
| /// | ||
| /// ```js | ||
| /// var a = function() { function a() {} }; | ||
| /// ``` | ||
| fn is_on_initializer(a: &Binding, b: &Binding) -> bool { | ||
| if let Some(b_initializer_expression) = b | ||
| .tree() | ||
| .parent::<JsVariableDeclarator>() | ||
| .and_then(|d| d.initializer()) | ||
| .and_then(|i| i.expression().ok()) | ||
| { | ||
| if let Some(a_parent) = a.tree().parent::<AnyIdentifiableExpression>() { | ||
| if a_parent.syntax() == b_initializer_expression.syntax() { | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| false | ||
| } | ||
|
|
||
| /// Checks if the binding `a` is a redeclaration of the binding `b`. Assumes that both bindings have the same identifier name. | ||
| fn is_redeclaration(model: &SemanticModel, a: &Binding, b: &Binding) -> bool { | ||
| if !is_declaration(a) || !is_declaration(b) { | ||
| return false; | ||
| } | ||
|
|
||
| let a_hoisted_scope = model.scope_hoisted_to(a.syntax()).unwrap_or(a.scope()); | ||
| let b_hoisted_scope = model.scope_hoisted_to(b.syntax()).unwrap_or(b.scope()); | ||
| a_hoisted_scope == b_hoisted_scope | ||
| } | ||
|
|
||
| /// Whether the binding is a declaration or not. | ||
| /// | ||
| /// Examples of declarations: | ||
| /// ```js | ||
| /// var a; | ||
| /// let b; | ||
| /// const c; | ||
| /// ``` | ||
| fn is_declaration(binding: &Binding) -> bool { | ||
| binding.tree().parent::<JsVariableDeclarator>().is_some() | ||
| || binding.tree().parent::<TsTypeAliasDeclaration>().is_some() | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| function a(x) { var b = function c() { var x = 'foo'; }; } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| --- | ||
| source: crates/biome_js_analyze/tests/spec_tests.rs | ||
| expression: invalid-01.js | ||
| --- | ||
| # Input | ||
| ```js | ||
| function a(x) { var b = function c() { var x = 'foo'; }; } | ||
|
|
||
| ``` | ||
|
|
||
| # Diagnostics | ||
| ``` | ||
| invalid-01.js:1:44 lint/nursery/noShadow ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ | ||
|
|
||
| i This variable shadows another variable with the same name in the outer scope. | ||
|
|
||
| > 1 │ function a(x) { var b = function c() { var x = 'foo'; }; } | ||
| │ ^ | ||
| 2 │ | ||
|
|
||
| i This is the shadowed variable, which is now inaccessible in the inner scope. | ||
|
|
||
| > 1 │ function a(x) { var b = function c() { var x = 'foo'; }; } | ||
| │ ^ | ||
| 2 │ | ||
|
|
||
| i Consider renaming this variable. It's easy to confuse the origin of variables if they share the same name. | ||
|
|
||
|
|
||
| ``` |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| var a = (x) => { var b = () => { var x = 'foo'; }; } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| --- | ||
| source: crates/biome_js_analyze/tests/spec_tests.rs | ||
| expression: invalid-02.js | ||
| --- | ||
| # Input | ||
| ```js | ||
| var a = (x) => { var b = () => { var x = 'foo'; }; } | ||
|
|
||
| ``` | ||
|
|
||
| # Diagnostics | ||
| ``` | ||
| invalid-02.js:1:38 lint/nursery/noShadow ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ | ||
|
|
||
| i This variable shadows another variable with the same name in the outer scope. | ||
|
|
||
| > 1 │ var a = (x) => { var b = () => { var x = 'foo'; }; } | ||
| │ ^ | ||
| 2 │ | ||
|
|
||
| i This is the shadowed variable, which is now inaccessible in the inner scope. | ||
|
|
||
| > 1 │ var a = (x) => { var b = () => { var x = 'foo'; }; } | ||
| │ ^ | ||
| 2 │ | ||
|
|
||
| i Consider renaming this variable. It's easy to confuse the origin of variables if they share the same name. | ||
|
|
||
|
|
||
| ``` |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| function a(x) { var b = function () { var x = 'foo'; }; } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| --- | ||
| source: crates/biome_js_analyze/tests/spec_tests.rs | ||
| expression: invalid-03.js | ||
| --- | ||
| # Input | ||
| ```js | ||
| function a(x) { var b = function () { var x = 'foo'; }; } | ||
|
|
||
| ``` | ||
|
|
||
| # Diagnostics | ||
| ``` | ||
| invalid-03.js:1:43 lint/nursery/noShadow ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ | ||
|
|
||
| i This variable shadows another variable with the same name in the outer scope. | ||
|
|
||
| > 1 │ function a(x) { var b = function () { var x = 'foo'; }; } | ||
| │ ^ | ||
| 2 │ | ||
|
|
||
| i This is the shadowed variable, which is now inaccessible in the inner scope. | ||
|
|
||
| > 1 │ function a(x) { var b = function () { var x = 'foo'; }; } | ||
| │ ^ | ||
| 2 │ | ||
|
|
||
| i Consider renaming this variable. It's easy to confuse the origin of variables if they share the same name. | ||
|
|
||
|
|
||
| ``` |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.