Repository navigation
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 7 minutes. View limit details
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe change adds log truncation and tracks log message counts in lexer snapshots. Parser snapshots no longer manage log state separately. TypeScript parsing now checks ChangesTypeScript lookahead and log restoration
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly summarizes both primary changes: lexer log truncation during restore and correct handling of out as a TypeScript type-parameter name. |
| Description check | |
The description provides detailed problem context, implementation details, verification coverage, performance measurements, known limitations, and follow-up items. It does not use the template heading… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
Full details: Description check
-
Explanation
The description provides detailed problem context, implementation details, verification coverage, performance measurements, known limitations, and follow-up items. It does not use the template headings exactly, but it includes the required change summary and verification information.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
|
Updated 10:05 PM PT - Oct 21st, 2026
❌ @robobun, your commit d49116d has some failures in 🧪 To try this PR locally: bunx bun-pr 45964That installs a local version of the PR into your bun-45964 --bun |
|
Status How this was reproduced, on release builds of main (116dd64 and bc1030a): With this branch both commands print PR: #45964 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
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 findings, I also checked the counter arithmetic in the new Log::truncate (src/ast/lib.rs) — every add_* path that pushes a Kind::Err/Kind::Warn message bumps the matching counter first, so the drain stays balanced and does not underflow; and the Err-propagating (stack/OOM) exit of the lexer_backtracker_* helpers — the messages dropped by the rewind there accompany an error that already aborts the parse, so nothing user-visible is lost.
Extended reasoning...
The change adds Log::truncate, makes Lexer::restore truncate the log to its snapshotted length (replacing ParserSnapshot's own three log fields and bunfig's hand-rolled drain), and narrows when out is read as a type-parameter modifier in parse_skip_typescript.rs, with new transpiler tests. It touches the TypeScript parser's speculative backtracking, which is behavior-sensitive but not a security surface. Two confirmed findings are posted inline (a TSX-only lost "Invalid identifier" diagnostic, and the variance-annotation lists still accepting out before a newline), so a human look is already signalled; this note only records the two additional concerns that were examined and ruled out.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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 checked the follow-up commit (e29539d): is_ts_arrow_fn_jsx now fails the parse when its scan logged an error, and the tsx test asserts Invalid identifier: "0a" rather than parity with the ts loader. I also checked the ParserSnapshot log-field removal for equivalence — every Log::add_* path increments the counter matching the pushed Kind, so Log::truncate recomputing by kind matches the old saved-counter restore, and is_log_disabled/prev_error_loc are already in LexerSnapshot — and confirmed every other rewind site (both backtracker helpers, next_token_matches, look_ahead_next_token_is_open_paren_or_less_than_or_dot, the conditional-arrow parser snapshot) disables lexer logging, so the new Err(SyntaxError) cannot fire inside a nested attempt. Since this changes which TS inputs parse (and deliberately diverges from esbuild on out), a maintainer's look at the semantic choice is still worthwhile.
Extended reasoning...
The change touches the JS/TS parser's speculative-parse rewind machinery (src/js_parser/lexer.rs, p.rs, typescript.rs, parse/parse_skip_typescript.rs), adds Log::truncate in src/ast/lib.rs reused by src/bunfig/arguments.rs, and changes how out is read in type parameter lists; it touches no security-sensitive surface. The second commit addresses the TSX lookahead issue raised in the prior review, and the refactor of ParserSnapshot's log restore is behavior-preserving by inspection. It is not approved outright because it is a semantic parser change with a stated divergence from esbuild and a noted behavior change for valid JS like a << out > (b.c) > (c), which a human maintainer should weigh.
There was a problem hiding this comment.
Thanks — the pushes since my last pass address both of my earlier notes (the TSX arrow lookahead now keeps its lexer error, with a test, and the two JSX cases assert the full printed output). I reviewed this version and did not find any bugs; because it changes how every lexer rewind in the core parser treats the log and how out is read in type parameter lists, a human look is still worthwhile.
What was reviewed:
- Log restoration refactor: every
errors/warningsincrement in src/ast/lib.rs pairs with anadd_msgpush of the same kind andadd_msgnever drops a message, so the per-message decrement inLog::truncateyields the same counters the oldParserSnapshotassigned back;Lexer::log()andP::log()alias the sameLog, so parser-logged messages are still rewound. - The new early
Errinis_ts_arrow_fn_jsx: it fires only for a lexer error inside the<...>tokens, which the following attempt would re-read with logging disabled, so the error is neither lost nor doubled. - The
outrule:word_follows_on_same_lineruns only when variance annotations are disallowed, so type/interface/class lists keep their previous behavior; the tests cover ts/tsx and the scan, scanImports, transform and transformSync APIs.
Extended reasoning...
The change touches the core TypeScript parser in src/js_parser (lexer snapshot/restore, the parser snapshot, type parameter skipping and the TSX arrow lookahead), adds Log::truncate in src/ast/lib.rs, reuses it in src/bunfig/arguments.rs, and adds about 210 lines of transpiler tests. It touches no authentication, cryptographic or injection-sensitive surface. The log counter refactor was verified behavior-preserving by reading every increment site, and the author's new commits address both prior inline notes with tests. It still warrants deferral rather than approval because the rewind semantics apply to all six lexer backtracking sites in a hot, widely exercised parsing path, and one prior note about variance-allowed lists was resolved by the author without a code change.
|
I worked on the same failure from a separate report. That report was 1. A check with tests that were written without this PR The branch in point 3 has 9 tests for this bug. All 9 fail on canary 1.4.3-canary.1+367d939d9. All 9 pass on main (91e7e3b) with the source change of 0c12add applied. 2. Valid inputs with Without the fix, each of these fails with x = { mask: 1 << pin, in: "a", out: "b" };
x = { a: 1 << b, in() {} };
enum E { A = 1 << B, in = 2 }
x = a < <in T/>; // .tsxThe first two are valid JavaScript. The path is the same as for 3. A different design for the log half main...robobun/d704c2b2/ts-out-type-parameter-name It does not change the lexer. While the lexer's log is disabled, the two errors of the type skipper wait in a list on I ran 143 inputs through both designs with the
|
0c12add to
0e6deb6
Compare
…type parameter name
In a TypeScript file `c << out` failed with `The modifier "out" is not valid here`.
So did `x = <out>(y)`, `async <out>(x)` and `function f<out>(v: out) {}`.
Where variance annotations are not allowed, `out` is now a modifier only when
an identifier or keyword follows it on the same line, as in the TypeScript
compiler. Otherwise it is the name of the type parameter.
The lexer snapshot records the length of the log, and `Lexer::restore`
truncates the log to it, as it does for the comment buffers. A failed
backtracking attempt and a lookahead leave nothing in the log. An attempt that
is kept keeps what it logged.
The lookahead scans with the log on and then rewinds. The attempt that reads
the same tokens next logs no lexer error, so the lookahead was the only
reporter of an invalid escaped name in `x = async <\u{30}a,>(y) => y`.
An error that the scan logs now stops the parse, with one message.
0e6deb6 to
d49116d
Compare
Problem
bun -e,c << outfails withThe modifier "out" is not valid here. So doesfunction f<out>(v: out) {}.skip_type_script_type_parameters(src/js_parser/parse/parse_skip_typescript.rs:1212) reads everyoutafter<as a variance modifier.src/js_parser/lexer.rs:367), but its logged message stays.Fix
outis a modifier only when a word follows it on the same line (tsc's rule).Lexer::snapshotrecords the length of the log andLexer::restoretruncates to it. A kept attempt keeps its messages.test/bundler/transpiler/transpiler.test.js(12 new tests, all fail on main),test/bundler/esbuild/ts.test.ts,test/js/bun/transpiler/.=>after<T>in a type, a differential test against tsc.Background
a < bcan start type arguments, so the parser tries that reading first. A failure rewinds the lexer.lexer.is_log_disabled). Two statements of the type skipper ignore that flag.Downsides
.tsxfile runs one lookahead. Release.textgrows 2,816 bytes.a << out > (b.c) > (c)now becomes a call, as with every other name. Before, it was refused.x = async < out in T > (y)is still refused. The attempt is kept there.Notes
Where it fails on main. Fuzzing found this. No user reported it. It fails in every place that reads text with the TypeScript parser:
.ts,.tsx,.mtsand.ctsfiles,bun -e,bun -pand stdin (plain JavaScript is read with the TSX loader there),Bun.Transpiler,bun build.bun checkandbun lintaccept the same file, because they use another parser. esbuild 0.21.5 and esbuild main give the same error, so this is a deliberate difference from esbuild.What changes for which input (
Bun.Transpiler, release builds of the merge base and of this branch, tsc is typescript 6.0.2transpileModule)c << out,x = c << out as number,x = c << out in d,x = c << out satisfies number,x = c << outbefore a line break, 10 more shift formsx = <out>y,x = <out>(y),x = async <out>(y),x = <out>(y) => y,x = <out extends string ? 1 : 2>yfunction f<out>(v: out) {},<out, T>,<out = number>,<const out>, methods, function types,foo<<out>() => void>()function f<out T>() {},function f<in T>() {},function f<out extends number>() {},type Foo<const T> = T,<in T>() => {},foo<<out T>() => void>()x = a << in,x = <in T>yUnexpected inUnexpected inx = a < [const: b] > cUnexpected "const", thenUnexpected constUnexpected const-->comment line that an attempt or a lookahead reads: afterx = a < b,for (async,import type from,yield import,<in TSXx = a < <b>, line break,--> t</b>withtsxBun.Transpilerthrows the warning of the failed attemptx = <\u{30}a,>(y) => ywithtsxInvalid identifier: "0a"two timestsloader, 25 of 25 and 0 withtsx. tsc: 0 of 30.Bun.Transpilerthrows on warnings too, so a doubled warning was visible there as two messages.The rule for
out. tsc reads a word as a modifier only whennextTokenIsOnSameLineAndCanFollowModifier. This change uses the part of that rule that is about words: an identifier or a keyword on the same line. The other tokens in tsc's list ([,{,*,..., a string, a number) cannot continue a type parameter list, so the result is a syntax error with either reading. Lists that allow variance annotations (type aliases, interfaces, classes) keep their current behavior: thereoutcould already be the name. They still accepttype Foo<out⏎T> = Tandtype Foo<out extends T> = T, which tsc rejects. To reject them would restrict input that bun accepts today.Why
Lexer::restore. Six places rewind the lexer: the two attempt helpers, three one-token lookaheads and the full parser snapshot. After each rewind the same text is read again, and that read logs what there is to log. Only the parser snapshot took the log back, with its own three fields. Those fields are gone.Log::truncateis the one implementation, andbunfiguses it in place of its own copy of the loop. The first version of this change truncated in the two attempt helpers. The new lookahead foroutthen needed a third copy, and three other lookaheads still logged the-->warning two times. Bun's newer lexer does the same in its rewind (src/sema/parser/lexer.rs,reset).The TSX arrow function lookahead.
is_ts_arrow_fn_jsxis the one rewind that runs with the log on. Forx = async <\u{30}a,>(y) => yit was the only reporter of the invalid name, because the kept attempt that reads the name next logs no lexer error. An error that this lookahead logs now ends the parse, with one message. With thetsloader that text is accepted before and after.Numbers (release builds of bc1030a and of this change on it, x64 Linux,
instructions:ufromperf_event_open, median of 5,bun build --no-bundleover 2,000,000 lines of one shape)x = a < b;(each line a failed attempt)x = f<T>(a);(each line a kept attempt)x = a > b;(no attempt)jsloaderfor (async in y);with thejsloader (one lookahead each)x = <a/>;withtsx(one lookahead each)x = <a/>;withjsxx = <T,>(y: T) => y;withtsxfunction f<T>() {}.ts,.tsx,.mts,.ctsfiles,transformSyncof each in one processLog::truncate).textskip_type_script_type_parameters<has the same instructionsis_ts_arrow_fn_jsxLog::truncate, and the lookahead foroutper TypeScript parser instanceperf,valgrindandbloatyare not installed on the machine that measured. The counter is a small program aroundperf_event_open.What stays wrong, and what gets worse
<T>(U)with no=>as a type. So an attempt at type arguments is kept fora << T > (b.c) > (c), and the text becomes the calla(c), for every nameT. tsc keeps the shift. Withoutthis text was refused before. Nowoutbehaves as the other names do. The fix is to require=>there. It changes output for text that main accepts, so it is a separate change.x = a << out in c > (b.c) > (d)still fails with the modifier error.x = async < out in T > (y)fails too: the attempt at type parameters before(is kept.x = f<T | >(y)printsx = f(y);before and after. js_parser: validate TypeScript class member modifiers like esbuild #40817 is about that.bun checkparser.