From ef751ca3a8fe0d6d3e4253ba04798cd1b6c24e45 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Mon, 24 Jun 2024 09:53:57 -0700 Subject: [PATCH 1/3] [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-poisoned] --- .../ReactiveScopes/BuildReactiveFunction.ts | 26 ++++++---- .../for-with-assignment-as-update.expect.md | 49 +++++++++++++++++++ .../compiler/for-with-assignment-as-update.js | 12 +++++ 3 files changed, 77 insertions(+), 10 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/for-with-assignment-as-update.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/for-with-assignment-as-update.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/BuildReactiveFunction.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/BuildReactiveFunction.ts index ebc1d63c6d881..377c3323eac66 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/BuildReactiveFunction.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/BuildReactiveFunction.ts @@ -921,14 +921,17 @@ class Driver { }); } else if (defaultBlock.instructions.length === 1) { const instr = defaultBlock.instructions[0]!; - let place: Place = instr.lvalue!; + let place: Place = instr.lvalue; let value: ReactiveValue = instr.value; - if (instr.value.kind === "StoreLocal") { - place = instr.value.lvalue.place; + if ( + value.kind === "StoreLocal" && + value.lvalue.place.identifier.name === null + ) { + place = value.lvalue.place; value = { kind: "LoadLocal", - place: instr.value.value, - loc: instr.value.value.loc, + place: value.value, + loc: value.value.loc, }; } return { @@ -939,14 +942,17 @@ class Driver { }; } else { const instr = defaultBlock.instructions.at(-1)!; - let place: Place = instr.lvalue!; + let place: Place = instr.lvalue; let value: ReactiveValue = instr.value; - if (instr.value.kind === "StoreLocal") { - place = instr.value.lvalue.place; + if ( + value.kind === "StoreLocal" && + value.lvalue.place.identifier.name === null + ) { + place = value.lvalue.place; value = { kind: "LoadLocal", - place: instr.value.value, - loc: instr.value.value.loc, + place: value.value, + loc: value.value.loc, }; } const sequence: ReactiveSequenceValue = { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/for-with-assignment-as-update.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/for-with-assignment-as-update.expect.md new file mode 100644 index 0000000000000..6d44cb418701d --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/for-with-assignment-as-update.expect.md @@ -0,0 +1,49 @@ + +## Input + +```javascript +function Component(props) { + let x = props.init; + for (let i = 0; i < 100; i = i + 1) { + x += i; + } + return [x]; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ init: 0 }], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +function Component(props) { + const $ = _c(2); + let x = props.init; + for (let i = 0; i < 100; i = i + 1) { + x = x + i; + } + let t0; + if ($[0] !== x) { + t0 = [x]; + $[0] = x; + $[1] = t0; + } else { + t0 = $[1]; + } + return t0; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ init: 0 }], +}; + +``` + +### Eval output +(kind: ok) [4950] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/for-with-assignment-as-update.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/for-with-assignment-as-update.js new file mode 100644 index 0000000000000..99a7e75d110fe --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/for-with-assignment-as-update.js @@ -0,0 +1,12 @@ +function Component(props) { + let x = props.init; + for (let i = 0; i < 100; i = i + 1) { + x += i; + } + return [x]; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ init: 0 }], +}; From 81bb35de9180c6eb0e959f0519749d8a0669644a Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Mon, 24 Jun 2024 10:04:20 -0700 Subject: [PATCH 2/3] Update on "[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-poisoned] --- .../src/ReactiveScopes/BuildReactiveFunction.ts | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/BuildReactiveFunction.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/BuildReactiveFunction.ts index 377c3323eac66..9e94e5f76e84b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/BuildReactiveFunction.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/BuildReactiveFunction.ts @@ -924,6 +924,13 @@ class Driver { let place: Place = instr.lvalue; let value: ReactiveValue = instr.value; if ( + // Value blocks generally end in a StoreLocal to assign the value of the + // expression for this branch. These StoreLocal instructions can be pruned, + // since we represent the value blocks as a compund value in ReactiveFunction + // (no phis). However, it's also possible to have a value block that ends in + // an AssignmentExpression, which we need to keep. So we only prune + // StoreLocal for temporaries — any named/promoted values must be used + // elsewhere and aren't safe to prune. value.kind === "StoreLocal" && value.lvalue.place.identifier.name === null ) { @@ -945,6 +952,13 @@ class Driver { let place: Place = instr.lvalue; let value: ReactiveValue = instr.value; if ( + // Value blocks generally end in a StoreLocal to assign the value of the + // expression for this branch. These StoreLocal instructions can be pruned, + // since we represent the value blocks as a compund value in ReactiveFunction + // (no phis). However, it's also possible to have a value block that ends in + // an AssignmentExpression, which we need to keep. So we only prune + // StoreLocal for temporaries — any named/promoted values must be used + // elsewhere and aren't safe to prune. value.kind === "StoreLocal" && value.lvalue.place.identifier.name === null ) { From 6eb2374de550f09879b6b437eadba6475041d11c Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Mon, 24 Jun 2024 10:11:14 -0700 Subject: [PATCH 3/3] Update on "[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-poisoned] --- .../ReactiveScopes/BuildReactiveFunction.ts | 32 +++++++++++-------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/BuildReactiveFunction.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/BuildReactiveFunction.ts index 9e94e5f76e84b..4a5176c7d6ed9 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/BuildReactiveFunction.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/BuildReactiveFunction.ts @@ -924,13 +924,15 @@ class Driver { let place: Place = instr.lvalue; let value: ReactiveValue = instr.value; if ( - // Value blocks generally end in a StoreLocal to assign the value of the - // expression for this branch. These StoreLocal instructions can be pruned, - // since we represent the value blocks as a compund value in ReactiveFunction - // (no phis). However, it's also possible to have a value block that ends in - // an AssignmentExpression, which we need to keep. So we only prune - // StoreLocal for temporaries — any named/promoted values must be used - // elsewhere and aren't safe to prune. + /* + * Value blocks generally end in a StoreLocal to assign the value of the + * expression for this branch. These StoreLocal instructions can be pruned, + * since we represent the value blocks as a compund value in ReactiveFunction + * (no phis). However, it's also possible to have a value block that ends in + * an AssignmentExpression, which we need to keep. So we only prune + * StoreLocal for temporaries — any named/promoted values must be used + * elsewhere and aren't safe to prune. + */ value.kind === "StoreLocal" && value.lvalue.place.identifier.name === null ) { @@ -952,13 +954,15 @@ class Driver { let place: Place = instr.lvalue; let value: ReactiveValue = instr.value; if ( - // Value blocks generally end in a StoreLocal to assign the value of the - // expression for this branch. These StoreLocal instructions can be pruned, - // since we represent the value blocks as a compund value in ReactiveFunction - // (no phis). However, it's also possible to have a value block that ends in - // an AssignmentExpression, which we need to keep. So we only prune - // StoreLocal for temporaries — any named/promoted values must be used - // elsewhere and aren't safe to prune. + /* + * Value blocks generally end in a StoreLocal to assign the value of the + * expression for this branch. These StoreLocal instructions can be pruned, + * since we represent the value blocks as a compund value in ReactiveFunction + * (no phis). However, it's also possible to have a value block that ends in + * an AssignmentExpression, which we need to keep. So we only prune + * StoreLocal for temporaries — any named/promoted values must be used + * elsewhere and aren't safe to prune. + */ value.kind === "StoreLocal" && value.lvalue.place.identifier.name === null ) {