From b4eca08cffd8a64a57129dfd5af4029c018d0841 Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Mon, 5 Aug 2024 22:20:45 -0400 Subject: [PATCH 1/2] [compiler][be] Less ambiguous error messages for validateMemo bailout [ghstack-poisoned] --- .../ValidatePreservedManualMemoization.ts | 57 +++++++++++-------- ...ack-captured-in-context-variable.expect.md | 2 +- ...ia-function-preserve-memoization.expect.md | 2 +- ...ified-later-preserve-memoization.expect.md | 2 +- ...sitive-useMemo-infer-mutate-deps.expect.md | 10 +--- ...back-captures-reassigned-context.expect.md | 4 +- ...back-captures-invalidating-value.expect.md | 2 +- 7 files changed, 42 insertions(+), 37 deletions(-) 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 f0009601f1d..29759e9e7b5 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts @@ -451,6 +451,25 @@ class Visitor extends ReactiveFunctionVisitor { depsFromSource, manualMemoId: instruction.value.manualMemoId, }; + + for (const {identifier, loc} of eachInstructionValueOperand( + instruction.value as InstructionValue, + )) { + if ( + identifier.scope != null && + !this.scopes.has(identifier.scope.id) && + !this.prunedScopes.has(identifier.scope.id) + ) { + state.errors.push({ + reason: + 'React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. This dependency may be mutated later, which could cause the value to change unexpectedly', + description: null, + severity: ErrorSeverity.CannotPreserveMemoization, + loc, + suggestions: null, + }); + } + } } if (instruction.value.kind === 'FinishMemoize') { CompilerError.invariant( @@ -464,30 +483,20 @@ class Visitor extends ReactiveFunctionVisitor { }, ); state.manualMemoState = null; - } - - const isDep = instruction.value.kind === 'StartMemoize'; - const isDecl = - instruction.value.kind === 'FinishMemoize' && !instruction.value.pruned; - if (isDep || isDecl) { - for (const value of eachInstructionValueOperand( - instruction.value as InstructionValue, - )) { - if ( - (isDep && - value.identifier.scope != null && - !this.scopes.has(value.identifier.scope.id) && - !this.prunedScopes.has(value.identifier.scope.id)) || - (isDecl && isUnmemoized(value.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 may be mutated later, which could cause the value to change unexpectedly', - description: null, - severity: ErrorSeverity.CannotPreserveMemoization, - loc: typeof instruction.loc !== 'symbol' ? instruction.loc : null, - suggestions: 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, + }); + } } } } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-repro-unmemoized-callback-captured-in-context-variable.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-repro-unmemoized-callback-captured-in-context-variable.expect.md index 8979a7642ff..30eb3ee27e6 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-repro-unmemoized-callback-captured-in-context-variable.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-repro-unmemoized-callback-captured-in-context-variable.expect.md @@ -53,7 +53,7 @@ export const FIXTURE_ENTRYPOINT = { 9 | const a = useHook(); 10 | // Because b is also part of that same mutable range, it can't be memoized either > 11 | const b = useMemo(() => ({}), []); - | ^^^^^^^^^^ CannotPreserveMemoization: React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. This value may be mutated later, which could cause the value to change unexpectedly (11:11) + | ^^^^^^^^^^^^^^^^^^^^^^^ 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 | // Conditional assignment without a subsequent mutation normally doesn't create a mutable 14 | // range, but in this case we're reassigning a context variable diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-useCallback-accesses-ref-mutated-later-via-function-preserve-memoization.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-useCallback-accesses-ref-mutated-later-via-function-preserve-memoization.expect.md index 813c5257667..ad21b4ba8c5 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-useCallback-accesses-ref-mutated-later-via-function-preserve-memoization.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-useCallback-accesses-ref-mutated-later-via-function-preserve-memoization.expect.md @@ -45,7 +45,7 @@ export const FIXTURE_ENTRYPOINT = { > 10 | ref.current.inner = event.target.value; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > 11 | }); - | ^^^^ CannotPreserveMemoization: React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. This value may be mutated later, which could cause the value to change unexpectedly (7:11) + | ^^^^ 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. (7:11) 12 | 13 | // The ref is modified later, extending its range and preventing memoization of onChange 14 | const reset = () => { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-useCallback-set-ref-nested-property-ref-modified-later-preserve-memoization.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-useCallback-set-ref-nested-property-ref-modified-later-preserve-memoization.expect.md index c92fd6c0f0d..5eeb8c6b0ab 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-useCallback-set-ref-nested-property-ref-modified-later-preserve-memoization.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-useCallback-set-ref-nested-property-ref-modified-later-preserve-memoization.expect.md @@ -42,7 +42,7 @@ export const FIXTURE_ENTRYPOINT = { > 10 | ref.current.inner = event.target.value; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > 11 | }); - | ^^^^ CannotPreserveMemoization: React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. This value may be mutated later, which could cause the value to change unexpectedly (7:11) + | ^^^^ 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. (7:11) 12 | 13 | // The ref is modified later, extending its range and preventing memoization of onChange 14 | ref.current.inner = null; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-useMemo-infer-mutate-deps.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-useMemo-infer-mutate-deps.expect.md index 1ac98d9c309..564796f18d4 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-useMemo-infer-mutate-deps.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-useMemo-infer-mutate-deps.expect.md @@ -29,14 +29,10 @@ export const FIXTURE_ENTRYPOINT = { ## Error ``` - 10 | const val = [1, 2, 3]; - 11 | -> 12 | return useMemo(() => { - | ^^^^^^^ -> 13 | return identity(val); - | ^^^^^^^^^^^^^^^^^^^^^^^^^ + 12 | return useMemo(() => { + 13 | return identity(val); > 14 | }, [val]); - | ^^^^ CannotPreserveMemoization: React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. This value may be mutated later, which could cause the value to change unexpectedly (12:14) + | ^^^ CannotPreserveMemoization: React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. This dependency may be mutated later, which could cause the value to change unexpectedly (14:14) 15 | } 16 | 17 | export const FIXTURE_ENTRYPOINT = { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.invalid-useCallback-captures-reassigned-context.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.invalid-useCallback-captures-reassigned-context.expect.md index 073a265e7e0..cfa2999835f 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.invalid-useCallback-captures-reassigned-context.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.invalid-useCallback-captures-reassigned-context.expect.md @@ -33,9 +33,9 @@ export const FIXTURE_ENTRYPOINT = { 10 | 11 | // makeArray() is captured, but depsList contains [props] > 12 | const cb = useCallback(() => [x], [x]); - | ^^^^^^^^^ CannotPreserveMemoization: React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. This value may be mutated later, which could cause the value to change unexpectedly (12:12) + | ^ CannotPreserveMemoization: React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. This dependency may be mutated later, which could cause the value to change unexpectedly (12:12) -CannotPreserveMemoization: React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. This value may be mutated later, which could cause the value to change unexpectedly (12:12) +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. (12:12) 13 | 14 | x = makeArray(); 15 | diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.todo-useCallback-captures-invalidating-value.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.todo-useCallback-captures-invalidating-value.expect.md index fef849d3413..1f26e03b836 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.todo-useCallback-captures-invalidating-value.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.todo-useCallback-captures-invalidating-value.expect.md @@ -31,7 +31,7 @@ export const FIXTURE_ENTRYPOINT = { 11 | x.push(props); 12 | > 13 | return useCallback(() => [x], [x]); - | ^^^^^^^^^ CannotPreserveMemoization: React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. This value may be mutated later, which could cause the value to change unexpectedly (13:13) + | ^^^^^^^^^ 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. (13:13) 14 | } 15 | 16 | export const FIXTURE_ENTRYPOINT = { From 27aa77b30c31ee7e2410915c25e99a0862f0389f Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Mon, 5 Aug 2024 23:14:48 -0400 Subject: [PATCH 2/2] Update on "[compiler][be] Less ambiguous error messages for validateMemo bailout" [ghstack-poisoned] --- .../ValidatePreservedManualMemoization.ts | 27 ++++++++++--------- 1 file changed, 14 insertions(+), 13 deletions(-) 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 29759e9e7b5..20448607090 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts @@ -433,15 +433,16 @@ class Visitor extends ReactiveFunctionVisitor { * recursively visits ReactiveValues and instructions */ this.recordTemporaries(instruction, state); - if (instruction.value.kind === 'StartMemoize') { + const value = instruction.value; + 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', - description: `Bad manual memoization ids: ${state.manualMemoState?.manualMemoId}, ${instruction.value.manualMemoId}`, - loc: instruction.value.loc, + description: `Bad manual memoization ids: ${state.manualMemoState?.manualMemoId}, ${value.manualMemoId}`, + loc: value.loc, suggestions: null, }); @@ -449,11 +450,11 @@ class Visitor extends ReactiveFunctionVisitor { loc: instruction.loc, decls: new Set(), depsFromSource, - manualMemoId: instruction.value.manualMemoId, + manualMemoId: value.manualMemoId, }; for (const {identifier, loc} of eachInstructionValueOperand( - instruction.value as InstructionValue, + value as InstructionValue, )) { if ( identifier.scope != null && @@ -471,21 +472,21 @@ class Visitor extends ReactiveFunctionVisitor { } } } - if (instruction.value.kind === 'FinishMemoize') { + if (value.kind === 'FinishMemoize') { CompilerError.invariant( state.manualMemoState != null && - state.manualMemoState.manualMemoId === instruction.value.manualMemoId, + state.manualMemoState.manualMemoId === value.manualMemoId, { reason: 'Unexpected mismatch between StartMemoize and FinishMemoize', - description: `Encountered StartMemoize id=${state.manualMemoState?.manualMemoId} followed by FinishMemoize id=${instruction.value.manualMemoId}`, - loc: instruction.value.loc, + description: `Encountered StartMemoize id=${state.manualMemoState?.manualMemoId} followed by FinishMemoize id=${value.manualMemoId}`, + loc: value.loc, suggestions: null, }, ); state.manualMemoState = null; - if (!instruction.value.pruned) { + if (!value.pruned) { for (const {identifier, loc} of eachInstructionValueOperand( - instruction.value as InstructionValue, + value as InstructionValue, )) { if (isUnmemoized(identifier, this.scopes)) { state.errors.push({