[compiler] Propagate CreateFunction effects for functions that return functions by josephsavona · Pull Request #33642 · facebook/react

josephsavona · 2025-06-25T17:56:19Z

If you have a local helper function that itself returns a function (() => () => { ... }), we currently infer the return effect of the outer function as Create mutable. We correctly track the aliasing, but we lose some precision because we don't understand that a function specifically is being returned.

Here, we do some extra analysis of which values are returned in InferMutationAliasingRanges, and if the sole return value is a function we infer a CreateFunction effect. We also infer an Assign (instead of a Create) if the sole return value was one of the context variables or parameters.

Stack created with Sapling. Best reviewed with ReviewStack.

josephsavona · 2025-06-25T17:58:23Z

...compiler/src/__tests__/fixtures/compiler/repro-returned-inner-fn-reassigns-context.expect.md

+    t1 = $[1];
+  }
+  const arr = t1;
+  fn(arr);


Before, all we knew is that fn (created by fnFactory) is a mutable value, and therefore could be mutating arr. Now we know that fn is a function, with a specific signature, and that it doesn't mutate it's argument. That means we can memoize arr independently.

josephsavona · 2025-06-25T17:58:45Z

compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts

+  const returned: Set<Node> = new Set();
+  const queue: Array<Node> = [state.nodes.get(fn.returns.identifier)!];
+  const seen: Set<Node> = new Set();
+  while (queue.length !== 0) {
+    const node = queue.pop()!;
+    if (seen.has(node)) {
+      continue;
+    }
+    seen.add(node);
+    for (const id of node.aliases.keys()) {
+      queue.push(state.nodes.get(id)!);
+    }
+    for (const id of node.createdFrom.keys()) {
+      queue.push(state.nodes.get(id)!);
+    }
+    if (node.id.id === fn.returns.identifier.id) {
+      continue;
+    }
+    switch (node.value.kind) {
+      case 'Assign':
+      case 'CreateFrom': {
+        break;
+      }
+      case 'Phi':
+      case 'Object':
+      case 'Function': {
+        returned.add(node);
+        break;
+      }
+      default: {
+        assertExhaustive(
+          node.value,
+          `Unexpected node value kind '${(node.value as any).kind}'`,
+        );
+      }
+    }
+  }


i want to clean this logic up a bit before landing

…33624) Closes #33577, a bug with ExtractScopeDeclarationsFromDestructuring and codegen when a function param is reassigned. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33624). * #33643 * #33642 * #33640 * #33625 * __->__ #33624

Small cosmetic win, found this when i was looking at some code internally with lots of cases that all share the same logic. Previously, all the but last one would have an empty block. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33625). * #33643 * #33642 * #33640 * __->__ #33625 * #33624

We now have `HIRFunction.returns: Place` as well as `returnType: Type`. I want to add additional return information, so as a first step i'm consolidating everything under an object at `HIRFunction.returns: {place: Place}`. We use the type of this place as the return type. Next step is to add more properties to this object to represent things like the return kind. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33640). * #33643 * #33642 * __->__ #33640 * #33625 * #33624

Small cosmetic win, found this when i was looking at some code internally with lots of cases that all share the same logic. Previously, all the but last one would have an empty block. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33625). * #33643 * #33642 * #33640 * __->__ #33625 * #33624 DiffTrain build for [e130c08](e130c08)

We now have `HIRFunction.returns: Place` as well as `returnType: Type`. I want to add additional return information, so as a first step i'm consolidating everything under an object at `HIRFunction.returns: {place: Place}`. We use the type of this place as the return type. Next step is to add more properties to this object to represent things like the return kind. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33640). * #33643 * #33642 * __->__ #33640 * #33625 * #33624 DiffTrain build for [123ff13](123ff13)

…33624) Closes #33577, a bug with ExtractScopeDeclarationsFromDestructuring and codegen when a function param is reassigned. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33624). * #33643 * #33642 * #33640 * #33625 * __->__ #33624 DiffTrain build for [9894c48](9894c48)

We currently assume that any functions passes as props may be event handlers or effect functions, and thus don't check for side effects such as mutating globals. However, if a prop is a function that returns JSX that is a sure sign that it's actually a render helper and not an event handler or effect function. So we now emit a `Render` effect for any prop that is a JSX-returning function, triggering all of our render validation. This required a small fix to InferTypes: we weren't correctly populating the `return` type of function types during unification. I also improved the printing of types so we can see the inferred return types. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33647). * #33643 * #33650 * #33642 * __->__ #33647

We currently assume that any functions passes as props may be event handlers or effect functions, and thus don't check for side effects such as mutating globals. However, if a prop is a function that returns JSX that is a sure sign that it's actually a render helper and not an event handler or effect function. So we now emit a `Render` effect for any prop that is a JSX-returning function, triggering all of our render validation. This required a small fix to InferTypes: we weren't correctly populating the `return` type of function types during unification. I also improved the printing of types so we can see the inferred return types. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33647). * #33643 * #33650 * #33642 * __->__ #33647 DiffTrain build for [33a1095](33a1095)

In InferReferenceEffects we used `InstructionValue` as the key to represent values, since each time we process an instruction this object will be the same. However this was always a bit of a hack, and in the new model and InferMutationAliasingEffects we can instead use the (creation) effect as the stable value. This avoids an extra layer of memoization since the effects are already interned anyway.

… functions If you have a local helper function that itself returns a function (`() => () => { ... }`), we currently infer the return effect of the outer function as `Create mutable`. We correctly track the aliasing, but we lose some precision because we don't understand that a function specifically is being returned. Here, we do some extra analysis of which values are returned in InferMutationAliasingRanges, and if the sole return value is a function we infer a `CreateFunction` effect. We also infer an `Assign` (instead of a Create) if the sole return value was one of the context variables or parameters.

This was referenced Jun 25, 2025

[compiler] Consolidate HIRFunction return information #33640

Merged

[compiler] Fix bug with reassigning function param in destructuring #33624

Merged

[compiler] Avoid empty switch cases #33625

Merged

facebook-github-bot added the CLA Signed label Jun 25, 2025

josephsavona marked this pull request as draft June 25, 2025 17:56

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

josephsavona commented Jun 25, 2025

View reviewed changes

josephsavona mentioned this pull request Jun 25, 2025

[compiler] Repro for missed case of global mutation #33643

Draft

This was referenced Jun 25, 2025

[compiler] Infer render helpers for additional validation #33647

Merged

[compiler] InferEffects uses effects as value keys #33650

Open

josephsavona force-pushed the pr33642 branch from 87dc463 to 5415e4e Compare June 27, 2025 00:07

josephsavona force-pushed the pr33642 branch 3 times, most recently from d7a7cfa to 7bf2cc9 Compare August 7, 2025 06:18

josephsavona added 2 commits August 28, 2025 14:55

josephsavona force-pushed the pr33642 branch from 7bf2cc9 to d2b2a1b Compare August 28, 2025 21:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[compiler] Propagate CreateFunction effects for functions that return functions#33642

[compiler] Propagate CreateFunction effects for functions that return functions#33642
josephsavona wants to merge 2 commits intomainfrom
pr33642

josephsavona commented Jun 25, 2025 •

edited

Loading

Uh oh!

josephsavona Jun 25, 2025

Uh oh!

josephsavona Jun 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

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

Uh oh!

Uh oh!

josephsavona Jun 25, 2025

Choose a reason for hiding this comment

Uh oh!

josephsavona Jun 25, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

josephsavona commented Jun 25, 2025 •

edited

Loading