diff --git a/compiler/apps/playground/babel.config.js b/compiler/apps/playground/babel.config.js index 0edf2e9b72..f65e0a2a79 100644 --- a/compiler/apps/playground/babel.config.js +++ b/compiler/apps/playground/babel.config.js @@ -13,7 +13,7 @@ module.exports = function (api) { [ 'babel-plugin-react-compiler', { - runtimeModule: 'react-compiler-runtime', + target: '18', }, ], ], diff --git a/compiler/package.json b/compiler/package.json index 12c6de56dc..b478c9f67a 100644 --- a/compiler/package.json +++ b/compiler/package.json @@ -19,7 +19,6 @@ "test": "yarn workspaces run test", "snap": "yarn workspace babel-plugin-react-compiler run snap", "snap:build": "yarn workspace snap run build", - "postinstall": "perl -p -i -e 's/react\\.element/react.transitional.element/' node_modules/fbt/lib/FbtReactUtil.js && perl -p -i -e 's/didWarnAboutUsingAct = false;/didWarnAboutUsingAct = true;/' node_modules/react-dom/cjs/react-dom-test-utils.development.js", "npm:publish": "node scripts/release/publish" }, "dependencies": { diff --git a/compiler/packages/babel-plugin-react-compiler/package.json b/compiler/packages/babel-plugin-react-compiler/package.json index fa8d5c06cf..5891ce2b85 100644 --- a/compiler/packages/babel-plugin-react-compiler/package.json +++ b/compiler/packages/babel-plugin-react-compiler/package.json @@ -9,7 +9,7 @@ ], "scripts": { "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", diff --git a/compiler/packages/babel-plugin-react-compiler/scripts/link-react-compiler-runtime.sh b/compiler/packages/babel-plugin-react-compiler/scripts/link-react-compiler-runtime.sh new file mode 100755 index 0000000000..4d1bcf1ec3 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/scripts/link-react-compiler-runtime.sh @@ -0,0 +1,10 @@ +#!/usr/bin/env bash +# Copyright (c) Meta Platforms, Inc. and affiliates. +# +# This source code is licensed under the MIT license found in the +# LICENSE file in the root directory of this source tree. + +set -eo pipefail + +yarn --silent workspace react-compiler-runtime link +yarn --silent workspace babel-plugin-react-compiler link react-compiler-runtime 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 e966497256..10bcebe44e 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Options.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Options.ts @@ -14,6 +14,7 @@ import { parseEnvironmentConfig, } from '../HIR/Environment'; import {hasOwnProperty} from '../Utils/utils'; +import {fromZodError} from 'zod-validation-error'; const PanicThresholdOptionsSchema = z.enum([ /* @@ -86,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 @@ -121,8 +111,19 @@ export type PluginOptions = { * Set this flag (on by default) to automatically check for this library and activate the support. */ enableReanimatedCheck: boolean; + + /** + * The minimum major version of React that the compiler should emit code for. If the target is 19 + * or higher, the compiler emits direct imports of React runtime APIs needed by the compiler. On + * versions prior to 19, an extra runtime package react-compiler-runtime is necessary to provide + * a userspace approximation of runtime APIs. + */ + target: CompilerReactTarget; }; +const CompilerReactTargetSchema = z.enum(['17', '18', '19']); +export type CompilerReactTarget = z.infer; + const CompilationModeSchema = z.enum([ /* * Compiles functions annotated with "use forget" or component/hook-like functions. @@ -202,7 +203,6 @@ export const defaultOptions: PluginOptions = { logger: null, gating: null, noEmit: false, - runtimeModule: null, eslintSuppressionRules: null, flowSuppressions: true, ignoreUseNoForget: false, @@ -210,6 +210,7 @@ export const defaultOptions: PluginOptions = { return filename.indexOf('node_modules') === -1; }, enableReanimatedCheck: true, + target: '19', } as const; export function parsePluginOptions(obj: unknown): PluginOptions { @@ -222,25 +223,49 @@ export function parsePluginOptions(obj: unknown): PluginOptions { // normalize string configs to be case insensitive value = value.toLowerCase(); } - if (key === 'environment') { - const environmentResult = parseEnvironmentConfig(value); - if (environmentResult.isErr()) { - CompilerError.throwInvalidConfig({ - reason: - 'Error in validating environment config. This is an advanced setting and not meant to be used directly', - description: environmentResult.unwrapErr().toString(), - suggestions: null, - loc: null, - }); + if (isCompilerFlag(key)) { + switch (key) { + case 'environment': { + const environmentResult = parseEnvironmentConfig(value); + if (environmentResult.isErr()) { + CompilerError.throwInvalidConfig({ + reason: + 'Error in validating environment config. This is an advanced setting and not meant to be used directly', + description: environmentResult.unwrapErr().toString(), + suggestions: null, + loc: null, + }); + } + parsedOptions[key] = environmentResult.unwrap(); + break; + } + case 'target': { + parsedOptions[key] = parseTargetConfig(value); + break; + } + default: { + parsedOptions[key] = value; + } } - parsedOptions[key] = environmentResult.unwrap(); - } else if (isCompilerFlag(key)) { - parsedOptions[key] = value; } } return {...defaultOptions, ...parsedOptions}; } +export function parseTargetConfig(value: unknown): CompilerReactTarget { + const parsed = CompilerReactTargetSchema.safeParse(value); + if (parsed.success) { + return parsed.data; + } else { + CompilerError.throwInvalidConfig({ + reason: 'Not a valid target', + description: `${fromZodError(parsed.error)}`, + suggestions: null, + loc: null, + }); + } +} + function isCompilerFlag(s: string): s is keyof PluginOptions { return hasOwnProperty(defaultOptions, s); } diff --git a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts index c2c7d8d640..d60f86e7fa 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts @@ -298,7 +298,6 @@ export function compileProgram( return; } const useMemoCacheIdentifier = program.scope.generateUidIdentifier('c'); - const moduleName = pass.opts.runtimeModule ?? 'react/compiler-runtime'; /* * Record lint errors and critical errors as depending on Forget's config, @@ -605,7 +604,7 @@ export function compileProgram( if (needsMemoCacheFunctionImport) { updateMemoCacheFunctionImport( program, - moduleName, + getReactCompilerRuntimeModule(pass.opts), useMemoCacheIdentifier.name, ); } @@ -638,8 +637,12 @@ function shouldSkipCompilation( } } - const moduleName = pass.opts.runtimeModule ?? 'react/compiler-runtime'; - if (hasMemoCacheFunctionImport(program, moduleName)) { + if ( + hasMemoCacheFunctionImport( + program, + getReactCompilerRuntimeModule(pass.opts), + ) + ) { return true; } return false; @@ -1126,3 +1129,31 @@ function checkFunctionReferencedBeforeDeclarationAtTopLevel( return errors.details.length > 0 ? errors : null; } + +type ReactCompilerRuntimeModule = + | 'react/compiler-runtime' // from react namespace + | 'react-compiler-runtime'; // npm package +function getReactCompilerRuntimeModule( + opts: PluginOptions, +): ReactCompilerRuntimeModule { + let moduleName: ReactCompilerRuntimeModule | null = null; + switch (opts.target) { + case '17': + case '18': { + moduleName = 'react-compiler-runtime'; + break; + } + case '19': { + moduleName = 'react/compiler-runtime'; + break; + } + default: + CompilerError.invariant(moduleName != null, { + reason: 'Expected target to already be validated', + description: null, + loc: null, + suggestions: null, + }); + } + return moduleName; +} 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..80593d6275 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,13 @@ 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 {collectOptionalChainSidemap} from './CollectOptionalChainDependencies'; import { BasicBlock, BlockId, @@ -10,14 +17,16 @@ import { Identifier, IdentifierId, InstructionId, + InstructionValue, ReactiveScopeDependency, ScopeId, } from './HIR'; +import {collectTemporariesSidemap} from './PropagateScopeDependenciesHIR'; /** - * 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 +51,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,27 +72,71 @@ 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, -): ReadonlyMap { + hoistableFromOptionals: ReadonlyMap, + nestedFnImmutableContext: ReadonlySet | null, +): ReadonlyMap { const registry = new PropertyPathRegistry(); - const nodes = collectNonNullsInBlocks(fn, temporaries, registry); - propagateNonNull(fn, nodes); + const functionExpressionLoads = collectFunctionExpressionFakeLoads(fn); + const actuallyEvaluatedTemporaries = new Map( + [...temporaries].filter(([id]) => !functionExpressionLoads.has(id)), + ); - const nodesKeyedByScopeId = new Map(); + /** + * 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); + + 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 = { @@ -96,17 +150,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 +182,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 +199,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; } @@ -184,56 +241,77 @@ 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, + context: CollectNonNullsInBlocksContext, +): PropertyPathNode | null { + let path = null; + if (instr.kind === 'PropertyLoad') { + path = context.temporaries.get(instr.object.identifier.id) ?? { + identifier: instr.object.identifier, + path: [], + }; + } else if (instr.kind === 'Destructure') { + path = context.temporaries.get(instr.value.identifier.id) ?? null; + } else if (instr.kind === 'ComputedLoad') { + path = context.temporaries.get(instr.object.identifier.id) ?? 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, - 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. @@ -245,49 +323,58 @@ 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) { const assumedNonNullObjects = new Set( knownNonNullIdentifiers, ); + + const maybeOptionalChain = context.hoistableFromOptionals.get(block.id); + if (maybeOptionalChain != null) { + assumedNonNullObjects.add( + context.registry.getOrCreateProperty(maybeOptionalChain), + ); + } 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, + 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(), ); - } 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 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); } } } @@ -303,6 +390,7 @@ function collectNonNullsInBlocks( function propagateNonNull( fn: HIRFunction, nodes: ReadonlyMap, + registry: PropertyPathRegistry, ): void { const blockSuccessors = new Map>(); const terminalPreds = new Set(); @@ -388,10 +476,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 +535,74 @@ 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); +} + +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/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/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/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..ab2cf4cf56 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDependenciesHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDependenciesHIR.ts @@ -18,8 +18,8 @@ import { IdentifierId, } from './HIR'; import { - BlockInfo, collectHoistablePropertyLoads, + keyByScopeId, } from './CollectHoistablePropertyLoads'; import { ScopeBlockTraversal, @@ -32,37 +32,60 @@ 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 = keyByScopeId( + fn, + collectHoistablePropertyLoads(fn, temporaries, hoistableObjects, null), + ); 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 ( @@ -188,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 { @@ -201,7 +224,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 +250,7 @@ function collectTemporariesSidemap( function getProperty( object: Place, propertyName: string, + optional: boolean, temporaries: ReadonlyMap, ): ReactiveScopeDependency { /* @@ -253,15 +282,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 +435,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 +520,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 +575,7 @@ function collectDependencies( fn: HIRFunction, usedOutsideDeclaringScope: ReadonlySet, temporaries: ReadonlyMap, + processedInstrsInOptional: ReadonlySet, ): Map> { const context = new Context(usedOutsideDeclaringScope, temporaries); @@ -572,33 +604,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/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/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 ( +