From 8be56418d31ba6ff0113c3ccb91ce3f3d11ee4fd Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Fri, 19 Jan 2024 16:03:58 -0800 Subject: [PATCH] InferReactivePlaces accounts for mutable aliasing Fixes T175227223. When inferring reactivity, mutation of a value with a reactive input marks the mutable value as reactive. However, we also need to account for aliases: ```javascript const x = []; const y = x; y.push(props.value); ``` Previously we would have only considered `y` reactive here, but `x` also becomes reactive. The implementation extracts out a helper from InferReactiveScopeVariables that builds a `DisjointSet` of disjoint sets of mutably aliased values. InferReactivePlaces then treats all instances of each mutable alias group as equivalent for reactivity purposes. --- .../src/Inference/InferReactivePlaces.ts | 34 ++- .../InferReactiveScopeVariables.ts | 194 +++++++++--------- ...ivity-via-aliased-mutation-array.expect.md | 84 ++++++++ .../reactivity-via-aliased-mutation-array.js | 22 ++ ...vity-via-aliased-mutation-lambda.expect.md | 91 ++++++++ .../reactivity-via-aliased-mutation-lambda.js | 25 +++ ...ay-with-capturing-map-after-hook.expect.md | 16 +- ...epro-scope-missing-mutable-range.expect.md | 15 +- 8 files changed, 369 insertions(+), 112 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactivity-via-aliased-mutation-array.expect.md create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactivity-via-aliased-mutation-array.js create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactivity-via-aliased-mutation-lambda.expect.md create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactivity-via-aliased-mutation-lambda.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 161647ddfb..41454fca5d 100644 --- a/compiler/packages/babel-plugin-react-forget/src/Inference/InferReactivePlaces.ts +++ b/compiler/packages/babel-plugin-react-forget/src/Inference/InferReactivePlaces.ts @@ -24,7 +24,11 @@ import { eachTerminalOperand, } from "../HIR/visitors"; import { hasBackEdge } from "../Optimization/DeadCodeElimination"; -import { isMutable } from "../ReactiveScopes/InferReactiveScopeVariables"; +import { + findDisjointMutableValues, + isMutable, +} from "../ReactiveScopes/InferReactiveScopeVariables"; +import DisjointSet from "../Utils/DisjointSet"; import { assertExhaustive } from "../Utils/utils"; /* @@ -87,7 +91,7 @@ import { assertExhaustive } from "../Utils/utils"; * there are no changes after a given pass over the CFG. */ export function inferReactivePlaces(fn: HIRFunction): void { - const reactiveIdentifiers = new ReactivityMap(); + const reactiveIdentifiers = new ReactivityMap(findDisjointMutableValues(fn)); for (const param of fn.params) { const place = param.kind === "Identifier" ? param : param.place; reactiveIdentifiers.markReactive(place); @@ -328,15 +332,33 @@ class ReactivityMap { hasChanges: boolean = false; reactive: Set = new Set(); + /** + * Sets of mutably aliased identifiers — these are the same foundation for determining + * reactive scopes a few passes later. The actual InferReactiveScopeVariables pass runs + * after LeaveSSA, which artificially merges mutable ranges in cases such as declarations + * that are later reassigned. Here we use only the underlying sets of mutably aliased values. + * + * Any identifier that has a mapping in this disjoint set will be treated as a stand in for + * its canonical identifier in all cases, so that any reactivity flowing into one identifier of + * an alias group will effectively make the whole alias group (all its identifiers) reactive. + */ + aliasedIdentifiers: DisjointSet; + + constructor(aliasedIdentifiers: DisjointSet) { + this.aliasedIdentifiers = aliasedIdentifiers; + } + isReactive(place: Place): boolean { - const reactive = this.reactive.has(place.identifier.id); + const reactive = this.isReactiveIdentifier(place.identifier); if (reactive) { place.reactive = true; } return reactive; } - isReactiveIdentifier(identifier: Identifier): boolean { + isReactiveIdentifier(inputIdentifier: Identifier): boolean { + const identifier = + this.aliasedIdentifiers.find(inputIdentifier) ?? inputIdentifier; return this.reactive.has(identifier.id); } @@ -345,7 +367,9 @@ class ReactivityMap { this.markReactiveIdentifier(place.identifier); } - markReactiveIdentifier(identifier: Identifier): void { + markReactiveIdentifier(inputIdentifier: Identifier): void { + const identifier = + this.aliasedIdentifiers.find(inputIdentifier) ?? inputIdentifier; if (!this.reactive.has(identifier.id)) { this.hasChanges = true; this.reactive.add(identifier.id); 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 79b550bc42..af982beac3 100644 --- a/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/InferReactiveScopeVariables.ts +++ b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/InferReactiveScopeVariables.ts @@ -84,100 +84,7 @@ export function inferReactiveScopeVariables(fn: HIRFunction): void { * Represents the set of reactive scopes as disjoint sets of identifiers * that mutate together. */ - const scopeIdentifiers = new DisjointSet(); - for (const [_, block] of fn.body.blocks) { - /* - * If a phi is mutated after creation, then we need to alias all of its operands such that they - * are assigned to the same scope. - */ - for (const phi of block.phis) { - if ( - // The phi was reset because it was not mutated after creation - phi.id.mutableRange.start + 1 !== phi.id.mutableRange.end && - phi.id.mutableRange.end > - (block.instructions.at(0)?.id ?? block.terminal.id) - ) { - for (const [, phiId] of phi.operands) { - scopeIdentifiers.union([phi.id, phiId]); - } - } - } - block.phis.clear(); - - for (const instr of block.instructions) { - const operands: Array = []; - const range = instr.lvalue.identifier.mutableRange; - if (range.end > range.start + 1 || mayAllocate(fn.env, instr)) { - operands.push(instr.lvalue!.identifier); - } - if ( - instr.value.kind === "StoreLocal" || - instr.value.kind === "StoreContext" - ) { - if ( - instr.value.lvalue.place.identifier.mutableRange.end > - instr.value.lvalue.place.identifier.mutableRange.start + 1 - ) { - operands.push(instr.value.lvalue.place.identifier); - } - if ( - isMutable(instr, instr.value.value) && - instr.value.value.identifier.mutableRange.start > 0 - ) { - operands.push(instr.value.value.identifier); - } - } else if (instr.value.kind === "Destructure") { - for (const place of eachPatternOperand(instr.value.lvalue.pattern)) { - if ( - place.identifier.mutableRange.end > - place.identifier.mutableRange.start + 1 - ) { - operands.push(place.identifier); - } - } - if ( - isMutable(instr, instr.value.value) && - instr.value.value.identifier.mutableRange.start > 0 - ) { - operands.push(instr.value.value.identifier); - } - } else if (instr.value.kind === "MethodCall") { - for (const operand of eachInstructionOperand(instr)) { - if ( - isMutable(instr, operand) && - /* - * exclude global variables from being added to scopes, we can't recreate them! - * TODO: improve handling of module-scoped variables and globals - */ - operand.identifier.mutableRange.start > 0 - ) { - operands.push(operand.identifier); - } - } - /* - * Ensure that the ComputedLoad to resolve the method is in the same scope as the - * call itself - */ - operands.push(instr.value.property.identifier); - } else { - for (const operand of eachInstructionOperand(instr)) { - if ( - isMutable(instr, operand) && - /* - * exclude global variables from being added to scopes, we can't recreate them! - * TODO: improve handling of module-scoped variables and globals - */ - operand.identifier.mutableRange.start > 0 - ) { - operands.push(operand.identifier); - } - } - } - if (operands.length !== 0) { - scopeIdentifiers.union(operands); - } - } - } + const scopeIdentifiers = findDisjointMutableValues(fn); // Maps each scope (by its identifying member) to a ScopeId value const scopes: Map = new Map(); @@ -278,3 +185,102 @@ function mayAllocate(env: Environment, instruction: Instruction): boolean { } } } + +export function findDisjointMutableValues( + fn: HIRFunction +): DisjointSet { + const scopeIdentifiers = new DisjointSet(); + for (const [_, block] of fn.body.blocks) { + /* + * If a phi is mutated after creation, then we need to alias all of its operands such that they + * are assigned to the same scope. + */ + for (const phi of block.phis) { + if ( + // The phi was reset because it was not mutated after creation + phi.id.mutableRange.start + 1 !== phi.id.mutableRange.end && + phi.id.mutableRange.end > + (block.instructions.at(0)?.id ?? block.terminal.id) + ) { + for (const [, phiId] of phi.operands) { + scopeIdentifiers.union([phi.id, phiId]); + } + } + } + + for (const instr of block.instructions) { + const operands: Array = []; + const range = instr.lvalue.identifier.mutableRange; + if (range.end > range.start + 1 || mayAllocate(fn.env, instr)) { + operands.push(instr.lvalue!.identifier); + } + if ( + instr.value.kind === "StoreLocal" || + instr.value.kind === "StoreContext" + ) { + if ( + instr.value.lvalue.place.identifier.mutableRange.end > + instr.value.lvalue.place.identifier.mutableRange.start + 1 + ) { + operands.push(instr.value.lvalue.place.identifier); + } + if ( + isMutable(instr, instr.value.value) && + instr.value.value.identifier.mutableRange.start > 0 + ) { + operands.push(instr.value.value.identifier); + } + } else if (instr.value.kind === "Destructure") { + for (const place of eachPatternOperand(instr.value.lvalue.pattern)) { + if ( + place.identifier.mutableRange.end > + place.identifier.mutableRange.start + 1 + ) { + operands.push(place.identifier); + } + } + if ( + isMutable(instr, instr.value.value) && + instr.value.value.identifier.mutableRange.start > 0 + ) { + operands.push(instr.value.value.identifier); + } + } else if (instr.value.kind === "MethodCall") { + for (const operand of eachInstructionOperand(instr)) { + if ( + isMutable(instr, operand) && + /* + * exclude global variables from being added to scopes, we can't recreate them! + * TODO: improve handling of module-scoped variables and globals + */ + operand.identifier.mutableRange.start > 0 + ) { + operands.push(operand.identifier); + } + } + /* + * Ensure that the ComputedLoad to resolve the method is in the same scope as the + * call itself + */ + operands.push(instr.value.property.identifier); + } else { + for (const operand of eachInstructionOperand(instr)) { + if ( + isMutable(instr, operand) && + /* + * exclude global variables from being added to scopes, we can't recreate them! + * TODO: improve handling of module-scoped variables and globals + */ + operand.identifier.mutableRange.start > 0 + ) { + operands.push(operand.identifier); + } + } + } + if (operands.length !== 0) { + scopeIdentifiers.union(operands); + } + } + } + return scopeIdentifiers; +} diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactivity-via-aliased-mutation-array.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactivity-via-aliased-mutation-array.expect.md new file mode 100644 index 0000000000..c529cbe311 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactivity-via-aliased-mutation-array.expect.md @@ -0,0 +1,84 @@ + +## Input + +```javascript +function Component(props) { + const x = []; + const y = x; + y.push(props.input); + + return [x[0]]; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [], + sequentialRenders: [ + { input: 42 }, + { input: 42 }, + { input: "sathya" }, + { input: "sathya" }, + { input: 42 }, + { input: "sathya" }, + { input: 42 }, + { input: "sathya" }, + ], +}; + +``` + +## Code + +```javascript +import { unstable_useMemoCache as useMemoCache } from "react"; +function Component(props) { + const $ = useMemoCache(4); + let x; + if ($[0] !== props.input) { + x = []; + const y = x; + y.push(props.input); + $[0] = props.input; + $[1] = x; + } else { + x = $[1]; + } + + const t0 = x[0]; + let t1; + if ($[2] !== t0) { + t1 = [t0]; + $[2] = t0; + $[3] = t1; + } else { + t1 = $[3]; + } + return t1; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [], + sequentialRenders: [ + { input: 42 }, + { input: 42 }, + { input: "sathya" }, + { input: "sathya" }, + { input: 42 }, + { input: "sathya" }, + { input: 42 }, + { input: "sathya" }, + ], +}; + +``` + +### Eval output +(kind: ok) [42] +[42] +["sathya"] +["sathya"] +[42] +["sathya"] +[42] +["sathya"] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactivity-via-aliased-mutation-array.js b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactivity-via-aliased-mutation-array.js new file mode 100644 index 0000000000..24252844d4 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactivity-via-aliased-mutation-array.js @@ -0,0 +1,22 @@ +function Component(props) { + const x = []; + const y = x; + y.push(props.input); + + return [x[0]]; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [], + sequentialRenders: [ + { input: 42 }, + { input: 42 }, + { input: "sathya" }, + { input: "sathya" }, + { input: 42 }, + { input: "sathya" }, + { input: 42 }, + { input: "sathya" }, + ], +}; diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactivity-via-aliased-mutation-lambda.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactivity-via-aliased-mutation-lambda.expect.md new file mode 100644 index 0000000000..659da1c8e0 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactivity-via-aliased-mutation-lambda.expect.md @@ -0,0 +1,91 @@ + +## Input + +```javascript +function Component(props) { + const x = []; + const f = (arg) => { + const y = x; + y.push(arg); + }; + f(props.input); + + return [x[0]]; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [], + sequentialRenders: [ + { input: 42 }, + { input: 42 }, + { input: "sathya" }, + { input: "sathya" }, + { input: 42 }, + { input: "sathya" }, + { input: 42 }, + { input: "sathya" }, + ], +}; + +``` + +## Code + +```javascript +import { unstable_useMemoCache as useMemoCache } from "react"; +function Component(props) { + const $ = useMemoCache(4); + let x; + if ($[0] !== props.input) { + x = []; + const f = (arg) => { + const y = x; + y.push(arg); + }; + + f(props.input); + $[0] = props.input; + $[1] = x; + } else { + x = $[1]; + } + + const t0 = x[0]; + let t1; + if ($[2] !== t0) { + t1 = [t0]; + $[2] = t0; + $[3] = t1; + } else { + t1 = $[3]; + } + return t1; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [], + sequentialRenders: [ + { input: 42 }, + { input: 42 }, + { input: "sathya" }, + { input: "sathya" }, + { input: 42 }, + { input: "sathya" }, + { input: 42 }, + { input: "sathya" }, + ], +}; + +``` + +### Eval output +(kind: ok) [42] +[42] +["sathya"] +["sathya"] +[42] +["sathya"] +[42] +["sathya"] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactivity-via-aliased-mutation-lambda.js b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactivity-via-aliased-mutation-lambda.js new file mode 100644 index 0000000000..071ed029e4 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactivity-via-aliased-mutation-lambda.js @@ -0,0 +1,25 @@ +function Component(props) { + const x = []; + const f = (arg) => { + const y = x; + y.push(arg); + }; + f(props.input); + + return [x[0]]; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [], + sequentialRenders: [ + { input: 42 }, + { input: 42 }, + { input: "sathya" }, + { input: "sathya" }, + { input: 42 }, + { input: "sathya" }, + { input: 42 }, + { input: "sathya" }, + ], +}; diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/repro-dont-memoize-array-with-capturing-map-after-hook.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/repro-dont-memoize-array-with-capturing-map-after-hook.expect.md index b0f65cbb3d..3c1fccb083 100644 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/repro-dont-memoize-array-with-capturing-map-after-hook.expect.md +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/repro-dont-memoize-array-with-capturing-map-after-hook.expect.md @@ -42,7 +42,7 @@ import { import { mutate } from "shared-runtime"; function Component(props) { - const $ = useMemoCache(5); + const $ = useMemoCache(6); const x = [{ ...props.value }]; let t0; let t1; @@ -66,21 +66,23 @@ function Component(props) { y = item; return {item.text}; }); - let t3; - if ($[2] !== onClick || $[3] !== t2) { - t3 = ( + const t3 = mutate(y); + let t4; + if ($[2] !== onClick || $[3] !== t2 || $[4] !== t3) { + t4 = (
{t2} - {mutate(y)} + {t3}
); $[2] = onClick; $[3] = t2; $[4] = t3; + $[5] = t4; } else { - t3 = $[4]; + t4 = $[5]; } - return t3; + return t4; } export const FIXTURE_ENTRYPOINT = { diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/repro-scope-missing-mutable-range.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/repro-scope-missing-mutable-range.expect.md index e069671fab..6bce4ce079 100644 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/repro-scope-missing-mutable-range.expect.md +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/repro-scope-missing-mutable-range.expect.md @@ -20,7 +20,7 @@ function HomeDiscoStoreItemTileRating(props) { ```javascript import { unstable_useMemoCache as useMemoCache } from "react"; function HomeDiscoStoreItemTileRating(props) { - const $ = useMemoCache(3); + const $ = useMemoCache(4); const item = useFragment(); let count; if ($[0] !== item) { @@ -34,14 +34,17 @@ function HomeDiscoStoreItemTileRating(props) { } else { count = $[1]; } - let t0; - if ($[2] === Symbol.for("react.memo_cache_sentinel")) { - t0 = {count}; + + const t0 = count; + let t1; + if ($[2] !== t0) { + t1 = {t0}; $[2] = t0; + $[3] = t1; } else { - t0 = $[2]; + t1 = $[3]; } - return t0; + return t1; } ```