From fbd68eef7f8fad5dd1fdceb0fb79162c3d972e59 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Mon, 25 Mar 2024 21:36:33 -0700 Subject: [PATCH] Fix for invalid mutable range in phi with backedge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes the repro added in 947832009997bf9149e88e583c46cc39f6a6136c - previously when computing mutable ranges of phis, we didn't check that all operands had been visited. This meant that a backedge could allow a phi's mutable range to start at 0. Then in PropagateScopeDeps, we might see reject dependencies of a scope since they appeared to start after a scope — only because the scope's start was incorrectly too early. The fix here is to initially set phi.id mutable ranges based on only on operands that are already visited. Then, during/after the fixpoint iteration of InferMutableRanges, we start account for all operands since we know they've been visited at least once and have a real range. --- .../src/Inference/InferMutableLifetimes.ts | 37 +++++++------ ...ssing-dependency-if-within-while.expect.md | 34 ++++++++---- .../for-in-statement-continue.expect.md | 52 +++++++++++++------ .../compiler/for-in-statement-continue.js | 12 ++++- .../packages/snap/src/SproutTodoFilter.ts | 1 - 5 files changed, 94 insertions(+), 42 deletions(-) diff --git a/compiler/packages/babel-plugin-react-forget/src/Inference/InferMutableLifetimes.ts b/compiler/packages/babel-plugin-react-forget/src/Inference/InferMutableLifetimes.ts index 2794d02e65..e1c4d3a762 100644 --- a/compiler/packages/babel-plugin-react-forget/src/Inference/InferMutableLifetimes.ts +++ b/compiler/packages/babel-plugin-react-forget/src/Inference/InferMutableLifetimes.ts @@ -5,7 +5,6 @@ * LICENSE file in the root directory of this source tree. */ -import { CompilerError } from "../CompilerError"; import { Effect, HIRFunction, @@ -102,22 +101,30 @@ export function inferMutableLifetimes( ): void { for (const [_, block] of func.body.blocks) { for (const phi of block.phis) { - let start = Number.MAX_SAFE_INTEGER; - let end = phi.id.mutableRange.end as number; for (const [_, operand] of phi.operands) { - start = Math.min(start, operand.mutableRange.start); - end = Math.max(end, operand.mutableRange.end); + if ( + operand.mutableRange.start === 0 && + operand.mutableRange.end === 0 + ) { + // operand's range is uninitialized, skip + continue; + } else if ( + phi.id.mutableRange.start === 0 && + phi.id.mutableRange.end === 0 + ) { + // phi's range is uninitialized, take the range from the operand + phi.id.mutableRange.start = operand.mutableRange.start; + phi.id.mutableRange.end = operand.mutableRange.end; + } else { + // else join the phi and operand's range + phi.id.mutableRange.start = makeInstructionId( + Math.min(phi.id.mutableRange.start, operand.mutableRange.start) + ); + phi.id.mutableRange.end = makeInstructionId( + Math.max(phi.id.mutableRange.end, operand.mutableRange.end) + ); + } } - CompilerError.invariant(start !== Number.MAX_SAFE_INTEGER, { - reason: "Expected phi to have a start range value", - description: null, - loc: null, - suggestions: null, - }); - phi.id.mutableRange = { - start: makeInstructionId(start), - end: makeInstructionId(end), - }; } for (const instr of block.instructions) { diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/bug-repro-missing-dependency-if-within-while.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/bug-repro-missing-dependency-if-within-while.expect.md index cacf7df91a..a1f5de135f 100644 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/bug-repro-missing-dependency-if-within-while.expect.md +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/bug-repro-missing-dependency-if-within-while.expect.md @@ -39,11 +39,11 @@ export const FIXTURE_ENTRYPOINT = { import { unstable_useMemoCache as useMemoCache } from "react"; const someGlobal = true; export default function Component(props) { - const $ = useMemoCache(1); + const $ = useMemoCache(4); const { b } = props; - let t0; - if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - const items = []; + let items; + if ($[0] !== b) { + items = []; let i = 0; while (i < 10) { if (someGlobal) { @@ -51,11 +51,18 @@ export default function Component(props) { i++; } } - - t0 = <>{items}; - $[0] = t0; + $[0] = b; + $[1] = items; } else { - t0 = $[0]; + items = $[1]; + } + let t0; + if ($[2] !== items) { + t0 = <>{items}; + $[2] = items; + $[3] = t0; + } else { + t0 = $[3]; } return t0; } @@ -76,4 +83,13 @@ export const FIXTURE_ENTRYPOINT = { }; ``` - \ No newline at end of file + +### Eval output +(kind: ok)
0
0
0
0
0
0
0
0
0
0
+
0
0
0
0
0
0
0
0
0
0
+
42
42
42
42
42
42
42
42
42
42
+
42
42
42
42
42
42
42
42
42
42
+
0
0
0
0
0
0
0
0
0
0
+
42
42
42
42
42
42
42
42
42
42
+
0
0
0
0
0
0
0
0
0
0
+
42
42
42
42
42
42
42
42
42
42
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/for-in-statement-continue.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/for-in-statement-continue.expect.md index 268555f8b0..213d414350 100644 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/for-in-statement-continue.expect.md +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/for-in-statement-continue.expect.md @@ -16,7 +16,17 @@ function Component(props) { export const FIXTURE_ENTRYPOINT = { fn: Component, - params: [{ value: { a: "a", continue: "skip", b: "b!" } }], + params: [{ value: { a: "a", continue: "skip", b: "hello!" } }], + sequentialRenders: [ + { value: { a: "a", continue: "skip", b: "hello!" } }, + { value: { a: "a", continue: "skip", b: "hello!" } }, + { value: { a: "skip!", continue: true } }, + { value: { a: "a", continue: "skip", b: "hello!" } }, + { value: { a: "skip!", continue: true } }, + { value: { a: "a", continue: "skip", b: "hello!" } }, + { value: { a: "skip!", continue: true } }, + { value: { a: "skip!", continue: true } }, + ], }; ``` @@ -26,18 +36,10 @@ export const FIXTURE_ENTRYPOINT = { ```javascript import { unstable_useMemoCache as useMemoCache } from "react"; function Component(props) { - const $ = useMemoCache(3); + const $ = useMemoCache(2); let x; - if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - let t0; - if ($[1] !== props.value) { - t0 = { ...props.value }; - $[1] = props.value; - $[2] = t0; - } else { - t0 = $[2]; - } - const object = t0; + if ($[0] !== props.value) { + const object = { ...props.value }; for (const y in object) { if (y === "continue") { continue; @@ -45,19 +47,37 @@ function Component(props) { x = object[y]; } - $[0] = x; + $[0] = props.value; + $[1] = x; } else { - x = $[0]; + x = $[1]; } return x; } export const FIXTURE_ENTRYPOINT = { fn: Component, - params: [{ value: { a: "a", continue: "skip", b: "b!" } }], + params: [{ value: { a: "a", continue: "skip", b: "hello!" } }], + sequentialRenders: [ + { value: { a: "a", continue: "skip", b: "hello!" } }, + { value: { a: "a", continue: "skip", b: "hello!" } }, + { value: { a: "skip!", continue: true } }, + { value: { a: "a", continue: "skip", b: "hello!" } }, + { value: { a: "skip!", continue: true } }, + { value: { a: "a", continue: "skip", b: "hello!" } }, + { value: { a: "skip!", continue: true } }, + { value: { a: "skip!", continue: true } }, + ], }; ``` ### Eval output -(kind: ok) "b!" \ No newline at end of file +(kind: ok) "hello!" +"hello!" +"skip!" +"hello!" +"skip!" +"hello!" +"skip!" +"skip!" \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/for-in-statement-continue.js b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/for-in-statement-continue.js index 330fbfb2ba..b74f17c773 100644 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/for-in-statement-continue.js +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/for-in-statement-continue.js @@ -12,5 +12,15 @@ function Component(props) { export const FIXTURE_ENTRYPOINT = { fn: Component, - params: [{ value: { a: "a", continue: "skip", b: "b!" } }], + params: [{ value: { a: "a", continue: "skip", b: "hello!" } }], + sequentialRenders: [ + { value: { a: "a", continue: "skip", b: "hello!" } }, + { value: { a: "a", continue: "skip", b: "hello!" } }, + { value: { a: "skip!", continue: true } }, + { value: { a: "a", continue: "skip", b: "hello!" } }, + { value: { a: "skip!", continue: true } }, + { value: { a: "a", continue: "skip", b: "hello!" } }, + { value: { a: "skip!", continue: true } }, + { value: { a: "skip!", continue: true } }, + ], }; diff --git a/compiler/packages/snap/src/SproutTodoFilter.ts b/compiler/packages/snap/src/SproutTodoFilter.ts index cbc2edd572..fef339c363 100644 --- a/compiler/packages/snap/src/SproutTodoFilter.ts +++ b/compiler/packages/snap/src/SproutTodoFilter.ts @@ -534,7 +534,6 @@ const skipFilter = new Set([ // bugs "bug-reduce-reactive-deps-return-in-scope", "bug-reduce-reactive-deps-break-in-scope", - "bug-repro-missing-dependency-if-within-while", // 'react-forget-runtime' not yet supported "flag-enable-emit-hook-guards",