[compiler] More fbt compatibility #34887

josephsavona · 2025-10-16T23:48:12Z

In my previous PR I fixed some cases but broke others. So, new approach. Two phase algorithm:

First pass is forward data flow to determine all usages of macros. This is necessary because many of Meta's macros have variants that can be accessed via properties, eg you can do macro(...) but also macro.variant(...).
Second pass is backwards data flow to find macro invocations (JSX and calls) and then merge their operands into the same scope as the macro call.

Note that this required updating PromoteUsedTemporaries to avoid promoting macro calls that have interposing instructions between their creation and usage. Macro calls in general are pure so it should be safe to reorder them.

In addition, we're now more precise about <fb:plural>, <fbt:param>, fbt.plural() and fbt.param(), which don't actually require all their arguments to be inlined. The whole point is that the plural/param value is an arbitrary value (along with a string name). So we no longer transitively inline the arguments, we just make sure that they don't get inadvertently promoted to named variables.

One caveat: we actually don't do anything to treat macro functions as non-mutating, so fbt.plural() and friends (function form) may still sometimes group arguments just due to mutability inference. In a follow-up, i'll work to infer the types of nested macro functions as non-mutating.

Stack created with Sapling. Best reviewed with ReviewStack.

...ages/babel-plugin-react-compiler/src/ReactiveScopes/MemoizeFbtAndMacroOperandsInSameScope.ts

josephsavona · 2025-10-17T15:59:08Z

