Repository navigation
Conversation
|
Warning Review limit reached
On-demand reviews are free for the next 23 days. After that, they cost $0.25 per reviewed file. Or wait 5 minutes for your next included review. View limit detailsLimit details: You’ve used all 5 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (26)
Comment |
|
Status: draft. It waits for a maintainer's answers to the "Questions for a maintainer" in the description: the default with no flag, the change with no setting, the names that Reproduced with bun 1.4.3 (367d939) and main 179664e, against tsc 6.0.2, esbuild 0.21.5 and Node 26.3:
The current head removes three defects of the push 86c20b3:
|
|
Updated 12:42 AM PT - Oct 26th, 2026
✅ @robobun, your commit 147707b1efb247009a70012020e3a308ee5cdd54 passed in 🧪 To try this PR locally: bunx bun-pr 40822That installs a local version of the PR into your bun-40822 --bun |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes TypeScript import-elision semantics across the parser, printer, resolver, and all four transpile entry points — including a documented user-visible behavior change under verbatimModuleSyntax: true — a human look would still be worthwhile.
What was reviewed:
TSUnusedImportFlagsthreading: resolver →ResultFlags(u8→u16) →ParseTask/ParseOptions/BundleOptions→ parser, plusBun.Transpilerand the runtime store/hooks — every construction site sets the field.- tsconfig parsing and
extendsmerge: the three settings areOption<bool>and merge base→child in the existing loop, so a child unset no longer clobbers an inherited"preserve";verbatimModuleSyntaxfolds to both bits. has_items_clauseonS::Import: distinguishesimport {}from bareimport; printer round-tripsimport {}/export {} fromand the minify-whitespacefromspacing check was updated to match;scan_importstreats an empty clause as "found" and clears the flag when it empties the clause itself.- Cache correctness: version bumped 27→28 and the flags are hashed into
hash_for_runtime_transpiler.
Extended reasoning...
Overview
This PR implements TypeScript's verbatimModuleSyntax, preserveValueImports, and importsNotUsedAsValues tsconfig settings, which control whether unused/type-only imports are elided. It introduces a TSUnusedImportFlags bitflags type and threads it from tsconfig parsing through the resolver (ResultFlags widened u8→u16), bundler ParseTask, parser Options, Bun.Transpiler, the runtime transpiler store, and jsc_hooks. The parser replaces the never-set preserve_unused_imports_ts boolean with the flags, drops had_type_only_imports bookkeeping from ImportClause, adds has_items_clause to S::Import so import {} from 'x' is distinguishable from import 'x', and updates scan_imports to honor KEEP_VALUES when not bundling/minifying-identifiers. The printer gains empty-brace round-tripping for both import {} and export {} from. The runtime transpiler cache version is bumped and the flags are added to the parser-options hash. 27 files, ~690 insertions.
Security risks
None identified. The change parses two additional boolean tsconfig fields and one already-parsed string enum, all of which gate compile-time elision decisions. No new untrusted-input parsing surface, no path handling, no allocation driven by external sizes, no auth/crypto.
Level of scrutiny
High. This is a semantic change to the TypeScript transpiler's import-elision behavior, ported from esbuild's model, and it applies across every transpile entry point (bun run, bun build, --no-bundle, Bun.Transpiler, scan-only). The PR itself calls out a user-visible behavior change: under verbatimModuleSyntax: true (which bun init emits), import { SomeType } from "./x" without type is now kept and will fail at runtime — correct per tsc (TS1484), but a change users will hit. Separately, import {} from 'x' and import { type T } from 'x' are now dropped in TypeScript by default where they were previously kept as bare imports. Both are correctness fixes toward tsc/esbuild parity, but they warrant a maintainer's sign-off on the compatibility trade-off. The extends-merge loop's variable naming (parent_config is actually the more-specific child being folded into the base accumulator mc) is pre-existing and the new lines follow the same pattern as use_define_for_class_fields, so the merge direction is correct.
Other factors
Test coverage is thorough: a new ts-unused-imports.test.ts matrix covers all four entry points × all three settings × extends inheritance, previously-dead esbuild tsconfig fixtures are moved above the early return and given real assertions, and transpiler.test.js gains a type as grammar table. The cache-key concern from REVIEW.md ("cache keys cover every input that shapes the output") is addressed — both the version bump and the hash addition are present. I checked that every S::Import construction site in the diff sets has_items_clause and that Default initializes it to false. No CODEOWNERS entries cover the changed paths. The bug hunt exited on dry_streak with no findings and no ruled-out candidates. Given the breadth (parser/printer/resolver/cache) and the intentional behavior change, this should get a human review rather than an automated approval.
|
Status: do not merge this PR as written. A rework is on the branch The bug is still on mainWith What is wrong with this PRI rebased it onto main and ran it.
What the rework branch does
Why it is not a PR yetTo honor the option changes which modules load in every
The last two rows are what the option means. tsc accepts both programs, and Node fails on them in the same way. Decisions for a maintainer
The rework continues after that decision. It still needs:
|
…NotUsedAsValues
A TypeScript import that no value uses is removed. tsconfig.json can ask
to keep it, and nothing read those settings. The parser now takes two
flags from the tsconfig.json that governs a file: KEEP_STMT
(`importsNotUsedAsValues: "preserve" | "error"`) keeps the statement as a
bare import, and KEEP_VALUES (`preserveValueImports`) keeps the unused
names. `verbatimModuleSyntax` sets both. The `extends` merge inherits each
setting.
The runtime and a bundle link what they print, so there the unused names
go and the statement stays. Only `bun build --no-bundle` prints the names.
Each file takes the flags of the tsconfig.json nearest to it. A file in
node_modules, a declaration file, and a file that becomes CommonJS at
runtime take none.
A TypeScript clause that binds no value (`import {} from "x"`,
`import { type T } from "x"`, `export {} from "x"`) is type-only and is
dropped unless KEEP_STMT is set. `S::Import::has_items_clause` records the
clause so that it prints as `import {} from "x"` when kept.
`parse_import_clause` follows esbuild's table for `{ type as ... }`.
The record of a statement that binds no value, or that sits in a `declare`
block, is created unused, and only the import scanner makes it live. The
bun:test global injection and the module type detection do not see it.
8f10198 to
64c765f
Compare
A JavaScript parse did work for flags that only a TypeScript parse reads: one record flag write per import statement, and the flag tests in the import scanner. The const generic of the parser removes them. - `process_import_statement` writes `IS_UNUSED` only for a TypeScript statement that starts unused. - `ImportScanner::scan` takes the flags as empty for JavaScript and clears `IS_UNUSED` only for TypeScript. - `S::Import` has a size assert: `has_items_clause` uses padding. New tests: a dev server bundle, a plugin's source at runtime and in `Bun.build`, and `bun build --tsconfig-override`.
…real directory
- `ImportRecordFlags::KEPT_BY_TSCONFIG` marks a TypeScript import that no
value uses and that tsconfig.json keeps. It replaces the provisional
`IS_UNUSED` of a kept clause with no value name. The `bun:test` global
injection ignores such a record, and the transpiler cache is dropped
for every `vitest` or `@jest/globals` record.
- A module statement inside a `declare` block creates no import record.
- The bundler reads the flags from the directory that a file is loaded
from (its real path), through `Resolver::unused_import_flags_ts`, as
the runtime does. A `Bun.build({ files })` entry and the second parse
of a "use client" file take flags too.
- A package that auto-install put in the global cache takes no flags.
With auto-install on, the flags keep no import of a bare specifier.
- A resolve error for a kept import has a note in place of the
`bun install` hint.
- `replace_exports` drops a kept statement only when a deleted export
used one of its names.
- `TSConfigJSON::unused_import_flags` computes the flags on read.
`bun repl` takes the top-level flags.
- `bun test` caches no output for a kept type-only import of `vitest`,
and each setting has its own cache key.
- A kept import makes a file an ES module in `bun run` and `bun build`.
- A workspace package linked into node_modules: `bun run`, one bundle
from `bun build`, and `--preserve-symlinks`.
- `Bun.build({ files })`, `Bun.build({ tsconfig })`, an override that
cannot be read, a package with two entry files, a module with no
extension or no directory, a path that a plugin resolved.
- The dev server after a save, and its module format in `bun build`.
- `bun build --no-bundle` with `--minify-identifiers`, and its output
when it runs.
- `Bun.Transpiler`: `transform()`, `declare` blocks, `exports.eliminate`.
- The evaluation order, the resolve errors and the note of `bun build`.
- Auto-install makes no request for a kept import, and a package in
the cache takes no setting.
…urn it off
- typescript.mdx: the module of a kept import must exist and runs at
the place of the statement, a browser bundle includes it, and
`bun lint --fix` rewrites `import { type T }` to `import type`.
The option comes from the tsconfig.json nearest to the directory
that a file is loaded from. The way to turn it off is the
tsconfig.json value. `--tsconfig-override` replaces every
tsconfig.json, and the dev server does not read it.
- The two older settings are unknown options for `bun check`.
- quickstart.mdx no longer adds the option to an existing project.
- Three lint pages say when `import {} from` and `import { type T }`
load the module.
`bun run` reports `Cannot find package 'x'` for an import that only tsconfig.json keeps. When the importer is a TypeScript file under such a tsconfig.json and only `@types/x` or `x.d.ts` exists, the error has a note that says so and names `import type`.
They ran in test/cli/run/run-autoinstall.test.ts, a file of 300 tests. They now run in test/bundler/transpiler/runtime-transpiler.test.ts with the rest of the matrix.
|
@dylan-conway this PR is a draft until you answer here. The description has the details under "Questions for a maintainer". The three that decide the shape:
This comment replaces the plan in #40822 (comment): the rework it names is now the head of this PR (0cf7e64), and it includes the change with no setting. |
The keys of `Bun.build({ files })` have forward slashes on every
platform. `Path::is_node_module` looks for the platform separator, so on
Windows an entry under node_modules took the tsconfig.json flags of the
working directory. The check now looks for `/node_modules/` in the key.
Related to #12744
Problem
verbatimModuleSyntax,preserveValueImportsandimportsNotUsedAsValuesdo nothing:preserve_unused_imports_tsisfalseat every site.import { unused } from "./b"never runs./b, unlike tsc and esbuild.import {} from "x"runsx, unlike tsc:scan_imports.rscannot tell an empty clause fromimport "x".Fix
bun run,bun testandbun buildlink their output, so they keepimport "./b"and drop the names.bun build --no-bundleprints them.ImportRecordFlags::KEPT_BY_TSCONFIGmarks an import that only tsconfig keeps.test/bundler/transpiler/runtime-transpiler.test.ts(67 of 83 new tests fail on main). Self-reviewed: 43 concerns raised, 32 addressed, 11 left to a maintainer (Notes).Background
verbatimModuleSyntax(set bybun init) keeps all butimport typeandtypenames.typethen fails to link.Downsides
verbatimModuleSyntax: trueproject now loads more modules.import { type Handler } from "aws-lambda"with only@types/aws-lambdainstalled fails withCannot find package, as in Node..text+10.8 KB. No new allocation or syscall.Notes
Status. Draft. It waits for a maintainer's answers to the questions below.
How this was found. By inspection and by comparison with tsc and esbuild, after a maintainer asked for esbuild's shape with no flag, the
import {} fromfix and the test matrix. No user report of a skipped side-effect import exists. #12744 asks for an error on a type imported withouttype. This PR does not add that error: the runtime and a bundle drop the unused name.Questions for a maintainer.
verbatimModuleSyntax: trueis in the tsconfig.json thatbun initwrites, so this PR changes which modules load in those projects with no flag. The section "What a kept import changes" lists the programs that run today and fail after it. If it must ship behind a switch, gate theverbatim_module_syntax == Some(true)arm ofTSConfigJSON::unused_import_flags. That switch makes the three keys inert. It does not bring back the behavior of main forimport {} from(question 2).import {} from "x",export {} from "x"andimport { type as } from "x"no longer runx. This follows tsc and esbuild.node file.tsruns them, and so does Bun today. It was part of the request, and js_parser: drop a TypeScript import whose clause binds nothing #41542 (closed in favor of this PR) held the same change. It has no opt-out other than a setting that keeps the statement. It can move to its own PR.bun build --no-bundleprints the unused names, as tsc and esbuild do. A type that is imported withouttypeis then a name with no export: the output fails when it runs (Export named 'SomeType' not found).bun runand a bundle drop such a name. The other choice is to drop the names in--no-bundletoo.import { type Handler } from "aws-lambda"downloads and runs the npm package of that name, which is not@types/aws-lambda.importsNotUsedAsValuesandpreserveValueImports. TypeScript 5.5 removed them andbun checkreports them as unknown options. The transpiler reads them because esbuild does.Reference matrix.
m.tsis one statement plus aconsole.log. "runs" means the module of the statement is evaluated. tsc 6.0.2 emit run in Node 26.3 and esbuild 0.21.5 agree on every cell. Main (179664e) matches 19 of 30 cells. This PR matches 30 of 30 inbun runand inbun build. Node's own type stripping (node m.ts) reads no tsconfig.json and runs the module in every row.{},verbatimModuleSyntax: false,isolatedModules: trueverbatimModuleSyntax: trueimport {} from "./p"export {} from "./p"import { type X } from "./p"import d, {} from "./p"import * as n from "./p"import "./p"Which tsconfig.json applies.
bun runandbun builduse one accessor,Resolver::unused_import_flags_ts. A workspace package that is linked into node_modules is loaded from its own directory, so it takes the setting in both. With--preserve-symlinksit is loaded from node_modules and takes none, as in esbuild.--tsconfig-overrideandBun.build({ tsconfig })replace the tsconfig.json of every directory.Bun.build({ files })entry,bun repl) takes the tsconfig.json of the working directory, and none for a path in node_modules.declareblock ask for no module. On mainbun buildfails fordeclare module "m" { import { T } from "./x" }(Could not resolve). That is fixed with and without the setting.module.exports) keeps nothing inbun run, as on main. A file with a kept import andrequire,__dirnameor "use strict" is an ES module, as it is forimport { T }.bun checkselects the tsconfig.json as tsc does (references, everyextendsform, on with no tsconfig.json). It can report TS1484 for a file that the runtime does not apply the setting to.What a kept import changes. Each row runs on main under
verbatimModuleSyntax: trueand differs after this PR. tsc's output does the same in Node.import { type Handler } from "aws-lambda", only@types/aws-lambdainstalledCannot find package 'aws-lambda', with a note that namesimport type.bun build:Could not resolve, with the note in place ofMaybe you need to "bun install"?import { type T } from "./types", onlytypes.d.tsexistsCannot find module './types', with the noteimport { type Config } from "./config"beforeimport "./setup"./configand its imports run before./setup. No errortype, used only as a type, in an import cycleCannot access 'Base' before initializationimport { type AppType } from "./server"./serverbun test --coverageA sample from the self-review, not repeated here: of 7 public projects that set the option, 5 have imports whose loading changes and 1 has an import that no longer resolves. The tests of elysia 1.4.28 pass with the option forced on.
Measurements. Release builds of main 179664e and of this PR (61e5fba, the source of the pushed head), linux-x64.
Bun__transpileFilebefore the transpile starts (gdb step count): +22 instructions per .js module (1382 to 1404), +43 per .ts module (1476 to 1519), +121 per .ts module underverbatimModuleSyntax(1481 to 1602).openat428 = 428,read823 = 823 (main = PR).bun buildwith 4 packages linked from an isolated store: DirInfo lookups 31 = 31, same list of opened paths, store directories opened 0 = 0.size_of, main to PR:S::Import48 to 48,ImportRecord96 to 96,resolver::Result296 to 296,ResultFlags1 to 2,DirInfo104 to 104,TSConfigJSON304 to 304,ParseTask408 to 408,ParseOptions432 to 440,TranspilerJob400 to 400..text+10,752 bytes (82,164,617 to 82,175,369), file +8,192 bytes.bun build --no-bundleon 20,000 import statements (instructions:u, 20 interleaved runs, sigma 0.02 per statement): +54.0 per TypeScript import statement (+48 forimport "x"), +1.0 per JavaScript import statement (+2 forimport "x"). A file with no import: 0. Main spends 6,600 instructions per import statement.bun build --no-bundleon each tracked file: 0 of 3,817 TypeScript files and 0 of 8,000 JavaScript files differ.verbatimModuleSyntax: true(136 TypeScript files): 8 files differ. 7 statements are newly kept and 1 name is newly printed. Each names a module that the program loads already (bun,react,svelte).scripts/builddoes not change.bun build --no-bundleagainst esbuild 0.21.5, 11 statements x 4 settings: 0 of 44 texts differ (main 22), apart from two spellings that main already has (import"j",export { } from).Bun.Transpiler:transformSync,scanandscanImportsdisagree in 0 of 132 cases (main 4).Order with open PRs.
parse_import_clause,TSConfigJSON,JSTranspiler.rsandbun.d.ts. The second to land takes version 45. It also repairs the double scan inP::to_ast: on main and here, a TypeScript file with one kept and one removedimport a = b.cscans its last statements twice, and an unused import among them still runs.extendsfollow package names, extension-less paths and arrays. Until it lands, only a relativeextendsthat ends in.jsoninherits these settings. After it, more projects take them.ParserOptions::init) change how a setting reaches the parser. The flags here are one more field on that path until one of them lands.KEPT_BY_TSCONFIGinImportRecordFlags: 12 open PRs add a flag at1 << 19(2026-10-26), and one of them lands first and keeps that bit. This PR takes1 << 31at its next push. Check first that main and no other open PR use that bit by then. Two different flags with one bit compile with no error and no warning, and each then reads as the other.Not done here.
bun runof a file that becomes CommonJS keeps nothing.bun buildkeeps the import there, as esbuild does.Bun.build({ files })with nested tsconfig.json files that disagree: afilesentry takes the working directory's.Bun.servedoes not read--tsconfig-override.bun testcaches no output for a file that importsvitestor@jest/globals, and now also not for a kept type-only import of them.bun rundoes cache it, sobun runof such a file and thenbun testcan load the package, as for a value import on main.bun initlabel and--minimalhelp text, the lint rule in the React templates, and test rows for--compile,--hotand HTML entries.Tests.
test/bundler/transpiler/runtime-transpiler.test.ts(the three settings inbun run,bun build,--no-bundle, withextends; each way to load a file, the dev server after a save; which tsconfig.json applies: nested, node_modules, override, plugin sources,files, a linked workspace package; CommonJS; the 6 x 5 statement matrix;declareblocks and declaration files; what a kept import asks for),test/bundler/transpiler/transpiler.test.js(Bun.Transpilertable withtransform(),declareblocks,exports.eliminate, thetype asgrammar),test/bundler/esbuild/tsconfig.test.ts(esbuild's fixtures, now asserted, and the dev server module format),test/cli/run/transpiler-cache.test.ts(cache key,bun testthenbun run),test/cli/run/run-autoinstall.test.ts(no request for a kept import, a package in the cache),test/integration/bun-types/fixture/bun.ts.Also run on the debug build, all pass:
test/bundler/esbuild/{ts,importstar_ts,importstar,dce,default}.test.ts,test/bundler/transpiler/{decorators,es-decorators,decorator-metadata,ts-use-define-for-class-fields,macro-test,assign-to-import,jsx-production}.test.ts,test/bundler/transpiler/export-default.test.js,test/bundler/{bundler_plugin,bundler_loader}.test.ts,test/js/bun/plugin/plugins.test.ts,test/bake/dev/{esm,bundle}.test.ts,test/js/bun/resolve/{resolve,resolve-error}.test.ts,test/cli/run/{run-autoinstall,autoinstall-cached-manifest}.test.ts,test/cli/test/bun-test.test.ts,test/js/bun/test/test-auto-import-jest-globals.test.js,test/internal/source-lints/.test/bundler/bun-build-api.test.tspasses but for one stress test that exceeds its 180 s limit on the debug build of this machine. That test takes 7.8 s on the release build and 7.6 s on main.no test proof · iteration 2 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/run/transpiler-cache.test.ts, test/bundler/transpiler/runtime-transpiler.test.ts