Validate that all variable references are consistently local/context
Validates that all references to a variable (pre-SSA) are consistently "local" references or "context" references. Ie, if a variable is declared as DeclareContext, any accesses must be eg LoadContext or StoreContext, not LoadLocal/StoreLocal. This will help with the issue from #2577 (assuming that we know a variable _is_ a context variable) but also provides a more precise bailout for an existing case with destructuring assignment to a context variable.
Joe Savona committed
Feb 9, 2024 at 14:09 UTC
6da1912eed161c766689eb4bde336b9854e912e7
7 files changed
+135
-52
compiler/packages/babel-plugin-react-forget/src/Entrypoint/Pipeline.ts
+2
@@ -69,6 +69,7 @@ import {
69
} from "../Utils/logger";
70
import { assertExhaustive } from "../Utils/utils";
71
import {
72
+ validateContextVariableLValues,
73
validateFrozenLambdas,
74
validateHooksUsage,
75
validateMemoizedEffectDependencies,
@@ -117,6 +118,7 @@ function* runWithEnvironment(
118
pruneMaybeThrows(hir);
119
yield log({ kind: "hir", name: "PruneMaybeThrows", value: hir });
120
121
+ validateContextVariableLValues(hir);
122
validateUseMemo(hir);
123
124
dropManualMemoization(hir);
compiler/packages/babel-plugin-react-forget/src/Validation/ValidateContextVariableLValues.ts
new
+106
@@ -0,0 +1,106 @@
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 { CompilerError } from "..";
9
+import { HIRFunction, IdentifierId, Place } from "../HIR";
10
+import { printPlace } from "../HIR/PrintHIR";
11
+import {
12
+ eachInstructionValueLValue,
13
+ eachPatternOperand,
14
+} from "../HIR/visitors";
15
+
16
+/**
17
+ * Validates that all store/load references to a given named identifier align with the
18
+ * "kind" of that variable (normal variable or context variable). For example, a context
19
+ * variable may not be loaded/stored with regular StoreLocal/LoadLocal/Destructure instructions.
20
+ */
21
+export function validateContextVariableLValues(fn: HIRFunction): void {
22
+ const identifierKinds: IdentifierKinds = new Map();
23
+ validateContextVariableLValuesImpl(fn, identifierKinds);
24
+}
25
+
26
+function validateContextVariableLValuesImpl(
27
+ fn: HIRFunction,
28
+ identifierKinds: IdentifierKinds
29
+): void {
30
+ for (const [, block] of fn.body.blocks) {
31
+ for (const instr of block.instructions) {
32
+ const { value } = instr;
33
+ switch (value.kind) {
34
+ case "DeclareContext":
35
+ case "StoreContext": {
36
+ visit(identifierKinds, value.lvalue.place, "context");
37
+ break;
38
+ }
39
+ case "LoadContext": {
40
+ visit(identifierKinds, value.place, "context");
41
+ break;
42
+ }
43
+ case "StoreLocal":
44
+ case "DeclareLocal": {
45
+ visit(identifierKinds, value.lvalue.place, "local");
46
+ break;
47
+ }
48
+ case "LoadLocal": {
49
+ visit(identifierKinds, value.place, "local");
50
+ break;
51
+ }
52
+ case "PostfixUpdate":
53
+ case "PrefixUpdate": {
54
+ visit(identifierKinds, value.lvalue, "local");
55
+ break;
56
+ }
57
+ case "Destructure": {
58
+ for (const lvalue of eachPatternOperand(value.lvalue.pattern)) {
59
+ visit(identifierKinds, lvalue, "local");
60
+ }
61
+ break;
62
+ }
63
+ case "ObjectMethod":
64
+ case "FunctionExpression": {
65
+ validateContextVariableLValuesImpl(
66
+ value.loweredFunc.func,
67
+ identifierKinds
68
+ );
69
+ break;
70
+ }
71
+ default: {
72
+ for (const _ of eachInstructionValueLValue(value)) {
73
+ CompilerError.throwTodo({
74
+ reason:
75
+ "ValidateContextVariableLValues: unhandled instruction variant",
76
+ loc: value.loc,
77
+ description: `Handle '${value.kind} lvalues`,
78
+ suggestions: null,
79
+ });
80
+ }
81
+ }
82
+ }
83
+ }
84
+ }
85
+}
86
+
87
+type IdentifierKinds = Map<IdentifierId, "local" | "context">;
88
+
89
+function visit(
90
+ identifiers: IdentifierKinds,
91
+ place: Place,
92
+ kind: "local" | "context"
93
+): void {
94
+ const prevKind = identifiers.get(place.identifier.id);
95
+ if (prevKind !== undefined && prevKind !== kind) {
96
+ CompilerError.invariant(false, {
97
+ reason: `Expected all references to a variable to be consistently local or context references`,
98
+ loc: place.loc,
99
+ description: `Identifier ${printPlace(
100
+ place
101
+ )} is referenced as a ${kind} variable, but was previously referenced as a ${prevKind} variable`,
102
+ suggestions: null,
103
+ });
104
+ }
105
+ identifiers.set(place.identifier.id, kind);
106
+}
compiler/packages/babel-plugin-react-forget/src/Validation/index.ts
+1
@@ -5,6 +5,7 @@
5
* LICENSE file in the root directory of this source tree.
6
*/
7
8
+export { validateContextVariableLValues } from "./ValidateContextVariableLValues";
9
export { validateFrozenLambdas } from "./ValidateFrozenLambdas";
10
export { validateHooksUsage } from "./ValidateHooksUsage";
11
export { validateMemoizedEffectDependencies } from "./ValidateMemoizedEffectDependencies";
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-repro-scope-missing-mutable-range.expect.md
new
+25
@@ -0,0 +1,25 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+function HomeDiscoStoreItemTileRating(props) {
6
+ const item = useFragment();
7
+ let count = 0;
8
+ const aggregates = item?.aggregates || [];
9
+ aggregates.forEach((aggregate) => {
10
+ count += aggregate.count || 0;
11
+ });
12
+
13
+ return <Text>{count}</Text>;
14
+}
15
+
16
+```
17
+
18
+
19
+## Error
20
+
21
+```
22
+[ReactForget] Invariant: Expected all references to a variable to be consistently local or context references. Identifier <unknown> count$6 is referenced as a local variable, but was previously referenced as a context variable (6:6)
23
+```
24
+
25
+
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-repro-scope-missing-mutable-range.js
renamed
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo.destructure-assignment-to-context-var.expect.md
+1
-1
@@ -18,7 +18,7 @@ function useFoo(props) {
18
## Error
19
20
```
21
-[ReactForget] Invariant: [InferReferenceEffects] Context variables are always mutable. (5:5)
21
+[ReactForget] Invariant: Expected all references to a variable to be consistently local or context references. Identifier <unknown> x$1 is referenced as a local variable, but was previously referenced as a context variable (3:3)
22
```
23
24
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/repro-scope-missing-mutable-range.expect.md
deleted
-51
@@ -1,51 +0,0 @@
1
-
2
-## Input
3
-
4
-```javascript
5
-function HomeDiscoStoreItemTileRating(props) {
6
- const item = useFragment();
7
- let count = 0;
8
- const aggregates = item?.aggregates || [];
9
- aggregates.forEach((aggregate) => {
10
- count += aggregate.count || 0;
11
- });
12
-
13
- return <Text>{count}</Text>;
14
-}
15
-
16
-```
17
-
18
-## Code
19
-
20
-```javascript
21
-import { unstable_useMemoCache as useMemoCache } from "react";
22
-function HomeDiscoStoreItemTileRating(props) {
23
- const $ = useMemoCache(4);
24
- const item = useFragment();
25
- let count;
26
- if ($[0] !== item) {
27
- count = 0;
28
- const aggregates = item?.aggregates || [];
29
- aggregates.forEach((aggregate) => {
30
- count = count + (aggregate.count || 0);
31
- });
32
- $[0] = item;
33
- $[1] = count;
34
- } else {
35
- count = $[1];
36
- }
37
-
38
- const t0 = count;
39
- let t1;
40
- if ($[2] !== t0) {
41
- t1 = <Text>{t0}</Text>;
42
- $[2] = t0;
43
- $[3] = t1;
44
- } else {
45
- t1 = $[3];
46
- }
47
- return t1;
48
-}
49
-
50
-```
51
-
\ No newline at end of file