From 3e157bbc27d5c8a4121cb99c3dd7d1af90becf03 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Wed, 1 Nov 2023 17:13:05 -0700 Subject: [PATCH] Propagate reactivity to other operands accounting for mutable ranges Previously if any operand was reactive, we transferred that reactivity to other operands that had a mutable effect (capture, conditionally mutate, mutate, or store). But a value can be captured without ever being modified again. This PR updates the logic to only transfer reactivity among operands that are actually mutable at the given instruction, based on the mutable range. This is strictly more precise. --- .../src/Inference/InferReactivePlaces.ts | 11 ++-- .../InferReactiveScopeVariables.ts | 2 +- ...hi-type-inference-property-store.expect.md | 18 +++--- ...nreactive-captured-with-reactive.expect.md | 50 ++++++++++++++++ ...ency-nonreactive-captured-with-reactive.js | 10 ++++ ...t-captured-with-reactive-mutated.expect.md | 59 +++++++++++++++++++ ...y-object-captured-with-reactive-mutated.js | 15 +++++ 7 files changed, 150 insertions(+), 15 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactive-dependency-nonreactive-captured-with-reactive.expect.md create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactive-dependency-nonreactive-captured-with-reactive.js create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactive-dependency-object-captured-with-reactive-mutated.expect.md create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactive-dependency-object-captured-with-reactive-mutated.js diff --git a/compiler/packages/babel-plugin-react-forget/src/Inference/InferReactivePlaces.ts b/compiler/packages/babel-plugin-react-forget/src/Inference/InferReactivePlaces.ts index abf0578f71..e350310383 100644 --- a/compiler/packages/babel-plugin-react-forget/src/Inference/InferReactivePlaces.ts +++ b/compiler/packages/babel-plugin-react-forget/src/Inference/InferReactivePlaces.ts @@ -23,6 +23,7 @@ import { eachTerminalOperand, } from "../HIR/visitors"; import { hasBackEdge } from "../Optimization/DeadCodeElimination"; +import { isMutable } from "../ReactiveScopes/InferReactiveScopeVariables"; import { assertExhaustive } from "../Utils/utils"; /** @@ -195,11 +196,13 @@ export function inferReactivePlaces(fn: HIRFunction): void { case Effect.Store: case Effect.ConditionallyMutate: case Effect.Mutate: { - const resolvedId = identifierMapping.get(operand.identifier); - if (resolvedId !== undefined) { - reactiveIdentifiers.markReactiveIdentifier(resolvedId); + if (isMutable(instruction, operand)) { + const resolvedId = identifierMapping.get(operand.identifier); + if (resolvedId !== undefined) { + reactiveIdentifiers.markReactiveIdentifier(resolvedId); + } + reactiveIdentifiers.markReactive(operand); } - reactiveIdentifiers.markReactive(operand); break; } case Effect.Freeze: diff --git a/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/InferReactiveScopeVariables.ts b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/InferReactiveScopeVariables.ts index ec7a031504..4f9c57d6fb 100644 --- a/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/InferReactiveScopeVariables.ts +++ b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/InferReactiveScopeVariables.ts @@ -212,7 +212,7 @@ export function inferReactiveScopeVariables(fn: HIRFunction): void { } // Is the operand mutable at this given instruction -function isMutable({ id }: Instruction, place: Place): boolean { +export function isMutable({ id }: Instruction, place: Place): boolean { const range = place.identifier.mutableRange; return id >= range.start && id < range.end; } diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/phi-type-inference-property-store.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/phi-type-inference-property-store.expect.md index 65dc68f39e..de9cb4fce2 100644 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/phi-type-inference-property-store.expect.md +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/phi-type-inference-property-store.expect.md @@ -32,7 +32,7 @@ export const FIXTURE_ENTRYPOINT = { ```javascript import { unstable_useMemoCache as useMemoCache } from "react"; // @debug function Component(props) { - const $ = useMemoCache(7); + const $ = useMemoCache(5); let t0; if ($[0] === Symbol.for("react.memo_cache_sentinel")) { t0 = {}; @@ -42,7 +42,7 @@ function Component(props) { } const x = t0; let y; - if ($[1] !== props || $[2] !== x) { + if ($[1] !== props) { if (props.cond) { y = {}; } else { @@ -51,19 +51,17 @@ function Component(props) { y.x = x; $[1] = props; - $[2] = x; - $[3] = y; + $[2] = y; } else { - y = $[3]; + y = $[2]; } let t1; - if ($[4] !== x || $[5] !== y) { + if ($[3] !== y) { t1 = [x, y]; - $[4] = x; - $[5] = y; - $[6] = t1; + $[3] = y; + $[4] = t1; } else { - t1 = $[6]; + t1 = $[4]; } return t1; } diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactive-dependency-nonreactive-captured-with-reactive.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactive-dependency-nonreactive-captured-with-reactive.expect.md new file mode 100644 index 0000000000..2db66ce7d6 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactive-dependency-nonreactive-captured-with-reactive.expect.md @@ -0,0 +1,50 @@ + +## Input + +```javascript +function Component(props) { + const x = {}; + const y = props.y; + return [x, y]; // x is captured here along with a reactive value. this shouldn't make `x` reactive! +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ y: 42 }], +}; + +``` + +## Code + +```javascript +import { unstable_useMemoCache as useMemoCache } from "react"; +function Component(props) { + const $ = useMemoCache(3); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 = {}; + $[0] = t0; + } else { + t0 = $[0]; + } + const x = t0; + const y = props.y; + let t1; + if ($[1] !== y) { + t1 = [x, y]; + $[1] = y; + $[2] = t1; + } else { + t1 = $[2]; + } + return t1; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ y: 42 }], +}; + +``` + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactive-dependency-nonreactive-captured-with-reactive.js b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactive-dependency-nonreactive-captured-with-reactive.js new file mode 100644 index 0000000000..877ac4fc55 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactive-dependency-nonreactive-captured-with-reactive.js @@ -0,0 +1,10 @@ +function Component(props) { + const x = {}; + const y = props.y; + return [x, y]; // x is captured here along with a reactive value. this shouldn't make `x` reactive! +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ y: 42 }], +}; diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactive-dependency-object-captured-with-reactive-mutated.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactive-dependency-object-captured-with-reactive-mutated.expect.md new file mode 100644 index 0000000000..c111af2fe8 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactive-dependency-object-captured-with-reactive-mutated.expect.md @@ -0,0 +1,59 @@ + +## Input + +```javascript +const { mutate } = require("shared-runtime"); + +function Component(props) { + const x = {}; + const y = props.y; + const z = [x, y]; + mutate(z); + // x's object identity can change bc it co-mutates with z, which is reactive via props.y + return [x]; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ y: 42 }], +}; + +``` + +## Code + +```javascript +import { unstable_useMemoCache as useMemoCache } from "react"; +const { mutate } = require("shared-runtime"); + +function Component(props) { + const $ = useMemoCache(4); + let x; + if ($[0] !== props.y) { + x = {}; + const y = props.y; + const z = [x, y]; + mutate(z); + $[0] = props.y; + $[1] = x; + } else { + x = $[1]; + } + let t0; + if ($[2] !== x) { + t0 = [x]; + $[2] = x; + $[3] = t0; + } else { + t0 = $[3]; + } + return t0; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ y: 42 }], +}; + +``` + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactive-dependency-object-captured-with-reactive-mutated.js b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactive-dependency-object-captured-with-reactive-mutated.js new file mode 100644 index 0000000000..97835f1f64 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactive-dependency-object-captured-with-reactive-mutated.js @@ -0,0 +1,15 @@ +const { mutate } = require("shared-runtime"); + +function Component(props) { + const x = {}; + const y = props.y; + const z = [x, y]; + mutate(z); + // x's object identity can change bc it co-mutates with z, which is reactive via props.y + return [x]; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ y: 42 }], +};