mirror of
https://github.com/oxc-project/oxc.git
synced 2026-09-14 19:36:11 +08:00
Hamody We
f6b22654d2
Closes #25339. ## The problem `no-redeclare` gated the entire rule on the file not being an ES module: ```rust fn should_run(&self, ctx: &ContextHost) -> bool { // ES modules run in their own scope, and don't conflict with existing globals !ctx.source_type().is_module() } ``` The comment justifies the globals half of the rule, and it is right about that: an ES module has its own top-level scope, so declaring `Object` or `self` there shadows nothing, which is why ESLint reports `builtinGlobals` redeclarations against the global scope rather than inside a module. But the gate took the rest with it — plain `var a = 3; var a = 10;`, redeclarations in nested scopes, and the TS type/value collision check added for #10396 all stopped running too. **One correction to the issue's framing, from testing it rather than reading it.** `.ts` is parsed as `ModuleKind::Unambiguous`, which resolves to `Module` only when ESM syntax is actually present. So a `.ts` file with no `import`/`export` is a script, and the rule *did* run there — `var a = 3; var a = 10;` on its own is reported today. What was dead is every `.ts` file that contains an import or an export, which in practice is nearly all of them. The effect the issue describes is real; the boundary is ESM syntax, not the extension. The rule's own tests hid this either way. Every failing case runs under `.change_rule_path_extension(".cts")` — CommonJS, never a module — and the single `.ts` block asserts a *pass*, which it would do whether or not the rule executed. ## The change Drop `should_run`, and narrow the exemption to the thing the comment actually describes: ```rust // ES modules run in their own scope, so a top-level `Object` or `self` shadows nothing. // Only the globals half of the rule is skipped for them -- redeclaring a local binding is // still a redeclaration. let builtin_globals = if self.builtin_globals && !ctx.source_type().is_module() { Some(&GLOBALS_BUILTIN) } else { None }; ``` `builtin_globals` is already the single switch for that half — it drives `is_builtin`, which chooses between `no_redeclare_as_builtin_in_diagnostic` and the plain redeclaration diagnostic — so `None` in modules disables exactly the globals reporting, for the builtin table and user-configured `globals` alike, and leaves both redeclaration loops running. ## Tests A new `.ts` block under the `es-modules` snapshot suffix. Every case carries ESM syntax so it resolves as a module — otherwise it would test the script path by accident, which is the trap above: - **pass** — `export {}; var Object = 0;` and `export {}; var globalThis = 0;` (a module shadows nothing, so these must stay quiet), plus a redeclaration-free case and a TS overload pair. - **fail** — `export {}; var a = 3; var a = 10;`; `export {}; function f() { var a; var a; }` (nested scope — nothing to do with global scope at all, and previously silent); the #10396 `type foo = 1; export function foo(): void; …` case; and `export {}; var Object = 0; var Object = 0;`. That last one is the seam this change creates, so it is worth pinning: inside a module the first `var Object` is no longer reported as shadowing a builtin, but the second is still reported as a redeclaration of the first. Without it, narrowing the gate could quietly have turned builtin-named symbols into a blind spot. The existing `.cts` blocks and their snapshot are untouched — `.cts` is CommonJS, so `is_module()` was already false there. ## Verification Ran locally with `cargo test -p oxc_linter --lib no_redeclare`: passes, the existing `eslint_no_redeclare.snap` is unchanged, and the new `eslint_no_redeclare@es-modules.snap` is included. `cargo clippy -p oxc_linter --lib` is clean. > AI disclosure, per the contributing policy: I used an AI assistant while investigating and writing this. I have read and understand the change, ran the tests myself, and stand behind it. --------- Co-authored-by: Cameron <cameron.clark@hey.com>