Report duplicate declarations for import bindings kept in TypeScript output#31240
Conversation
…output TypeScript allows a later declaration to take over the name of an import binding because the import may be type-only. When the import is not elided (Bun.Transpiler keeps unused imports by default), both bindings were printed, producing output that fails to re-parse. Record the collision in declare_symbol and report "has already been declared" from ImportScanner when the import binding survives import elision.
|
Warning Review limit reached
Your plan currently allows 2 reviews/hour. Refill in 24 minutes and 10 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe PR adds a TypeScript-only import-binding redeclaration check in the parser and three regression tests: two covering redeclaration vs. elided imports for the transpiler, and one ensuring a user-imported JSX Fragment isn't conflated with the auto-imported helper. ChangesImport Re-declaration Detection
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Updated 6:33 AM PT - May 23rd, 2026
❌ @robobun, your commit 23bc6fb has 2 failures in
Add 🧪 To try this PR locally: bunx bun-pr 31240That installs a local version of the PR into your bun-31240 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Delete your slop cmoments.
|
Removed in b955b25. |
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Is there a chaeper way to do this? Can you delete your slop commetns/
|
Done in 7cad1b1 — dropped the side table. The scanner now compares each kept import binding against the module-scope member for its name; if the member no longer points at the binding, the name was re-declared. No new field on the parser, no work in declare_symbol, nothing for non-TypeScript files, and the remaining comments went with it. Same errors, same tests. |
Generated symbols (the JSX runtime auto-imports in bundle mode) reuse raw names and take over the scope member without replacing the import, which false-positived the duplicate check on valid TSX.
What
Parser fuzzing found two inputs where the transpiler's own output fails to re-parse (both are duplicate-binding early errors per spec, and errors in
tsc):Cause
Scope::can_merge_symbol_kindsdeliberately lets any later declaration take over the name of an import binding when TypeScript is enabled (the import may be type-only, e.g.import type-less interface imports, namespace declaration merging). That lenience is only sound when the shadowed import is later elided as unused. The runtime and the bundler always trim unused imports for TS, butBun.Transpilerdefaults to keeping them, so the shadowed import binding was printed right next to the declaration that replaced it — output that declares the same name twice.Fix
In
ImportScanner::scan(src/js_parser/scan/scan_imports.rs), after unused-import elision (which runs post-visit, once use counts are known), each import binding still in the statement whose symbol was replaced (itslinkis set byReplaceWithNew) is checked against the module scope's member for that name; when the member points at the final symbol in that chain of replacements, it's reported as"X" has already been declaredat the re-declaration (the member's location) with a note at the import. Compiler-generated symbols that reuse a name (the JSX runtime auto-imports in bundle mode) don't set that link, so they can't trip the check. No parser state is added,declare_symbolis untouched, and non-TypeScript files are unaffected.So the error fires when a re-declared import binding is kept in the output, and the existing lenience is preserved whenever the import is actually elided:
import{Observable}from""+import{Observable} from "x"(imports kept)import { Foo } from "."; export class Foo {}(imports kept)trimUnusedImports: true(runtime/bundler default for TS)import type { Foo } from "x"; class Foo {}class Foo {}import { type Foo, Bar } from "x"; class Foo {}import { Foo } from "x"; declare class Foo {}/ interface / overload-only signaturesimport { Foo } from "x"; namespace Foo {}at runtime (valid TS declaration merging)import { Foo } from "x"; import Foo = Bar.Baz(unused, imports kept)Bun.Transpiler.scan()with a TS loader now also reports the error for these inputs (it already did for JS loaders);scanImports()is unchanged.Verification
bun bd test test/bundler/transpiler/transpiler.test.js— new testsre-declaring an import binding that is kept in the output is an error/re-declaring an elided import binding is allowedpass; the error-case test fails on a build without the fix.test/bundler/transpiler/,test/js/bun/transpiler/,test/bundler/esbuild/ts.test.ts,test/bundler/esbuild/importstar_ts.test.ts, andtest/js/bun/resolve/import-defer.test.tspass.bun buildbehavior on the repro files is unchanged (shadowed imports still elided there).