Skip to content

fix: catch parameter alias handling and snapshot updates for noBackwardRangeExtension#639

Closed
everettbu wants to merge 3 commits into
mainfrom
fix/set-state-in-effect-hof-35910
Closed

fix: catch parameter alias handling and snapshot updates for noBackwardRangeExtension#639
everettbu wants to merge 3 commits into
mainfrom
fix/set-state-in-effect-hof-35910

Conversation

@everettbu

@everettbu everettbu commented Mar 1, 2026

Copy link
Copy Markdown

Mirror of facebook/react#35934
Original author: fresh3nough


Summary

Fixes failing compiler snapshot tests introduced by the noBackwardRangeExtension changes for method call receivers.

Problem

The noBackwardRangeExtension heuristic in InferMutationAliasingRanges.ts walks backward through alias edges to detect function call results (identified by having maybeAliases). However, catch parameters are aliased to call results in try blocks via terminal Alias effects. This caused the heuristic to incorrectly treat catch parameters as independently-memoizable function call results, preventing backward range extension and causing:

  • 4 compiler crashes (Invariant: Expected a break target) when try-catch blocks got split across reactive scopes
  • 1 snapshot regression where catch-aliased values were incorrectly scoped separately

Fix

Track catch handler binding identifiers in AliasingState.catchParams. During the backward-alias walk in mutate(), stop traversal when a catch parameter is encountered. This prevents the heuristic from walking through catch parameter aliases to reach call results with maybeAliases.

Changes

  • InferMutationAliasingRanges.ts: Added catchParams: Set<Identifier> to AliasingState, populated from maybe-throw terminal Alias effects, and checked during the backward-alias heuristic walk
  • 8 snapshot updates: Updated .expect.md files for tests where the noBackwardRangeExtension change produces different but functionally correct memoization output (all eval outputs remain correct)

Tests fixed

  • try-catch-try-value-modified-in-catch (and propagate-scope-deps-hir-fork variant)
  • try-catch-try-value-modified-in-catch-escaping (and propagate-scope-deps-hir-fork variant)
  • try-catch-alias-try-values

Closes react/react#35902

@greptile-apps

greptile-apps Bot commented Mar 1, 2026

Copy link
Copy Markdown

Greptile Summary

This PR combines two changes: (1) a fix for the noBackwardRangeExtension heuristic in the React compiler that was incorrectly treating catch parameters as independently-memoizable function call results, causing 4 compiler crashes and 1 snapshot regression in try-catch blocks, and (2) support for detecting HOF-wrapped components (const MyComponent = wrap(() => {})) in both the compiler's getFunctionName and the ESLint plugin's checkTopLevelNode heuristic.

  • Catch parameter fix: Adds catchParams: Set<Identifier> to AliasingState populated from maybe-throw terminal Alias effects, and stops the backward-alias walk when a catch parameter is encountered. This is a well-targeted fix that prevents scope boundary violations in try-catch blocks.
  • HOF-wrapped component detection: Extends getFunctionName in Program.ts to recognize functions passed as arguments to call expressions assigned to variables, and broadens the ESLint pre-filter to include CallExpression init types. New test coverage for both immutability and set-state-in-effect rules.
  • 8 snapshot updates reflect the improved memoization granularity from the noBackwardRangeExtension changes — function call results (e.g., makeArray(), new Highlight(), fbt._()) are now independently memoized rather than being merged into larger reactive scopes.

Confidence Score: 4/5

  • This PR is safe to merge — the core fix is well-reasoned with thorough comments, and all snapshot updates reflect expected behavior changes.
  • The catch parameter handling logic is correct: effect.into is the catch binding, only maybe-throw terminals produce Alias effects, and the backward-alias walk guard prevents the identified class of crashes. The HOF detection changes are structurally sound. The snapshot changes consistently show improved memoization granularity. Score reflects the complexity of the aliasing state machine changes and the importance of the scope boundary fix.
  • Pay close attention to InferMutationAliasingRanges.ts — it contains the core fix with the most complex logic changes around the backward-alias walk and skipRangeExtension propagation.

Important Files Changed

