@samitouri / QOS-React-1 / commits / 72c27b6893

Feature flag for transitively freezing values

This PR adds a feature flag to model a potential new-in-practice rule in React: that freezing a function expression also freezes its closed-over values, transitively. For example, in the following code `data` is frozen when the lambda that captures it is is passed to useEffect: ```javascript const data = []; // useEffect freezes its argument (the function expr), which transitively freezes its captured value data useEffect(() => { foo(data); }, [data]); data.push(true); // ERROR: mutating a frozen value mutate(data); // we conservatively assume this doesn't mutate but could be wrong ``` Note that this rule has never been written down or enforced. It is theoretically equivalent to the rule (already implemented in Forget) that values captured by JSX are frozen: ```javascript const style = {...}; <div style={style}>...</div> style.width = 10; // ERROR: mutating a frozen value mutate(style); // we conservatively assume this doesn't mutate but could be wrong ``` However, JSX is typically constructed toward the very end of a render function. Thus in practice there isn't much subsequent code that could even modify such a captured value. But for the useEffect case (and other hooks that take closures as arguments), they tend to occur much earlier in a render function. There's more code that can run later and still modify the captured values, without causing issues in practice. The _practical_ rule today is that you can't modify values captured by frozen lambdas _after the component returns_: it's fine in practice to modify captured values between calling eg useEffect and returning from render. Thus this feature flag is fairly likely to break some percent of real product code. I'm adding this so that we can experiment and see how unsafe it actually is.

