[compiler][rfc] Hacky retry pipeline for fire #32164

mofeiZ · 2025-01-22T22:49:31Z

Hacky retry pipeline for when transforming fire(...) calls encounters validation, todo, or memoization invariant bailouts. Would love feedback on how we implement this to be extensible to other compiler non-memoization features (e.g. inlineJSX)

Some observations:

Compiler "front-end" passes (e.g. lower, type, effect, and mutability inferences) should be shared for all compiler features -- memo and otherwise
Many passes (anything dealing with reactive scope ranges, scope blocks / dependencies, and optimizations such as ReactiveIR [compiler] Early sketch of ReactiveIR #31974) can be left out of the retry pipeline. This PR hackily skips memoization features by removing reactive scope creation, but we probably should restructure the pipeline to skip these entirely on a retry
We should maintain a canonical set of "validation flags"

Note the newly added fixtures are prefixed with bailout-... when the retry fire pipeline is used. These fixture outputs contain correctly inserted useFire calls and no memoization.

mofeiZ · 2025-01-22T22:52:52Z

...ages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.insert-fire.expect.md

+   8 |   useEffect(() => {
+>  9 |     fire(foo(props), bar);
+     |     ^^^^^^^^^^^^^^^^^^^^^ InvalidReact: Cannot compile `fire`. fire() can only take in a single call expression as an argument but received multiple arguments (9:9)
+
+InvalidReact: Cannot compile `fire`. fire() can only take in a single call expression as an argument but received a spread argument (10:10)
+
+InvalidReact: Cannot compile `fire`. `fire()` can only receive a function call such as `fire(fn(a,b)). Method calls and other expressions are not allowed (11:11)
+
+InvalidReact: Cannot compile `fire`. `fire()` can only receive a function call such as `fire(fn(a,b)). Method calls and other expressions are not allowed (12:12)
+  10 |     fire(...foo);
+  11 |     fire(bar);
+  12 |     fire(props.foo());
+```


Note that we continue to emit fire-related bailouts

mofeiZ · 2025-01-22T22:54:05Z

...react-compiler/src/__tests__/fixtures/compiler/fire/bailout-retry/todo-use-no-memo.expect.md

+// @enableFire
+import { fire } from "react";
+
+function Component({ props, bar }) {
+  "use no memo";
+  const foo = () => {
+    console.log(props);
+  };
+  useEffect(() => {
+    fire(foo(props));
+    fire(foo());
+    fire(bar());
+  });
+
+  return null;
+}


This is a todo fixture because "use no memo" directives probably should directly invoke the retry pipeline (e.g. no memo, no validation, just lower -> transformFire -> codegen)

josephsavona

awesome!

josephsavona · 2025-01-23T00:59:57Z

Re the "hacky" framing of this in the PR title: I think this is fine. I can see having something like a compilation or output mode that we use when running the pipeline. Rather than changing environment settings when we re-run, various passes could take effect based on the combination of the mode + env flags. The modes might be:

Full: transform, validate, memoize. This is the default today.
Transform: Along the lines of the backup mode in this PR. Only applies transforms that do not require following the rules of React, like fire().
Validate: This could be used for ESLint.

Start of a simple retry pipeline. Would love feedback on how we implement this to be extensible to other compiler non-memoization features (e.g. inlineJSX)

Hacky retry pipeline for when transforming `fire(...)` calls encounters validation, todo, or memoization invariant bailouts. Would love feedback on how we implement this to be extensible to other compiler non-memoization features (e.g. inlineJSX) Some observations: - Compiler "front-end" passes (e.g. lower, type, effect, and mutability inferences) should be shared for all compiler features -- memo and otherwise - Many passes (anything dealing with reactive scope ranges, scope blocks / dependencies, and optimizations such as ReactiveIR facebook#31974) can be left out of the retry pipeline. This PR hackily skips memoization features by removing reactive scope creation, but we probably should restructure the pipeline to skip these entirely on a retry - We should maintain a canonical set of "validation flags" Note the newly added fixtures are prefixed with `bailout-...` when the retry fire pipeline is used. These fixture outputs contain correctly inserted `useFire` calls and no memoization. DiffTrain build for [152bfe3](facebook@152bfe3)

@josephsavona

Removes `EnvironmentConfig.enableMinimalTransformsForRetry` in favor of `run` modes. This is a minimal difference but lets us explicitly opt certain compiler passes in/out of different modes. Retry flags don't really make sense to have in `EnvironmentConfig` anyways as the config is user-facing API, while retrying is a compiler implementation detail. (per @josephsavona's feedback #32164 (comment)) > Re the "hacky" framing of this in the PR title: I think this is fine. I can see having something like a compilation or output mode that we use when running the pipeline. Rather than changing environment settings when we re-run, various passes could take effect based on the combination of the mode + env flags. The modes might be: > > * Full: transform, validate, memoize. This is the default today. > * Transform: Along the lines of the backup mode in this PR. Only applies transforms that do not require following the rules of React, like `fire()`. > * Validate: This could be used for ESLint.

@josephsavona

Removes `EnvironmentConfig.enableMinimalTransformsForRetry` in favor of `run` modes. This is a minimal difference but lets us explicitly opt certain compiler passes in/out of different modes. Retry flags don't really make sense to have in `EnvironmentConfig` anyways as the config is user-facing API, while retrying is a compiler implementation detail. (per @josephsavona's feedback #32164 (comment)) > Re the "hacky" framing of this in the PR title: I think this is fine. I can see having something like a compilation or output mode that we use when running the pipeline. Rather than changing environment settings when we re-run, various passes could take effect based on the combination of the mode + env flags. The modes might be: > > * Full: transform, validate, memoize. This is the default today. > * Transform: Along the lines of the backup mode in this PR. Only applies transforms that do not require following the rules of React, like `fire()`. > * Validate: This could be used for ESLint.

@josephsavona

Removes `EnvironmentConfig.enableMinimalTransformsForRetry` in favor of `run` parameters. This is a minimal difference but lets us explicitly opt out certain compiler passes based on mode parameters, instead of environment configurations Retry flags don't really make sense to have in `EnvironmentConfig` anyways as the config is user-facing API, while retrying is a compiler implementation detail. (per @josephsavona's feedback #32164 (comment)) > Re the "hacky" framing of this in the PR title: I think this is fine. I can see having something like a compilation or output mode that we use when running the pipeline. Rather than changing environment settings when we re-run, various passes could take effect based on the combination of the mode + env flags. The modes might be: > > * Full: transform, validate, memoize. This is the default today. > * Transform: Along the lines of the backup mode in this PR. Only applies transforms that do not require following the rules of React, like `fire()`. > * Validate: This could be used for ESLint.

@josephsavona

Removes `EnvironmentConfig.enableMinimalTransformsForRetry` in favor of `run` parameters. This is a minimal difference but lets us explicitly opt out certain compiler passes based on mode parameters, instead of environment configurations Retry flags don't really make sense to have in `EnvironmentConfig` anyways as the config is user-facing API, while retrying is a compiler implementation detail. (per @josephsavona's feedback #32164 (comment)) > Re the "hacky" framing of this in the PR title: I think this is fine. I can see having something like a compilation or output mode that we use when running the pipeline. Rather than changing environment settings when we re-run, various passes could take effect based on the combination of the mode + env flags. The modes might be: > > * Full: transform, validate, memoize. This is the default today. > * Transform: Along the lines of the backup mode in this PR. Only applies transforms that do not require following the rules of React, like `fire()`. > * Validate: This could be used for ESLint.

@josephsavona

Removes `EnvironmentConfig.enableMinimalTransformsForRetry` in favor of `run` parameters. This is a minimal difference but lets us explicitly opt out certain compiler passes based on mode parameters, instead of environment configurations Retry flags don't really make sense to have in `EnvironmentConfig` anyways as the config is user-facing API, while retrying is a compiler implementation detail. (per @josephsavona's feedback #32164 (comment)) > Re the "hacky" framing of this in the PR title: I think this is fine. I can see having something like a compilation or output mode that we use when running the pipeline. Rather than changing environment settings when we re-run, various passes could take effect based on the combination of the mode + env flags. The modes might be: > > * Full: transform, validate, memoize. This is the default today. > * Transform: Along the lines of the backup mode in this PR. Only applies transforms that do not require following the rules of React, like `fire()`. > * Validate: This could be used for ESLint.

@josephsavona

Removes `EnvironmentConfig.enableMinimalTransformsForRetry` in favor of `run` parameters. This is a minimal difference but lets us explicitly opt out certain compiler passes based on mode parameters, instead of environment configurations Retry flags don't really make sense to have in `EnvironmentConfig` anyways as the config is user-facing API, while retrying is a compiler implementation detail. (per @josephsavona's feedback #32164 (comment)) > Re the "hacky" framing of this in the PR title: I think this is fine. I can see having something like a compilation or output mode that we use when running the pipeline. Rather than changing environment settings when we re-run, various passes could take effect based on the combination of the mode + env flags. The modes might be: > > * Full: transform, validate, memoize. This is the default today. > * Transform: Along the lines of the backup mode in this PR. Only applies transforms that do not require following the rules of React, like `fire()`. > * Validate: This could be used for ESLint.

@josephsavona

Removes `EnvironmentConfig.enableMinimalTransformsForRetry` in favor of `run` parameters. This is a minimal difference but lets us explicitly opt out certain compiler passes based on mode parameters, instead of environment configurations Retry flags don't really make sense to have in `EnvironmentConfig` anyways as the config is user-facing API, while retrying is a compiler implementation detail. (per @josephsavona's feedback #32164 (comment)) > Re the "hacky" framing of this in the PR title: I think this is fine. I can see having something like a compilation or output mode that we use when running the pipeline. Rather than changing environment settings when we re-run, various passes could take effect based on the combination of the mode + env flags. The modes might be: > > * Full: transform, validate, memoize. This is the default today. > * Transform: Along the lines of the backup mode in this PR. Only applies transforms that do not require following the rules of React, like `fire()`. > * Validate: This could be used for ESLint.

@josephsavona

…32511) Removes `EnvironmentConfig.enableMinimalTransformsForRetry` in favor of `run` parameters. This is a minimal difference but lets us explicitly opt out certain compiler passes based on mode parameters, instead of environment configurations Retry flags don't really make sense to have in `EnvironmentConfig` anyways as the config is user-facing API, while retrying is a compiler implementation detail. (per @josephsavona's feedback #32164 (comment)) > Re the "hacky" framing of this in the PR title: I think this is fine. I can see having something like a compilation or output mode that we use when running the pipeline. Rather than changing environment settings when we re-run, various passes could take effect based on the combination of the mode + env flags. The modes might be: > > * Full: transform, validate, memoize. This is the default today. > * Transform: Along the lines of the backup mode in this PR. Only applies transforms that do not require following the rules of React, like `fire()`. > * Validate: This could be used for ESLint. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32511). * #32512 * __->__ #32511

@josephsavona

…32511) Removes `EnvironmentConfig.enableMinimalTransformsForRetry` in favor of `run` parameters. This is a minimal difference but lets us explicitly opt out certain compiler passes based on mode parameters, instead of environment configurations Retry flags don't really make sense to have in `EnvironmentConfig` anyways as the config is user-facing API, while retrying is a compiler implementation detail. (per @josephsavona's feedback #32164 (comment)) > Re the "hacky" framing of this in the PR title: I think this is fine. I can see having something like a compilation or output mode that we use when running the pipeline. Rather than changing environment settings when we re-run, various passes could take effect based on the combination of the mode + env flags. The modes might be: > > * Full: transform, validate, memoize. This is the default today. > * Transform: Along the lines of the backup mode in this PR. Only applies transforms that do not require following the rules of React, like `fire()`. > * Validate: This could be used for ESLint. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32511). * #32512 * __->__ #32511 DiffTrain build for [7939d92](7939d92)

Fixing bug from #32164 -- prior to this PR, we inserted hook guards even for functions that bailed out of compilation.

Fixing bug from #32164 -- prior to this PR, we inserted hook guards even for functions that bailed out of compilation. DiffTrain build for [0962f68](0962f68)

facebook-github-bot added the CLA Signed label Jan 22, 2025

github-actions bot added the React Core Team Opened by a member of the React Core Team label Jan 22, 2025

vercel bot deployed to Preview January 22, 2025 22:51 View deployment

mofeiZ commented Jan 22, 2025

View reviewed changes

josephsavona approved these changes Jan 23, 2025

View reviewed changes

mofeiZ force-pushed the retry-fire branch from 09019ba to 0ed9f4f Compare January 31, 2025 23:01

vercel bot deployed to Preview January 31, 2025 23:04 View deployment

[compiler][rfc] Simple retry for fire

7f29580

Start of a simple retry pipeline. Would love feedback on how we implement this to be extensible to other compiler non-memoization features (e.g. inlineJSX)

mofeiZ force-pushed the retry-fire branch from 0ed9f4f to 7f29580 Compare January 31, 2025 23:53

vercel bot deployed to Preview January 31, 2025 23:54 View deployment

mofeiZ merged commit 152bfe3 into facebook:main Jan 31, 2025
20 checks passed

This was referenced Feb 3, 2025

Upgrade React from 37906d4d-20250127 to a4b2d0d5-20250203 vercel/next.js#75617

Closed

Upgrade React from c492f975-20250128 to a4b2d0d5-20250203 vercel/next.js#75649

Merged

mofeiZ mentioned this pull request Mar 4, 2025

[compiler] clean up retry pipeline: fireRetry flag -> compileMode #32511

Merged

mofeiZ added a commit that referenced this pull request Mar 20, 2025

[compiler][bugfix] Don't insert hook guards in retry pipeline

8d2084b

Fixing bug from #32164 -- prior to this PR, we inserted hook guards even for functions that bailed out of compilation.

mofeiZ mentioned this pull request Mar 20, 2025

[compiler][bugfix] Don't insert hook guards in retry pipeline #32665

Merged

mofeiZ added a commit that referenced this pull request Mar 20, 2025

[compiler][bugfix] Don't insert hook guards in retry pipeline (#32665)

0962f68

Fixing bug from #32164 -- prior to this PR, we inserted hook guards even for functions that bailed out of compilation.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[compiler][rfc] Hacky retry pipeline for fire #32164

[compiler][rfc] Hacky retry pipeline for fire #32164

Uh oh!

mofeiZ commented Jan 22, 2025 •

edited

Loading

Uh oh!

mofeiZ Jan 22, 2025

Uh oh!

mofeiZ Jan 22, 2025

Uh oh!

josephsavona Jan 23, 2025

Uh oh!

josephsavona left a comment

Uh oh!

josephsavona commented Jan 23, 2025

Uh oh!

Uh oh!

Uh oh!

[compiler][rfc] Hacky retry pipeline for fire #32164

[compiler][rfc] Hacky retry pipeline for fire #32164

Uh oh!

Conversation

mofeiZ commented Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mofeiZ Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

mofeiZ Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

josephsavona Jan 23, 2025

Choose a reason for hiding this comment

Uh oh!

josephsavona left a comment

Choose a reason for hiding this comment

Uh oh!

josephsavona commented Jan 23, 2025

Uh oh!

Uh oh!

Uh oh!

mofeiZ commented Jan 22, 2025 •

edited

Loading