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 01/23] [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 35c23ed074..049987e392 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 58a36e7023..abaa793c96 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 50a4a206ff..0d566a57ca 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 5b3a765783..956e014042 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 30aa539e5a..64a0616cac 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 50a4a206ff..0d566a57ca 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 5b3a765783..956e014042 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 02/23] 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 9692a1256a..03c03cfba0 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 03/23] [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 | 375 ++++++++++------- .../src/HIR/HIR.ts | 1 + .../src/HIR/PropagateScopeDependenciesHIR.ts | 113 +++--- .../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 | 98 +++-- .../optional-member-expression-as-memo-dep.js | 25 +- ...ptional-member-expression-single.expect.md | 86 ++-- .../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, 2229 insertions(+), 415 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 cb778c3292..3603416ee6 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 0000000000..4532947842 --- /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 f2bb0b31f0..f5567b3e53 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(); - #getOrCreateRoot( + /** + * @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', + ); + + 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. + 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 */ - currNode.accessType = merge( - currNode.accessType, - PropertyAccessType.Dependency, + let hoistableCursor: HoistableNode | undefined = + this.#hoistableObjects.get(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 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, ); } - markNodesNonNull(dep: ReactiveScopePropertyDependency): void { - const accessType = PropertyAccessType.NonNullAccess; - let currNode = this.#roots.get(dep.identifier); - - 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); - } - } - - /** - * 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 PropertyAccessType { - Access = 'Access', - NonNullAccess = 'NonNullAccess', - Dependency = 'Dependency', - NonNullDependency = 'NonNullDependency', -} - -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. +/* + * 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} + * ``` */ -function isNonNull(access: PropertyAccessType): boolean { +enum PropertyAccessType { + OptionalAccess = 'OptionalAccess', + UnconditionalAccess = 'UnconditionalAccess', + OptionalDependency = 'OptionalDependency', + UnconditionalDependency = 'UnconditionalDependency', +} + +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 615ec18feb..873082bdbe 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 1fe218c352..a7346e0e6b 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); } - for (const instr of block.instructions) { - handleInstruction(instr, context); + // 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 place of eachTerminalOperand(block.terminal)) { - context.visitOperand(place); + for (const instr of block.instructions) { + if (!processedInstrsInOptional.has(instr.id)) { + handleInstruction(instr, context); + } + } + + 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 6b813d5975..aa91c48b1b 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 0000000000..31e2cadf9f --- /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 0000000000..479048085e --- /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 0000000000..0acf33b2ed --- /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 0000000000..d00cb4fee6 --- /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 c34b79a848..77875f789d 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"; - props?.items.edges?.nodes; - let t0; +function Component(t0) { + const $ = _c(7); + const { arg } = 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; - } else { - t1 = $[1]; - } - t0 = t1; - const data = t0; let t2; - if ($[2] !== data) { - t2 = ; - $[2] = data; - $[3] = t2; + if ($[0] !== arg?.items.edges?.nodes) { + t2 = arg?.items.edges?.nodes.map(identity); + $[0] = arg?.items.edges?.nodes; + $[1] = t2; } else { - t2 = $[3]; + t2 = $[1]; } - return 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 { + 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: 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 d82d36b547..73f0f4d421 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 a4cf6d767d..6e44a97b45 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; - } else { - t2 = $[3]; - } + t1 = x; + const data = t1; + const t2 = arg?.items; let t3; - if ($[4] !== t2 || $[5] !== data) { - t3 = ; - $[4] = t2; - $[5] = data; - $[6] = t3; + if ($[2] !== t2) { + t3 = [t2]; + $[2] = t2; + $[3] = t3; } else { - t3 = $[6]; + t3 = $[3]; } - return t3; + 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: 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 5750d7af3a..62ac31dd6d 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 8b52187920..e0196bdc19 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 e885982310..0000000000 --- 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 6ff87d0c46..0000000000 --- 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 3559b2bd58..0000000000 --- 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 429f168836..0000000000 --- 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 535a0ce074..0000000000 --- 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 0000000000..757ad2666d --- /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 0000000000..750e422861 --- /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 0000000000..56b987c677 --- /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 0000000000..48f3b2de2a --- /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 0000000000..d0486cd8c2 --- /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 0000000000..d248c472f5 --- /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 0000000000..b4a55fcb61 --- /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 0000000000..f15b9b8e9b --- /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 0000000000..8f54a0edb5 --- /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 6db3983d10..9a524e6357 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 d56dcb63ae..f13bfe7d61 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 0f155c79de..a13a918a31 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 cf2d1d4137..df9dec4fb6 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 0000000000..8703c30cb0 --- /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 0000000000..2275412d77 --- /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 cf4e4f9327..39f301432e 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]; From edacbde73f50cc9dc00819d61275cd43f12665c1 Mon Sep 17 00:00:00 2001 From: mofeiZ <34200447+mofeiZ@users.noreply.github.com> Date: Thu, 3 Oct 2024 13:55:59 -0400 Subject: [PATCH 04/23] [compiler][hir-rewrite] Check mutability of base identifier when hoisting (#31032) Stack from [ghstack](https://github.com/ezyang/ghstack) (oldest at bottom): * #31066 * __->__ #31032 Prior to this PR, we check whether the property load source (e.g. the evaluation of `` in `.property`) is mutable + scoped to determine whether the property load itself is eligible for hoisting. This changes to check the base identifier of the load. - This is needed for the next PR #31066. We want to evaluate whether the base identifier is mutable within the context of the *outermost function*. This is because all LoadLocals and PropertyLoads within a nested function declaration have mutable-ranges within the context of the function, but the base identifier is a context variable. - A side effect is that we no longer infer loads from props / other function arguments as mutable in edge cases (e.g. props escaping out of try-blocks or being assigned to context variables) --- .../src/HIR/CollectHoistablePropertyLoads.ts | 109 ++++++++---------- ...-try-catch-maybe-null-dependency.expect.md | 65 +++++++++++ .../bug-try-catch-maybe-null-dependency.ts | 22 ++++ .../try-catch-maybe-null-dependency.expect.md | 79 +++++++++++++ .../try-catch-maybe-null-dependency.ts | 23 ++++ ...value-modified-in-catch-escaping.expect.md | 11 +- ...atch-try-value-modified-in-catch.expect.md | 11 +- .../packages/snap/src/SproutTodoFilter.ts | 1 + 8 files changed, 250 insertions(+), 71 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-try-catch-maybe-null-dependency.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-try-catch-maybe-null-dependency.ts create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/try-catch-maybe-null-dependency.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/try-catch-maybe-null-dependency.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 3603416ee6..3118db2378 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts @@ -15,7 +15,7 @@ import { HIRFunction, Identifier, IdentifierId, - InstructionId, + InstructionValue, ReactiveScopeDependency, ScopeId, } from './HIR'; @@ -209,33 +209,23 @@ class PropertyPathRegistry { } } -function addNonNullPropertyPath( - source: Identifier, - sourceNode: PropertyPathNode, - instrId: InstructionId, - knownImmutableIdentifiers: Set, - result: Set, -): void { - /** - * Since this runs *after* buildReactiveScopeTerminals, identifier mutable ranges - * are not valid with respect to current instruction id numbering. - * We use attached reactive scope ranges as a proxy for mutable range, but this - * is an overestimate as (1) scope ranges merge and align to form valid program - * blocks and (2) passes like MemoizeFbtAndMacroOperands may assign scopes to - * non-mutable identifiers. - * - * See comment at top of function for why we track known immutable identifiers. - */ - const isMutableAtInstr = - source.mutableRange.end > source.mutableRange.start + 1 && - source.scope != null && - inRange({id: instrId}, source.scope.range); - if ( - !isMutableAtInstr || - knownImmutableIdentifiers.has(sourceNode.fullPath.identifier.id) - ) { - result.add(sourceNode); +function getMaybeNonNullInInstruction( + instr: InstructionValue, + temporaries: ReadonlyMap, + registry: PropertyPathRegistry, +): PropertyPathNode | null { + let path = null; + if (instr.kind === 'PropertyLoad') { + path = temporaries.get(instr.object.identifier.id) ?? { + identifier: instr.object.identifier, + path: [], + }; + } else if (instr.kind === 'Destructure') { + path = temporaries.get(instr.value.identifier.id) ?? null; + } else if (instr.kind === 'ComputedLoad') { + path = temporaries.get(instr.object.identifier.id) ?? null; } + return path != null ? registry.getOrCreateProperty(path) : null; } function collectNonNullsInBlocks( @@ -286,41 +276,38 @@ function collectNonNullsInBlocks( ); } for (const instr of block.instructions) { - if (instr.value.kind === 'PropertyLoad') { - const source = temporaries.get(instr.value.object.identifier.id) ?? { - identifier: instr.value.object.identifier, - path: [], - }; - addNonNullPropertyPath( - instr.value.object.identifier, - registry.getOrCreateProperty(source), - instr.id, - knownImmutableIdentifiers, - assumedNonNullObjects, - ); - } else if (instr.value.kind === 'Destructure') { - const source = instr.value.value.identifier.id; - const sourceNode = temporaries.get(source); - if (sourceNode != null) { - addNonNullPropertyPath( - instr.value.value.identifier, - registry.getOrCreateProperty(sourceNode), - instr.id, - knownImmutableIdentifiers, - assumedNonNullObjects, - ); - } - } else if (instr.value.kind === 'ComputedLoad') { - const source = instr.value.object.identifier.id; - const sourceNode = temporaries.get(source); - if (sourceNode != null) { - addNonNullPropertyPath( - instr.value.object.identifier, - registry.getOrCreateProperty(sourceNode), - instr.id, - knownImmutableIdentifiers, - assumedNonNullObjects, + const maybeNonNull = getMaybeNonNullInInstruction( + instr.value, + temporaries, + registry, + ); + if (maybeNonNull != null) { + const baseIdentifier = maybeNonNull.fullPath.identifier; + /** + * Since this runs *after* buildReactiveScopeTerminals, identifier mutable ranges + * are not valid with respect to current instruction id numbering. + * We use attached reactive scope ranges as a proxy for mutable range, but this + * is an overestimate as (1) scope ranges merge and align to form valid program + * blocks and (2) passes like MemoizeFbtAndMacroOperands may assign scopes to + * non-mutable identifiers. + * + * See comment at top of function for why we track known immutable identifiers. + */ + const isMutableAtInstr = + baseIdentifier.mutableRange.end > + baseIdentifier.mutableRange.start + 1 && + baseIdentifier.scope != null && + inRange( + { + id: instr.id, + }, + baseIdentifier.scope.range, ); + if ( + !isMutableAtInstr || + knownImmutableIdentifiers.has(baseIdentifier.id) + ) { + assumedNonNullObjects.add(maybeNonNull); } } } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-try-catch-maybe-null-dependency.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-try-catch-maybe-null-dependency.expect.md new file mode 100644 index 0000000000..56ca1f7722 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-try-catch-maybe-null-dependency.expect.md @@ -0,0 +1,65 @@ + +## Input + +```javascript +import {identity} from 'shared-runtime'; + +/** + * Not safe to hoist read of maybeNullObject.value.inner outside of the + * try-catch block, as that might throw + */ +function useFoo(maybeNullObject: {value: {inner: number}} | null) { + const y = []; + try { + y.push(identity(maybeNullObject.value.inner)); + } catch { + y.push('null'); + } + + return y; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [null], + sequentialRenders: [null, {value: 2}, {value: 3}, null], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { identity } from "shared-runtime"; + +/** + * Not safe to hoist read of maybeNullObject.value.inner outside of the + * try-catch block, as that might throw + */ +function useFoo(maybeNullObject) { + const $ = _c(2); + let y; + if ($[0] !== maybeNullObject.value.inner) { + y = []; + try { + y.push(identity(maybeNullObject.value.inner)); + } catch { + y.push("null"); + } + $[0] = maybeNullObject.value.inner; + $[1] = y; + } else { + y = $[1]; + } + return y; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [null], + sequentialRenders: [null, { value: 2 }, { value: 3 }, null], +}; + +``` + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-try-catch-maybe-null-dependency.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-try-catch-maybe-null-dependency.ts new file mode 100644 index 0000000000..555ace1940 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-try-catch-maybe-null-dependency.ts @@ -0,0 +1,22 @@ +import {identity} from 'shared-runtime'; + +/** + * Not safe to hoist read of maybeNullObject.value.inner outside of the + * try-catch block, as that might throw + */ +function useFoo(maybeNullObject: {value: {inner: number}} | null) { + const y = []; + try { + y.push(identity(maybeNullObject.value.inner)); + } catch { + y.push('null'); + } + + return y; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [null], + sequentialRenders: [null, {value: 2}, {value: 3}, null], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/try-catch-maybe-null-dependency.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/try-catch-maybe-null-dependency.expect.md new file mode 100644 index 0000000000..cd2c4eb9df --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/try-catch-maybe-null-dependency.expect.md @@ -0,0 +1,79 @@ + +## Input + +```javascript +// @enablePropagateDepsInHIR +import {identity} from 'shared-runtime'; + +/** + * Not safe to hoist read of maybeNullObject.value.inner outside of the + * try-catch block, as that might throw + */ +function useFoo(maybeNullObject: {value: {inner: number}} | null) { + const y = []; + try { + y.push(identity(maybeNullObject.value.inner)); + } catch { + y.push('null'); + } + + return y; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [null], + sequentialRenders: [null, {value: 2}, {value: 3}, null], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @enablePropagateDepsInHIR +import { identity } from "shared-runtime"; + +/** + * Not safe to hoist read of maybeNullObject.value.inner outside of the + * try-catch block, as that might throw + */ +function useFoo(maybeNullObject) { + const $ = _c(4); + let y; + if ($[0] !== maybeNullObject) { + y = []; + try { + let t0; + if ($[2] !== maybeNullObject.value.inner) { + t0 = identity(maybeNullObject.value.inner); + $[2] = maybeNullObject.value.inner; + $[3] = t0; + } else { + t0 = $[3]; + } + y.push(t0); + } catch { + y.push("null"); + } + $[0] = maybeNullObject; + $[1] = y; + } else { + y = $[1]; + } + return y; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [null], + sequentialRenders: [null, { value: 2 }, { value: 3 }, null], +}; + +``` + +### Eval output +(kind: ok) ["null"] +[null] +[null] +["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/try-catch-maybe-null-dependency.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/try-catch-maybe-null-dependency.ts new file mode 100644 index 0000000000..bdbd903117 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/try-catch-maybe-null-dependency.ts @@ -0,0 +1,23 @@ +// @enablePropagateDepsInHIR +import {identity} from 'shared-runtime'; + +/** + * Not safe to hoist read of maybeNullObject.value.inner outside of the + * try-catch block, as that might throw + */ +function useFoo(maybeNullObject: {value: {inner: number}} | null) { + const y = []; + try { + y.push(identity(maybeNullObject.value.inner)); + } catch { + y.push('null'); + } + + return y; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [null], + sequentialRenders: [null, {value: 2}, {value: 3}, null], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/try-catch-try-value-modified-in-catch-escaping.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/try-catch-try-value-modified-in-catch-escaping.expect.md index 914001f373..f69994b0a8 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/try-catch-try-value-modified-in-catch-escaping.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/try-catch-try-value-modified-in-catch-escaping.expect.md @@ -32,9 +32,9 @@ import { c as _c } from "react/compiler-runtime"; // @enablePropagateDepsInHIR const { throwInput } = require("shared-runtime"); function Component(props) { - const $ = _c(2); + const $ = _c(3); let x; - if ($[0] !== props) { + if ($[0] !== props.y || $[1] !== props.e) { try { const y = []; y.push(props.y); @@ -44,10 +44,11 @@ function Component(props) { e.push(props.e); x = e; } - $[0] = props; - $[1] = x; + $[0] = props.y; + $[1] = props.e; + $[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/try-catch-try-value-modified-in-catch.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/try-catch-try-value-modified-in-catch.expect.md index 30ecdf6d59..bc47228371 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/try-catch-try-value-modified-in-catch.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/try-catch-try-value-modified-in-catch.expect.md @@ -31,9 +31,9 @@ import { c as _c } from "react/compiler-runtime"; // @enablePropagateDepsInHIR const { throwInput } = require("shared-runtime"); function Component(props) { - const $ = _c(2); + const $ = _c(3); let t0; - if ($[0] !== props) { + if ($[0] !== props.y || $[1] !== props.e) { t0 = Symbol.for("react.early_return_sentinel"); bb0: { try { @@ -47,10 +47,11 @@ function Component(props) { break bb0; } } - $[0] = props; - $[1] = t0; + $[0] = props.y; + $[1] = props.e; + $[2] = t0; } else { - t0 = $[1]; + t0 = $[2]; } if (t0 !== Symbol.for("react.early_return_sentinel")) { return t0; diff --git a/compiler/packages/snap/src/SproutTodoFilter.ts b/compiler/packages/snap/src/SproutTodoFilter.ts index 0ae22a643b..c40392884d 100644 --- a/compiler/packages/snap/src/SproutTodoFilter.ts +++ b/compiler/packages/snap/src/SproutTodoFilter.ts @@ -478,6 +478,7 @@ const skipFilter = new Set([ 'fbt/bug-fbt-plural-multiple-function-calls', 'fbt/bug-fbt-plural-multiple-mixed-call-tag', 'bug-invalid-hoisting-functionexpr', + 'bug-try-catch-maybe-null-dependency', 'reduce-reactive-deps/bug-merge-uncond-optional-chain-and-cond', 'original-reactive-scopes-fork/bug-nonmutating-capture-in-unsplittable-memo-block', 'original-reactive-scopes-fork/bug-hoisted-declaration-with-scope', From 1460d67c5b9a0d4498b4d22e1a5a6c0ccac85fdd Mon Sep 17 00:00:00 2001 From: mofeiZ <34200447+mofeiZ@users.noreply.github.com> Date: Thu, 3 Oct 2024 14:41:32 -0400 Subject: [PATCH 05/23] [compiler][hir] Only hoist always-accessed PropertyLoads from function decls (#31066) Stack from [ghstack](https://github.com/ezyang/ghstack) (oldest at bottom): * __->__ #31066 * #31032 Prior to this PR, we consider all of a nested function's accessed paths as 'hoistable' (to the basic block in which the function was defined). Now, we traverse nested functions and find all paths hoistable to their *entry block*. Note that this only replaces the *hoisting* part of function declarations, not dependencies. This realistically only affects optional chains within functions, which always get truncated to its inner non-optional path (see [todo-infer-function-uncond-optionals-hoisted.tsx](https://github.com/facebook/react/blob/576f3c0aa898cb99da1b7bf15317756e25c13708/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/todo-infer-function-uncond-optionals-hoisted.tsx)) See newly added test fixtures for details Update: Note that toggling `enableTreatFunctionDepsAsConditional` makes a non-trivial impact on granularity of inferred deps (i.e. we find that function declarations uniquely identify some paths as hoistable). Snapshot comparison of internal code shows ~2.5% of files get worse dependencies ([internal link](https://www.internalfb.com/phabricator/paste/view/P1625792186)) --- .../src/HIR/CollectHoistablePropertyLoads.ts | 231 ++++++++++++------ .../src/HIR/PropagateScopeDependenciesHIR.ts | 12 +- ...r-function-cond-access-local-var.expect.md | 97 ++++++++ .../infer-function-cond-access-local-var.tsx | 33 +++ ...function-cond-access-not-hoisted.expect.md | 80 ++++++ ...infer-function-cond-access-not-hoisted.tsx | 25 ++ ...r-function-uncond-access-hoisted.expect.md | 52 ++++ .../infer-function-uncond-access-hoisted.tsx | 13 + ...n-uncond-access-hoists-other-dep.expect.md | 92 +++++++ ...unction-uncond-access-hoists-other-dep.tsx | 25 ++ ...function-uncond-access-local-var.expect.md | 73 ++++++ ...infer-function-uncond-access-local-var.tsx | 16 ++ ...uncond-optional-hoists-other-dep.expect.md | 91 +++++++ ...ction-uncond-optional-hoists-other-dep.tsx | 24 ++ ...function-uncond-access-local-var.expect.md | 73 ++++++ ...ested-function-uncond-access-local-var.tsx | 16 ++ ...er-nested-function-uncond-access.expect.md | 66 +++++ .../infer-nested-function-uncond-access.tsx | 18 ++ ...nfer-object-method-uncond-access.expect.md | 70 ++++++ .../infer-object-method-uncond-access.tsx | 18 ++ ...unction-uncond-optionals-hoisted.expect.md | 64 +++++ ...nfer-function-uncond-optionals-hoisted.tsx | 18 ++ ...function-cond-access-not-hoisted.expect.md | 73 ++++++ ...infer-function-cond-access-not-hoisted.tsx | 23 ++ ...unction-uncond-optionals-hoisted.expect.md | 61 +++++ ...nfer-function-uncond-optionals-hoisted.tsx | 16 ++ .../packages/snap/src/SproutTodoFilter.ts | 1 + 27 files changed, 1306 insertions(+), 75 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-cond-access-local-var.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-cond-access-local-var.tsx create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-cond-access-not-hoisted.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-cond-access-not-hoisted.tsx create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-uncond-access-hoisted.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-uncond-access-hoisted.tsx create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-uncond-access-hoists-other-dep.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-uncond-access-hoists-other-dep.tsx create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-uncond-access-local-var.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-uncond-access-local-var.tsx create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-uncond-optional-hoists-other-dep.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-uncond-optional-hoists-other-dep.tsx create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-nested-function-uncond-access-local-var.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-nested-function-uncond-access-local-var.tsx create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-nested-function-uncond-access.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-nested-function-uncond-access.tsx create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-object-method-uncond-access.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-object-method-uncond-access.tsx create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/todo-infer-function-uncond-optionals-hoisted.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/todo-infer-function-uncond-optionals-hoisted.tsx create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/bug-infer-function-cond-access-not-hoisted.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/bug-infer-function-cond-access-not-hoisted.tsx create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/todo-infer-function-uncond-optionals-hoisted.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/todo-infer-function-uncond-optionals-hoisted.tsx 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 3118db2378..80593d6275 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts @@ -7,6 +7,7 @@ import { Set_union, getOrInsertDefault, } from '../Utils/utils'; +import {collectOptionalChainSidemap} from './CollectOptionalChainDependencies'; import { BasicBlock, BlockId, @@ -15,10 +16,12 @@ import { HIRFunction, Identifier, IdentifierId, + InstructionId, InstructionValue, ReactiveScopeDependency, ScopeId, } from './HIR'; +import {collectTemporariesSidemap} from './PropagateScopeDependenciesHIR'; /** * Helper function for `PropagateScopeDependencies`. Uses control flow graph @@ -83,28 +86,57 @@ export function collectHoistablePropertyLoads( fn: HIRFunction, temporaries: ReadonlyMap, hoistableFromOptionals: ReadonlyMap, -): ReadonlyMap { + nestedFnImmutableContext: ReadonlySet | null, +): ReadonlyMap { const registry = new PropertyPathRegistry(); - const nodes = collectNonNullsInBlocks( - fn, - temporaries, + const functionExpressionLoads = collectFunctionExpressionFakeLoads(fn); + const actuallyEvaluatedTemporaries = new Map( + [...temporaries].filter(([id]) => !functionExpressionLoads.has(id)), + ); + + /** + * Due to current limitations of mutable range inference, there are edge cases in + * which we infer known-immutable values (e.g. props or hook params) to have a + * mutable range and scope. + * (see `destructure-array-declaration-to-context-var` fixture) + * We track known immutable identifiers to reduce regressions (as PropagateScopeDeps + * is being rewritten to HIR). + */ + const knownImmutableIdentifiers = new Set(); + if (fn.fnType === 'Component' || fn.fnType === 'Hook') { + for (const p of fn.params) { + if (p.kind === 'Identifier') { + knownImmutableIdentifiers.add(p.identifier.id); + } + } + } + const nodes = collectNonNullsInBlocks(fn, { + temporaries: actuallyEvaluatedTemporaries, + knownImmutableIdentifiers, hoistableFromOptionals, registry, - ); + nestedFnImmutableContext, + }); propagateNonNull(fn, nodes, registry); - const nodesKeyedByScopeId = new Map(); + return nodes; +} + +export function keyByScopeId( + fn: HIRFunction, + source: ReadonlyMap, +): ReadonlyMap { + const keyedByScopeId = new Map(); for (const [_, block] of fn.body.blocks) { if (block.terminal.kind === 'scope') { - nodesKeyedByScopeId.set( + keyedByScopeId.set( block.terminal.scope.id, - nodes.get(block.terminal.block)!, + source.get(block.terminal.block)!, ); } } - - return nodesKeyedByScopeId; + return keyedByScopeId; } export type BlockInfo = { @@ -211,45 +243,75 @@ class PropertyPathRegistry { function getMaybeNonNullInInstruction( instr: InstructionValue, - temporaries: ReadonlyMap, - registry: PropertyPathRegistry, + context: CollectNonNullsInBlocksContext, ): PropertyPathNode | null { let path = null; if (instr.kind === 'PropertyLoad') { - path = temporaries.get(instr.object.identifier.id) ?? { + path = context.temporaries.get(instr.object.identifier.id) ?? { identifier: instr.object.identifier, path: [], }; } else if (instr.kind === 'Destructure') { - path = temporaries.get(instr.value.identifier.id) ?? null; + path = context.temporaries.get(instr.value.identifier.id) ?? null; } else if (instr.kind === 'ComputedLoad') { - path = temporaries.get(instr.object.identifier.id) ?? null; + path = context.temporaries.get(instr.object.identifier.id) ?? null; } - return path != null ? registry.getOrCreateProperty(path) : null; + return path != null ? context.registry.getOrCreateProperty(path) : null; } +function isImmutableAtInstr( + identifier: Identifier, + instr: InstructionId, + context: CollectNonNullsInBlocksContext, +): boolean { + if (context.nestedFnImmutableContext != null) { + /** + * Comparing instructions ids across inner-outer function bodies is not valid, as they are numbered + */ + return context.nestedFnImmutableContext.has(identifier.id); + } else { + /** + * Since this runs *after* buildReactiveScopeTerminals, identifier mutable ranges + * are not valid with respect to current instruction id numbering. + * We use attached reactive scope ranges as a proxy for mutable range, but this + * is an overestimate as (1) scope ranges merge and align to form valid program + * blocks and (2) passes like MemoizeFbtAndMacroOperands may assign scopes to + * non-mutable identifiers. + * + * See comment in exported function for why we track known immutable identifiers. + */ + const mutableAtInstr = + identifier.mutableRange.end > identifier.mutableRange.start + 1 && + identifier.scope != null && + inRange( + { + id: instr, + }, + identifier.scope.range, + ); + return ( + !mutableAtInstr || context.knownImmutableIdentifiers.has(identifier.id) + ); + } +} + +type CollectNonNullsInBlocksContext = { + temporaries: ReadonlyMap; + knownImmutableIdentifiers: ReadonlySet; + hoistableFromOptionals: ReadonlyMap; + registry: PropertyPathRegistry; + /** + * (For nested / inner function declarations) + * Context variables (i.e. captured from an outer scope) that are immutable. + * Note that this technically could be merged into `knownImmutableIdentifiers`, + * but are currently kept separate for readability. + */ + nestedFnImmutableContext: ReadonlySet | null; +}; function collectNonNullsInBlocks( fn: HIRFunction, - temporaries: ReadonlyMap, - hoistableFromOptionals: ReadonlyMap, - registry: PropertyPathRegistry, + context: CollectNonNullsInBlocksContext, ): ReadonlyMap { - /** - * Due to current limitations of mutable range inference, there are edge cases in - * which we infer known-immutable values (e.g. props or hook params) to have a - * mutable range and scope. - * (see `destructure-array-declaration-to-context-var` fixture) - * We track known immutable identifiers to reduce regressions (as PropagateScopeDeps - * is being rewritten to HIR). - */ - const knownImmutableIdentifiers = new Set(); - if (fn.fnType === 'Component' || fn.fnType === 'Hook') { - for (const p of fn.params) { - if (p.kind === 'Identifier') { - knownImmutableIdentifiers.add(p.identifier.id); - } - } - } /** * Known non-null objects such as functional component props can be safely * read from any block. @@ -261,7 +323,9 @@ function collectNonNullsInBlocks( fn.params[0].kind === 'Identifier' ) { const identifier = fn.params[0].identifier; - knownNonNullIdentifiers.add(registry.getOrCreateIdentifier(identifier)); + knownNonNullIdentifiers.add( + context.registry.getOrCreateIdentifier(identifier), + ); } const nodes = new Map(); for (const [_, block] of fn.body.blocks) { @@ -269,45 +333,48 @@ function collectNonNullsInBlocks( knownNonNullIdentifiers, ); - const maybeOptionalChain = hoistableFromOptionals.get(block.id); + const maybeOptionalChain = context.hoistableFromOptionals.get(block.id); if (maybeOptionalChain != null) { assumedNonNullObjects.add( - registry.getOrCreateProperty(maybeOptionalChain), + context.registry.getOrCreateProperty(maybeOptionalChain), ); } for (const instr of block.instructions) { - const maybeNonNull = getMaybeNonNullInInstruction( - instr.value, - temporaries, - registry, - ); - if (maybeNonNull != null) { - const baseIdentifier = maybeNonNull.fullPath.identifier; - /** - * Since this runs *after* buildReactiveScopeTerminals, identifier mutable ranges - * are not valid with respect to current instruction id numbering. - * We use attached reactive scope ranges as a proxy for mutable range, but this - * is an overestimate as (1) scope ranges merge and align to form valid program - * blocks and (2) passes like MemoizeFbtAndMacroOperands may assign scopes to - * non-mutable identifiers. - * - * See comment at top of function for why we track known immutable identifiers. - */ - const isMutableAtInstr = - baseIdentifier.mutableRange.end > - baseIdentifier.mutableRange.start + 1 && - baseIdentifier.scope != null && - inRange( - { - id: instr.id, - }, - baseIdentifier.scope.range, - ); - if ( - !isMutableAtInstr || - knownImmutableIdentifiers.has(baseIdentifier.id) - ) { - assumedNonNullObjects.add(maybeNonNull); + const maybeNonNull = getMaybeNonNullInInstruction(instr.value, context); + if ( + maybeNonNull != null && + isImmutableAtInstr(maybeNonNull.fullPath.identifier, instr.id, context) + ) { + assumedNonNullObjects.add(maybeNonNull); + } + if ( + instr.value.kind === 'FunctionExpression' && + !fn.env.config.enableTreatFunctionDepsAsConditional + ) { + const innerFn = instr.value.loweredFunc; + const innerTemporaries = collectTemporariesSidemap( + innerFn.func, + new Set(), + ); + const innerOptionals = collectOptionalChainSidemap(innerFn.func); + const innerHoistableMap = collectHoistablePropertyLoads( + innerFn.func, + innerTemporaries, + innerOptionals.hoistableObjects, + context.nestedFnImmutableContext ?? + new Set( + innerFn.func.context + .filter(place => + isImmutableAtInstr(place.identifier, instr.id, context), + ) + .map(place => place.identifier.id), + ), + ); + const innerHoistables = assertNonNull( + innerHoistableMap.get(innerFn.func.body.entry), + ); + for (const entry of innerHoistables.assumedNonNullObjects) { + assumedNonNullObjects.add(entry); } } } @@ -515,3 +582,27 @@ function reduceMaybeOptionalChains( } } while (changed); } + +function collectFunctionExpressionFakeLoads( + fn: HIRFunction, +): Set { + const sources = new Map(); + const functionExpressionReferences = new Set(); + + for (const [_, block] of fn.body.blocks) { + for (const {lvalue, value} of block.instructions) { + if (value.kind === 'FunctionExpression') { + for (const reference of value.loweredFunc.dependencies) { + let curr: IdentifierId | undefined = reference.identifier.id; + while (curr != null) { + functionExpressionReferences.add(curr); + curr = sources.get(curr); + } + } + } else if (value.kind === 'PropertyLoad') { + sources.set(lvalue.identifier.id, value.object.identifier.id); + } + } + } + return functionExpressionReferences; +} 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 a7346e0e6b..ab2cf4cf56 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDependenciesHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDependenciesHIR.ts @@ -17,7 +17,10 @@ import { areEqualPaths, IdentifierId, } from './HIR'; -import {collectHoistablePropertyLoads} from './CollectHoistablePropertyLoads'; +import { + collectHoistablePropertyLoads, + keyByScopeId, +} from './CollectHoistablePropertyLoads'; import { ScopeBlockTraversal, eachInstructionOperand, @@ -41,10 +44,9 @@ export function propagateScopeDependenciesHIR(fn: HIRFunction): void { hoistableObjects, } = collectOptionalChainSidemap(fn); - const hoistablePropertyLoads = collectHoistablePropertyLoads( + const hoistablePropertyLoads = keyByScopeId( fn, - temporaries, - hoistableObjects, + collectHoistablePropertyLoads(fn, temporaries, hoistableObjects, null), ); const scopeDeps = collectDependencies( @@ -209,7 +211,7 @@ function findTemporariesUsedOutsideDeclaringScope( * of $1, as the evaluation of `arr.length` changes between instructions $1 and * $3. We do not track $1 -> arr.length in this case. */ -function collectTemporariesSidemap( +export function collectTemporariesSidemap( fn: HIRFunction, usedOutsideDeclaringScope: ReadonlySet, ): ReadonlyMap { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-cond-access-local-var.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-cond-access-local-var.expect.md new file mode 100644 index 0000000000..c0f8aa97cd --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-cond-access-local-var.expect.md @@ -0,0 +1,97 @@ + +## Input + +```javascript +// @enablePropagateDepsInHIR + +import {shallowCopy, mutate, Stringify} from 'shared-runtime'; + +function useFoo({ + a, + shouldReadA, +}: { + a: {b: {c: number}; x: number}; + shouldReadA: boolean; +}) { + const local = shallowCopy(a); + mutate(local); + return ( + { + if (shouldReadA) return local.b.c; + return null; + }} + shouldInvokeFns={true} + /> + ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{a: null, shouldReadA: true}], + sequentialRenders: [ + {a: null, shouldReadA: true}, + {a: null, shouldReadA: false}, + {a: {b: {c: 4}}, shouldReadA: true}, + ], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @enablePropagateDepsInHIR + +import { shallowCopy, mutate, Stringify } from "shared-runtime"; + +function useFoo(t0) { + const $ = _c(5); + const { a, shouldReadA } = t0; + let local; + if ($[0] !== a) { + local = shallowCopy(a); + mutate(local); + $[0] = a; + $[1] = local; + } else { + local = $[1]; + } + let t1; + if ($[2] !== shouldReadA || $[3] !== local) { + t1 = ( + { + if (shouldReadA) { + return local.b.c; + } + return null; + }} + shouldInvokeFns={true} + /> + ); + $[2] = shouldReadA; + $[3] = local; + $[4] = t1; + } else { + t1 = $[4]; + } + return t1; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{ a: null, shouldReadA: true }], + sequentialRenders: [ + { a: null, shouldReadA: true }, + { a: null, shouldReadA: false }, + { a: { b: { c: 4 } }, shouldReadA: true }, + ], +}; + +``` + +### Eval output +(kind: ok) [[ (exception in render) TypeError: Cannot read properties of undefined (reading 'c') ]] +
{"fn":{"kind":"Function","result":null},"shouldInvokeFns":true}
+
{"fn":{"kind":"Function","result":4},"shouldInvokeFns":true}
\ 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/infer-function-cond-access-local-var.tsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-cond-access-local-var.tsx new file mode 100644 index 0000000000..fdf22dc970 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-cond-access-local-var.tsx @@ -0,0 +1,33 @@ +// @enablePropagateDepsInHIR + +import {shallowCopy, mutate, Stringify} from 'shared-runtime'; + +function useFoo({ + a, + shouldReadA, +}: { + a: {b: {c: number}; x: number}; + shouldReadA: boolean; +}) { + const local = shallowCopy(a); + mutate(local); + return ( + { + if (shouldReadA) return local.b.c; + return null; + }} + shouldInvokeFns={true} + /> + ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{a: null, shouldReadA: true}], + sequentialRenders: [ + {a: null, shouldReadA: true}, + {a: null, shouldReadA: false}, + {a: {b: {c: 4}}, shouldReadA: true}, + ], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-cond-access-not-hoisted.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-cond-access-not-hoisted.expect.md new file mode 100644 index 0000000000..e37b8365a2 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-cond-access-not-hoisted.expect.md @@ -0,0 +1,80 @@ + +## Input + +```javascript +// @enablePropagateDepsInHIR + +import {Stringify} from 'shared-runtime'; + +function Foo({a, shouldReadA}) { + return ( + { + if (shouldReadA) return a.b.c; + return null; + }} + shouldInvokeFns={true} + /> + ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{a: null, shouldReadA: true}], + sequentialRenders: [ + {a: null, shouldReadA: true}, + {a: null, shouldReadA: false}, + {a: {b: {c: 4}}, shouldReadA: true}, + ], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @enablePropagateDepsInHIR + +import { Stringify } from "shared-runtime"; + +function Foo(t0) { + const $ = _c(3); + const { a, shouldReadA } = t0; + let t1; + if ($[0] !== shouldReadA || $[1] !== a) { + t1 = ( + { + if (shouldReadA) { + return a.b.c; + } + return null; + }} + shouldInvokeFns={true} + /> + ); + $[0] = shouldReadA; + $[1] = a; + $[2] = t1; + } else { + t1 = $[2]; + } + return t1; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{ a: null, shouldReadA: true }], + sequentialRenders: [ + { a: null, shouldReadA: true }, + { a: null, shouldReadA: false }, + { a: { b: { c: 4 } }, shouldReadA: true }, + ], +}; + +``` + +### Eval output +(kind: ok) [[ (exception in render) TypeError: Cannot read properties of null (reading 'b') ]] +
{"fn":{"kind":"Function","result":null},"shouldInvokeFns":true}
+
{"fn":{"kind":"Function","result":4},"shouldInvokeFns":true}
\ 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/infer-function-cond-access-not-hoisted.tsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-cond-access-not-hoisted.tsx new file mode 100644 index 0000000000..5c71d57750 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-cond-access-not-hoisted.tsx @@ -0,0 +1,25 @@ +// @enablePropagateDepsInHIR + +import {Stringify} from 'shared-runtime'; + +function Foo({a, shouldReadA}) { + return ( + { + if (shouldReadA) return a.b.c; + return null; + }} + shouldInvokeFns={true} + /> + ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{a: null, shouldReadA: true}], + sequentialRenders: [ + {a: null, shouldReadA: true}, + {a: null, shouldReadA: false}, + {a: {b: {c: 4}}, shouldReadA: true}, + ], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-uncond-access-hoisted.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-uncond-access-hoisted.expect.md new file mode 100644 index 0000000000..1ddc7495bc --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-uncond-access-hoisted.expect.md @@ -0,0 +1,52 @@ + +## Input + +```javascript +// @enablePropagateDepsInHIR + +import {Stringify} from 'shared-runtime'; + +function useFoo({a}) { + return a.b.c} shouldInvokeFns={true} />; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{a: null}], + sequentialRenders: [{a: null}, {a: {b: {c: 4}}}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @enablePropagateDepsInHIR + +import { Stringify } from "shared-runtime"; + +function useFoo(t0) { + const $ = _c(2); + const { a } = t0; + let t1; + if ($[0] !== a.b.c) { + t1 = a.b.c} shouldInvokeFns={true} />; + $[0] = a.b.c; + $[1] = t1; + } else { + t1 = $[1]; + } + return t1; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{ a: null }], + sequentialRenders: [{ a: null }, { a: { b: { c: 4 } } }], +}; + +``` + +### Eval output +(kind: ok) [[ (exception in render) TypeError: Cannot read properties of null (reading 'b') ]] +
{"fn":{"kind":"Function","result":4},"shouldInvokeFns":true}
\ 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/infer-function-uncond-access-hoisted.tsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-uncond-access-hoisted.tsx new file mode 100644 index 0000000000..9cc72a36d7 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-uncond-access-hoisted.tsx @@ -0,0 +1,13 @@ +// @enablePropagateDepsInHIR + +import {Stringify} from 'shared-runtime'; + +function useFoo({a}) { + return a.b.c} shouldInvokeFns={true} />; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{a: null}], + sequentialRenders: [{a: null}, {a: {b: {c: 4}}}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-uncond-access-hoists-other-dep.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-uncond-access-hoists-other-dep.expect.md new file mode 100644 index 0000000000..89b4d281f8 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-uncond-access-hoists-other-dep.expect.md @@ -0,0 +1,92 @@ + +## Input + +```javascript +// @enablePropagateDepsInHIR + +import {identity, makeArray, Stringify, useIdentity} from 'shared-runtime'; + +function Foo({a, cond}) { + // Assume fn will be uncond evaluated, so we can safely evaluate {a., + // a.b. [a, a.b.c]; + useIdentity(null); + const x = makeArray(); + if (cond) { + x.push(identity(a.b.c)); + } + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{a: null, cond: true}], + sequentialRenders: [ + {a: null, cond: true}, + {a: {b: {c: 4}}, cond: true}, + {a: {b: {c: 4}}, cond: true}, + ], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @enablePropagateDepsInHIR + +import { identity, makeArray, Stringify, useIdentity } from "shared-runtime"; + +function Foo(t0) { + const $ = _c(8); + const { a, cond } = t0; + let t1; + if ($[0] !== a) { + t1 = () => [a, a.b.c]; + $[0] = a; + $[1] = t1; + } else { + t1 = $[1]; + } + const fn = t1; + useIdentity(null); + let x; + if ($[2] !== cond || $[3] !== a.b.c) { + x = makeArray(); + if (cond) { + x.push(identity(a.b.c)); + } + $[2] = cond; + $[3] = a.b.c; + $[4] = x; + } else { + x = $[4]; + } + let t2; + if ($[5] !== fn || $[6] !== x) { + t2 = ; + $[5] = fn; + $[6] = x; + $[7] = t2; + } else { + t2 = $[7]; + } + return t2; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{ a: null, cond: true }], + sequentialRenders: [ + { a: null, cond: true }, + { a: { b: { c: 4 } }, cond: true }, + { a: { b: { c: 4 } }, cond: true }, + ], +}; + +``` + +### Eval output +(kind: ok) [[ (exception in render) TypeError: Cannot read properties of null (reading 'b') ]] +
{"fn":{"kind":"Function","result":[{"b":{"c":4}},4]},"x":[4],"shouldInvokeFns":true}
+
{"fn":{"kind":"Function","result":[{"b":{"c":4}},4]},"x":[4],"shouldInvokeFns":true}
\ 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/infer-function-uncond-access-hoists-other-dep.tsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-uncond-access-hoists-other-dep.tsx new file mode 100644 index 0000000000..a9956ed8a5 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-uncond-access-hoists-other-dep.tsx @@ -0,0 +1,25 @@ +// @enablePropagateDepsInHIR + +import {identity, makeArray, Stringify, useIdentity} from 'shared-runtime'; + +function Foo({a, cond}) { + // Assume fn will be uncond evaluated, so we can safely evaluate {a., + // a.b. [a, a.b.c]; + useIdentity(null); + const x = makeArray(); + if (cond) { + x.push(identity(a.b.c)); + } + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{a: null, cond: true}], + sequentialRenders: [ + {a: null, cond: true}, + {a: {b: {c: 4}}, cond: true}, + {a: {b: {c: 4}}, cond: true}, + ], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-uncond-access-local-var.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-uncond-access-local-var.expect.md new file mode 100644 index 0000000000..741a30d7de --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-uncond-access-local-var.expect.md @@ -0,0 +1,73 @@ + +## Input + +```javascript +// @enablePropagateDepsInHIR + +import {mutate, shallowCopy, Stringify} from 'shared-runtime'; + +function useFoo({a}: {a: {b: {c: number}}}) { + const local = shallowCopy(a); + mutate(local); + const fn = () => local.b.c; + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{a: null}], + sequentialRenders: [{a: null}, {a: {b: {c: 4}}}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @enablePropagateDepsInHIR + +import { mutate, shallowCopy, Stringify } from "shared-runtime"; + +function useFoo(t0) { + const $ = _c(6); + const { a } = t0; + let local; + if ($[0] !== a) { + local = shallowCopy(a); + mutate(local); + $[0] = a; + $[1] = local; + } else { + local = $[1]; + } + let t1; + if ($[2] !== local.b.c) { + t1 = () => local.b.c; + $[2] = local.b.c; + $[3] = t1; + } else { + t1 = $[3]; + } + const fn = t1; + let t2; + if ($[4] !== fn) { + t2 = ; + $[4] = fn; + $[5] = t2; + } else { + t2 = $[5]; + } + return t2; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{ a: null }], + sequentialRenders: [{ a: null }, { a: { b: { c: 4 } } }], +}; + +``` + +### Eval output +(kind: ok) [[ (exception in render) TypeError: Cannot read properties of undefined (reading 'c') ]] +
{"fn":{"kind":"Function","result":4},"shouldInvokeFns":true}
\ 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/infer-function-uncond-access-local-var.tsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-uncond-access-local-var.tsx new file mode 100644 index 0000000000..16a0964351 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-uncond-access-local-var.tsx @@ -0,0 +1,16 @@ +// @enablePropagateDepsInHIR + +import {mutate, shallowCopy, Stringify} from 'shared-runtime'; + +function useFoo({a}: {a: {b: {c: number}}}) { + const local = shallowCopy(a); + mutate(local); + const fn = () => local.b.c; + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{a: null}], + sequentialRenders: [{a: null}, {a: {b: {c: 4}}}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-uncond-optional-hoists-other-dep.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-uncond-optional-hoists-other-dep.expect.md new file mode 100644 index 0000000000..591e04de7b --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-uncond-optional-hoists-other-dep.expect.md @@ -0,0 +1,91 @@ + +## Input + +```javascript +// @enablePropagateDepsInHIR + +import {identity, makeArray, Stringify, useIdentity} from 'shared-runtime'; + +function Foo({a, cond}) { + // Assume fn can be uncond evaluated, so we can safely evaluate a.b?.c. + const fn = () => [a, a.b?.c.d]; + useIdentity(null); + const arr = makeArray(); + if (cond) { + arr.push(identity(a.b?.c.e)); + } + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{a: null, cond: true}], + sequentialRenders: [ + {a: null, cond: true}, + {a: {b: {c: {d: 5}}}, cond: true}, + {a: {b: null}, cond: false}, + ], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @enablePropagateDepsInHIR + +import { identity, makeArray, Stringify, useIdentity } from "shared-runtime"; + +function Foo(t0) { + const $ = _c(8); + const { a, cond } = t0; + let t1; + if ($[0] !== a) { + t1 = () => [a, a.b?.c.d]; + $[0] = a; + $[1] = t1; + } else { + t1 = $[1]; + } + const fn = t1; + useIdentity(null); + let arr; + if ($[2] !== cond || $[3] !== a.b?.c.e) { + arr = makeArray(); + if (cond) { + arr.push(identity(a.b?.c.e)); + } + $[2] = cond; + $[3] = a.b?.c.e; + $[4] = arr; + } else { + arr = $[4]; + } + let t2; + if ($[5] !== fn || $[6] !== arr) { + t2 = ; + $[5] = fn; + $[6] = arr; + $[7] = t2; + } else { + t2 = $[7]; + } + return t2; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{ a: null, cond: true }], + sequentialRenders: [ + { a: null, cond: true }, + { a: { b: { c: { d: 5 } } }, cond: true }, + { a: { b: null }, cond: false }, + ], +}; + +``` + +### Eval output +(kind: ok) [[ (exception in render) TypeError: Cannot read properties of null (reading 'b') ]] +
{"fn":{"kind":"Function","result":[{"b":{"c":{"d":5}}},5]},"arr":[null],"shouldInvokeFns":true}
+
{"fn":{"kind":"Function","result":[{"b":null},null]},"arr":[],"shouldInvokeFns":true}
\ 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/infer-function-uncond-optional-hoists-other-dep.tsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-uncond-optional-hoists-other-dep.tsx new file mode 100644 index 0000000000..3b538de991 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-uncond-optional-hoists-other-dep.tsx @@ -0,0 +1,24 @@ +// @enablePropagateDepsInHIR + +import {identity, makeArray, Stringify, useIdentity} from 'shared-runtime'; + +function Foo({a, cond}) { + // Assume fn can be uncond evaluated, so we can safely evaluate a.b?.c. + const fn = () => [a, a.b?.c.d]; + useIdentity(null); + const arr = makeArray(); + if (cond) { + arr.push(identity(a.b?.c.e)); + } + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{a: null, cond: true}], + sequentialRenders: [ + {a: null, cond: true}, + {a: {b: {c: {d: 5}}}, cond: true}, + {a: {b: null}, cond: false}, + ], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-nested-function-uncond-access-local-var.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-nested-function-uncond-access-local-var.expect.md new file mode 100644 index 0000000000..ca65ce72bc --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-nested-function-uncond-access-local-var.expect.md @@ -0,0 +1,73 @@ + +## Input + +```javascript +// @enablePropagateDepsInHIR + +import {shallowCopy, Stringify, mutate} from 'shared-runtime'; + +function useFoo({a}: {a: {b: {c: number}}}) { + const local = shallowCopy(a); + mutate(local); + const fn = () => [() => local.b.c]; + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{a: null}], + sequentialRenders: [{a: null}, {a: {b: {c: 4}}}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @enablePropagateDepsInHIR + +import { shallowCopy, Stringify, mutate } from "shared-runtime"; + +function useFoo(t0) { + const $ = _c(6); + const { a } = t0; + let local; + if ($[0] !== a) { + local = shallowCopy(a); + mutate(local); + $[0] = a; + $[1] = local; + } else { + local = $[1]; + } + let t1; + if ($[2] !== local.b.c) { + t1 = () => [() => local.b.c]; + $[2] = local.b.c; + $[3] = t1; + } else { + t1 = $[3]; + } + const fn = t1; + let t2; + if ($[4] !== fn) { + t2 = ; + $[4] = fn; + $[5] = t2; + } else { + t2 = $[5]; + } + return t2; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{ a: null }], + sequentialRenders: [{ a: null }, { a: { b: { c: 4 } } }], +}; + +``` + +### Eval output +(kind: ok) [[ (exception in render) TypeError: Cannot read properties of undefined (reading 'c') ]] +
{"fn":{"kind":"Function","result":[{"kind":"Function","result":4}]},"shouldInvokeFns":true}
\ 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/infer-nested-function-uncond-access-local-var.tsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-nested-function-uncond-access-local-var.tsx new file mode 100644 index 0000000000..d351a19464 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-nested-function-uncond-access-local-var.tsx @@ -0,0 +1,16 @@ +// @enablePropagateDepsInHIR + +import {shallowCopy, Stringify, mutate} from 'shared-runtime'; + +function useFoo({a}: {a: {b: {c: number}}}) { + const local = shallowCopy(a); + mutate(local); + const fn = () => [() => local.b.c]; + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{a: null}], + sequentialRenders: [{a: null}, {a: {b: {c: 4}}}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-nested-function-uncond-access.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-nested-function-uncond-access.expect.md new file mode 100644 index 0000000000..22b17977cb --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-nested-function-uncond-access.expect.md @@ -0,0 +1,66 @@ + +## Input + +```javascript +// @enablePropagateDepsInHIR + +import {Stringify} from 'shared-runtime'; + +function useFoo({a}) { + const fn = () => { + return () => ({ + value: a.b.c, + }); + }; + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{a: null}], + sequentialRenders: [{a: null}, {a: {b: {c: 4}}}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @enablePropagateDepsInHIR + +import { Stringify } from "shared-runtime"; + +function useFoo(t0) { + const $ = _c(4); + const { a } = t0; + let t1; + if ($[0] !== a.b.c) { + t1 = () => () => ({ value: a.b.c }); + $[0] = a.b.c; + $[1] = t1; + } else { + t1 = $[1]; + } + const fn = t1; + let t2; + if ($[2] !== fn) { + t2 = ; + $[2] = fn; + $[3] = t2; + } else { + t2 = $[3]; + } + return t2; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{ a: null }], + sequentialRenders: [{ a: null }, { a: { b: { c: 4 } } }], +}; + +``` + +### Eval output +(kind: ok) [[ (exception in render) TypeError: Cannot read properties of null (reading 'b') ]] +
{"fn":{"kind":"Function","result":{"kind":"Function","result":{"value":4}}},"shouldInvokeFns":true}
\ 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/infer-nested-function-uncond-access.tsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-nested-function-uncond-access.tsx new file mode 100644 index 0000000000..41d004a689 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-nested-function-uncond-access.tsx @@ -0,0 +1,18 @@ +// @enablePropagateDepsInHIR + +import {Stringify} from 'shared-runtime'; + +function useFoo({a}) { + const fn = () => { + return () => ({ + value: a.b.c, + }); + }; + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{a: null}], + sequentialRenders: [{a: null}, {a: {b: {c: 4}}}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-object-method-uncond-access.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-object-method-uncond-access.expect.md new file mode 100644 index 0000000000..7d75470550 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-object-method-uncond-access.expect.md @@ -0,0 +1,70 @@ + +## Input + +```javascript +// @enablePropagateDepsInHIR + +import {identity, Stringify} from 'shared-runtime'; + +function useFoo({a}) { + const x = { + fn() { + return identity(a.b.c); + }, + }; + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{a: null}], + sequentialRenders: [{a: null}, {a: {b: {c: 4}}}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @enablePropagateDepsInHIR + +import { identity, Stringify } from "shared-runtime"; + +function useFoo(t0) { + const $ = _c(4); + const { a } = t0; + let t1; + if ($[0] !== a.b.c) { + t1 = { + fn() { + return identity(a.b.c); + }, + }; + $[0] = a.b.c; + $[1] = t1; + } else { + t1 = $[1]; + } + const x = t1; + let t2; + if ($[2] !== x) { + t2 = ; + $[2] = x; + $[3] = t2; + } else { + t2 = $[3]; + } + return t2; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{ a: null }], + sequentialRenders: [{ a: null }, { a: { b: { c: 4 } } }], +}; + +``` + +### Eval output +(kind: ok) [[ (exception in render) TypeError: Cannot read properties of null (reading 'b') ]] +
{"x":{"fn":{"kind":"Function","result":4}},"shouldInvokeFns":true}
\ 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/infer-object-method-uncond-access.tsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-object-method-uncond-access.tsx new file mode 100644 index 0000000000..b05b482bd3 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-object-method-uncond-access.tsx @@ -0,0 +1,18 @@ +// @enablePropagateDepsInHIR + +import {identity, Stringify} from 'shared-runtime'; + +function useFoo({a}) { + const x = { + fn() { + return identity(a.b.c); + }, + }; + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{a: null}], + sequentialRenders: [{a: null}, {a: {b: {c: 4}}}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/todo-infer-function-uncond-optionals-hoisted.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/todo-infer-function-uncond-optionals-hoisted.expect.md new file mode 100644 index 0000000000..02e60eff91 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/todo-infer-function-uncond-optionals-hoisted.expect.md @@ -0,0 +1,64 @@ + +## Input + +```javascript +// @enablePropagateDepsInHIR + +import {Stringify} from 'shared-runtime'; + +function useFoo({a}) { + return a.b?.c.d?.e} shouldInvokeFns={true} />; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{a: null}], + sequentialRenders: [ + {a: null}, + {a: {b: null}}, + {a: {b: {c: {d: null}}}}, + {a: {b: {c: {d: {e: 4}}}}}, + ], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @enablePropagateDepsInHIR + +import { Stringify } from "shared-runtime"; + +function useFoo(t0) { + const $ = _c(2); + const { a } = t0; + let t1; + if ($[0] !== a.b) { + t1 = a.b?.c.d?.e} shouldInvokeFns={true} />; + $[0] = a.b; + $[1] = t1; + } else { + t1 = $[1]; + } + return t1; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{ a: null }], + sequentialRenders: [ + { a: null }, + { a: { b: null } }, + { a: { b: { c: { d: null } } } }, + { a: { b: { c: { d: { e: 4 } } } } }, + ], +}; + +``` + +### Eval output +(kind: ok) [[ (exception in render) TypeError: Cannot read properties of null (reading 'b') ]] +
{"fn":{"kind":"Function"},"shouldInvokeFns":true}
+
{"fn":{"kind":"Function"},"shouldInvokeFns":true}
+
{"fn":{"kind":"Function","result":4},"shouldInvokeFns":true}
\ 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/todo-infer-function-uncond-optionals-hoisted.tsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/todo-infer-function-uncond-optionals-hoisted.tsx new file mode 100644 index 0000000000..4a2072131e --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/todo-infer-function-uncond-optionals-hoisted.tsx @@ -0,0 +1,18 @@ +// @enablePropagateDepsInHIR + +import {Stringify} from 'shared-runtime'; + +function useFoo({a}) { + return a.b?.c.d?.e} shouldInvokeFns={true} />; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{a: null}], + sequentialRenders: [ + {a: null}, + {a: {b: null}}, + {a: {b: {c: {d: null}}}}, + {a: {b: {c: {d: {e: 4}}}}}, + ], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/bug-infer-function-cond-access-not-hoisted.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/bug-infer-function-cond-access-not-hoisted.expect.md new file mode 100644 index 0000000000..4d45d3f3c6 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/bug-infer-function-cond-access-not-hoisted.expect.md @@ -0,0 +1,73 @@ + +## Input + +```javascript +import {Stringify} from 'shared-runtime'; + +function Foo({a, shouldReadA}) { + return ( + { + if (shouldReadA) return a.b.c; + return null; + }} + shouldInvokeFns={true} + /> + ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{a: null, shouldReadA: true}], + sequentialRenders: [ + {a: null, shouldReadA: true}, + {a: null, shouldReadA: false}, + {a: {b: {c: 4}}, shouldReadA: true}, + ], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { Stringify } from "shared-runtime"; + +function Foo(t0) { + const $ = _c(3); + const { a, shouldReadA } = t0; + let t1; + if ($[0] !== shouldReadA || $[1] !== a.b.c) { + t1 = ( + { + if (shouldReadA) { + return a.b.c; + } + return null; + }} + shouldInvokeFns={true} + /> + ); + $[0] = shouldReadA; + $[1] = a.b.c; + $[2] = t1; + } else { + t1 = $[2]; + } + return t1; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{ a: null, shouldReadA: true }], + sequentialRenders: [ + { a: null, shouldReadA: true }, + { a: null, shouldReadA: false }, + { a: { b: { c: 4 } }, shouldReadA: true }, + ], +}; + +``` + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/bug-infer-function-cond-access-not-hoisted.tsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/bug-infer-function-cond-access-not-hoisted.tsx new file mode 100644 index 0000000000..e571ee7b95 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/bug-infer-function-cond-access-not-hoisted.tsx @@ -0,0 +1,23 @@ +import {Stringify} from 'shared-runtime'; + +function Foo({a, shouldReadA}) { + return ( + { + if (shouldReadA) return a.b.c; + return null; + }} + shouldInvokeFns={true} + /> + ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{a: null, shouldReadA: true}], + sequentialRenders: [ + {a: null, shouldReadA: true}, + {a: null, shouldReadA: false}, + {a: {b: {c: 4}}, shouldReadA: true}, + ], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/todo-infer-function-uncond-optionals-hoisted.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/todo-infer-function-uncond-optionals-hoisted.expect.md new file mode 100644 index 0000000000..157e2de81a --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/todo-infer-function-uncond-optionals-hoisted.expect.md @@ -0,0 +1,61 @@ + +## Input + +```javascript +import {Stringify} from 'shared-runtime'; + +function useFoo({a}) { + return a.b?.c.d?.e} shouldInvokeFns={true} />; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{a: null}], + sequentialRenders: [ + {a: null}, + {a: {b: null}}, + {a: {b: {c: {d: null}}}}, + {a: {b: {c: {d: {e: 4}}}}}, + ], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { Stringify } from "shared-runtime"; + +function useFoo(t0) { + const $ = _c(2); + const { a } = t0; + let t1; + if ($[0] !== a.b) { + t1 = a.b?.c.d?.e} shouldInvokeFns={true} />; + $[0] = a.b; + $[1] = t1; + } else { + t1 = $[1]; + } + return t1; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{ a: null }], + sequentialRenders: [ + { a: null }, + { a: { b: null } }, + { a: { b: { c: { d: null } } } }, + { a: { b: { c: { d: { e: 4 } } } } }, + ], +}; + +``` + +### Eval output +(kind: ok) [[ (exception in render) TypeError: Cannot read properties of null (reading 'b') ]] +
{"fn":{"kind":"Function"},"shouldInvokeFns":true}
+
{"fn":{"kind":"Function"},"shouldInvokeFns":true}
+
{"fn":{"kind":"Function","result":4},"shouldInvokeFns":true}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/todo-infer-function-uncond-optionals-hoisted.tsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/todo-infer-function-uncond-optionals-hoisted.tsx new file mode 100644 index 0000000000..11d5858434 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/todo-infer-function-uncond-optionals-hoisted.tsx @@ -0,0 +1,16 @@ +import {Stringify} from 'shared-runtime'; + +function useFoo({a}) { + return a.b?.c.d?.e} shouldInvokeFns={true} />; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{a: null}], + sequentialRenders: [ + {a: null}, + {a: {b: null}}, + {a: {b: {c: {d: null}}}}, + {a: {b: {c: {d: {e: 4}}}}}, + ], +}; diff --git a/compiler/packages/snap/src/SproutTodoFilter.ts b/compiler/packages/snap/src/SproutTodoFilter.ts index c40392884d..8597f66dbd 100644 --- a/compiler/packages/snap/src/SproutTodoFilter.ts +++ b/compiler/packages/snap/src/SproutTodoFilter.ts @@ -479,6 +479,7 @@ const skipFilter = new Set([ 'fbt/bug-fbt-plural-multiple-mixed-call-tag', 'bug-invalid-hoisting-functionexpr', 'bug-try-catch-maybe-null-dependency', + 'reduce-reactive-deps/bug-infer-function-cond-access-not-hoisted', 'reduce-reactive-deps/bug-merge-uncond-optional-chain-and-cond', 'original-reactive-scopes-fork/bug-nonmutating-capture-in-unsplittable-memo-block', 'original-reactive-scopes-fork/bug-hoisted-declaration-with-scope', From 91c42a14c7a698fe6baeab770d3c2548fcdf32b4 Mon Sep 17 00:00:00 2001 From: lauren Date: Mon, 7 Oct 2024 12:39:14 -0400 Subject: [PATCH 06/23] [rcr][ez] Clean up unused $read from rcr (#31136) --- compiler/packages/react-compiler-runtime/src/index.ts | 8 -------- 1 file changed, 8 deletions(-) diff --git a/compiler/packages/react-compiler-runtime/src/index.ts b/compiler/packages/react-compiler-runtime/src/index.ts index cefc934ae1..823a49ab81 100644 --- a/compiler/packages/react-compiler-runtime/src/index.ts +++ b/compiler/packages/react-compiler-runtime/src/index.ts @@ -36,14 +36,6 @@ export function c(size: number) { })[0]; } -export function $read(memoCache: MemoCache, index: number) { - const value = memoCache[index]; - if (value === $empty) { - throw new Error('useMemoCache: read before write'); - } - return value; -} - const LazyGuardDispatcher: {[key: string]: (...args: Array) => any} = {}; [ 'readContext', From 68d59d43d5640f7e44b46bfa7ee758de063767b4 Mon Sep 17 00:00:00 2001 From: mofeiZ <34200447+mofeiZ@users.noreply.github.com> Date: Mon, 7 Oct 2024 13:09:39 -0400 Subject: [PATCH 07/23] [compiler][ez] Fix reanimated custom type defs for imports (#31137) When we added support for Reanimated, we didn't distinguish between true globals (i.e. identifiers with no static resolutions), module types, and imports #29188. For the past 3-4 months, Reanimated imports were not being matched to the correct hook / function shape we match globals and module imports against two different registries. This PR fixes our support for Reanimated library functions imported under `react-native-reanimated`. See test fixtures for details --- .../src/HIR/Environment.ts | 13 +++-- .../src/HIR/Globals.ts | 22 +++---- ...d-reanimated-shared-value-writes.expect.md | 36 ++++++++++++ ...mported-reanimated-shared-value-writes.jsx | 15 +++++ .../compiler/reanimated-no-memo-arg.expect.md | 2 + .../compiler/reanimated-no-memo-arg.js | 1 + .../reanimated-shared-value-writes.expect.md | 57 +++++++++++++++++++ .../reanimated-shared-value-writes.jsx | 18 ++++++ .../packages/snap/src/SproutTodoFilter.ts | 1 + 9 files changed, 149 insertions(+), 16 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-non-imported-reanimated-shared-value-writes.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-non-imported-reanimated-shared-value-writes.jsx create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reanimated-shared-value-writes.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reanimated-shared-value-writes.jsx diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts index 50905bc581..b7d2b645c5 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts @@ -16,7 +16,7 @@ import { DEFAULT_SHAPES, Global, GlobalRegistry, - installReAnimatedTypes, + getReanimatedModuleType, installTypeConfig, } from './Globals'; import { @@ -688,7 +688,8 @@ export class Environment { } if (config.enableCustomTypeDefinitionForReanimated) { - installReAnimatedTypes(this.#globals, this.#shapes); + const reanimatedModuleType = getReanimatedModuleType(this.#shapes); + this.#moduleTypes.set(REANIMATED_MODULE_NAME, reanimatedModuleType); } this.#contextIdentifiers = contextIdentifiers; @@ -734,11 +735,11 @@ export class Environment { } #resolveModuleType(moduleName: string, loc: SourceLocation): Global | null { - if (this.config.moduleTypeProvider == null) { - return null; - } let moduleType = this.#moduleTypes.get(moduleName); if (moduleType === undefined) { + if (this.config.moduleTypeProvider == null) { + return null; + } const unparsedModuleConfig = this.config.moduleTypeProvider(moduleName); if (unparsedModuleConfig != null) { const parsedModuleConfig = TypeSchema.safeParse(unparsedModuleConfig); @@ -957,6 +958,8 @@ export class Environment { } } +const REANIMATED_MODULE_NAME = 'react-native-reanimated'; + // From https://github.com/facebook/react/blob/main/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js#LL18C1-L23C2 export function isHookName(name: string): boolean { return /^use[A-Z0-9]/.test(name); diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/Globals.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/Globals.ts index 1128c51cae..2525b87bd8 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Globals.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Globals.ts @@ -25,7 +25,7 @@ import { addHook, addObject, } from './ObjectShape'; -import {BuiltInType, PolyType} from './Types'; +import {BuiltInType, ObjectType, PolyType} from './Types'; import {TypeConfig} from './TypeSchema'; import {assertExhaustive} from '../Utils/utils'; import {isHookName} from './Environment'; @@ -652,10 +652,7 @@ export function installTypeConfig( } } -export function installReAnimatedTypes( - globals: GlobalRegistry, - registry: ShapeRegistry, -): void { +export function getReanimatedModuleType(registry: ShapeRegistry): ObjectType { // hooks that freeze args and return frozen value const frozenHooks = [ 'useFrameCallback', @@ -665,8 +662,9 @@ export function installReAnimatedTypes( 'useAnimatedReaction', 'useWorkletCallback', ]; + const reanimatedType: Array<[string, BuiltInType]> = []; for (const hook of frozenHooks) { - globals.set( + reanimatedType.push([ hook, addHook(registry, { positionalParams: [], @@ -677,7 +675,7 @@ export function installReAnimatedTypes( calleeEffect: Effect.Read, hookKind: 'Custom', }), - ); + ]); } /** @@ -686,7 +684,7 @@ export function installReAnimatedTypes( */ const mutableHooks = ['useSharedValue', 'useDerivedValue']; for (const hook of mutableHooks) { - globals.set( + reanimatedType.push([ hook, addHook(registry, { positionalParams: [], @@ -697,7 +695,7 @@ export function installReAnimatedTypes( calleeEffect: Effect.Read, hookKind: 'Custom', }), - ); + ]); } // functions that return mutable value @@ -711,7 +709,7 @@ export function installReAnimatedTypes( 'executeOnUIRuntimeSync', ]; for (const fn of funcs) { - globals.set( + reanimatedType.push([ fn, addFunction(registry, [], { positionalParams: [], @@ -721,6 +719,8 @@ export function installReAnimatedTypes( returnValueKind: ValueKind.Mutable, noAlias: true, }), - ); + ]); } + + return addObject(registry, null, reanimatedType); } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-non-imported-reanimated-shared-value-writes.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-non-imported-reanimated-shared-value-writes.expect.md new file mode 100644 index 0000000000..f1399a41b6 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-non-imported-reanimated-shared-value-writes.expect.md @@ -0,0 +1,36 @@ + +## Input + +```javascript +// @enableCustomTypeDefinitionForReanimated + +/** + * Test that a global (i.e. non-imported) useSharedValue is treated as an + * unknown hook. + */ +function SomeComponent() { + const sharedVal = useSharedValue(0); + return ( + + }}> + Click me + ); } @@ -28,7 +32,9 @@ export const FIXTURE_ENTRYPOINT = { ## Code ```javascript -import { c as _c } from "react-forget-runtime"; // @runtimeModule="react-forget-runtime" +import { c as _c } from "react/compiler-runtime"; // @runtimeModule="react-compiler-runtime" +import { useState } from "react"; + function Component(props) { const $ = _c(5); const [x, setX] = useState(1); @@ -48,11 +54,13 @@ function Component(props) { let t1; if ($[3] !== t0) { t1 = ( - ); $[3] = t0; $[4] = t1; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/userspace-use-memo-cache.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/userspace-use-memo-cache.js index 013a1be330..1e6d5f435d 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/userspace-use-memo-cache.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/userspace-use-memo-cache.js @@ -1,4 +1,6 @@ -// @runtimeModule="react-forget-runtime" +// @runtimeModule="react-compiler-runtime" +import {useState} from 'react'; + function Component(props) { const [x, setX] = useState(1); let y; @@ -6,10 +8,12 @@ function Component(props) { y = x * 2; } return ( - + }}> + Click me + ); } From 23cd3aca283817b8c359e806e9c7bc6b26fcd27c Mon Sep 17 00:00:00 2001 From: lauren Date: Mon, 7 Oct 2024 18:07:12 -0400 Subject: [PATCH 14/23] [rcr] Remove runtimeModule compiler option (#31145) Now that the compiler always injects `react-compiler-runtime`, this option is unnecessary. --- .../src/Entrypoint/Options.ts | 12 --- .../userspace-use-memo-cache.expect.md | 80 ------------------- .../compiler/userspace-use-memo-cache.js | 24 ------ compiler/packages/snap/src/compiler.ts | 6 -- 4 files changed, 122 deletions(-) delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/userspace-use-memo-cache.expect.md delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/userspace-use-memo-cache.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Options.ts b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Options.ts index b092800a2e..10bcebe44e 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Options.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Options.ts @@ -87,17 +87,6 @@ export type PluginOptions = { */ compilationMode: CompilationMode; - /* - * If enabled, Forget will import `useMemoCache` from the given module - * instead of `react/compiler-runtime`. - * - * ``` - * // If set to "react-compiler-runtime" - * import {c as useMemoCache} from 'react-compiler-runtime'; - * ``` - */ - runtimeModule?: string | null | undefined; - /** * By default React Compiler will skip compilation of code that suppresses the default * React ESLint rules, since this is a strong indication that the code may be breaking React rules @@ -214,7 +203,6 @@ export const defaultOptions: PluginOptions = { logger: null, gating: null, noEmit: false, - runtimeModule: null, eslintSuppressionRules: null, flowSuppressions: true, ignoreUseNoForget: false, diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/userspace-use-memo-cache.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/userspace-use-memo-cache.expect.md deleted file mode 100644 index 972d106907..0000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/userspace-use-memo-cache.expect.md +++ /dev/null @@ -1,80 +0,0 @@ - -## Input - -```javascript -// @runtimeModule="react-compiler-runtime" -import {useState} from 'react'; - -function Component(props) { - const [x, setX] = useState(1); - let y; - if (props.cond) { - y = x * 2; - } - return ( - - ); -} - -export const FIXTURE_ENTRYPOINT = { - fn: Component, - params: [true], - isComponent: true, -}; - -``` - -## Code - -```javascript -import { c as _c } from "react/compiler-runtime"; // @runtimeModule="react-compiler-runtime" -import { useState } from "react"; - -function Component(props) { - const $ = _c(5); - const [x, setX] = useState(1); - let y; - if ($[0] !== props.cond || $[1] !== x) { - if (props.cond) { - y = x * 2; - } - $[0] = props.cond; - $[1] = x; - $[2] = y; - } else { - y = $[2]; - } - - const t0 = y; - let t1; - if ($[3] !== t0) { - t1 = ( - - ); - $[3] = t0; - $[4] = t1; - } else { - t1 = $[4]; - } - return t1; -} - -export const FIXTURE_ENTRYPOINT = { - fn: Component, - params: [true], - isComponent: true, -}; - -``` - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/userspace-use-memo-cache.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/userspace-use-memo-cache.js deleted file mode 100644 index 1e6d5f435d..0000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/userspace-use-memo-cache.js +++ /dev/null @@ -1,24 +0,0 @@ -// @runtimeModule="react-compiler-runtime" -import {useState} from 'react'; - -function Component(props) { - const [x, setX] = useState(1); - let y; - if (props.cond) { - y = x * 2; - } - return ( - - ); -} - -export const FIXTURE_ENTRYPOINT = { - fn: Component, - params: [true], - isComponent: true, -}; diff --git a/compiler/packages/snap/src/compiler.ts b/compiler/packages/snap/src/compiler.ts index 72212ea0e9..cd907575fb 100644 --- a/compiler/packages/snap/src/compiler.ts +++ b/compiler/packages/snap/src/compiler.ts @@ -48,7 +48,6 @@ function makePluginOptions( let enableEmitFreeze = null; let enableEmitHookGuards = null; let compilationMode: CompilationMode = 'all'; - let runtimeModule = null; let panicThreshold: PanicThresholdOptions = 'all_errors'; let hookPattern: string | null = null; // TODO(@mofeiZ) rewrite snap fixtures to @validatePreserveExistingMemo:false @@ -104,10 +103,6 @@ function makePluginOptions( importSpecifierName: '$dispatcherGuard', }; } - const runtimeModuleMatch = /@runtimeModule="([^"]+)"/.exec(firstLine); - if (runtimeModuleMatch) { - runtimeModule = runtimeModuleMatch[1]; - } const targetMatch = /@target="([^"]+)"/.exec(firstLine); if (targetMatch) { @@ -251,7 +246,6 @@ function makePluginOptions( gating, panicThreshold, noEmit: false, - runtimeModule, eslintSuppressionRules, flowSuppressions, ignoreUseNoForget, From f74f6cd945675158eb40402041305e7af4ce731c Mon Sep 17 00:00:00 2001 From: lauren Date: Mon, 7 Oct 2024 18:50:07 -0400 Subject: [PATCH 15/23] [rcr] Publish react-compiler-runtime to npm (#31146) Updates our publishing scripts to also publish react-compiler-runtime. --- compiler/packages/react-compiler-runtime/README.md | 5 +++++ compiler/scripts/release/shared/packages.js | 1 + 2 files changed, 6 insertions(+) create mode 100644 compiler/packages/react-compiler-runtime/README.md diff --git a/compiler/packages/react-compiler-runtime/README.md b/compiler/packages/react-compiler-runtime/README.md new file mode 100644 index 0000000000..8f650f962d --- /dev/null +++ b/compiler/packages/react-compiler-runtime/README.md @@ -0,0 +1,5 @@ +# react-compiler-runtime + +Backwards compatible shim for runtime APIs used by React Compiler. Primarily meant for React versions prior to 19, but it will also work on > 19. + +See also https://github.com/reactwg/react-compiler/discussions/6. diff --git a/compiler/scripts/release/shared/packages.js b/compiler/scripts/release/shared/packages.js index c5513d1470..533041d119 100644 --- a/compiler/scripts/release/shared/packages.js +++ b/compiler/scripts/release/shared/packages.js @@ -2,6 +2,7 @@ const PUBLISHABLE_PACKAGES = [ 'babel-plugin-react-compiler', 'eslint-plugin-react-compiler', 'react-compiler-healthcheck', + 'react-compiler-runtime', ]; module.exports = { From ed966dac4a025fd37580e8197e5b271044ffbd9f Mon Sep 17 00:00:00 2001 From: lauren Date: Mon, 7 Oct 2024 20:37:41 -0400 Subject: [PATCH 16/23] [compiler] Fix busted postinstall script (#31147) --- compiler/packages/babel-plugin-react-compiler/package.json | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/package.json b/compiler/packages/babel-plugin-react-compiler/package.json index d9bfe9de73..5891ce2b85 100644 --- a/compiler/packages/babel-plugin-react-compiler/package.json +++ b/compiler/packages/babel-plugin-react-compiler/package.json @@ -8,9 +8,8 @@ "dist" ], "scripts": { - "postinstall": "./scripts/link-react-compiler-runtime.sh", "build": "rimraf dist && rollup --config --bundleConfigAsCjs", - "test": "yarn snap:ci", + "test": "./scripts/link-react-compiler-runtime.sh && yarn snap:ci", "jest": "yarn build && ts-node node_modules/.bin/jest", "snap": "node ../snap/dist/main.js", "snap:build": "yarn workspace snap run build", From bf0c054649f0573c184499bd571f08150152c086 Mon Sep 17 00:00:00 2001 From: Ruslan Lesiutin Date: Wed, 9 Oct 2024 09:54:34 +0100 Subject: [PATCH 17/23] fix[react-devtools]: wrap key string in preformatted text html element (#31153) Fixes https://github.com/facebook/react/issues/28984. --- .../src/devtools/views/Components/Element.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-devtools-shared/src/devtools/views/Components/Element.js b/packages/react-devtools-shared/src/devtools/views/Components/Element.js index 48bfbe9090..b58451cb51 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/Element.js +++ b/packages/react-devtools-shared/src/devtools/views/Components/Element.js @@ -166,7 +166,7 @@ export default function Element({data, index, style}: Props): React.Node { className={styles.KeyValue} title={key} onDoubleClick={handleKeyDoubleClick}> - {key} +
{key}
" From dbf80c8d7a823041d83baff8b0dca8892ce27411 Mon Sep 17 00:00:00 2001 From: Ruslan Lesiutin Date: Wed, 9 Oct 2024 13:23:23 +0100 Subject: [PATCH 18/23] fix[react-devtools]: update profiling status before receiving response from backend (#31117) We can't wait for a response from Backend, because it might take some time to actually finish profiling. We should keep a flag on the frontend side, so user can quickly see the feedback in the UI. --- .../src/devtools/ProfilerStore.js | 52 ++++++++++++++----- .../src/devtools/store.js | 2 +- .../views/Profiler/ProfilerContext.js | 2 +- .../devtools/views/Settings/SettingsModal.js | 2 +- .../Settings/SettingsModalContextToggle.js | 2 +- 5 files changed, 42 insertions(+), 18 deletions(-) diff --git a/packages/react-devtools-shared/src/devtools/ProfilerStore.js b/packages/react-devtools-shared/src/devtools/ProfilerStore.js index a55487ec19..5ed7e69f29 100644 --- a/packages/react-devtools-shared/src/devtools/ProfilerStore.js +++ b/packages/react-devtools-shared/src/devtools/ProfilerStore.js @@ -11,6 +11,7 @@ import EventEmitter from '../events'; import {prepareProfilingDataFrontendFromBackendAndStore} from './views/Profiler/utils'; import ProfilingCache from './ProfilingCache'; import Store from './store'; +import {logEvent} from 'react-devtools-shared/src/Logger'; import type {FrontendBridge} from 'react-devtools-shared/src/bridge'; import type {ProfilingDataBackend} from 'react-devtools-shared/src/backend/types'; @@ -67,7 +68,12 @@ export default class ProfilerStore extends EventEmitter<{ // The backend is currently profiling. // When profiling is in progress, operations are stored so that we can later reconstruct past commit trees. - _isProfiling: boolean = false; + _isBackendProfiling: boolean = false; + + // Mainly used for optimistic UI. + // This could be false, but at the same time _isBackendProfiling could be true + // for cases when Backend is busy serializing a chunky payload. + _isProfilingBasedOnUserInput: boolean = false; // Tracks whether a specific renderer logged any profiling data during the most recent session. _rendererIDsThatReportedProfilingData: Set = new Set(); @@ -86,7 +92,8 @@ export default class ProfilerStore extends EventEmitter<{ super(); this._bridge = bridge; - this._isProfiling = defaultIsProfiling; + this._isBackendProfiling = defaultIsProfiling; + this._isProfilingBasedOnUserInput = defaultIsProfiling; this._store = store; bridge.addListener('operations', this.onBridgeOperations); @@ -139,8 +146,8 @@ export default class ProfilerStore extends EventEmitter<{ return this._rendererQueue.size > 0 || this._dataBackends.length > 0; } - get isProfiling(): boolean { - return this._isProfiling; + get isProfilingBasedOnUserInput(): boolean { + return this._isProfilingBasedOnUserInput; } get profilingCache(): ProfilingCache { @@ -151,7 +158,7 @@ export default class ProfilerStore extends EventEmitter<{ return this._dataFrontend; } set profilingData(value: ProfilingDataFrontend | null): void { - if (this._isProfiling) { + if (this._isBackendProfiling) { console.warn( 'Profiling data cannot be updated while profiling is in progress.', ); @@ -186,6 +193,9 @@ export default class ProfilerStore extends EventEmitter<{ startProfiling(): void { this._bridge.send('startProfiling', this._store.recordChangeDescriptions); + this._isProfilingBasedOnUserInput = true; + this.emit('isProfiling'); + // Don't actually update the local profiling boolean yet! // Wait for onProfilingStatus() to confirm the status has changed. // This ensures the frontend and backend are in sync wrt which commits were profiled. @@ -195,8 +205,12 @@ export default class ProfilerStore extends EventEmitter<{ stopProfiling(): void { this._bridge.send('stopProfiling'); - // Don't actually update the local profiling boolean yet! - // Wait for onProfilingStatus() to confirm the status has changed. + // Backend might be busy serializing the payload, so we are going to display + // optimistic UI to the user that profiling is stopping. + this._isProfilingBasedOnUserInput = false; + this.emit('isProfiling'); + + // Wait for onProfilingStatus() to confirm the status has changed, this will update _isBackendProfiling. // This ensures the frontend and backend are in sync wrt which commits were profiled. // We do this to avoid mismatches on e.g. CommitTreeBuilder that would cause errors. } @@ -229,7 +243,7 @@ export default class ProfilerStore extends EventEmitter<{ const rendererID = operations[0]; const rootID = operations[1]; - if (this._isProfiling) { + if (this._isBackendProfiling) { let profilingOperations = this._inProgressOperationsByRootID.get(rootID); if (profilingOperations == null) { profilingOperations = [operations]; @@ -252,8 +266,8 @@ export default class ProfilerStore extends EventEmitter<{ onBridgeProfilingData: (dataBackend: ProfilingDataBackend) => void = dataBackend => { - if (this._isProfiling) { - // This should never happen, but if it does- ignore previous profiling data. + if (this._isBackendProfiling) { + // This should never happen, but if it does, then ignore previous profiling data. return; } @@ -289,7 +303,7 @@ export default class ProfilerStore extends EventEmitter<{ }; onProfilingStatus: (isProfiling: boolean) => void = isProfiling => { - if (this._isProfiling === isProfiling) { + if (this._isBackendProfiling === isProfiling) { return; } @@ -319,15 +333,25 @@ export default class ProfilerStore extends EventEmitter<{ }); } - this._isProfiling = isProfiling; + this._isBackendProfiling = isProfiling; + // _isProfilingBasedOnUserInput should already be updated from startProfiling, stopProfiling, or constructor. + if (this._isProfilingBasedOnUserInput !== isProfiling) { + logEvent({ + event_name: 'error', + error_message: `Unexpected profiling status. Expected ${this._isProfilingBasedOnUserInput.toString()}, but received ${isProfiling.toString()}.`, + error_stack: new Error().stack, + error_component_stack: null, + }); + + // If happened, fallback to displaying the value from Backend + this._isProfilingBasedOnUserInput = isProfiling; + } // Invalidate suspense cache if profiling data is being (re-)recorded. // Note that we clear again, in case any views read from the cache while profiling. // (That would have resolved a now-stale value without any profiling data.) this._cache.invalidate(); - this.emit('isProfiling'); - // If we've just finished a profiling session, we need to fetch data stored in each renderer interface // and re-assemble it on the front-end into a format (ProfilingDataFrontend) that can power the Profiler UI. // During this time, DevTools UI should probably not be interactive. diff --git a/packages/react-devtools-shared/src/devtools/store.js b/packages/react-devtools-shared/src/devtools/store.js index 8af997d928..9f4343beab 100644 --- a/packages/react-devtools-shared/src/devtools/store.js +++ b/packages/react-devtools-shared/src/devtools/store.js @@ -324,7 +324,7 @@ export default class Store extends EventEmitter<{ return this._componentFilters; } set componentFilters(value: Array): void { - if (this._profilerStore.isProfiling) { + if (this._profilerStore.isProfilingBasedOnUserInput) { // Re-mounting a tree while profiling is in progress might break a lot of assumptions. // If necessary, we could support this- but it doesn't seem like a necessary use case. this._throwAndEmitError( diff --git a/packages/react-devtools-shared/src/devtools/views/Profiler/ProfilerContext.js b/packages/react-devtools-shared/src/devtools/views/Profiler/ProfilerContext.js index f4ebc26a07..1ef7bb2b7e 100644 --- a/packages/react-devtools-shared/src/devtools/views/Profiler/ProfilerContext.js +++ b/packages/react-devtools-shared/src/devtools/views/Profiler/ProfilerContext.js @@ -98,7 +98,7 @@ function ProfilerContextController({children}: Props): React.Node { getCurrentValue: () => ({ didRecordCommits: profilerStore.didRecordCommits, isProcessingData: profilerStore.isProcessingData, - isProfiling: profilerStore.isProfiling, + isProfiling: profilerStore.isProfilingBasedOnUserInput, profilingData: profilerStore.profilingData, supportsProfiling: store.rootSupportsBasicProfiling, }), diff --git a/packages/react-devtools-shared/src/devtools/views/Settings/SettingsModal.js b/packages/react-devtools-shared/src/devtools/views/Settings/SettingsModal.js index f6652ada3a..023b342a0c 100644 --- a/packages/react-devtools-shared/src/devtools/views/Settings/SettingsModal.js +++ b/packages/react-devtools-shared/src/devtools/views/Settings/SettingsModal.js @@ -39,7 +39,7 @@ export default function SettingsModal(): React.Node { // Explicitly disallow it for now. const isProfilingSubscription = useMemo( () => ({ - getCurrentValue: () => profilerStore.isProfiling, + getCurrentValue: () => profilerStore.isProfilingBasedOnUserInput, subscribe: (callback: Function) => { profilerStore.addListener('isProfiling', callback); return () => profilerStore.removeListener('isProfiling', callback); diff --git a/packages/react-devtools-shared/src/devtools/views/Settings/SettingsModalContextToggle.js b/packages/react-devtools-shared/src/devtools/views/Settings/SettingsModalContextToggle.js index 306f071511..94fa5c4111 100644 --- a/packages/react-devtools-shared/src/devtools/views/Settings/SettingsModalContextToggle.js +++ b/packages/react-devtools-shared/src/devtools/views/Settings/SettingsModalContextToggle.js @@ -29,7 +29,7 @@ export default function SettingsModalContextToggle(): React.Node { // Explicitly disallow it for now. const isProfilingSubscription = useMemo( () => ({ - getCurrentValue: () => profilerStore.isProfiling, + getCurrentValue: () => profilerStore.isProfilingBasedOnUserInput, subscribe: (callback: Function) => { profilerStore.addListener('isProfiling', callback); return () => profilerStore.removeListener('isProfiling', callback); From 389a2deebc2dc41deb268f4b543709989d688d69 Mon Sep 17 00:00:00 2001 From: Ruslan Lesiutin Date: Wed, 9 Oct 2024 13:26:16 +0100 Subject: [PATCH 19/23] refactor[react-devtools/fiber/renderer]: optimize durations resolution (#31118) Stacked on https://github.com/facebook/react/pull/31117. No need for sending long float numbers and to have resolution less than a microsecond, we end up formatting it on a Frontend side: https://github.com/facebook/react/blob/6c7b41da3de12be2d95c60181b3fe896f824f13a/packages/react-devtools-shared/src/devtools/views/Profiler/utils.js#L359-L360 --- .../src/backend/fiber/renderer.js | 24 +++++++++++++++---- .../src/backend/utils/index.js | 9 +++++++ 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/packages/react-devtools-shared/src/backend/fiber/renderer.js b/packages/react-devtools-shared/src/backend/fiber/renderer.js index 9732bf105c..8ce815c31f 100644 --- a/packages/react-devtools-shared/src/backend/fiber/renderer.js +++ b/packages/react-devtools-shared/src/backend/fiber/renderer.js @@ -44,6 +44,7 @@ import { } from 'react-devtools-shared/src/utils'; import { formatConsoleArgumentsToSingleString, + formatDurationToMicrosecondsGranularity, gt, gte, parseSourceFromComponentStack, @@ -5074,8 +5075,14 @@ export function attach( const fiberSelfDurations: Array<[number, number]> = []; for (let i = 0; i < durations.length; i += 3) { const fiberID = durations[i]; - fiberActualDurations.push([fiberID, durations[i + 1]]); - fiberSelfDurations.push([fiberID, durations[i + 2]]); + fiberActualDurations.push([ + fiberID, + formatDurationToMicrosecondsGranularity(durations[i + 1]), + ]); + fiberSelfDurations.push([ + fiberID, + formatDurationToMicrosecondsGranularity(durations[i + 2]), + ]); } commitData.push({ @@ -5083,11 +5090,18 @@ export function attach( changeDescriptions !== null ? Array.from(changeDescriptions.entries()) : null, - duration: maxActualDuration, - effectDuration, + duration: + formatDurationToMicrosecondsGranularity(maxActualDuration), + effectDuration: + effectDuration !== null + ? formatDurationToMicrosecondsGranularity(effectDuration) + : null, fiberActualDurations, fiberSelfDurations, - passiveEffectDuration, + passiveEffectDuration: + passiveEffectDuration !== null + ? formatDurationToMicrosecondsGranularity(passiveEffectDuration) + : null, priorityLevel, timestamp: commitTime, updaters, diff --git a/packages/react-devtools-shared/src/backend/utils/index.js b/packages/react-devtools-shared/src/backend/utils/index.js index 1e7934af98..c07536e422 100644 --- a/packages/react-devtools-shared/src/backend/utils/index.js +++ b/packages/react-devtools-shared/src/backend/utils/index.js @@ -331,3 +331,12 @@ export function parseSourceFromComponentStack( return parseSourceFromFirefoxStack(componentStack); } + +// 0.123456789 => 0.123 +// Expects high-resolution timestamp in milliseconds, like from performance.now() +// Mainly used for optimizing the size of serialized profiling payload +export function formatDurationToMicrosecondsGranularity( + duration: number, +): number { + return Math.round(duration * 1000) / 1000; +} From 4a86ec5a66d0dd375f8433d380f71ade3e67d5d0 Mon Sep 17 00:00:00 2001 From: Ruslan Lesiutin Date: Wed, 9 Oct 2024 13:32:04 +0100 Subject: [PATCH 20/23] fix[react-devtools]: removed redundant startProfiling call (#31131) Stacked on https://github.com/facebook/react/pull/31118. See last commit. We don't need to call `startProfiling()` here, because we delegate this to the Renderer itself: https://github.com/facebook/react/blob/830e823cd2c6ee675636d31320b10350e8ade9ae/packages/react-devtools-shared/src/backend/fiber/renderer.js#L5227-L5232 Since this is de-facto the constructor of Renderer, this will be called earlier. Validated via testing the reload-to-profile for Chrome browser extension. --- packages/react-devtools-shared/src/backend/agent.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/packages/react-devtools-shared/src/backend/agent.js b/packages/react-devtools-shared/src/backend/agent.js index 88450fe29e..2a0e9feb37 100644 --- a/packages/react-devtools-shared/src/backend/agent.js +++ b/packages/react-devtools-shared/src/backend/agent.js @@ -714,10 +714,6 @@ export default class Agent extends EventEmitter<{ ) { this._rendererInterfaces[rendererID] = rendererInterface; - if (this._isProfiling) { - rendererInterface.startProfiling(this._recordChangeDescriptions); - } - rendererInterface.setTraceUpdatesEnabled(this._traceUpdatesEnabled); // When the renderer is attached, we need to tell it whether From 1d8d12005fc9d856c4c936b269adb4f52bf82e47 Mon Sep 17 00:00:00 2001 From: Ruslan Lesiutin Date: Wed, 9 Oct 2024 13:34:01 +0100 Subject: [PATCH 21/23] fix[react-devtools]: remove all listeners when Agent is shutdown (#31151) Based on https://github.com/facebook/react/pull/31049, credits to @EdmondChuiHW. What is happening here: 1. Once Agent is destroyed, unsubscribe own listeners and bridge listeners. 2. [Browser extension only] Once Agent is destroyed, unsubscribe listeners from BackendManager. 3. [Browser extension only] I've discovered that `backendManager.js` content script can get injected multiple times by the browser. When Frontend is initializing, it will create Store first, and then execute a content script for bootstraping backend manager. If Frontend was destroyed somewhere between these 2 steps, Backend won't be notified, because it is not initialized yet, so it will not unsubscribe listeners correctly. We might end up duplicating listeners, and the next time Frontend is launched, it will report an issues "Cannot add / remove node ...", because same operations are emitted twice. To reproduce 3 you can do the following: 1. Click reload-to-profile 2. Right after when both app and Chrome DevTools panel are reloaded, close Chrome DevTools. 3. Open Chrome DevTools again, open Profiler panel and observe "Cannot add / remove node ..." error in the UI. --- .../src/contentScripts/backendManager.js | 33 ++++++++++++++----- .../src/backend/agent.js | 3 ++ .../src/backend/fiber/renderer.js | 2 +- .../src/backend/index.js | 7 ++-- 4 files changed, 31 insertions(+), 14 deletions(-) diff --git a/packages/react-devtools-extensions/src/contentScripts/backendManager.js b/packages/react-devtools-extensions/src/contentScripts/backendManager.js index 9d3ec41433..63519b506d 100644 --- a/packages/react-devtools-extensions/src/contentScripts/backendManager.js +++ b/packages/react-devtools-extensions/src/contentScripts/backendManager.js @@ -16,6 +16,7 @@ import {COMPACT_VERSION_NAME} from 'react-devtools-extensions/src/utils'; import {getIsReloadAndProfileSupported} from 'react-devtools-shared/src/utils'; let welcomeHasInitialized = false; +const requiredBackends = new Set(); function welcome(event: $FlowFixMe) { if ( @@ -49,8 +50,6 @@ function welcome(event: $FlowFixMe) { setup(window.__REACT_DEVTOOLS_GLOBAL_HOOK__); } -window.addEventListener('message', welcome); - function setup(hook: ?DevToolsHook) { // this should not happen, but Chrome can be weird sometimes if (hook == null) { @@ -71,20 +70,27 @@ function setup(hook: ?DevToolsHook) { updateRequiredBackends(); // register renderers that inject themselves later. - hook.sub('renderer', ({renderer}) => { + const unsubscribeRendererListener = hook.sub('renderer', ({renderer}) => { registerRenderer(renderer, hook); updateRequiredBackends(); }); // listen for backend installations. - hook.sub('devtools-backend-installed', version => { - activateBackend(version, hook); - updateRequiredBackends(); + const unsubscribeBackendInstallationListener = hook.sub( + 'devtools-backend-installed', + version => { + activateBackend(version, hook); + updateRequiredBackends(); + }, + ); + + const unsubscribeShutdownListener: () => void = hook.sub('shutdown', () => { + unsubscribeRendererListener(); + unsubscribeBackendInstallationListener(); + unsubscribeShutdownListener(); }); } -const requiredBackends = new Set(); - function registerRenderer(renderer: ReactRenderer, hook: DevToolsHook) { let version = renderer.reconcilerVersion || renderer.version; if (!hasAssignedBackend(version)) { @@ -139,6 +145,7 @@ function activateBackend(version: string, hook: DevToolsHook) { // If we received 'shutdown' from `agent`, we assume the `bridge` is already shutting down, // and that caused the 'shutdown' event on the `agent`, so we don't need to call `bridge.shutdown()` here. hook.emit('shutdown'); + delete window.__REACT_DEVTOOLS_BACKEND_MANAGER_INJECTED__; }); initBackend(hook, agent, window, getIsReloadAndProfileSupported()); @@ -178,3 +185,13 @@ function updateRequiredBackends() { '*', ); } + +/* + * Make sure this is executed only once in case Frontend is reloaded multiple times while Backend is initializing + * We can't use `reactDevToolsAgent` field on a global Hook object, because it only cleaned up after both Frontend and Backend initialized + */ +if (!window.__REACT_DEVTOOLS_BACKEND_MANAGER_INJECTED__) { + window.__REACT_DEVTOOLS_BACKEND_MANAGER_INJECTED__ = true; + + window.addEventListener('message', welcome); +} diff --git a/packages/react-devtools-shared/src/backend/agent.js b/packages/react-devtools-shared/src/backend/agent.js index 2a0e9feb37..f5fde694e7 100644 --- a/packages/react-devtools-shared/src/backend/agent.js +++ b/packages/react-devtools-shared/src/backend/agent.js @@ -750,6 +750,9 @@ export default class Agent extends EventEmitter<{ shutdown: () => void = () => { // Clean up the overlay if visible, and associated events. this.emit('shutdown'); + + this._bridge.removeAllListeners(); + this.removeAllListeners(); }; startProfiling: (recordChangeDescriptions: boolean) => void = diff --git a/packages/react-devtools-shared/src/backend/fiber/renderer.js b/packages/react-devtools-shared/src/backend/fiber/renderer.js index 8ce815c31f..91fe467a95 100644 --- a/packages/react-devtools-shared/src/backend/fiber/renderer.js +++ b/packages/react-devtools-shared/src/backend/fiber/renderer.js @@ -3473,7 +3473,7 @@ export function attach( } function cleanup() { - // We don't patch any methods so there is no cleanup. + isProfiling = false; } function rootSupportsProfiling(root: any) { diff --git a/packages/react-devtools-shared/src/backend/index.js b/packages/react-devtools-shared/src/backend/index.js index 86714b7f61..ace5dbc8c0 100644 --- a/packages/react-devtools-shared/src/backend/index.js +++ b/packages/react-devtools-shared/src/backend/index.js @@ -80,15 +80,12 @@ export function initBackend( }); hook.reactDevtoolsAgent = null; }; - agent.addListener('shutdown', onAgentShutdown); - subs.push(() => { - agent.removeListener('shutdown', onAgentShutdown); - }); + // Agent's event listeners are cleaned up by Agent in `shutdown` implementation. + agent.addListener('shutdown', onAgentShutdown); agent.addListener('updateHookSettings', settings => { hook.settings = settings; }); - agent.addListener('getHookSettings', () => { if (hook.settings != null) { agent.onHookSettings(hook.settings); From bfe91fbecf183f85fc1c4f909e12a6833a247319 Mon Sep 17 00:00:00 2001 From: Ruslan Lesiutin Date: Wed, 9 Oct 2024 13:57:02 +0100 Subject: [PATCH 22/23] refactor[react-devtools]: flatten reload and profile config (#31132) Stacked on https://github.com/facebook/react/pull/31131. See last commit. This is a clean-up and a pre-requisite for next changes: 1. `ReloadAndProfileConfig` is now split into boolean value and settings object. This is mainly because I will add one more setting soon, and also because settings might be persisted for a longer time than the flag which signals if the Backend was reloaded for profiling. Ideally, this settings should probably be moved to the global Hook object, same as we did for console patching. 2. Host is now responsible for reseting the cached values, Backend will execute provided `onReloadAndProfileFlagsReset` callback. --- packages/react-devtools-core/src/backend.js | 41 ++++++++--- .../src/contentScripts/backendManager.js | 15 +++- .../src/contentScripts/installHook.js | 13 +++- packages/react-devtools-inline/src/backend.js | 14 +++- .../src/attachRenderer.js | 8 ++- .../src/backend/agent.js | 33 +++------ .../src/backend/fiber/renderer.js | 11 ++- .../src/backend/types.js | 12 +--- packages/react-devtools-shared/src/hook.js | 15 ++-- packages/react-devtools-shared/src/utils.js | 68 +++++++++---------- 10 files changed, 131 insertions(+), 99 deletions(-) diff --git a/packages/react-devtools-core/src/backend.js b/packages/react-devtools-core/src/backend.js index 54a6b9b48a..9305155ab0 100644 --- a/packages/react-devtools-core/src/backend.js +++ b/packages/react-devtools-core/src/backend.js @@ -26,8 +26,7 @@ import type { import type { DevToolsHook, DevToolsHookSettings, - ReloadAndProfileConfig, - ReloadAndProfileConfigPersistence, + ProfilingSettings, } from 'react-devtools-shared/src/backend/types'; import type {ResolveNativeStyle} from 'react-devtools-shared/src/backend/NativeStyleEditor/setupNativeStyleEditor'; @@ -42,7 +41,9 @@ type ConnectOptions = { websocket?: ?WebSocket, onSettingsUpdated?: (settings: $ReadOnly) => void, isReloadAndProfileSupported?: boolean, - reloadAndProfileConfigPersistence?: ReloadAndProfileConfigPersistence, + isProfiling?: boolean, + onReloadAndProfile?: (recordChangeDescriptions: boolean) => void, + onReloadAndProfileFlagsReset?: () => void, }; let savedComponentFilters: Array = @@ -63,9 +64,15 @@ export function initialize( maybeSettingsOrSettingsPromise?: | DevToolsHookSettings | Promise, - reloadAndProfileConfig?: ReloadAndProfileConfig, + shouldStartProfilingNow: boolean = false, + profilingSettings?: ProfilingSettings, ) { - installHook(window, maybeSettingsOrSettingsPromise, reloadAndProfileConfig); + installHook( + window, + maybeSettingsOrSettingsPromise, + shouldStartProfilingNow, + profilingSettings, + ); } export function connectToDevTools(options: ?ConnectOptions) { @@ -86,7 +93,9 @@ export function connectToDevTools(options: ?ConnectOptions) { isAppActive = () => true, onSettingsUpdated, isReloadAndProfileSupported = getIsReloadAndProfileSupported(), - reloadAndProfileConfigPersistence, + isProfiling, + onReloadAndProfile, + onReloadAndProfileFlagsReset, } = options || {}; const protocol = useHttps ? 'wss' : 'ws'; @@ -180,7 +189,11 @@ export function connectToDevTools(options: ?ConnectOptions) { // TODO (npm-packages) Warn if "isBackendStorageAPISupported" // $FlowFixMe[incompatible-call] found when upgrading Flow - const agent = new Agent(bridge, reloadAndProfileConfigPersistence); + const agent = new Agent(bridge, isProfiling, onReloadAndProfile); + if (typeof onReloadAndProfileFlagsReset === 'function') { + onReloadAndProfileFlagsReset(); + } + if (onSettingsUpdated != null) { agent.addListener('updateHookSettings', onSettingsUpdated); } @@ -320,7 +333,9 @@ type ConnectWithCustomMessagingOptions = { resolveRNStyle?: ResolveNativeStyle, onSettingsUpdated?: (settings: $ReadOnly) => void, isReloadAndProfileSupported?: boolean, - reloadAndProfileConfigPersistence?: ReloadAndProfileConfigPersistence, + isProfiling?: boolean, + onReloadAndProfile?: (recordChangeDescriptions: boolean) => void, + onReloadAndProfileFlagsReset?: () => void, }; export function connectWithCustomMessagingProtocol({ @@ -331,7 +346,9 @@ export function connectWithCustomMessagingProtocol({ resolveRNStyle, onSettingsUpdated, isReloadAndProfileSupported = getIsReloadAndProfileSupported(), - reloadAndProfileConfigPersistence, + isProfiling, + onReloadAndProfile, + onReloadAndProfileFlagsReset, }: ConnectWithCustomMessagingOptions): Function { const hook: ?DevToolsHook = window.__REACT_DEVTOOLS_GLOBAL_HOOK__; if (hook == null) { @@ -368,7 +385,11 @@ export function connectWithCustomMessagingProtocol({ bridge.send('overrideComponentFilters', savedComponentFilters); } - const agent = new Agent(bridge, reloadAndProfileConfigPersistence); + const agent = new Agent(bridge, isProfiling, onReloadAndProfile); + if (typeof onReloadAndProfileFlagsReset === 'function') { + onReloadAndProfileFlagsReset(); + } + if (onSettingsUpdated != null) { agent.addListener('updateHookSettings', onSettingsUpdated); } diff --git a/packages/react-devtools-extensions/src/contentScripts/backendManager.js b/packages/react-devtools-extensions/src/contentScripts/backendManager.js index 63519b506d..402a137857 100644 --- a/packages/react-devtools-extensions/src/contentScripts/backendManager.js +++ b/packages/react-devtools-extensions/src/contentScripts/backendManager.js @@ -14,6 +14,11 @@ import type { import {hasAssignedBackend} from 'react-devtools-shared/src/backend/utils'; import {COMPACT_VERSION_NAME} from 'react-devtools-extensions/src/utils'; import {getIsReloadAndProfileSupported} from 'react-devtools-shared/src/utils'; +import { + getIfReloadedAndProfiling, + onReloadAndProfile, + onReloadAndProfileFlagsReset, +} from 'react-devtools-shared/src/utils'; let welcomeHasInitialized = false; const requiredBackends = new Set(); @@ -140,7 +145,15 @@ function activateBackend(version: string, hook: DevToolsHook) { }, }); - const agent = new Agent(bridge); + const agent = new Agent( + bridge, + getIfReloadedAndProfiling(), + onReloadAndProfile, + ); + // Agent read flags successfully, we can count it as successful launch + // Clean up flags, so that next reload won't start profiling + onReloadAndProfileFlagsReset(); + agent.addListener('shutdown', () => { // If we received 'shutdown' from `agent`, we assume the `bridge` is already shutting down, // and that caused the 'shutdown' event on the `agent`, so we don't need to call `bridge.shutdown()` here. diff --git a/packages/react-devtools-extensions/src/contentScripts/installHook.js b/packages/react-devtools-extensions/src/contentScripts/installHook.js index b7b96ed247..e70e97b285 100644 --- a/packages/react-devtools-extensions/src/contentScripts/installHook.js +++ b/packages/react-devtools-extensions/src/contentScripts/installHook.js @@ -1,4 +1,8 @@ import {installHook} from 'react-devtools-shared/src/hook'; +import { + getIfReloadedAndProfiling, + getProfilingSettings, +} from 'react-devtools-shared/src/utils'; let resolveHookSettingsInjection; @@ -34,8 +38,15 @@ if (!window.hasOwnProperty('__REACT_DEVTOOLS_GLOBAL_HOOK__')) { payload: {handshake: true}, }); + const shouldStartProfiling = getIfReloadedAndProfiling(); + const profilingSettings = getProfilingSettings(); // Can't delay hook installation, inject settings lazily - installHook(window, hookSettingsPromise); + installHook( + window, + hookSettingsPromise, + shouldStartProfiling, + profilingSettings, + ); // Detect React window.__REACT_DEVTOOLS_GLOBAL_HOOK__.on( diff --git a/packages/react-devtools-inline/src/backend.js b/packages/react-devtools-inline/src/backend.js index 41af7809be..354970446d 100644 --- a/packages/react-devtools-inline/src/backend.js +++ b/packages/react-devtools-inline/src/backend.js @@ -8,7 +8,12 @@ import setupNativeStyleEditor from 'react-devtools-shared/src/backend/NativeStyl import type {BackendBridge} from 'react-devtools-shared/src/bridge'; import type {Wall} from 'react-devtools-shared/src/frontend/types'; -import {getIsReloadAndProfileSupported} from 'react-devtools-shared/src/utils'; +import { + getIfReloadedAndProfiling, + getIsReloadAndProfileSupported, + onReloadAndProfile, + onReloadAndProfileFlagsReset, +} from 'react-devtools-shared/src/utils'; function startActivation(contentWindow: any, bridge: BackendBridge) { const onSavedPreferences = (data: $FlowFixMe) => { @@ -63,7 +68,12 @@ function startActivation(contentWindow: any, bridge: BackendBridge) { } function finishActivation(contentWindow: any, bridge: BackendBridge) { - const agent = new Agent(bridge); + const agent = new Agent( + bridge, + getIfReloadedAndProfiling(), + onReloadAndProfile, + ); + onReloadAndProfileFlagsReset(); const hook = contentWindow.__REACT_DEVTOOLS_GLOBAL_HOOK__; if (hook) { diff --git a/packages/react-devtools-shared/src/attachRenderer.js b/packages/react-devtools-shared/src/attachRenderer.js index cd7a348b65..fedf76293c 100644 --- a/packages/react-devtools-shared/src/attachRenderer.js +++ b/packages/react-devtools-shared/src/attachRenderer.js @@ -12,8 +12,8 @@ import type { RendererInterface, DevToolsHook, RendererID, + ProfilingSettings, } from 'react-devtools-shared/src/backend/types'; -import type {ReloadAndProfileConfig} from './backend/types'; import {attach as attachFlight} from 'react-devtools-shared/src/backend/flight/renderer'; import {attach as attachFiber} from 'react-devtools-shared/src/backend/fiber/renderer'; @@ -30,7 +30,8 @@ export default function attachRenderer( id: RendererID, renderer: ReactRenderer, global: Object, - reloadAndProfileConfig: ReloadAndProfileConfig, + shouldStartProfilingNow: boolean, + profilingSettings: ProfilingSettings, ): RendererInterface | void { // only attach if the renderer is compatible with the current version of the backend if (!isMatchingRender(renderer.reconcilerVersion || renderer.version)) { @@ -55,7 +56,8 @@ export default function attachRenderer( id, renderer, global, - reloadAndProfileConfig, + shouldStartProfilingNow, + profilingSettings, ); } else if (renderer.ComponentTree) { // react-dom v15 diff --git a/packages/react-devtools-shared/src/backend/agent.js b/packages/react-devtools-shared/src/backend/agent.js index f5fde694e7..12704899ec 100644 --- a/packages/react-devtools-shared/src/backend/agent.js +++ b/packages/react-devtools-shared/src/backend/agent.js @@ -26,11 +26,9 @@ import type { RendererID, RendererInterface, DevToolsHookSettings, - ReloadAndProfileConfigPersistence, } from './types'; import type {ComponentFilter} from 'react-devtools-shared/src/frontend/types'; import {isReactNativeEnvironment} from './utils'; -import {defaultReloadAndProfileConfigPersistence} from '../utils'; import { sessionStorageGetItem, sessionStorageRemoveItem, @@ -151,33 +149,21 @@ export default class Agent extends EventEmitter<{ }> { _bridge: BackendBridge; _isProfiling: boolean = false; - _recordChangeDescriptions: boolean = false; _rendererInterfaces: {[key: RendererID]: RendererInterface, ...} = {}; _persistedSelection: PersistedSelection | null = null; _persistedSelectionMatch: PathMatch | null = null; _traceUpdatesEnabled: boolean = false; - _reloadAndProfileConfigPersistence: ReloadAndProfileConfigPersistence; + _onReloadAndProfile: ((recordChangeDescriptions: boolean) => void) | void; constructor( bridge: BackendBridge, - reloadAndProfileConfigPersistence?: ReloadAndProfileConfigPersistence = defaultReloadAndProfileConfigPersistence, + isProfiling: boolean = false, + onReloadAndProfile?: (recordChangeDescriptions: boolean) => void, ) { super(); - this._reloadAndProfileConfigPersistence = reloadAndProfileConfigPersistence; - const {getReloadAndProfileConfig, setReloadAndProfileConfig} = - reloadAndProfileConfigPersistence; - const reloadAndProfileConfig = getReloadAndProfileConfig(); - if (reloadAndProfileConfig.shouldReloadAndProfile) { - this._recordChangeDescriptions = - reloadAndProfileConfig.recordChangeDescriptions; - this._isProfiling = true; - - setReloadAndProfileConfig({ - shouldReloadAndProfile: false, - recordChangeDescriptions: false, - }); - } + this._isProfiling = isProfiling; + this._onReloadAndProfile = onReloadAndProfile; const persistedSelectionString = sessionStorageGetItem( SESSION_STORAGE_LAST_SELECTION_KEY, @@ -674,10 +660,9 @@ export default class Agent extends EventEmitter<{ reloadAndProfile: (recordChangeDescriptions: boolean) => void = recordChangeDescriptions => { - this._reloadAndProfileConfigPersistence.setReloadAndProfileConfig({ - shouldReloadAndProfile: true, - recordChangeDescriptions, - }); + if (typeof this._onReloadAndProfile === 'function') { + this._onReloadAndProfile(recordChangeDescriptions); + } // This code path should only be hit if the shell has explicitly told the Store that it supports profiling. // In that case, the shell must also listen for this specific message to know when it needs to reload the app. @@ -757,7 +742,6 @@ export default class Agent extends EventEmitter<{ startProfiling: (recordChangeDescriptions: boolean) => void = recordChangeDescriptions => { - this._recordChangeDescriptions = recordChangeDescriptions; this._isProfiling = true; for (const rendererID in this._rendererInterfaces) { const renderer = ((this._rendererInterfaces[ @@ -770,7 +754,6 @@ export default class Agent extends EventEmitter<{ stopProfiling: () => void = () => { this._isProfiling = false; - this._recordChangeDescriptions = false; for (const rendererID in this._rendererInterfaces) { const renderer = ((this._rendererInterfaces[ (rendererID: any) diff --git a/packages/react-devtools-shared/src/backend/fiber/renderer.js b/packages/react-devtools-shared/src/backend/fiber/renderer.js index 91fe467a95..57aca225b8 100644 --- a/packages/react-devtools-shared/src/backend/fiber/renderer.js +++ b/packages/react-devtools-shared/src/backend/fiber/renderer.js @@ -104,7 +104,6 @@ import { supportsOwnerStacks, supportsConsoleTasks, } from './DevToolsFiberComponentStack'; -import type {ReloadAndProfileConfig} from '../types'; // $FlowFixMe[method-unbinding] const toString = Object.prototype.toString; @@ -136,6 +135,7 @@ import type { WorkTagMap, CurrentDispatcherRef, LegacyDispatcherRef, + ProfilingSettings, } from '../types'; import type { ComponentFilter, @@ -864,7 +864,8 @@ export function attach( rendererID: number, renderer: ReactRenderer, global: Object, - reloadAndProfileConfig: ReloadAndProfileConfig, + shouldStartProfilingNow: boolean, + profilingSettings: ProfilingSettings, ): RendererInterface { // Newer versions of the reconciler package also specific reconciler version. // If that version number is present, use it. @@ -5225,10 +5226,8 @@ export function attach( } // Automatically start profiling so that we don't miss timing info from initial "mount". - if (reloadAndProfileConfig.shouldReloadAndProfile) { - const shouldRecordChangeDescriptions = - reloadAndProfileConfig.recordChangeDescriptions; - startProfiling(shouldRecordChangeDescriptions); + if (shouldStartProfilingNow) { + startProfiling(profilingSettings.recordChangeDescriptions); } function getNearestFiber(devtoolsInstance: DevToolsInstance): null | Fiber { diff --git a/packages/react-devtools-shared/src/backend/types.js b/packages/react-devtools-shared/src/backend/types.js index c6f743546e..61546f2c28 100644 --- a/packages/react-devtools-shared/src/backend/types.js +++ b/packages/react-devtools-shared/src/backend/types.js @@ -485,20 +485,10 @@ export type DevToolsBackend = { setupNativeStyleEditor?: SetupNativeStyleEditor, }; -export type ReloadAndProfileConfig = { - shouldReloadAndProfile: boolean, +export type ProfilingSettings = { recordChangeDescriptions: boolean, }; -// Linter doesn't speak Flow's `Partial` type -// eslint-disable-next-line no-undef -type PartialReloadAndProfileConfig = Partial; - -export type ReloadAndProfileConfigPersistence = { - setReloadAndProfileConfig: (config: PartialReloadAndProfileConfig) => void, - getReloadAndProfileConfig: () => ReloadAndProfileConfig, -}; - export type DevToolsHook = { listeners: {[key: string]: Array, ...}, rendererInterfaces: Map, diff --git a/packages/react-devtools-shared/src/hook.js b/packages/react-devtools-shared/src/hook.js index 3b45c7417d..4aa6518cd1 100644 --- a/packages/react-devtools-shared/src/hook.js +++ b/packages/react-devtools-shared/src/hook.js @@ -16,7 +16,7 @@ import type { RendererInterface, DevToolsBackend, DevToolsHookSettings, - ReloadAndProfileConfig, + ProfilingSettings, } from './backend/types'; import { @@ -27,7 +27,6 @@ import { import attachRenderer from './attachRenderer'; import formatConsoleArguments from 'react-devtools-shared/src/backend/utils/formatConsoleArguments'; import formatWithStyles from 'react-devtools-shared/src/backend/utils/formatWithStyles'; -import {defaultReloadAndProfileConfigPersistence} from './utils'; // React's custom built component stack strings match "\s{4}in" // Chrome's prefix matches "\s{4}at" @@ -51,12 +50,17 @@ function areStackTracesEqual(a: string, b: string): boolean { const targetConsole: Object = console; +const defaultProfilingSettings: ProfilingSettings = { + recordChangeDescriptions: false, +}; + export function installHook( target: any, maybeSettingsOrSettingsPromise?: | DevToolsHookSettings | Promise, - reloadAndProfileConfig?: ReloadAndProfileConfig = defaultReloadAndProfileConfigPersistence.getReloadAndProfileConfig(), + shouldStartProfilingNow: boolean = false, + profilingSettings: ProfilingSettings = defaultProfilingSettings, ): DevToolsHook | null { if (target.hasOwnProperty('__REACT_DEVTOOLS_GLOBAL_HOOK__')) { return null; @@ -195,6 +199,8 @@ export function installHook( } catch (err) {} } + // TODO: isProfiling should be stateful, and we should update it once profiling is finished + const isProfiling = shouldStartProfilingNow; let uidCounter = 0; function inject(renderer: ReactRenderer): number { const id = ++uidCounter; @@ -215,7 +221,8 @@ export function installHook( id, renderer, target, - reloadAndProfileConfig, + isProfiling, + profilingSettings, ); if (rendererInterface != null) { hook.rendererInterfaces.set(id, rendererInterface); diff --git a/packages/react-devtools-shared/src/utils.js b/packages/react-devtools-shared/src/utils.js index 715834334f..d3f18920fc 100644 --- a/packages/react-devtools-shared/src/utils.js +++ b/packages/react-devtools-shared/src/utils.js @@ -56,8 +56,9 @@ import { localStorageGetItem, localStorageSetItem, sessionStorageGetItem, + sessionStorageRemoveItem, sessionStorageSetItem, -} from './storage'; +} from 'react-devtools-shared/src/storage'; import {meta} from './hydration'; import isArray from './isArray'; @@ -67,12 +68,11 @@ import type { SerializedElement as SerializedElementFrontend, LRUCache, } from 'react-devtools-shared/src/frontend/types'; -import type {SerializedElement as SerializedElementBackend} from 'react-devtools-shared/src/backend/types'; -import {isSynchronousXHRSupported} from './backend/utils'; import type { - ReloadAndProfileConfig, - ReloadAndProfileConfigPersistence, -} from './backend/types'; + ProfilingSettings, + SerializedElement as SerializedElementBackend, +} from 'react-devtools-shared/src/backend/types'; +import {isSynchronousXHRSupported} from './backend/utils'; // $FlowFixMe[method-unbinding] const hasOwnProperty = Object.prototype.hasOwnProperty; @@ -990,34 +990,30 @@ export function getIsReloadAndProfileSupported(): boolean { return isBackendStorageAPISupported && isSynchronousXHRSupported(); } -export const defaultReloadAndProfileConfigPersistence: ReloadAndProfileConfigPersistence = - { - setReloadAndProfileConfig({ - shouldReloadAndProfile, - recordChangeDescriptions, - }): void { - if (shouldReloadAndProfile != null) { - sessionStorageSetItem( - SESSION_STORAGE_RELOAD_AND_PROFILE_KEY, - shouldReloadAndProfile ? 'true' : 'false', - ); - } - if (recordChangeDescriptions != null) { - sessionStorageSetItem( - SESSION_STORAGE_RECORD_CHANGE_DESCRIPTIONS_KEY, - recordChangeDescriptions ? 'true' : 'false', - ); - } - }, - getReloadAndProfileConfig(): ReloadAndProfileConfig { - return { - shouldReloadAndProfile: - sessionStorageGetItem(SESSION_STORAGE_RELOAD_AND_PROFILE_KEY) === - 'true', - recordChangeDescriptions: - sessionStorageGetItem( - SESSION_STORAGE_RECORD_CHANGE_DESCRIPTIONS_KEY, - ) === 'true', - }; - }, +// Expected to be used only by browser extension and react-devtools-inline +export function getIfReloadedAndProfiling(): boolean { + return ( + sessionStorageGetItem(SESSION_STORAGE_RELOAD_AND_PROFILE_KEY) === 'true' + ); +} + +export function getProfilingSettings(): ProfilingSettings { + return { + recordChangeDescriptions: + sessionStorageGetItem(SESSION_STORAGE_RECORD_CHANGE_DESCRIPTIONS_KEY) === + 'true', }; +} + +export function onReloadAndProfile(recordChangeDescriptions: boolean): void { + sessionStorageSetItem(SESSION_STORAGE_RELOAD_AND_PROFILE_KEY, 'true'); + sessionStorageSetItem( + SESSION_STORAGE_RECORD_CHANGE_DESCRIPTIONS_KEY, + recordChangeDescriptions ? 'true' : 'false', + ); +} + +export function onReloadAndProfileFlagsReset(): void { + sessionStorageRemoveItem(SESSION_STORAGE_RELOAD_AND_PROFILE_KEY); + sessionStorageRemoveItem(SESSION_STORAGE_RECORD_CHANGE_DESCRIPTIONS_KEY); +} From d5bba18b5d81f234657586865248c5b6849599cd Mon Sep 17 00:00:00 2001 From: Ruslan Lesiutin Date: Wed, 9 Oct 2024 15:27:04 +0100 Subject: [PATCH 23/23] fix[react-devtools]: record timeline data only when supported (#31154) Stacked on https://github.com/facebook/react/pull/31132. See last commit. There are 2 issues: 1. We've been recording timeline events, even if Timeline Profiler was not supported by the Host. We've been doing this for React Native, for example, which would significantly regress perf of recording a profiling session, but we were not even using this data. 2. Currently, we are generating component stack for every state update event. This is extremely expensive, and we should not be doing this. We can't currently fix the second one, because we would still need to generate all these stacks, and this would still take quite a lot of time. As of right now, we can't generate a component stack lazily without relying on the fact that reference to the Fiber is not stale. With `enableOwnerStacks` we could populate component stacks in some collection, which would be cached at the Backend, and then returned only once Frontend asks for it. This approach also eliminates the need for keeping a reference to a Fiber. --- .../src/backend/agent.js | 55 ++++++++------- .../src/backend/fiber/renderer.js | 18 +++-- .../src/backend/profilingHooks.js | 67 ++++++++++++------- .../src/backend/types.js | 6 +- packages/react-devtools-shared/src/bridge.js | 8 ++- .../react-devtools-shared/src/constants.js | 2 + .../src/devtools/ProfilerStore.js | 5 +- .../views/Profiler/ReloadAndProfileButton.js | 7 +- packages/react-devtools-shared/src/hook.js | 1 + packages/react-devtools-shared/src/utils.js | 13 +++- 10 files changed, 123 insertions(+), 59 deletions(-) diff --git a/packages/react-devtools-shared/src/backend/agent.js b/packages/react-devtools-shared/src/backend/agent.js index 12704899ec..e05abf51ab 100644 --- a/packages/react-devtools-shared/src/backend/agent.js +++ b/packages/react-devtools-shared/src/backend/agent.js @@ -153,12 +153,17 @@ export default class Agent extends EventEmitter<{ _persistedSelection: PersistedSelection | null = null; _persistedSelectionMatch: PathMatch | null = null; _traceUpdatesEnabled: boolean = false; - _onReloadAndProfile: ((recordChangeDescriptions: boolean) => void) | void; + _onReloadAndProfile: + | ((recordChangeDescriptions: boolean, recordTimeline: boolean) => void) + | void; constructor( bridge: BackendBridge, isProfiling: boolean = false, - onReloadAndProfile?: (recordChangeDescriptions: boolean) => void, + onReloadAndProfile?: ( + recordChangeDescriptions: boolean, + recordTimeline: boolean, + ) => void, ) { super(); @@ -658,17 +663,19 @@ export default class Agent extends EventEmitter<{ this._bridge.send('isReloadAndProfileSupportedByBackend', true); }; - reloadAndProfile: (recordChangeDescriptions: boolean) => void = - recordChangeDescriptions => { - if (typeof this._onReloadAndProfile === 'function') { - this._onReloadAndProfile(recordChangeDescriptions); - } + reloadAndProfile: ({ + recordChangeDescriptions: boolean, + recordTimeline: boolean, + }) => void = ({recordChangeDescriptions, recordTimeline}) => { + if (typeof this._onReloadAndProfile === 'function') { + this._onReloadAndProfile(recordChangeDescriptions, recordTimeline); + } - // This code path should only be hit if the shell has explicitly told the Store that it supports profiling. - // In that case, the shell must also listen for this specific message to know when it needs to reload the app. - // The agent can't do this in a way that is renderer agnostic. - this._bridge.send('reloadAppForProfiling'); - }; + // This code path should only be hit if the shell has explicitly told the Store that it supports profiling. + // In that case, the shell must also listen for this specific message to know when it needs to reload the app. + // The agent can't do this in a way that is renderer agnostic. + this._bridge.send('reloadAppForProfiling'); + }; renamePath: RenamePathParams => void = ({ hookID, @@ -740,17 +747,19 @@ export default class Agent extends EventEmitter<{ this.removeAllListeners(); }; - startProfiling: (recordChangeDescriptions: boolean) => void = - recordChangeDescriptions => { - this._isProfiling = true; - for (const rendererID in this._rendererInterfaces) { - const renderer = ((this._rendererInterfaces[ - (rendererID: any) - ]: any): RendererInterface); - renderer.startProfiling(recordChangeDescriptions); - } - this._bridge.send('profilingStatus', this._isProfiling); - }; + startProfiling: ({ + recordChangeDescriptions: boolean, + recordTimeline: boolean, + }) => void = ({recordChangeDescriptions, recordTimeline}) => { + this._isProfiling = true; + for (const rendererID in this._rendererInterfaces) { + const renderer = ((this._rendererInterfaces[ + (rendererID: any) + ]: any): RendererInterface); + renderer.startProfiling(recordChangeDescriptions, recordTimeline); + } + this._bridge.send('profilingStatus', this._isProfiling); + }; stopProfiling: () => void = () => { this._isProfiling = false; diff --git a/packages/react-devtools-shared/src/backend/fiber/renderer.js b/packages/react-devtools-shared/src/backend/fiber/renderer.js index 57aca225b8..d582e2a3ce 100644 --- a/packages/react-devtools-shared/src/backend/fiber/renderer.js +++ b/packages/react-devtools-shared/src/backend/fiber/renderer.js @@ -5035,6 +5035,7 @@ export function attach( let isProfiling: boolean = false; let profilingStartTime: number = 0; let recordChangeDescriptions: boolean = false; + let recordTimeline: boolean = false; let rootToCommitProfilingMetadataMap: CommitProfilingMetadataMap | null = null; @@ -5176,12 +5177,16 @@ export function attach( } } - function startProfiling(shouldRecordChangeDescriptions: boolean) { + function startProfiling( + shouldRecordChangeDescriptions: boolean, + shouldRecordTimeline: boolean, + ) { if (isProfiling) { return; } recordChangeDescriptions = shouldRecordChangeDescriptions; + recordTimeline = shouldRecordTimeline; // Capture initial values as of the time profiling starts. // It's important we snapshot both the durations and the id-to-root map, @@ -5212,7 +5217,7 @@ export function attach( rootToCommitProfilingMetadataMap = new Map(); if (toggleProfilingStatus !== null) { - toggleProfilingStatus(true); + toggleProfilingStatus(true, recordTimeline); } } @@ -5221,13 +5226,18 @@ export function attach( recordChangeDescriptions = false; if (toggleProfilingStatus !== null) { - toggleProfilingStatus(false); + toggleProfilingStatus(false, recordTimeline); } + + recordTimeline = false; } // Automatically start profiling so that we don't miss timing info from initial "mount". if (shouldStartProfilingNow) { - startProfiling(profilingSettings.recordChangeDescriptions); + startProfiling( + profilingSettings.recordChangeDescriptions, + profilingSettings.recordTimeline, + ); } function getNearestFiber(devtoolsInstance: DevToolsInstance): null | Fiber { diff --git a/packages/react-devtools-shared/src/backend/profilingHooks.js b/packages/react-devtools-shared/src/backend/profilingHooks.js index 47a0103530..a8111713c9 100644 --- a/packages/react-devtools-shared/src/backend/profilingHooks.js +++ b/packages/react-devtools-shared/src/backend/profilingHooks.js @@ -97,7 +97,10 @@ export function setPerformanceMock_ONLY_FOR_TESTING( } export type GetTimelineData = () => TimelineData | null; -export type ToggleProfilingStatus = (value: boolean) => void; +export type ToggleProfilingStatus = ( + value: boolean, + recordTimeline?: boolean, +) => void; type Response = { getTimelineData: GetTimelineData, @@ -839,7 +842,10 @@ export function createProfilingHooks({ } } - function toggleProfilingStatus(value: boolean) { + function toggleProfilingStatus( + value: boolean, + recordTimeline: boolean = false, + ) { if (isProfiling !== value) { isProfiling = value; @@ -875,34 +881,45 @@ export function createProfilingHooks({ currentReactComponentMeasure = null; currentReactMeasuresStack = []; currentFiberStacks = new Map(); - currentTimelineData = { - // Session wide metadata; only collected once. - internalModuleSourceToRanges, - laneToLabelMap: laneToLabelMap || new Map(), - reactVersion, + if (recordTimeline) { + currentTimelineData = { + // Session wide metadata; only collected once. + internalModuleSourceToRanges, + laneToLabelMap: laneToLabelMap || new Map(), + reactVersion, - // Data logged by React during profiling session. - componentMeasures: [], - schedulingEvents: [], - suspenseEvents: [], - thrownErrors: [], + // Data logged by React during profiling session. + componentMeasures: [], + schedulingEvents: [], + suspenseEvents: [], + thrownErrors: [], - // Data inferred based on what React logs. - batchUIDToMeasuresMap: new Map(), - duration: 0, - laneToReactMeasureMap, - startTime: 0, + // Data inferred based on what React logs. + batchUIDToMeasuresMap: new Map(), + duration: 0, + laneToReactMeasureMap, + startTime: 0, - // Data only available in Chrome profiles. - flamechart: [], - nativeEvents: [], - networkMeasures: [], - otherUserTimingMarks: [], - snapshots: [], - snapshotHeight: 0, - }; + // Data only available in Chrome profiles. + flamechart: [], + nativeEvents: [], + networkMeasures: [], + otherUserTimingMarks: [], + snapshots: [], + snapshotHeight: 0, + }; + } nextRenderShouldStartNewBatch = true; } else { + // This is __EXPENSIVE__. + // We could end up with hundreds of state updated, and for each one of them + // would try to create a component stack with possibly hundreds of Fibers. + // Creating a cache of component stacks won't help, generating a single stack is already expensive enough. + // We should find a way to lazily generate component stacks on demand, when user inspects a specific event. + // If we succeed with moving React DevTools Timeline Profiler to Performance panel, then Timeline Profiler would probably be removed. + // If not, then once enableOwnerStacks is adopted, revisit this again and cache component stacks per Fiber, + // but only return them when needed, sending hundreds of component stacks is beyond the Bridge's bandwidth. + // Postprocess Profile data if (currentTimelineData !== null) { currentTimelineData.schedulingEvents.forEach(event => { diff --git a/packages/react-devtools-shared/src/backend/types.js b/packages/react-devtools-shared/src/backend/types.js index 61546f2c28..9d9d9a8eb5 100644 --- a/packages/react-devtools-shared/src/backend/types.js +++ b/packages/react-devtools-shared/src/backend/types.js @@ -419,7 +419,10 @@ export type RendererInterface = { renderer: ReactRenderer | null, setTraceUpdatesEnabled: (enabled: boolean) => void, setTrackedPath: (path: Array | null) => void, - startProfiling: (recordChangeDescriptions: boolean) => void, + startProfiling: ( + recordChangeDescriptions: boolean, + recordTimeline: boolean, + ) => void, stopProfiling: () => void, storeAsGlobal: ( id: number, @@ -487,6 +490,7 @@ export type DevToolsBackend = { export type ProfilingSettings = { recordChangeDescriptions: boolean, + recordTimeline: boolean, }; export type DevToolsHook = { diff --git a/packages/react-devtools-shared/src/bridge.js b/packages/react-devtools-shared/src/bridge.js index dde6e7c3ff..cb494e1b3c 100644 --- a/packages/react-devtools-shared/src/bridge.js +++ b/packages/react-devtools-shared/src/bridge.js @@ -16,6 +16,7 @@ import type { ProfilingDataBackend, RendererID, DevToolsHookSettings, + ProfilingSettings, } from 'react-devtools-shared/src/backend/types'; import type {StyleAndLayout as StyleAndLayoutPayload} from 'react-devtools-shared/src/backend/NativeStyleEditor/types'; @@ -206,6 +207,9 @@ export type BackendEvents = { hookSettings: [$ReadOnly], }; +type StartProfilingParams = ProfilingSettings; +type ReloadAndProfilingParams = ProfilingSettings; + type FrontendEvents = { clearErrorsAndWarnings: [{rendererID: RendererID}], clearErrorsForElementID: [ElementAndRendererID], @@ -226,13 +230,13 @@ type FrontendEvents = { overrideSuspense: [OverrideSuspense], overrideValueAtPath: [OverrideValueAtPath], profilingData: [ProfilingDataBackend], - reloadAndProfile: [boolean], + reloadAndProfile: [ReloadAndProfilingParams], renamePath: [RenamePath], savedPreferences: [SavedPreferencesParams], setTraceUpdatesEnabled: [boolean], shutdown: [], startInspectingHost: [], - startProfiling: [boolean], + startProfiling: [StartProfilingParams], stopInspectingHost: [boolean], stopProfiling: [], storeAsGlobal: [StoreAsGlobalParams], diff --git a/packages/react-devtools-shared/src/constants.js b/packages/react-devtools-shared/src/constants.js index 6893610b46..b087381659 100644 --- a/packages/react-devtools-shared/src/constants.js +++ b/packages/react-devtools-shared/src/constants.js @@ -41,6 +41,8 @@ export const LOCAL_STORAGE_PARSE_HOOK_NAMES_KEY = 'React::DevTools::parseHookNames'; export const SESSION_STORAGE_RECORD_CHANGE_DESCRIPTIONS_KEY = 'React::DevTools::recordChangeDescriptions'; +export const SESSION_STORAGE_RECORD_TIMELINE_KEY = + 'React::DevTools::recordTimeline'; export const SESSION_STORAGE_RELOAD_AND_PROFILE_KEY = 'React::DevTools::reloadAndProfile'; export const LOCAL_STORAGE_BROWSER_THEME = 'React::DevTools::theme'; diff --git a/packages/react-devtools-shared/src/devtools/ProfilerStore.js b/packages/react-devtools-shared/src/devtools/ProfilerStore.js index 5ed7e69f29..b9bbdb11df 100644 --- a/packages/react-devtools-shared/src/devtools/ProfilerStore.js +++ b/packages/react-devtools-shared/src/devtools/ProfilerStore.js @@ -191,7 +191,10 @@ export default class ProfilerStore extends EventEmitter<{ } startProfiling(): void { - this._bridge.send('startProfiling', this._store.recordChangeDescriptions); + this._bridge.send('startProfiling', { + recordChangeDescriptions: this._store.recordChangeDescriptions, + recordTimeline: this._store.supportsTimeline, + }); this._isProfilingBasedOnUserInput = true; this.emit('isProfiling'); diff --git a/packages/react-devtools-shared/src/devtools/views/Profiler/ReloadAndProfileButton.js b/packages/react-devtools-shared/src/devtools/views/Profiler/ReloadAndProfileButton.js index 90dba6f7d8..95ac82a973 100644 --- a/packages/react-devtools-shared/src/devtools/views/Profiler/ReloadAndProfileButton.js +++ b/packages/react-devtools-shared/src/devtools/views/Profiler/ReloadAndProfileButton.js @@ -54,8 +54,11 @@ export default function ReloadAndProfileButton({ // For now, let's just skip doing it entirely to avoid paying snapshot costs for data we don't need. // startProfiling(); - bridge.send('reloadAndProfile', recordChangeDescriptions); - }, [bridge, recordChangeDescriptions]); + bridge.send('reloadAndProfile', { + recordChangeDescriptions, + recordTimeline: store.supportsTimeline, + }); + }, [bridge, recordChangeDescriptions, store]); if (!supportsReloadAndProfile) { return null; diff --git a/packages/react-devtools-shared/src/hook.js b/packages/react-devtools-shared/src/hook.js index 4aa6518cd1..d754140f96 100644 --- a/packages/react-devtools-shared/src/hook.js +++ b/packages/react-devtools-shared/src/hook.js @@ -52,6 +52,7 @@ const targetConsole: Object = console; const defaultProfilingSettings: ProfilingSettings = { recordChangeDescriptions: false, + recordTimeline: false, }; export function installHook( diff --git a/packages/react-devtools-shared/src/utils.js b/packages/react-devtools-shared/src/utils.js index d3f18920fc..bbf1599898 100644 --- a/packages/react-devtools-shared/src/utils.js +++ b/packages/react-devtools-shared/src/utils.js @@ -38,6 +38,7 @@ import { LOCAL_STORAGE_OPEN_IN_EDITOR_URL, SESSION_STORAGE_RELOAD_AND_PROFILE_KEY, SESSION_STORAGE_RECORD_CHANGE_DESCRIPTIONS_KEY, + SESSION_STORAGE_RECORD_TIMELINE_KEY, } from './constants'; import { ComponentFilterElementType, @@ -1002,18 +1003,28 @@ export function getProfilingSettings(): ProfilingSettings { recordChangeDescriptions: sessionStorageGetItem(SESSION_STORAGE_RECORD_CHANGE_DESCRIPTIONS_KEY) === 'true', + recordTimeline: + sessionStorageGetItem(SESSION_STORAGE_RECORD_TIMELINE_KEY) === 'true', }; } -export function onReloadAndProfile(recordChangeDescriptions: boolean): void { +export function onReloadAndProfile( + recordChangeDescriptions: boolean, + recordTimeline: boolean, +): void { sessionStorageSetItem(SESSION_STORAGE_RELOAD_AND_PROFILE_KEY, 'true'); sessionStorageSetItem( SESSION_STORAGE_RECORD_CHANGE_DESCRIPTIONS_KEY, recordChangeDescriptions ? 'true' : 'false', ); + sessionStorageSetItem( + SESSION_STORAGE_RECORD_TIMELINE_KEY, + recordTimeline ? 'true' : 'false', + ); } export function onReloadAndProfileFlagsReset(): void { sessionStorageRemoveItem(SESSION_STORAGE_RELOAD_AND_PROFILE_KEY); sessionStorageRemoveItem(SESSION_STORAGE_RECORD_CHANGE_DESCRIPTIONS_KEY); + sessionStorageRemoveItem(SESSION_STORAGE_RECORD_TIMELINE_KEY); }