Skip to content

fix(codegen): parenthesize let/async for-of head target - #23008

Merged
graphite-app[bot] merged 1 commit into
mainfrom
fix/codegen-for-of-let-async
Jun 6, 2026
Merged

graphite-app[bot] merged 1 commit into
mainfrom
fix/codegen-for-of-let-async

Conversation

@Boshen

@Boshen Boshen commented Jun 6, 2026 •

Copy link
Copy Markdown
Member

What

A for...of head may not start with the let token, nor be the bare identifier async (ECMA-262: for ( [lookahead ∉ { let, async of }] LeftHandSideExpression of ...). Such a head must be parenthesized.

Approach

The wrap is decided inside the for-of head printing (ForOfStatement::gen), so the hot IdentifierReference path stays untouched. A small structural check determines whether the emitted head would start with let, mirroring codegen's output decisions:

  • member / TS-wrapper / parenthesized expressions are walked to their leftmost object (let.x, let.x[0], (let).x)
  • let[...] already self-wraps to (let)[...], so it is not double-wrapped

A bare non-await async head is handled with a direct check. for...in heads are never wrapped.

Examples

Input Before After
for ((let) of x) for (let of x) ❌ for ((let) of x) ✅
for ((let.x) of x) for (let.x of x) ❌ for ((let.x) of x) ✅
for (((let).x) of x) for (let.x of x) ❌ for ((let.x) of x) ✅
for ((let[0]) of x) for (let[0] of x) ❌ for ((let)[0] of x) ✅
for ((async) of x) for ((async) of x) for ((async) of x)
for (async in x) for ((async) in x) for (async in x) ✅

All outputs verified with node --check.

@github-actions github-actions Bot added the A-codegen Area - Code Generation label Jun 6, 2026
@Boshen Boshen added the 0-merge Merge with Graphite Merge Queue label Jun 6, 2026

Boshen commented Jun 6, 2026 •

Copy link
Copy Markdown
Member Author

Merge activity

  • Jun 6, 8:25 AM UTC: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Jun 6, 1:07 PM UTC: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Jun 6, 1:07 PM UTC: Boshen added this pull request to the Graphite merge queue.
  • Jun 6, 1:12 PM UTC: Merged by the Graphite merge queue.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9e78a43d4f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/oxc_codegen/src/gen.rs Outdated
@codspeed

codspeed Bot commented Jun 6, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 57 untouched benchmarks
⏩ 9 skipped benchmarks1


Comparing fix/codegen-for-of-let-async (9e78a43) with main (187e1a5)

Open in CodSpeed

Footnotes

  1. 9 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

@Boshen
Boshen force-pushed the fix/codegen-for-of-let-async branch from 9e78a43 to 20c5017 Compare June 6, 2026 10:02
@Boshen

Boshen commented Jun 6, 2026

Copy link
Copy Markdown
Member Author

Addressed the P2 review: replaced the FORBID_IN-based for-in/for-of detection (which leaked into a nested for-of head through a function body, emitting invalid for (async of xs)) with a structural check in ForOfStatement::gen. The wrap now depends only on the for-of's own head, not inherited context. for...in heads stay unwrapped. Added a nested-for-of regression test.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 20c50176c7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/oxc_codegen/src/gen.rs Outdated
@Boshen
Boshen force-pushed the fix/codegen-for-of-let-async branch from 20c5017 to 96f8e8f Compare June 6, 2026 10:32
@Boshen

Boshen commented Jun 6, 2026

Copy link
Copy Markdown
Member Author

Addressed the second P2 review: the wrap now covers any for-of head whose emitted leading token is let (e.g. for ((let.x) of x), let.x[0]), not just a bare AssignmentTargetIdentifier. let[...] is handled via its existing self-wrap to (let)[...] (no double parens), and async.x / foo.let stay unwrapped. Added regression tests.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 96f8e8faa1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/oxc_codegen/src/gen.rs
@Boshen
Boshen force-pushed the fix/codegen-for-of-let-async branch from 96f8e8f to 79b1fd5 Compare June 6, 2026 11:25
@Boshen

Boshen commented Jun 6, 2026

Copy link
Copy Markdown
Member Author

Addressed the third P2 review: the helper now looks through ParenthesizedExpression too, since codegen drops redundant parens around a let object ((let).x -> let.x). for (((let).x) of xs) now emits for ((let.x) of xs) instead of the invalid for (let.x of xs). Added a regression test; output verified with node --check.

@Boshen
Boshen force-pushed the fix/codegen-for-of-let-async branch 5 times, most recently from e45f6b9 to 2fd3c24 Compare June 6, 2026 12:23
@Boshen Boshen added 0-merge Merge with Graphite Merge Queue and removed 0-merge Merge with Graphite Merge Queue labels Jun 6, 2026
## What

A `for...of` head may not start with the `let` token, nor be the bare identifier `async` (ECMA-262: `for ( [lookahead ∉ { let, async of }] LeftHandSideExpression of ...`). Such a head must be parenthesized.

## Approach

The wrap is decided **inside the for-of head printing** (`ForOfStatement::gen`), so the hot `IdentifierReference` path stays untouched. A small structural check determines whether the emitted head would start with `let`, mirroring codegen's output decisions:

- member / TS-wrapper / parenthesized expressions are walked to their leftmost object (`let.x`, `let.x[0]`, `(let).x`)
- `let[...]` already self-wraps to `(let)[...]`, so it is not double-wrapped

A bare non-`await` `async` head is handled with a direct check. `for...in` heads are never wrapped.

## Examples

| Input | Before | After |
| --- | --- | --- |
| `for ((let) of x)` | `for (let of x)` ❌ | `for ((let) of x)` ✅ |
| `for ((let.x) of x)` | `for (let.x of x)` ❌ | `for ((let.x) of x)` ✅ |
| `for (((let).x) of x)` | `for (let.x of x)` ❌ | `for ((let.x) of x)` ✅ |
| `for ((let[0]) of x)` | `for (let[0] of x)` ❌ | `for ((let)[0] of x)` ✅ |
| `for ((async) of x)` | `for ((async) of x)` | `for ((async) of x)` |
| `for (async in x)` | `for ((async) in x)` | `for (async in x)` ✅ |

All outputs verified with `node --check`.
@graphite-app
graphite-app Bot force-pushed the fix/codegen-for-of-let-async branch from 2fd3c24 to 597ed85 Compare June 6, 2026 13:08
@graphite-app
graphite-app Bot merged commit 597ed85 into main Jun 6, 2026
29 checks passed
@graphite-app graphite-app Bot removed the 0-merge Merge with Graphite Merge Queue label Jun 6, 2026
@graphite-app
graphite-app Bot deleted the fix/codegen-for-of-let-async branch June 6, 2026 13:12
Boshen added a commit that referenced this pull request Jun 8, 2026
### 💥 BREAKING CHANGES

- ee4dc73 ast: [**BREAKING**] Add `#[non_exhaustive]` to AST nodes
(#23046) (overlookmotel)
- 4c35362 ast: [**BREAKING**] Add
`AstBuilder::template_element_escape_raw` and
`template_element_escape_raw_with_lone_surrogates` methods (#23047)
(overlookmotel)

### 🚀 Features

- b846ab2 react_compiler: Integrate the Rust port of the React Compiler
(#22942) (Boshen)
- 5b8dd68 parser: Report TS1255 for invalid class definite assertions
(#22917) (camc314)
- 85efabf semantic: Make building the class table optional, off by
default (#22862) (Boshen)

### 🐛 Bug Fixes

- 556acdc codegen: Parenthesize TS-cast assignment targets (#23112)
(Boshen)
- 37169ff codegen: Don't emit space between postfix `--` and `>` when
minifying (#23036) (Boshen)
- a4b1bf7 codegen: Drop redundant whitespace in minified TypeScript
output (#23038) (Boshen)
- cf53285 parser: Report reserved type-declaration names in the parser
(#23035) (Boshen)
- 4e44969 ast: Fix UB in `escape_template_element_raw` (#23052)
(overlookmotel)
- c543154 parser: Report comma operator in JSX expression in the parser
(#23030) (Boshen)
- 325c94f codegen: Tighten conditional-type and constructor-type
whitespace when minifying (#23033) (Boshen)
- 95dd3a2 parser: Report `import type` alias to a non-external reference
in the parser (#23032) (Boshen)
- 90180b8 codegen: Drop space after `:` in function return type when
minifying (#23028) (Boshen)
- 6da876e parser: Report `abstract` private class field in the parser
(#23029) (Boshen)
- 28467ce codegen: Don't emit space before a postfix update operand when
minifying (#23027) (Boshen)
- cb29926 codegen: Drop redundant space after `export default` when
minifying (#23024) (Boshen)
- 62965ae codegen: Drop redundant space after `else` when minifying
(#23025) (Boshen)
- 989230a parser: Report compound assignment to non-simple target in the
parser (#23022) (Boshen)
- 06f367c parser: Report `super.#field` private access in the parser
(#23014) (Boshen)
- 184edef codegen: Print space before `const`/`declare` enum modifier
(#23013) (Boshen)
- 4d722e0 parser: Report duplicate switch `default` clause in the parser
(#23012) (Boshen)
- 597ed85 codegen: Parenthesize `let`/`async` for-of head target
(#23008) (Boshen)
- 8b631bf codegen: Remove stray space before mapped type value colon
(#23010) (Boshen)
- c08407e codegen: Don't over-parenthesize `in` inside an arrow in a
for-init (#23009) (Boshen)
- 600cd6f codegen: Parenthesize lower-precedence
`TSInstantiationExpression` operand (#23007) (Boshen)
- 187e1a5 codegen: Don't leak space after comment-only JSX expression
container (#23006) (Boshen)
- 294c473 codegen: Don't over-parenthesize `TSTypeAssertion` operand
(#23004) (Boshen)
- 786d96f codegen: Give `TSTypeAssertion` unary precedence (#23002)
(Boshen)
- 1295882 parser: Report `new.target` and `import.meta` syntax errors in
the parser (#23003) (Boshen)
- d727b6b codegen: Parenthesize `await` expression as base of `**`
(#23001) (Boshen)
- 67dfa08 codegen: Keep parentheses around `new` callees containing a
call (#22997) (Boshen)
- 17e7cf3 parser: Disallow unerasable `as`/`satisfies` assertions
(#22986) (Boshen)
- beb46d3 parser: Commit to module goal on decorated exports (#22941)
(Boshen)
- 49e63f7 isolated-declarations: Require annotations for satisfies
initializers (#22898) (camc314)
- 8c93601 isolated-declarations: Allow unknown enum initializer in
non-const enum (#22900) (camc314)

### ⚡ Performance

- 7d89909 parser: Peek instead of lookahead for yield disambiguation
(#23071) (Boshen)
- bf872f0 parser: Skip arrow lookahead for a parenthesized literal
(#23070) (Boshen)
- d19fc54 parser: Guard type-argument speculation behind an angle-token
check (#23069) (Boshen)
- 8eb5507 parser: Skip redundant member-rest re-scan on call entry
(#23068) (Boshen)
- 883dfc1 parser: Skip parse_call_expression_rest when no call follows
(#23063) (Boshen)
- b171153 parser: Peek before the await-using lookahead (#23059)
(Boshen)
- 56f21bd parser: Use peek_token for the TS `asserts` type predicate
(#23058) (Boshen)
- 68805ac parser: Use peek_token instead of checkpoint/rewind for
single-token decisions (#23056) (Boshen)
- 1f9d8eb ast: `AstBuilder::template_element_escape_raw` avoid
allocation if no escape required (#23053) (overlookmotel)
- 502b04d semantic: Move cold function redeclaration handling into
`#[cold]` function (#22973) (overlookmotel)

### 📚 Documentation

- 275d318 napi/minifier: Point `target` to oxc docs (#23102) (camc314)

Co-authored-by: Boshen <1430279+Boshen@users.noreply.github.com>
camc314 pushed a commit that referenced this pull request Jul 3, 2026
### 💥 BREAKING CHANGES

- ee4dc73 ast: [**BREAKING**] Add `#[non_exhaustive]` to AST nodes
(#23046) (overlookmotel)
- 4c35362 ast: [**BREAKING**] Add
`AstBuilder::template_element_escape_raw` and
`template_element_escape_raw_with_lone_surrogates` methods (#23047)
(overlookmotel)

### 🚀 Features

- b846ab2 react_compiler: Integrate the Rust port of the React Compiler
(#22942) (Boshen)
- 5b8dd68 parser: Report TS1255 for invalid class definite assertions
(#22917) (camc314)
- 85efabf semantic: Make building the class table optional, off by
default (#22862) (Boshen)

### 🐛 Bug Fixes

- 556acdc codegen: Parenthesize TS-cast assignment targets (#23112)
(Boshen)
- 37169ff codegen: Don't emit space between postfix `--` and `>` when
minifying (#23036) (Boshen)
- a4b1bf7 codegen: Drop redundant whitespace in minified TypeScript
output (#23038) (Boshen)
- cf53285 parser: Report reserved type-declaration names in the parser
(#23035) (Boshen)
- 4e44969 ast: Fix UB in `escape_template_element_raw` (#23052)
(overlookmotel)
- c543154 parser: Report comma operator in JSX expression in the parser
(#23030) (Boshen)
- 325c94f codegen: Tighten conditional-type and constructor-type
whitespace when minifying (#23033) (Boshen)
- 95dd3a2 parser: Report `import type` alias to a non-external reference
in the parser (#23032) (Boshen)
- 90180b8 codegen: Drop space after `:` in function return type when
minifying (#23028) (Boshen)
- 6da876e parser: Report `abstract` private class field in the parser
(#23029) (Boshen)
- 28467ce codegen: Don't emit space before a postfix update operand when
minifying (#23027) (Boshen)
- cb29926 codegen: Drop redundant space after `export default` when
minifying (#23024) (Boshen)
- 62965ae codegen: Drop redundant space after `else` when minifying
(#23025) (Boshen)
- 989230a parser: Report compound assignment to non-simple target in the
parser (#23022) (Boshen)
- 06f367c parser: Report `super.#field` private access in the parser
(#23014) (Boshen)
- 184edef codegen: Print space before `const`/`declare` enum modifier
(#23013) (Boshen)
- 4d722e0 parser: Report duplicate switch `default` clause in the parser
(#23012) (Boshen)
- 597ed85 codegen: Parenthesize `let`/`async` for-of head target
(#23008) (Boshen)
- 8b631bf codegen: Remove stray space before mapped type value colon
(#23010) (Boshen)
- c08407e codegen: Don't over-parenthesize `in` inside an arrow in a
for-init (#23009) (Boshen)
- 600cd6f codegen: Parenthesize lower-precedence
`TSInstantiationExpression` operand (#23007) (Boshen)
- 187e1a5 codegen: Don't leak space after comment-only JSX expression
container (#23006) (Boshen)
- 294c473 codegen: Don't over-parenthesize `TSTypeAssertion` operand
(#23004) (Boshen)
- 786d96f codegen: Give `TSTypeAssertion` unary precedence (#23002)
(Boshen)
- 1295882 parser: Report `new.target` and `import.meta` syntax errors in
the parser (#23003) (Boshen)
- d727b6b codegen: Parenthesize `await` expression as base of `**`
(#23001) (Boshen)
- 67dfa08 codegen: Keep parentheses around `new` callees containing a
call (#22997) (Boshen)
- 17e7cf3 parser: Disallow unerasable `as`/`satisfies` assertions
(#22986) (Boshen)
- beb46d3 parser: Commit to module goal on decorated exports (#22941)
(Boshen)
- 49e63f7 isolated-declarations: Require annotations for satisfies
initializers (#22898) (camc314)
- 8c93601 isolated-declarations: Allow unknown enum initializer in
non-const enum (#22900) (camc314)

### ⚡ Performance

- 7d89909 parser: Peek instead of lookahead for yield disambiguation
(#23071) (Boshen)
- bf872f0 parser: Skip arrow lookahead for a parenthesized literal
(#23070) (Boshen)
- d19fc54 parser: Guard type-argument speculation behind an angle-token
check (#23069) (Boshen)
- 8eb5507 parser: Skip redundant member-rest re-scan on call entry
(#23068) (Boshen)
- 883dfc1 parser: Skip parse_call_expression_rest when no call follows
(#23063) (Boshen)
- b171153 parser: Peek before the await-using lookahead (#23059)
(Boshen)
- 56f21bd parser: Use peek_token for the TS `asserts` type predicate
(#23058) (Boshen)
- 68805ac parser: Use peek_token instead of checkpoint/rewind for
single-token decisions (#23056) (Boshen)
- 1f9d8eb ast: `AstBuilder::template_element_escape_raw` avoid
allocation if no escape required (#23053) (overlookmotel)
- 502b04d semantic: Move cold function redeclaration handling into
`#[cold]` function (#22973) (overlookmotel)

### 📚 Documentation

- 275d318 napi/minifier: Point `target` to oxc docs (#23102) (camc314)

Co-authored-by: Boshen <1430279+Boshen@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-codegen Area - Code Generation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant