From 2e123c278c6968596133d11062e2dc9f166276f0 Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Mon, 5 Aug 2024 22:20:50 -0400 Subject: [PATCH 1/2] [compiler] Patch ValidatePreserveMemo to bailout correctly for refs [ghstack-poisoned] --- .../ValidatePreservedManualMemoization.ts | 57 +++++++++++++++--- ...e-mutable-ref-memo-not-preserved.expect.md | 45 -------------- ...false-positive-prune-nonescaping.expect.md | 38 ++++++++++++ .../error.false-positive-prune-nonescaping.ts | 17 ++++++ ...ropped-infer-always-invalidating.expect.md | 42 ++++++++++++++ ...Memo-dropped-infer-always-invalidating.ts} | 4 +- ....maybe-mutable-ref-not-preserved.expect.md | 35 +++++++++++ .../error.maybe-mutable-ref-not-preserved.ts} | 0 ...ropped-infer-always-invalidating.expect.md | 58 ------------------- 9 files changed, 182 insertions(+), 114 deletions(-) delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-maybe-mutable-ref-memo-not-preserved.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-prune-nonescaping.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-prune-nonescaping.ts create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-useMemo-dropped-infer-always-invalidating.expect.md rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/{useMemo-dropped-infer-always-invalidating.ts => error.false-positive-useMemo-dropped-infer-always-invalidating.ts} (81%) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.maybe-mutable-ref-not-preserved.expect.md rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{bug-maybe-mutable-ref-memo-not-preserved.ts => preserve-memo-validation/error.maybe-mutable-ref-not-preserved.ts} (100%) delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useMemo-dropped-infer-always-invalidating.expect.md 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 29759e9e7b58..31e15f78f8ce 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts @@ -30,6 +30,7 @@ import { ReactiveFunctionVisitor, visitReactiveFunction, } from '../ReactiveScopes/visitors'; +import {getOrInsertDefault} from '../Utils/utils'; /** * Validates that all explicit manual memoization (useMemo/useCallback) was accurately @@ -52,6 +53,16 @@ export function validatePreservedManualMemoization(fn: ReactiveFunction): void { const DEBUG = false; type ManualMemoBlockState = { + /** + * Tracks reassigned temporaries. + * This is necessary because useMemo calls are usually inlined. + * Inlining produces a `let` declaration, followed by reassignments + * to the newly declared variable (one per return statement). + * Since InferReactiveScopes does not merge scopes across reassigned + * variables (except in the case of a mutate-after-phi), we need to + * track reassignments to validate we're retaining manual memo. + */ + reassignments: Map>; // The source of the original memoization, used when reporting errors loc: SourceLocation; @@ -433,6 +444,19 @@ class Visitor extends ReactiveFunctionVisitor { * recursively visits ReactiveValues and instructions */ this.recordTemporaries(instruction, state); + const value = instruction.value; + if ( + value.kind === 'StoreLocal' && + value.lvalue.kind === 'Reassign' && + state.manualMemoState != null + ) { + const ids = getOrInsertDefault( + state.manualMemoState.reassignments, + value.lvalue.place.identifier.declarationId, + new Set(), + ); + ids.add(value.value.identifier); + } if (instruction.value.kind === 'StartMemoize') { let depsFromSource: Array | null = null; if (instruction.value.deps != null) { @@ -450,6 +474,7 @@ class Visitor extends ReactiveFunctionVisitor { decls: new Set(), depsFromSource, manualMemoId: instruction.value.manualMemoId, + reassignments: new Map(), }; for (const {identifier, loc} of eachInstructionValueOperand( @@ -482,20 +507,34 @@ class Visitor extends ReactiveFunctionVisitor { suggestions: null, }, ); + const reassignments = state.manualMemoState.reassignments; state.manualMemoState = null; if (!instruction.value.pruned) { for (const {identifier, loc} of eachInstructionValueOperand( instruction.value as InstructionValue, )) { - if (isUnmemoized(identifier, this.scopes)) { - state.errors.push({ - reason: - '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.', - description: null, - severity: ErrorSeverity.CannotPreserveMemoization, - loc, - suggestions: null, - }); + let manualMemoRvals; + if (identifier.scope == null) { + // If the manual memo was a useMemo that got inlined, iterate through + // all reassignments to the iife temporary to ensure they're memoized. + manualMemoRvals = reassignments.get(identifier.declarationId) ?? [ + identifier, + ]; + } else { + manualMemoRvals = [identifier]; + } + + for (const identifier of manualMemoRvals) { + if (isUnmemoized(identifier, this.scopes)) { + state.errors.push({ + reason: + '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.', + description: null, + severity: ErrorSeverity.CannotPreserveMemoization, + loc, + suggestions: null, + }); + } } } } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-maybe-mutable-ref-memo-not-preserved.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-maybe-mutable-ref-memo-not-preserved.expect.md deleted file mode 100644 index ce19a1dc4eac..000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-maybe-mutable-ref-memo-not-preserved.expect.md +++ /dev/null @@ -1,45 +0,0 @@ - -## Input - -```javascript -// @validatePreserveExistingMemoizationGuarantees:true - -import {useRef, useMemo} from 'react'; -import {makeArray} from 'shared-runtime'; - -function useFoo() { - const r = useRef(); - return useMemo(() => makeArray(r), []); -} - -export const FIXTURE_ENTRYPOINT = { - fn: useFoo, - params: [], -}; - -``` - -## Code - -```javascript -// @validatePreserveExistingMemoizationGuarantees:true - -import { useRef, useMemo } from "react"; -import { makeArray } from "shared-runtime"; - -function useFoo() { - const r = useRef(); - let t0; - t0 = makeArray(r); - return t0; -} - -export const FIXTURE_ENTRYPOINT = { - fn: useFoo, - params: [], -}; - -``` - -### Eval output -(kind: ok) [{}] \ 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-prune-nonescaping.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-prune-nonescaping.expect.md new file mode 100644 index 000000000000..103b91ff6a35 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-prune-nonescaping.expect.md @@ -0,0 +1,38 @@ + +## Input + +```javascript +// @validatePreserveExistingMemoizationGuarantees + +import {useMemo} from 'react'; +import {identity} from 'shared-runtime'; + +/** + * This is technically a false positive, although it makes sense + * to bailout as source code might be doing something sketchy. + */ +function useFoo(x) { + useMemo(() => identity(x), [x]); +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [2], +}; + +``` + + +## Error + +``` + 9 | */ + 10 | function useFoo(x) { +> 11 | useMemo(() => identity(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. (11:11) + 12 | } + 13 | + 14 | 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/error.false-positive-prune-nonescaping.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-prune-nonescaping.ts new file mode 100644 index 000000000000..95059470add6 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-prune-nonescaping.ts @@ -0,0 +1,17 @@ +// @validatePreserveExistingMemoizationGuarantees + +import {useMemo} from 'react'; +import {identity} from 'shared-runtime'; + +/** + * This is technically a false positive, although it makes sense + * to bailout as source code might be doing something sketchy. + */ +function useFoo(x) { + useMemo(() => identity(x), [x]); +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [2], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-useMemo-dropped-infer-always-invalidating.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-useMemo-dropped-infer-always-invalidating.expect.md new file mode 100644 index 000000000000..855058d25b68 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-useMemo-dropped-infer-always-invalidating.expect.md @@ -0,0 +1,42 @@ + +## Input + +```javascript +// @validatePreserveExistingMemoizationGuarantees + +import {useMemo} from 'react'; +import {useHook} from 'shared-runtime'; + +// useMemo values may not be memoized in Forget output if we +// infer that their deps always invalidate. +// This is technically a false positive as the useMemo in source +// was effectively a no-op +function useFoo(props) { + const x = []; + useHook(); + x.push(props); + + return useMemo(() => [x], [x]); +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{}], +}; + +``` + + +## Error + +``` + 13 | x.push(props); + 14 | +> 15 | return useMemo(() => [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/useMemo-dropped-infer-always-invalidating.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-useMemo-dropped-infer-always-invalidating.ts similarity index 81% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useMemo-dropped-infer-always-invalidating.ts rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-useMemo-dropped-infer-always-invalidating.ts index bb6f655a2c36..3265182c51df 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useMemo-dropped-infer-always-invalidating.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-useMemo-dropped-infer-always-invalidating.ts @@ -5,8 +5,8 @@ import {useHook} from 'shared-runtime'; // useMemo values may not be memoized in Forget output if we // infer that their deps always invalidate. -// This is still correct as the useMemo in source was effectively -// a no-op already. +// This is technically a false positive as the useMemo in source +// was effectively a no-op function useFoo(props) { const x = []; useHook(); diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.maybe-mutable-ref-not-preserved.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.maybe-mutable-ref-not-preserved.expect.md new file mode 100644 index 000000000000..61fe8b8deb7e --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.maybe-mutable-ref-not-preserved.expect.md @@ -0,0 +1,35 @@ + +## Input + +```javascript +// @validatePreserveExistingMemoizationGuarantees:true + +import {useRef, useMemo} from 'react'; +import {makeArray} from 'shared-runtime'; + +function useFoo() { + const r = useRef(); + return useMemo(() => makeArray(r), []); +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [], +}; + +``` + + +## Error + +``` + 6 | function useFoo() { + 7 | const r = useRef(); +> 8 | return useMemo(() => makeArray(r), []); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 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. (8:8) + 9 | } + 10 | + 11 | export const FIXTURE_ENTRYPOINT = { +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-maybe-mutable-ref-memo-not-preserved.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.maybe-mutable-ref-not-preserved.ts similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-maybe-mutable-ref-memo-not-preserved.ts rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.maybe-mutable-ref-not-preserved.ts diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useMemo-dropped-infer-always-invalidating.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useMemo-dropped-infer-always-invalidating.expect.md deleted file mode 100644 index f4e77ed2a778..000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useMemo-dropped-infer-always-invalidating.expect.md +++ /dev/null @@ -1,58 +0,0 @@ - -## Input - -```javascript -// @validatePreserveExistingMemoizationGuarantees - -import {useMemo} from 'react'; -import {useHook} from 'shared-runtime'; - -// useMemo values may not be memoized in Forget output if we -// infer that their deps always invalidate. -// This is still correct as the useMemo in source was effectively -// a no-op already. -function useFoo(props) { - const x = []; - useHook(); - x.push(props); - - return useMemo(() => [x], [x]); -} - -export const FIXTURE_ENTRYPOINT = { - fn: useFoo, - params: [{}], -}; - -``` - -## Code - -```javascript -// @validatePreserveExistingMemoizationGuarantees - -import { useMemo } from "react"; -import { useHook } from "shared-runtime"; - -// useMemo values may not be memoized in Forget output if we -// infer that their deps always invalidate. -// This is still correct as the useMemo in source was effectively -// a no-op already. -function useFoo(props) { - const x = []; - useHook(); - x.push(props); - let t0; - t0 = [x]; - return t0; -} - -export const FIXTURE_ENTRYPOINT = { - fn: useFoo, - params: [{}], -}; - -``` - -### Eval output -(kind: ok) [[{}]] \ No newline at end of file From fda840689b27ae6f7924704e43ac283ff7af2369 Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Mon, 5 Aug 2024 23:03:23 -0400 Subject: [PATCH 2/2] Update on "[compiler] Patch ValidatePreserveMemo to bailout correctly for refs" --- **Background** ValidatePreserveExistingMemo checks that useMemos are preserved by checking that the returned result of the existing `useMemo`/`useCallbacks` either: 1. is never assigned a reactive scope (e.g. inferred to be a global or primitive) 2. has an *unpruned* reactive scope We prune scopes for a few reasons. This is a bit overly conservative as there are valid reasons for pruning (see special-casing for `PruneAlwaysInvalidatingScopes`). Note that this bugfix actually exposes some of these. We can either special-case more pruning passes to be valid (potentially introducing `PrunedScope.reason`) or continue to be conservative to retain source semantics. **This PR** This PR fixes a bug around ValidatePreserveUseMemo + iife inlining. For useMemos that are successfully inlined, we previously always hit case 1: assuming that useMemo return values don't need reactive scopes due to type inference. When inlining useMemo, we convert the useMemo to an iife and inline it, synthesizing a temporary variable for recording the iife return value. ```js // source useMemo(() => foo(), ...); // hir $0 = Decl t$0 $1 = LoadGlobal "foo" $2 = CallExpression $1 $3 = Reassign t$0 = $2 $4 = FinishMemoize decl=t$0 // validation marker ``` `$2` gets assigned a reactive scope. But `t$0` is never mutated after the reassignment (note that this is impossible by construction -- `validatePreserveExistingMemo` would fail earlier in inferReferenceEffects in that case), so it does not get a scope. This PR patches `validatePreserveExistingMemo` to check through reassignments to the `FinishMemoize` value. Since FinishMemoize only ever records rvalues (the result of the useMemo / useCallback call itself), this should only affect inlined useMemos. Following the same example, we previously would check that `t$0` preserves existing memo semantics. Now, we check `$2` instead. TODO: test by syncing internally [ghstack-poisoned] --- .../ReactiveScopes/PruneNonEscapingScopes.ts | 49 +++++++++++++---- .../ValidatePreservedManualMemoization.ts | 28 +++++----- ...false-positive-prune-nonescaping.expect.md | 38 -------------- ...g-useMemo-mult-returns-primitive.expect.md | 52 +++++++++++++++++++ ...escaping-useMemo-mult-returns-primitive.ts | 19 +++++++ ...nonescaping-useMemo-mult-returns.expect.md | 52 +++++++++++++++++++ .../prune-nonescaping-useMemo-mult-returns.ts | 19 +++++++ .../prune-nonescaping-useMemo.expect.md | 50 ++++++++++++++++++ ...caping.ts => prune-nonescaping-useMemo.ts} | 0 9 files changed, 244 insertions(+), 63 deletions(-) delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-prune-nonescaping.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/prune-nonescaping-useMemo-mult-returns-primitive.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/prune-nonescaping-useMemo-mult-returns-primitive.ts create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/prune-nonescaping-useMemo-mult-returns.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/prune-nonescaping-useMemo-mult-returns.ts create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/prune-nonescaping-useMemo.expect.md rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/{error.false-positive-prune-nonescaping.ts => prune-nonescaping-useMemo.ts} (100%) diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts index 2951d5048530..b2e91fa30272 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts @@ -9,6 +9,7 @@ import {CompilerError} from '../CompilerError'; import { DeclarationId, Environment, + Identifier, InstructionId, Pattern, Place, @@ -24,7 +25,7 @@ import { isMutableEffect, } from '../HIR'; import {getFunctionCallSignature} from '../Inference/InferReferenceEffects'; -import {assertExhaustive} from '../Utils/utils'; +import {assertExhaustive, getOrInsertDefault} from '../Utils/utils'; import {getPlaceScope} from './BuildReactiveBlocks'; import { ReactiveFunctionTransform, @@ -935,6 +936,11 @@ class PruneScopesTransform extends ReactiveFunctionTransform< Set > { prunedScopes: Set = new Set(); + /** + * Track reassignments so we can correctly set `pruned` flags for + * inlined useMemos. + */ + reassignments: Map> = new Map(); override transformScope( scopeBlock: ReactiveScopeBlock, @@ -977,24 +983,45 @@ class PruneScopesTransform extends ReactiveFunctionTransform< } } + /** + * If we pruned the scope for a non-escaping value, we know it doesn't + * need to be memoized. Remove associated `Memoize` instructions so that + * we don't report false positives on "missing" memoization of these values. + */ override transformInstruction( instruction: ReactiveInstruction, state: Set, ): Transformed { this.traverseInstruction(instruction, state); - /** - * If we pruned the scope for a non-escaping value, we know it doesn't - * need to be memoized. Remove associated `Memoize` instructions so that - * we don't report false positives on "missing" memoization of these values. - */ - if (instruction.value.kind === 'FinishMemoize') { - const identifier = instruction.value.decl.identifier; + const value = instruction.value; + if (value.kind === 'StoreLocal' && value.lvalue.kind === 'Reassign') { + const ids = getOrInsertDefault( + this.reassignments, + value.lvalue.place.identifier.declarationId, + new Set(), + ); + ids.add(value.value.identifier); + } else if (value.kind === 'FinishMemoize') { + let decls; + if (value.decl.identifier.scope == null) { + /** + * If the manual memo was a useMemo that got inlined, iterate through + * all reassignments to the iife temporary to ensure they're memoized. + */ + decls = this.reassignments.get(value.decl.identifier.declarationId) ?? [ + value.decl.identifier, + ]; + } else { + decls = [value.decl.identifier]; + } + if ( - identifier.scope !== null && - this.prunedScopes.has(identifier.scope.id) + [...decls].every( + decl => decl.scope == null || this.prunedScopes.has(decl.scope.id), + ) ) { - instruction.value.pruned = true; + value.pruned = true; } } 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 31e15f78f8ce..67778956a162 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts @@ -457,10 +457,10 @@ class Visitor extends ReactiveFunctionVisitor { ); ids.add(value.value.identifier); } - if (instruction.value.kind === 'StartMemoize') { + if (value.kind === 'StartMemoize') { let depsFromSource: Array | null = null; - if (instruction.value.deps != null) { - depsFromSource = instruction.value.deps; + if (value.deps != null) { + depsFromSource = value.deps; } CompilerError.invariant(state.manualMemoState == null, { reason: 'Unexpected nested StartMemoize instructions', @@ -473,7 +473,7 @@ class Visitor extends ReactiveFunctionVisitor { loc: instruction.loc, decls: new Set(), depsFromSource, - manualMemoId: instruction.value.manualMemoId, + manualMemoId: value.manualMemoId, reassignments: new Map(), }; @@ -496,7 +496,7 @@ class Visitor extends ReactiveFunctionVisitor { } } } - if (instruction.value.kind === 'FinishMemoize') { + if (value.kind === 'FinishMemoize') { CompilerError.invariant( state.manualMemoState != null && state.manualMemoState.manualMemoId === instruction.value.manualMemoId, @@ -509,22 +509,22 @@ class Visitor extends ReactiveFunctionVisitor { ); const reassignments = state.manualMemoState.reassignments; state.manualMemoState = null; - if (!instruction.value.pruned) { + if (!value.pruned) { for (const {identifier, loc} of eachInstructionValueOperand( instruction.value as InstructionValue, )) { - let manualMemoRvals; + let decls; if (identifier.scope == null) { - // If the manual memo was a useMemo that got inlined, iterate through - // all reassignments to the iife temporary to ensure they're memoized. - manualMemoRvals = reassignments.get(identifier.declarationId) ?? [ - identifier, - ]; + /** + * If the manual memo was a useMemo that got inlined, iterate through + * all reassignments to the iife temporary to ensure they're memoized. + */ + decls = reassignments.get(identifier.declarationId) ?? [identifier]; } else { - manualMemoRvals = [identifier]; + decls = [identifier]; } - for (const identifier of manualMemoRvals) { + for (const identifier of decls) { if (isUnmemoized(identifier, this.scopes)) { state.errors.push({ reason: diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-prune-nonescaping.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-prune-nonescaping.expect.md deleted file mode 100644 index 103b91ff6a35..000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-prune-nonescaping.expect.md +++ /dev/null @@ -1,38 +0,0 @@ - -## Input - -```javascript -// @validatePreserveExistingMemoizationGuarantees - -import {useMemo} from 'react'; -import {identity} from 'shared-runtime'; - -/** - * This is technically a false positive, although it makes sense - * to bailout as source code might be doing something sketchy. - */ -function useFoo(x) { - useMemo(() => identity(x), [x]); -} - -export const FIXTURE_ENTRYPOINT = { - fn: useFoo, - params: [2], -}; - -``` - - -## Error - -``` - 9 | */ - 10 | function useFoo(x) { -> 11 | useMemo(() => identity(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. (11:11) - 12 | } - 13 | - 14 | 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/prune-nonescaping-useMemo-mult-returns-primitive.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/prune-nonescaping-useMemo-mult-returns-primitive.expect.md new file mode 100644 index 000000000000..8850f869d07b --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/prune-nonescaping-useMemo-mult-returns-primitive.expect.md @@ -0,0 +1,52 @@ + +## Input + +```javascript +// @validatePreserveExistingMemoizationGuarantees + +import {useMemo} from 'react'; +import {identity} from 'shared-runtime'; + +function useFoo(cond) { + useMemo(() => { + if (cond) { + return 2; + } else { + return identity(5); + } + }, [cond]); +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [true], +}; + +``` + +## Code + +```javascript +// @validatePreserveExistingMemoizationGuarantees + +import { useMemo } from "react"; +import { identity } from "shared-runtime"; + +function useFoo(cond) { + let t0; + if (cond) { + t0 = 2; + } else { + t0 = identity(5); + } +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [true], +}; + +``` + +### Eval output +(kind: ok) \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/prune-nonescaping-useMemo-mult-returns-primitive.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/prune-nonescaping-useMemo-mult-returns-primitive.ts new file mode 100644 index 000000000000..a79c84823ccc --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/prune-nonescaping-useMemo-mult-returns-primitive.ts @@ -0,0 +1,19 @@ +// @validatePreserveExistingMemoizationGuarantees + +import {useMemo} from 'react'; +import {identity} from 'shared-runtime'; + +function useFoo(cond) { + useMemo(() => { + if (cond) { + return 2; + } else { + return identity(5); + } + }, [cond]); +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [true], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/prune-nonescaping-useMemo-mult-returns.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/prune-nonescaping-useMemo-mult-returns.expect.md new file mode 100644 index 000000000000..af959b9865bc --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/prune-nonescaping-useMemo-mult-returns.expect.md @@ -0,0 +1,52 @@ + +## Input + +```javascript +// @validatePreserveExistingMemoizationGuarantees + +import {useMemo} from 'react'; +import {identity} from 'shared-runtime'; + +function useFoo(cond) { + useMemo(() => { + if (cond) { + return identity(10); + } else { + return identity(5); + } + }, [cond]); +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [true], +}; + +``` + +## Code + +```javascript +// @validatePreserveExistingMemoizationGuarantees + +import { useMemo } from "react"; +import { identity } from "shared-runtime"; + +function useFoo(cond) { + let t0; + if (cond) { + t0 = identity(10); + } else { + t0 = identity(5); + } +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [true], +}; + +``` + +### Eval output +(kind: ok) \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/prune-nonescaping-useMemo-mult-returns.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/prune-nonescaping-useMemo-mult-returns.ts new file mode 100644 index 000000000000..377293410611 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/prune-nonescaping-useMemo-mult-returns.ts @@ -0,0 +1,19 @@ +// @validatePreserveExistingMemoizationGuarantees + +import {useMemo} from 'react'; +import {identity} from 'shared-runtime'; + +function useFoo(cond) { + useMemo(() => { + if (cond) { + return identity(10); + } else { + return identity(5); + } + }, [cond]); +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [true], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/prune-nonescaping-useMemo.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/prune-nonescaping-useMemo.expect.md new file mode 100644 index 000000000000..27759f5db30f --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/prune-nonescaping-useMemo.expect.md @@ -0,0 +1,50 @@ + +## Input + +```javascript +// @validatePreserveExistingMemoizationGuarantees + +import {useMemo} from 'react'; +import {identity} from 'shared-runtime'; + +/** + * This is technically a false positive, although it makes sense + * to bailout as source code might be doing something sketchy. + */ +function useFoo(x) { + useMemo(() => identity(x), [x]); +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [2], +}; + +``` + +## Code + +```javascript +// @validatePreserveExistingMemoizationGuarantees + +import { useMemo } from "react"; +import { identity } from "shared-runtime"; + +/** + * This is technically a false positive, although it makes sense + * to bailout as source code might be doing something sketchy. + */ +function useFoo(x) { + let t0; + t0 = identity(x); +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [2], +}; + +``` + +### Eval output +(kind: ok) \ 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-prune-nonescaping.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/prune-nonescaping-useMemo.ts similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-prune-nonescaping.ts rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/prune-nonescaping-useMemo.ts