Filename Overview
compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts Core fix: adds catchParams tracking to AliasingState and a backward-alias walk guard to prevent catch parameters from being incorrectly identified as function call results. Also threads noBackwardRangeExtension and skipRangeExtension through the mutation traversal. Logic is sound and well-commented.
compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts Sets noBackwardRangeExtension for method call receivers (where receiver !== function) in the default Apply handler. Clean, targeted change.
compiler/packages/babel-plugin-react-compiler/src/Inference/AliasingEffects.ts Adds optional noBackwardRangeExtension field to MutateTransitiveConditionally effect type. Minimal, backward-compatible type change.
compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts Adds CallExpression parent detection in getFunctionName for HOF-wrapped components (e.g., const MyComponent = wrap(() => {})). Previous review flagged IIFE concern but the AST path check is structurally correct for the argument position.
packages/eslint-plugin-react-hooks/src/shared/RunReactCompiler.ts Adds CallExpression to the heuristic init type check in checkTopLevelNode for HOF-wrapped components. Slightly broadens the pre-filter heuristic (matches IIFEs too) but acceptable for a pre-compilation gate.
packages/eslint-plugin-react-hooks/tests/ReactCompilerRuleTypescript-test.ts Adds test coverage for HOF-wrapped components: immutability rule (prop mutation) and set-state-in-effect rule detection. Tests are well-structured and cover both valid and invalid cases.

Last reviewed commit: 99d6d02

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

14 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Comment on lines +1327 to +1333
} else if (
parent.isCallExpression() &&
parent.parentPath.isVariableDeclarator() &&
parent.parentPath.get('init').node === parent.node
) {
// const MyComponent = wrap(() => {});
id = parent.parentPath.get('id');

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

IIFE incorrectly matched by CallExpression branch

This new branch matches any function whose parent is a CallExpression assigned to a variable, but it doesn't distinguish whether the function is an argument (HOF pattern: wrap(() => {})) or the callee (IIFE pattern: (() => {})()).

For const Items = (() => { ... })(), the arrow function is the callee of the CallExpression. The AST path is identical — parent.isCallExpression() is true, parent.parentPath.isVariableDeclarator() is true — so getFunctionName would return Items. Since Items starts with uppercase, getComponentOrHookLike would then incorrectly classify this IIFE as a component.

Consider adding a guard to ensure the function is an argument, not the callee:

Suggested change
} else if (
parent.isCallExpression() &&
parent.parentPath.isVariableDeclarator() &&
parent.parentPath.get('init').node === parent.node
) {
// const MyComponent = wrap(() => {});
id = parent.parentPath.get('id');
} else if (
parent.isCallExpression() &&
parent.get('callee').node !== path.node &&
parent.parentPath.isVariableDeclarator() &&
parent.parentPath.get('init').node === parent.node
) {
// const MyComponent = wrap(() => {});
id = parent.parentPath.get('id');
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts
Line: 1327-1333

Comment:
**IIFE incorrectly matched by `CallExpression` branch**

This new branch matches any function whose parent is a `CallExpression` assigned to a variable, but it doesn't distinguish whether the function is an **argument** (HOF pattern: `wrap(() => {})`) or the **callee** (IIFE pattern: `(() => {})()`).

For `const Items = (() => { ... })()`, the arrow function is the `callee` of the `CallExpression`. The AST path is identical — `parent.isCallExpression()` is true, `parent.parentPath.isVariableDeclarator()` is true — so `getFunctionName` would return `Items`. Since `Items` starts with uppercase, `getComponentOrHookLike` would then incorrectly classify this IIFE as a component.

Consider adding a guard to ensure the function is an argument, not the callee:

```suggestion
  } else if (
    parent.isCallExpression() &&
    parent.get('callee').node !== path.node &&
    parent.parentPath.isVariableDeclarator() &&
    parent.parentPath.get('init').node === parent.node
  ) {
    // const MyComponent = wrap(() => {});
    id = parent.parentPath.get('id');
```

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

Fix in Claude Code Fix in Codex

fresh3nough and others added 3 commits March 1, 2026 17:33
…n noBackwardRangeExtension heuristic

Track catch handler bindings in AliasingState so the backward-alias
walk in mutate() stops at catch parameters. Without this, catch
parameters aliased to call results in try blocks incorrectly trigger
the effectiveSkipAliases heuristic, preventing backward range
extension and causing 'Expected a break target' crashes when try-catch
blocks get split across reactive scopes.

Fixes 4 compiler crashes (try-catch-try-value-modified-in-catch and
variants) and 1 snapshot regression (try-catch-alias-try-values).
Updates 8 snapshot expectations for tests where the noBackwardRangeExtension
change produces different but functionally correct memoization output.

Closes #35902
@everettbu everettbu changed the title fix: detect setState in useEffect for anonymous component callbacks passed to HOFs fix: catch parameter alias handling and snapshot updates for noBackwardRangeExtension Mar 1, 2026
@everettbu
everettbu force-pushed the fix/set-state-in-effect-hof-35910 branch from cc3ca6b to 99d6d02 Compare March 1, 2026 23:17
@everettbu

Copy link
Copy Markdown
Author

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

@everettbu everettbu closed this Mar 2, 2026
@everettbu
everettbu deleted the fix/set-state-in-effect-hof-35910 branch March 2, 2026 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Compiler Bug]: false negative, compiler skips memoizing a variable that should be memoized

2 participants