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, +};