Joe Savona committed Nov 6, 2023 at 08:33 UTC 72c27b6893188be833839923b33d739a93b3ed6e
8 files changed +290 -12
compiler/packages/babel-plugin-react-forget/src/HIR/Environment.ts
+36
@@ -175,6 +175,41 @@ export type EnvironmentConfig = {
175 */
176 enableEmitFreeze: ExternalFunction | null;
177
178 + /**
179 + * Forget infers certain operations as "freezing" a value, such that those
180 + * values should not be subsequently mutated. By default this freeze operation
181 + * applies to the value itself and its direct aliases, but not values captured
182 + * by the value being frozen.
183 + *
184 + * In the following, passing `x` to JSX freezes it, which includes freezing `y`
185 + * and `z` which x may alias:
186 + *
187 + * ```
188 + * let x;
189 + * if (cond) {
190 + * x = y
191 + * } else {
192 + * x = z;
193 + * }
194 + * <div>{x}</div>
195 + * ```
196 + *
197 + * However, in the following example we currently only consider x itself to be
198 + * frozen, not `y` or `z`:
199 + *
200 + * ```
201 + * let y = ...;
202 + * let z = ...;
203 + * let x = () => { return [y, z]; };
204 + * <div>{x}</div>
205 + * ```
206 + *
207 + * With this flag enabled, function expression dependencies (values closed over)
208 + * are transitively frozen when the function itself is frozen. So in this case,
209 + * `y` and `z` would be frozen when `x` is frozen.
210 + */
211 + enableTransitivelyFreezeFunctionExpressions: boolean;
212 +
213 /**
214 * Enable merging consecutive scopes that invalidate together.
215 *
@@ -246,6 +281,7 @@ export const DEFAULT_ENVIRONMENT_CONFIG: Readonly<EnvironmentConfig> = {
281 enableEmitFreeze: null,
282 enableForest: false,
283 enableChangeVariableCodegen: false,
284 + enableTransitivelyFreezeFunctionExpressions: false,
285
286 validateFrozenLambdas: false,
287 validateNoSetStateInRender: false,
compiler/packages/babel-plugin-react-forget/src/Inference/InferReferenceEffects.ts
+24 -5
@@ -94,7 +94,7 @@ export default function inferReferenceEffects(
94 ): void {
95 // Initial state contains function params
96 // TODO: include module declarations here as well
97 - const initialState = InferenceState.empty();
97 + const initialState = InferenceState.empty(fn.env);
98 const value: InstructionValue = {
99 kind: "Primitive",
100 loc: fn.loc,
@@ -186,6 +186,8 @@ export default function inferReferenceEffects(
186 * Maintains a mapping of top-level variables to the kind of value they hold
187 */
188 class InferenceState {
189 + #env: Environment;
190 +
191 // The kind of reach value, based on its allocation site
192 #values: Map<InstructionValue, ValueKind>;
193 // The set of values pointed to by each identifier. This is a set
@@ -194,15 +196,17 @@ class InferenceState {
196 #variables: Map<IdentifierId, Set<InstructionValue>>;
197
198 constructor(
199 + env: Environment,
200 values: Map<InstructionValue, ValueKind>,
201 variables: Map<IdentifierId, Set<InstructionValue>>
202 ) {
203 + this.#env = env;
204 this.#values = values;
205 this.#variables = variables;
206 }
207
204 - static empty(): InferenceState {
205 - return new InferenceState(new Map(), new Map());
208 + static empty(env: Environment): InferenceState {
209 + return new InferenceState(env, new Map(), new Map());
210 }
211
212 /**
@@ -317,7 +321,17 @@ class InferenceState {
321 ) {
322 effect = Effect.Freeze;
323 valueKind = ValueKind.Frozen;
320 - values.forEach((value) => this.#values.set(value, ValueKind.Frozen));
324 + values.forEach((value) => {
325 + this.#values.set(value, ValueKind.Frozen);
326 +
327 + if (this.#env.config.enableTransitivelyFreezeFunctionExpressions) {
328 + if (value.kind === "FunctionExpression") {
329 + for (const operand of eachInstructionValueOperand(value)) {
330 + this.reference(operand, Effect.Freeze);
331 + }
332 + }
333 + }
334 + });
335 } else {
336 effect = Effect.Read;
337 }
@@ -482,6 +496,7 @@ class InferenceState {
496 return null;
497 } else {
498 return new InferenceState(
499 + this.#env,
500 nextValues ?? new Map(this.#values),
501 nextVariables ?? new Map(this.#variables)
502 );
@@ -494,7 +509,11 @@ class InferenceState {
509 * clone cheaper.
510 */
511 clone(): InferenceState {
497 - return new InferenceState(new Map(this.#values), new Map(this.#variables));
512 + return new InferenceState(
513 + this.#env,
514 + new Map(this.#values),
515 + new Map(this.#variables)
516 + );
517 }
518
519 /**
compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/AssertScopeInstructionsWithinScope.ts
+27 -7
@@ -43,18 +43,39 @@ import { ReactiveFunctionVisitor } from "./visitors";
43 export function assertScopeInstructionsWithinScopes(
44 fn: ReactiveFunction
45 ): void {
46 - visitReactiveFunction(fn, new Visitor(), undefined);
46 + const existingScopes = new Set<ScopeId>();
47 + visitReactiveFunction(fn, new FindAllScopesVisitor(), existingScopes);
48 + visitReactiveFunction(
49 + fn,
50 + new CheckInstructionsAgainstScopesVisitor(),
51 + existingScopes
52 + );
53 }
54
49 -class Visitor extends ReactiveFunctionVisitor<void> {
50 - seenScopes: Set<ScopeId> = new Set();
55 +class FindAllScopesVisitor extends ReactiveFunctionVisitor<Set<ScopeId>> {
56 + override visitScope(block: ReactiveScopeBlock, state: Set<ScopeId>): void {
57 + this.traverseScope(block, state);
58 + state.add(block.scope.id);
59 + }
60 +}
61 +
62 +class CheckInstructionsAgainstScopesVisitor extends ReactiveFunctionVisitor<
63 + Set<ScopeId>
64 +> {
65 activeScopes: Set<ScopeId> = new Set();
66
53 - override visitPlace(id: InstructionId, place: Place, _state: void): void {
67 + override visitPlace(
68 + id: InstructionId,
69 + place: Place,
70 + state: Set<ScopeId>
71 + ): void {
72 const scope = getPlaceScope(id, place);
73 if (
74 scope !== null &&
57 - this.seenScopes.has(scope.id) &&
75 + // is there a scope for this at all, or did we end up pruning this scope?
76 + state.has(scope.id) &&
77 + // if the scope exists somewhere, it must be active or else this is a straggler
78 + // instruction
79 !this.activeScopes.has(scope.id)
80 ) {
81 CompilerError.invariant(false, {
@@ -67,8 +88,7 @@ class Visitor extends ReactiveFunctionVisitor<void> {
88 }
89 }
90
70 - override visitScope(block: ReactiveScopeBlock, state: void): void {
71 - this.seenScopes.add(block.scope.id);
91 + override visitScope(block: ReactiveScopeBlock, state: Set<ScopeId>): void {
92 this.activeScopes.add(block.scope.id);
93 this.traverseScope(block, state);
94 this.activeScopes.delete(block.scope.id);
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/transitive-freeze-array.expect.md new
+67
@@ -0,0 +1,67 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +// @enableTransitivelyFreezeFunctionExpressions
6 +const { mutate } = require("shared-runtime");
7 +
8 +function Component(props) {
9 + const x = {};
10 + const y = {};
11 + const items = [x, y];
12 + items.pop();
13 + <div>{items}</div>; // note: enableTransitivelyFreezeFunctionExpressions only visits function expressions, not arrays, so this doesn't freeze x/y
14 + mutate(y); // ok! not part of `items` anymore bc of items.pop()
15 + return [x, y, items];
16 +}
17 +
18 +export const FIXTURE_ENTRYPOINT = {
19 + fn: Component,
20 + params: [{}],
21 +};
22 +
23 +```
24 +
25 +## Code
26 +
27 +```javascript
28 +import { unstable_useMemoCache as useMemoCache } from "react"; // @enableTransitivelyFreezeFunctionExpressions
29 +const { mutate } = require("shared-runtime");
30 +
31 +function Component(props) {
32 + const $ = useMemoCache(4);
33 + let x;
34 + let y;
35 + let items;
36 + if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
37 + x = {};
38 + y = {};
39 + items = [x, y];
40 + items.pop();
41 +
42 + mutate(y);
43 + $[0] = x;
44 + $[1] = y;
45 + $[2] = items;
46 + } else {
47 + x = $[0];
48 + y = $[1];
49 + items = $[2];
50 + }
51 + let t0;
52 + if ($[3] === Symbol.for("react.memo_cache_sentinel")) {
53 + t0 = [x, y, items];
54 + $[3] = t0;
55 + } else {
56 + t0 = $[3];
57 + }
58 + return t0;
59 +}
60 +
61 +export const FIXTURE_ENTRYPOINT = {
62 + fn: Component,
63 + params: [{}],
64 +};
65 +
66 +```
67 +
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/transitive-freeze-array.js new
+17
@@ -0,0 +1,17 @@
1 +// @enableTransitivelyFreezeFunctionExpressions
2 +const { mutate } = require("shared-runtime");
3 +
4 +function Component(props) {
5 + const x = {};
6 + const y = {};
7 + const items = [x, y];
8 + items.pop();
9 + <div>{items}</div>; // note: enableTransitivelyFreezeFunctionExpressions only visits function expressions, not arrays, so this doesn't freeze x/y
10 + mutate(y); // ok! not part of `items` anymore bc of items.pop()
11 + return [x, y, items];
12 +}
13 +
14 +export const FIXTURE_ENTRYPOINT = {
15 + fn: Component,
16 + params: [{}],
17 +};
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/transitive-freeze-function-expressions.expect.md new
+95
@@ -0,0 +1,95 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +// @enableTransitivelyFreezeFunctionExpressions
6 +function Component(props) {
7 + const { data, loadNext, isLoadingNext } =
8 + usePaginationFragment(props.key).items ?? [];
9 +
10 + const loadMoreWithTiming = () => {
11 + if (data.length === 0) {
12 + return;
13 + }
14 + loadNext();
15 + };
16 +
17 + useEffect(() => {
18 + if (isLoadingNext) {
19 + return;
20 + }
21 + loadMoreWithTiming();
22 + }, [isLoadingNext, loadMoreWithTiming]);
23 +
24 + const items = data.map((x) => x);
25 +
26 + return items;
27 +}
28 +
29 +```
30 +
31 +## Code
32 +
33 +```javascript
34 +import { unstable_useMemoCache as useMemoCache } from "react"; // @enableTransitivelyFreezeFunctionExpressions
35 +function Component(props) {
36 + const $ = useMemoCache(10);
37 + const { data, loadNext, isLoadingNext } =
38 + usePaginationFragment(props.key).items ?? [];
39 + let t0;
40 + if ($[0] !== data.length || $[1] !== loadNext) {
41 + t0 = () => {
42 + if (data.length === 0) {
43 + return;
44 + }
45 +
46 + loadNext();
47 + };
48 + $[0] = data.length;
49 + $[1] = loadNext;
50 + $[2] = t0;
51 + } else {
52 + t0 = $[2];
53 + }
54 + const loadMoreWithTiming = t0;
55 + let t1;
56 + let t2;
57 + if ($[3] !== isLoadingNext || $[4] !== loadMoreWithTiming) {
58 + t1 = () => {
59 + if (isLoadingNext) {
60 + return;
61 + }
62 +
63 + loadMoreWithTiming();
64 + };
65 + t2 = [isLoadingNext, loadMoreWithTiming];
66 + $[3] = isLoadingNext;
67 + $[4] = loadMoreWithTiming;
68 + $[5] = t1;
69 + $[6] = t2;
70 + } else {
71 + t1 = $[5];
72 + t2 = $[6];
73 + }
74 + useEffect(t1, t2);
75 + let t4;
76 + if ($[7] !== data) {
77 + let t3;
78 + if ($[9] === Symbol.for("react.memo_cache_sentinel")) {
79 + t3 = (x) => x;
80 + $[9] = t3;
81 + } else {
82 + t3 = $[9];
83 + }
84 + t4 = data.map(t3);
85 + $[7] = data;
86 + $[8] = t4;
87 + } else {
88 + t4 = $[8];
89 + }
90 + const items = t4;
91 + return items;
92 +}
93 +
94 +```
95 +
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/transitive-freeze-function-expressions.js new
+23
@@ -0,0 +1,23 @@
1 +// @enableTransitivelyFreezeFunctionExpressions
2 +function Component(props) {
3 + const { data, loadNext, isLoadingNext } =
4 + usePaginationFragment(props.key).items ?? [];
5 +
6 + const loadMoreWithTiming = () => {
7 + if (data.length === 0) {
8 + return;
9 + }
10 + loadNext();
11 + };
12 +
13 + useEffect(() => {
14 + if (isLoadingNext) {
15 + return;
16 + }
17 + loadMoreWithTiming();
18 + }, [isLoadingNext, loadMoreWithTiming]);
19 +
20 + const items = data.map((x) => x);
21 +
22 + return items;
23 +}
compiler/packages/sprout/src/SproutTodoFilter.ts
+1
@@ -475,6 +475,7 @@ const skipFilter = new Set([
475
476 // Tested e2e in forget-feedback repo
477 "userspace-use-memo-cache",
478 + "transitive-freeze-function-expressions",
479 ]);
480
481 export default skipFilter;