From 99c056abb0dac0e1a15b2c85b620b72c625e065b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Tue, 1 Oct 2024 11:28:51 -0700 Subject: [PATCH 1/3] [Flight] Allow aborting encodeReply (#31106) Allow aborting encoding arguments to a Server Action if a Promise doesn't resolve. That way at least part of the arguments can be used on the receiving side. This leaves it unresolved in the stream rather than encoding an error. This should error on the receiving side when the stream closes but it doesn't right now in the Edge/Browser versions because closing happens immediately before we've had a chance to call `.then()` so the Chunks are still in pending state. This is an existing bug also in FlightClient. --- .../react-client/src/ReactFlightReplyClient.js | 17 ++++++++++++++++- .../src/client/ReactFlightDOMClientBrowser.js | 16 ++++++++++++++-- .../src/client/ReactFlightDOMClientBrowser.js | 16 ++++++++++++++-- .../src/client/ReactFlightDOMClientEdge.js | 16 ++++++++++++++-- .../src/__tests__/ReactFlightDOMReply-test.js | 16 ++++++++++++++++ .../src/client/ReactFlightDOMClientBrowser.js | 16 ++++++++++++++-- .../src/client/ReactFlightDOMClientEdge.js | 16 ++++++++++++++-- 7 files changed, 102 insertions(+), 11 deletions(-) diff --git a/packages/react-client/src/ReactFlightReplyClient.js b/packages/react-client/src/ReactFlightReplyClient.js index 35c23ed074e09..049987e39297f 100644 --- a/packages/react-client/src/ReactFlightReplyClient.js +++ b/packages/react-client/src/ReactFlightReplyClient.js @@ -185,7 +185,7 @@ export function processReply( temporaryReferences: void | TemporaryReferenceSet, resolve: (string | FormData) => void, reject: (error: mixed) => void, -): void { +): (reason: mixed) => void { let nextPartId = 1; let pendingParts = 0; let formData: null | FormData = null; @@ -841,6 +841,19 @@ export function processReply( return JSON.stringify(model, resolveToJSON); } + function abort(reason: mixed): void { + if (pendingParts > 0) { + pendingParts = 0; // Don't resolve again later. + // Resolve with what we have so far, which may have holes at this point. + // They'll error when the stream completes on the server. + if (formData === null) { + resolve(json); + } else { + resolve(formData); + } + } + } + const json = serializeModel(root, 0); if (formData === null) { @@ -854,6 +867,8 @@ export function processReply( resolve(formData); } } + + return abort; } const boundCache: WeakMap< diff --git a/packages/react-server-dom-esm/src/client/ReactFlightDOMClientBrowser.js b/packages/react-server-dom-esm/src/client/ReactFlightDOMClientBrowser.js index 58a36e7023ca2..abaa793c96a71 100644 --- a/packages/react-server-dom-esm/src/client/ReactFlightDOMClientBrowser.js +++ b/packages/react-server-dom-esm/src/client/ReactFlightDOMClientBrowser.js @@ -121,12 +121,12 @@ function createFromFetch( function encodeReply( value: ReactServerValue, - options?: {temporaryReferences?: TemporaryReferenceSet}, + options?: {temporaryReferences?: TemporaryReferenceSet, signal?: AbortSignal}, ): Promise< string | URLSearchParams | FormData, > /* We don't use URLSearchParams yet but maybe */ { return new Promise((resolve, reject) => { - processReply( + const abort = processReply( value, '', options && options.temporaryReferences @@ -135,6 +135,18 @@ function encodeReply( resolve, reject, ); + if (options && options.signal) { + const signal = options.signal; + if (signal.aborted) { + abort((signal: any).reason); + } else { + const listener = () => { + abort((signal: any).reason); + signal.removeEventListener('abort', listener); + }; + signal.addEventListener('abort', listener); + } + } }); } diff --git a/packages/react-server-dom-turbopack/src/client/ReactFlightDOMClientBrowser.js b/packages/react-server-dom-turbopack/src/client/ReactFlightDOMClientBrowser.js index 50a4a206ffb44..0d566a57caf5f 100644 --- a/packages/react-server-dom-turbopack/src/client/ReactFlightDOMClientBrowser.js +++ b/packages/react-server-dom-turbopack/src/client/ReactFlightDOMClientBrowser.js @@ -120,12 +120,12 @@ function createFromFetch( function encodeReply( value: ReactServerValue, - options?: {temporaryReferences?: TemporaryReferenceSet}, + options?: {temporaryReferences?: TemporaryReferenceSet, signal?: AbortSignal}, ): Promise< string | URLSearchParams | FormData, > /* We don't use URLSearchParams yet but maybe */ { return new Promise((resolve, reject) => { - processReply( + const abort = processReply( value, '', options && options.temporaryReferences @@ -134,6 +134,18 @@ function encodeReply( resolve, reject, ); + if (options && options.signal) { + const signal = options.signal; + if (signal.aborted) { + abort((signal: any).reason); + } else { + const listener = () => { + abort((signal: any).reason); + signal.removeEventListener('abort', listener); + }; + signal.addEventListener('abort', listener); + } + } }); } diff --git a/packages/react-server-dom-turbopack/src/client/ReactFlightDOMClientEdge.js b/packages/react-server-dom-turbopack/src/client/ReactFlightDOMClientEdge.js index 5b3a765783a52..956e014042733 100644 --- a/packages/react-server-dom-turbopack/src/client/ReactFlightDOMClientEdge.js +++ b/packages/react-server-dom-turbopack/src/client/ReactFlightDOMClientEdge.js @@ -149,12 +149,12 @@ function createFromFetch( function encodeReply( value: ReactServerValue, - options?: {temporaryReferences?: TemporaryReferenceSet}, + options?: {temporaryReferences?: TemporaryReferenceSet, signal?: AbortSignal}, ): Promise< string | URLSearchParams | FormData, > /* We don't use URLSearchParams yet but maybe */ { return new Promise((resolve, reject) => { - processReply( + const abort = processReply( value, '', options && options.temporaryReferences @@ -163,6 +163,18 @@ function encodeReply( resolve, reject, ); + if (options && options.signal) { + const signal = options.signal; + if (signal.aborted) { + abort((signal: any).reason); + } else { + const listener = () => { + abort((signal: any).reason); + signal.removeEventListener('abort', listener); + }; + signal.addEventListener('abort', listener); + } + } }); } diff --git a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMReply-test.js b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMReply-test.js index 30aa539e5ab5b..64a0616cacc2a 100644 --- a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMReply-test.js +++ b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMReply-test.js @@ -618,4 +618,20 @@ describe('ReactFlightDOMReply', () => { const root = await ReactServerDOMServer.decodeReply(body, webpackServerMap); expect(root.prop.obj).toBe(root.prop); }); + + it('can abort an unresolved model and get the partial result', async () => { + const promise = new Promise(r => {}); + const controller = new AbortController(); + const bodyPromise = ReactServerDOMClient.encodeReply( + {promise: promise, hello: 'world'}, + {signal: controller.signal}, + ); + controller.abort(); + + const result = await ReactServerDOMServer.decodeReply(await bodyPromise); + expect(result.hello).toBe('world'); + // TODO: await result.promise should reject at this point because the stream + // has closed but that's a bug in both ReactFlightReplyServer and ReactFlightClient. + // It just halts in this case. + }); }); diff --git a/packages/react-server-dom-webpack/src/client/ReactFlightDOMClientBrowser.js b/packages/react-server-dom-webpack/src/client/ReactFlightDOMClientBrowser.js index 50a4a206ffb44..0d566a57caf5f 100644 --- a/packages/react-server-dom-webpack/src/client/ReactFlightDOMClientBrowser.js +++ b/packages/react-server-dom-webpack/src/client/ReactFlightDOMClientBrowser.js @@ -120,12 +120,12 @@ function createFromFetch( function encodeReply( value: ReactServerValue, - options?: {temporaryReferences?: TemporaryReferenceSet}, + options?: {temporaryReferences?: TemporaryReferenceSet, signal?: AbortSignal}, ): Promise< string | URLSearchParams | FormData, > /* We don't use URLSearchParams yet but maybe */ { return new Promise((resolve, reject) => { - processReply( + const abort = processReply( value, '', options && options.temporaryReferences @@ -134,6 +134,18 @@ function encodeReply( resolve, reject, ); + if (options && options.signal) { + const signal = options.signal; + if (signal.aborted) { + abort((signal: any).reason); + } else { + const listener = () => { + abort((signal: any).reason); + signal.removeEventListener('abort', listener); + }; + signal.addEventListener('abort', listener); + } + } }); } diff --git a/packages/react-server-dom-webpack/src/client/ReactFlightDOMClientEdge.js b/packages/react-server-dom-webpack/src/client/ReactFlightDOMClientEdge.js index 5b3a765783a52..956e014042733 100644 --- a/packages/react-server-dom-webpack/src/client/ReactFlightDOMClientEdge.js +++ b/packages/react-server-dom-webpack/src/client/ReactFlightDOMClientEdge.js @@ -149,12 +149,12 @@ function createFromFetch( function encodeReply( value: ReactServerValue, - options?: {temporaryReferences?: TemporaryReferenceSet}, + options?: {temporaryReferences?: TemporaryReferenceSet, signal?: AbortSignal}, ): Promise< string | URLSearchParams | FormData, > /* We don't use URLSearchParams yet but maybe */ { return new Promise((resolve, reject) => { - processReply( + const abort = processReply( value, '', options && options.temporaryReferences @@ -163,6 +163,18 @@ function encodeReply( resolve, reject, ); + if (options && options.signal) { + const signal = options.signal; + if (signal.aborted) { + abort((signal: any).reason); + } else { + const listener = () => { + abort((signal: any).reason); + signal.removeEventListener('abort', listener); + }; + signal.addEventListener('abort', listener); + } + } }); } From 459fd418cfbd1f2f1be58efd8c89a0e0ecfb6d44 Mon Sep 17 00:00:00 2001 From: Timothy Yung Date: Tue, 1 Oct 2024 17:25:59 -0700 Subject: [PATCH 2/3] Define `HostInstance` type for React Native (#31101) ## Summary Creates a new `HostInstance` type for React Native, to more accurately capture the intent most developers have when using the `NativeMethods` type or `React.ElementRef>`. Since `React.ElementRef>` is typed as `React.AbstractComponent`, that means `React.ElementRef>` is equivalent to `NativeMethods` which is equivalent to `HostInstance`. ## How did you test this change? ``` $ yarn $ yarn flow fabric ``` --- .../src/ReactNativeTypes.js | 29 ++++++++----------- 1 file changed, 12 insertions(+), 17 deletions(-) diff --git a/packages/react-native-renderer/src/ReactNativeTypes.js b/packages/react-native-renderer/src/ReactNativeTypes.js index 9692a1256acff..03c03cfba0072 100644 --- a/packages/react-native-renderer/src/ReactNativeTypes.js +++ b/packages/react-native-renderer/src/ReactNativeTypes.js @@ -112,31 +112,32 @@ export interface INativeMethods { measure(callback: MeasureOnSuccessCallback): void; measureInWindow(callback: MeasureInWindowOnSuccessCallback): void; measureLayout( - relativeToNativeNode: number | ElementRef>, + relativeToNativeNode: number | HostInstance, onSuccess: MeasureLayoutOnSuccessCallback, onFail?: () => void, ): void; setNativeProps(nativeProps: {...}): void; } -export type NativeMethods = $ReadOnly<{| +export type NativeMethods = $ReadOnly<{ blur(): void, focus(): void, measure(callback: MeasureOnSuccessCallback): void, measureInWindow(callback: MeasureInWindowOnSuccessCallback): void, measureLayout( - relativeToNativeNode: number | ElementRef>, + relativeToNativeNode: number | HostInstance, onSuccess: MeasureLayoutOnSuccessCallback, onFail?: () => void, ): void, setNativeProps(nativeProps: {...}): void, -|}>; +}>; // This validates that INativeMethods and NativeMethods stay in sync using Flow! declare const ensureNativeMethodsAreSynced: NativeMethods; (ensureNativeMethodsAreSynced: INativeMethods); -export type HostComponent = AbstractComponent>; +export type HostInstance = NativeMethods; +export type HostComponent = AbstractComponent; type SecretInternalsType = { computeComponentStackForErrorReporting(tag: number): string, @@ -209,7 +210,7 @@ export type RenderRootOptions = { export type ReactNativeType = { findHostInstance_DEPRECATED( componentOrHandle: ?(ElementRef | number), - ): ?ElementRef>, + ): ?HostInstance, findNodeHandle( componentOrHandle: ?(ElementRef | number), ): ?number, @@ -218,14 +219,11 @@ export type ReactNativeType = { child: PublicInstance | HostComponent, ): boolean, dispatchCommand( - handle: ElementRef>, + handle: HostInstance, command: string, args: Array, ): void, - sendAccessibilityEvent( - handle: ElementRef>, - eventType: string, - ): void, + sendAccessibilityEvent(handle: HostInstance, eventType: string): void, render( element: MixedElement, containerTag: number, @@ -247,20 +245,17 @@ type PublicTextInstance = mixed; export type ReactFabricType = { findHostInstance_DEPRECATED( componentOrHandle: ?(ElementRef | number), - ): ?ElementRef>, + ): ?HostInstance, findNodeHandle( componentOrHandle: ?(ElementRef | number), ): ?number, dispatchCommand( - handle: ElementRef>, + handle: HostInstance, command: string, args: Array, ): void, isChildPublicInstance(parent: PublicInstance, child: PublicInstance): boolean, - sendAccessibilityEvent( - handle: ElementRef>, - eventType: string, - ): void, + sendAccessibilityEvent(handle: HostInstance, eventType: string): void, render( element: MixedElement, containerTag: number, From 0751fac747452af8c0494900b4afa7c56ee7b32c Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Wed, 2 Oct 2024 12:53:57 -0400 Subject: [PATCH 3/3] [compiler] Optional chaining for dependencies (HIR rewrite) Adds HIR version of `PropagateScopeDeps` to handle optional chaining. Internally, this improves memoization on ~4% of compiled files (internal links: [1](https://www.internalfb.com/intern/paste/P1610406497/)) Summarizing the changes in this PR. 1. `CollectOptionalChainDependencies` recursively traverses optional blocks down to the base. From the base, we build up a set of `baseIdentifier.propertyA?.propertyB` mappings. The tricky bit here is that optional blocks sometimes reference other optional blocks that are *not* part of the same chain e.g. a(c?.d)?.d. See code + comments in `traverseOptionalBlock` for how we avoid concatenating unrelated blocks. 2. Adding optional chains into non-null object calculation. (Note that marking `a?.b` as 'non-null' means that `a?.b.c` is safe to evaluate, *not* `(a?.b).c`. Happy to rename this / reword comments accordingly if there's a better term) This pass is split into two stages. (1) collecting non-null objects by block and (2) propagating non-null objects across blocks. The only significant change here was to (2). We add an extra reduce step `X=Reduce(Union(X, Intersect(X_neighbors)))` to merge optional and non-optional nodes (e.g. nonNulls=`{a, a?.b}` reduces to `{a, a.b}`) 3. Adding optional chains into dependency calculation. This was the trickiest. We need to take the "maximal" property chain as a dependency. Prior to this PR, we avoided taking subpaths e.g. `a.b` of `a.b.c` as dependencies by only visiting non-PropertyLoad/LoadLocal instructions. This effectively only recorded the property-path at site-of-use. Unfortunately, this *quite* doesn't work for optional chains for a few reasons: - We would need to skip relevant `StoreLocal`/`Branch terminal` instructions (but only those within optional blocks that have been successfully read). - Given an optional chain, either (1) only a subpath or (2) the entire path can be represented as a PropertyLoad. We cannot directly add the last hoistable optional-block as a dependency as MethodCalls are an edge case e.g. given a?.b.c(), we should depend on `a?.b`, not `a?.b.c` This means that we add its dependency at either the innermost unhoistable optional-block or when encountering it within its phi-join. 4. Handle optional chains in DeriveMinimalDependenciesHIR. This was also a bit tricky to formulate. Ideally, we would avoid a 2^3 case join (cond | uncond cfg, optional | not optional load, access | dependency). This PR attempts to simplify by building two trees 1. First add each hoistable path into a tree containing `Optional | NonOptional` nodes. 2. Then add each dependency into another tree containing `Optional | NonOptional`, `Access | Dependency` nodes, truncating the dependency at the earliest non-hoistable node (i.e. non-matching pair when walking the hoistable tree) ghstack-source-id: a2170f26280dfbf65a4893d8a658f863a0fd0c88 Pull Request resolved: https://github.com/facebook/react/pull/31037 --- .../src/HIR/CollectHoistablePropertyLoads.ts | 128 +++++- .../HIR/CollectOptionalChainDependencies.ts | 382 ++++++++++++++++++ .../src/HIR/DeriveMinimalDependenciesHIR.ts | 367 ++++++++++------- .../src/HIR/HIR.ts | 1 + .../src/HIR/PropagateScopeDependenciesHIR.ts | 111 +++-- .../src/Utils/utils.ts | 24 ++ ...equential-optional-chain-nonnull.expect.md | 71 ++++ ...infer-sequential-optional-chain-nonnull.ts | 20 + .../compiler/nested-optional-chains.expect.md | 229 +++++++++++ .../compiler/nested-optional-chains.ts | 91 +++++ ...al-member-expression-as-memo-dep.expect.md | 96 +++-- .../optional-member-expression-as-memo-dep.js | 25 +- ...ptional-member-expression-single.expect.md | 82 ++-- .../optional-member-expression-single.js | 20 +- ...-optional-call-chain-in-optional.expect.md | 2 +- ...al-member-expression-as-memo-dep.expect.md | 32 -- ...-optional-member-expression-as-memo-dep.js | 7 - ...ession-single-with-unconditional.expect.md | 42 -- ...ptional-member-expression-single.expect.md | 39 -- ....todo-optional-member-expression-single.js | 10 - ...equential-optional-chain-nonnull.expect.md | 74 ++++ ...infer-sequential-optional-chain-nonnull.ts | 22 + .../nested-optional-chains.expect.md | 232 +++++++++++ .../nested-optional-chains.ts | 93 +++++ ...al-member-expression-as-memo-dep.expect.md | 98 +++++ .../optional-member-expression-as-memo-dep.js | 24 ++ ...ession-single-with-unconditional.expect.md | 62 +++ ...r-expression-single-with-unconditional.js} | 0 ...ptional-member-expression-single.expect.md | 91 +++++ .../optional-member-expression-single.js | 22 + ...properties-inside-optional-chain.expect.md | 4 +- .../conditional-member-expr.expect.md | 4 +- .../memberexpr-join-optional-chain.expect.md | 4 +- .../memberexpr-join-optional-chain2.expect.md | 21 +- ...e-uncond-optional-chain-and-cond.expect.md | 72 ++++ .../merge-uncond-optional-chain-and-cond.ts | 22 + ...epro-scope-missing-mutable-range.expect.md | 4 +- 37 files changed, 2221 insertions(+), 407 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/HIR/CollectOptionalChainDependencies.ts create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-sequential-optional-chain-nonnull.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-sequential-optional-chain-nonnull.ts create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nested-optional-chains.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nested-optional-chains.ts delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/error.todo-optional-member-expression-as-memo-dep.expect.md delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/error.todo-optional-member-expression-as-memo-dep.js delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/error.todo-optional-member-expression-single-with-unconditional.expect.md delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/error.todo-optional-member-expression-single.expect.md delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/error.todo-optional-member-expression-single.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/infer-sequential-optional-chain-nonnull.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/infer-sequential-optional-chain-nonnull.ts create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/nested-optional-chains.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/nested-optional-chains.ts create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/optional-member-expression-as-memo-dep.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/optional-member-expression-as-memo-dep.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/optional-member-expression-single-with-unconditional.expect.md rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/{error.todo-optional-member-expression-single-with-unconditional.js => optional-member-expression-single-with-unconditional.js} (100%) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/optional-member-expression-single.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/optional-member-expression-single.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/merge-uncond-optional-chain-and-cond.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/merge-uncond-optional-chain-and-cond.ts diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts index cb778c329226b..3603416ee6887 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts @@ -1,6 +1,12 @@ import {CompilerError} from '../CompilerError'; import {inRange} from '../ReactiveScopes/InferReactiveScopeVariables'; -import {Set_intersect, Set_union, getOrInsertDefault} from '../Utils/utils'; +import { + Set_equal, + Set_filter, + Set_intersect, + Set_union, + getOrInsertDefault, +} from '../Utils/utils'; import { BasicBlock, BlockId, @@ -15,9 +21,9 @@ import { } from './HIR'; /** - * Helper function for `PropagateScopeDependencies`. - * Uses control flow graph analysis to determine which `Identifier`s can - * be assumed to be non-null objects, on a per-block basis. + * Helper function for `PropagateScopeDependencies`. Uses control flow graph + * analysis to determine which `Identifier`s can be assumed to be non-null + * objects, on a per-block basis. * * Here is an example: * ```js @@ -42,15 +48,16 @@ import { * } * ``` * - * Note that we currently do NOT account for mutable / declaration range - * when doing the CFG-based traversal, producing results that are technically + * Note that we currently do NOT account for mutable / declaration range when + * doing the CFG-based traversal, producing results that are technically * incorrect but filtered by PropagateScopeDeps (which only takes dependencies * on constructed value -- i.e. a scope's dependencies must have mutable ranges * ending earlier than the scope start). * - * Take this example, this function will infer x.foo.bar as non-nullable for bb0, - * via the intersection of bb1 & bb2 which in turn comes from bb3. This is technically - * incorrect bb0 is before / during x's mutable range. + * Take this example, this function will infer x.foo.bar as non-nullable for + * bb0, via the intersection of bb1 & bb2 which in turn comes from bb3. This is + * technically incorrect bb0 is before / during x's mutable range. + * ``` * bb0: * const x = ...; * if cond then bb1 else bb2 @@ -62,15 +69,30 @@ import { * goto bb3: * bb3: * x.foo.bar + * ``` + * + * @param fn + * @param temporaries sidemap of identifier -> baseObject.a.b paths. Does not + * contain optional chains. + * @param hoistableFromOptionals sidemap of optionalBlock -> baseObject?.a + * optional paths for which it's safe to evaluate non-optional loads (see + * CollectOptionalChainDependencies). + * @returns */ export function collectHoistablePropertyLoads( fn: HIRFunction, temporaries: ReadonlyMap, + hoistableFromOptionals: ReadonlyMap, ): ReadonlyMap { const registry = new PropertyPathRegistry(); - const nodes = collectNonNullsInBlocks(fn, temporaries, registry); - propagateNonNull(fn, nodes); + const nodes = collectNonNullsInBlocks( + fn, + temporaries, + hoistableFromOptionals, + registry, + ); + propagateNonNull(fn, nodes, registry); const nodesKeyedByScopeId = new Map(); for (const [_, block] of fn.body.blocks) { @@ -96,17 +118,21 @@ export type BlockInfo = { */ type RootNode = { properties: Map; + optionalProperties: Map; parent: null; // Recorded to make later computations simpler fullPath: ReactiveScopeDependency; + hasOptional: boolean; root: IdentifierId; }; type PropertyPathNode = | { properties: Map; + optionalProperties: Map; parent: PropertyPathNode; fullPath: ReactiveScopeDependency; + hasOptional: boolean; } | RootNode; @@ -124,10 +150,12 @@ class PropertyPathRegistry { rootNode = { root: identifier.id, properties: new Map(), + optionalProperties: new Map(), fullPath: { identifier, path: [], }, + hasOptional: false, parent: null, }; this.roots.set(identifier.id, rootNode); @@ -139,23 +167,20 @@ class PropertyPathRegistry { parent: PropertyPathNode, entry: DependencyPathEntry, ): PropertyPathNode { - if (entry.optional) { - CompilerError.throwTodo({ - reason: 'handle optional nodes', - loc: GeneratedSource, - }); - } - let child = parent.properties.get(entry.property); + const map = entry.optional ? parent.optionalProperties : parent.properties; + let child = map.get(entry.property); if (child == null) { child = { properties: new Map(), + optionalProperties: new Map(), parent: parent, fullPath: { identifier: parent.fullPath.identifier, path: parent.fullPath.path.concat(entry), }, + hasOptional: parent.hasOptional || entry.optional, }; - parent.properties.set(entry.property, child); + map.set(entry.property, child); } return child; } @@ -216,6 +241,7 @@ function addNonNullPropertyPath( function collectNonNullsInBlocks( fn: HIRFunction, temporaries: ReadonlyMap, + hoistableFromOptionals: ReadonlyMap, registry: PropertyPathRegistry, ): ReadonlyMap { /** @@ -252,6 +278,13 @@ function collectNonNullsInBlocks( const assumedNonNullObjects = new Set( knownNonNullIdentifiers, ); + + const maybeOptionalChain = hoistableFromOptionals.get(block.id); + if (maybeOptionalChain != null) { + assumedNonNullObjects.add( + registry.getOrCreateProperty(maybeOptionalChain), + ); + } for (const instr of block.instructions) { if (instr.value.kind === 'PropertyLoad') { const source = temporaries.get(instr.value.object.identifier.id) ?? { @@ -303,6 +336,7 @@ function collectNonNullsInBlocks( function propagateNonNull( fn: HIRFunction, nodes: ReadonlyMap, + registry: PropertyPathRegistry, ): void { const blockSuccessors = new Map>(); const terminalPreds = new Set(); @@ -388,10 +422,17 @@ function propagateNonNull( const prevObjects = assertNonNull(nodes.get(nodeId)).assumedNonNullObjects; const mergedObjects = Set_union(prevObjects, neighborAccesses); + reduceMaybeOptionalChains(mergedObjects, registry); assertNonNull(nodes.get(nodeId)).assumedNonNullObjects = mergedObjects; traversalState.set(nodeId, 'done'); - changed ||= prevObjects.size !== mergedObjects.size; + /** + * Note that it's not sufficient to compare set sizes since + * reduceMaybeOptionalChains may replace optional-chain loads with + * unconditional loads. This could in turn change `assumedNonNullObjects` of + * downstream blocks and backedges. + */ + changed ||= !Set_equal(prevObjects, mergedObjects); return changed; } const traversalState = new Map(); @@ -440,3 +481,50 @@ export function assertNonNull, U>( }); return value; } + +/** + * Any two optional chains with different operations . vs ?. but the same set of + * property strings paths de-duplicates. + * + * Intuitively: given ?.b, we know to be either hoistable or not. + * If unconditional reads from are hoistable, we can replace all + * ?.PROPERTY_STRING subpaths with .PROPERTY_STRING + */ +function reduceMaybeOptionalChains( + nodes: Set, + registry: PropertyPathRegistry, +): void { + let optionalChainNodes = Set_filter(nodes, n => n.hasOptional); + if (optionalChainNodes.size === 0) { + return; + } + let changed: boolean; + do { + changed = false; + + for (const original of optionalChainNodes) { + let {identifier, path: origPath} = original.fullPath; + let currNode: PropertyPathNode = + registry.getOrCreateIdentifier(identifier); + for (let i = 0; i < origPath.length; i++) { + const entry = origPath[i]; + // If the base is known to be non-null, replace with a non-optional load + const nextEntry: DependencyPathEntry = + entry.optional && nodes.has(currNode) + ? {property: entry.property, optional: false} + : entry; + currNode = PropertyPathRegistry.getOrCreatePropertyEntry( + currNode, + nextEntry, + ); + } + if (currNode !== original) { + changed = true; + optionalChainNodes.delete(original); + optionalChainNodes.add(currNode); + nodes.delete(original); + nodes.add(currNode); + } + } + } while (changed); +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectOptionalChainDependencies.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectOptionalChainDependencies.ts new file mode 100644 index 0000000000000..453294784246f --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectOptionalChainDependencies.ts @@ -0,0 +1,382 @@ +import {CompilerError} from '..'; +import {assertNonNull} from './CollectHoistablePropertyLoads'; +import { + BlockId, + BasicBlock, + InstructionId, + IdentifierId, + ReactiveScopeDependency, + BranchTerminal, + TInstruction, + PropertyLoad, + StoreLocal, + GotoVariant, + TBasicBlock, + OptionalTerminal, + HIRFunction, + DependencyPathEntry, +} from './HIR'; +import {printIdentifier} from './PrintHIR'; + +export function collectOptionalChainSidemap( + fn: HIRFunction, +): OptionalChainSidemap { + const context: OptionalTraversalContext = { + blocks: fn.body.blocks, + seenOptionals: new Set(), + processedInstrsInOptional: new Set(), + temporariesReadInOptional: new Map(), + hoistableObjects: new Map(), + }; + for (const [_, block] of fn.body.blocks) { + if ( + block.terminal.kind === 'optional' && + !context.seenOptionals.has(block.id) + ) { + traverseOptionalBlock( + block as TBasicBlock, + context, + null, + ); + } + } + + return { + temporariesReadInOptional: context.temporariesReadInOptional, + processedInstrsInOptional: context.processedInstrsInOptional, + hoistableObjects: context.hoistableObjects, + }; +} +export type OptionalChainSidemap = { + /** + * Stores the correct property mapping (e.g. `a?.b` instead of `a.b`) for + * dependency calculation. Note that we currently do not store anything on + * outer phi nodes. + */ + temporariesReadInOptional: ReadonlyMap; + /** + * Records instructions (PropertyLoads, StoreLocals, and test terminals) + * processed in this pass. When extracting dependencies in + * PropagateScopeDependencies, these instructions are skipped. + * + * E.g. given a?.b + * ``` + * bb0 + * $0 = LoadLocal 'a' + * test $0 then=bb1 <- Avoid adding dependencies from these instructions, as + * bb1 the sidemap produced by readOptionalBlock already maps + * $1 = PropertyLoad $0.'b' <- $1 and $2 back to a?.b. Instead, we want to add a?.b + * StoreLocal $2 = $1 <- as a dependency when $1 or $2 are later used in either + * - an unhoistable expression within an outer optional + * block e.g. MethodCall + * - a phi node (if the entire optional value is hoistable) + * ``` + * + * Note that mapping blockIds to their evaluated dependency path does not + * work, since values produced by inner optional chains may be referenced in + * outer ones + * ``` + * a?.b.c() + * -> + * bb0 + * $0 = LoadLocal 'a' + * test $0 then=bb1 + * bb1 + * $1 = PropertyLoad $0.'b' + * StoreLocal $2 = $1 + * goto bb2 + * bb2 + * test $2 then=bb3 + * bb3: + * $3 = PropertyLoad $2.'c' + * StoreLocal $4 = $3 + * goto bb4 + * bb4 + * test $4 then=bb5 + * bb5: + * $5 = MethodCall $2.$4() <--- here, we want to take a dep on $2 and $4! + * ``` + */ + processedInstrsInOptional: ReadonlySet; + /** + * Records optional chains for which we can safely evaluate non-optional + * PropertyLoads. e.g. given `a?.b.c`, we can evaluate any load from `a?.b` at + * the optional terminal in bb1. + * ```js + * bb1: + * ... + * Optional optional=false test=bb2 fallth=... + * bb2: + * Optional optional=true test=bb3 fallth=... + * ... + * ``` + */ + hoistableObjects: ReadonlyMap; +}; + +type OptionalTraversalContext = { + blocks: ReadonlyMap; + + // Track optional blocks to avoid outer calls into nested optionals + seenOptionals: Set; + + processedInstrsInOptional: Set; + temporariesReadInOptional: Map; + hoistableObjects: Map; +}; + +/** + * Match the consequent and alternate blocks of an optional. + * @returns propertyload computed by the consequent block, or null if the + * consequent block is not a simple PropertyLoad. + */ +function matchOptionalTestBlock( + terminal: BranchTerminal, + blocks: ReadonlyMap, +): { + consequentId: IdentifierId; + property: string; + propertyId: IdentifierId; + storeLocalInstrId: InstructionId; + consequentGoto: BlockId; +} | null { + const consequentBlock = assertNonNull(blocks.get(terminal.consequent)); + if ( + consequentBlock.instructions.length === 2 && + consequentBlock.instructions[0].value.kind === 'PropertyLoad' && + consequentBlock.instructions[1].value.kind === 'StoreLocal' + ) { + const propertyLoad: TInstruction = consequentBlock + .instructions[0] as TInstruction; + const storeLocal: StoreLocal = consequentBlock.instructions[1].value; + const storeLocalInstrId = consequentBlock.instructions[1].id; + CompilerError.invariant( + propertyLoad.value.object.identifier.id === terminal.test.identifier.id, + { + reason: + '[OptionalChainDeps] Inconsistent optional chaining property load', + description: `Test=${printIdentifier(terminal.test.identifier)} PropertyLoad base=${printIdentifier(propertyLoad.value.object.identifier)}`, + loc: propertyLoad.loc, + }, + ); + + CompilerError.invariant( + storeLocal.value.identifier.id === propertyLoad.lvalue.identifier.id, + { + reason: '[OptionalChainDeps] Unexpected storeLocal', + loc: propertyLoad.loc, + }, + ); + if ( + consequentBlock.terminal.kind !== 'goto' || + consequentBlock.terminal.variant !== GotoVariant.Break + ) { + return null; + } + const alternate = assertNonNull(blocks.get(terminal.alternate)); + + CompilerError.invariant( + alternate.instructions.length === 2 && + alternate.instructions[0].value.kind === 'Primitive' && + alternate.instructions[1].value.kind === 'StoreLocal', + { + reason: 'Unexpected alternate structure', + loc: terminal.loc, + }, + ); + + return { + consequentId: storeLocal.lvalue.place.identifier.id, + property: propertyLoad.value.property, + propertyId: propertyLoad.lvalue.identifier.id, + storeLocalInstrId, + consequentGoto: consequentBlock.terminal.block, + }; + } + return null; +} + +/** + * Traverse into the optional block and all transitively referenced blocks to + * collect sidemaps of optional chain dependencies. + * + * @returns the IdentifierId representing the optional block if the block and + * all transitively referenced optional blocks precisely represent a chain of + * property loads. If any part of the optional chain is not hoistable, returns + * null. + */ +function traverseOptionalBlock( + optional: TBasicBlock, + context: OptionalTraversalContext, + outerAlternate: BlockId | null, +): IdentifierId | null { + context.seenOptionals.add(optional.id); + const maybeTest = context.blocks.get(optional.terminal.test)!; + let test: BranchTerminal; + let baseObject: ReactiveScopeDependency; + if (maybeTest.terminal.kind === 'branch') { + CompilerError.invariant(optional.terminal.optional, { + reason: '[OptionalChainDeps] Expect base case to be always optional', + loc: optional.terminal.loc, + }); + /** + * Optional base expressions are currently within value blocks which cannot + * be interrupted by scope boundaries. As such, the only dependencies we can + * hoist out of optional chains are property load chains with no intervening + * instructions. + * + * Ideally, we would be able to flatten base instructions out of optional + * blocks, but this would require changes to HIR. + * + * For now, only match base expressions that are straightforward + * PropertyLoad chains + */ + if ( + maybeTest.instructions.length === 0 || + maybeTest.instructions[0].value.kind !== 'LoadLocal' + ) { + return null; + } + const path: Array = []; + for (let i = 1; i < maybeTest.instructions.length; i++) { + const instrVal = maybeTest.instructions[i].value; + const prevInstr = maybeTest.instructions[i - 1]; + if ( + instrVal.kind === 'PropertyLoad' && + instrVal.object.identifier.id === prevInstr.lvalue.identifier.id + ) { + path.push({property: instrVal.property, optional: false}); + } else { + return null; + } + } + CompilerError.invariant( + maybeTest.terminal.test.identifier.id === + maybeTest.instructions.at(-1)!.lvalue.identifier.id, + { + reason: '[OptionalChainDeps] Unexpected test expression', + loc: maybeTest.terminal.loc, + }, + ); + baseObject = { + identifier: maybeTest.instructions[0].value.place.identifier, + path, + }; + test = maybeTest.terminal; + } else if (maybeTest.terminal.kind === 'optional') { + /** + * This is either + * - ?.property (optional=true) + * - .property (optional=false) + * - + * - a optional base block with a separate nested optional-chain (e.g. a(c?.d)?.d) + */ + const testBlock = context.blocks.get(maybeTest.terminal.fallthrough)!; + if (testBlock!.terminal.kind !== 'branch') { + /** + * Fallthrough of the inner optional should be a block with no + * instructions, terminating with Test($) + */ + CompilerError.throwTodo({ + reason: `Unexpected terminal kind \`${testBlock.terminal.kind}\` for optional fallthrough block`, + loc: maybeTest.terminal.loc, + }); + } + /** + * Recurse into inner optional blocks to collect inner optional-chain + * expressions, regardless of whether we can match the outer one to a + * PropertyLoad. + */ + const innerOptional = traverseOptionalBlock( + maybeTest as TBasicBlock, + context, + testBlock.terminal.alternate, + ); + if (innerOptional == null) { + return null; + } + + /** + * Check that the inner optional is part of the same optional-chain as the + * outer one. This is not guaranteed, e.g. given a(c?.d)?.d + * ``` + * bb0: + * Optional test=bb1 + * bb1: + * $0 = LoadLocal a <-- part 1 of the outer optional-chaining base + * Optional test=bb2 fallth=bb5 <-- start of optional chain for c?.d + * bb2: + * ... (optional chain for c?.d) + * ... + * bb5: + * $1 = phi(c.d, undefined) <-- part 2 (continuation) of the outer optional-base + * $2 = Call $0($1) + * Branch $2 ... + * ``` + */ + if (testBlock.terminal.test.identifier.id !== innerOptional) { + return null; + } + + if (!optional.terminal.optional) { + /** + * If this is an non-optional load participating in an optional chain + * (e.g. loading the `c` property in `a?.b.c`), record that PropertyLoads + * from the inner optional value are hoistable. + */ + context.hoistableObjects.set( + optional.id, + assertNonNull(context.temporariesReadInOptional.get(innerOptional)), + ); + } + baseObject = assertNonNull( + context.temporariesReadInOptional.get(innerOptional), + ); + test = testBlock.terminal; + } else { + return null; + } + + if (test.alternate === outerAlternate) { + CompilerError.invariant(optional.instructions.length === 0, { + reason: + '[OptionalChainDeps] Unexpected instructions an inner optional block. ' + + 'This indicates that the compiler may be incorrectly concatenating two unrelated optional chains', + loc: optional.terminal.loc, + }); + } + const matchConsequentResult = matchOptionalTestBlock(test, context.blocks); + if (!matchConsequentResult) { + // Optional chain consequent is not hoistable e.g. a?.[computed()] + return null; + } + CompilerError.invariant( + matchConsequentResult.consequentGoto === optional.terminal.fallthrough, + { + reason: '[OptionalChainDeps] Unexpected optional goto-fallthrough', + description: `${matchConsequentResult.consequentGoto} != ${optional.terminal.fallthrough}`, + loc: optional.terminal.loc, + }, + ); + const load = { + identifier: baseObject.identifier, + path: [ + ...baseObject.path, + { + property: matchConsequentResult.property, + optional: optional.terminal.optional, + }, + ], + }; + context.processedInstrsInOptional.add( + matchConsequentResult.storeLocalInstrId, + ); + context.processedInstrsInOptional.add(test.id); + context.temporariesReadInOptional.set( + matchConsequentResult.consequentId, + load, + ); + context.temporariesReadInOptional.set(matchConsequentResult.propertyId, load); + return matchConsequentResult.consequentId; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/DeriveMinimalDependenciesHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/DeriveMinimalDependenciesHIR.ts index f2bb0b31f05c9..f5567b3e536d1 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/DeriveMinimalDependenciesHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/DeriveMinimalDependenciesHIR.ts @@ -6,97 +6,173 @@ */ import {CompilerError} from '../CompilerError'; -import {GeneratedSource, Identifier, ReactiveScopeDependency} from '../HIR'; +import { + DependencyPathEntry, + GeneratedSource, + Identifier, + ReactiveScopeDependency, +} from '../HIR'; import {printIdentifier} from '../HIR/PrintHIR'; import {ReactiveScopePropertyDependency} from '../ReactiveScopes/DeriveMinimalDependencies'; -const ENABLE_DEBUG_INVARIANTS = true; - /** * Simpler fork of DeriveMinimalDependencies, see PropagateScopeDependenciesHIR * for detailed explanation. */ export class ReactiveScopeDependencyTreeHIR { - #roots: Map = new Map(); + /** + * Paths from which we can hoist PropertyLoads. If an `identifier`, + * `identifier.path`, or `identifier?.path` is in this map, it is safe to + * evaluate (non-optional) PropertyLoads from. + */ + #hoistableObjects: Map = new Map(); + #deps: Map = new Map(); + + /** + * @param hoistableObjects a set of paths from which we can safely evaluate + * PropertyLoads. Note that we expect these to not contain duplicates (e.g. + * both `a?.b` and `a.b`) only because CollectHoistablePropertyLoads merges + * duplicates when traversing the CFG. + */ + constructor(hoistableObjects: Iterable) { + for (const {path, identifier} of hoistableObjects) { + let currNode = ReactiveScopeDependencyTreeHIR.#getOrCreateRoot( + identifier, + this.#hoistableObjects, + path.length > 0 && path[0].optional ? 'Optional' : 'NonNull', + ); - #getOrCreateRoot( + for (let i = 0; i < path.length; i++) { + const prevAccessType = currNode.properties.get( + path[i].property, + )?.accessType; + const accessType = + i + 1 < path.length && path[i + 1].optional ? 'Optional' : 'NonNull'; + CompilerError.invariant( + prevAccessType == null || prevAccessType === accessType, + { + reason: 'Conflicting access types', + loc: GeneratedSource, + }, + ); + let nextNode = currNode.properties.get(path[i].property); + if (nextNode == null) { + nextNode = { + properties: new Map(), + accessType, + }; + currNode.properties.set(path[i].property, nextNode); + } + currNode = nextNode; + } + } + } + + static #getOrCreateRoot( identifier: Identifier, - accessType: PropertyAccessType, - ): DependencyNode { + roots: Map>, + defaultAccessType: T, + ): TreeNode { // roots can always be accessed unconditionally in JS - let rootNode = this.#roots.get(identifier); + let rootNode = roots.get(identifier); if (rootNode === undefined) { rootNode = { properties: new Map(), - accessType, + accessType: defaultAccessType, }; - this.#roots.set(identifier, rootNode); + roots.set(identifier, rootNode); } return rootNode; } + /** + * Join a dependency with `#hoistableObjects` to record the hoistable + * dependency. This effectively truncates @param dep to its maximal + * safe-to-evaluate subpath + */ addDependency(dep: ReactiveScopePropertyDependency): void { - const {path} = dep; - let currNode = this.#getOrCreateRoot(dep.identifier, MIN_ACCESS_TYPE); - - const accessType = PropertyAccessType.Access; - - currNode.accessType = merge(currNode.accessType, accessType); - - for (const property of path) { - // all properties read 'on the way' to a dependency are marked as 'access' - let currChild = makeOrMergeProperty( - currNode, - property.property, - accessType, - ); - currNode = currChild; - } - - /* - * If this property does not have a conditional path (i.e. a.b.c), the - * final property node should be marked as an conditional/unconditional - * `dependency` as based on control flow. - */ - currNode.accessType = merge( - currNode.accessType, - PropertyAccessType.Dependency, + const {identifier, path} = dep; + let depCursor = ReactiveScopeDependencyTreeHIR.#getOrCreateRoot( + identifier, + this.#deps, + PropertyAccessType.UnconditionalAccess, ); - } + /** + * hoistableCursor is null if depCursor is not an object we can hoist + * property reads from otherwise, it represents the same node in the + * hoistable / cfg-informed tree + */ + let hoistableCursor: HoistableNode | undefined = + this.#hoistableObjects.get(identifier); - markNodesNonNull(dep: ReactiveScopePropertyDependency): void { - const accessType = PropertyAccessType.NonNullAccess; - let currNode = this.#roots.get(dep.identifier); + // All properties read 'on the way' to a dependency are marked as 'access' + for (const entry of path) { + let nextHoistableCursor: HoistableNode | undefined; + let nextDepCursor: DependencyNode; + if (entry.optional) { + /** + * No need to check the access type since we can match both optional or non-optionals + * in the hoistable + * e.g. a?.b is hoistable if a.b is hoistable + */ + if (hoistableCursor != null) { + nextHoistableCursor = hoistableCursor?.properties.get(entry.property); + } - let cursor = 0; - while (currNode != null && cursor < dep.path.length) { - currNode.accessType = merge(currNode.accessType, accessType); - currNode = currNode.properties.get(dep.path[cursor++].property); - } - if (currNode != null) { - currNode.accessType = merge(currNode.accessType, accessType); + let accessType; + if ( + hoistableCursor != null && + hoistableCursor.accessType === 'NonNull' + ) { + /** + * For an optional chain dep `a?.b`: if the hoistable tree only + * contains `a`, we can keep either `a?.b` or 'a.b' as a dependency. + * (note that we currently do the latter for perf) + */ + accessType = PropertyAccessType.UnconditionalAccess; + } else { + /** + * Given that it's safe to evaluate `depCursor` and optional load + * never throws, it's also safe to evaluate `depCursor?.entry` + */ + accessType = PropertyAccessType.OptionalAccess; + } + nextDepCursor = makeOrMergeProperty( + depCursor, + entry.property, + accessType, + ); + } else if ( + hoistableCursor != null && + hoistableCursor.accessType === 'NonNull' + ) { + nextHoistableCursor = hoistableCursor.properties.get(entry.property); + nextDepCursor = makeOrMergeProperty( + depCursor, + entry.property, + PropertyAccessType.UnconditionalAccess, + ); + } else { + /** + * Break to truncate the dependency on its first non-optional entry that PropertyLoads are not hoistable from + */ + break; + } + depCursor = nextDepCursor; + hoistableCursor = nextHoistableCursor; } + // mark the final node as a dependency + depCursor.accessType = merge( + depCursor.accessType, + PropertyAccessType.OptionalDependency, + ); } - /** - * Derive a set of minimal dependencies that are safe to - * access unconditionally (with respect to nullthrows behavior) - */ deriveMinimalDependencies(): Set { const results = new Set(); - for (const [rootId, rootNode] of this.#roots.entries()) { - if (ENABLE_DEBUG_INVARIANTS) { - assertWellFormedTree(rootNode); - } - const deps = deriveMinimalDependenciesInSubtree(rootNode, []); - - for (const dep of deps) { - results.add({ - identifier: rootId, - path: dep.path.map(s => ({property: s, optional: false})), - }); - } + for (const [rootId, rootNode] of this.#deps.entries()) { + collectMinimalDependenciesInSubtree(rootNode, rootId, [], results); } return results; @@ -110,7 +186,7 @@ export class ReactiveScopeDependencyTreeHIR { printDeps(includeAccesses: boolean): string { let res: Array> = []; - for (const [rootId, rootNode] of this.#roots.entries()) { + for (const [rootId, rootNode] of this.#deps.entries()) { const rootResults = printSubtree(rootNode, includeAccesses).map( result => `${printIdentifier(rootId)}.${result}`, ); @@ -118,31 +194,64 @@ export class ReactiveScopeDependencyTreeHIR { } return res.flat().join('\n'); } + + static debug(roots: Map>): string { + const buf: Array = [`tree() [`]; + for (const [rootId, rootNode] of roots) { + buf.push(`${printIdentifier(rootId)} (${rootNode.accessType}):`); + this.#debugImpl(buf, rootNode, 1); + } + buf.push(']'); + return buf.length > 2 ? buf.join('\n') : buf.join(''); + } + + static #debugImpl( + buf: Array, + node: TreeNode, + depth: number = 0, + ): void { + for (const [property, childNode] of node.properties) { + buf.push(`${' '.repeat(depth)}.${property} (${childNode.accessType}):`); + this.#debugImpl(buf, childNode, depth + 1); + } + } } +/* + * Enum representing the access type of single property on a parent object. + * We distinguish on two independent axes: + * Optional / Unconditional: + * - whether this property is an optional load (within an optional chain) + * Access / Dependency: + * - Access: this property is read on the path of a dependency. We do not + * need to track change variables for accessed properties. Tracking accesses + * helps Forget do more granular dependency tracking. + * - Dependency: this property is read as a dependency and we must track changes + * to it for correctness. + * ```javascript + * // props.a is a dependency here and must be tracked + * deps: {props.a, props.a.b} ---> minimalDeps: {props.a} + * // props.a is just an access here and does not need to be tracked + * deps: {props.a.b} ---> minimalDeps: {props.a.b} + * ``` + */ enum PropertyAccessType { - Access = 'Access', - NonNullAccess = 'NonNullAccess', - Dependency = 'Dependency', - NonNullDependency = 'NonNullDependency', + OptionalAccess = 'OptionalAccess', + UnconditionalAccess = 'UnconditionalAccess', + OptionalDependency = 'OptionalDependency', + UnconditionalDependency = 'UnconditionalDependency', } -const MIN_ACCESS_TYPE = PropertyAccessType.Access; -/** - * "NonNull" means that PropertyReads from a node are side-effect free, - * as the node is (1) immutable and (2) has unconditional propertyloads - * somewhere in the cfg. - */ -function isNonNull(access: PropertyAccessType): boolean { +function isOptional(access: PropertyAccessType): boolean { return ( - access === PropertyAccessType.NonNullAccess || - access === PropertyAccessType.NonNullDependency + access === PropertyAccessType.OptionalAccess || + access === PropertyAccessType.OptionalDependency ); } function isDependency(access: PropertyAccessType): boolean { return ( - access === PropertyAccessType.Dependency || - access === PropertyAccessType.NonNullDependency + access === PropertyAccessType.OptionalDependency || + access === PropertyAccessType.UnconditionalDependency ); } @@ -150,92 +259,70 @@ function merge( access1: PropertyAccessType, access2: PropertyAccessType, ): PropertyAccessType { - const resultisNonNull = isNonNull(access1) || isNonNull(access2); + const resultIsUnconditional = !(isOptional(access1) && isOptional(access2)); const resultIsDependency = isDependency(access1) || isDependency(access2); /* * Straightforward merge. * This can be represented as bitwise OR, but is written out for readability * - * Observe that `NonNullAccess | Dependency` produces an + * Observe that `UnconditionalAccess | ConditionalDependency` produces an * unconditionally accessed conditional dependency. We currently use these * as we use unconditional dependencies. (i.e. to codegen change variables) */ - if (resultisNonNull) { + if (resultIsUnconditional) { if (resultIsDependency) { - return PropertyAccessType.NonNullDependency; + return PropertyAccessType.UnconditionalDependency; } else { - return PropertyAccessType.NonNullAccess; + return PropertyAccessType.UnconditionalAccess; } } else { + // result is optional if (resultIsDependency) { - return PropertyAccessType.Dependency; + return PropertyAccessType.OptionalDependency; } else { - return PropertyAccessType.Access; + return PropertyAccessType.OptionalAccess; } } } -type DependencyNode = { - properties: Map; - accessType: PropertyAccessType; +type TreeNode = { + properties: Map>; + accessType: T; }; +type HoistableNode = TreeNode<'Optional' | 'NonNull'>; +type DependencyNode = TreeNode; -type ReduceResultNode = { - path: Array; -}; - -function assertWellFormedTree(node: DependencyNode): void { - let nonNullInChildren = false; - for (const childNode of node.properties.values()) { - assertWellFormedTree(childNode); - nonNullInChildren ||= isNonNull(childNode.accessType); - } - if (nonNullInChildren) { - CompilerError.invariant(isNonNull(node.accessType), { - reason: - '[DeriveMinimialDependencies] Not well formed tree, unexpected non-null node', - description: node.accessType, - loc: GeneratedSource, - }); - } -} - -function deriveMinimalDependenciesInSubtree( +/** + * TODO: this is directly pasted from DeriveMinimalDependencies. Since we no + * longer have conditionally accessed nodes, we can simplify + * + * Recursively calculates minimal dependencies in a subtree. + * @param node DependencyNode representing a dependency subtree. + * @returns a minimal list of dependencies in this subtree. + */ +function collectMinimalDependenciesInSubtree( node: DependencyNode, - path: Array, -): Array { + rootIdentifier: Identifier, + path: Array, + results: Set, +): void { if (isDependency(node.accessType)) { - /** - * If this node is a dependency, we truncate the subtree - * and return this node. e.g. deps=[`obj.a`, `obj.a.b`] - * reduces to deps=[`obj.a`] - */ - return [{path}]; + results.add({identifier: rootIdentifier, path}); } else { - if (isNonNull(node.accessType)) { - /* - * Only recurse into subtree dependencies if this node - * is known to be non-null. - */ - const result: Array = []; - for (const [childName, childNode] of node.properties) { - result.push( - ...deriveMinimalDependenciesInSubtree(childNode, [ - ...path, - childName, - ]), - ); - } - return result; - } else { - /* - * This only occurs when this subtree contains a dependency, - * but this node is potentially nullish. As we currently - * don't record optional property paths as scope dependencies, - * we truncate and record this node as a dependency. - */ - return [{path}]; + for (const [childName, childNode] of node.properties) { + collectMinimalDependenciesInSubtree( + childNode, + rootIdentifier, + [ + ...path, + { + property: childName, + optional: isOptional(childNode.accessType), + }, + ], + results, + ); } } } 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 615ec18feb962..873082bdbeaa0 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts @@ -367,6 +367,7 @@ export type BasicBlock = { preds: Set; phis: Set; }; +export type TBasicBlock = BasicBlock & {terminal: T}; /* * Terminal nodes generally represent statements that affect control flow, such as diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDependenciesHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDependenciesHIR.ts index 1fe218c352818..a7346e0e6b984 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDependenciesHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDependenciesHIR.ts @@ -17,10 +17,7 @@ import { areEqualPaths, IdentifierId, } from './HIR'; -import { - BlockInfo, - collectHoistablePropertyLoads, -} from './CollectHoistablePropertyLoads'; +import {collectHoistablePropertyLoads} from './CollectHoistablePropertyLoads'; import { ScopeBlockTraversal, eachInstructionOperand, @@ -32,37 +29,61 @@ import {Stack, empty} from '../Utils/Stack'; import {CompilerError} from '../CompilerError'; import {Iterable_some} from '../Utils/utils'; import {ReactiveScopeDependencyTreeHIR} from './DeriveMinimalDependenciesHIR'; +import {collectOptionalChainSidemap} from './CollectOptionalChainDependencies'; export function propagateScopeDependenciesHIR(fn: HIRFunction): void { const usedOutsideDeclaringScope = findTemporariesUsedOutsideDeclaringScope(fn); const temporaries = collectTemporariesSidemap(fn, usedOutsideDeclaringScope); + const { + temporariesReadInOptional, + processedInstrsInOptional, + hoistableObjects, + } = collectOptionalChainSidemap(fn); - const hoistablePropertyLoads = collectHoistablePropertyLoads(fn, temporaries); + const hoistablePropertyLoads = collectHoistablePropertyLoads( + fn, + temporaries, + hoistableObjects, + ); const scopeDeps = collectDependencies( fn, usedOutsideDeclaringScope, - temporaries, + new Map([...temporaries, ...temporariesReadInOptional]), + processedInstrsInOptional, ); /** * Derive the minimal set of hoistable dependencies for each scope. */ for (const [scope, deps] of scopeDeps) { - const tree = new ReactiveScopeDependencyTreeHIR(); + if (deps.length === 0) { + continue; + } /** - * Step 1: Add every dependency used by this scope (e.g. `a.b.c`) + * Step 1: Find hoistable accesses, given the basic block in which the scope + * begins. + */ + const hoistables = hoistablePropertyLoads.get(scope.id); + CompilerError.invariant(hoistables != null, { + reason: '[PropagateScopeDependencies] Scope not found in tracked blocks', + loc: GeneratedSource, + }); + /** + * Step 2: Calculate hoistable dependencies. */ + const tree = new ReactiveScopeDependencyTreeHIR( + [...hoistables.assumedNonNullObjects].map(o => o.fullPath), + ); for (const dep of deps) { tree.addDependency({...dep}); } + /** - * Step 2: Mark hoistable dependencies, given the basic block in - * which the scope begins. + * Step 3: Reduce dependencies to a minimal set. */ - recordHoistablePropertyReads(hoistablePropertyLoads, scope.id, tree); const candidates = tree.deriveMinimalDependencies(); for (const candidateDep of candidates) { if ( @@ -201,7 +222,12 @@ function collectTemporariesSidemap( ); if (value.kind === 'PropertyLoad' && !usedOutside) { - const property = getProperty(value.object, value.property, temporaries); + const property = getProperty( + value.object, + value.property, + false, + temporaries, + ); temporaries.set(lvalue.identifier.id, property); } else if ( value.kind === 'LoadLocal' && @@ -222,6 +248,7 @@ function collectTemporariesSidemap( function getProperty( object: Place, propertyName: string, + optional: boolean, temporaries: ReadonlyMap, ): ReactiveScopeDependency { /* @@ -253,15 +280,12 @@ function getProperty( if (resolvedDependency == null) { property = { identifier: object.identifier, - path: [{property: propertyName, optional: false}], + path: [{property: propertyName, optional}], }; } else { property = { identifier: resolvedDependency.identifier, - path: [ - ...resolvedDependency.path, - {property: propertyName, optional: false}, - ], + path: [...resolvedDependency.path, {property: propertyName, optional}], }; } return property; @@ -409,8 +433,13 @@ class Context { ); } - visitProperty(object: Place, property: string): void { - const nextDependency = getProperty(object, property, this.#temporaries); + visitProperty(object: Place, property: string, optional: boolean): void { + const nextDependency = getProperty( + object, + property, + optional, + this.#temporaries, + ); this.visitDependency(nextDependency); } @@ -489,7 +518,7 @@ function handleInstruction(instr: Instruction, context: Context): void { } } else if (value.kind === 'PropertyLoad') { if (context.isUsedOutsideDeclaringScope(lvalue)) { - context.visitProperty(value.object, value.property); + context.visitProperty(value.object, value.property, false); } } else if (value.kind === 'StoreLocal') { context.visitOperand(value.value); @@ -544,6 +573,7 @@ function collectDependencies( fn: HIRFunction, usedOutsideDeclaringScope: ReadonlySet, temporaries: ReadonlyMap, + processedInstrsInOptional: ReadonlySet, ): Map> { const context = new Context(usedOutsideDeclaringScope, temporaries); @@ -572,33 +602,26 @@ function collectDependencies( context.exitScope(scopeBlockInfo.scope, scopeBlockInfo?.pruned); } + // Record referenced optional chains in phis + for (const phi of block.phis) { + for (const operand of phi.operands) { + const maybeOptionalChain = temporaries.get(operand[1].id); + if (maybeOptionalChain) { + context.visitDependency(maybeOptionalChain); + } + } + } for (const instr of block.instructions) { - handleInstruction(instr, context); + if (!processedInstrsInOptional.has(instr.id)) { + handleInstruction(instr, context); + } } - for (const place of eachTerminalOperand(block.terminal)) { - context.visitOperand(place); + + if (!processedInstrsInOptional.has(block.terminal.id)) { + for (const place of eachTerminalOperand(block.terminal)) { + context.visitOperand(place); + } } } return context.deps; } - -/** - * Compute the set of hoistable property reads. - */ -function recordHoistablePropertyReads( - nodes: ReadonlyMap, - scopeId: ScopeId, - tree: ReactiveScopeDependencyTreeHIR, -): void { - const node = nodes.get(scopeId); - CompilerError.invariant(node != null, { - reason: '[PropagateScopeDependencies] Scope not found in tracked blocks', - loc: GeneratedSource, - }); - - for (const item of node.assumedNonNullObjects) { - tree.markNodesNonNull({ - ...item.fullPath, - }); - } -} diff --git a/compiler/packages/babel-plugin-react-compiler/src/Utils/utils.ts b/compiler/packages/babel-plugin-react-compiler/src/Utils/utils.ts index 6b813d597560c..aa91c48b1b0db 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Utils/utils.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Utils/utils.ts @@ -82,6 +82,17 @@ export function getOrInsertDefault( return defaultValue; } } +export function Set_equal(a: ReadonlySet, b: ReadonlySet): boolean { + if (a.size !== b.size) { + return false; + } + for (const item of a) { + if (!b.has(item)) { + return false; + } + } + return true; +} export function Set_union(a: ReadonlySet, b: ReadonlySet): Set { const union = new Set(a); @@ -128,6 +139,19 @@ export function nonNull, U>( return value != null; } +export function Set_filter( + source: ReadonlySet, + fn: (arg: T) => boolean, +): Set { + const result = new Set(); + for (const entry of source) { + if (fn(entry)) { + result.add(entry); + } + } + return result; +} + export function hasNode( input: NodePath, ): input is NodePath> { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-sequential-optional-chain-nonnull.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-sequential-optional-chain-nonnull.expect.md new file mode 100644 index 0000000000000..31e2cadf9f7db --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-sequential-optional-chain-nonnull.expect.md @@ -0,0 +1,71 @@ + +## Input + +```javascript +function useFoo({a}) { + let x = []; + x.push(a?.b.c?.d.e); + x.push(a.b?.c.d?.e); + return x; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{a: null}], + sequentialRenders: [ + {a: null}, + {a: null}, + {a: {}}, + {a: {b: {c: {d: {e: 42}}}}}, + {a: {b: {c: {d: {e: 43}}}}}, + {a: {b: {c: {d: {e: undefined}}}}}, + {a: {b: undefined}}, + ], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +function useFoo(t0) { + const $ = _c(2); + const { a } = t0; + let x; + if ($[0] !== a.b.c.d) { + x = []; + x.push(a?.b.c?.d.e); + x.push(a.b?.c.d?.e); + $[0] = a.b.c.d; + $[1] = x; + } else { + x = $[1]; + } + return x; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{ a: null }], + sequentialRenders: [ + { a: null }, + { a: null }, + { a: {} }, + { a: { b: { c: { d: { e: 42 } } } } }, + { a: { b: { c: { d: { e: 43 } } } } }, + { a: { b: { c: { d: { e: undefined } } } } }, + { a: { b: undefined } }, + ], +}; + +``` + +### Eval output +(kind: ok) [[ (exception in render) TypeError: Cannot read properties of null (reading 'b') ]] +[[ (exception in render) TypeError: Cannot read properties of null (reading 'b') ]] +[[ (exception in render) TypeError: Cannot read properties of undefined (reading 'c') ]] +[42,42] +[43,43] +[null,null] +[[ (exception in render) TypeError: Cannot read properties of undefined (reading 'c') ]] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-sequential-optional-chain-nonnull.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-sequential-optional-chain-nonnull.ts new file mode 100644 index 0000000000000..479048085e925 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-sequential-optional-chain-nonnull.ts @@ -0,0 +1,20 @@ +function useFoo({a}) { + let x = []; + x.push(a?.b.c?.d.e); + x.push(a.b?.c.d?.e); + return x; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{a: null}], + sequentialRenders: [ + {a: null}, + {a: null}, + {a: {}}, + {a: {b: {c: {d: {e: 42}}}}}, + {a: {b: {c: {d: {e: 43}}}}}, + {a: {b: {c: {d: {e: undefined}}}}}, + {a: {b: undefined}}, + ], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nested-optional-chains.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nested-optional-chains.expect.md new file mode 100644 index 0000000000000..0acf33b2ed87f --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nested-optional-chains.expect.md @@ -0,0 +1,229 @@ + +## Input + +```javascript +import {identity} from 'shared-runtime'; + +/** + * identity(...)?.toString() is the outer optional, and prop?.value is the inner + * one. + * Note that prop?. + */ +function useFoo({ + prop1, + prop2, + prop3, + prop4, + prop5, + prop6, +}: { + prop1: null | {value: number}; + prop2: null | {inner: {value: number}}; + prop3: null | {fn: (val: any) => NonNullable}; + prop4: null | {inner: {value: number}}; + prop5: null | {fn: (val: any) => NonNullable}; + prop6: null | {inner: {value: number}}; +}) { + // prop1?.value should be hoisted as the dependency of x + const x = identity(prop1?.value)?.toString(); + + // prop2?.inner.value should be hoisted as the dependency of y + const y = identity(prop2?.inner.value)?.toString(); + + // prop3 and prop4?.inner should be hoisted as the dependency of z + const z = prop3?.fn(prop4?.inner.value).toString(); + + // prop5 and prop6?.inner should be hoisted as the dependency of zz + const zz = prop5?.fn(prop6?.inner.value)?.toString(); + return [x, y, z, zz]; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [ + { + prop1: null, + prop2: null, + prop3: null, + prop4: null, + prop5: null, + prop6: null, + }, + ], + sequentialRenders: [ + { + prop1: null, + prop2: null, + prop3: null, + prop4: null, + prop5: null, + prop6: null, + }, + { + prop1: {value: 2}, + prop2: {inner: {value: 3}}, + prop3: {fn: identity}, + prop4: {inner: {value: 4}}, + prop5: {fn: identity}, + prop6: {inner: {value: 4}}, + }, + { + prop1: {value: 2}, + prop2: {inner: {value: 3}}, + prop3: {fn: identity}, + prop4: {inner: {value: 4}}, + prop5: {fn: identity}, + prop6: {inner: {value: undefined}}, + }, + { + prop1: {value: 2}, + prop2: {inner: {value: undefined}}, + prop3: {fn: identity}, + prop4: {inner: {value: undefined}}, + prop5: {fn: identity}, + prop6: {inner: {value: undefined}}, + }, + { + prop1: {value: 2}, + prop2: {}, + prop3: {fn: identity}, + prop4: {}, + prop5: {fn: identity}, + prop6: {inner: {value: undefined}}, + }, + ], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { identity } from "shared-runtime"; + +/** + * identity(...)?.toString() is the outer optional, and prop?.value is the inner + * one. + * Note that prop?. + */ +function useFoo(t0) { + const $ = _c(15); + const { prop1, prop2, prop3, prop4, prop5, prop6 } = t0; + let t1; + if ($[0] !== prop1?.value) { + t1 = identity(prop1?.value)?.toString(); + $[0] = prop1?.value; + $[1] = t1; + } else { + t1 = $[1]; + } + const x = t1; + let t2; + if ($[2] !== prop2?.inner) { + t2 = identity(prop2?.inner.value)?.toString(); + $[2] = prop2?.inner; + $[3] = t2; + } else { + t2 = $[3]; + } + const y = t2; + let t3; + if ($[4] !== prop3 || $[5] !== prop4) { + t3 = prop3?.fn(prop4?.inner.value).toString(); + $[4] = prop3; + $[5] = prop4; + $[6] = t3; + } else { + t3 = $[6]; + } + const z = t3; + let t4; + if ($[7] !== prop5 || $[8] !== prop6) { + t4 = prop5?.fn(prop6?.inner.value)?.toString(); + $[7] = prop5; + $[8] = prop6; + $[9] = t4; + } else { + t4 = $[9]; + } + const zz = t4; + let t5; + if ($[10] !== x || $[11] !== y || $[12] !== z || $[13] !== zz) { + t5 = [x, y, z, zz]; + $[10] = x; + $[11] = y; + $[12] = z; + $[13] = zz; + $[14] = t5; + } else { + t5 = $[14]; + } + return t5; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [ + { + prop1: null, + prop2: null, + prop3: null, + prop4: null, + prop5: null, + prop6: null, + }, + ], + + sequentialRenders: [ + { + prop1: null, + prop2: null, + prop3: null, + prop4: null, + prop5: null, + prop6: null, + }, + { + prop1: { value: 2 }, + prop2: { inner: { value: 3 } }, + prop3: { fn: identity }, + prop4: { inner: { value: 4 } }, + prop5: { fn: identity }, + prop6: { inner: { value: 4 } }, + }, + { + prop1: { value: 2 }, + prop2: { inner: { value: 3 } }, + prop3: { fn: identity }, + prop4: { inner: { value: 4 } }, + prop5: { fn: identity }, + prop6: { inner: { value: undefined } }, + }, + { + prop1: { value: 2 }, + prop2: { inner: { value: undefined } }, + prop3: { fn: identity }, + prop4: { inner: { value: undefined } }, + prop5: { fn: identity }, + prop6: { inner: { value: undefined } }, + }, + { + prop1: { value: 2 }, + prop2: {}, + prop3: { fn: identity }, + prop4: {}, + prop5: { fn: identity }, + prop6: { inner: { value: undefined } }, + }, + ], +}; + +``` + +### Eval output +(kind: ok) [null,null,null,null] +["2","3","4","4"] +["2","3","4",null] +[[ (exception in render) TypeError: Cannot read properties of undefined (reading 'toString') ]] +[[ (exception in render) TypeError: Cannot read properties of undefined (reading 'value') ]] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nested-optional-chains.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nested-optional-chains.ts new file mode 100644 index 0000000000000..d00cb4fee6983 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nested-optional-chains.ts @@ -0,0 +1,91 @@ +import {identity} from 'shared-runtime'; + +/** + * identity(...)?.toString() is the outer optional, and prop?.value is the inner + * one. + * Note that prop?. + */ +function useFoo({ + prop1, + prop2, + prop3, + prop4, + prop5, + prop6, +}: { + prop1: null | {value: number}; + prop2: null | {inner: {value: number}}; + prop3: null | {fn: (val: any) => NonNullable}; + prop4: null | {inner: {value: number}}; + prop5: null | {fn: (val: any) => NonNullable}; + prop6: null | {inner: {value: number}}; +}) { + // prop1?.value should be hoisted as the dependency of x + const x = identity(prop1?.value)?.toString(); + + // prop2?.inner.value should be hoisted as the dependency of y + const y = identity(prop2?.inner.value)?.toString(); + + // prop3 and prop4?.inner should be hoisted as the dependency of z + const z = prop3?.fn(prop4?.inner.value).toString(); + + // prop5 and prop6?.inner should be hoisted as the dependency of zz + const zz = prop5?.fn(prop6?.inner.value)?.toString(); + return [x, y, z, zz]; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [ + { + prop1: null, + prop2: null, + prop3: null, + prop4: null, + prop5: null, + prop6: null, + }, + ], + sequentialRenders: [ + { + prop1: null, + prop2: null, + prop3: null, + prop4: null, + prop5: null, + prop6: null, + }, + { + prop1: {value: 2}, + prop2: {inner: {value: 3}}, + prop3: {fn: identity}, + prop4: {inner: {value: 4}}, + prop5: {fn: identity}, + prop6: {inner: {value: 4}}, + }, + { + prop1: {value: 2}, + prop2: {inner: {value: 3}}, + prop3: {fn: identity}, + prop4: {inner: {value: 4}}, + prop5: {fn: identity}, + prop6: {inner: {value: undefined}}, + }, + { + prop1: {value: 2}, + prop2: {inner: {value: undefined}}, + prop3: {fn: identity}, + prop4: {inner: {value: undefined}}, + prop5: {fn: identity}, + prop6: {inner: {value: undefined}}, + }, + { + prop1: {value: 2}, + prop2: {}, + prop3: {fn: identity}, + prop4: {}, + prop5: {fn: identity}, + prop6: {inner: {value: undefined}}, + }, + ], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-as-memo-dep.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-as-memo-dep.expect.md index c34b79a848ba8..77875f789d7a8 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-as-memo-dep.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-as-memo-dep.expect.md @@ -3,12 +3,29 @@ ```javascript // @validatePreserveExistingMemoizationGuarantees @enableOptionalDependencies -function Component(props) { +import {identity, ValidateMemoization} from 'shared-runtime'; +import {useMemo} from 'react'; + +function Component({arg}) { const data = useMemo(() => { - return props?.items.edges?.nodes.map(); - }, [props?.items.edges?.nodes]); - return ; + return arg?.items.edges?.nodes.map(identity); + }, [arg?.items.edges?.nodes]); + return ( + + ); } +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{arg: null}], + sequentialRenders: [ + {arg: null}, + {arg: null}, + {arg: {items: {edges: null}}}, + {arg: {items: {edges: null}}}, + {arg: {items: {edges: {nodes: [1, 2, 'hello']}}}}, + {arg: {items: {edges: {nodes: [1, 2, 'hello']}}}}, + ], +}; ``` @@ -16,33 +33,66 @@ function Component(props) { ```javascript import { c as _c } from "react/compiler-runtime"; // @validatePreserveExistingMemoizationGuarantees @enableOptionalDependencies -function Component(props) { - const $ = _c(4); +import { identity, ValidateMemoization } from "shared-runtime"; +import { useMemo } from "react"; + +function Component(t0) { + const $ = _c(7); + const { arg } = t0; - props?.items.edges?.nodes; - let t0; + arg?.items.edges?.nodes; let t1; - if ($[0] !== props?.items.edges?.nodes) { - t1 = props?.items.edges?.nodes.map(); - $[0] = props?.items.edges?.nodes; - $[1] = t1; + let t2; + if ($[0] !== arg?.items.edges?.nodes) { + t2 = arg?.items.edges?.nodes.map(identity); + $[0] = arg?.items.edges?.nodes; + $[1] = t2; } else { - t1 = $[1]; + t2 = $[1]; } - t0 = t1; - const data = t0; - let t2; - if ($[2] !== data) { - t2 = ; - $[2] = data; - $[3] = t2; + t1 = t2; + const data = t1; + + const t3 = arg?.items.edges?.nodes; + let t4; + if ($[2] !== t3) { + t4 = [t3]; + $[2] = t3; + $[3] = t4; + } else { + t4 = $[3]; + } + let t5; + if ($[4] !== t4 || $[5] !== data) { + t5 = ; + $[4] = t4; + $[5] = data; + $[6] = t5; } else { - t2 = $[3]; + t5 = $[6]; } - return t2; + return t5; } +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ arg: null }], + sequentialRenders: [ + { arg: null }, + { arg: null }, + { arg: { items: { edges: null } } }, + { arg: { items: { edges: null } } }, + { arg: { items: { edges: { nodes: [1, 2, "hello"] } } } }, + { arg: { items: { edges: { nodes: [1, 2, "hello"] } } } }, + ], +}; + ``` ### Eval output -(kind: exception) Fixture not implemented \ No newline at end of file +(kind: ok)
{"inputs":[null]}
+
{"inputs":[null]}
+
{"inputs":[null]}
+
{"inputs":[null]}
+
{"inputs":[[1,2,"hello"]],"output":[1,2,"hello"]}
+
{"inputs":[[1,2,"hello"]],"output":[1,2,"hello"]}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-as-memo-dep.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-as-memo-dep.js index d82d36b547970..73f0f4d421a4c 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-as-memo-dep.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-as-memo-dep.js @@ -1,7 +1,24 @@ // @validatePreserveExistingMemoizationGuarantees @enableOptionalDependencies -function Component(props) { +import {identity, ValidateMemoization} from 'shared-runtime'; +import {useMemo} from 'react'; + +function Component({arg}) { const data = useMemo(() => { - return props?.items.edges?.nodes.map(); - }, [props?.items.edges?.nodes]); - return ; + return arg?.items.edges?.nodes.map(identity); + }, [arg?.items.edges?.nodes]); + return ( + + ); } +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{arg: null}], + sequentialRenders: [ + {arg: null}, + {arg: null}, + {arg: {items: {edges: null}}}, + {arg: {items: {edges: null}}}, + {arg: {items: {edges: {nodes: [1, 2, 'hello']}}}}, + {arg: {items: {edges: {nodes: [1, 2, 'hello']}}}}, + ], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-single.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-single.expect.md index a4cf6d767d1c3..6e44a97b456b3 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-single.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-single.expect.md @@ -4,15 +4,27 @@ ```javascript // @validatePreserveExistingMemoizationGuarantees @enableOptionalDependencies import {ValidateMemoization} from 'shared-runtime'; -function Component(props) { +import {useMemo} from 'react'; +function Component({arg}) { const data = useMemo(() => { const x = []; - x.push(props?.items); + x.push(arg?.items); return x; - }, [props?.items]); - return ; + }, [arg?.items]); + return ; } +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{arg: {items: 2}}], + sequentialRenders: [ + {arg: {items: 2}}, + {arg: {items: 2}}, + {arg: null}, + {arg: null}, + ], +}; + ``` ## Code @@ -20,44 +32,60 @@ function Component(props) { ```javascript import { c as _c } from "react/compiler-runtime"; // @validatePreserveExistingMemoizationGuarantees @enableOptionalDependencies import { ValidateMemoization } from "shared-runtime"; -function Component(props) { +import { useMemo } from "react"; +function Component(t0) { const $ = _c(7); + const { arg } = t0; - props?.items; - let t0; + arg?.items; + let t1; let x; - if ($[0] !== props?.items) { + if ($[0] !== arg?.items) { x = []; - x.push(props?.items); - $[0] = props?.items; + x.push(arg?.items); + $[0] = arg?.items; $[1] = x; } else { x = $[1]; } - t0 = x; - const data = t0; - const t1 = props?.items; - let t2; - if ($[2] !== t1) { - t2 = [t1]; - $[2] = t1; - $[3] = t2; + t1 = x; + const data = t1; + const t2 = arg?.items; + let t3; + if ($[2] !== t2) { + t3 = [t2]; + $[2] = t2; + $[3] = t3; } else { - t2 = $[3]; + t3 = $[3]; } - let t3; - if ($[4] !== t2 || $[5] !== data) { - t3 = ; - $[4] = t2; + let t4; + if ($[4] !== t3 || $[5] !== data) { + t4 = ; + $[4] = t3; $[5] = data; - $[6] = t3; + $[6] = t4; } else { - t3 = $[6]; + t4 = $[6]; } - return t3; + return t4; } +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ arg: { items: 2 } }], + sequentialRenders: [ + { arg: { items: 2 } }, + { arg: { items: 2 } }, + { arg: null }, + { arg: null }, + ], +}; + ``` ### Eval output -(kind: exception) Fixture not implemented \ No newline at end of file +(kind: ok)
{"inputs":[2],"output":[2]}
+
{"inputs":[2],"output":[2]}
+
{"inputs":[null],"output":[null]}
+
{"inputs":[null],"output":[null]}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-single.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-single.js index 5750d7af3a0e0..62ac31dd6d1b1 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-single.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-single.js @@ -1,10 +1,22 @@ // @validatePreserveExistingMemoizationGuarantees @enableOptionalDependencies import {ValidateMemoization} from 'shared-runtime'; -function Component(props) { +import {useMemo} from 'react'; +function Component({arg}) { const data = useMemo(() => { const x = []; - x.push(props?.items); + x.push(arg?.items); return x; - }, [props?.items]); - return ; + }, [arg?.items]); + return ; } + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{arg: {items: 2}}], + sequentialRenders: [ + {arg: {items: 2}}, + {arg: {items: 2}}, + {arg: null}, + {arg: null}, + ], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/error.todo-optional-call-chain-in-optional.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/error.todo-optional-call-chain-in-optional.expect.md index 8b52187920cbe..e0196bdc1945c 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/error.todo-optional-call-chain-in-optional.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/error.todo-optional-call-chain-in-optional.expect.md @@ -26,7 +26,7 @@ export const FIXTURE_ENTRYPONT = { 2 | function useFoo(props: {value: {x: string; y: string} | null}) { 3 | const value = props.value; > 4 | return createArray(value?.x, value?.y)?.join(', '); - | ^^^^^^^^ Todo: Unexpected terminal kind `optional` for optional test block (4:4) + | ^^^^^^^^ Todo: Unexpected terminal kind `optional` for optional fallthrough block (4:4) 5 | } 6 | 7 | function createArray(...args: Array): Array { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/error.todo-optional-member-expression-as-memo-dep.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/error.todo-optional-member-expression-as-memo-dep.expect.md deleted file mode 100644 index e885982310117..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/error.todo-optional-member-expression-as-memo-dep.expect.md +++ /dev/null @@ -1,32 +0,0 @@ - -## Input - -```javascript -// @validatePreserveExistingMemoizationGuarantees @enableOptionalDependencies @enablePropagateDepsInHIR -function Component(props) { - const data = useMemo(() => { - return props?.items.edges?.nodes.map(); - }, [props?.items.edges?.nodes]); - return ; -} - -``` - - -## Error - -``` - 1 | // @validatePreserveExistingMemoizationGuarantees @enableOptionalDependencies @enablePropagateDepsInHIR - 2 | function Component(props) { -> 3 | const data = useMemo(() => { - | ^^^^^^^ -> 4 | return props?.items.edges?.nodes.map(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -> 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/propagate-scope-deps-hir-fork/error.todo-optional-member-expression-as-memo-dep.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/error.todo-optional-member-expression-as-memo-dep.js deleted file mode 100644 index 6ff87d0c46329..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/error.todo-optional-member-expression-as-memo-dep.js +++ /dev/null @@ -1,7 +0,0 @@ -// @validatePreserveExistingMemoizationGuarantees @enableOptionalDependencies @enablePropagateDepsInHIR -function Component(props) { - const data = useMemo(() => { - return props?.items.edges?.nodes.map(); - }, [props?.items.edges?.nodes]); - return ; -} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/error.todo-optional-member-expression-single-with-unconditional.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/error.todo-optional-member-expression-single-with-unconditional.expect.md deleted file mode 100644 index 3559b2bd58b28..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/error.todo-optional-member-expression-single-with-unconditional.expect.md +++ /dev/null @@ -1,42 +0,0 @@ - -## Input - -```javascript -// @validatePreserveExistingMemoizationGuarantees @enableOptionalDependencies @enablePropagateDepsInHIR -import {ValidateMemoization} from 'shared-runtime'; -function Component(props) { - const data = useMemo(() => { - const x = []; - x.push(props?.items); - x.push(props.items); - return x; - }, [props.items]); - return ; -} - -``` - - -## Error - -``` - 2 | import {ValidateMemoization} from 'shared-runtime'; - 3 | function Component(props) { -> 4 | const data = useMemo(() => { - | ^^^^^^^ -> 5 | const x = []; - | ^^^^^^^^^^^^^^^^^ -> 6 | x.push(props?.items); - | ^^^^^^^^^^^^^^^^^ -> 7 | x.push(props.items); - | ^^^^^^^^^^^^^^^^^ -> 8 | return x; - | ^^^^^^^^^^^^^^^^^ -> 9 | }, [props.items]); - | ^^^^ 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:9) - 10 | return ; - 11 | } - 12 | -``` - - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/error.todo-optional-member-expression-single.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/error.todo-optional-member-expression-single.expect.md deleted file mode 100644 index 429f168836b82..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/error.todo-optional-member-expression-single.expect.md +++ /dev/null @@ -1,39 +0,0 @@ - -## Input - -```javascript -// @validatePreserveExistingMemoizationGuarantees @enableOptionalDependencies @enablePropagateDepsInHIR -import {ValidateMemoization} from 'shared-runtime'; -function Component(props) { - const data = useMemo(() => { - const x = []; - x.push(props?.items); - return x; - }, [props?.items]); - return ; -} - -``` - - -## Error - -``` - 2 | import {ValidateMemoization} from 'shared-runtime'; - 3 | function Component(props) { -> 4 | const data = useMemo(() => { - | ^^^^^^^ -> 5 | const x = []; - | ^^^^^^^^^^^^^^^^^ -> 6 | x.push(props?.items); - | ^^^^^^^^^^^^^^^^^ -> 7 | return x; - | ^^^^^^^^^^^^^^^^^ -> 8 | }, [props?.items]); - | ^^^^ 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:8) - 9 | return ; - 10 | } - 11 | -``` - - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/error.todo-optional-member-expression-single.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/error.todo-optional-member-expression-single.js deleted file mode 100644 index 535a0ce074419..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/error.todo-optional-member-expression-single.js +++ /dev/null @@ -1,10 +0,0 @@ -// @validatePreserveExistingMemoizationGuarantees @enableOptionalDependencies @enablePropagateDepsInHIR -import {ValidateMemoization} from 'shared-runtime'; -function Component(props) { - const data = useMemo(() => { - const x = []; - x.push(props?.items); - return x; - }, [props?.items]); - return ; -} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/infer-sequential-optional-chain-nonnull.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/infer-sequential-optional-chain-nonnull.expect.md new file mode 100644 index 0000000000000..757ad2666d962 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/infer-sequential-optional-chain-nonnull.expect.md @@ -0,0 +1,74 @@ + +## Input + +```javascript +// @enablePropagateDepsInHIR + +function useFoo({a}) { + let x = []; + x.push(a?.b.c?.d.e); + x.push(a.b?.c.d?.e); + return x; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{a: null}], + sequentialRenders: [ + {a: null}, + {a: null}, + {a: {}}, + {a: {b: {c: {d: {e: 42}}}}}, + {a: {b: {c: {d: {e: 43}}}}}, + {a: {b: {c: {d: {e: undefined}}}}}, + {a: {b: undefined}}, + ], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @enablePropagateDepsInHIR + +function useFoo(t0) { + const $ = _c(2); + const { a } = t0; + let x; + if ($[0] !== a.b.c.d.e) { + x = []; + x.push(a?.b.c?.d.e); + x.push(a.b?.c.d?.e); + $[0] = a.b.c.d.e; + $[1] = x; + } else { + x = $[1]; + } + return x; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{ a: null }], + sequentialRenders: [ + { a: null }, + { a: null }, + { a: {} }, + { a: { b: { c: { d: { e: 42 } } } } }, + { a: { b: { c: { d: { e: 43 } } } } }, + { a: { b: { c: { d: { e: undefined } } } } }, + { a: { b: undefined } }, + ], +}; + +``` + +### Eval output +(kind: ok) [[ (exception in render) TypeError: Cannot read properties of null (reading 'b') ]] +[[ (exception in render) TypeError: Cannot read properties of null (reading 'b') ]] +[[ (exception in render) TypeError: Cannot read properties of undefined (reading 'c') ]] +[42,42] +[43,43] +[null,null] +[[ (exception in render) TypeError: Cannot read properties of undefined (reading 'c') ]] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/infer-sequential-optional-chain-nonnull.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/infer-sequential-optional-chain-nonnull.ts new file mode 100644 index 0000000000000..750e422861c3a --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/infer-sequential-optional-chain-nonnull.ts @@ -0,0 +1,22 @@ +// @enablePropagateDepsInHIR + +function useFoo({a}) { + let x = []; + x.push(a?.b.c?.d.e); + x.push(a.b?.c.d?.e); + return x; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{a: null}], + sequentialRenders: [ + {a: null}, + {a: null}, + {a: {}}, + {a: {b: {c: {d: {e: 42}}}}}, + {a: {b: {c: {d: {e: 43}}}}}, + {a: {b: {c: {d: {e: undefined}}}}}, + {a: {b: undefined}}, + ], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/nested-optional-chains.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/nested-optional-chains.expect.md new file mode 100644 index 0000000000000..56b987c677e20 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/nested-optional-chains.expect.md @@ -0,0 +1,232 @@ + +## Input + +```javascript +// @enablePropagateDepsInHIR + +import {identity} from 'shared-runtime'; + +/** + * identity(...)?.toString() is the outer optional, and prop?.value is the inner + * one. + * Note that prop?. + */ +function useFoo({ + prop1, + prop2, + prop3, + prop4, + prop5, + prop6, +}: { + prop1: null | {value: number}; + prop2: null | {inner: {value: number}}; + prop3: null | {fn: (val: any) => NonNullable}; + prop4: null | {inner: {value: number}}; + prop5: null | {fn: (val: any) => NonNullable}; + prop6: null | {inner: {value: number}}; +}) { + // prop1?.value should be hoisted as the dependency of x + const x = identity(prop1?.value)?.toString(); + + // prop2?.inner.value should be hoisted as the dependency of y + const y = identity(prop2?.inner.value)?.toString(); + + // prop3 and prop4?.inner should be hoisted as the dependency of z + const z = prop3?.fn(prop4?.inner.value).toString(); + + // prop5 and prop6?.inner should be hoisted as the dependency of zz + const zz = prop5?.fn(prop6?.inner.value)?.toString(); + return [x, y, z, zz]; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [ + { + prop1: null, + prop2: null, + prop3: null, + prop4: null, + prop5: null, + prop6: null, + }, + ], + sequentialRenders: [ + { + prop1: null, + prop2: null, + prop3: null, + prop4: null, + prop5: null, + prop6: null, + }, + { + prop1: {value: 2}, + prop2: {inner: {value: 3}}, + prop3: {fn: identity}, + prop4: {inner: {value: 4}}, + prop5: {fn: identity}, + prop6: {inner: {value: 4}}, + }, + { + prop1: {value: 2}, + prop2: {inner: {value: 3}}, + prop3: {fn: identity}, + prop4: {inner: {value: 4}}, + prop5: {fn: identity}, + prop6: {inner: {value: undefined}}, + }, + { + prop1: {value: 2}, + prop2: {inner: {value: undefined}}, + prop3: {fn: identity}, + prop4: {inner: {value: undefined}}, + prop5: {fn: identity}, + prop6: {inner: {value: undefined}}, + }, + { + prop1: {value: 2}, + prop2: {}, + prop3: {fn: identity}, + prop4: {}, + prop5: {fn: identity}, + prop6: {inner: {value: undefined}}, + }, + ], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @enablePropagateDepsInHIR + +import { identity } from "shared-runtime"; + +/** + * identity(...)?.toString() is the outer optional, and prop?.value is the inner + * one. + * Note that prop?. + */ +function useFoo(t0) { + const $ = _c(15); + const { prop1, prop2, prop3, prop4, prop5, prop6 } = t0; + let t1; + if ($[0] !== prop1?.value) { + t1 = identity(prop1?.value)?.toString(); + $[0] = prop1?.value; + $[1] = t1; + } else { + t1 = $[1]; + } + const x = t1; + let t2; + if ($[2] !== prop2?.inner.value) { + t2 = identity(prop2?.inner.value)?.toString(); + $[2] = prop2?.inner.value; + $[3] = t2; + } else { + t2 = $[3]; + } + const y = t2; + let t3; + if ($[4] !== prop3 || $[5] !== prop4?.inner) { + t3 = prop3?.fn(prop4?.inner.value).toString(); + $[4] = prop3; + $[5] = prop4?.inner; + $[6] = t3; + } else { + t3 = $[6]; + } + const z = t3; + let t4; + if ($[7] !== prop5 || $[8] !== prop6?.inner) { + t4 = prop5?.fn(prop6?.inner.value)?.toString(); + $[7] = prop5; + $[8] = prop6?.inner; + $[9] = t4; + } else { + t4 = $[9]; + } + const zz = t4; + let t5; + if ($[10] !== x || $[11] !== y || $[12] !== z || $[13] !== zz) { + t5 = [x, y, z, zz]; + $[10] = x; + $[11] = y; + $[12] = z; + $[13] = zz; + $[14] = t5; + } else { + t5 = $[14]; + } + return t5; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [ + { + prop1: null, + prop2: null, + prop3: null, + prop4: null, + prop5: null, + prop6: null, + }, + ], + + sequentialRenders: [ + { + prop1: null, + prop2: null, + prop3: null, + prop4: null, + prop5: null, + prop6: null, + }, + { + prop1: { value: 2 }, + prop2: { inner: { value: 3 } }, + prop3: { fn: identity }, + prop4: { inner: { value: 4 } }, + prop5: { fn: identity }, + prop6: { inner: { value: 4 } }, + }, + { + prop1: { value: 2 }, + prop2: { inner: { value: 3 } }, + prop3: { fn: identity }, + prop4: { inner: { value: 4 } }, + prop5: { fn: identity }, + prop6: { inner: { value: undefined } }, + }, + { + prop1: { value: 2 }, + prop2: { inner: { value: undefined } }, + prop3: { fn: identity }, + prop4: { inner: { value: undefined } }, + prop5: { fn: identity }, + prop6: { inner: { value: undefined } }, + }, + { + prop1: { value: 2 }, + prop2: {}, + prop3: { fn: identity }, + prop4: {}, + prop5: { fn: identity }, + prop6: { inner: { value: undefined } }, + }, + ], +}; + +``` + +### Eval output +(kind: ok) [null,null,null,null] +["2","3","4","4"] +["2","3","4",null] +[[ (exception in render) TypeError: Cannot read properties of undefined (reading 'toString') ]] +[[ (exception in render) TypeError: Cannot read properties of undefined (reading 'value') ]] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/nested-optional-chains.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/nested-optional-chains.ts new file mode 100644 index 0000000000000..48f3b2de2a72b --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/nested-optional-chains.ts @@ -0,0 +1,93 @@ +// @enablePropagateDepsInHIR + +import {identity} from 'shared-runtime'; + +/** + * identity(...)?.toString() is the outer optional, and prop?.value is the inner + * one. + * Note that prop?. + */ +function useFoo({ + prop1, + prop2, + prop3, + prop4, + prop5, + prop6, +}: { + prop1: null | {value: number}; + prop2: null | {inner: {value: number}}; + prop3: null | {fn: (val: any) => NonNullable}; + prop4: null | {inner: {value: number}}; + prop5: null | {fn: (val: any) => NonNullable}; + prop6: null | {inner: {value: number}}; +}) { + // prop1?.value should be hoisted as the dependency of x + const x = identity(prop1?.value)?.toString(); + + // prop2?.inner.value should be hoisted as the dependency of y + const y = identity(prop2?.inner.value)?.toString(); + + // prop3 and prop4?.inner should be hoisted as the dependency of z + const z = prop3?.fn(prop4?.inner.value).toString(); + + // prop5 and prop6?.inner should be hoisted as the dependency of zz + const zz = prop5?.fn(prop6?.inner.value)?.toString(); + return [x, y, z, zz]; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [ + { + prop1: null, + prop2: null, + prop3: null, + prop4: null, + prop5: null, + prop6: null, + }, + ], + sequentialRenders: [ + { + prop1: null, + prop2: null, + prop3: null, + prop4: null, + prop5: null, + prop6: null, + }, + { + prop1: {value: 2}, + prop2: {inner: {value: 3}}, + prop3: {fn: identity}, + prop4: {inner: {value: 4}}, + prop5: {fn: identity}, + prop6: {inner: {value: 4}}, + }, + { + prop1: {value: 2}, + prop2: {inner: {value: 3}}, + prop3: {fn: identity}, + prop4: {inner: {value: 4}}, + prop5: {fn: identity}, + prop6: {inner: {value: undefined}}, + }, + { + prop1: {value: 2}, + prop2: {inner: {value: undefined}}, + prop3: {fn: identity}, + prop4: {inner: {value: undefined}}, + prop5: {fn: identity}, + prop6: {inner: {value: undefined}}, + }, + { + prop1: {value: 2}, + prop2: {}, + prop3: {fn: identity}, + prop4: {}, + prop5: {fn: identity}, + prop6: {inner: {value: undefined}}, + }, + ], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/optional-member-expression-as-memo-dep.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/optional-member-expression-as-memo-dep.expect.md new file mode 100644 index 0000000000000..d0486cd8c29fd --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/optional-member-expression-as-memo-dep.expect.md @@ -0,0 +1,98 @@ + +## Input + +```javascript +// @validatePreserveExistingMemoizationGuarantees @enableOptionalDependencies @enablePropagateDepsInHIR +import {identity, ValidateMemoization} from 'shared-runtime'; +import {useMemo} from 'react'; + +function Component({arg}) { + const data = useMemo(() => { + return arg?.items.edges?.nodes.map(identity); + }, [arg?.items.edges?.nodes]); + return ( + + ); +} +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{arg: null}], + sequentialRenders: [ + {arg: null}, + {arg: null}, + {arg: {items: {edges: null}}}, + {arg: {items: {edges: null}}}, + {arg: {items: {edges: {nodes: [1, 2, 'hello']}}}}, + {arg: {items: {edges: {nodes: [1, 2, 'hello']}}}}, + ], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validatePreserveExistingMemoizationGuarantees @enableOptionalDependencies @enablePropagateDepsInHIR +import { identity, ValidateMemoization } from "shared-runtime"; +import { useMemo } from "react"; + +function Component(t0) { + const $ = _c(7); + const { arg } = t0; + + arg?.items.edges?.nodes; + let t1; + let t2; + if ($[0] !== arg?.items.edges?.nodes) { + t2 = arg?.items.edges?.nodes.map(identity); + $[0] = arg?.items.edges?.nodes; + $[1] = t2; + } else { + t2 = $[1]; + } + t1 = t2; + const data = t1; + + const t3 = arg?.items.edges?.nodes; + let t4; + if ($[2] !== t3) { + t4 = [t3]; + $[2] = t3; + $[3] = t4; + } else { + t4 = $[3]; + } + let t5; + if ($[4] !== t4 || $[5] !== data) { + t5 = ; + $[4] = t4; + $[5] = data; + $[6] = t5; + } else { + t5 = $[6]; + } + return t5; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ arg: null }], + sequentialRenders: [ + { arg: null }, + { arg: null }, + { arg: { items: { edges: null } } }, + { arg: { items: { edges: null } } }, + { arg: { items: { edges: { nodes: [1, 2, "hello"] } } } }, + { arg: { items: { edges: { nodes: [1, 2, "hello"] } } } }, + ], +}; + +``` + +### Eval output +(kind: ok)
{"inputs":[null]}
+
{"inputs":[null]}
+
{"inputs":[null]}
+
{"inputs":[null]}
+
{"inputs":[[1,2,"hello"]],"output":[1,2,"hello"]}
+
{"inputs":[[1,2,"hello"]],"output":[1,2,"hello"]}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/optional-member-expression-as-memo-dep.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/optional-member-expression-as-memo-dep.js new file mode 100644 index 0000000000000..d248c472f5aa7 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/optional-member-expression-as-memo-dep.js @@ -0,0 +1,24 @@ +// @validatePreserveExistingMemoizationGuarantees @enableOptionalDependencies @enablePropagateDepsInHIR +import {identity, ValidateMemoization} from 'shared-runtime'; +import {useMemo} from 'react'; + +function Component({arg}) { + const data = useMemo(() => { + return arg?.items.edges?.nodes.map(identity); + }, [arg?.items.edges?.nodes]); + return ( + + ); +} +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{arg: null}], + sequentialRenders: [ + {arg: null}, + {arg: null}, + {arg: {items: {edges: null}}}, + {arg: {items: {edges: null}}}, + {arg: {items: {edges: {nodes: [1, 2, 'hello']}}}}, + {arg: {items: {edges: {nodes: [1, 2, 'hello']}}}}, + ], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/optional-member-expression-single-with-unconditional.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/optional-member-expression-single-with-unconditional.expect.md new file mode 100644 index 0000000000000..b4a55fcb61eee --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/optional-member-expression-single-with-unconditional.expect.md @@ -0,0 +1,62 @@ + +## Input + +```javascript +// @validatePreserveExistingMemoizationGuarantees @enableOptionalDependencies @enablePropagateDepsInHIR +import {ValidateMemoization} from 'shared-runtime'; +function Component(props) { + const data = useMemo(() => { + const x = []; + x.push(props?.items); + x.push(props.items); + return x; + }, [props.items]); + return ; +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validatePreserveExistingMemoizationGuarantees @enableOptionalDependencies @enablePropagateDepsInHIR +import { ValidateMemoization } from "shared-runtime"; +function Component(props) { + const $ = _c(7); + let t0; + let x; + if ($[0] !== props.items) { + x = []; + x.push(props?.items); + x.push(props.items); + $[0] = props.items; + $[1] = x; + } else { + x = $[1]; + } + t0 = x; + const data = t0; + let t1; + if ($[2] !== props.items) { + t1 = [props.items]; + $[2] = props.items; + $[3] = t1; + } else { + t1 = $[3]; + } + let t2; + if ($[4] !== t1 || $[5] !== data) { + t2 = ; + $[4] = t1; + $[5] = data; + $[6] = t2; + } else { + t2 = $[6]; + } + return t2; +} + +``` + +### 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/propagate-scope-deps-hir-fork/error.todo-optional-member-expression-single-with-unconditional.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/optional-member-expression-single-with-unconditional.js similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/error.todo-optional-member-expression-single-with-unconditional.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/optional-member-expression-single-with-unconditional.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/optional-member-expression-single.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/optional-member-expression-single.expect.md new file mode 100644 index 0000000000000..f15b9b8e9b816 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/optional-member-expression-single.expect.md @@ -0,0 +1,91 @@ + +## Input + +```javascript +// @validatePreserveExistingMemoizationGuarantees @enableOptionalDependencies @enablePropagateDepsInHIR +import {ValidateMemoization} from 'shared-runtime'; +import {useMemo} from 'react'; +function Component({arg}) { + const data = useMemo(() => { + const x = []; + x.push(arg?.items); + return x; + }, [arg?.items]); + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{arg: {items: 2}}], + sequentialRenders: [ + {arg: {items: 2}}, + {arg: {items: 2}}, + {arg: null}, + {arg: null}, + ], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validatePreserveExistingMemoizationGuarantees @enableOptionalDependencies @enablePropagateDepsInHIR +import { ValidateMemoization } from "shared-runtime"; +import { useMemo } from "react"; +function Component(t0) { + const $ = _c(7); + const { arg } = t0; + + arg?.items; + let t1; + let x; + if ($[0] !== arg?.items) { + x = []; + x.push(arg?.items); + $[0] = arg?.items; + $[1] = x; + } else { + x = $[1]; + } + t1 = x; + const data = t1; + const t2 = arg?.items; + let t3; + if ($[2] !== t2) { + t3 = [t2]; + $[2] = t2; + $[3] = t3; + } else { + t3 = $[3]; + } + let t4; + if ($[4] !== t3 || $[5] !== data) { + t4 = ; + $[4] = t3; + $[5] = data; + $[6] = t4; + } else { + t4 = $[6]; + } + return t4; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ arg: { items: 2 } }], + sequentialRenders: [ + { arg: { items: 2 } }, + { arg: { items: 2 } }, + { arg: null }, + { arg: null }, + ], +}; + +``` + +### Eval output +(kind: ok)
{"inputs":[2],"output":[2]}
+
{"inputs":[2],"output":[2]}
+
{"inputs":[null],"output":[null]}
+
{"inputs":[null],"output":[null]}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/optional-member-expression-single.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/optional-member-expression-single.js new file mode 100644 index 0000000000000..8f54a0edb517d --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/optional-member-expression-single.js @@ -0,0 +1,22 @@ +// @validatePreserveExistingMemoizationGuarantees @enableOptionalDependencies @enablePropagateDepsInHIR +import {ValidateMemoization} from 'shared-runtime'; +import {useMemo} from 'react'; +function Component({arg}) { + const data = useMemo(() => { + const x = []; + x.push(arg?.items); + return x; + }, [arg?.items]); + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{arg: {items: 2}}], + sequentialRenders: [ + {arg: {items: 2}}, + {arg: {items: 2}}, + {arg: null}, + {arg: null}, + ], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reactive-dependencies-non-optional-properties-inside-optional-chain.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reactive-dependencies-non-optional-properties-inside-optional-chain.expect.md index 6db3983d10751..9a524e6357a1b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reactive-dependencies-non-optional-properties-inside-optional-chain.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reactive-dependencies-non-optional-properties-inside-optional-chain.expect.md @@ -16,9 +16,9 @@ import { c as _c } from "react/compiler-runtime"; // @enablePropagateDepsInHIR function Component(props) { const $ = _c(2); let t0; - if ($[0] !== props.post.feedback.comments) { + if ($[0] !== props.post.feedback.comments?.edges) { t0 = props.post.feedback.comments?.edges?.map(render); - $[0] = props.post.feedback.comments; + $[0] = props.post.feedback.comments?.edges; $[1] = t0; } else { t0 = $[1]; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/conditional-member-expr.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/conditional-member-expr.expect.md index d56dcb63aed18..f13bfe7d61705 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/conditional-member-expr.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/conditional-member-expr.expect.md @@ -31,10 +31,10 @@ import { c as _c } from "react/compiler-runtime"; // @enablePropagateDepsInHIR function Component(props) { const $ = _c(2); let x; - if ($[0] !== props.a) { + if ($[0] !== props.a?.b) { x = []; x.push(props.a?.b); - $[0] = props.a; + $[0] = props.a?.b; $[1] = x; } else { x = $[1]; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/memberexpr-join-optional-chain.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/memberexpr-join-optional-chain.expect.md index 0f155c79dea93..a13a918a312cb 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/memberexpr-join-optional-chain.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/memberexpr-join-optional-chain.expect.md @@ -46,11 +46,11 @@ import { c as _c } from "react/compiler-runtime"; // @enablePropagateDepsInHIR function Component(props) { const $ = _c(2); let x; - if ($[0] !== props.a) { + if ($[0] !== props.a.b) { x = []; x.push(props.a?.b); x.push(props.a.b.c); - $[0] = props.a; + $[0] = props.a.b; $[1] = x; } else { x = $[1]; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/memberexpr-join-optional-chain2.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/memberexpr-join-optional-chain2.expect.md index cf2d1d413741e..df9dec4fb6827 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/memberexpr-join-optional-chain2.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/memberexpr-join-optional-chain2.expect.md @@ -22,16 +22,25 @@ export const FIXTURE_ENTRYPOINT = { ```javascript import { c as _c } from "react/compiler-runtime"; // @enablePropagateDepsInHIR function Component(props) { - const $ = _c(2); + const $ = _c(5); let x; - if ($[0] !== props.items) { + if ($[0] !== props.items?.length || $[1] !== props.items?.edges) { x = []; x.push(props.items?.length); - x.push(props.items?.edges?.map?.(render)?.filter?.(Boolean) ?? []); - $[0] = props.items; - $[1] = x; + let t0; + if ($[3] !== props.items?.edges) { + t0 = props.items?.edges?.map?.(render)?.filter?.(Boolean) ?? []; + $[3] = props.items?.edges; + $[4] = t0; + } else { + t0 = $[4]; + } + x.push(t0); + $[0] = props.items?.length; + $[1] = props.items?.edges; + $[2] = x; } else { - x = $[1]; + x = $[2]; } return x; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/merge-uncond-optional-chain-and-cond.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/merge-uncond-optional-chain-and-cond.expect.md new file mode 100644 index 0000000000000..8703c30cb09e0 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/merge-uncond-optional-chain-and-cond.expect.md @@ -0,0 +1,72 @@ + +## Input + +```javascript +// @enablePropagateDepsInHIR +import {identity} from 'shared-runtime'; + +/** + * Very contrived text fixture showing that it's technically incorrect to merge + * a conditional dependency (e.g. dep.path in `cond ? dep.path : ...`) and an + * unconditionally evaluated optional chain (`dep?.path`). + * + * + * when screen is non-null, useFoo returns { title: null } or "(not null)" + * when screen is null, useFoo throws + */ +function useFoo({screen}: {screen: null | undefined | {title_text: null}}) { + return screen?.title_text != null + ? '(not null)' + : identity({title: screen.title_text}); +} +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{screen: null}], + sequentialRenders: [{screen: {title_bar: undefined}}, {screen: null}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @enablePropagateDepsInHIR +import { identity } from "shared-runtime"; + +/** + * Very contrived text fixture showing that it's technically incorrect to merge + * a conditional dependency (e.g. dep.path in `cond ? dep.path : ...`) and an + * unconditionally evaluated optional chain (`dep?.path`). + * + * + * when screen is non-null, useFoo returns { title: null } or "(not null)" + * when screen is null, useFoo throws + */ +function useFoo(t0) { + const $ = _c(2); + const { screen } = t0; + let t1; + if ($[0] !== screen) { + t1 = + screen?.title_text != null + ? "(not null)" + : identity({ title: screen.title_text }); + $[0] = screen; + $[1] = t1; + } else { + t1 = $[1]; + } + return t1; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{ screen: null }], + sequentialRenders: [{ screen: { title_bar: undefined } }, { screen: null }], +}; + +``` + +### Eval output +(kind: ok) {} +[[ (exception in render) TypeError: Cannot read properties of null (reading 'title_text') ]] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/merge-uncond-optional-chain-and-cond.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/merge-uncond-optional-chain-and-cond.ts new file mode 100644 index 0000000000000..2275412d777be --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/merge-uncond-optional-chain-and-cond.ts @@ -0,0 +1,22 @@ +// @enablePropagateDepsInHIR +import {identity} from 'shared-runtime'; + +/** + * Very contrived text fixture showing that it's technically incorrect to merge + * a conditional dependency (e.g. dep.path in `cond ? dep.path : ...`) and an + * unconditionally evaluated optional chain (`dep?.path`). + * + * + * when screen is non-null, useFoo returns { title: null } or "(not null)" + * when screen is null, useFoo throws + */ +function useFoo({screen}: {screen: null | undefined | {title_text: null}}) { + return screen?.title_text != null + ? '(not null)' + : identity({title: screen.title_text}); +} +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{screen: null}], + sequentialRenders: [{screen: {title_bar: undefined}}, {screen: null}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/repro-scope-missing-mutable-range.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/repro-scope-missing-mutable-range.expect.md index cf4e4f93274bb..39f301432e51f 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/repro-scope-missing-mutable-range.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/repro-scope-missing-mutable-range.expect.md @@ -24,14 +24,14 @@ function HomeDiscoStoreItemTileRating(props) { const $ = _c(4); const item = useFragment(); let count; - if ($[0] !== item) { + if ($[0] !== item?.aggregates) { count = 0; const aggregates = item?.aggregates || []; aggregates.forEach((aggregate) => { count = count + (aggregate.count || 0); count; }); - $[0] = item; + $[0] = item?.aggregates; $[1] = count; } else { count = $[1];