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

josephsavona · 2025-06-25T18:02:45Z

I realized a pattern of global mutations that we don't currently detect (both in the old and new inference models). If you hide the mutation inside a function returned from another function, we lose track of it:

const f = () => () => {
  global.property = true;
};
f()();

Roughly speaking, we need to track that if the return value of f is mutated, that it should count as triggering some effects. Right now we encode the idea that a function specifically can have side effects if it is mutated, but other values don't have a way to represent this. I'm thinking that we change the shape of the Create effect a bit, and allow room for an optional "mutation effects" array. Then, InferMutationAliasingRanges can visit these effects like it does when trying to find transitive function effects.

Stack created with Sapling. Best reviewed with ReviewStack.

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

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

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.

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.

I realized a pattern of global mutations that we don't currently detect (both in the old and new inference models). If you hide the mutation inside a function returned from another function, we lose track of it: ```js const f = () => () => { global.property = true; }; f()(); ``` Roughly speaking, we need to track that if the return value of `f` is mutated, that it should count as triggering some effects. Right now we encode the idea that a function specifically can have side effects if it is mutated, but other values don't have a way to represent this. I'm thinking that we change the shape of the `Create` effect a bit, and allow room for an optional "mutation effects" array. Then, InferMutationAliasingRanges can visit these effects like it does when trying to find transitive function effects.

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

josephsavona marked this pull request as draft June 25, 2025 18:02

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

josephsavona mentioned this pull request Jun 25, 2025

[compiler] Infer render helpers for additional validation #33647

Open

josephsavona force-pushed the pr33643 branch 2 times, most recently from 34cb6e6 to d82f755 Compare June 26, 2025 02:18

josephsavona mentioned this pull request Jun 26, 2025

[compiler] InferEffects uses effects as value keys #33650

Open

josephsavona added 4 commits June 26, 2025 17:07

josephsavona force-pushed the pr33643 branch from d82f755 to 60f898e Compare June 27, 2025 00:07

kushxg mentioned this pull request Jul 1, 2025

Mirror of upstream PR #33643 kushxg/react-mirror#182

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

josephsavona commented Jun 25, 2025 •

edited

Loading

Uh oh!

Uh oh!

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Uh oh!

josephsavona commented Jun 25, 2025 •

edited

Loading