Skip to content

[compiler] Fix for loops in try/catch#460

Closed
everettbu wants to merge 4 commits into
mainfrom
bug-investigator-agent-2
Closed

[compiler] Fix for loops in try/catch#460
everettbu wants to merge 4 commits into
mainfrom
bug-investigator-agent-2

Conversation

@everettbu

Copy link
Copy Markdown

Mirror of facebook/react#35686
Original author: josephsavona


This is a combination of a) a subagent for investigating compiler errors and b) testing that agent by fixing bugs with for loops within try/catch. My recent diffs to support maybe-throw within value blocks was incomplete and handled many cases, like optionals/logicals/etc within try/catch. However, the handling for for loops was making more assumptions and needed additional fixes.

Key changes:

  • maybe-throw terminal handler is now nullable. PruneMaybeThrows nulls the handler for blocks that cannot throw, rather than changing to a goto. This preserves more information, and makes it easier for BuildReactiveFunction's visitValueBlock() to reconstruct the value blocks
  • Updates BuildReactiveFunction's handling of for init/test/update (and similar for for..of and for..in) to correctly extract value blocks. The previous logic made assumptions about the shape of the SequenceExpression which were incorrect in some cases within try/catch. The new helper extracts a flattened SequenceExpression.

Supporting changes:

  • The agent itself (tested via this diff)
  • Updated the script for invoking snap to keep compiler/ as the working directory, allowing relative paths to work more easily
  • Add an --update (-u) flag to yarn snap minimize, which updates the fixture in place w the minimized version

Fix for for..in and for..of within try/catch. The logic to extract the init/test values didn't work in the presence of maybe-throw terminals. We fix that with more robust extraction of the value block values for this case, consistently flattening nested sequence expressions into a single ReactiveValue.
@everettbu everettbu added CLA Signed React Core Team Opened by a member of the React Core Team labels Feb 3, 2026
@greptile-apps

greptile-apps Bot commented Feb 3, 2026

Copy link
Copy Markdown

Greptile Overview

Greptile Summary

This PR fixes React Compiler handling for for/for..of/for..in loops inside try/catch by preserving maybe-throw structure (making handler nullable and pruning by nulling it rather than rewriting to goto) and by teaching BuildReactiveFunction to correctly reconstruct init/test value blocks via a helper that flattens nested SequenceExpressions.

It also updates related HIR utilities (printing, successor iteration, aliasing inference) for nullable handlers, adds new repro fixtures that now compile successfully, and improves yarn snap minimize by adding an in-place --update flag and adjusting runner behavior.

One issue: yarn snap minimize’s CLI path currently mishandles single-line inputs (no \n), which can cause it to ignore fixture directives when determining language/sourceType.

Confidence Score: 4/5

  • This PR is broadly safe to merge, with one identified correctness bug in the snap minimize CLI path for single-line inputs.
  • Core compiler changes are localized and the nullable maybe-throw handler is consistently handled in updated utilities and inference, with new fixtures covering key cases. The main remaining concern is the firstLine extraction in runMinimizeCommand, which can cause minimize to parse directives incorrectly for files without a newline.
  • compiler/packages/snap/src/runner.ts

Important Files Changed

Filename Overview
compiler/.claude/agents/investigate-error.md Adds a new Claude agent guide for investigating compiler errors; no functional code changes.
compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts Makes MaybeThrowTerminal.handler nullable; requires downstream null-handling (mostly updated in this PR).
compiler/packages/babel-plugin-react-compiler/src/HIR/visitors.ts Updates terminal successor mapping/iteration for maybe-throw to skip null handler.
compiler/packages/babel-plugin-react-compiler/src/Optimization/PruneMaybeThrows.ts Prunes maybe-throw by nulling handler instead of rewriting to goto, preserving HIR structure for later reconstruction.
compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/BuildReactiveFunction.ts Adds helper to flatten nested SequenceExpressions and preserve lvalue assignment for for/for-of/for-in init/test blocks; looks correct for intended try/catch cases.
compiler/packages/snap/src/minimize.ts Removes CLI wrapper from minimize.ts; core minimize() now used from runner.
compiler/packages/snap/src/reporter.ts Adds optional verbose flag to suppress per-fixture PASS/FAIL logs by default.
compiler/packages/snap/src/runner.ts Moves minimize CLI into runner and adds --update flag; contains a bug where single-line inputs yield empty firstLine and ignore directives when parsing language/sourceType.

@greptile-apps greptile-apps 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.

6 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@everettbu
everettbu force-pushed the bug-investigator-agent-2 branch from 0c7b7ca to f09e434 Compare February 3, 2026 21:19

@greptile-apps greptile-apps 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.

7 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@everettbu everettbu changed the title [compiler] Fixes for for loops in try/catch [compiler] Fix for loops in try/catch Feb 3, 2026
@everettbu
everettbu force-pushed the bug-investigator-agent-2 branch from f09e434 to 7e8ae2d Compare February 3, 2026 22:17

@greptile-apps greptile-apps 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.

8 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Comment on lines +158 to +164
// Read the input file
const input = fs.readFileSync(inputPath, 'utf-8');
const filename = path.basename(inputPath);
const firstLine = input.substring(0, input.indexOf('\n'));
const language = parseLanguage(firstLine);
const sourceType = parseSourceType(firstLine);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Single-line input breaks

runMinimizeCommand() computes firstLine via input.substring(0, input.indexOf('\n')) (runner.ts:161). If the file has no newline, indexOf returns -1 and JS substring(0, -1) yields an empty string, so parseLanguage/parseSourceType fall back to defaults and any fixture directives on the first line are ignored. This can change minimization behavior/output for single-line fixtures.

Suggested change
// Read the input file
const input = fs.readFileSync(inputPath, 'utf-8');
const filename = path.basename(inputPath);
const firstLine = input.substring(0, input.indexOf('\n'));
const language = parseLanguage(firstLine);
const sourceType = parseSourceType(firstLine);
const newlineIdx = input.indexOf('\n');
const firstLine = newlineIdx === -1 ? input : input.substring(0, newlineIdx);
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/snap/src/runner.ts
Line: 158:164

Comment:
**Single-line input breaks**

`runMinimizeCommand()` computes `firstLine` via `input.substring(0, input.indexOf('\n'))` (runner.ts:161). If the file has no newline, `indexOf` returns `-1` and JS `substring(0, -1)` yields an empty string, so `parseLanguage/parseSourceType` fall back to defaults and any fixture directives on the first line are ignored. This can change minimization behavior/output for single-line fixtures.

```suggestion
  const newlineIdx = input.indexOf('\n');
  const firstLine = newlineIdx === -1 ? input : input.substring(0, newlineIdx);
```

How can I resolve this? If you propose a fix, please make it concise.

@everettbu

Copy link
Copy Markdown
Author

Upstream PR was closed or merged. Code is synced via branch mirror.

@everettbu everettbu closed this Feb 3, 2026
@everettbu
everettbu deleted the bug-investigator-agent-2 branch February 3, 2026 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed React Core Team Opened by a member of the React Core Team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants