Skip to content

Commit 6f15060

Browse files
antoinezanardiantoinezanardi
authored
refactor(eslint/block-scoped-var): clean up implementation and improve documentation (#13417)
## Refactor and Standardize: `eslint/block-scoped-var` This PR refactors the implementation of the `block-scoped-var` rule to make the codebase more **idiomatic Rust**, improving readability, maintainability, and alignment with Rust best practices. In addition, the rule documentation has been **standardized and enhanced** following the documentation skeleton described in #13389. ### Changes included * Refactored rule logic with clearer naming, early returns, and idiomatic Rust patterns (`if let`, `Option`, `Iterator`, etc.). * Improved structure for better readability and maintainability. * Standardized rule documentation for consistency across the linter rules. > [!NOTE] > This PR is focused solely on refactoring and documentation; no test behavior is modified.
1 parent 52c92b5 commit 6f15060

File tree

1 file changed

+146
-51
lines changed

1 file changed

+146
-51
lines changed

‎crates/oxc_linter/src/rules/eslint/block_scoped_var.rs‎

Lines changed: 146 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,18 @@
1-
use oxc_ast::{AstKind, ast::VariableDeclarationKind};
1+
use crate::{AstNode, context::LintContext, rule::Rule};
2+
use oxc_ast::{
3+
AstKind,
4+
ast::{BindingPattern, VariableDeclarationKind},
5+
};
26
use oxc_diagnostics::OxcDiagnostic;
37
use oxc_macros::declare_oxc_lint;
48
use oxc_span::{GetSpan, Span};
9+
use oxc_syntax::{scope::ScopeId, symbol::SymbolId};
510

6-
use crate::{AstNode, context::LintContext, rule::Rule};
7-
8-
fn redeclaration_diagnostic(decl_span: Span, redecl_span: Span, name: &str) -> OxcDiagnostic {
11+
fn redeclaration_diagnostic(decl_span: Span, redeclare_span: Span, name: &str) -> OxcDiagnostic {
912
OxcDiagnostic::warn(format!("'{name}' is used outside of binding context."))
1013
.with_help(format!("Variable '{name}' is used outside its declaration block. Declare it outside the block or use 'let'/'const'."))
1114
.with_labels([
12-
redecl_span.label("it is redeclared here"),
15+
redeclare_span.label("it is redeclared here"),
1316
decl_span.label(format!("'{name}' is first declared here")),
1417
])
1518
}
@@ -28,36 +31,96 @@ pub struct BlockScopedVar;
2831
declare_oxc_lint!(
2932
/// ### What it does
3033
///
31-
/// Generates warnings when variables are used outside of the block in which they were defined.
32-
/// This emulates C-style block scope.
34+
/// Enforces that variables are both **declared** and **used** within the same block scope.
35+
/// This rule prevents accidental use of variables outside their intended block, mimicking C-style block scoping in JavaScript.
3336
///
3437
/// ### Why is this bad?
3538
///
36-
/// This rule aims to reduce the usage of variables outside of their binding context
37-
/// and emulate traditional block scope from other languages.
38-
/// This is to help newcomers to the language avoid difficult bugs with variable hoisting.
39+
/// JavaScript’s `var` declarations are hoisted to the top of their enclosing function, which can cause variables declared in a block (e.g., inside an `if` or `for`) to be accessible outside of it.
40+
/// This can lead to hard-to-find bugs.
41+
/// By enforcing block scoping, this rule helps avoid hoisting issues and aligns more closely with how other languages treat block variables.
42+
///
43+
/// ### Options
44+
///
45+
/// No options available for this rule.
3946
///
4047
/// ### Examples
4148
///
4249
/// Examples of **incorrect** code for this rule:
4350
/// ```js
51+
/// /* block-scoped-var: "error" */
52+
///
4453
/// function doIf() {
4554
/// if (true) {
4655
/// var build = true;
4756
/// }
4857
/// console.log(build);
4958
/// }
59+
///
60+
/// function doLoop() {
61+
/// for (var i = 0; i < 10; i++) {
62+
/// // do something
63+
/// }
64+
/// console.log(i); // i is accessible here
65+
/// }
66+
///
67+
/// function doSomething() {
68+
/// if (true) {
69+
/// var foo = 1;
70+
/// }
71+
/// if (false) {
72+
/// foo = 2;
73+
/// }
74+
/// }
75+
///
76+
/// function doTry() {
77+
/// try {
78+
/// var foo = 1;
79+
/// } catch (e) {
80+
/// console.log(foo);
81+
/// }
82+
/// }
83+
///
5084
/// ```
5185
///
5286
/// Examples of **correct** code for this rule:
5387
/// ```js
88+
/// /* block-scoped-var: "error" */
89+
///
5490
/// function doIf() {
5591
/// var build;
5692
/// if (true) {
5793
/// build = true;
58-
/// }
94+
/// }
5995
/// console.log(build);
6096
/// }
97+
///
98+
/// function doLoop() {
99+
/// var i;
100+
/// for (i = 0; i < 10; i++) {
101+
/// // do something
102+
/// }
103+
/// console.log(i);
104+
/// }
105+
///
106+
/// function doSomething() {
107+
/// var foo;
108+
/// if (true) {
109+
/// foo = 1;
110+
/// }
111+
/// if (false) {
112+
/// foo = 2;
113+
/// }
114+
/// }
115+
///
116+
/// function doTry() {
117+
/// var foo;
118+
/// try {
119+
/// foo = 1;
120+
/// } catch (e) {
121+
/// console.log(foo);
122+
/// }
123+
/// }
61124
/// ```
62125
BlockScopedVar,
63126
eslint,
@@ -72,54 +135,86 @@ impl Rule for BlockScopedVar {
72135
if decl.kind != VariableDeclarationKind::Var {
73136
return;
74137
}
75-
let cur_node_scope_id = node.scope_id();
76-
if !ctx.scoping().scope_flags(cur_node_scope_id).is_strict_mode() {
138+
let scope_id = node.scope_id();
139+
if !ctx.scoping().scope_flags(scope_id).is_strict_mode() {
77140
return;
78141
}
79-
// `scope_arr` contains all the scopes that are children of the current scope
142+
// `scope_ids` contains all the scopes that are children of the current scope
80143
// we should eliminate all of them
81-
let scope_arr = ctx.scoping().iter_all_scope_child_ids(node.scope_id()).collect::<Vec<_>>();
144+
let scope_ids = ctx.scoping().iter_all_scope_child_ids(node.scope_id()).collect::<Vec<_>>();
82145

83-
let declarations = &decl.declarations;
84-
for item in declarations {
85-
let id = &item.id;
86-
// e.g. "var [a, b] = [1, 2]"
87-
for ident in id.get_binding_identifiers() {
88-
let name = ident.name.as_str();
89-
let Some(symbol_id) = ctx.scoping().find_binding(node.scope_id(), name) else {
90-
continue;
91-
};
92-
// e.g. "if (true} { var a = 4; } else { var a = 4; }"
93-
// in this case we can't find the reference of 'a' by call `get_resolved_references`
94-
// so i use `symbol_redeclarations` to find all the redeclarations
95-
for redeclaration in ctx.scoping().symbol_redeclarations(symbol_id) {
96-
let re_scope_id = ctx.nodes().get_node(redeclaration.declaration).scope_id();
97-
if !scope_arr.contains(&re_scope_id) && re_scope_id != cur_node_scope_id {
98-
ctx.diagnostic(redeclaration_diagnostic(
99-
item.id.span(),
100-
redeclaration.span,
101-
name,
102-
));
103-
}
104-
}
105-
// e.g. "var a = 4; console.log(a);"
106-
for reference in ctx.scoping().get_resolved_references(symbol_id) {
107-
let reference_scope_id = ctx.nodes().get_node(reference.node_id()).scope_id();
108-
if !scope_arr.contains(&reference_scope_id)
109-
&& reference_scope_id != cur_node_scope_id
110-
{
111-
ctx.diagnostic(use_outside_scope_diagnostic(
112-
item.id.span(),
113-
ctx.reference_span(reference),
114-
name,
115-
));
116-
}
117-
}
118-
}
146+
for item in &decl.declarations {
147+
run_for_declaration(&item.id, &scope_ids, node, ctx);
119148
}
120149
}
121150
}
122151

152+
fn run_for_all_references(
153+
(pattern, name, symbol): (&BindingPattern, &str, &SymbolId),
154+
scope_ids: &[ScopeId],
155+
node: &AstNode,
156+
ctx: &LintContext,
157+
) {
158+
ctx.scoping()
159+
.get_resolved_references(*symbol)
160+
.filter(|reference| {
161+
let reference_scope_id = ctx.nodes().get_node(reference.node_id()).scope_id();
162+
163+
reference_scope_id != node.scope_id() && !scope_ids.contains(&reference_scope_id)
164+
})
165+
.for_each(|reference| {
166+
ctx.diagnostic(use_outside_scope_diagnostic(
167+
pattern.span(),
168+
ctx.reference_span(reference),
169+
name,
170+
));
171+
});
172+
}
173+
174+
fn run_for_all_redeclarations(
175+
(pattern, name, symbol): (&BindingPattern, &str, &SymbolId),
176+
scope_ids: &[ScopeId],
177+
node: &AstNode,
178+
ctx: &LintContext,
179+
) {
180+
ctx.scoping()
181+
.symbol_redeclarations(*symbol)
182+
.iter()
183+
.filter(|redeclaration| {
184+
let redeclare_scope_id = ctx.nodes().get_node(redeclaration.declaration).scope_id();
185+
186+
redeclare_scope_id != node.scope_id() && !scope_ids.contains(&redeclare_scope_id)
187+
})
188+
.for_each(|redeclaration| {
189+
ctx.diagnostic(redeclaration_diagnostic(pattern.span(), redeclaration.span, name));
190+
});
191+
}
192+
193+
fn run_for_declaration(
194+
pattern: &BindingPattern,
195+
scope_ids: &[ScopeId],
196+
node: &AstNode,
197+
ctx: &LintContext,
198+
) {
199+
// e.g. "var [a, b] = [1, 2]"
200+
for ident in pattern.get_binding_identifiers() {
201+
let name = ident.name.as_str();
202+
let Some(symbol) = ctx.scoping().find_binding(node.scope_id(), name) else {
203+
continue;
204+
};
205+
206+
let binding = (pattern, name, &symbol);
207+
208+
// e.g. "if (true) { var a = 4; } else { var a = 4; }"
209+
// in this case we can't find the reference of 'a' by call `get_resolved_references`
210+
// so I use `symbol_redeclarations` to find all the redeclarations
211+
run_for_all_redeclarations(binding, scope_ids, node, ctx);
212+
213+
// e.g. "var a = 4; console.log(a);"
214+
run_for_all_references(binding, scope_ids, node, ctx);
215+
}
216+
}
217+
123218
#[test]
124219
fn test() {
125220
use crate::tester::Tester;

0 commit comments

Comments
 (0)