...abel-plugin-react-compiler/src/__tests__/fixtures/compiler/idx-method-no-outlining.expect.md

  let t0;
  if ($[0] !== props) {
-    t0 = idx(props, _temp);
+    t0 = idx(props, (_) => _.group.label);


expected: we treat all custom macros as needing their args to be transitively memoized. this file previously only targeted a subset of idx

josephsavona · 2025-10-17T15:59:44Z

...in-react-compiler/src/__tests__/fixtures/compiler/idx-method-no-outlining-wildcard.expect.md

  let t1;
  if ($[2] !== props) {
-    t1 = idx.a(props, _temp2);
+    t1 = idx.a(props, (__0) => __0.group.label);


similar to the other idx case, all sub-properties of custom macros are treated as needing transitive inlining of their args

josephsavona · 2025-10-17T16:00:29Z

...abel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/repro-fbt-param-nested-fbt.js

  'use memo';
  return (
-    <Stringify>
+    <div>


rewritten so that we can actually check the output

josephsavona · 2025-10-17T16:00:34Z

...ugin-react-compiler/src/__tests__/fixtures/compiler/fbt/repro-fbt-param-nested-fbt.expect.md

      
 ### Eval output
-(kind: ok) <div>{"children":"Name: , "}</div>
+(kind: ok) <div>Name: first, (inner)last</div>


josephsavona · 2025-10-17T16:01:11Z

...eact-compiler/src/__tests__/fixtures/compiler/fbt/fbtparam-with-jsx-fragment-value.expect.md

+    const t1 = identity(props.text);
+    let t2;
+    if ($[2] !== t1) {
+      t2 = <>{t1}</>;
+      $[2] = t1;
+      $[3] = t2;
+    } else {
+      t2 = $[3];
+    }


fbt.param values are allowed to be independently memoized

josephsavona · 2025-10-17T16:01:27Z

...act-compiler/src/__tests__/fixtures/compiler/fbt/fbtparam-with-jsx-element-content.expect.md

-
-              <Text type="h4">{name}</Text>,
-            ),
+            fbt._param("item author", t2),


fbt param values are allowed to be independently memoized

josephsavona · 2025-10-17T16:01:39Z

...-compiler/src/__tests__/fixtures/compiler/fbt/fbt-preserve-whitespace-two-subtrees.expect.md

-        fbt._param("user1", <span key={name1}>{t2}</span>),
-        fbt._param("user2", <span key={name2}>{t3}</span>),
-      ],
+      [fbt._param("user1", t3), fbt._param("user2", t5)],


josephsavona · 2025-10-17T16:01:47Z

...ct-compiler/src/__tests__/fixtures/compiler/fbt/fbt-param-with-trailing-whitespace.expect.md

          { "*": "{count} votes for {option}", _1: "1 vote for {option}" },
          [
-            fbt._plural(identity(props.count), "count"),
+            fbt._plural(t1, "count"),


same for fbt.plural

josephsavona · 2025-10-17T16:02:39Z

...ages/babel-plugin-react-compiler/src/ReactiveScopes/MemoizeFbtAndMacroOperandsInSameScope.ts

-
-  visit(fn, fbtMacroTags, macroTagsCalls, macroMethods, macroValues);
-
-  for (const root of macroValues.keys()) {


no more fixpoint loop, this should be faster

mofeiZ · 2025-10-17T16:15:17Z

...ages/babel-plugin-react-compiler/src/ReactiveScopes/MemoizeFbtAndMacroOperandsInSameScope.ts

+        case 'CallExpression':
+        case 'MethodCall': {
+          const scope = lvalue.identifier.scope;
+          if (scope == null) {


In following case,

we're processing a transitive macro (e.g. imagine a function that returned a boolean but expected its arguments to be ast-static)

lvalue is unscoped due to type inference knowing it is a primitive type

the function call arguments are scoped

// ModuleDefinition: `macro` is non-mutating and returns a primitive const isEnabled: boolean = macro('MyFeatureFlag', { staticKey: "staticValue" });

we may need to prune the scope of { staticKey: "staticValue" }

Hmm, note that we visit in reverse order, so if the parent needed its operands inlined, it would have already updated their lvalues to have a scope

mofeiZ

Makes sense to land for now if this passes all internal checks. I still wonder if this will cause bugs for cases like the following. But we can fix it if it becomes an issue / when we add typing for fbt

// source
fbt.param("name", foo({}))

// potential output
const t0 = "name";
const t1 = foo;
let t2;
if (/* inputs changed */) { t2 = {} } else { /* reuse */ }
fbt.param(t0, t1(t2))

In my previous PR I fixed some cases but broke others. So, new approach. Two phase algorithm: * First pass is forward data flow to determine all usages of macros. This is necessary because many of Meta's macros have variants that can be accessed via properties, eg you can do `macro(...)` but also `macro.variant(...)`. * Second pass is backwards data flow to find macro invocations (JSX and calls) and then merge their operands into the same scope as the macro call. Note that this required updating PromoteUsedTemporaries to avoid promoting macro calls that have interposing instructions between their creation and usage. Macro calls in general are pure so it should be safe to reorder them. In addition, we're now more precise about `<fb:plural>`, `<fbt:param>`, `fbt.plural()` and `fbt.param()`, which don't actually require all their arguments to be inlined. The whole point is that the plural/param value is an arbitrary value (along with a string name). So we no longer transitively inline the arguments, we just make sure that they don't get inadvertently promoted to named variables. One caveat: we actually don't do anything to treat macro functions as non-mutating, so `fbt.plural()` and friends (function form) may still sometimes group arguments just due to mutability inference. In a follow-up, i'll work to infer the types of nested macro functions as non-mutating.

josephsavona · 2025-10-17T18:22:51Z

I still wonder if this will cause bugs for cases like the following

I double-checked and I can't construct a case where PromoteUsedTemporaries is an issue now. You're right that if we add typing for macro functions this might be an issue but we can tackle that all together.

In my previous PR I fixed some cases but broke others. So, new approach. Two phase algorithm: * First pass is forward data flow to determine all usages of macros. This is necessary because many of Meta's macros have variants that can be accessed via properties, eg you can do `macro(...)` but also `macro.variant(...)`. * Second pass is backwards data flow to find macro invocations (JSX and calls) and then merge their operands into the same scope as the macro call. Note that this required updating PromoteUsedTemporaries to avoid promoting macro calls that have interposing instructions between their creation and usage. Macro calls in general are pure so it should be safe to reorder them. In addition, we're now more precise about `<fb:plural>`, `<fbt:param>`, `fbt.plural()` and `fbt.param()`, which don't actually require all their arguments to be inlined. The whole point is that the plural/param value is an arbitrary value (along with a string name). So we no longer transitively inline the arguments, we just make sure that they don't get inadvertently promoted to named variables. One caveat: we actually don't do anything to treat macro functions as non-mutating, so `fbt.plural()` and friends (function form) may still sometimes group arguments just due to mutability inference. In a follow-up, i'll work to infer the types of nested macro functions as non-mutating. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34887). * #34900 * __->__ #34887 DiffTrain build for [adbc32d](adbc32d)

@poteto

As part of the new inference model we updated to (correctly) treat destructuring spread as creating a new mutable object. This had the unfortunate side-effect of reducing precision on destructuring of props, though: ```js function Component({x, ...rest}) { const z = rest.z; identity(z); return <Stringify x={x} z={z} />; } ``` Memoized as the following, where we don't realize that `z` is actually frozen: ```js function Component(t0) { const $ = _c(6); let x; let z; if ($[0] !== t0) { const { x: t1, ...rest } = t0; x = t1; z = rest.z; identity(z); ... ``` #34341 was our first thought of how to do this (thanks @poteto for exploring this idea!). But during review it became clear that it was a bit more complicated than I had thought. So this PR explores a more conservative alternative. The idea is: * Track known sources of frozen values: component props, hook params, and hook return values. * Find all object spreads where the rvalue is a known frozen value. * Look at how such objects are used, and if they are only used to access properties (PropertyLoad/Destructure), pass to hooks, or pass to jsx then we can be very confident the object is not mutated. We consider any such objects to be frozen, even though technically spread creates a new object. See new fixtures for more examples. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34900). * __->__ #34900 * #34887

@poteto

As part of the new inference model we updated to (correctly) treat destructuring spread as creating a new mutable object. This had the unfortunate side-effect of reducing precision on destructuring of props, though: ```js function Component({x, ...rest}) { const z = rest.z; identity(z); return <Stringify x={x} z={z} />; } ``` Memoized as the following, where we don't realize that `z` is actually frozen: ```js function Component(t0) { const $ = _c(6); let x; let z; if ($[0] !== t0) { const { x: t1, ...rest } = t0; x = t1; z = rest.z; identity(z); ... ``` #34341 was our first thought of how to do this (thanks @poteto for exploring this idea!). But during review it became clear that it was a bit more complicated than I had thought. So this PR explores a more conservative alternative. The idea is: * Track known sources of frozen values: component props, hook params, and hook return values. * Find all object spreads where the rvalue is a known frozen value. * Look at how such objects are used, and if they are only used to access properties (PropertyLoad/Destructure), pass to hooks, or pass to jsx then we can be very confident the object is not mutated. We consider any such objects to be frozen, even though technically spread creates a new object. See new fixtures for more examples. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34900). * __->__ #34900 * #34887 DiffTrain build for [c35f6a3](c35f6a3)

@poteto

As part of the new inference model we updated to (correctly) treat destructuring spread as creating a new mutable object. This had the unfortunate side-effect of reducing precision on destructuring of props, though: ```js function Component({x, ...rest}) { const z = rest.z; identity(z); return <Stringify x={x} z={z} />; } ``` Memoized as the following, where we don't realize that `z` is actually frozen: ```js function Component(t0) { const $ = _c(6); let x; let z; if ($[0] !== t0) { const { x: t1, ...rest } = t0; x = t1; z = rest.z; identity(z); ... ``` #34341 was our first thought of how to do this (thanks @poteto for exploring this idea!). But during review it became clear that it was a bit more complicated than I had thought. So this PR explores a more conservative alternative. The idea is: * Track known sources of frozen values: component props, hook params, and hook return values. * Find all object spreads where the rvalue is a known frozen value. * Look at how such objects are used, and if they are only used to access properties (PropertyLoad/Destructure), pass to hooks, or pass to jsx then we can be very confident the object is not mutated. We consider any such objects to be frozen, even though technically spread creates a new object. See new fixtures for more examples. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34900). * __->__ #34900 * #34887 DiffTrain build for [c35f6a3](c35f6a3)

In my previous PR I fixed some cases but broke others. So, new approach. Two phase algorithm: * First pass is forward data flow to determine all usages of macros. This is necessary because many of Meta's macros have variants that can be accessed via properties, eg you can do `macro(...)` but also `macro.variant(...)`. * Second pass is backwards data flow to find macro invocations (JSX and calls) and then merge their operands into the same scope as the macro call. Note that this required updating PromoteUsedTemporaries to avoid promoting macro calls that have interposing instructions between their creation and usage. Macro calls in general are pure so it should be safe to reorder them. In addition, we're now more precise about `<fb:plural>`, `<fbt:param>`, `fbt.plural()` and `fbt.param()`, which don't actually require all their arguments to be inlined. The whole point is that the plural/param value is an arbitrary value (along with a string name). So we no longer transitively inline the arguments, we just make sure that they don't get inadvertently promoted to named variables. One caveat: we actually don't do anything to treat macro functions as non-mutating, so `fbt.plural()` and friends (function form) may still sometimes group arguments just due to mutability inference. In a follow-up, i'll work to infer the types of nested macro functions as non-mutating. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34887). * facebook#34900 * __->__ facebook#34887 DiffTrain build for [adbc32d](facebook@adbc32d)

meta-cla bot added the CLA Signed label Oct 16, 2025

josephsavona requested review from mofeiZ and mvitousek and removed request for mofeiZ October 16, 2025 23:48

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

josephsavona commented Oct 16, 2025

View reviewed changes

...ages/babel-plugin-react-compiler/src/ReactiveScopes/MemoizeFbtAndMacroOperandsInSameScope.ts Outdated Show resolved Hide resolved

josephsavona force-pushed the pr34887 branch 5 times, most recently from 46fa6c2 to 08cbe39 Compare October 17, 2025 15:49

josephsavona marked this pull request as ready for review October 17, 2025 15:50

josephsavona force-pushed the pr34887 branch from 08cbe39 to aa36cf4 Compare October 17, 2025 15:52

josephsavona commented Oct 17, 2025

View reviewed changes

mofeiZ reviewed Oct 17, 2025

View reviewed changes

josephsavona force-pushed the pr34887 branch from aa36cf4 to a7afe44 Compare October 17, 2025 16:37

mofeiZ approved these changes Oct 17, 2025

View reviewed changes

josephsavona mentioned this pull request Oct 17, 2025

[compiler] Optimize props spread for common cases #34900

Merged

josephsavona force-pushed the pr34887 branch from a7afe44 to 38b39be Compare October 17, 2025 18:12

josephsavona merged commit adbc32d into main Oct 17, 2025
20 checks passed

This was referenced Oct 17, 2025

[compiler][rfc] Switch eslint-plugin-react-compiler to single rule for Meta compat #34903

Draft

[compiler] Fix false positive for useMemo reassigning context vars #34904

Open


		visit(fn, fbtMacroTags, macroTagsCalls, macroMethods, macroValues);

		for (const root of macroValues.keys()) {

[compiler] More fbt compatibility #34887

[compiler] More fbt compatibility #34887

Conversation

josephsavona commented Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mofeiZ left a comment • 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!

josephsavona commented Oct 17, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

josephsavona commented Oct 16, 2025 •

edited

Loading

mofeiZ left a comment •

edited

Loading