Repository navigation
Conversation
…ed code
Bun's parser leaves some early errors to the engine: it prints the code
and JavaScriptCore refuses the printed text. Where a pass removes the
code or prints it differently, nothing reports the error.
The test transpiles each program under `if (1) { }` and under
`if (0) { }` and asks the engine about the printed text.
erased-early-errors.txt lists the programs that are refused in one place
and accepted in the other. A second test loads each listed program as a
file in a child process.
The programs are the early-error lists of the AOT suite, read in place,
and a table in the test for the rules that those lists lack.
|
Status: the PR is ready for a maintainer. The test passes on a release build and a debug build of b1305dd. How to see what the table records, with a build of main: echo 'if (globalThis.never) { x = /(/; }' > kept.js && bun kept.js
# SyntaxError: Invalid regular expression: missing )
echo 'if (0) { x = /(/; } console.log("ran");' > removed.js && bun removed.js
# ranHow to run the test: CI: build 137962 is finished. |
|
No actionable comments were generated in the recent review. WalkthroughThe change adds a regression test for early-error behavior in JavaScript, CommonJS, and TypeScript snippets. It compares expected classifications with engine rejection checks and Bun’s results for retained and removed files. ChangesEarly-error comparison
Suggested reviewers: Merge Risk:
|
…st.ts The table has its own programs, each under the outcome that it has today: reported, erased under `if (0)`, erased where the code stays, or no early error. One child process loads each program in two files. The test reads no list of another suite, and it has no list file and no switch to write one.
|
The three review comments are addressed in de8915c, and each thread has the details.
The title and the description are updated to match. |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline nit, I also checked that vm.SourceTextModule parses eagerly in its constructor, so the isRefusedAsWritten oracle for .js rows is live; that the classifier throws on any unexpected error name and yields an outcome absent from the table (so a surprise fails toEqual rather than passing silently); and that the "use strict" hoisting in fileOf matches the three directive rows. The rewrite into runtime-transpiler.test.ts with an inline table resolves the earlier points about a separate file and the AOT-list coupling.
Extended reasoning...
Test-only change: 197 added lines in test/bundler/transpiler/runtime-transpiler.test.ts adding one test that spawns a single Bun process over ~200 generated files and compares a hard-coded four-way outcome table of ~100 early-error programs against observed behavior. It touches no production code and no security-sensitive surface. The table is an empirical snapshot of current Bun+JSC behavior that cannot be verified here without running the test, and a maintainer may want to weigh its maintenance cost on WebKit or parser changes, so a human look is still worthwhile.
…r table can't judge The table takes .js, .cjs and .ts lists. Any other extension is an error. A .ts program that loads in both files is an error too: the engine can't read it, so nothing says whether it has an early error.
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
The extension fall-through raised last round is addressed: unknown list extensions now throw in the row builder, and isRefusedAsWritten throws for .ts instead of filing the row under "has no early error". Beyond the inline nit, I also checked hermeticity (--no-install, harness tempDir, using registered before any assertion), that stdout/stderr/exited are drained together, and that vm.SourceTextModule exists in Bun's node:vm (src/js/node/vm.ts:479) — none of these are a problem. Since the table deliberately records current behaviour rather than desired behaviour, and the description itself poses a question for a maintainer about new.target under if (0), a maintainer's look is still worthwhile.
Extended reasoning...
The change adds one characterization test (+205 lines) to test/bundler/transpiler/runtime-transpiler.test.ts with no source changes and touches no security-sensitive surface. The prior-round classifier gap for .mjs/.cts rows is fixed in this push, and the remaining inline finding is a nit about the test passing under USE_SYSTEM_BUN=1. Deferring rather than approving because the table encodes a product position (which early errors Bun is allowed to erase) that the author explicitly asks a maintainer to decide.
The test fails when Bun reports an error less than its line says, or refuses a valid program. A change that reports more of these errors passes, so a fix does not fail the test before its lines are moved.
Behaviour change: none
Problem
if (0) { x = /(/; }runs.bun test --coverageturns dead code elimination off and refuses the same file.Fix
test/bundler/transpiler/runtime-transpiler.test.tsholds 95 programs. A childbunloads each program in two files: underif (0), and under a condition that Bun can't decide.if (0), and 16 where the code stays. The other 4 programs are valid controls.runtime-transpiler.test.tson release and debug builds of b1305dd. Self-reviewed: 9 concerns raised, 9 addressed, and the 5 review comments too.Background
bun run --debug-aotrefuses 67 of the 80 erased programs. 11 of the other 13 have an invalid RegExp literal.Notes
The table on main (95 programs)
if (0)accessors.jsassignment-targets.jsawait.jsclass-members.jsdeclarations.jsdelete.jsdelete.tslabels-and-jumps.jsmodules.jsnew.jsregexp-literals.jsreserved-names.jsreserved-names.tssloppy-code.cjsstrict-code.cjsusing.js/\P{assigned}/u;inregexp-literals.jsis the program of #15219.How an outcome is decided
if (globalThis.never) { <program> }andif (0) { <program> }. A"use strict";at the start of a program stays above theif.bun --no-installloads each file, a.cjsfile withrequireand any other file withimport(). The package has"type": "module"..jsfile as a module, a.cjsfile as the body of a CommonJS function. If it refuses the text, the error is erased where the code stays. The engine can't read TypeScript, so a.tsprogram that loads in both files is an error of the table.Other checks, outside the test
node --checkrefuses all 88.jsand.cjsprograms that have an early error. It accepts the 4 controls. The other 3 programs are TypeScript.bun run --debug-aotwas asked about each of the 80 erased programs in the file where it is erased. It stops on 67 with a TypeScript diagnostic, such as TS2392 for two constructors. It runs 13: the 11 with an invalid RegExp literal,"use strict"; x = 010;andfunction f() { "use strict"; with ({}) {} }. Sobun runandbun run --aotalready disagree on 67 of these files.Positions that are in the tree
test/aot/parallel/language/invalid-regexp-in-dropped-code.jshas to run under--aotas it does underbun.src/aot/lower/expression/array.rssays thatbun runnever parses a literal that its transpiler leaves out.new.targetoutside a function:src/js_parser/visit/visit_expr.rssays that the parser's error "is intentionally not emitted". The AOT front end refuses it, also underif (0).if (0) { new.target; }in a module, isbun run(it runs) orbun run --debug-aot(TS17013) right?Open PRs that change an outcome
var/letpattern and ofusingin afor (;;)head #47992 is about a pattern with no initializer (let [a];), Keep the parentheses of an optional chain undernewand as a template tag, and rejectnew a?.b()#41638 aboutnew a?.b(), and AOT: front end, test runs and tests for the rewritten compiler #47942 about a function-level"use strict". Each of them is about errors of this table. If one reports more of them, the test still passes, and its lines can move up.A floor, not an exact record
if (0), erased where the code stays. The test fails when a program is below its line, or when a valid program is refused.toEqualand passed.Earlier shapes of this PR
lint.test.tsandoxc-errors.test.ts. To share them, 13,400 lines move, andgit merge-treeshows thatbun check: TypeScript 7.1 #46383 and Reject ? and ! after an object literal property name like oxc #47762 then conflict.Bun.Transpiler. It kept the result in a list file. A program added to those lists would fail this test with no conflict for git to show. With the lists of AOT: front end, test runs and tests for the rewritten compiler #47942 it asked for six more lines. Programs with anexportwere never examined, because a block can't hold one.if (0) { module.exports = 1; } new.target;is a module underbun testand CommonJS underbun test --coverage.What the test doesn't find
let a; export { a as "\uD83D" };runs, and Node.js refuses it.Cost
[auto-merge] gate passed · iteration 1 · 1 files touched
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 0 rejected · iteration 1
evidence per changed file