[compiler] detect and throw on untransformed required features #32512

mofeiZ · 2025-03-04T05:02:20Z

Traverse program after running compiler transform to find untransformed references to compiler features (e.g. inferEffectDeps, fire).

Hard error to fail the babel pipeline when the compiler fails to transform these features to give predictable runtime semantics. Untransformed calls to functions like fire will throw at runtime anyways, so let's fail the build to catch these earlier.

Note that with this fails the build regardless of panicThreshold

mofeiZ · 2025-03-04T16:13:43Z

...es/compiler/infer-effect-dependencies/bailout-retry/error.callsite-in-non-react-fn.expect.md

+  4 | function nonReactFn(arg) {
+> 5 |   useEffect(() => [1, 2, arg]);
+    |   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Todo: Untransformed reference to experimental compiler-only feature (5:5)
+  6 | }
+  7 |


Erroring on an untransformed call anywhere within the program is intentional, as we expect untransformed calls to be invalid / throw at runtime.

mofeiZ · 2025-03-04T16:14:20Z

...tures/compiler/infer-effect-dependencies/bailout-retry/error.non-inlined-effect-fn.expect.md

+> 20 |   useEffect(f);
+     |   ^^^^^^^^^^^^ Todo: Untransformed reference to experimental compiler-only feature (20:20)


Same here -- this error is intentional as we expect untransformed calls to be invalid / throw at runtime.

mofeiZ · 2025-03-04T16:15:52Z

...es/compiler/infer-effect-dependencies/bailout-retry/error.callsite-in-non-react-fn.expect.md

