The library for web and native user interfaces.
Validate frozen lambdas are actually frozen
Adds validation to reject freezing mutable lambdas, since lambdas cannot _be_ frozen, they're either frozen or not. Example invalid code: ```javascript function Component(props) { const x = {value: ""}; const onChange = (e) => { // MUTATION!!!!! x.value = e.target.value; setX(x); }; return <input value={x.value} onChange={onChange} />; } ``` Note that there is a separate issue in which we are not detecting lambdas that would definitely modify immutable values. We may need to distinguish ConditionallyCapture (captures if the value is mutable, otherwise readonly) from Capture (definitely mutates) in order to make that case work. But already this validation helps prevent some invalid code.
Joe Savona committed
May 31, 2023 at 14:03 UTC
9c453ad26ffd32249b4f44b04d9b371010200139
16 files changed
+217
-85
compiler/forget/packages/snap/src/compiler-worker.ts
+1
@@ -159,6 +159,7 @@ export async function compile(
159
memoizeJsxElements,
160
validateHooksUsage: true,
161
validateRefAccessDuringRender,
162
+ validateFrozenLambdas: true,
163
},
164
logger: null,
165
gating,
compiler/forget/src/CompilerPipeline.ts
+5
@@ -13,6 +13,7 @@ import {
13
mergeConsecutiveBlocks,
14
ReactiveFunction,
15
validateConsistentIdentifiers,
16
+ validateFrozenLambdas,
17
validateHooksUsage,
18
validateNoRefAccessInRender,
19
validateTerminalSuccessors,
@@ -116,6 +117,10 @@ export function* run(
117
inferReferenceEffects(hir);
118
yield log({ kind: "hir", name: "InferReferenceEffects", value: hir });
119
120
+ if (env.validateFrozenLambdas) {
121
+ validateFrozenLambdas(hir);
122
+ }
123
+
124
// Note: Has to come after infer reference effects because "dead" code may still affect inference
125
deadCodeElimination(hir);
126
yield log({ kind: "hir", name: "DeadCodeElimination", value: hir });
compiler/forget/src/HIR/Environment.ts
+10
@@ -74,6 +74,14 @@ export type EnvironmentConfig = Partial<{
74
*/
75
validateRefAccessDuringRender: boolean;
76
77
+ /**
78
+ * Validate that mutable lambdas are not passed where a frozen value is expected, since mutable
79
+ * lambdas cannot be frozen. The only mutation allowed inside a frozen lambda is of ref values.
80
+ *
81
+ * Defaults to false
82
+ */
83
+ validateFrozenLambdas: boolean;
84
+
85
/**
86
* Enable inlining of `useMemo()` function expressions so that they can be more optimally
87
* compiled.
@@ -127,6 +135,7 @@ export class Environment {
135
#nextBlock: number = 0;
136
validateHooksUsage: boolean;
137
validateRefAccessDuringRender: boolean;
138
+ validateFrozenLambdas: boolean;
139
enableFunctionCallSignatureOptimizations: boolean;
140
enableAssumeHooksFollowRulesOfReact: boolean;
141
enableTreatHooksAsFunctions: boolean;
@@ -164,6 +173,7 @@ export class Environment {
173
this.validateHooksUsage = config?.validateHooksUsage ?? false;
174
this.validateRefAccessDuringRender =
175
config?.validateRefAccessDuringRender ?? false;
176
+ this.validateFrozenLambdas = config?.validateFrozenLambdas ?? false;
177
this.enableFunctionCallSignatureOptimizations =
178
config?.enableFunctionCallSignatureOptimizations ?? false;
179
this.enableAssumeHooksFollowRulesOfReact =
compiler/forget/src/HIR/ValidateFrozenLambdas.ts
new
+102
@@ -0,0 +1,102 @@
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
+ CompilerError,
10
+ CompilerErrorDetail,
11
+ ErrorSeverity,
12
+} from "../CompilerError";
13
+import {
14
+ Effect,
15
+ FunctionExpression,
16
+ HIRFunction,
17
+ IdentifierId,
18
+ isMutableEffect,
19
+ isRefValueType,
20
+ isUseRefType,
21
+} from "./HIR";
22
+import { eachInstructionValueOperand } from "./visitors";
23
+
24
+/**
25
+ * Various APIs in React take ownership of the values passed to them, such that it is invalid
26
+ * to subsequently modify those values. Examples include:
27
+ * - Passing a value as a prop to JSX. Subsequently mutating this value will result in undefined
28
+ * behavior, since the mutation may or may not be observed depending on when the child re-renders.
29
+ * In addition, the value may be used as an input to memoization in children, and mutation could
30
+ * invalidate that memoization.
31
+ * - Passing a value to `useState()`, for the same reason.
32
+ * - Passing a value to a hook, for the same reason.
33
+ *
34
+ * Most "normal" data types (objects, arrays, etc) can be "frozen" when passed to a React API simply
35
+ * by not calling any mutating methods on them. However, mutable lambdas are an exception: if a lambda
36
+ * has side-effects, there is no way to do something to the lambda that would allow calling it without
37
+ * triggering those side effects. The only thing a developer could do is not call the lambda, but developers
38
+ * also have no way of knowing that they can't call the lambda.
39
+ *
40
+ * From a type system perspective, the above APIs that "take ownership" of their values really accept
41
+ * *already frozen* values as input. Thus it is invalid to pass a value that cannot be frozen to these APIs,
42
+ * and it is therefore invalid to pass a mutable lambda.
43
+ *
44
+ * This pass validates the above rule. Note that this validation can by bypassed by storing a mutable lambda
45
+ * inside some value (eg as an array element or object property). In these cases we trust that the developer
46
+ * is not breaking the rules. The goal of this validation is to find cases that are provably wrong and help
47
+ * the developer fix the mistake earlier.
48
+ */
49
+export function validateFrozenLambdas(fn: HIRFunction): void {
50
+ const lambdas = new Map<IdentifierId, FunctionExpression>();
51
+ const temporaries = new Map<IdentifierId, IdentifierId>();
52
+
53
+ const errors = new CompilerError();
54
+ for (const [, block] of fn.body.blocks) {
55
+ for (const instr of block.instructions) {
56
+ if (instr.value.kind === "FunctionExpression") {
57
+ lambdas.set(instr.lvalue.identifier.id, instr.value);
58
+ } else if (instr.value.kind === "LoadLocal") {
59
+ const resolvedId =
60
+ temporaries.get(instr.value.place.identifier.id) ??
61
+ instr.value.place.identifier.id;
62
+ temporaries.set(instr.lvalue.identifier.id, resolvedId);
63
+ } else if (instr.value.kind === "StoreLocal") {
64
+ const resolvedId =
65
+ temporaries.get(instr.value.value.identifier.id) ??
66
+ instr.value.value.identifier.id;
67
+ temporaries.set(instr.value.lvalue.place.identifier.id, resolvedId);
68
+ } else {
69
+ for (const operand of eachInstructionValueOperand(instr.value)) {
70
+ if (operand.effect === Effect.Freeze) {
71
+ const operandId =
72
+ temporaries.get(operand.identifier.id) ?? operand.identifier.id;
73
+ const lambda = lambdas.get(operandId);
74
+ if (
75
+ lambda !== undefined &&
76
+ lambda.dependencies.some(
77
+ (place) =>
78
+ isMutableEffect(place.effect, place.loc) &&
79
+ !isRefValueType(place.identifier) &&
80
+ !isUseRefType(place.identifier)
81
+ )
82
+ ) {
83
+ errors.pushErrorDetail(
84
+ new CompilerErrorDetail({
85
+ codeframe: null,
86
+ description: null,
87
+ loc: typeof operand.loc !== "symbol" ? operand.loc : null,
88
+ reason:
89
+ "Cannot use a mutable function where an immutable value is expected",
90
+ severity: ErrorSeverity.InvalidInput,
91
+ })
92
+ );
93
+ }
94
+ }
95
+ }
96
+ }
97
+ }
98
+ }
99
+ if (errors.hasErrors()) {
100
+ throw errors;
101
+ }
102
+}
compiler/forget/src/HIR/index.ts
+1
@@ -18,6 +18,7 @@ export {
18
export { mergeConsecutiveBlocks } from "./MergeConsecutiveBlocks";
19
export { printFunction, printHIR } from "./PrintHIR";
20
export { validateConsistentIdentifiers } from "./ValidateConsistentIdentifiers";
21
+export { validateFrozenLambdas } from "./ValidateFrozenLambdas";
22
export { validateHooksUsage } from "./ValidateHooksUsage";
23
export { validateNoRefAccessInRender } from "./ValidateNoRefAccesInRender";
24
export { validateTerminalSuccessors } from "./ValidateTerminalSuccessors";
compiler/forget/src/__tests__/fixtures/compiler/capture-func-passed-to-jsx.expect.md
deleted
-75
@@ -1,75 +0,0 @@
1
-
2
-## Input
3
-
4
-```javascript
5
-function component(a, b) {
6
- let y = { b };
7
- let z = { a };
8
- let x = function () {
9
- z.a = 2;
10
- y.b;
11
- };
12
- let t = <Foo x={x}></Foo>;
13
- mutate(x); // x should be frozen here
14
- return t;
15
-}
16
-
17
-```
18
-
19
-## Code
20
-
21
-```javascript
22
-import { unstable_useMemoCache as useMemoCache } from "react";
23
-function component(a, b) {
24
- const $ = useMemoCache(9);
25
- const c_0 = $[0] !== b;
26
- let t0;
27
- if (c_0) {
28
- t0 = { b };
29
- $[0] = b;
30
- $[1] = t0;
31
- } else {
32
- t0 = $[1];
33
- }
34
- const y = t0;
35
- const c_2 = $[2] !== a;
36
- let t1;
37
- if (c_2) {
38
- t1 = { a };
39
- $[2] = a;
40
- $[3] = t1;
41
- } else {
42
- t1 = $[3];
43
- }
44
- const z = t1;
45
- const c_4 = $[4] !== z.a;
46
- const c_5 = $[5] !== y.b;
47
- let t2;
48
- if (c_4 || c_5) {
49
- t2 = function () {
50
- z.a = 2;
51
- y.b;
52
- };
53
- $[4] = z.a;
54
- $[5] = y.b;
55
- $[6] = t2;
56
- } else {
57
- t2 = $[6];
58
- }
59
- const x = t2;
60
- const c_7 = $[7] !== x;
61
- let t3;
62
- if (c_7) {
63
- t3 = <Foo x={x} />;
64
- $[7] = x;
65
- $[8] = t3;
66
- } else {
67
- t3 = $[8];
68
- }
69
- const t = t3;
70
- mutate(x);
71
- return t;
72
-}
73
-
74
-```
75
-
\ No newline at end of file
compiler/forget/src/__tests__/fixtures/compiler/destructuring-mixed-scope-and-local-variables-with-default.expect.md
+4
-4
@@ -17,7 +17,7 @@ function Component(props) {
17
if (!comments.length) {
18
return;
19
}
20
- log(comments.length);
20
+ console.log(comments.length);
21
};
22
allUrls.push(...urls);
23
return <Media media={media} onClick={onClick} />;
@@ -38,7 +38,7 @@ function Component(props) {
38
if (c_0) {
39
const allUrls = [];
40
41
- const { media: t0, comments: t2, urls: t81 } = post;
41
+ const { media: t0, comments: t2, urls: t82 } = post;
42
const c_3 = $[3] !== t0;
43
let t1;
44
if (c_3) {
@@ -59,7 +59,7 @@ function Component(props) {
59
t3 = $[6];
60
}
61
const comments = t3;
62
- const urls = t81 === undefined ? [] : t81;
62
+ const urls = t82 === undefined ? [] : t82;
63
const c_7 = $[7] !== comments.length;
64
let t4;
65
if (c_7) {
@@ -67,7 +67,7 @@ function Component(props) {
67
if (!comments.length) {
68
return;
69
}
70
- log(comments.length);
70
+ console.log(comments.length);
71
};
72
$[7] = comments.length;
73
$[8] = t4;
compiler/forget/src/__tests__/fixtures/compiler/destructuring-mixed-scope-and-local-variables-with-default.js
+1
-1
@@ -13,7 +13,7 @@ function Component(props) {
13
if (!comments.length) {
14
return;
15
}
16
- log(comments.length);
16
+ console.log(comments.length);
17
};
18
allUrls.push(...urls);
19
return <Media media={media} onClick={onClick} />;
compiler/forget/src/__tests__/fixtures/compiler/destructuring-mixed-scope-declarations-and-locals.expect.md
+4
-4
@@ -17,7 +17,7 @@ function Component(props) {
17
if (!comments.length) {
18
return;
19
}
20
- log(comments.length);
20
+ console.log(comments.length);
21
};
22
allUrls.push(...urls);
23
return <Media media={media} onClick={onClick} />;
@@ -38,8 +38,8 @@ function Component(props) {
38
if (c_0) {
39
const allUrls = [];
40
41
- const { media: t83, comments, urls } = post;
42
- media = t83;
41
+ const { media: t85, comments, urls } = post;
42
+ media = t85;
43
const c_3 = $[3] !== comments.length;
44
let t0;
45
if (c_3) {
@@ -47,7 +47,7 @@ function Component(props) {
47
if (!comments.length) {
48
return;
49
}
50
- log(comments.length);
50
+ console.log(comments.length);
51
};
52
$[3] = comments.length;
53
$[4] = t0;
compiler/forget/src/__tests__/fixtures/compiler/destructuring-mixed-scope-declarations-and-locals.js
+1
-1
@@ -13,7 +13,7 @@ function Component(props) {
13
if (!comments.length) {
14
return;
15
}
16
- log(comments.length);
16
+ console.log(comments.length);
17
};
18
allUrls.push(...urls);
19
return <Media media={media} onClick={onClick} />;
compiler/forget/src/__tests__/fixtures/compiler/error.invalid-capture-func-passed-to-jsx.expect.md
new
+26
@@ -0,0 +1,26 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+function component(a, b) {
6
+ let y = { b };
7
+ let z = { a };
8
+ let x = function () {
9
+ z.a = 2;
10
+ y.b;
11
+ };
12
+ let t = <Foo x={x}></Foo>;
13
+ mutate(x); // x should be frozen here
14
+ return t;
15
+}
16
+
17
+```
18
+
19
+
20
+## Error
21
+
22
+```
23
+[ReactForget] InvalidInput: Cannot use a mutable function where an immutable value is expected (8:8)
24
+```
25
+
26
+
\ No newline at end of file
compiler/forget/src/__tests__/fixtures/compiler/error.invalid-capture-func-passed-to-jsx.js
renamed
compiler/forget/src/__tests__/fixtures/compiler/error.invalid-freeze-mutable-lambda-mutate-local.expect.md
new
+24
@@ -0,0 +1,24 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+function Component(props) {
6
+ const x = {};
7
+ const onChange = (e) => {
8
+ // INVALID! should use copy-on-write and pass the new value
9
+ x.value = e.target.value;
10
+ setX(x);
11
+ };
12
+ return <input value={x.value} onChange={onChange} />;
13
+}
14
+
15
+```
16
+
17
+
18
+## Error
19
+
20
+```
21
+[ReactForget] InvalidInput: Cannot use a mutable function where an immutable value is expected (8:8)
22
+```
23
+
24
+
\ No newline at end of file
compiler/forget/src/__tests__/fixtures/compiler/error.invalid-freeze-mutable-lambda-mutate-local.js
new
+9
@@ -0,0 +1,9 @@
1
+function Component(props) {
2
+ const x = {};
3
+ const onChange = (e) => {
4
+ // INVALID! should use copy-on-write and pass the new value
5
+ x.value = e.target.value;
6
+ setX(x);
7
+ };
8
+ return <input value={x.value} onChange={onChange} />;
9
+}
compiler/forget/src/__tests__/fixtures/compiler/error.invalid-freeze-mutable-lambda-reassign-local.expect.md
new
+22
@@ -0,0 +1,22 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+function Component(props) {
6
+ let x = "";
7
+ const onChange = (e) => {
8
+ x = e.target.value;
9
+ };
10
+ return <input value={x} onChange={onChange} />;
11
+}
12
+
13
+```
14
+
15
+
16
+## Error
17
+
18
+```
19
+[ReactForget] InvalidInput: Cannot use a mutable function where an immutable value is expected (6:6)
20
+```
21
+
22
+
\ No newline at end of file
compiler/forget/src/__tests__/fixtures/compiler/error.invalid-freeze-mutable-lambda-reassign-local.js
new
+7
@@ -0,0 +1,7 @@
1
+function Component(props) {
2
+ let x = "";
3
+ const onChange = (e) => {
4
+ x = e.target.value;
5
+ };
6
+ return <input value={x} onChange={onChange} />;
7
+}