From 4d84bee1723ad024058dda7e9a47774146912c77 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Mon, 22 Jan 2024 15:35:56 -0800 Subject: [PATCH] Propagate reactive scope dependencies transitively During PruneNonReactiveDependencies, we sometimes need to promote a value from non-reactive to reactive if it ended up being grouped in the same reactive scope as some other reactive value. This generally happens due to interleaving mutations. In this case all downstream usage of the promoted value need to also be considered reactive. Fully propagating the reactivity requires re-running InferReactivePlaces, to account for things like control reactivity. We can't yet reuse that pass here though, because we haven't unified the pipeline on HIR yet. For now, we propagate the reactivity through local variables and downstream reactive scopes. See test fixtures for some examples that now correctly propagate reactivity and some that need the full reactivity inference to run correctly. The latter cases are handled in the next PR. --- .../PruneNonReactiveDependencies.ts | 100 +++++++++++++++--- ...y-from-interleaved-reactivity-if.expect.md | 77 ++++++++++++++ ...pendency-from-interleaved-reactivity-if.js | 31 ++++++ ...-analysis-interleaved-reactivity.expect.md | 11 +- 4 files changed, 199 insertions(+), 20 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactive-control-dependency-from-interleaved-reactivity-if.expect.md create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactive-control-dependency-from-interleaved-reactivity-if.js diff --git a/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/PruneNonReactiveDependencies.ts b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/PruneNonReactiveDependencies.ts index 4501487f1e..6e8c3b903a 100644 --- a/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/PruneNonReactiveDependencies.ts +++ b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/PruneNonReactiveDependencies.ts @@ -5,7 +5,14 @@ * LICENSE file in the root directory of this source tree. */ -import { IdentifierId, ReactiveFunction, ReactiveScopeBlock } from "../HIR"; +import { + IdentifierId, + ReactiveFunction, + ReactiveInstruction, + ReactiveScopeBlock, + isSetStateType, +} from "../HIR"; +import { eachPatternOperand } from "../HIR/visitors"; import { collectReactiveIdentifiers } from "./CollectReactiveIdentifiers"; import { ReactiveFunctionVisitor, visitReactiveFunction } from "./visitors"; @@ -23,27 +30,90 @@ export function pruneNonReactiveDependencies(fn: ReactiveFunction): void { type ReactiveIdentifiers = Set; class Visitor extends ReactiveFunctionVisitor { - override visitScope( - scope: ReactiveScopeBlock, + override visitInstruction( + instruction: ReactiveInstruction, state: ReactiveIdentifiers ): void { - this.traverseScope(scope, state); - for (const dep of scope.scope.dependencies) { - const isReactive = state.has(dep.identifier.id); - if (!isReactive) { - scope.scope.dependencies.delete(dep); + this.traverseInstruction(instruction, state); + + const { lvalue, value } = instruction; + switch (value.kind) { + case "LoadLocal": { + if (lvalue !== null && state.has(value.place.identifier.id)) { + state.add(lvalue.identifier.id); + } + break; + } + case "StoreLocal": { + if (state.has(value.value.identifier.id)) { + state.add(value.lvalue.place.identifier.id); + if (lvalue !== null) { + state.add(lvalue.identifier.id); + } + } + break; + } + case "Destructure": { + if (state.has(value.value.identifier.id)) { + for (const lvalue of eachPatternOperand(value.lvalue.pattern)) { + if (isSetStateType(lvalue.identifier)) { + continue; + } + state.add(lvalue.identifier.id); + } + if (lvalue !== null) { + state.add(lvalue.identifier.id); + } + } + break; + } + case "PropertyLoad": { + if ( + lvalue !== null && + state.has(value.object.identifier.id) && + !isSetStateType(lvalue.identifier) + ) { + state.add(lvalue.identifier.id); + } + break; + } + case "ComputedLoad": { + if ( + lvalue !== null && + (state.has(value.object.identifier.id) || + state.has(value.property.identifier.id)) + ) { + state.add(lvalue.identifier.id); + } + break; } } - if (scope.scope.dependencies.size === 0) { - // If a scope has no dependencies, then its declarations are all non-reactive - for (const [, declaration] of scope.scope.declarations) { - state.delete(declaration.identifier.id); + } + + override visitScope( + scopeBlock: ReactiveScopeBlock, + state: ReactiveIdentifiers + ): void { + this.traverseScope(scopeBlock, state); + for (const dep of scopeBlock.scope.dependencies) { + const isReactive = state.has(dep.identifier.id); + if (!isReactive) { + scopeBlock.scope.dependencies.delete(dep); } - } else { - // otherwise, all the scope's declarations are reactive - for (const [, declaration] of scope.scope.declarations) { + } + if (scopeBlock.scope.dependencies.size !== 0) { + /** + * If any of a scope's dependencies are reactive, then all of its + * outputs will re-evaluate whenever those dependencies change. + * Mark all of the outputs as reactive to reflect the fact that + * they may change in practice based on a reactive input. + */ + for (const [, declaration] of scopeBlock.scope.declarations) { state.add(declaration.identifier.id); } + for (const reassignment of scopeBlock.scope.reassignments) { + state.add(reassignment.id); + } } } } diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactive-control-dependency-from-interleaved-reactivity-if.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactive-control-dependency-from-interleaved-reactivity-if.expect.md new file mode 100644 index 0000000000..098077f59f --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactive-control-dependency-from-interleaved-reactivity-if.expect.md @@ -0,0 +1,77 @@ + +## Input + +```javascript +function Component(props) { + // a and b are independent but their mutations are interleaved, so + // they get grouped in a reactive scope. this means that a becomes + // reactive since it will effectively re-evaluate based on a reactive + // input + const a = []; + const b = []; + b.push(props.cond); + a.push(null); + + // Downstream consumer of a, which initially seems non-reactive except + // that a becomes reactive, per above + const c = [a]; + + let x; + if (c[0]) { + x = 1; + } else { + x = 2; + } + // The values assigned to `x` are non-reactive, but the value of `x` + // depends on the "control" value `c[0]` which becomes reactive via + // being interleaved with `b`. + // Therefore x should be treated as reactive too. + return [x]; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ cond: true }], +}; + +``` + +## Code + +```javascript +import { unstable_useMemoCache as useMemoCache } from "react"; +function Component(props) { + const $ = useMemoCache(1); + + const a = []; + const b = []; + b.push(props.cond); + a.push(null); + + const c = [a]; + + let x; + if (c[0]) { + x = 1; + } else { + x = 2; + } + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 = [x]; + $[0] = t0; + } else { + t0 = $[0]; + } + return t0; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ cond: true }], +}; + +``` + +### Eval output +(kind: ok) [1] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactive-control-dependency-from-interleaved-reactivity-if.js b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactive-control-dependency-from-interleaved-reactivity-if.js new file mode 100644 index 0000000000..9c07d33b12 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactive-control-dependency-from-interleaved-reactivity-if.js @@ -0,0 +1,31 @@ +function Component(props) { + // a and b are independent but their mutations are interleaved, so + // they get grouped in a reactive scope. this means that a becomes + // reactive since it will effectively re-evaluate based on a reactive + // input + const a = []; + const b = []; + b.push(props.cond); + a.push(null); + + // Downstream consumer of a, which initially seems non-reactive except + // that a becomes reactive, per above + const c = [a]; + + let x; + if (c[0]) { + x = 1; + } else { + x = 2; + } + // The values assigned to `x` are non-reactive, but the value of `x` + // depends on the "control" value `c[0]` which becomes reactive via + // being interleaved with `b`. + // Therefore x should be treated as reactive too. + return [x]; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ cond: true }], +}; diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactivity-analysis-interleaved-reactivity.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactivity-analysis-interleaved-reactivity.expect.md index 22d2109a07..7cdb73ba9d 100644 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactivity-analysis-interleaved-reactivity.expect.md +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactivity-analysis-interleaved-reactivity.expect.md @@ -35,7 +35,7 @@ export const FIXTURE_ENTRYPOINT = { ```javascript import { unstable_useMemoCache as useMemoCache } from "react"; function Component(props) { - const $ = useMemoCache(6); + const $ = useMemoCache(7); let a; if ($[0] !== props.b) { a = {}; @@ -57,12 +57,13 @@ function Component(props) { } const c = t0; let t1; - if ($[4] !== a) { + if ($[4] !== c || $[5] !== a) { t1 = [c, a]; - $[4] = a; - $[5] = t1; + $[4] = c; + $[5] = a; + $[6] = t1; } else { - t1 = $[5]; + t1 = $[6]; } return t1; }