@samitouri / QOS-React-1 / commits / 8570116bd1

[compiler] Fix for uncalled functions that are known-mutable

If a function captures a mutable value but never gets called, we don't infer a mutable range for that function. This means that we also don't alias the function with its mutable captures. This case is tricky, because we don't generally know for sure what is a mutation and what may just be a normal function call. For example: ```js hook useFoo() { const x = makeObject(); return () => { return readObject(x); // could be a mutation! } } ``` If we pessimistically assume that all such cases are mutations, we'd have to group lots of memo scopes together unnecessarily. However, if there is definitely a mutation: ```js hook useFoo(createEntryForKey) { const cache = new WeakMap(); return (key) => { let entry = cache.get(key); if (entry == null) { entry = createEntryForKey(key); cache.set(key, entry); // known mutation! } return entry; } } ``` Then we have to ensure that the function and its mutable captures alias together and end up in the same scope. However, aliasing together isn't enough if the function and operands all have empty mutable ranges (end = start + 1). This pass finds function expressions and object methods that have an empty mutable range and known-mutable operands which also don't have a mutable range, and ensures that the function and those operands are aliased together *and* that their ranges are updated to end after the function expression. This is sufficient to ensure that a reactive scope is created for the alias set. NOTE: The alternative is to reject these cases. If we do that we'd also want to similarly disallow cases like passing a mutable function to a hook. ghstack-source-id: 5d8158246a320e80d8da3f0e395ac1953d8920a2 Pull Request resolved: https://github.com/facebook/react/pull/33078

