ai slop - #36754
Conversation
…orts link
Explicitly imported .d.ts/.d.mts/.d.cts files were transpiled like ordinary
TypeScript: every type-only export was elided from the module record, so
re-export chains across declaration files failed at link time with
"SyntaxError: export 'X' not found", and specifiers like ./foo.mjs that
only ship as foo.d.mts failed to resolve.
Declaration files now behave the way tsc treats them:
- The parser synthesizes var bindings (undefined) for module-scope type
aliases, interfaces, and declare statements, keeps type-marked
import/export specifiers as runtime ones, and keeps export type { } /
export type * from statements, so every declared name is a real export.
- The resolver prefers declaration-file siblings for relative runtime
specifiers when the importer is a declaration file, falls back to
.d.ts/.d.mts/.d.cts for missing .js/.mjs/.cjs files (the declaration half
of tsc's loadModuleFromFile behavior), and matches the "types" exports
condition for bare specifiers from declaration-file importers.
Runtime values defined in the imported declaration file itself are
preserved; values reached through a declaration-file graph are undefined,
matching the file's type-shim role.
Fixes #36751
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not a duplicate of #35605, though they overlap on the issue family. #35605 is the general fix for type-only re-exports in ordinary .ts files (attaching ModuleInfo to every runtime ESM transpile) and is blocked on a WebKit bump (oven-sh/WebKit#345, not in the current pin). This PR is scoped to declaration files only and covers things #35605 does not touch: resolving |
|
Updated 6:27 AM PT - Aug 2nd, 2026
❌ @robobun, your commit 5573aa7 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 36754That installs a local version of the PR into your bun-36754 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesTypeScript declaration files are detected by extension and receive dedicated parser and resolver behavior. The parser preserves type-only links and synthesizes runtime bindings. Resolution uses TypeScript declaration-file support
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bundler/bundle_v2.rs`:
- Around line 2197-2221: Make the resolver flag reset panic-safe at
src/bundler/bundle_v2.rs lines 2197-2221 and 6085-6100, and
src/jsc/VirtualMachine.rs lines 4085-4119. In each site, create a
scopeguard::guard immediately after setting
importer_is_type_script_declaration_file, have the guarded resolver call
(resolve, resolve_with_framework, or resolve_and_auto_install) return its result
directly, and remove the trailing plain reset; mirror the existing guard pattern
in jsc_hooks.rs::resolve.
In `@src/resolver/resolver.rs`:
- Around line 2690-2712: Extract the repeated condition-selection logic into a
Resolver helper, such as export_condition_for_kind, covering
require/require-resolve, style, and import kinds while respecting
importer_is_type_script_declaration_file. Replace the matching blocks at all
five identified call sites with the helper, adding a narrower helper only where
callers cannot receive At or AtConditional kinds.
In `@test/js/bun/typescript/declaration-files.test.ts`:
- Line 37: In test/js/bun/typescript/declaration-files.test.ts, replace the
partial stderr checks at lines 37-37, 65-65, 96-96, 118-119, 150-150, and
165-165 with strict empty-string assertions. At lines 181-184, add the same
stderr assertion before validating stdout, preserving each fixture’s existing
stdout assertions.
- Around line 101-121: Expand the declaration-file resolution test around the
existing relative .mjs case to cover every resolver mapping: .js resolving to a
.d.ts sibling, .js falling back to .d.mts when .d.ts is absent, and .cjs
resolving to a .d.cts sibling. Add fixtures and assertions for each case, using
a CommonJS entry fixture for the .cjs/.d.cts scenario while preserving the
existing success and export checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ada5f223-adce-4b86-88d3-fb371aedbd4f
📒 Files selected for processing (18)
src/ast/loader.rssrc/bundler/ParseTask.rssrc/bundler/bundle_v2.rssrc/bundler/options.rssrc/bundler/transpiler.rssrc/js_parser/p.rssrc/js_parser/parse/mod.rssrc/js_parser/parse/parse_entry.rssrc/js_parser/parse/parse_fn.rssrc/js_parser/parse/parse_import_export.rssrc/js_parser/parse/parse_skip_typescript.rssrc/js_parser/parse/parse_stmt.rssrc/js_parser/parse/parse_typescript.rssrc/jsc/VirtualMachine.rssrc/resolver/options.rssrc/resolver/resolver.rssrc/runtime/jsc_hooks.rstest/js/bun/typescript/declaration-files.test.ts
Extract the exports-condition selection into Conditions::for_kind, assert empty stderr in the declaration-file tests, and cover the .js/.cjs declaration-sibling mappings.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Class/interface declaration merging on the default export (a real default plus an elided type-only one, in either order) must keep the real default as the sole one. The stub decision now happens in synthesize_declaration_file_bindings, emitted only when no real default export exists. Pinned with merge fixtures in both orders.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/js_parser/parse/parse_stmt.rs:1644-1663— The fourthimport typesibling —import type foo = require(...)(theT::TEqualsarm at :1635-1644) — still setsopts.is_typescript_declare = trueunconditionally and elides toS::TypeScript{}, so in a.d.ctsimport type helper = require("./helper"); export { helper };leaveshelperunbound and the export clause is stripped, while the ESM equivalentimport type { helper } from "./helper"now works. Not a regression (pre-PR behavior identical), but per REVIEW.md's "fix the whole class — parallel switch arms" this is the oneimport typestatement form left out; gatingis_typescript_declare = trueon!(typescript_declaration_file && is_module_scope && !dts_suppress_type_name_recording)would let it fall through to the value path like the other three arms.Extended reasoning...
What the incompleteness is
This PR gives three of the four statement-level
import typeforms a declaration-file retention transform so the local binding exists for later re-exports:import type Foo from '...'— parse_stmt.rs:1647-1660 (theelseafter theT::TEqualscheck)import type * as Foo from '...'— parse_stmt.rs:1672-1687 (T::TAsteriskarm)import type { Foo } from '...'— parse_stmt.rs:1698-1715 (T::TOpenBracearm)
The fourth sibling in the same
match p.lexer.tokenblock —import type foo = require('...')/import type foo = bar.baz— is theT::TEqualsarm at :1635-1644, textually 9 lines above the first patched arm. It still unconditionally setsopts.is_typescript_declare = trueand callsparse_type_script_import_equals_stmt, with notypescript_declaration_file && opts.is_module_scope && !p.dts_suppress_type_name_recordinggate.Code path
For a
.d.ctscontaining:import type helper = require("./helper"); export { helper };
t_import→T::TIdentifierwithdefault_name == b"type"→ innerT::TIdentifier(identifier ≠from) → re-readdefault_name = "helper", advance past it.p.lexer.token == T::TEquals(:1635) →opts.is_typescript_declare = true(:1638) →parse_type_script_import_equals_stmt(loc, opts, ..., "helper").parse_type_script_import_equals_stmt(parse_typescript.rs:547-551) parses= require("./helper"), then hitsif opts.is_typescript_declare { return Ok(p.s(S::TypeScript {}, loc)); }— without callingdeclare_symbolforhelper, adding an import record, or callingrecord_declaration_file_type_name.export { helper }→t_export_clause_stmt→parse_export_clauseproduces anS::ExportClausewhose item carrieshelperviastore_name_in_refonly;helperis added todts_export_clause_aliasesbut that only downgrades a synthesized export — it doesn't create one.- End-of-parse:
synthesize_declaration_file_bindingswalksdts_type_name_order, which does not containhelper(never recorded), so novar helper;is emitted. - Visit pass,
s_export_clause:find_symbol("helper")resolves toUnbound→ the item is stripped → the export clause becomes empty. - An importer doing
import { helper } from "./api.d.cts"fails at link time withexport 'helper' not found.
Why existing code doesn't prevent it
record_declaration_file_type_nameis never called on this path — the early return inparse_type_script_import_equals_stmtfires before any symbol work.dts_export_clause_aliases(populated by step 4) is only consulted insidesynthesize_declaration_file_bindingsto decide whether an already-recorded name should be exported; it cannot create a binding for a name that was never recorded.- The
wrapper.d.ctstest fixture at declaration-files.test.ts:271 uses the value formimport helper = require("./helper.cjs")(notypekeyword), whereopts.is_typescript_declarestays false andparse_type_script_import_equals_stmtdeclareshelperas aConstantsymbol — so it doesn't exercise theimport type X = require(...)branch.
Step-by-step contrast with the ESM sibling
The equivalent that this PR does fix:
// api.d.cts import type { helper } from "./helper"; export { helper };
Here
T::TOpenBrace(:1698) seestypescript_declaration_file && is_module_scope && !dts_suppress_type_name_recording, builds a realS::Import, and callsprocess_import_statement, which declareshelperin scope.export { helper }then finds a bound symbol at visit time and keeps it. Same file, same re-export, only the import-equals form fails.Impact and severity
Not a regression — pre-PR,
import type X = require(...)was already elided andexport { X }was already stripped, so nothing that worked before is broken. This is a REVIEW.md "fix the whole class in the same PR — parallel switch arms" incompleteness: three of fourimport typestatement forms in the same match block got the declaration-file retention transform; the fourth did not.import type X = require('pkg')is the idiomatic type-namespace import in.d.ctsfiles (DefinitelyTyped's CommonJS declarations useimport type express = require('express')extensively), so the asymmetry is user-visible, but re-exporting that local via a bareexport { X }clause (rather thanexport = Xor using it type-position-only) is uncommon enough that this is a nit rather than blocking.Fix
Gate the
is_typescript_declare = trueon the same predicate as the three siblings:if p.lexer.token == T::TEquals { // "import type foo = require('bar');" // "import type foo = bar.baz;" if !(p.options.typescript_declaration_file && opts.is_module_scope && !p.dts_suppress_type_name_recording) { opts.is_typescript_declare = true; } return p.parse_type_script_import_equals_stmt(...); }
This lets
parse_type_script_import_equals_stmtfall through to its value path (declareshelperasSymbolKind::Constant, lowers toconst helper = require("./helper")), matching how the other threeimport typeforms are kept as runtime imports in declaration files. Alternatively, callp.record_declaration_file_type_name(default_name, opts.is_export)?before the return sohelperat least gets a synthesizedvar helper;— but that leaves itundefinedwhere the value form would give it the real require result, so the first option is more consistent.
export * as X from next to an exported type named X produced a duplicate
export; the star alias now joins dts_export_clause_aliases so the re-export
wins. import type x = require(...) records x for an undefined synthesized
binding (not the require result, since the namespace-path form would
evaluate a synthesized undefined) so export { x } clauses link.
export { X as default } or export { default } from next to a type-only
export default produced two exports named default; the stub now also
defers to a recorded default clause alias. Pinned in the default test.
export declare const X next to export { X } emitted two exports named X
because the declare-local conversion bypassed the clause-alias dedup.
Module-scope declare locals in declaration files now record their names
like every other type-only form and get their binding from
synthesize_declaration_file_bindings, which already defers to clause
aliases. The namespace-scope conversion is unchanged, and the
declare_binding module-scope arm is no longer needed. Pinned with a
redundant export { version } clause in the fixture.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js_parser/parse/parse_typescript.rs (1)
411-417: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGuard
local_type_names.putwith the samename_is_identifiercheck.
name_textis stale for the string-literal form ofdeclare module "foo" {}, per the comment at line 213-214. The newrecord_declaration_file_type_namecall at line 415-416 correctly skips recording whenname_is_identifieris false. Thep.local_type_names.put(name_text, true)?;call on line 413 does not have this guard and still runs unconditionally with the same potentially-stale value.Apply the same guard to both calls.
🐛 Proposed fix
p.pop_and_discard_scope(scope_index); if opts.is_module_scope { - p.local_type_names.put(name_text, true)?; if name_is_identifier { + p.local_type_names.put(name_text, true)?; p.record_declaration_file_type_name(name_text, opts.is_export)?; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/js_parser/parse/parse_typescript.rs` around lines 411 - 417, In the module-scope handling after p.pop_and_discard_scope, guard p.local_type_names.put(name_text, true)? with name_is_identifier, matching the existing guard around p.record_declaration_file_type_name. Ensure both operations run only for identifier-form declarations and skip string-literal declare module names.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/js_parser/parse/parse_typescript.rs`:
- Around line 411-417: In the module-scope handling after
p.pop_and_discard_scope, guard p.local_type_names.put(name_text, true)? with
name_is_identifier, matching the existing guard around
p.record_declaration_file_type_name. Ensure both operations run only for
identifier-form declarations and skip string-literal declare module names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f0c9c8e9-c88b-46b8-a4ab-57e197837e54
📒 Files selected for processing (4)
src/js_parser/p.rssrc/js_parser/parse/parse_stmt.rssrc/js_parser/parse/parse_typescript.rstest/js/bun/typescript/declaration-files.test.ts
declare module string names left stale lexer text in local_type_names; both bookkeeping calls now run only for identifier-form names.
A kept export type { Foo } clause next to a real export class Foo (or
enum, namespace, function, local) duplicated the exported name. The
synthesis pass now collects real exported declaration names and strips
matching clause items, one order-independent dedup site for every form.
Pinned with a real class plus redundant type clause in the fixture.
| } | ||
|
|
||
| /// tsc's `isDeclarationFileName`, minus the rare `.d.*.ts` form. | ||
| pub fn is_type_script_declaration_file(path: &[u8]) -> bool { |
There was a problem hiding this comment.
Is there some more canonical function that offers this behaviour? Module resolution is a complicated topic. The code that implements it must be extremely clear and easy to understand and read, and should be colocated. Maybe make a pull request before this one that improves and refactors and restructures many parts of module resolution; stacking this PR afterwards on top.
|
This seems to actually be the correct fix #35605 |
|
Agreed that #35605 is the more principled fix for the linking half: attaching ModuleInfo fixes type-only re-exports in every TypeScript file, not just declaration files, and it would handle this issue's repro including value preservation, with no declaration-file special-casing in the parser. It needs the WebKit-side switch arms in oven-sh/WebKit#345 before the runtime default can flip. What ModuleInfo does not cover is the resolution side this PR also touches, which only matters when a declaration entry is loaded explicitly: tsc-style declaration emit references Happy to go whichever way you prefer:
Also answered the regression question on the issue: the repro fails identically on 1.0.0, 1.2.0, and 1.3.0, so this never worked; details in #36751 (comment). |
|
This PR has been closed because it was flagged as AI slop. Many AI-generated PRs are fine, but this one was identified as having one or more of the following issues:
If you believe this was done in error, please leave a comment explaining why. |
github-actions
Bot
changed the title
|
Closing is fine per the review direction: #35605 (ModuleInfo on runtime ESM transpiles) is the principled fix for the type-only re-export linking this targeted, pending oven-sh/WebKit#345. The branch remains available if the resolver-side pieces (declaration-sibling resolution for .mjs specifiers inside declaration files, and the types exports condition) turn out to be wanted as a follow-up once that lands. |
2 participants
This PR has been marked as AI slop and the description has been updated to avoid confusion or misleading reviewers.
Many AI PRs are fine, but sometimes they submit a PR too early, fail to test if the problem is real, fail to reproduce the problem, or fail to test that the problem is fixed. If you think this PR is not AI slop, please leave a comment.