diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts index 2044860709093..bbe0f17f59425 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts @@ -276,9 +276,16 @@ function validateInferredDep( } class Visitor extends ReactiveFunctionVisitor { + /** + * Records all completed scopes (regardless of transitive memoization + * of scope dependencies) + * + * Both @scopes and @prunedScopes are live sets. We rely on iterating + * the reactive-ir in evaluation order, as they are used to determine + * whether scope dependencies / declarations have completed mutation. + */ scopes: Set = new Set(); prunedScopes: Set = new Set(); - scopeMapping = new Map(); temporaries: Map = new Map(); /** @@ -394,25 +401,9 @@ class Visitor extends ReactiveFunctionVisitor { } } - /* - * 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); - } + this.scopes.add(scopeBlock.scope.id); + for (const id of scopeBlock.scope.merged) { + this.scopes.add(id); } } @@ -453,6 +444,23 @@ class Visitor extends ReactiveFunctionVisitor { manualMemoId: value.manualMemoId, }; + /** + * We check that each scope dependency is either: + * (1) Not scoped + * Checking `identifier.scope == null` is a proxy for whether the dep + * is a primitive, global, or other guaranteed non-allocating value. + * Non-allocating values do not need memoization. + * Note that this is a conservative estimate as some primitive-typed + * variables do receive scopes. + * (2) Scoped (a maybe newly-allocated value with a mutable range) + * Here, we check that the dependency's scope has completed before + * the manual useMemo as a proxy for mutable-range checking. This + * validates that there are no potential rule-of-react violations + * in source. + * Note that scope range is an overly conservative proxy as we merge + * overlapping ranges. + * See fixture `error.false-positive-useMemo-overlap-scopes` + */ for (const {identifier, loc} of eachInstructionValueOperand( value as InstructionValue, )) { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-extended-contextvar-scope.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-extended-contextvar-scope.expect.md deleted file mode 100644 index 7adedf39a1518..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-extended-contextvar-scope.expect.md +++ /dev/null @@ -1,57 +0,0 @@ - -## Input - -```javascript -// @validatePreserveExistingMemoizationGuarantees:true - -import {useCallback} from 'react'; -import {Stringify, useIdentity} from 'shared-runtime'; - -/** - * Here, the *inferred* dependencies of cb are `a` and `t1 = LoadContext capture x_@1`. - * - t1 does not have a scope as it captures `x` after x's mutable range - * - `x` is a context variable, which means its mutable range extends to all - * references / aliases. - * - `a`, `b`, and `x` get the same mutable range due to potential aliasing. - * - * We currently bail out because `a` has a scope and is not transitively memoized - * (as its scope is pruned due to a hook call) - */ -function useBar({a, b}, cond) { - let x = useIdentity({val: 3}); - if (cond) { - x = b; - } - - const cb = useCallback(() => { - return [a, x]; - }, [a, x]); - - return ; -} - -export const FIXTURE_ENTRYPOINT = { - fn: useBar, - params: [{a: 1, b: 2}, true], -}; - -``` - - -## Error - -``` - 20 | } - 21 | -> 22 | const cb = useCallback(() => { - | ^^^^^^^ -> 23 | return [a, x]; - | ^^^^^^^^^^^^^^^^^^ -> 24 | }, [a, x]); - | ^^^^ CannotPreserveMemoization: React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. This value was memoized in source but not in compilation output. (22:24) - 25 | - 26 | return ; - 27 | } -``` - - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-useCallback-dep-scope-pruned.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-useCallback-dep-scope-pruned.expect.md deleted file mode 100644 index 43df10dee2cf7..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-useCallback-dep-scope-pruned.expect.md +++ /dev/null @@ -1,42 +0,0 @@ - -## Input - -```javascript -// @validatePreserveExistingMemoizationGuarantees -import {useCallback} from 'react'; -import {identity, useIdentity} from 'shared-runtime'; - -/** - * Repro showing a manual memo whose declaration (useCallback's 1st argument) - * is memoized, but not its dependency (x). In this case, `x`'s scope is pruned - * due to hook-call flattening. - */ -function useFoo(a) { - const x = identity(a); - useIdentity(2); - mutate(x); - - return useCallback(() => [x, []], [x]); -} - -export const FIXTURE_ENTRYPOINT = { - fn: useFoo, - params: [3], -}; - -``` - - -## Error - -``` - 13 | mutate(x); - 14 | -> 15 | return useCallback(() => [x, []], [x]); - | ^^^^^^^^^^^^^ CannotPreserveMemoization: React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. This value was memoized in source but not in compilation output. (15:15) - 16 | } - 17 | - 18 | export const FIXTURE_ENTRYPOINT = { -``` - - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-dep-scope-pruned.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-dep-scope-pruned.expect.md new file mode 100644 index 0000000000000..184779869e9a3 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-dep-scope-pruned.expect.md @@ -0,0 +1,65 @@ + +## Input + +```javascript +// @validatePreserveExistingMemoizationGuarantees +import {useCallback} from 'react'; +import {identity, useIdentity} from 'shared-runtime'; + +/** + * Repro showing a manual memo whose declaration (useCallback's 1st argument) + * is memoized, but not its dependency (x). In this case, `x`'s scope is pruned + * due to hook-call flattening. + */ +function useFoo(a) { + const x = identity(a); + useIdentity(2); + mutate(x); + + return useCallback(() => [x, []], [x]); +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [3], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validatePreserveExistingMemoizationGuarantees +import { useCallback } from "react"; +import { identity, useIdentity } from "shared-runtime"; + +/** + * Repro showing a manual memo whose declaration (useCallback's 1st argument) + * is memoized, but not its dependency (x). In this case, `x`'s scope is pruned + * due to hook-call flattening. + */ +function useFoo(a) { + const $ = _c(2); + const x = identity(a); + useIdentity(2); + mutate(x); + let t0; + if ($[0] !== x) { + t0 = () => [x, []]; + $[0] = x; + $[1] = t0; + } else { + t0 = $[1]; + } + return t0; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [3], +}; + +``` + +### Eval output +(kind: exception) mutate is not defined \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-useCallback-dep-scope-pruned.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-dep-scope-pruned.ts similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-useCallback-dep-scope-pruned.ts rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-dep-scope-pruned.ts diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-extended-contextvar-scope.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-extended-contextvar-scope.expect.md new file mode 100644 index 0000000000000..b141c27614d24 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-extended-contextvar-scope.expect.md @@ -0,0 +1,105 @@ + +## Input + +```javascript +// @validatePreserveExistingMemoizationGuarantees:true + +import {useCallback} from 'react'; +import {Stringify, useIdentity} from 'shared-runtime'; + +/** + * Here, the *inferred* dependencies of cb are `a` and `t1 = LoadContext capture x_@1`. + * - t1 does not have a scope as it captures `x` after x's mutable range + * - `x` is a context variable, which means its mutable range extends to all + * references / aliases. + * - `a`, `b`, and `x` get the same mutable range due to potential aliasing. + * + * We currently bail out because `a` has a scope and is not transitively memoized + * (as its scope is pruned due to a hook call) + */ +function useBar({a, b}, cond) { + let x = useIdentity({val: 3}); + if (cond) { + x = b; + } + + const cb = useCallback(() => { + return [a, x]; + }, [a, x]); + + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useBar, + params: [{a: 1, b: 2}, true], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validatePreserveExistingMemoizationGuarantees:true + +import { useCallback } from "react"; +import { Stringify, useIdentity } from "shared-runtime"; + +/** + * Here, the *inferred* dependencies of cb are `a` and `t1 = LoadContext capture x_@1`. + * - t1 does not have a scope as it captures `x` after x's mutable range + * - `x` is a context variable, which means its mutable range extends to all + * references / aliases. + * - `a`, `b`, and `x` get the same mutable range due to potential aliasing. + * + * We currently bail out because `a` has a scope and is not transitively memoized + * (as its scope is pruned due to a hook call) + */ +function useBar(t0, cond) { + const $ = _c(6); + const { a, b } = t0; + let t1; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t1 = { val: 3 }; + $[0] = t1; + } else { + t1 = $[0]; + } + let x; + x = useIdentity(t1); + if (cond) { + x = b; + } + + const t2 = x; + let t3; + if ($[1] !== a || $[2] !== t2) { + t3 = () => [a, x]; + $[1] = a; + $[2] = t2; + $[3] = t3; + } else { + t3 = $[3]; + } + x; + const cb = t3; + let t4; + if ($[4] !== cb) { + t4 = ; + $[4] = cb; + $[5] = t4; + } else { + t4 = $[5]; + } + return t4; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useBar, + params: [{ a: 1, b: 2 }, true], +}; + +``` + +### Eval output +(kind: ok)
{"cb":"[[ function params=0 ]]","shouldInvoke":true}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-extended-contextvar-scope.tsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-extended-contextvar-scope.tsx similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-extended-contextvar-scope.tsx rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useCallback-extended-contextvar-scope.tsx