Joe Savona committed May 3, 2025 at 09:15 UTC 8570116bd1ba872537132de42b997ec56c726659
4 files changed +251
compiler/packages/babel-plugin-react-compiler/src/Inference/InerAliasForUncalledFunctions.ts new
+134
@@ -0,0 +1,134 @@
1 +/**
2 + * Copyright (c) Meta Platforms, Inc. and affiliates.
3 + *
4 + * This source code is licensed under the MIT license found in the
5 + * LICENSE file in the root directory of this source tree.
6 + */
7 +
8 +import {
9 + Effect,
10 + HIRFunction,
11 + Identifier,
12 + isMutableEffect,
13 + isRefOrRefLikeMutableType,
14 + makeInstructionId,
15 +} from '../HIR/HIR';
16 +import {eachInstructionValueOperand} from '../HIR/visitors';
17 +import {isMutable} from '../ReactiveScopes/InferReactiveScopeVariables';
18 +import DisjointSet from '../Utils/DisjointSet';
19 +
20 +/**
21 + * If a function captures a mutable value but never gets called, we don't infer a
22 + * mutable range for that function. This means that we also don't alias the function
23 + * with its mutable captures.
24 + *
25 + * This case is tricky, because we don't generally know for sure what is a mutation
26 + * and what may just be a normal function call. For example:
27 + *
28 + * ```
29 + * hook useFoo() {
30 + * const x = makeObject();
31 + * return () => {
32 + * return readObject(x); // could be a mutation!
33 + * }
34 + * }
35 + * ```
36 + *
37 + * If we pessimistically assume that all such cases are mutations, we'd have to group
38 + * lots of memo scopes together unnecessarily. However, if there is definitely a mutation:
39 + *
40 + * ```
41 + * hook useFoo(createEntryForKey) {
42 + * const cache = new WeakMap();
43 + * return (key) => {
44 + * let entry = cache.get(key);
45 + * if (entry == null) {
46 + * entry = createEntryForKey(key);
47 + * cache.set(key, entry); // known mutation!
48 + * }
49 + * return entry;
50 + * }
51 + * }
52 + * ```
53 + *
54 + * Then we have to ensure that the function and its mutable captures alias together and
55 + * end up in the same scope. However, aliasing together isn't enough if the function
56 + * and operands all have empty mutable ranges (end = start + 1).
57 + *
58 + * This pass finds function expressions and object methods that have an empty mutable range
59 + * and known-mutable operands which also don't have a mutable range, and ensures that the
60 + * function and those operands are aliased together *and* that their ranges are updated to
61 + * end after the function expression. This is sufficient to ensure that a reactive scope is
62 + * created for the alias set.
63 + */
64 +export function inferAliasForUncalledFunctions(
65 + fn: HIRFunction,
66 + aliases: DisjointSet<Identifier>,
67 +): void {
68 + for (const block of fn.body.blocks.values()) {
69 + instrs: for (const instr of block.instructions) {
70 + const {lvalue, value} = instr;
71 + if (
72 + value.kind !== 'ObjectMethod' &&
73 + value.kind !== 'FunctionExpression'
74 + ) {
75 + continue;
76 + }
77 + /*
78 + * If the function is known to be mutated, we will have
79 + * already aliased any mutable operands with it
80 + */
81 + const range = lvalue.identifier.mutableRange;
82 + if (range.end > range.start + 1) {
83 + continue;
84 + }
85 + /*
86 + * If the function already has operands with an active mutable range,
87 + * then we don't need to do anything — the function will have already
88 + * been visited and included in some mutable alias set. This case can
89 + * also occur due to visiting the same function in an earlier iteration
90 + * of the outer fixpoint loop.
91 + */
92 + for (const operand of eachInstructionValueOperand(value)) {
93 + if (isMutable(instr, operand)) {
94 + continue instrs;
95 + }
96 + }
97 + const operands: Set<Identifier> = new Set();
98 + for (const effect of value.loweredFunc.func.effects ?? []) {
99 + if (effect.kind !== 'ContextMutation') {
100 + continue;
101 + }
102 + /*
103 + * We're looking for known-mutations only, so we look at the effects
104 + * rather than function context
105 + */
106 + if (effect.effect === Effect.Store || effect.effect === Effect.Mutate) {
107 + for (const operand of effect.places) {
108 + /*
109 + * It's possible that function effect analysis thinks there was a context mutation,
110 + * but then InferReferenceEffects figures out some operands are globals and therefore
111 + * creates a non-mutable effect for those operands.
112 + * We should change InferReferenceEffects to swap the ContextMutation for a global
113 + * mutation in that case, but for now we just filter them out here
114 + */
115 + if (
116 + isMutableEffect(operand.effect, operand.loc) &&
117 + !isRefOrRefLikeMutableType(operand.identifier.type)
118 + ) {
119 + operands.add(operand.identifier);
120 + }
121 + }
122 + }
123 + }
124 + if (operands.size !== 0) {
125 + operands.add(lvalue.identifier);
126 + aliases.union([...operands]);
127 + // Update mutable ranges, if the ranges are empty then a reactive scope isn't created
128 + for (const operand of operands) {
129 + operand.mutableRange.end = makeInstructionId(instr.id + 1);
130 + }
131 + }
132 + }
133 + }
134 +}
compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutableRanges.ts
+2
@@ -6,6 +6,7 @@
6 */
7
8 import {HIRFunction, Identifier} from '../HIR/HIR';
9 +import {inferAliasForUncalledFunctions} from './InerAliasForUncalledFunctions';
10 import {inferAliases} from './InferAlias';
11 import {inferAliasForPhis} from './InferAliasForPhis';
12 import {inferAliasForStores} from './InferAliasForStores';
@@ -76,6 +77,7 @@ export function inferMutableRanges(ir: HIRFunction): void {
77 while (true) {
78 inferMutableRangesForAlias(ir, aliases);
79 inferAliasForPhis(ir, aliases);
80 + inferAliasForUncalledFunctions(ir, aliases);
81 const nextAliases = aliases.canonicalize();
82 if (areEqualMaps(prevAliases, nextAliases)) {
83 break;
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-uncalled-function-capturing-mutable-values-memoizes-with-captures-values.expect.md new
+77
@@ -0,0 +1,77 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +// @flow
6 +/**
7 + * This hook returns a function that when called with an input object,
8 + * will return the result of mapping that input with the supplied map
9 + * function. Results are cached, so if the same input is passed again,
10 + * the same output object will be returned.
11 + *
12 + * Note that this technically violates the rules of React and is unsafe:
13 + * hooks must return immutable objects and be pure, and a function which
14 + * captures and mutates a value when called is inherently not pure.
15 + *
16 + * However, in this case it is technically safe _if_ the mapping function
17 + * is pure *and* the resulting objects are never modified. This is because
18 + * the function only caches: the result of `returnedFunction(someInput)`
19 + * strictly depends on `returnedFunction` and `someInput`, and cannot
20 + * otherwise change over time.
21 + */
22 +hook useMemoMap<TInput: interface {}, TOutput>(
23 + map: TInput => TOutput
24 +): TInput => TOutput {
25 + return useMemo(() => {
26 + // The original issue is that `cache` was not memoized together with the returned
27 + // function. This was because neither appears to ever be mutated — the function
28 + // is known to mutate `cache` but the function isn't called.
29 + //
30 + // The fix is to detect cases like this — functions that are mutable but not called -
31 + // and ensure that their mutable captures are aliased together into the same scope.
32 + const cache = new WeakMap<TInput, TOutput>();
33 + return input => {
34 + let output = cache.get(input);
35 + if (output == null) {
36 + output = map(input);
37 + cache.set(input, output);
38 + }
39 + return output;
40 + };
41 + }, [map]);
42 +}
43 +
44 +```
45 +
46 +## Code
47 +
48 +```javascript
49 +import { c as _c } from "react/compiler-runtime";
50 +
51 +function useMemoMap(map) {
52 + const $ = _c(2);
53 + let t0;
54 + let t1;
55 + if ($[0] !== map) {
56 + const cache = new WeakMap();
57 + t1 = (input) => {
58 + let output = cache.get(input);
59 + if (output == null) {
60 + output = map(input);
61 + cache.set(input, output);
62 + }
63 + return output;
64 + };
65 + $[0] = map;
66 + $[1] = t1;
67 + } else {
68 + t1 = $[1];
69 + }
70 + t0 = t1;
71 + return t0;
72 +}
73 +
74 +```
75 +
76 +### Eval output
77 +(kind: exception) Fixture not implemented
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-uncalled-function-capturing-mutable-values-memoizes-with-captures-values.js new
+38
@@ -0,0 +1,38 @@
1 +// @flow
2 +/**
3 + * This hook returns a function that when called with an input object,
4 + * will return the result of mapping that input with the supplied map
5 + * function. Results are cached, so if the same input is passed again,
6 + * the same output object will be returned.
7 + *
8 + * Note that this technically violates the rules of React and is unsafe:
9 + * hooks must return immutable objects and be pure, and a function which
10 + * captures and mutates a value when called is inherently not pure.
11 + *
12 + * However, in this case it is technically safe _if_ the mapping function
13 + * is pure *and* the resulting objects are never modified. This is because
14 + * the function only caches: the result of `returnedFunction(someInput)`
15 + * strictly depends on `returnedFunction` and `someInput`, and cannot
16 + * otherwise change over time.
17 + */
18 +hook useMemoMap<TInput: interface {}, TOutput>(
19 + map: TInput => TOutput
20 +): TInput => TOutput {
21 + return useMemo(() => {
22 + // The original issue is that `cache` was not memoized together with the returned
23 + // function. This was because neither appears to ever be mutated — the function
24 + // is known to mutate `cache` but the function isn't called.
25 + //
26 + // The fix is to detect cases like this — functions that are mutable but not called -
27 + // and ensure that their mutable captures are aliased together into the same scope.
28 + const cache = new WeakMap<TInput, TOutput>();
29 + return input => {
30 + let output = cache.get(input);
31 + if (output == null) {
32 + output = map(input);
33 + cache.set(input, output);
34 + }
35 + return output;
36 + };
37 + }, [map]);
38 +}