From 7ff06dd2973251673219abdb365383102d69cea3 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Fri, 2 May 2025 13:30:34 +0900 Subject: [PATCH 1/2] [compiler] ValidatePreservedManualMemoization reports detailed errors This pass didn't previously report the precise difference btw inferred/manual dependencies unless a debug flag was set. But the error message is really good (nice job @mofeiz): the only catch is that in theory the inferred dep could be a temporary that can't trivially be reported to the user. But the messages are really useful for quickly verifying why the compiler couldn't preserve memoization. So here we switch to outputting a detailed message about the discrepancy btw inferred/manual deps so long as the inferred dep root is a named variable. I also slightly adjusted the message to handle the case where there is no diagnostic, which can occur if there were no manual deps but the compiler inferred a dependency. [ghstack-poisoned] --- .../ValidatePreservedManualMemoization.ts | 34 +++++++++++-------- ...ession-with-conditional-optional.expect.md | 2 +- ...mber-expression-with-conditional.expect.md | 2 +- ...as-memo-dep-non-optional-in-body.expect.md | 2 +- .../error.ref-like-name-not-Ref.expect.md | 2 +- .../error.ref-like-name-not-a-ref.expect.md | 2 +- ...ack-conditional-access-own-scope.expect.md | 2 +- ...ck-infer-conditional-value-block.expect.md | 4 +-- ...nvalid-useCallback-read-maybeRef.expect.md | 2 +- ...be-invalid-useMemo-read-maybeRef.expect.md | 2 +- ...ve-use-memo-ref-missing-reactive.expect.md | 2 +- .../error.useCallback-aliased-var.expect.md | 2 +- ...lback-conditional-access-noAlloc.expect.md | 2 +- ...less-specific-conditional-access.expect.md | 2 +- ...or.useCallback-property-call-dep.expect.md | 2 +- .../error.useMemo-aliased-var.expect.md | 2 +- ...less-specific-conditional-access.expect.md | 2 +- ...specific-conditional-value-block.expect.md | 4 +-- ...emo-property-call-chained-object.expect.md | 2 +- .../error.useMemo-property-call-dep.expect.md | 2 +- ...o-unrelated-mutation-in-depslist.expect.md | 2 +- ...ession-with-conditional-optional.expect.md | 2 +- ...mber-expression-with-conditional.expect.md | 2 +- 23 files changed, 43 insertions(+), 39 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 85fec7a75333b..943c74efdcd9e 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts @@ -5,6 +5,7 @@ * LICENSE file in the root directory of this source tree. */ +import prettyFormat from 'pretty-format'; import {CompilerError, ErrorSeverity} from '../CompilerError'; import { DeclarationId, @@ -141,14 +142,14 @@ function getCompareDependencyResultDescription( ): string { switch (result) { case CompareDependencyResult.Ok: - return 'dependencies equal'; + return 'Dependencies equal'; case CompareDependencyResult.RootDifference: case CompareDependencyResult.PathDifference: - return 'inferred different dependency than source'; + return 'Inferred different dependency than source'; case CompareDependencyResult.RefAccessDifference: - return 'differences in ref.current access'; + return 'Differences in ref.current access'; case CompareDependencyResult.Subpath: - return 'inferred less specific property than source'; + return 'Inferred less specific property than source'; } } @@ -279,17 +280,20 @@ function validateInferredDep( severity: ErrorSeverity.CannotPreserveMemoization, reason: 'React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. The inferred dependencies did not match the manually specified dependencies, which could cause the value to change more or less frequently than expected', - description: DEBUG - ? `The inferred dependency was \`${prettyPrintScopeDependency( - dep, - )}\`, but the source dependencies were [${validDepsInMemoBlock - .map(dep => printManualMemoDependency(dep, true)) - .join(', ')}]. Detail: ${ - errorDiagnostic - ? getCompareDependencyResultDescription(errorDiagnostic) - : 'none' - }` - : null, + description: + DEBUG || + // If the dependency is a named variable then we can report it. Otherwise only print in debug mode + (dep.identifier.name != null && dep.identifier.name.kind === 'named') + ? `The inferred dependency was \`${prettyPrintScopeDependency( + dep, + )}\`, but the source dependencies were [${validDepsInMemoBlock + .map(dep => printManualMemoDependency(dep, true)) + .join(', ')}]. ${ + errorDiagnostic + ? getCompareDependencyResultDescription(errorDiagnostic) + : 'Inferred dependency not present in source' + }` + : null, loc: memoLocation, suggestions: null, }); diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hoist-optional-member-expression-with-conditional-optional.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hoist-optional-member-expression-with-conditional-optional.expect.md index d9c2b599998b7..048fee7ee1d58 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hoist-optional-member-expression-with-conditional-optional.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hoist-optional-member-expression-with-conditional-optional.expect.md @@ -41,7 +41,7 @@ function Component(props) { > 10 | return x; | ^^^^^^^^^^^^^^^^^ > 11 | }, [props?.items, props.cond]); - | ^^^^ CannotPreserveMemoization: React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. The inferred dependencies did not match the manually specified dependencies, which could cause the value to change more or less frequently than expected (4:11) + | ^^^^ CannotPreserveMemoization: React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. The inferred dependencies did not match the manually specified dependencies, which could cause the value to change more or less frequently than expected. The inferred dependency was `props.items`, but the source dependencies were [props?.items, props.cond]. Inferred different dependency than source (4:11) 12 | return ( 13 | 14 | ); diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hoist-optional-member-expression-with-conditional.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hoist-optional-member-expression-with-conditional.expect.md index 57b7d48facbd5..ca3ee2ae1388e 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hoist-optional-member-expression-with-conditional.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hoist-optional-member-expression-with-conditional.expect.md @@ -41,7 +41,7 @@ function Component(props) { > 10 | return x; | ^^^^^^^^^^^^^^^^^ > 11 | }, [props?.items, props.cond]); - | ^^^^ CannotPreserveMemoization: React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. The inferred dependencies did not match the manually specified dependencies, which could cause the value to change more or less frequently than expected (4:11) + | ^^^^ CannotPreserveMemoization: React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. The inferred dependencies did not match the manually specified dependencies, which could cause the value to change more or less frequently than expected. The inferred dependency was `props.items`, but the source dependencies were [props?.items, props.cond]. Inferred different dependency than source (4:11) 12 | return ( 13 | 14 | ); diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-optional-member-expression-as-memo-dep-non-optional-in-body.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-optional-member-expression-as-memo-dep-non-optional-in-body.expect.md index ba5b30418069a..7d6c4b38a7a4b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-optional-member-expression-as-memo-dep-non-optional-in-body.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-optional-member-expression-as-memo-dep-non-optional-in-body.expect.md @@ -29,7 +29,7 @@ function Component(props) { > 6 | // deps are optional | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > 7 | }, [props.items?.edges?.nodes]); - | ^^^^ CannotPreserveMemoization: React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. The inferred dependencies did not match the manually specified dependencies, which could cause the value to change more or less frequently than expected (3:7) + | ^^^^ CannotPreserveMemoization: React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. The inferred dependencies did not match the manually specified dependencies, which could cause the value to change more or less frequently than expected. The inferred dependency was `props.items.edges.nodes`, but the source dependencies were [props.items?.edges?.nodes]. Inferred different dependency than source (3:7) 8 | return ; 9 | } 10 | diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.ref-like-name-not-Ref.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.ref-like-name-not-Ref.expect.md index cf15ab13d12e1..2b4943ba491b4 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.ref-like-name-not-Ref.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.ref-like-name-not-Ref.expect.md @@ -38,7 +38,7 @@ export const FIXTURE_ENTRYPOINT = { > 12 | Ref.current?.click(); | ^^^^^^^^^^^^^^^^^^^^^^^^^ > 13 | }, []); - | ^^^^ CannotPreserveMemoization: React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. The inferred dependencies did not match the manually specified dependencies, which could cause the value to change more or less frequently than expected (11:13) + | ^^^^ CannotPreserveMemoization: React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. The inferred dependencies did not match the manually specified dependencies, which could cause the value to change more or less frequently than expected. The inferred dependency was `Ref.current`, but the source dependencies were []. Inferred dependency not present in source (11:13) 14 | 15 | return