[compiler] Fix assignment within for update expression
When converting value blocks from HIR to ReactiveFunction, we have to drop StoreLocal assignments that represent the assignment of the phi, since ReactiveFunction supports compound expressions. These StoreLocals are only present to represent the conditional assignment of the value itself - but it's also possible for the expression to have contained an assignment expression. Before, in trying to strip the first category of StoreLocal we also accidentally stripped the second category. Now we check that the assignment is for a temporary, and don't strip otherwise. ghstack-source-id: e7759c963bbc1bbff2d3230534b049199e3262ad Pull Request resolved: https://github.com/facebook/react/pull/30067
Joe Savona committed
Jun 24, 2024 at 10:11 UTC
f5d2feb4f069a36140d5e605f5eebc52badcc214
3 files changed
+95
-10
compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/BuildReactiveFunction.ts
+34
-10
@@ -921,14 +921,26 @@ class Driver {
921
});
922
} else if (defaultBlock.instructions.length === 1) {
923
const instr = defaultBlock.instructions[0]!;
924
- let place: Place = instr.lvalue!;
924
+ let place: Place = instr.lvalue;
925
let value: ReactiveValue = instr.value;
926
- if (instr.value.kind === "StoreLocal") {
927
- place = instr.value.lvalue.place;
926
+ if (
927
+ /*
928
+ * Value blocks generally end in a StoreLocal to assign the value of the
929
+ * expression for this branch. These StoreLocal instructions can be pruned,
930
+ * since we represent the value blocks as a compund value in ReactiveFunction
931
+ * (no phis). However, it's also possible to have a value block that ends in
932
+ * an AssignmentExpression, which we need to keep. So we only prune
933
+ * StoreLocal for temporaries — any named/promoted values must be used
934
+ * elsewhere and aren't safe to prune.
935
+ */
936
+ value.kind === "StoreLocal" &&
937
+ value.lvalue.place.identifier.name === null
938
+ ) {
939
+ place = value.lvalue.place;
940
value = {
941
kind: "LoadLocal",
930
- place: instr.value.value,
931
- loc: instr.value.value.loc,
942
+ place: value.value,
943
+ loc: value.value.loc,
944
};
945
}
946
return {
@@ -939,14 +951,26 @@ class Driver {
951
};
952
} else {
953
const instr = defaultBlock.instructions.at(-1)!;
942
- let place: Place = instr.lvalue!;
954
+ let place: Place = instr.lvalue;
955
let value: ReactiveValue = instr.value;
944
- if (instr.value.kind === "StoreLocal") {
945
- place = instr.value.lvalue.place;
956
+ if (
957
+ /*
958
+ * Value blocks generally end in a StoreLocal to assign the value of the
959
+ * expression for this branch. These StoreLocal instructions can be pruned,
960
+ * since we represent the value blocks as a compund value in ReactiveFunction
961
+ * (no phis). However, it's also possible to have a value block that ends in
962
+ * an AssignmentExpression, which we need to keep. So we only prune
963
+ * StoreLocal for temporaries — any named/promoted values must be used
964
+ * elsewhere and aren't safe to prune.
965
+ */
966
+ value.kind === "StoreLocal" &&
967
+ value.lvalue.place.identifier.name === null
968
+ ) {
969
+ place = value.lvalue.place;
970
value = {
971
kind: "LoadLocal",
948
- place: instr.value.value,
949
- loc: instr.value.value.loc,
972
+ place: value.value,
973
+ loc: value.value.loc,
974
};
975
}
976
const sequence: ReactiveSequenceValue = {
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/for-with-assignment-as-update.expect.md
new
+49
@@ -0,0 +1,49 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+function Component(props) {
6
+ let x = props.init;
7
+ for (let i = 0; i < 100; i = i + 1) {
8
+ x += i;
9
+ }
10
+ return [x];
11
+}
12
+
13
+export const FIXTURE_ENTRYPOINT = {
14
+ fn: Component,
15
+ params: [{ init: 0 }],
16
+};
17
+
18
+```
19
+
20
+## Code
21
+
22
+```javascript
23
+import { c as _c } from "react/compiler-runtime";
24
+function Component(props) {
25
+ const $ = _c(2);
26
+ let x = props.init;
27
+ for (let i = 0; i < 100; i = i + 1) {
28
+ x = x + i;
29
+ }
30
+ let t0;
31
+ if ($[0] !== x) {
32
+ t0 = [x];
33
+ $[0] = x;
34
+ $[1] = t0;
35
+ } else {
36
+ t0 = $[1];
37
+ }
38
+ return t0;
39
+}
40
+
41
+export const FIXTURE_ENTRYPOINT = {
42
+ fn: Component,
43
+ params: [{ init: 0 }],
44
+};
45
+
46
+```
47
+
48
+### Eval output
49
+(kind: ok) [4950]
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/for-with-assignment-as-update.js
new
+12
@@ -0,0 +1,12 @@
1
+function Component(props) {
2
+ let x = props.init;
3
+ for (let i = 0; i < 100; i = i + 1) {
4
+ x += i;
5
+ }
6
+ return [x];
7
+}
8
+
9
+export const FIXTURE_ENTRYPOINT = {
10
+ fn: Component,
11
+ params: [{ init: 0 }],
12
+};