From 721234006c52a24516274c494f220c552e1191a4 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Wed, 28 Aug 2024 10:44:45 -0700 Subject: [PATCH] [compiler] Add DependencyPath optional property Adds an `optional: boolean` property to each token in a DependencyPath, currently always set to false. Also updates the equality and printing logic for paths to account for this field. Subsequent PRs will update our logic to determine which manual dependencies were optional, then we can start inferring optional deps as well. ghstack-source-id: 882ee8745e6a115b3dc80eaff183e2a0e27ad6f6 Pull Request resolved: https://github.com/facebook/react/pull/30813 --- .../src/HIR/HIR.ts | 7 ++-- .../src/HIR/PrintHIR.ts | 2 +- .../src/Inference/DropManualMemoization.ts | 3 +- .../DeriveMinimalDependencies.ts | 9 ++++-- .../PropagateScopeDependencies.ts | 2 +- .../ValidatePreservedManualMemoization.ts | 2 +- ...al-member-expression-as-memo-dep.expect.md | 32 +++++++++++++++++++ ...-optional-member-expression-as-memo-dep.js | 7 ++++ 8 files changed, 55 insertions(+), 9 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-optional-member-expression-as-memo-dep.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-optional-member-expression-as-memo-dep.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts index e74ec52203b..3bc7b135b6c 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts @@ -1500,10 +1500,13 @@ export type ReactiveScopeDependency = { export function areEqualPaths(a: DependencyPath, b: DependencyPath): boolean { return ( a.length === b.length && - a.every((item, ix) => item.property === b[ix].property) + a.every( + (item, ix) => + item.property === b[ix].property && item.optional === b[ix].optional, + ) ); } -export type DependencyPath = Array<{property: string}>; +export type DependencyPath = Array<{property: string; optional: boolean}>; /* * Simulated opaque type for BlockIds to prevent using normal numbers as block ids diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts index 8bcab1d7b25..ecf0b5f0c60 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts @@ -869,7 +869,7 @@ export function printManualMemoDependency( ? val.root.value.identifier.name.value : printIdentifier(val.root.value.identifier); } - return `${rootStr}${val.path.length > 0 ? '.' : ''}${val.path.join('.')}`; + return `${rootStr}${val.path.map(v => `${v.optional ? '?.' : '.'}${v.property}`).join('')}`; } export function printType(type: Type): string { if (type.kind === 'Type') return ''; diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts index e60d8a99145..c580b3e8b75 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts @@ -68,7 +68,8 @@ export function collectMaybeMemoDependencies( if (object != null) { return { root: object.root, - path: [...object.path, {property: value.property}], + // TODO: determine if the access is optional + path: [...object.path, {property: value.property, optional: false}], }; } break; diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/DeriveMinimalDependencies.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/DeriveMinimalDependencies.ts index 13420ee140e..dcf67a36e5c 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/DeriveMinimalDependencies.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/DeriveMinimalDependencies.ts @@ -6,7 +6,7 @@ */ import {CompilerError} from '../CompilerError'; -import {Identifier, ReactiveScopeDependency} from '../HIR'; +import {DependencyPath, Identifier, ReactiveScopeDependency} from '../HIR'; import {printIdentifier} from '../HIR/PrintHIR'; import {assertExhaustive} from '../Utils/utils'; @@ -252,7 +252,7 @@ type DependencyNode = { }; type ReduceResultNode = { - relativePath: Array<{property: string}>; + relativePath: DependencyPath; accessType: PropertyAccessType; }; @@ -283,7 +283,10 @@ function deriveMinimalDependenciesInSubtree( const childResult = deriveMinimalDependenciesInSubtree(childNode).map( ({relativePath, accessType}) => { return { - relativePath: [{property: childName}, ...relativePath], + relativePath: [ + {property: childName, optional: false}, + ...relativePath, + ], accessType, }; }, diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PropagateScopeDependencies.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PropagateScopeDependencies.ts index 0e47a8dcedf..8fd324ae2c9 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PropagateScopeDependencies.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PropagateScopeDependencies.ts @@ -485,7 +485,7 @@ class Context { }; } - objectDependency.path.push({property}); + objectDependency.path.push({property, optional: false}); return objectDependency; } 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 43a477f3418..5a9a947d88b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts @@ -116,7 +116,7 @@ function prettyPrintScopeDependency(val: ReactiveScopeDependency): string { } else { rootStr = '[unnamed]'; } - return `${rootStr}${val.path.length > 0 ? '.' : ''}${val.path.join('.')}`; + return `${rootStr}${val.path.map(v => `${v.optional ? '?.' : '.'}${v.property}`).join('')}`; } enum CompareDependencyResult { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-optional-member-expression-as-memo-dep.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-optional-member-expression-as-memo-dep.expect.md new file mode 100644 index 00000000000..7e4145a27c1 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-optional-member-expression-as-memo-dep.expect.md @@ -0,0 +1,32 @@ + +## Input + +```javascript +// @validatePreserveExistingMemoizationGuarantees +function Component(props) { + const data = useMemo(() => { + return props.items?.edges?.nodes ?? []; + }, [props.items?.edges?.nodes]); + return ; +} + +``` + + +## Error + +``` + 1 | // @validatePreserveExistingMemoizationGuarantees + 2 | function Component(props) { +> 3 | const data = useMemo(() => { + | ^^^^^^^ +> 4 | return props.items?.edges?.nodes ?? []; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +> 5 | }, [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:5) + 6 | return ; + 7 | } + 8 | +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-optional-member-expression-as-memo-dep.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-optional-member-expression-as-memo-dep.js new file mode 100644 index 00000000000..fd8cf0214c8 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-optional-member-expression-as-memo-dep.js @@ -0,0 +1,7 @@ +// @validatePreserveExistingMemoizationGuarantees +function Component(props) { + const data = useMemo(() => { + return props.items?.edges?.nodes ?? []; + }, [props.items?.edges?.nodes]); + return ; +}