[compiler] Avoid bailouts when inserting gating #32598
Merged
+526
−96
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Mar 13, 2025
github-actions
bot
added
the
React Core Team
mofeiZ
force-pushed
the
pr32598
branch
2 times, most recently
from
a02fdbd to
216068c
Compare
mofeiZ
commented
Mar 13, 2025
| CompilerError.throwTodo({ | ||
| reason: `Encountered a function used before its declaration, which breaks Forget's gating codegen due to hoisting`, | ||
| description: `Rewrite the reference to ${fnPath.node.id?.name ?? 'this function'} to not rely on hoisting to fix this issue`, | ||
| if (referencedBeforeDeclaration && fnPath.isFunctionDeclaration()) { |
There was a problem hiding this comment.
Note that this changes us to only bail out for function declarations which are referenced before declaration.
See invalid-fnexpr-reference and reassigned-fnexpr-variable fixtures for examples why we don't need special handling for other function kinds.
This should be correct as other referenced-before-declaration identifiers are handled by directly replacing ast nodes. This may error at runtime, but still preserves source semantics (e.g. compiler-optimized code errors if and only if source code errors)
mofeiZ
commented
Mar 13, 2025
| @@ -0,0 +1,62 @@ | |||
|
|
|||
There was a problem hiding this comment.
This fixture no longer bails out (component syntax refs produce a hoisted React.forwardRef)
mofeiZ
commented
Mar 13, 2025
| @@ -0,0 +1,61 @@ | |||
|
|
|||
There was a problem hiding this comment.
This fixture also no longer bails out (renamed from error.gating-hoisting)
mofeiZ
added a commit
that referenced
this pull request
Mar 13, 2025
…2597) Some code movement for the next PR --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32597). * #32598 * __->__ #32597
This change fixes a coverage hole in rolling out with `gating`. Prior to this PR, configuring `gating` causes React Compiler to bail out of optimizing some functions. This means that it's not entirely safe to cutover from `gating` enabled for all users (i.e. rolled out 100%) to removing the `gating` config altogether, as new functions may be opted into compilation when they stop bailing out due to gating-specific logic. This is technically slightly slower due to the additional function indirection. An alternative approach is to recommend running a codemod to insert `use no memo`s on currently-bailing out functions before removing the`gating` config. --- Tested [internally]( https://fburl.com/diff/q982ovua) by enabling on a page that previously had a few hundred bailouts due to gating + hoisted function declarations and (1) clicking around locally and (2) running a bunch of e2e tests
github-actions bot
pushed a commit
that referenced
this pull request
Mar 13, 2025
This change fixes a coverage hole in rolling out with `gating`. Prior to this PR, configuring `gating` causes React Compiler to bail out of optimizing some functions. This means that it's not entirely safe to cutover from `gating` enabled for all users (i.e. rolled out 100%) to removing the `gating` config altogether, as new functions may be opted into compilation when they stop bailing out due to gating-specific logic. This is technically slightly slower due to the additional function indirection. An alternative approach is to recommend running a codemod to insert `use no memo`s on currently-bailing out functions before removing the`gating` config. --- Tested [internally]( https://fburl.com/diff/q982ovua) by enabling on a page that previously had a few hundred bailouts due to gating + hoisted function declarations and (1) clicking around locally and (2) running a bunch of e2e tests DiffTrain build for [d92e571](d92e571)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change fixes a coverage hole in rolling out with
gating. Prior to this PR, configuringgatingcauses React Compiler to bail out of optimizing some functions.This means that it's not entirely safe to cutover from
gatingenabled for all users (i.e. rolled out 100%) to removing thegatingconfig altogether, as new functions may be opted into compilation when they stop bailing out due to gating-specific logic.This is technically slightly slower due to the additional function indirection. An alternative approach is to recommend running a codemod to insert
use no memos on currently-bailing out functions before removing thegatingconfig.Tested internally by enabling on a page that previously had a few hundred bailouts due to gating + hoisted function declarations and (1) clicking around locally and (2) running a bunch of e2e tests