[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

This PR also throws retry pipeline errors, which shows better errors than the current generic TODO message

Stack created with Sapling. Best reviewed with ReviewStack.

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)

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

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)

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* This PR also throws retry pipeline errors, which shows better errors than the current generic TODO message

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?

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?

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)

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

mofeiZ mentioned this pull request Mar 4, 2025

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

Open

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mofeiZ commented Mar 4, 2025 •

edited

Loading

mofeiZ Mar 4, 2025 •

edited

Loading

mofeiZ Mar 4, 2025

mofeiZ Mar 4, 2025

mofeiZ Mar 5, 2025

jbrown215 left a comment

jbrown215 Mar 5, 2025

jbrown215 Mar 5, 2025

jbrown215 Mar 5, 2025

jbrown215 Mar 5, 2025

jbrown215 Mar 5, 2025

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

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

Are you sure you want to change the base?

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

Conversation

mofeiZ commented Mar 4, 2025 • edited Loading

This PR also throws retry pipeline errors, which shows better errors than the current generic TODO message

mofeiZ Mar 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrown215 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mofeiZ commented Mar 4, 2025 •

edited

Loading

mofeiZ Mar 4, 2025 •

edited

Loading