Skip to content

Commit 402d8b7

Browse files
shulaodashulaoda
committed
refactor(linter): improve eslint/no-redeclare (#9976)
Related to #8237 - Improve the documentation - ~~Align diagnostic format with `eslint`~~ - Fix errors exposed after adding test cases
1 parent ff13be6 commit 402d8b7

File tree

2 files changed

+96
-55
lines changed

2 files changed

+96
-55
lines changed

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

+55-52
Original file line numberDiff line numberDiff line change
@@ -1,28 +1,21 @@
1-
use oxc_ast::{
2-
AstKind,
3-
ast::{BindingIdentifier, BindingPatternKind},
4-
};
1+
use javascript_globals::GLOBALS;
52
use oxc_diagnostics::OxcDiagnostic;
63
use oxc_macros::declare_oxc_lint;
74
use oxc_span::Span;
85
use oxc_syntax::symbol::SymbolId;
96

107
use crate::{context::LintContext, rule::Rule};
118

12-
fn no_redeclare_diagnostic(id_name: &str, decl_span: Span, re_decl_span: Span) -> OxcDiagnostic {
13-
OxcDiagnostic::warn(format!("'{id_name}' is already defined.")).with_labels([
14-
decl_span.label(format!("'{id_name}' is already defined.")),
9+
fn no_redeclare_diagnostic(name: &str, decl_span: Span, re_decl_span: Span) -> OxcDiagnostic {
10+
OxcDiagnostic::warn(format!("'{name}' is already defined.")).with_labels([
11+
decl_span.label(format!("'{name}' is already defined.")),
1512
re_decl_span.label("It can not be redeclare here."),
1613
])
1714
}
1815

19-
fn no_redeclare_as_builtin_in_diagnostic(builtin_name: &str, span: Span) -> OxcDiagnostic {
20-
OxcDiagnostic::warn(format!(
21-
"'{builtin_name}' is already defined as a built-in global variable."
22-
))
23-
.with_label(
24-
span.label(format!("'{builtin_name}' is already defined as a built-in global variable.")),
25-
)
16+
fn no_redeclare_as_builtin_in_diagnostic(name: &str, span: Span) -> OxcDiagnostic {
17+
OxcDiagnostic::warn(format!("'{name}' is already defined as a built-in global variable."))
18+
.with_label(span)
2619
}
2720

2821
#[derive(Debug, Default, Clone)]
@@ -33,17 +26,35 @@ pub struct NoRedeclare {
3326
declare_oxc_lint!(
3427
/// ### What it does
3528
///
36-
/// Disallow variable redeclaration
29+
/// This rule disallows redeclaring variables within the same scope, ensuring that each variable
30+
/// is declared only once. It helps avoid confusion and unintended behavior in code.
3731
///
3832
/// ### Why is this bad?
3933
///
40-
/// n JavaScript, it’s possible to redeclare the same variable name using var. This can lead to confusion as to where the variable is actually declared and initialized.
34+
/// Redeclaring variables in the same scope can lead to unexpected behavior, overwriting existing values,
35+
/// and making the code harder to understand and maintain.
4136
///
42-
/// ### Example
37+
/// ### Examples
38+
///
39+
/// Examples of **incorrect** code for this rule:
4340
/// ```javascript
4441
/// var a = 3;
4542
/// var a = 10;
4643
/// ```
44+
///
45+
/// Examples of **correct** code for this rule:
46+
/// ```javascript
47+
/// var a = 3;
48+
/// a = 10;
49+
/// ```
50+
///
51+
/// ### Options
52+
///
53+
/// #### builtinGlobals
54+
///
55+
/// `{ type: bool, default: false }`
56+
///
57+
/// When set `true`, it flags redeclaring built-in globals (e.g., `let Object = 1;`).
4758
NoRedeclare,
4859
eslint,
4960
pedantic
@@ -61,40 +72,22 @@ impl Rule for NoRedeclare {
6172
}
6273

6374
fn run_on_symbol(&self, symbol_id: SymbolId, ctx: &LintContext) {
64-
let symbol_table = ctx.scoping();
65-
let decl_node_id = symbol_table.symbol_declaration(symbol_id);
66-
match ctx.nodes().kind(decl_node_id) {
67-
AstKind::VariableDeclarator(var) => {
68-
if let BindingPatternKind::BindingIdentifier(ident) = &var.id.kind {
69-
let symbol_name = symbol_table.symbol_name(symbol_id);
70-
if symbol_name == ident.name.as_str() {
71-
for span in ctx.scoping().symbol_redeclarations(symbol_id) {
72-
self.report_diagnostic(ctx, *span, ident);
73-
}
74-
}
75-
}
76-
}
77-
AstKind::FormalParameter(param) => {
78-
if let BindingPatternKind::BindingIdentifier(ident) = &param.pattern.kind {
79-
let symbol_name = symbol_table.symbol_name(symbol_id);
80-
if symbol_name == ident.name.as_str() {
81-
for span in ctx.scoping().symbol_redeclarations(symbol_id) {
82-
self.report_diagnostic(ctx, *span, ident);
83-
}
84-
}
85-
}
86-
}
87-
_ => {}
75+
let name = ctx.scoping().symbol_name(symbol_id);
76+
let is_builtin = self.built_in_globals
77+
&& (GLOBALS["builtin"].contains_key(name) || ctx.globals().is_enabled(name));
78+
79+
let decl_span = ctx.scoping().symbol_span(symbol_id);
80+
81+
if is_builtin {
82+
ctx.diagnostic(no_redeclare_as_builtin_in_diagnostic(name, decl_span));
8883
}
89-
}
90-
}
9184

92-
impl NoRedeclare {
93-
fn report_diagnostic(&self, ctx: &LintContext, span: Span, ident: &BindingIdentifier) {
94-
if self.built_in_globals && ctx.env_contains_var(&ident.name) {
95-
ctx.diagnostic(no_redeclare_as_builtin_in_diagnostic(ident.name.as_str(), ident.span));
96-
} else {
97-
ctx.diagnostic(no_redeclare_diagnostic(ident.name.as_str(), ident.span, span));
85+
for span in ctx.scoping().symbol_redeclarations(symbol_id) {
86+
if is_builtin {
87+
ctx.diagnostic(no_redeclare_as_builtin_in_diagnostic(name, *span));
88+
} else {
89+
ctx.diagnostic(no_redeclare_diagnostic(name, decl_span, *span));
90+
}
9891
}
9992
}
10093
}
@@ -144,21 +137,31 @@ fn test() {
144137
("class C { static { var a; { var a; } } }", None),
145138
("class C { static { { var a; } var a; } }", None),
146139
("class C { static { { var a; } { var a; } } }", None),
147-
// ("var Object = 0;", Some(serde_json::json!([{ "builtinGlobals": true }]))),
140+
(
141+
"var Object = 0; var Object = 0; var globalThis = 0;",
142+
Some(serde_json::json!([{ "builtinGlobals": true }])),
143+
),
148144
(
149145
"var a; var {a = 0, b: Object = 0} = {};",
150146
Some(serde_json::json!([{ "builtinGlobals": true }])),
151147
),
152-
// ("var globalThis = 0;", Some(serde_json::json!([{ "builtinGlobals": true }]))),
153148
(
154149
"var a; var {a = 0, b: globalThis = 0} = {};",
155150
Some(serde_json::json!([{ "builtinGlobals": true }])),
156151
),
157152
("function f() { var a; var a; }", None),
158-
("function f(a) { var a; }", None),
153+
("function f(a, b = 1) { var a; var b;}", None),
159154
("function f() { var a; if (test) { var a; } }", None),
160155
("for (var a, a;;);", None),
161156
];
162157

163158
Tester::new(NoRedeclare::NAME, NoRedeclare::PLUGIN, pass, fail).test_and_snapshot();
159+
160+
let fail = vec![(
161+
"var foo;",
162+
Some(serde_json::json!([{ "builtinGlobals": true }])),
163+
Some(serde_json::json!({ "globals": { "foo": false }})),
164+
)];
165+
166+
Tester::new(NoRedeclare::NAME, NoRedeclare::PLUGIN, vec![], fail).test();
164167
}

‎crates/oxc_linter/src/snapshots/eslint_no_redeclare.snap

+41-3
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,24 @@ source: crates/oxc_linter/src/tester.rs
123123
· ╰── 'a' is already defined.
124124
╰────
125125

126+
⚠ eslint(no-redeclare): 'Object' is already defined as a built-in global variable.
127+
╭─[no_redeclare.tsx:1:5]
128+
1 │ var Object = 0; var Object = 0; var globalThis = 0;
129+
· ──────
130+
╰────
131+
132+
⚠ eslint(no-redeclare): 'Object' is already defined as a built-in global variable.
133+
╭─[no_redeclare.tsx:1:21]
134+
1 │ var Object = 0; var Object = 0; var globalThis = 0;
135+
· ──────
136+
╰────
137+
138+
⚠ eslint(no-redeclare): 'globalThis' is already defined as a built-in global variable.
139+
╭─[no_redeclare.tsx:1:37]
140+
1 │ var Object = 0; var Object = 0; var globalThis = 0;
141+
· ──────────
142+
╰────
143+
126144
⚠ eslint(no-redeclare): 'a' is already defined.
127145
╭─[no_redeclare.tsx:1:5]
128146
1 │ var a; var {a = 0, b: Object = 0} = {};
@@ -131,6 +149,12 @@ source: crates/oxc_linter/src/tester.rs
131149
· ╰── 'a' is already defined.
132150
╰────
133151

152+
⚠ eslint(no-redeclare): 'Object' is already defined as a built-in global variable.
153+
╭─[no_redeclare.tsx:1:23]
154+
1 │ var a; var {a = 0, b: Object = 0} = {};
155+
· ──────
156+
╰────
157+
134158
⚠ eslint(no-redeclare): 'a' is already defined.
135159
╭─[no_redeclare.tsx:1:5]
136160
1 │ var a; var {a = 0, b: globalThis = 0} = {};
@@ -139,6 +163,12 @@ source: crates/oxc_linter/src/tester.rs
139163
· ╰── 'a' is already defined.
140164
╰────
141165

166+
⚠ eslint(no-redeclare): 'globalThis' is already defined as a built-in global variable.
167+
╭─[no_redeclare.tsx:1:23]
168+
1 │ var a; var {a = 0, b: globalThis = 0} = {};
169+
· ──────────
170+
╰────
171+
142172
⚠ eslint(no-redeclare): 'a' is already defined.
143173
╭─[no_redeclare.tsx:1:20]
144174
1 │ function f() { var a; var a; }
@@ -149,12 +179,20 @@ source: crates/oxc_linter/src/tester.rs
149179

150180
⚠ eslint(no-redeclare): 'a' is already defined.
151181
╭─[no_redeclare.tsx:1:12]
152-
1 │ function f(a) { var a; }
153-
· ┬ ┬
154-
· │ ╰── It can not be redeclare here.
182+
1 │ function f(a, b = 1) { var a; var b;}
183+
· ┬ ┬
184+
· │ ╰── It can not be redeclare here.
155185
· ╰── 'a' is already defined.
156186
╰────
157187

188+
⚠ eslint(no-redeclare): 'b' is already defined.
189+
╭─[no_redeclare.tsx:1:15]
190+
1 │ function f(a, b = 1) { var a; var b;}
191+
· ┬ ┬
192+
· │ ╰── It can not be redeclare here.
193+
· ╰── 'b' is already defined.
194+
╰────
195+
158196
⚠ eslint(no-redeclare): 'a' is already defined.
159197
╭─[no_redeclare.tsx:1:20]
160198
1 │ function f() { var a; if (test) { var a; } }

0 commit comments

Comments
 (0)