Skip to content

Commit

Permalink
[compiler] Add DependencyPath optional property
Browse files Browse the repository at this point in the history
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: facebook/react#30813
  • Loading branch information
josephsavona committed Aug 28, 2024
1 parent e9356cc commit 7212340
Show file tree
Hide file tree
Showing 8 changed files with 55 additions and 9 deletions.
7 changes: 5 additions & 2 deletions compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 '';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -252,7 +252,7 @@ type DependencyNode = {
};

type ReduceResultNode = {
relativePath: Array<{property: string}>;
relativePath: DependencyPath;
accessType: PropertyAccessType;
};

Expand Down Expand Up @@ -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,
};
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -485,7 +485,7 @@ class Context {
};
}

objectDependency.path.push({property});
objectDependency.path.push({property, optional: false});

return objectDependency;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@

## Input

```javascript
// @validatePreserveExistingMemoizationGuarantees
function Component(props) {
const data = useMemo(() => {
return props.items?.edges?.nodes ?? [];
}, [props.items?.edges?.nodes]);
return <Foo data={data} />;
}

```
## 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 <Foo data={data} />;
7 | }
8 |
```
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// @validatePreserveExistingMemoizationGuarantees
function Component(props) {
const data = useMemo(() => {
return props.items?.edges?.nodes ?? [];
}, [props.items?.edges?.nodes]);
return <Foo data={data} />;
}

0 comments on commit 7212340

Please sign in to comment.