Derive decorator options from tsconfig in scanImports and the REPL - #33384
Derive decorator options from tsconfig in scanImports and the REPL#33384robobun wants to merge 2 commits into
Conversation
… the REPL scanImports() and the REPL built their parser options from scratch and left features.standard_decorators at its false default, so both rejected `accessor` fields and (outside TypeScript) standard decorators that transformSync() and scan() accept. For TypeScript they also parsed the legacy decorator grammar even when the tsconfig asked for the standard one. Add RuntimeFeatures::standard_decorators_for() as the single source of truth for the decorator dialect and use it at every option-building site.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
WalkthroughA new helper function ChangesStandard decorators consistency
Sequence Diagram(s)sequenceDiagram
participant ParseEntryPoint as ParseTask/Transpiler/scanImports/REPL
participant RuntimeFeatures as Runtime::Features
ParseEntryPoint->>RuntimeFeatures: standard_decorators_for(loader, experimental_decorators, emit_decorator_metadata)
RuntimeFeatures-->>ParseEntryPoint: enabled: bool
ParseEntryPoint->>ParseEntryPoint: set opts.features.standard_decorators = enabled
Compact Metadata Poem 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 1:31 PM PT - Jul 5th, 2026
✅ @robobun, your commit f9ce1f1e2ca4964d39033914ad57a9ad8c96bfc2 passed in 🧪 To try this PR locally: bunx bun-pr 33384That installs a local version of the PR into your bun-33384 --bun |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
LGTM — clean extraction of the decorator-dialect predicate into one helper, wired into the two call sites that were missing it, with thorough test coverage.
Extended reasoning...
Overview
This PR fixes Bun.Transpiler#scanImports and bun repl rejecting standard decorators and accessor fields that transformSync accepts. Root cause: both build ParserOptions directly and never set features.standard_decorators (defaults to false), while the full transpile path derives it from tsconfig. The fix extracts the existing inline predicate into RuntimeFeatures::standard_decorators_for(loader, experimental_decorators, emit_decorator_metadata) and calls it from all four sites: the two existing bundler paths (behavior-preserving refactor — the function body is byte-identical to the inline expression it replaces) plus JSTranspiler::scan_imports and Repl::transform_for_repl (the actual bug fixes). The REPL also now propagates emit_decorator_metadata, which it was previously dropping.
Security risks
None. This is pure parser-option plumbing — a boolean derived from already-loaded tsconfig/CLI state and assigned to a features struct. No new I/O, no untrusted input parsing, no unsafe blocks, no memory management changes.
Level of scrutiny
Low-to-moderate. The refactor portion is mechanically verifiable (same expression, moved). The fix portion adds one field assignment at each of two well-understood option-construction sites, reading from the same config sources the sibling code paths (get_parse_result, vm.transpiler.options) already use. I confirmed js_ast in transpiler.rs aliases bun_js_parser so the helper path resolves, and that vm.transpiler.options.{experimental_decorators,emit_decorator_metadata} exist on BundleOptions.
Other factors
- 11 new transpiler tests (
describe.eachover all four loaders, plus@dec()()dialect-discriminator tests both with and withoutexperimentalDecorators, each cross-checked againstscan()) and 2 new REPL tests (accessor lowering; tsconfig-driven legacy metadata). Tests assert on evaluation output rather than echoed input, and the PR description states they fail on main. - No CODEOWNER-covered files touched.
- No outstanding human review comments; only bot noise (CodeRabbit rate-limit notice, robobun build link).
- The bug-hunting system found nothing.
- The PR description explicitly addresses interaction with #29201 and designed the dialect tests to remain valid if that lands.
There was a problem hiding this comment.
LGTM — clean option-plumbing fix that extracts the duplicated standard_decorators expression into one helper and wires it into the two sites that were missing it.
Extended reasoning...
Overview
This PR fixes Bun.Transpiler#scanImports and bun repl rejecting valid standard-decorator / accessor syntax that transformSync accepts. It adds RuntimeFeatures::standard_decorators_for(loader, experimental, emit_metadata) in src/js_parser/parser.rs as the single source of truth for the boolean, then calls it from four sites: two existing (bundler/transpiler.rs, bundler/ParseTask.rs — pure refactors of the identical inline expression) and two new (JSTranspiler::scan_imports, Repl::transform_for_repl). The REPL also picks up emit_decorator_metadata, which it was previously dropping. Tests are added for all four loaders in transpiler.test.js (cross-checking scanImports against scan) and two REPL tests covering both the accessor-lowering and the tsconfig-selects-legacy-grammar paths.
Security risks
None. The change only threads existing tsconfig-derived booleans into parser feature flags; no new I/O, no untrusted-input parsing, no auth/crypto/permissions surface.
Level of scrutiny
Low-to-medium. The two bundler-side edits are provably behavior-preserving (same expression, extracted to a function). The two new call sites only affect which decorator grammar the fast-path import scanner and the REPL parser accept — developer-facing surfaces with no correctness impact on emitted code beyond aligning with what transformSync already does. The helper's placement on Runtime::Features and its re-export via RuntimeFeatures follow the file's existing conventions; the js_ast:: vs js_parser:: path difference across call sites resolves to the same type via the crate's alias (use bun_js_parser as js_ast in transpiler.rs).
Other factors
Test coverage is thorough: describe.each over ts/tsx/js/jsx, both scanImports and scan asserted for consistency, and a @dec()() discriminator to prove which dialect is selected with and without experimentalDecorators. The REPL tests use tempDir + cwd to isolate tsconfig, and assert on evaluation output rather than echoed input. No CODEOWNERS cover the touched files. No prior human reviews or outstanding comments. The bug hunter found nothing.
CI statusThe diff is green. On The single red check, The earlier build (68600) had two macOS timeouts, Neither is reachable from this change. The diff is five files of parser-option plumbing: a boolean derived from tsconfig, threaded into I am not pushing another retrigger commit. Happy to rebase or re-run on request. Recap for review
13 tests added (11 in |
Repro
It is wider than
accessor. With thejs/jsxloaders,scanImportsalso rejects plain standard decorators:And for TypeScript it silently used the legacy decorator grammar even with no
experimentalDecorators, so@dec()() class C {}scanned fine but failed to transpile.bun replhas the same bug from the same cause:class C { accessor x = 1 }fails the internal parse, falls back to raw evaluation, and JavaScriptCore reportsSyntaxError: Unexpected identifier 'x'.Cause
JSTranspiler::scan_importsand the REPL'stransform_for_replboth build their parser options withParserOptions::init(jsx, loader)and never touchfeatures.standard_decoratorsdefaults tofalse, and the parser gates theaccessorkeyword and JS-side@decoratorson it (parse_property.rs,parse_stmt.rs,parse_typescript.rs,parse_prefix.rs).transformSync/scango throughTranspiler::parse, which derives the flag from the tsconfig. The fast import scanner never did, so it accepted and rejected a different language than the transpiler it fronts.Fix
Add
RuntimeFeatures::standard_decorators_for(loader, experimental_decorators, emit_decorator_metadata)as the single source of truth for which decorator dialect to parse, and call it from every site that builds parser options:src/bundler/transpiler.rs,src/bundler/ParseTask.rs— the two existing copies of the expression, unchanged behavior.src/runtime/api/JSTranspiler.rs—scanImportsnow reads the sameconfig.tsconfigthatget_parse_resultdoes.src/runtime/cli/repl.rs— the REPL readsvm.transpiler.options, and also wires upemit_decorator_metadata, which it was dropping.scanImportsnow accepts exactly whattransformSyncaccepts, and the tsconfig still selects the dialect: underexperimentalDecoratorsthe legacy grammar stays in force.Verification
test/bundler/transpiler/transpiler.test.js—accessorfields, decorators on classes, and decorators on class members across thets/tsx/js/jsxloaders, each asserted againstscan()as well. Plus@dec()()(legal in the legacy grammar, illegal in the standard one) to pin down which dialect is selected with and withoutexperimentalDecorators.test/js/bun/repl/repl.test.ts—accessorevaluates in the REPL, and a tsconfig withexperimentalDecorators+emitDecoratorMetadatastill gets the legacy lowering with design-time metadata.11 new transpiler tests and 2 new REPL tests fail on
mainand pass with this change.Related
#29201 proposes removing the
standard_decoratorsgate onaccessorentirely (esbuild and tsc accept the keyword underexperimentalDecorators). That is a separate parser-level fix; this PR is about the option plumbing, and the two do not overlap in the files they touch. The tests here deliberately use@dec()()rather thanaccessorto pin the decorator dialect, so they stay correct if #29201 lands.