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 f5567b3e536..07f44a59a26 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/DeriveMinimalDependenciesHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/DeriveMinimalDependenciesHIR.ts @@ -13,7 +13,6 @@ import { ReactiveScopeDependency, } from '../HIR'; import {printIdentifier} from '../HIR/PrintHIR'; -import {ReactiveScopePropertyDependency} from '../ReactiveScopes/DeriveMinimalDependencies'; /** * Simpler fork of DeriveMinimalDependencies, see PropagateScopeDependenciesHIR @@ -91,7 +90,7 @@ export class ReactiveScopeDependencyTreeHIR { * dependency. This effectively truncates @param dep to its maximal * safe-to-evaluate subpath */ - addDependency(dep: ReactiveScopePropertyDependency): void { + addDependency(dep: ReactiveScopeDependency): void { const {identifier, path} = dep; let depCursor = ReactiveScopeDependencyTreeHIR.#getOrCreateRoot( identifier, 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 4df6da6c170..e124caee7a2 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts @@ -108,16 +108,7 @@ export type ReactiveValue = | ReactiveLogicalValue | ReactiveSequenceValue | ReactiveTernaryValue - | ReactiveOptionalCallValue - | ReactiveFunctionValue; - -export type ReactiveFunctionValue = { - kind: 'ReactiveFunctionValue'; - fn: ReactiveFunction; - dependencies: Array; - returnType: t.FlowType | t.TSType | null; - loc: SourceLocation; -}; + | ReactiveOptionalCallValue; export type ReactiveLogicalValue = { kind: 'LogicalExpression'; diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts index 7a6586730f9..19d3f707b91 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts @@ -6,7 +6,6 @@ */ import generate from '@babel/generator'; -import {printReactiveFunction} from '..'; import {CompilerError} from '../CompilerError'; import {printReactiveScopeSummary} from '../ReactiveScopes/PrintReactiveFunction'; import DisjointSet from '../Utils/DisjointSet'; @@ -701,10 +700,6 @@ export function printInstructionValue(instrValue: ReactiveValue): string { value = `FinishMemoize decl=${printPlace(instrValue.decl)}`; break; } - case 'ReactiveFunctionValue': { - value = `FunctionValue ${printReactiveFunction(instrValue.fn)}`; - break; - } default: { assertExhaustive( instrValue, diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts index 53812628743..a439b4cd012 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts @@ -10,7 +10,6 @@ import { Effect, HIRFunction, Identifier, - IdentifierId, LoweredFunction, isRefOrRefValue, makeInstructionId, @@ -18,27 +17,9 @@ import { import {deadCodeElimination} from '../Optimization'; import {inferReactiveScopeVariables} from '../ReactiveScopes'; import {rewriteInstructionKindsBasedOnReassignment} from '../SSA'; -import {inferMutableContextVariables} from './InferMutableContextVariables'; import {inferMutableRanges} from './InferMutableRanges'; import inferReferenceEffects from './InferReferenceEffects'; -// Helper class to track indirections such as LoadLocal and PropertyLoad. -export class IdentifierState { - properties: Map = new Map(); - - resolve(identifier: Identifier): Identifier { - const resolved = this.properties.get(identifier.id); - if (resolved !== undefined) { - return resolved; - } - return identifier; - } - - alias(lvalue: Identifier, value: Identifier): void { - this.properties.set(lvalue.id, this.properties.get(value.id) ?? value); - } -} - export default function analyseFunctions(func: HIRFunction): void { for (const [_, block] of func.body.blocks) { for (const instr of block.instructions) { @@ -78,7 +59,6 @@ function lower(func: HIRFunction): void { } function infer(loweredFunc: LoweredFunction): void { - const knownMutated = inferMutableContextVariables(loweredFunc.func); for (const operand of loweredFunc.func.context) { const identifier = operand.identifier; CompilerError.invariant(operand.effect === Effect.Unknown, { @@ -95,10 +75,11 @@ function infer(loweredFunc: LoweredFunction): void { * render */ operand.effect = Effect.Capture; - } else if (knownMutated.has(operand)) { - operand.effect = Effect.Mutate; } else if (isMutatedOrReassigned(identifier)) { - // Note that this also reflects if identifier is ConditionallyMutated + /** + * Reflects direct reassignments, PropertyStores, and ConditionallyMutate + * (directly or through maybe-aliases) + */ operand.effect = Effect.Capture; } else { operand.effect = Effect.Read; diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutableContextVariables.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutableContextVariables.ts deleted file mode 100644 index 00254727215..00000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutableContextVariables.ts +++ /dev/null @@ -1,105 +0,0 @@ -/** - * 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. - */ - -import {Effect, HIRFunction, Identifier, Place} from '../HIR'; -import { - eachInstructionValueOperand, - eachTerminalOperand, -} from '../HIR/visitors'; -import {IdentifierState} from './AnalyseFunctions'; - -/* - * This pass infers which of the given function's context (free) variables - * are definitively mutated by the function. This analysis is *partial*, - * and only annotates provable mutations, and may miss mutations via indirections. - * The intent of this pass is to drive validations, rejecting known-bad code - * while avoiding false negatives, and the inference should *not* be used to - * drive changes in output. - * - * Note that a complete analysis is possible but would have too many false negatives. - * The approach would be to run LeaveSSA and InferReactiveScopeVariables in order to - * find all possible aliases of a context variable which may be mutated. However, this - * can lead to false negatives: - * - * ``` - * const [x, setX] = useState(null); // x is frozen - * const fn = () => { // context=[x] - * const z = {}; // z is mutable - * foo(z, x); // potentially mutate z and x - * z.a = true; // definitively mutate z - * } - * fn(); - * ``` - * - * When we analyze function expressions we assume that context variables are mutable, - * so we assume that `x` is mutable. We infer that `foo(z, x)` could be mutating the - * two variables to alias each other, such that `z.a = true` could be mutating `x`, - * and we would infer that `x` is definitively mutated. Then when we run - * InferReferenceEffects on the outer code we'd reject it, since there is a definitive - * mutation of a frozen value. - * - * Thus the actual implementation looks at only basic aliasing. The above example would - * pass, since z does not directly alias `x`. However, mutations through trivial aliases - * are detected: - * - * ``` - * const [x, setX] = useState(null); // x is frozen - * const fn = () => { // context=[x] - * const z = x; - * z.a = true; // ERROR: mutates x - * } - * fn(); - * ``` - */ -export function inferMutableContextVariables(fn: HIRFunction): Set { - const state = new IdentifierState(); - const knownMutatedIdentifiers = new Set(); - for (const [, block] of fn.body.blocks) { - for (const instr of block.instructions) { - switch (instr.value.kind) { - case 'PropertyLoad': - case 'ComputedLoad': { - state.alias(instr.lvalue.identifier, instr.value.object.identifier); - break; - } - case 'LoadLocal': - case 'LoadContext': { - if (instr.lvalue.identifier.name === null) { - state.alias(instr.lvalue.identifier, instr.value.place.identifier); - } - break; - } - default: { - for (const operand of eachInstructionValueOperand(instr.value)) { - visitOperand(state, knownMutatedIdentifiers, operand); - } - } - } - } - for (const operand of eachTerminalOperand(block.terminal)) { - visitOperand(state, knownMutatedIdentifiers, operand); - } - } - const results = new Set(); - for (const operand of fn.context) { - if (knownMutatedIdentifiers.has(operand.identifier)) { - results.add(operand); - } - } - return results; -} - -function visitOperand( - state: IdentifierState, - knownMutatedIdentifiers: Set, - operand: Place, -): void { - const resolved = state.resolve(operand.identifier); - if (operand.effect === Effect.Mutate || operand.effect === Effect.Store) { - knownMutatedIdentifiers.add(resolved); - } -} diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts index cf40d86abc5..93b70bd7d1d 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts @@ -2231,7 +2231,6 @@ function codegenInstructionValue( ); break; } - case 'ReactiveFunctionValue': case 'StartMemoize': case 'FinishMemoize': case 'Debugger': diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/DeriveMinimalDependencies.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/DeriveMinimalDependencies.ts deleted file mode 100644 index c7e16cce7ac..00000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/DeriveMinimalDependencies.ts +++ /dev/null @@ -1,639 +0,0 @@ -/** - * 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. - */ - -import {CompilerError} from '../CompilerError'; -import {DependencyPath, Identifier, ReactiveScopeDependency} from '../HIR'; -import {printIdentifier} from '../HIR/PrintHIR'; -import {assertExhaustive} from '../Utils/utils'; - -/* - * We need to understand optional member expressions only when determining - * dependencies of a ReactiveScope (i.e. in {@link PropagateScopeDependencies}), - * hence why this type lives here (not in HIR.ts) - */ -export type ReactiveScopePropertyDependency = ReactiveScopeDependency; - -/* - * Finalizes a set of ReactiveScopeDependencies to produce a set of minimal unconditional - * dependencies, preserving granular accesses when possible. - * - * Correctness properties: - * - All dependencies to a ReactiveBlock must be tracked. - * We can always truncate a dependency's path to a subpath, due to Forget assuming - * deep immutability. If the value produced by a subpath has not changed, then - * dependency must have not changed. - * i.e. props.a === $[..] implies props.a.b === $[..] - * - * Note the inverse is not true, but this only means a false positive (we run the - * reactive block more than needed). - * i.e. props.a !== $[..] does not imply props.a.b !== $[..] - * - * - The dependencies of a finalized ReactiveBlock must be all safe to access - * unconditionally (i.e. preserve program semantics with respect to nullthrows). - * If a dependency is only accessed within a conditional, we must track the nearest - * unconditionally accessed subpath instead. - * @param initialDeps - * @returns - */ -export class ReactiveScopeDependencyTree { - #roots: Map = new Map(); - - #getOrCreateRoot(identifier: Identifier): DependencyNode { - // roots can always be accessed unconditionally in JS - let rootNode = this.#roots.get(identifier); - - if (rootNode === undefined) { - rootNode = { - properties: new Map(), - accessType: PropertyAccessType.UnconditionalAccess, - }; - this.#roots.set(identifier, rootNode); - } - return rootNode; - } - - add(dep: ReactiveScopePropertyDependency, inConditional: boolean): void { - const {path} = dep; - let currNode = this.#getOrCreateRoot(dep.identifier); - - for (const item of path) { - // all properties read 'on the way' to a dependency are marked as 'access' - let currChild = getOrMakeProperty(currNode, item.property); - const accessType = inConditional - ? PropertyAccessType.ConditionalAccess - : item.optional - ? PropertyAccessType.OptionalAccess - : PropertyAccessType.UnconditionalAccess; - currChild.accessType = merge(currChild.accessType, accessType); - currNode = currChild; - } - - /** - * The final property node should be marked as an conditional/unconditional - * `dependency` as based on control flow. - */ - const depType = inConditional - ? PropertyAccessType.ConditionalDependency - : isOptional(currNode.accessType) - ? PropertyAccessType.OptionalDependency - : PropertyAccessType.UnconditionalDependency; - - currNode.accessType = merge(currNode.accessType, depType); - } - - deriveMinimalDependencies(): Set { - const results = new Set(); - for (const [rootId, rootNode] of this.#roots.entries()) { - const deps = deriveMinimalDependenciesInSubtree(rootNode, null); - CompilerError.invariant( - deps.every( - dep => - dep.accessType === PropertyAccessType.UnconditionalDependency || - dep.accessType == PropertyAccessType.OptionalDependency, - ), - { - reason: - '[PropagateScopeDependencies] All dependencies must be reduced to unconditional dependencies.', - description: null, - loc: null, - suggestions: null, - }, - ); - - for (const dep of deps) { - results.add({ - identifier: rootId, - path: dep.relativePath, - }); - } - } - - return results; - } - - addDepsFromInnerScope( - depsFromInnerScope: ReactiveScopeDependencyTree, - innerScopeInConditionalWithinParent: boolean, - checkValidDepIdFn: (dep: ReactiveScopeDependency) => boolean, - ): void { - for (const [id, otherRoot] of depsFromInnerScope.#roots) { - if (!checkValidDepIdFn({identifier: id, path: []})) { - continue; - } - let currRoot = this.#getOrCreateRoot(id); - addSubtree(currRoot, otherRoot, innerScopeInConditionalWithinParent); - if (!isUnconditional(currRoot.accessType)) { - currRoot.accessType = isDependency(currRoot.accessType) - ? PropertyAccessType.UnconditionalDependency - : PropertyAccessType.UnconditionalAccess; - } - } - } - - promoteDepsFromExhaustiveConditionals( - trees: Array, - ): void { - CompilerError.invariant(trees.length > 1, { - reason: 'Expected trees to be at least 2 elements long.', - description: null, - loc: null, - suggestions: null, - }); - - for (const [id, root] of this.#roots) { - const nodesForRootId = mapNonNull(trees, tree => { - const node = tree.#roots.get(id); - if (node != null && isUnconditional(node.accessType)) { - return node; - } else { - return null; - } - }); - if (nodesForRootId) { - addSubtreeIntersection( - root.properties, - nodesForRootId.map(root => root.properties), - ); - } - } - } - - /* - * Prints dependency tree to string for debugging. - * @param includeAccesses - * @returns string representation of DependencyTree - */ - printDeps(includeAccesses: boolean): string { - let res = []; - - for (const [rootId, rootNode] of this.#roots.entries()) { - const rootResults = printSubtree(rootNode, includeAccesses).map( - result => `${printIdentifier(rootId)}.${result}`, - ); - res.push(rootResults); - } - return res.flat().join('\n'); - } - - debug(): string { - const buf: Array = [`tree() [`]; - for (const [rootId, rootNode] of this.#roots) { - buf.push(`${printIdentifier(rootId)} (${rootNode.accessType}):`); - this.#debugImpl(buf, rootNode, 1); - } - buf.push(']'); - return buf.length > 2 ? buf.join('\n') : buf.join(''); - } - - #debugImpl( - buf: Array, - node: DependencyNode, - depth: number = 0, - ): void { - for (const [property, childNode] of node.properties) { - buf.push(`${' '.repeat(depth)}.${property} (${childNode.accessType}):`); - this.#debugImpl(buf, childNode, depth + 1); - } - } -} - -/* - * Enum representing the access type of single property on a parent object. - * We distinguish on two independent axes: - * Conditional / Unconditional: - * - whether this property is accessed unconditionally (within the ReactiveBlock) - * Access / Dependency: - * - Access: this property is read on the path of a dependency. We do not - * need to track change variables for accessed properties. Tracking accesses - * helps Forget do more granular dependency tracking. - * - Dependency: this property is read as a dependency and we must track changes - * to it for correctness. - * - * ```javascript - * // props.a is a dependency here and must be tracked - * deps: {props.a, props.a.b} ---> minimalDeps: {props.a} - * // props.a is just an access here and does not need to be tracked - * deps: {props.a.b} ---> minimalDeps: {props.a.b} - * ``` - */ -enum PropertyAccessType { - ConditionalAccess = 'ConditionalAccess', - OptionalAccess = 'OptionalAccess', - UnconditionalAccess = 'UnconditionalAccess', - ConditionalDependency = 'ConditionalDependency', - OptionalDependency = 'OptionalDependency', - UnconditionalDependency = 'UnconditionalDependency', -} - -const MIN_ACCESS_TYPE = PropertyAccessType.ConditionalAccess; -function isUnconditional(access: PropertyAccessType): boolean { - return ( - access === PropertyAccessType.UnconditionalAccess || - access === PropertyAccessType.UnconditionalDependency - ); -} -function isDependency(access: PropertyAccessType): boolean { - return ( - access === PropertyAccessType.ConditionalDependency || - access === PropertyAccessType.OptionalDependency || - access === PropertyAccessType.UnconditionalDependency - ); -} -function isOptional(access: PropertyAccessType): boolean { - return ( - access === PropertyAccessType.OptionalAccess || - access === PropertyAccessType.OptionalDependency - ); -} - -function merge( - access1: PropertyAccessType, - access2: PropertyAccessType, -): PropertyAccessType { - const resultIsUnconditional = - isUnconditional(access1) || isUnconditional(access2); - const resultIsDependency = isDependency(access1) || isDependency(access2); - const resultIsOptional = isOptional(access1) || isOptional(access2); - - /* - * Straightforward merge. - * This can be represented as bitwise OR, but is written out for readability - * - * 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 (resultIsUnconditional) { - if (resultIsDependency) { - return PropertyAccessType.UnconditionalDependency; - } else { - return PropertyAccessType.UnconditionalAccess; - } - } else if (resultIsOptional) { - if (resultIsDependency) { - return PropertyAccessType.OptionalDependency; - } else { - return PropertyAccessType.OptionalAccess; - } - } else { - if (resultIsDependency) { - return PropertyAccessType.ConditionalDependency; - } else { - return PropertyAccessType.ConditionalAccess; - } - } -} - -type DependencyNode = { - properties: Map; - accessType: PropertyAccessType; -}; - -type ReduceResultNode = { - relativePath: DependencyPath; - accessType: PropertyAccessType; -}; - -function promoteResult( - accessType: PropertyAccessType, - path: {property: string; optional: boolean} | null, -): Array { - const result: ReduceResultNode = { - relativePath: [], - accessType, - }; - if (path !== null) { - result.relativePath.push(path); - } - return [result]; -} - -function prependPath( - results: Array, - path: {property: string; optional: boolean} | null, -): Array { - if (path === null) { - return results; - } - return results.map(result => { - return { - accessType: result.accessType, - relativePath: [path, ...result.relativePath], - }; - }); -} - -/* - * Recursively calculates minimal dependencies in a subtree. - * @param dep DependencyNode representing a dependency subtree. - * @returns a minimal list of dependencies in this subtree. - */ -function deriveMinimalDependenciesInSubtree( - dep: DependencyNode, - property: string | null, -): Array { - const results: Array = []; - for (const [childName, childNode] of dep.properties) { - const childResult = deriveMinimalDependenciesInSubtree( - childNode, - childName, - ); - results.push(...childResult); - } - - switch (dep.accessType) { - case PropertyAccessType.UnconditionalDependency: { - return promoteResult( - PropertyAccessType.UnconditionalDependency, - property !== null ? {property, optional: false} : null, - ); - } - case PropertyAccessType.UnconditionalAccess: { - if ( - results.every( - ({accessType}) => - accessType === PropertyAccessType.UnconditionalDependency || - accessType === PropertyAccessType.OptionalDependency, - ) - ) { - // all children are unconditional dependencies, return them to preserve granularity - return prependPath( - results, - property !== null ? {property, optional: false} : null, - ); - } else { - /* - * at least one child is accessed conditionally, so this node needs to be promoted to - * unconditional dependency - */ - return promoteResult( - PropertyAccessType.UnconditionalDependency, - property !== null ? {property, optional: false} : null, - ); - } - } - case PropertyAccessType.OptionalDependency: { - return promoteResult( - PropertyAccessType.OptionalDependency, - property !== null ? {property, optional: true} : null, - ); - } - case PropertyAccessType.OptionalAccess: { - if ( - results.every( - ({accessType}) => - accessType === PropertyAccessType.UnconditionalDependency || - accessType === PropertyAccessType.OptionalDependency, - ) - ) { - // all children are unconditional dependencies, return them to preserve granularity - return prependPath( - results, - property !== null ? {property, optional: true} : null, - ); - } else { - /* - * at least one child is accessed conditionally, so this node needs to be promoted to - * unconditional dependency - */ - return promoteResult( - PropertyAccessType.OptionalDependency, - property !== null ? {property, optional: true} : null, - ); - } - } - case PropertyAccessType.ConditionalAccess: - case PropertyAccessType.ConditionalDependency: { - if ( - results.every( - ({accessType}) => - accessType === PropertyAccessType.ConditionalDependency, - ) - ) { - /* - * No children are accessed unconditionally, so we cannot promote this node to - * unconditional access. - * Truncate results of child nodes here, since we shouldn't access them anyways - */ - return promoteResult( - PropertyAccessType.ConditionalDependency, - property !== null ? {property, optional: true} : null, - ); - } else { - /* - * at least one child is accessed unconditionally, so this node can be promoted to - * unconditional dependency - */ - return promoteResult( - PropertyAccessType.UnconditionalDependency, - property !== null ? {property, optional: true} : null, - ); - } - } - default: { - assertExhaustive( - dep.accessType, - '[PropgateScopeDependencies] Unhandled access type!', - ); - } - } -} - -/* - * Demote all unconditional accesses + dependencies in subtree to the - * conditional equivalent, mutating subtree in place. - * @param subtree unconditional node representing a subtree of dependencies - */ -function demoteSubtreeToConditional(subtree: DependencyNode): void { - const stack: Array = [subtree]; - - let node; - while ((node = stack.pop()) !== undefined) { - const {accessType, properties} = node; - if (!isUnconditional(accessType)) { - // A conditionally accessed node should not have unconditional children - continue; - } - node.accessType = isDependency(accessType) - ? PropertyAccessType.ConditionalDependency - : PropertyAccessType.ConditionalAccess; - - for (const childNode of properties.values()) { - if (isUnconditional(accessType)) { - /* - * No conditional node can have an unconditional node as a child, so - * we only process childNode if it is unconditional - */ - stack.push(childNode); - } - } - } -} - -/* - * Calculates currNode = union(currNode, otherNode), mutating currNode in place - * If demoteOtherNode is specified, we demote the subtree represented by - * otherNode to conditional access/deps before taking the union. - * - * This is a helper function used to join an inner scope to its parent scope. - * @param currNode (mutable) return by argument - * @param otherNode (move) {@link addSubtree} takes ownership of the subtree - * represented by otherNode, which may be mutated or moved to currNode. It is - * invalid to use otherNode after this call. - * - * Note that @param otherNode may contain both conditional and unconditional nodes, - * due to inner control flow and conditional member expressions - * - * @param demoteOtherNode - */ -function addSubtree( - currNode: DependencyNode, - otherNode: DependencyNode, - demoteOtherNode: boolean, -): void { - let otherType = otherNode.accessType; - if (demoteOtherNode) { - otherType = isDependency(otherType) - ? PropertyAccessType.ConditionalDependency - : PropertyAccessType.ConditionalAccess; - } - currNode.accessType = merge(currNode.accessType, otherType); - - for (const [propertyName, otherChild] of otherNode.properties) { - const currChild = currNode.properties.get(propertyName); - if (currChild) { - // recursively calculate currChild = union(currChild, otherChild) - addSubtree(currChild, otherChild, demoteOtherNode); - } else { - /* - * if currChild doesn't exist, we can just move otherChild - * currChild = otherChild. - */ - if (demoteOtherNode) { - demoteSubtreeToConditional(otherChild); - } - currNode.properties.set(propertyName, otherChild); - } - } -} - -/* - * Adds intersection(otherProperties) to currProperties, mutating - * currProperties in place. i.e. - * currProperties = union(currProperties, intersection(otherProperties)) - * - * Used to merge unconditional accesses from exhaustive conditional branches - * into the parent ReactiveDeps Tree. - * intersection(currProperties) is determined as such: - * - a node is present in the intersection iff it is present in all every - * branch - * - the type of an added node is `UnconditionalDependency` if it is a - * dependency in at least one branch (otherwise `UnconditionalAccess`) - * - * @param otherProperties (read-only) an array of node properties containing - * conditionally and unconditionally accessed nodes. Each element - * represents asubtree of reactive dependencies from a single CFG - * branch. - * otherProperties must represent all reachable branches. - * @param currProperties (mutable) return by argument properties of a node - * - * otherProperties and currProperties must be properties of disjoint nodes - * that represent the same dependency (identifier + path). - */ -function addSubtreeIntersection( - currProperties: Map, - otherProperties: Array>, -): void { - CompilerError.invariant(otherProperties.length > 1, { - reason: - '[DeriveMinimalDependencies] Expected otherProperties to be at least 2 elements long.', - description: null, - loc: null, - suggestions: null, - }); - - /* - * otherProperties here may contain unconditional nodes as the result of - * recursively merging exhaustively conditional children with unconditionally - * accessed nodes (e.g. in the test condition itself) - * See `reduce-reactive-cond-deps-cfg-nested-testifelse` fixture for example - */ - - for (const [propertyName, currNode] of currProperties) { - const otherNodes = mapNonNull(otherProperties, properties => { - const node = properties.get(propertyName); - if (node != null && isUnconditional(node.accessType)) { - return node; - } else { - return null; - } - }); - - /* - * intersection(otherNodes[propertyName]) only exists if each element in - * otherProperties accesses propertyName. - */ - if (otherNodes) { - addSubtreeIntersection( - currNode.properties, - otherNodes.map(node => node.properties), - ); - - const isDep = otherNodes.some(tree => isDependency(tree.accessType)); - const externalAccessType = isDep - ? PropertyAccessType.UnconditionalDependency - : PropertyAccessType.UnconditionalAccess; - currNode.accessType = merge(externalAccessType, currNode.accessType); - } - } -} - -function printSubtree( - node: DependencyNode, - includeAccesses: boolean, -): Array { - const results: Array = []; - for (const [propertyName, propertyNode] of node.properties) { - if (includeAccesses || isDependency(propertyNode.accessType)) { - results.push(`${propertyName} (${propertyNode.accessType})`); - } - const propertyResults = printSubtree(propertyNode, includeAccesses); - results.push(...propertyResults.map(result => `${propertyName}.${result}`)); - } - return results; -} - -function getOrMakeProperty( - node: DependencyNode, - property: string, -): DependencyNode { - let child = node.properties.get(property); - if (child == null) { - child = { - properties: new Map(), - accessType: MIN_ACCESS_TYPE, - }; - node.properties.set(property, child); - } - return child; -} - -function mapNonNull, V, U>( - arr: Array, - fn: (arg0: U) => T | undefined | null, -): Array | null { - const result = []; - for (let i = 0; i < arr.length; i++) { - const element = fn(arr[i]); - if (element) { - result.push(element); - } else { - return null; - } - } - return result; -} diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts index 5a9aa6b2a73..441d529b396 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts @@ -772,14 +772,6 @@ function computeMemoizationInputs( rvalues: operands, }; } - case 'ReactiveFunctionValue': { - CompilerError.invariant(false, { - reason: `Unexpected ReactiveFunctionValue node`, - description: null, - loc: value.loc, - suggestions: null, - }); - } case 'UnsupportedNode': { CompilerError.invariant(false, { reason: `Unexpected unsupported node`, diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/visitors.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/visitors.ts index 487de2767a5..4ad05aa302a 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/visitors.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/visitors.ts @@ -73,15 +73,6 @@ export class ReactiveFunctionVisitor { this.visitValue(value.id, value.value, state); break; } - case 'ReactiveFunctionValue': { - this.visitReactiveFunctionValue( - id, - value.dependencies, - value.fn, - state, - ); - break; - } default: { for (const place of eachInstructionValueOperand(value)) { this.visitPlace(id, place, state); @@ -434,18 +425,6 @@ export class ReactiveFunctionTransform< } break; } - case 'ReactiveFunctionValue': { - const nextValue = this.transformReactiveFunctionValue( - id, - value.dependencies, - value.fn, - state, - ); - if (nextValue.kind === 'replace') { - value.fn = nextValue.value; - } - break; - } default: { for (const place of eachInstructionValueOperand(value)) { this.visitPlace(id, place, state); @@ -619,10 +598,6 @@ export function* eachReactiveValueOperand( yield* eachReactiveValueOperand(instrValue.alternate); break; } - case 'ReactiveFunctionValue': { - yield* instrValue.dependencies; - break; - } default: { yield* eachInstructionValueOperand(instrValue); } diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts index e7615320c7b..28be6c166d2 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts @@ -327,13 +327,6 @@ class Visitor extends ReactiveFunctionVisitor { case 'OptionalExpression': { return this.recordDepsInValue(value.value, state); } - case 'ReactiveFunctionValue': { - CompilerError.throwTodo({ - reason: - 'Handle ReactiveFunctionValue in ValidatePreserveManualMemoization', - loc: value.loc, - }); - } case 'ConditionalExpression': { this.recordDepsInValue(value.test, state); this.recordDepsInValue(value.consequent, state); diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutate-global-in-effect-fixpoint.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutate-global-in-effect-fixpoint.expect.md index 942daec1dd0..76e4432fe90 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutate-global-in-effect-fixpoint.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutate-global-in-effect-fixpoint.expect.md @@ -19,9 +19,14 @@ function Component() { // capture into a separate variable that is not a context variable. const y = x; + /** + * Note that this fixture currently produces a stale effect closure if `y = x + * = someGlobal` changes between renders. Under current compiler assumptions, + * that would be a rule of react violation. + */ useEffect(() => { y.value = 'hello'; - }, []); + }); useEffect(() => { setState(someGlobal.value); @@ -46,57 +51,50 @@ import { useEffect, useState } from "react"; let someGlobal = { value: null }; function Component() { - const $ = _c(7); + const $ = _c(5); const [state, setState] = useState(someGlobal); + + let x = someGlobal; + while (x == null) { + x = someGlobal; + } + + const y = x; let t0; - let t1; - let t2; if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - let x = someGlobal; - while (x == null) { - x = someGlobal; - } - - const y = x; - t0 = useEffect; - t1 = () => { + t0 = () => { y.value = "hello"; }; - t2 = []; $[0] = t0; + } else { + t0 = $[0]; + } + useEffect(t0); + let t1; + let t2; + if ($[1] === Symbol.for("react.memo_cache_sentinel")) { + t1 = () => { + setState(someGlobal.value); + }; + t2 = [someGlobal]; $[1] = t1; $[2] = t2; } else { - t0 = $[0]; t1 = $[1]; t2 = $[2]; } - t0(t1, t2); - let t3; + useEffect(t1, t2); + + const t3 = String(state); let t4; - if ($[3] === Symbol.for("react.memo_cache_sentinel")) { - t3 = () => { - setState(someGlobal.value); - }; - t4 = [someGlobal]; + if ($[3] !== t3) { + t4 =
{t3}
; $[3] = t3; $[4] = t4; } else { - t3 = $[3]; t4 = $[4]; } - useEffect(t3, t4); - - const t5 = String(state); - let t6; - if ($[5] !== t5) { - t6 =
{t5}
; - $[5] = t5; - $[6] = t6; - } else { - t6 = $[6]; - } - return t6; + return t4; } export const FIXTURE_ENTRYPOINT = { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutate-global-in-effect-fixpoint.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutate-global-in-effect-fixpoint.js index 84bd42aaefd..6e44adf2041 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutate-global-in-effect-fixpoint.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutate-global-in-effect-fixpoint.js @@ -15,9 +15,14 @@ function Component() { // capture into a separate variable that is not a context variable. const y = x; + /** + * Note that this fixture currently produces a stale effect closure if `y = x + * = someGlobal` changes between renders. Under current compiler assumptions, + * that would be a rule of react violation. + */ useEffect(() => { y.value = 'hello'; - }, []); + }); useEffect(() => { setState(someGlobal.value); diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reanimated-shared-value-writes.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reanimated-shared-value-writes.expect.md index 8f808c94b30..0a19a854289 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reanimated-shared-value-writes.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reanimated-shared-value-writes.expect.md @@ -36,21 +36,22 @@ import { useSharedValue } from "react-native-reanimated"; * of render */ function SomeComponent() { - const $ = _c(3); + const $ = _c(2); const sharedVal = useSharedValue(0); - - const T0 = Button; - const t0 = () => (sharedVal.value = Math.random()); - let t1; - if ($[0] !== T0 || $[1] !== t0) { - t1 = ; - $[0] = T0; + let t0; + if ($[0] !== sharedVal) { + t0 = ( +