Repository navigation
Conversation
|
Updated 4:50 AM PT - Oct 22nd, 2026
⏳ @robobun, your commit 3fe65a0 is still building in
|
|
Status Reproduced on main (017b243) and on 1.4.3-canary with
tsc 6.0.2 reports no parse diagnostics for both files, and The fix is in this PR (#46065). |
![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 finding, I also checked: the STOP_BEFORE_NAME early return in parse_expr_common skipping the __PURE__ handling (it only fires on a bare type/interface/namespace/module identifier followed by another identifier, so no call or new on a plain statement loses its annotation); the speculative type-parameter skip in ts_decl_follows_name leaving state behind (it runs under speculate, which restores the full parser snapshot, same as the existing conditional-arrow lookahead); and module.exports = ... now routing through the cold parse_ts_decl_keyword_expr (next token is ., not as/satisfies, so it is one extra call and no speculation).
Extended reasoning...
The change touches the TypeScript statement parser (new branch in parse_stmt_fallthrough plus two cold helpers), adds a const-generic flag to parse_expr_common, extracts a generic speculate helper used by the refactored conditional-arrow lookahead, and bumps the transpiler cache version to 36. No security-sensitive surface is involved. A confirmed finding on the namespace/module/interface { heuristic is posted inline, so a human should look at that before merging.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe parser now distinguishes TypeScript declarations named ChangesTypeScript Declaration Name Parsing
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title is concise and accurately identifies the main parser change: support for TypeScript declarations named as or satisfies. |
| 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. |
| Description check | |
The description is complete and relevant. It explains the problem, fix, scope, trade-offs, test coverage, verification results, and known limitations. It does not use the template headings exactly, bu… |
Full details: Description check
-
Explanation
The description is complete and relevant. It explains the problem, fix, scope, trade-offs, test coverage, verification results, and known limitations. It does not use the template headings exactly, but it provides the required information in equivalent sections.
✨ Finishing Touches: 💡 1
-
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
-
- 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.
b4ca7a3 to
d1cc644
Compare
There was a problem hiding this comment.
I re-reviewed the re-pushed version and found no bugs; since this changes statement parsing in the TypeScript parser and deliberately trades one accepted shape for another, a human maintainer's look is still worthwhile.
What was reviewed:
P::speculateagainst the inline snapshot/restore code it replaces inparse_skip_typescript.rs— same snapshot, log-disable, restore order and the same StackOverflow/Alloc pass-through, so the conditional-arrow path is unchanged.- The
STOP_BEFORE_NAMEearly return inparse_expr_common—parse_suffixonly continues on an identifier token foras/satisfies, andparse_ts_decl_keyword_exprre-runsparse_suffixin exactly that case when no declaration follows, so other callers and the non-declaration cast path keep main's behavior; the skipped@ __PURE__branch is covered by the new dce test. ts_decl_follows_namelookahead respectsStatementScope::Nestedfor namespace/module and the newline rule is enforced downstream by the existing keyword handlers;EXPECTED_VERSIONis bumped for the changedinterface;output.- The open tradeoff for a maintainer: a top-level
namespace/module/interfacevariable cast to an object type (namespace as { a?: number };) now parses as a declaration and errors, matching tsc but diverging from esbuild and from main. The PR documents this under Downsides and pins it in tests.
Extended reasoning...
The change touches the TypeScript statement fallthrough in src/js_parser/parse/parse_stmt.rs, adds a const-generic flag to parse_expr_common in src/js_parser/parse/mod.rs, extracts a speculate helper in src/js_parser/p.rs, and bumps the transpiler cache version; it touches no security-sensitive surface. The refactors were checked line-by-line against the code they replace and are behavior-preserving, and the new tests cover ts/tsx loaders, transform/scan/scanImports, nested scopes, js/jsx negatives and a stack-exhaustion shape. Deferring rather than approving because the PR intentionally changes which inputs parse (object-type casts of variables named namespace/module/interface become declarations), a tsc-versus-esbuild policy choice for a parser maintainer, and because the author's reply to the prior inline comment on that point is not independently resolved.
A statement led by `type`, `interface`, `namespace` or `module` was
parsed as an expression first. The suffix loop took a following `as` or
`satisfies` as the cast operator, so `type as = number` was a syntax
error and `interface as {}` became the statement `interface;`.
For these four keywords the statement parser now stops the expression
before an identifier. If that identifier is `as` or `satisfies`, the
parser looks past it. When the rest of a declaration follows, the
identifier is the name. If not, the suffix loop runs and reads the cast
as before, so `type satisfies never;` still casts a variable named
`type`.
`parse_expr_common` takes a const flag for the stop, so the keyword
path shares its body. `P::speculate` holds the snapshot, restore and
error triage of a speculative parse for this look past the name and for
the arrow return type in a conditional.
d1cc644 to
3fe65a0
Compare
There was a problem hiding this comment.
I reviewed the latest revision and didn't find any bugs. Because this changes a hot parser path and deliberately follows tsc rather than esbuild/main for namespace/module/interface followed by as/satisfies and an object type (now a declaration body, not a cast), a human look at that tradeoff is still worthwhile.
What was reviewed:
- The
STOP_BEFORE_NAMEearly return inparse_expr_common: the skippedparse_suffixonly acts on an identifier token when it isas/satisfies(sfx_handle_typescript_as), whichparse_ts_decl_keyword_exprhandles itself, and the skipped__PURE__branch cannot apply when an identifier follows the prefix. - ASI:
type\nas = 3still reachesparse_stmt_fallthrough_ts_keyword'shas_newline_beforechecks and falls through to an expression statement, as before. speculate: the lexer snapshot restoresis_log_disabled, and the conditional-arrow-return-type refactor moves the old snapshot/restore/error-triage code unchanged.
Extended reasoning...
The change touches the TypeScript statement parser (src/js_parser/parse/parse_stmt.rs, parse/mod.rs, parse_skip_typescript.rs, p.rs), adds a const-generic flag to the shared parse_expr_common, extracts a speculate helper, bumps the runtime transpiler cache version, and adds tests in transpiler.test.js and dce.test.ts. It touches no security-sensitive surface. The code is coherent and the tests cover the ASI, cast-stays-cast, nested-scope and js/jsx-loader variants, but the parser is a hot path and the PR intentionally changes the parse of a cast-to-object-type after these keywords to match tsc, which is a behavior tradeoff a maintainer should weigh rather than an obviously correct mechanical change.
Problem
type as = number;fails witherror: Unexpected =.interface as { a: number }becomes the statementinterface;. tsc andbun checkaccept both.parse_stmt_fallthrough(src/js_parser/parse/parse_stmt.rs:1720) parses an expression first. Its suffix loop (parse_suffix.rs:23) takes the name as the cast operator.Fix
type,interface,namespaceormodule, the parser looks pastasorsatisfies. If=,{,extendsor.follows, it is the name. If not, it stays a cast (type satisfies never;).test/bundler/transpiler/transpiler.test.js(40 new tests, 36 fail on main) andtest/bundler/esbuild/dce.test.ts(1 new).Background
parse_prefixreads one operand. The suffix loop reads the operators after it, such asx as T.bun checkalways read an identifier after the keyword as the name. That rule also rejectsdefault: type satisfies never;, which runs today (satisfiesdoes not work on a variable calledtypemicrosoft/TypeScript#58248). It is 20 source lines shorter.interface;.Downsides
foo(1);: 3,262 to 3,268).type T = number;: 1,496 to 1,368.src/js/node/fs.ts+0.03% instructions,lib.dom.d.ts-0.59%. Release binary text: +1,280 B.namespace,moduleorinterfaceto a{type is now a declaration.namespace as { a?: number };is nowUnexpected :.Notes
There is no GitHub issue. A differential run against tsc and
bun checkfound the inputs.test/cli/check/differential-cases.jsonalready has one as case91-R05a. In a module, the strayinterface;is reported as"interface" is a reserved word and cannot be used in strict mode.Inputs (tsc 6.0.2
parseDiagnostics, esbuild 0.28.2, Bun main, this PR)type as = number;type satisfies<T> = T;interface as { a: number }interface;namespace as { export const x = 1 }module as.b {..}namespace as {}namespace;namespace;declare,export,export declare,export default interfacetype as number;switch (type) { default: type satisfies never; }namespace as T;type as <T>() => T;type as { a: number };function f() { namespace as {} }namespace;namespace;namespace;namespace as { a: number };namespace as { a?: number };namespace as {} | null;interface as { a: number } | nullvar y = type as number;(type as number);type+ line break +as;swc 1.16, oxc 0.154 and babel 8 accept the
typeandinterfacedeclarations. babel and swc took this as a bug report (babel/babel#16048, swc-project/swc#8121) and moved to the tsc rule.The rule
asorsatisfiesfollows the keyword and the rest of a declaration follows that name. In every other caseparse_suffixruns on the same tokens as on main.type: optional type parameters, then=. Sotype as<T> = Tis an alias andtype as <T>() => T;is a cast.interface: optional type parameters, then{orextends.namespace/module:{or., and not in a nested scope, where Bun has no namespace declarations.|,&orasfollows them. esbuild and swc read the cast.Measurements (release builds of main 017b243 and of this change on that commit, user-mode instructions from
perf_event_open)emit_decorator_metadataswitch moved intoP::speculate(see below). That adds one byte load and two byte stores to the cold look past the name.foo(1);3,136.1 to 3,136.1. In .ts:foo(1);3,262.1 to 3,268.1,expect(1);3,394.3 to 3,400.3,x = y as T;3,990.8 to 3,996.8,type T = number;1,496.1 to 1,368.1,interface I { a: string; b: number }2,523.2 to 2,404.2,namespace N { export const a = 1 }10,635.6 to 10,514.6,module.exports.a = 1;4,878.9 to 4,830.9,type = 1;3,079.9 to 3,029.9.type as number;2,461.5 to 3,064.5.type as = number;costs 1,889.1.bun build --no-bundle, min of 5:lib.dom.d.ts38,060,378 to 37,835,915 (-0.59%).src/js/node/fs.ts7,945,336 to 7,947,504 (+0.027%).src/js/node/http2.ts26,459,206 to 26,462,659 (+0.013%).typescript.jsas .ts (9.1 MB) 1,130,788,509 to 1,130,936,080 (+0.013%).llvm-nm -S,llvm-objdump -d):parse_expr_commonx4,parse_suffixx4,parse_prefixx4,parse_expr_or_let_stmtx4 and the JavaScriptparse_stmtx2 have the same size and 0 differing instructions. The TypeScriptparse_stmtx2 grows from 3,455 B to 3,891 B and its frame from 152 B to 168 B. New and cold:parse_ts_decl_keyword_expr505 B x2 (frame 56 B),ts_decl_follows_name533 B x2.x = function(){3,315 to 3,314,module.exports = function(){3,316 to 3,315,type = function(){3,315 to 3,315,module.exports = () => {2,719 to 2,824..text81,368,037 to 81,369,317 (+1,280 B).llvm-sizetext total +1,880 B.bun build --no-bundleare identical for 20,475 of 20,475 TypeScript files in this checkout,node_modulesincluded.Tests
test/bundler/transpiler/transpiler.test.js,runtime-transpiler.test.ts,scope-mismatch-panic.test.ts,transpiler-stack-overflow.test.ts,decorators.test.ts,decorator-metadata.test.ts,es-decorators.test.ts,ts-enum-redecl-panic.test.ts,ts-use-define-for-class-fields.test.ts,test/bundler/esbuild/{ts,dce,importstar_ts}.test.ts,test/js/bun/transpiler/{repl-transform,parse-error-column}.test.ts,test/cli/run/transpiler-cache.test.ts.is_identifierin the new branch (checked with a build). "the keyword is an identifier when no name follows it on the line" fails without the token test inparse_expr_common. "the js and jsx loaders have no such declarations" aborts withoutIS_TYPESCRIPT_ENABLEDin the new branch.ALLOW_IN_OUT_VARIANCE_ANNOTATIONSin the look past the name only changes what is logged, and the log is restored. No test can fail without it. It is there so that the look skips with the flags of the declaration parse.is,asserts,infer,keyof,readonly,unique,out,ofandtypeas names already work on main.Self-review
RuntimeTranspilerCachekey from the build revision (two concerns), thebun fmtparentheses fornamespace(two concerns), a run of the transpiler over the tsc corpora intest/cli/check.Other changes in the diff
parse_expr_commonhas the const parameterSTOP_BEFORE_NAME. With it, an identifier after the prefix ends the expression. The three existing callers passfalse.P::speculateholds the snapshot, the restore and the error triage of a speculative parse. The look past the name uses it, and so does the arrow return type in a conditional (is_type_script_arrow_return_type_after_question_and_before_colon), which had the same lines inline.P::speculatealso turnsemit_decorator_metadataoff during the attempt, as Hardening follow-up: fix regressions from #46021 and more crashes #46270 did at the arrow site. A metadata lookup adds an unbound symbol to the module scope, and the restore does not remove it from that scope. The test of Hardening follow-up: fix regressions from #46021 and more crashes #46270 ("emitDecoratorMetadata in an arrow function with a return type in a true branch") passes.P::log(). With js_parser: truncate the log inLexer::restore, and readoutas a type parameter name #45964, a lexer rewind is enough.Not fixed here
type! Foo = number;andtype as any Foo = number;still declareFoo. tsc rejects both.declare,abstractandglobaldecide after the suffix loop, as before.declare as number;prints nothing. tsc printsdeclare;.function f() { namespace Foo {} }is an error on main and stays one.export type as = number;already parses (t_exportreadstypeitself). tsc rejects it.import { type as } from "x"keeps a runtimeimport "x", where{ type Foo }is dropped. js_parser: drop a TypeScript import whose clause binds nothing #41542 is open for that arm.using => 1;andlet => 1;fail withExpected ";" but found "=>". parser: fix whereusingdeclarations may appear (switch clauses, for heads,awaitnewline) #40813 edits that arm.bun fmtprints(namespace as { a: number });without the parentheses, which is now a namespace.type,interfaceandmodulekeep their parentheses there (src/format/js/parentheses/expression.rs:671).References
export typebefore a newline, andimport type from#40818 and js_parser: treat "async as T" / "async satisfies T" as a cast, not an arrow #34246 also followed tsc where esbuild rejects valid TypeScript. js_parser: treat "async as T" / "async satisfies T" as a cast, not an arrow #34246 keptasync as Ta cast.bun check: fix differences fromtsc#44665 added this four-keyword test to this function for thebun checkmode of this parser, with the one-token rule of tsc. Addbun lintandbun format#44805 removed that mode. The rule now lives in the parser ofbun check(src/sema/parser/parser/stmt.rs:286).type as T;,type satisfies T;,namespace as T;andmodule as T;syntax errors, as in tsc. Its source diff is +43/-1 against +86/-25 here.typeidentifier should not trigger an parsing error when part of an assertion microsoft/TypeScript#58712 tried to keeptype satisfies T;a cast in tsc with a look past the name. It was closed without a merge.[human-review] gate passed · iteration 0 · 7 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file