From 445baf9ae4356acb9e19dd04f31159a49171ca2a Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Mon, 27 Nov 2023 12:31:27 -0800 Subject: [PATCH] Extend effect dep validation to handle pruned memoization Extends the validation that effect deps are memoized to handle an additional case that @gsathya pointed out: when a dependency has a reactive scope but that scope ends up being pruned. We track reactive scopes which actually exist in the ReactiveFunction, and reject useEffect deps that have an associated reactive scope but where that scope does not exist (bc it got pruned). --- .../babel-plugin-react-forget/src/HIR/HIR.ts | 8 ++ .../InferReactiveScopeVariables.ts | 1 + ...rgeReactiveScopesThatInvalidateTogether.ts | 1 + .../ValidateMemoizedEffectDependencies.ts | 55 ++++++++++++-- ...-memoized-bc-range-overlaps-hook.expect.md | 29 ++++++++ ...dep-not-memoized-bc-range-overlaps-hook.js | 14 ++++ ...ged-scopes-are-valid-effect-deps.expect.md | 73 +++++++++++++++++++ .../merged-scopes-are-valid-effect-deps.js | 19 +++++ 8 files changed, 193 insertions(+), 7 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-useEffect-dep-not-memoized-bc-range-overlaps-hook.expect.md create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-useEffect-dep-not-memoized-bc-range-overlaps-hook.js create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/merged-scopes-are-valid-effect-deps.expect.md create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/merged-scopes-are-valid-effect-deps.js diff --git a/compiler/packages/babel-plugin-react-forget/src/HIR/HIR.ts b/compiler/packages/babel-plugin-react-forget/src/HIR/HIR.ts index 2fbf684ca1..8e5b08e28e 100644 --- a/compiler/packages/babel-plugin-react-forget/src/HIR/HIR.ts +++ b/compiler/packages/babel-plugin-react-forget/src/HIR/HIR.ts @@ -1016,6 +1016,14 @@ export type ReactiveScope = { dependencies: ReactiveScopeDependencies; declarations: Map; reassignments: Set; + + /* + * Some passes may merge scopes together. The merged set contains the + * ids of scopes that were merged into this one, for passes that need + * to track which scopes are still present (in some form) vs scopes that + * no longer exist due to being pruned. + */ + merged: Set; }; export type ReactiveScopeDependencies = Set; 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 9a74a869f2..6cf90fcd92 100644 --- a/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/InferReactiveScopeVariables.ts +++ b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/InferReactiveScopeVariables.ts @@ -199,6 +199,7 @@ export function inferReactiveScopeVariables(fn: HIRFunction): void { dependencies: new Set(), declarations: new Map(), reassignments: new Set(), + merged: new Set(), }; scopes.set(groupIdentifier, scope); } else { diff --git a/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/MergeReactiveScopesThatInvalidateTogether.ts b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/MergeReactiveScopesThatInvalidateTogether.ts index 4a48376c25..b16d7591aa 100644 --- a/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/MergeReactiveScopesThatInvalidateTogether.ts +++ b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/MergeReactiveScopesThatInvalidateTogether.ts @@ -314,6 +314,7 @@ class Transform extends ReactiveFunctionTransform { + scopes: Set = new Set(); + + override visitScope( + scopeBlock: ReactiveScopeBlock, + state: CompilerError + ): void { + this.traverseScope(scopeBlock, state); + + /* + * Record scopes that exist in the AST so we can later check to see if + * effect dependencies which should be memoized (have a scope assigned) + * actually are memoized (that scope exists). + * However, we only record scopes if *their* dependencies are also + * memoized, allowing a transitive memoization check. + */ + let areDependenciesMemoized = true; + for (const dep of scopeBlock.scope.dependencies) { + if (isUnmemoized(dep.identifier, this.scopes)) { + areDependenciesMemoized = false; + break; + } + } + if (areDependenciesMemoized) { + this.scopes.add(scopeBlock.scope.id); + for (const id of scopeBlock.scope.merged) { + this.scopes.add(id); + } + } + } + override visitInstruction( instruction: ReactiveInstruction, state: CompilerError @@ -63,7 +99,8 @@ class Visitor extends ReactiveFunctionVisitor { const deps = instruction.value.args[1]!; if ( deps.kind === "Identifier" && - isMutable(instruction as Instruction, deps) + (isMutable(instruction as Instruction, deps) || + isUnmemoized(deps.identifier, this.scopes)) ) { state.push({ reason: @@ -78,6 +115,10 @@ class Visitor extends ReactiveFunctionVisitor { } } +function isUnmemoized(operand: Identifier, scopes: Set): boolean { + return operand.scope != null && !scopes.has(operand.scope.id); +} + function isEffectHook(identifier: Identifier): boolean { return ( isUseEffectHookType(identifier) || diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-useEffect-dep-not-memoized-bc-range-overlaps-hook.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-useEffect-dep-not-memoized-bc-range-overlaps-hook.expect.md new file mode 100644 index 0000000000..d3bbd71c45 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-useEffect-dep-not-memoized-bc-range-overlaps-hook.expect.md @@ -0,0 +1,29 @@ + +## Input + +```javascript +// @validateMemoizedEffectDependencies +function Component(props) { + // Items cannot be memoized bc its mutation spans a hook call + const items = [props.value]; + const [state, _setState] = useState(null); + mutate(items); + + // Items is no longer mutable here, but it hasn't been memoized + useEffect(() => { + console.log(items); + }, [items]); + + return [items, state]; +} + +``` + + +## Error + +``` +[ReactForget] InvalidReact: This effect may trigger an infinite loop: one or more of its dependencies could not be memoized due to a later mutation (9:11) +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-useEffect-dep-not-memoized-bc-range-overlaps-hook.js b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-useEffect-dep-not-memoized-bc-range-overlaps-hook.js new file mode 100644 index 0000000000..20a0d9b606 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-useEffect-dep-not-memoized-bc-range-overlaps-hook.js @@ -0,0 +1,14 @@ +// @validateMemoizedEffectDependencies +function Component(props) { + // Items cannot be memoized bc its mutation spans a hook call + const items = [props.value]; + const [state, _setState] = useState(null); + mutate(items); + + // Items is no longer mutable here, but it hasn't been memoized + useEffect(() => { + console.log(items); + }, [items]); + + return [items, state]; +} diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/merged-scopes-are-valid-effect-deps.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/merged-scopes-are-valid-effect-deps.expect.md new file mode 100644 index 0000000000..16355c078d --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/merged-scopes-are-valid-effect-deps.expect.md @@ -0,0 +1,73 @@ + +## Input + +```javascript +// @validateMemoizedEffectDependencies + +import { useEffect } from "react"; + +function Component(props) { + const y = [[props.value]]; // merged w scope for inner array + + useEffect(() => { + console.log(y); + }, [y]); // should still be a valid dependency here + + return y; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ value: 42 }], + isComponent: false, +}; + +``` + +## Code + +```javascript +// @validateMemoizedEffectDependencies + +import { useEffect, unstable_useMemoCache as useMemoCache } from "react"; + +function Component(props) { + const $ = useMemoCache(5); + let t0; + if ($[0] !== props.value) { + t0 = [[props.value]]; + $[0] = props.value; + $[1] = t0; + } else { + t0 = $[1]; + } + const y = t0; + let t1; + let t2; + if ($[2] !== y) { + t1 = () => { + console.log(y); + }; + t2 = [y]; + $[2] = y; + $[3] = t1; + $[4] = t2; + } else { + t1 = $[3]; + t2 = $[4]; + } + useEffect(t1, t2); + return y; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ value: 42 }], + isComponent: false, +}; + +``` + +### Eval output +(kind: ok) [[42]] +logs: [[ [ 42 ] ]] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/merged-scopes-are-valid-effect-deps.js b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/merged-scopes-are-valid-effect-deps.js new file mode 100644 index 0000000000..ec1e65d64d --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/merged-scopes-are-valid-effect-deps.js @@ -0,0 +1,19 @@ +// @validateMemoizedEffectDependencies + +import { useEffect } from "react"; + +function Component(props) { + const y = [[props.value]]; // merged w scope for inner array + + useEffect(() => { + console.log(y); + }, [y]); // should still be a valid dependency here + + return y; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ value: 42 }], + isComponent: false, +};