From 9b27356fa74c703cb5bcbd255c2fda702d154125 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Mon, 26 Aug 2024 12:20:15 -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-poisoned] --- .../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 e74ec52203bab..3bc7b135b6ced 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 8bcab1d7b25ae..ecf0b5f0c6041 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 e60d8a9914583..c580b3e8b759f 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 13420ee140e63..dcf67a36e5c6b 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 0e47a8dcedfb2..8fd324ae2c9aa 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 43a477f3418e6..5a9a947d88bfd 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 0000000000000..7e4145a27c16b --- /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 0000000000000..fd8cf0214c87b --- /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 ; +}