Skip to content

[compiler][fire] Only allow values captured by the effect to be fired#79

Open
everettbu wants to merge 2 commits into
mainfrom
pr32505
Open

[compiler][fire] Only allow values captured by the effect to be fired#79
everettbu wants to merge 2 commits into
mainfrom
pr32505

Conversation

@everettbu

@everettbu everettbu commented Dec 12, 2025

Copy link
Copy Markdown

Mirror of facebook/react#32505
Original author: jbrown215



Stack created with Sapling. Best reviewed with ReviewStack.

  • -> #32505
  • #32504

@everettbu everettbu added CLA Signed React Core Team Opened by a member of the React Core Team labels Dec 12, 2025
@greptile-apps

greptile-apps Bot commented Dec 12, 2025

Copy link
Copy Markdown

Greptile Overview

Greptile Summary

This PR changes the fire() syntax from accepting call expressions to accepting only identifiers, transforming fire(foo(args)) into fire(foo)(args).

Key changes:

  • Replaced call expression handling with identifier-only validation in TransformFire.ts
  • Added ensureLinearConsumption() validation to ensure fire() results are only used as callees
  • Added errorIfNotCapturedFromComponentScope() check to ensure fired functions are captured from the component/hook scope, not defined inside the effect
  • Added tracking of fire call results through #fireCallResultsToReplacedBindings to properly replace the callee in subsequent call expressions
  • Updated withUseEffectLambdaScope() to accept the lambda parameter to extract captured identifiers

Test updates:

  • All existing tests updated to use new fire(foo)(args) syntax instead of fire(foo(args))
  • New test added (error.invalid-capture.js) to validate rejection of functions defined inside the effect callback

Confidence Score: 5/5

  • This PR is safe to merge - well-tested syntax change with comprehensive validation
  • The changes are well-structured with proper error handling, comprehensive test coverage including both valid and error cases, and maintain backward compatibility through the bailout-retry mechanism. The new validation ensures fire() is used correctly by only accepting captured identifiers.
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
compiler/packages/babel-plugin-react-compiler/src/Transform/TransformFire.ts 5/5 Changed fire() to accept identifiers instead of call expressions, added validation to ensure fired values are captured from component scope, and added linear consumption checks
compiler/packages/babel-plugin-react-compiler/src/tests/fixtures/compiler/transform-fire/error.invalid-capture.js 5/5 New test validating that fire() rejects identifiers defined inside the useEffect callback
compiler/packages/babel-plugin-react-compiler/src/tests/fixtures/compiler/transform-fire/basic.js 5/5 Updated syntax from fire(foo(props)) to fire(foo)(props)

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

54 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

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