From d3ed495294ef5ae3bc13fc98ec452250b2f7c4e6 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Thu, 22 Aug 2024 17:35:03 -0700 Subject: [PATCH 1/3] [compiler] Special-case phi inference for mixed readonly type This allows us to handle common operations such as `useFragment(...).edges.nodes ?? []` where we have a `Phi(MixedReadonly, Array)`. The underlying pattern remains general-purpose and not Relay-specific, and any API that returns transitively "mixed" data (primitives, arrays, plain objects) can benefit from the same type refinement. [ghstack-poisoned] --- .../src/TypeInference/InferTypes.ts | 22 +++- ...ng-memoization-lack-of-phi-types.expect.md | 42 ------- ...ng-memoization-lack-of-phi-types.expect.md | 105 ++++++++++++++++++ ...-missing-memoization-lack-of-phi-types.js} | 0 4 files changed, 125 insertions(+), 44 deletions(-) delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-repro-missing-memoization-lack-of-phi-types.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-missing-memoization-lack-of-phi-types.expect.md rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{error.todo-repro-missing-memoization-lack-of-phi-types.js => repro-missing-memoization-lack-of-phi-types.js} (100%) diff --git a/compiler/packages/babel-plugin-react-compiler/src/TypeInference/InferTypes.ts b/compiler/packages/babel-plugin-react-compiler/src/TypeInference/InferTypes.ts index 6a20a278bee7f..911399c9323ec 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/TypeInference/InferTypes.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/TypeInference/InferTypes.ts @@ -14,7 +14,9 @@ import { Identifier, IdentifierId, Instruction, + isArrayType, makeType, + ObjectType, PropType, Type, typeEquals, @@ -25,6 +27,7 @@ import { BuiltInArrayId, BuiltInFunctionId, BuiltInJsxId, + BuiltInMixedReadonlyId, BuiltInObjectId, BuiltInPropsId, BuiltInRefValueId, @@ -496,8 +499,23 @@ class Unifier { if (candidateType === null) { candidateType = resolved; } else if (!typeEquals(resolved, candidateType)) { - candidateType = null; - break; + function isArrayOrMixedReadonly(ty: Type): ty is ObjectType { + return ( + ty.kind === 'Object' && + (ty.shapeId === BuiltInArrayId || + ty.shapeId === BuiltInMixedReadonlyId) + ); + } + if ( + isArrayOrMixedReadonly(resolved) && + isArrayOrMixedReadonly(candidateType) + ) { + candidateType = + resolved.shapeId === BuiltInArrayId ? resolved : candidateType; + } else { + candidateType = null; + break; + } } // else same type, continue } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-repro-missing-memoization-lack-of-phi-types.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-repro-missing-memoization-lack-of-phi-types.expect.md deleted file mode 100644 index d70b36133fc39..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-repro-missing-memoization-lack-of-phi-types.expect.md +++ /dev/null @@ -1,42 +0,0 @@ - -## Input - -```javascript -// @flow @validatePreserveExistingMemoizationGuarantees -import {useMemo} from 'react'; -import {useFragment} from 'shared-runtime'; - -function Component() { - const data = useFragment(); - const nodes = data.nodes ?? []; - const flatMap = nodes.flatMap(node => node.items); - const filtered = flatMap.filter(item => item != null); - const map = useMemo(() => filtered.map(), [filtered]); - const index = filtered.findIndex(x => x === null); - - return ( -
- {map} - {index} -
- ); -} - -``` - - -## Error - -``` - 8 | const flatMap = nodes.flatMap(node => node.items); - 9 | const filtered = flatMap.filter(item => item != null); -> 10 | const map = useMemo(() => filtered.map(), [filtered]); - | ^^^^^^^^ 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 (10:10) - -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. (10:10) - 11 | const index = filtered.findIndex(x => x === null); - 12 | - 13 | return ( -``` - - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-missing-memoization-lack-of-phi-types.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-missing-memoization-lack-of-phi-types.expect.md new file mode 100644 index 0000000000000..80d046ec1807a --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-missing-memoization-lack-of-phi-types.expect.md @@ -0,0 +1,105 @@ + +## Input + +```javascript +// @flow @validatePreserveExistingMemoizationGuarantees +import {useMemo} from 'react'; +import {useFragment} from 'shared-runtime'; + +function Component() { + const data = useFragment(); + const nodes = data.nodes ?? []; + const flatMap = nodes.flatMap(node => node.items); + const filtered = flatMap.filter(item => item != null); + const map = useMemo(() => filtered.map(), [filtered]); + const index = filtered.findIndex(x => x === null); + + return ( +
+ {map} + {index} +
+ ); +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { useMemo } from "react"; +import { useFragment } from "shared-runtime"; + +function Component() { + const $ = _c(11); + const data = useFragment(); + let t0; + if ($[0] !== data.nodes) { + t0 = data.nodes ?? []; + $[0] = data.nodes; + $[1] = t0; + } else { + t0 = $[1]; + } + const nodes = t0; + let t1; + if ($[2] !== nodes) { + t1 = nodes.flatMap(_temp); + $[2] = nodes; + $[3] = t1; + } else { + t1 = $[3]; + } + const flatMap = t1; + let t2; + if ($[4] !== flatMap) { + t2 = flatMap.filter(_temp2); + $[4] = flatMap; + $[5] = t2; + } else { + t2 = $[5]; + } + const filtered = t2; + let t3; + let t4; + if ($[6] !== filtered) { + t4 = filtered.map(); + $[6] = filtered; + $[7] = t4; + } else { + t4 = $[7]; + } + t3 = t4; + const map = t3; + const index = filtered.findIndex(_temp3); + let t5; + if ($[8] !== map || $[9] !== index) { + t5 = ( +
+ {map} + {index} +
+ ); + $[8] = map; + $[9] = index; + $[10] = t5; + } else { + t5 = $[10]; + } + return t5; +} +function _temp3(x) { + return x === null; +} +function _temp2(item) { + return item != null; +} +function _temp(node) { + return node.items; +} + +``` + +### Eval output +(kind: exception) Fixture not implemented \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-repro-missing-memoization-lack-of-phi-types.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-missing-memoization-lack-of-phi-types.js similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-repro-missing-memoization-lack-of-phi-types.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-missing-memoization-lack-of-phi-types.js From e291fc3f71718bfdb73744e69cd232d6c9c2e5c1 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Thu, 22 Aug 2024 17:44:14 -0700 Subject: [PATCH 2/3] Update on "[compiler] Special-case phi inference for mixed readonly type" This allows us to handle common operations such as `useFragment(...).edges.nodes ?? []` where we have a `Phi(MixedReadonly, Array)`. The underlying pattern remains general-purpose and not Relay-specific, and any API that returns transitively "mixed" data (primitives, arrays, plain objects) can benefit from the same type refinement. [ghstack-poisoned] --- .../src/TypeInference/InferTypes.ts | 54 ++++++++++++++----- 1 file changed, 40 insertions(+), 14 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/TypeInference/InferTypes.ts b/compiler/packages/babel-plugin-react-compiler/src/TypeInference/InferTypes.ts index 911399c9323ec..2bbbf6c07b1cf 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/TypeInference/InferTypes.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/TypeInference/InferTypes.ts @@ -499,22 +499,12 @@ class Unifier { if (candidateType === null) { candidateType = resolved; } else if (!typeEquals(resolved, candidateType)) { - function isArrayOrMixedReadonly(ty: Type): ty is ObjectType { - return ( - ty.kind === 'Object' && - (ty.shapeId === BuiltInArrayId || - ty.shapeId === BuiltInMixedReadonlyId) - ); - } - if ( - isArrayOrMixedReadonly(resolved) && - isArrayOrMixedReadonly(candidateType) - ) { - candidateType = - resolved.shapeId === BuiltInArrayId ? resolved : candidateType; - } else { + const unionType = tryUnionTypes(resolved, candidateType); + if (unionType === null) { candidateType = null; break; + } else { + candidateType = unionType; } } // else same type, continue } @@ -668,3 +658,39 @@ const RefLikeNameRE = /^(?:[a-zA-Z$_][a-zA-Z$_0-9]*)Ref$|^ref$/; function isRefLikeName(t: PropType): boolean { return RefLikeNameRE.test(t.objectName) && t.propertyName === 'current'; } + +function tryUnionTypes(ty1: Type, ty2: Type): Type | null { + let readonlyType: Type; + let otherType: Type; + if (ty1.kind === 'Object' && ty1.shapeId === BuiltInMixedReadonlyId) { + readonlyType = ty1; + otherType = ty2; + } else if (ty2.kind === 'Object' && ty2.shapeId === BuiltInMixedReadonlyId) { + readonlyType = ty2; + otherType = ty1; + } else { + return null; + } + if (otherType.kind === 'Primitive') { + /** + * Union(Primitive | MixedReadonly) = MixedReadonly + * + * For example, `data ?? null` could return `data`, the fact that RHS + * is a primitive doesn't guarantee the result is a primitive. + */ + return readonlyType; + } else if ( + otherType.kind === 'Object' && + otherType.shapeId === BuiltInArrayId + ) { + /** + * Union(Array | MixedReadonly) = Array + * + * In practice this pattern means the result is always an array. Given + * that this behavior requires opting-in to the mixedreadonly type + * (via moduleTypeProvider) this seems like a reasonable heuristic. + */ + return otherType; + } + return null; +} From 08adeb3e5af92b4dab5deac113053f6be65f6ae3 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Thu, 22 Aug 2024 17:51:22 -0700 Subject: [PATCH 3/3] Update on "[compiler] Special-case phi inference for mixed readonly type" This allows us to handle common operations such as `useFragment(...).edges.nodes ?? []` where we have a `Phi(MixedReadonly, Array)`. The underlying pattern remains general-purpose and not Relay-specific, and any API that returns transitively "mixed" data (primitives, arrays, plain objects) can benefit from the same type refinement. [ghstack-poisoned] --- .../babel-plugin-react-compiler/src/TypeInference/InferTypes.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/TypeInference/InferTypes.ts b/compiler/packages/babel-plugin-react-compiler/src/TypeInference/InferTypes.ts index 2bbbf6c07b1cf..d9f7ffd5bf8b8 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/TypeInference/InferTypes.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/TypeInference/InferTypes.ts @@ -14,9 +14,7 @@ import { Identifier, IdentifierId, Instruction, - isArrayType, makeType, - ObjectType, PropType, Type, typeEquals,