+## Input
+
+```javascript
+// @inferEffectDependencies @compilationMode(infer) @panicThreshold(none)


Note that these fixtures error even with @panicThreshold(none). This PR changes inferEffectDeps to be a "required" feature (e.g. if enabled, relevant code must either be transformed or fail compilation)

mofeiZ · 2025-03-05T15:11:19Z

compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts

+        logError(err, pass, fn.node.loc ?? null);
+        throw err;


This isn't quite right -- we want

only retry compilation if fire or effect dependencies might be called

stash or return this error to later throw (in ValidateNoUntransformed)

jbrown215

Behaviors look good to me! We'll definitely need more actionable error messages so that people can figure out how to make the build errors go away, but this is an excellent start. Left some clarifying questions inline.

jbrown215 · 2025-03-05T15:36:40Z

...tures/compiler/infer-effect-dependencies/bailout-retry/error.non-inlined-effect-fn.expect.md

+  }
+
+  // No inferred dep array, the argument is not a lambda
+  useEffect(f);


There are useEffect wrappers that forward a function + dep array that will block us from enabling this rule, e.g.:

function useMyEffect(fn, deps) { useEffect(fn, deps); // ... other stuff }

We should allow non-lambda arguments so long as a dep array is provided, I think?

Makes sense! Right now we only matches inferEffectDependencies function calls with the same number of arguments as configured with numRequiredArgs

The example you gave useEffect(fn, args) wouldn't match the test runner config since it has two arguments instead of one

jbrown215 · 2025-03-05T15:41:48Z

...ests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.todo-syntax.expect.md

+
+/**
+ * Note that a react compiler-based transform still has limitations on JS syntax.
+ * We should surface these as actionable lint / build errors to devs.


The fix here is to specify the dep array (for special effect, that's a second dep array), is that right?

Yep exactly -- I'll edit the error messages to be more actionable

jbrown215 · 2025-03-05T15:42:44Z

...ests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.use-no-memo.expect.md

+import {useEffect} from 'react';
+
+function Component({propVal}) {
+  'use no memo';


Does this mean 'use no memo' should not disable autodeps/fire? (that sounds right to me, just checking if that's your intent)

Yep that's right -- ideally we'd still run the compiler on use no memo (just with memoization disabled), but that's still a todo

jbrown215 · 2025-03-05T15:43:43Z

...ts__/fixtures/compiler/transform-fire/bailout-retry/error.todo-infer-deps-on-retry.expect.md

+function useFoo({cond}) {
+  const ref = useRef();
+  const derived = cond ? ref.current : makeObject();
+  useSpecialEffect(() => {


Ah, we currently depend on memoization? I thought we only depended on reactivity analysis

Yeah the implementation currently depends on mutability analysis + reactive scopes. ref.current values are a bit of an edge case for mutable ranges, but this is really an implementation todo (specifically, the retry pipeline should not use reactive scopes for dependency analysis)

For devs, this should just an unknown compiler todo bailout with "please supply your own dependency array as the compiler could not transpile this".

jackpope

Walked through it together. Examples make sense.

Error messages do need improvement to be actionable but that can also be a follow up PR.

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

Traverse program after running compiler transform to find untransformed references to compiler features (e.g. `inferEffectDeps`, `fire`). Hard error to fail the babel pipeline when the compiler fails to transform these features to give predictable runtime semantics. Untransformed calls to functions like `fire` will throw at runtime anyways, so let's fail the build to catch these earlier. Note that with this fails the build *regardless of panicThreshold*

mofeiZ · 2025-03-14T15:41:49Z

Update: synced offline with @jackpope to improve the error diagnostic. Also checked that this is correctly reported by our eslint plugin.

This PR should now be ready to merge

Traverse program after running compiler transform to find untransformed references to compiler features (e.g. `inferEffectDeps`, `fire`). Hard error to fail the babel pipeline when the compiler fails to transform these features to give predictable runtime semantics. Untransformed calls to functions like `fire` will throw at runtime anyways, so let's fail the build to catch these earlier. Note that with this fails the build *regardless of panicThreshold* DiffTrain build for [5398b71](5398b71)

mofeiZ mentioned this pull request Mar 4, 2025

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

Merged

facebook-github-bot added the CLA Signed label Mar 4, 2025

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

vercel bot had a problem deploying to Preview March 4, 2025 05:03 Failure

mofeiZ force-pushed the pr32512 branch from c5f8ddb to 83c32ef Compare March 4, 2025 14:39

vercel bot deployed to Preview March 4, 2025 14:41 View deployment

mofeiZ force-pushed the pr32512 branch from 83c32ef to 61002b1 Compare March 4, 2025 14:42

vercel bot deployed to Preview March 4, 2025 14:43 View deployment

mofeiZ commented Mar 4, 2025

View reviewed changes

mofeiZ marked this pull request as ready for review March 4, 2025 17:24

mofeiZ changed the title ~~[compiler] detect and hard error on untransformed required features~~ [compiler] detect and throw on untransformed required features Mar 4, 2025

mofeiZ force-pushed the pr32512 branch from 61002b1 to d1569ea Compare March 4, 2025 23:15

vercel bot deployed to Preview March 4, 2025 23:16 View deployment

mofeiZ commented Mar 5, 2025

View reviewed changes

mofeiZ force-pushed the pr32512 branch from d1569ea to 4b91f12 Compare March 5, 2025 15:23

vercel bot deployed to Preview March 5, 2025 15:24 View deployment

jbrown215 approved these changes Mar 5, 2025

View reviewed changes

jackpope approved these changes Mar 12, 2025

View reviewed changes

mofeiZ force-pushed the pr32512 branch from 4b91f12 to 9c76224 Compare March 13, 2025 22:01

vercel bot deployed to Preview March 13, 2025 22:04 View deployment

mofeiZ force-pushed the pr32512 branch from 9c76224 to eeb1eae Compare March 13, 2025 23:33

vercel bot deployed to Preview March 13, 2025 23:34 View deployment

mofeiZ force-pushed the pr32512 branch from eeb1eae to 674803e Compare March 13, 2025 23:47

vercel bot deployed to Preview March 13, 2025 23:48 View deployment

mofeiZ force-pushed the pr32512 branch from 674803e to 63c1bc8 Compare March 13, 2025 23:55

vercel bot deployed to Preview March 13, 2025 23:56 View deployment

mofeiZ force-pushed the pr32512 branch from 63c1bc8 to 25be14e Compare March 13, 2025 23:57

vercel bot deployed to Preview March 13, 2025 23:58 View deployment

mofeiZ force-pushed the pr32512 branch from 25be14e to a6c39c3 Compare March 14, 2025 13:21

vercel bot deployed to Preview March 14, 2025 13:22 View deployment

mofeiZ force-pushed the pr32512 branch from a6c39c3 to 87a6da3 Compare March 14, 2025 15:38

vercel bot deployed to Preview March 14, 2025 15:39 View deployment

mofeiZ force-pushed the pr32512 branch from 87a6da3 to 3e8c356 Compare March 14, 2025 15:40

vercel bot deployed to Preview March 14, 2025 15:41 View deployment

mofeiZ merged commit 5398b71 into main Mar 14, 2025
33 of 34 checks passed

vercel-release-bot mentioned this pull request Mar 14, 2025

Upgrade React from 6aa8254b-20250312 to 5398b711-20250314 vercel/next.js#77129

Merged

poteto deleted the pr32512 branch March 26, 2025 22:36

		> 20 \| useEffect(f);
		\| ^^^^^^^^^^^^ Todo: Untransformed reference to experimental compiler-only feature (20:20)

[compiler] detect and throw on untransformed required features #32512

[compiler] detect and throw on untransformed required features #32512

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbrown215 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jackpope left a comment

Choose a reason for hiding this comment

Uh oh!

mofeiZ commented Mar 14, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mofeiZ commented Mar 4, 2025 •

edited

Loading

mofeiZ Mar 4, 2025 •

edited

Loading