From 7c0c9abea0ea75c6eb130fa6de54911a12046dca Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Tue, 18 Feb 2025 09:16:26 -0700 Subject: [PATCH] [compiler] Delete LoweredFunction.dependencies and hoisted instructions LoweredFunction dependencies were exclusively used for dependency extraction (in `propagateScopeDeps`). Now that we have a `propagateScopeDepsHIR` that recursively traverses into nested functions, we can delete `dependencies` and their associated synthetic `LoadLocal`/`PropertyLoad` instructions. [Internal snapshot diff](https://www.internalfb.com/phabricator/paste/view/P1716950202) for this change shows ~.2% of files changed. I [read through ~60 of the changed files](https://www.internalfb.com/phabricator/paste/view/P1733074307) - most changes are due to better outlining (due to better DCE) - a few changes in memo inference are due to changed ordering ``` // source arr.map(() => contextVar.inner); // previous instructions $0 = LoadLocal arr $1 = $0.map // Below instructions are synthetic $2 = LoadLocal contextVar $3 = $2.inner $4 = Function deps=$3 context=contextVar { ... } ``` - a few changes are effectively bugfixes (see `aliased-nested-scope-fn-expr`) --- .../src/HIR/BuildHIR.ts | 154 +----------- .../src/HIR/CollectHoistablePropertyLoads.ts | 37 +-- .../src/HIR/Environment.ts | 2 - .../src/HIR/HIR.ts | 1 - .../HIR/MergeOverlappingReactiveScopesHIR.ts | 17 +- .../src/HIR/PrintHIR.ts | 5 +- .../src/HIR/PropagateScopeDependenciesHIR.ts | 5 +- .../src/HIR/visitors.ts | 7 +- .../src/Inference/AnalyseFunctions.ts | 146 +++--------- .../Inference/InferMutableContextVariables.ts | 23 +- .../src/Inference/InferReferenceEffects.ts | 48 ++-- .../src/Optimization/LowerContextAccess.ts | 1 - .../src/Optimization/OutlineFunctions.ts | 1 - .../InferReactiveScopeVariables.ts | 8 + .../src/SSA/EliminateRedundantPhi.ts | 19 ++ .../src/SSA/EnterSSA.ts | 3 - .../src/Transform/TransformFire.ts | 53 +---- .../aliased-nested-scope-fn-expr.expect.md | 120 ++++++++++ .../compiler/aliased-nested-scope-fn-expr.tsx | 46 ++++ ...iased-nested-scope-truncated-dep.expect.md | 221 ++++++++++++++++++ .../aliased-nested-scope-truncated-dep.tsx | 93 ++++++++ ...access-in-unused-callback-nested.expect.md | 40 ++-- .../capturing-func-mutate-2.expect.md | 1 - .../capturing-func-no-mutate.expect.md | 12 +- ...capturing-func-simple-alias-iife.expect.md | 1 - ...ction-alias-computed-load-2-iife.expect.md | 1 - ...ction-alias-computed-load-3-iife.expect.md | 2 - ...ction-alias-computed-load-4-iife.expect.md | 1 - ...unction-alias-computed-load-iife.expect.md | 1 - ...capturing-reference-changes-type.expect.md | 1 - .../codegen-inline-iife-reassign.expect.md | 3 +- ...-into-function-expression-global.expect.md | 7 +- ...to-function-expression-primitive.expect.md | 7 +- ...gation-into-function-expressions.expect.md | 9 +- ...text-variable-as-jsx-element-tag.expect.md | 3 +- ...ting-simple-function-declaration.expect.md | 10 +- ...call-freezes-captured-identifier.expect.md | 41 ++++ ....hook-call-freezes-captured-identifier.tsx | 20 ++ ...call-freezes-captured-memberexpr.expect.md | 41 ++++ ....hook-call-freezes-captured-memberexpr.jsx | 20 ++ ...on-with-shadowed-local-same-name.expect.md | 2 +- ...setstate-captured-indirectly-jsx.expect.md | 81 +++++++ ...isting-setstate-captured-indirectly-jsx.js | 17 ++ .../compiler/hoisting-setstate.expect.md | 77 ++++++ .../fixtures/compiler/hoisting-setstate.js | 28 +++ ...call-freezes-captured-memberexpr.expect.md | 87 +++++++ .../hook-call-freezes-captured-memberexpr.tsx | 21 ++ .../jsx-local-tag-in-lambda.expect.md | 7 +- .../jsx-memberexpr-tag-in-lambda.expect.md | 7 +- ...mutated-non-reactive-to-reactive.expect.md | 1 - .../lambda-mutated-ref-non-reactive.expect.md | 1 - ...ed-function-shadowed-identifiers.expect.md | 5 +- ...o-reordering-depslist-assignment.expect.md | 1 - ...e-phis-in-lambda-capture-context.expect.md | 29 +-- .../use-operator-conditional.expect.md | 1 - 55 files changed, 1123 insertions(+), 473 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/aliased-nested-scope-fn-expr.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/aliased-nested-scope-fn-expr.tsx create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/aliased-nested-scope-truncated-dep.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/aliased-nested-scope-truncated-dep.tsx create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hook-call-freezes-captured-identifier.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hook-call-freezes-captured-identifier.tsx create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hook-call-freezes-captured-memberexpr.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hook-call-freezes-captured-memberexpr.jsx create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-setstate-captured-indirectly-jsx.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-setstate-captured-indirectly-jsx.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-setstate.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-setstate.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hook-call-freezes-captured-memberexpr.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hook-call-freezes-captured-memberexpr.tsx diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts index d5c3ea6a492a9..67cdcab0381f2 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts @@ -7,7 +7,6 @@ import {NodePath, Scope} from '@babel/traverse'; import * as t from '@babel/types'; -import {Expression} from '@babel/types'; import invariant from 'invariant'; import { CompilerError, @@ -75,7 +74,7 @@ export function lower( parent: NodePath | null = null, ): Result { const builder = new HIRBuilder(env, parent ?? func, bindings, capturedRefs); - const context: Array = []; + const context: HIRFunction['context'] = []; for (const ref of capturedRefs ?? []) { context.push({ @@ -3378,7 +3377,7 @@ function lowerFunction( >, ): LoweredFunction | null { const componentScope: Scope = builder.parentFunction.scope; - const captured = gatherCapturedDeps(builder, expr, componentScope); + const capturedContext = gatherCapturedContext(expr, componentScope); /* * TODO(gsn): In the future, we could only pass in the context identifiers @@ -3392,7 +3391,7 @@ function lowerFunction( expr, builder.environment, builder.bindings, - [...builder.context, ...captured.identifiers], + [...builder.context, ...capturedContext], builder.parentFunction, ); let loweredFunc: HIRFunction; @@ -3405,7 +3404,6 @@ function lowerFunction( loweredFunc = lowering.unwrap(); return { func: loweredFunc, - dependencies: captured.refs, }; } @@ -4079,14 +4077,6 @@ function lowerAssignment( } } -function isValidDependency(path: NodePath): boolean { - const parent: NodePath = path.parentPath; - return ( - !path.node.computed && - !(parent.isCallExpression() && parent.get('callee') === path) - ); -} - function captureScopes({from, to}: {from: Scope; to: Scope}): Set { let scopes: Set = new Set(); while (from) { @@ -4101,8 +4091,7 @@ function captureScopes({from, to}: {from: Scope; to: Scope}): Set { return scopes; } -function gatherCapturedDeps( - builder: HIRBuilder, +function gatherCapturedContext( fn: NodePath< | t.FunctionExpression | t.ArrowFunctionExpression @@ -4110,10 +4099,8 @@ function gatherCapturedDeps( | t.ObjectMethod >, componentScope: Scope, -): {identifiers: Array; refs: Array} { - const capturedIds: Map = new Map(); - const capturedRefs: Set = new Set(); - const seenPaths: Set = new Set(); +): Array { + const capturedIds = new Set(); /* * Capture all the scopes from the parent of this function up to and including @@ -4124,33 +4111,11 @@ function gatherCapturedDeps( to: componentScope, }); - function addCapturedId(bindingIdentifier: t.Identifier): number { - if (!capturedIds.has(bindingIdentifier)) { - const index = capturedIds.size; - capturedIds.set(bindingIdentifier, index); - return index; - } else { - return capturedIds.get(bindingIdentifier)!; - } - } - function handleMaybeDependency( - path: - | NodePath - | NodePath - | NodePath, + path: NodePath | NodePath, ): void { // Base context variable to depend on let baseIdentifier: NodePath | NodePath; - /* - * Base expression to depend on, which (for now) may contain non side-effectful - * member expressions - */ - let dependency: - | NodePath - | NodePath - | NodePath - | NodePath; if (path.isJSXOpeningElement()) { const name = path.get('name'); if (!(name.isJSXMemberExpression() || name.isJSXIdentifier())) { @@ -4166,115 +4131,20 @@ function gatherCapturedDeps( 'Invalid logic in gatherCapturedDeps', ); baseIdentifier = current; - - /* - * Get the expression to depend on, which may involve PropertyLoads - * for member expressions - */ - let currentDep: - | NodePath - | NodePath - | NodePath = baseIdentifier; - - while (true) { - const nextDep: null | NodePath = currentDep.parentPath; - if (nextDep && nextDep.isJSXMemberExpression()) { - currentDep = nextDep; - } else { - break; - } - } - dependency = currentDep; - } else if (path.isMemberExpression()) { - // Calculate baseIdentifier - let currentId: NodePath = path; - while (currentId.isMemberExpression()) { - currentId = currentId.get('object'); - } - if (!currentId.isIdentifier()) { - return; - } - baseIdentifier = currentId; - - /* - * Get the expression to depend on, which may involve PropertyLoads - * for member expressions - */ - let currentDep: - | NodePath - | NodePath - | NodePath = baseIdentifier; - - while (true) { - const nextDep: null | NodePath = currentDep.parentPath; - if ( - nextDep && - nextDep.isMemberExpression() && - isValidDependency(nextDep) - ) { - currentDep = nextDep; - } else { - break; - } - } - - dependency = currentDep; } else { baseIdentifier = path; - dependency = path; } /* * Skip dependency path, as we already tried to recursively add it (+ all subexpressions) * as a dependency. */ - dependency.skip(); + path.skip(); // Add the base identifier binding as a dependency. const binding = baseIdentifier.scope.getBinding(baseIdentifier.node.name); - if (binding === undefined || !pureScopes.has(binding.scope)) { - return; - } - const idKey = String(addCapturedId(binding.identifier)); - - // Add the expression (potentially a memberexpr path) as a dependency. - let exprKey = idKey; - if (dependency.isMemberExpression()) { - let pathTokens = []; - let current: NodePath = dependency; - while (current.isMemberExpression()) { - const property = current.get('property') as NodePath; - pathTokens.push(property.node.name); - current = current.get('object'); - } - - exprKey += '.' + pathTokens.reverse().join('.'); - } else if (dependency.isJSXMemberExpression()) { - let pathTokens = []; - let current: NodePath = - dependency; - while (current.isJSXMemberExpression()) { - const property = current.get('property'); - pathTokens.push(property.node.name); - current = current.get('object'); - } - } - - if (!seenPaths.has(exprKey)) { - let loweredDep: Place; - if (dependency.isJSXIdentifier()) { - loweredDep = lowerValueToTemporary(builder, { - kind: 'LoadLocal', - place: lowerIdentifier(builder, dependency), - loc: path.node.loc ?? GeneratedSource, - }); - } else if (dependency.isJSXMemberExpression()) { - loweredDep = lowerJsxMemberExpression(builder, dependency); - } else { - loweredDep = lowerExpressionToTemporary(builder, dependency); - } - capturedRefs.add(loweredDep); - seenPaths.add(exprKey); + if (binding !== undefined && pureScopes.has(binding.scope)) { + capturedIds.add(binding.identifier); } } @@ -4305,13 +4175,13 @@ function gatherCapturedDeps( return; } else if (path.isJSXElement()) { handleMaybeDependency(path.get('openingElement')); - } else if (path.isMemberExpression() || path.isIdentifier()) { + } else if (path.isIdentifier()) { handleMaybeDependency(path); } }, }); - return {identifiers: [...capturedIds.keys()], refs: [...capturedRefs]}; + return [...capturedIds.keys()]; } function notNull(value: T | null): value is T { 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 d3c919a6d8afe..a422570fffead 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts @@ -131,15 +131,7 @@ function collectHoistablePropertyLoadsImpl( fn: HIRFunction, context: CollectHoistablePropertyLoadsContext, ): ReadonlyMap { - const functionExpressionLoads = collectFunctionExpressionFakeLoads(fn); - const actuallyEvaluatedTemporaries = new Map( - [...context.temporaries].filter(([id]) => !functionExpressionLoads.has(id)), - ); - - const nodes = collectNonNullsInBlocks(fn, { - ...context, - temporaries: actuallyEvaluatedTemporaries, - }); + const nodes = collectNonNullsInBlocks(fn, context); propagateNonNull(fn, nodes, context.registry); if (DEBUG_PRINT) { @@ -598,30 +590,3 @@ function reduceMaybeOptionalChains( } } while (changed); } - -function collectFunctionExpressionFakeLoads( - fn: HIRFunction, -): Set { - const sources = new Map(); - const functionExpressionReferences = new Set(); - - for (const [_, block] of fn.body.blocks) { - for (const {lvalue, value} of block.instructions) { - if ( - value.kind === 'FunctionExpression' || - value.kind === 'ObjectMethod' - ) { - 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/Environment.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts index 113af2025dd2b..8de9218a662e2 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts @@ -245,8 +245,6 @@ const EnvironmentConfigSchema = z.object({ */ enableUseTypeAnnotations: z.boolean().default(false), - enableFunctionDependencyRewrite: z.boolean().default(true), - /** * Enables inference of optional dependency chains. Without this flag * a property chain such as `props?.items?.foo` will infer as a dep on 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 a6a9cbcd48ec5..4df6da6c1704c 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts @@ -722,7 +722,6 @@ export type ObjectProperty = { }; export type LoweredFunction = { - dependencies: Array; func: HIRFunction; }; diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/MergeOverlappingReactiveScopesHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/MergeOverlappingReactiveScopesHIR.ts index a3740539b295b..5b286e917d0d3 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/MergeOverlappingReactiveScopesHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/MergeOverlappingReactiveScopesHIR.ts @@ -148,6 +148,14 @@ function collectScopeInfo(fn: HIRFunction): ScopeInfo { const scope = place.identifier.scope; if (scope != null) { placeScopes.set(place, scope); + /** + * Record both mutating and non-mutating scopes to merge scopes with + * still-mutating values with inner scopes that alias those values + * (see `nonmutating-capture-in-unsplittable-memo-block`) + * + * Note that this isn't perfect, as it also leads to merging of mutating + * scopes with JSX single-instruction scopes (see `mutation-within-jsx`) + */ if (scope.range.start !== scope.range.end) { getOrInsertDefault(scopeStarts, scope.range.start, new Set()).add( scope, @@ -254,7 +262,7 @@ function visitPlace( * of the stack to the mutated outer scope. */ const placeScope = getPlaceScope(id, place); - if (placeScope != null && isMutable({id} as any, place)) { + if (placeScope != null && isMutable({id}, place)) { const placeScopeIdx = activeScopes.indexOf(placeScope); if (placeScopeIdx !== -1 && placeScopeIdx !== activeScopes.length - 1) { joined.union([placeScope, ...activeScopes.slice(placeScopeIdx + 1)]); @@ -275,6 +283,13 @@ function getOverlappingReactiveScopes( for (const instr of block.instructions) { visitInstructionId(instr.id, context, state); for (const place of eachInstructionOperand(instr)) { + if ( + (instr.value.kind === 'FunctionExpression' || + instr.value.kind === 'ObjectMethod') && + place.identifier.type.kind === 'Primitive' + ) { + continue; + } visitPlace(instr.id, place, state); } for (const place of eachInstructionLValue(instr)) { 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 a6f6c606e118d..7a6586730f9c1 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts @@ -538,9 +538,6 @@ export function printInstructionValue(instrValue: ReactiveValue): string { .split('\n') .map(line => ` ${line}`) .join('\n'); - const deps = instrValue.loweredFunc.dependencies - .map(dep => printPlace(dep)) - .join(','); const context = instrValue.loweredFunc.func.context .map(dep => printPlace(dep)) .join(','); @@ -557,7 +554,7 @@ export function printInstructionValue(instrValue: ReactiveValue): string { }) .join(', ') ?? ''; const type = printType(instrValue.loweredFunc.func.returnType).trim(); - value = `${kind} ${name} @deps[${deps}] @context[${context}] @effects[${effects}]${type !== '' ? ` return${type}` : ''}:\n${fn}`; + value = `${kind} ${name} @context[${context}] @effects[${effects}]${type !== '' ? ` return${type}` : ''}:\n${fn}`; break; } case 'TaggedTemplateExpression': { 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 08856e9143139..4cb84870a8a33 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDependenciesHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDependenciesHIR.ts @@ -738,9 +738,8 @@ function collectDependencies( } for (const instr of block.instructions) { if ( - fn.env.config.enableFunctionDependencyRewrite && - (instr.value.kind === 'FunctionExpression' || - instr.value.kind === 'ObjectMethod') + instr.value.kind === 'FunctionExpression' || + instr.value.kind === 'ObjectMethod' ) { context.declare(instr.lvalue.identifier, { id: instr.id, diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/visitors.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/visitors.ts index c9ee803bfaffd..49ff3c256e016 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/visitors.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/visitors.ts @@ -193,7 +193,7 @@ export function* eachInstructionValueOperand( } case 'ObjectMethod': case 'FunctionExpression': { - yield* instrValue.loweredFunc.dependencies; + yield* instrValue.loweredFunc.func.context; break; } case 'TaggedTemplateExpression': { @@ -517,8 +517,9 @@ export function mapInstructionValueOperands( } case 'ObjectMethod': case 'FunctionExpression': { - instrValue.loweredFunc.dependencies = - instrValue.loweredFunc.dependencies.map(d => fn(d)); + instrValue.loweredFunc.func.context = + instrValue.loweredFunc.func.context.map(d => fn(d)); + break; } case 'TaggedTemplateExpression': { 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 75bd8f6811ffb..5381262874369 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts @@ -10,9 +10,8 @@ import { Effect, HIRFunction, Identifier, - IdentifierName, + IdentifierId, LoweredFunction, - Place, isRefOrRefValue, makeInstructionId, } from '../HIR'; @@ -23,78 +22,39 @@ import {inferMutableContextVariables} from './InferMutableContextVariables'; import {inferMutableRanges} from './InferMutableRanges'; import inferReferenceEffects from './InferReferenceEffects'; -type Dependency = { - identifier: Identifier; - path: Array; -}; - // Helper class to track indirections such as LoadLocal and PropertyLoad. export class IdentifierState { - properties: Map = new Map(); + properties: Map = new Map(); resolve(identifier: Identifier): Identifier { - const resolved = this.properties.get(identifier); + const resolved = this.properties.get(identifier.id); if (resolved !== undefined) { - return resolved.identifier; + return resolved; } return identifier; } - declareProperty(lvalue: Place, object: Place, property: string): void { - const objectDependency = this.properties.get(object.identifier); - let nextDependency: Dependency; - if (objectDependency === undefined) { - nextDependency = {identifier: object.identifier, path: [property]}; - } else { - nextDependency = { - identifier: objectDependency.identifier, - path: [...objectDependency.path, property], - }; - } - this.properties.set(lvalue.identifier, nextDependency); - } - - declareTemporary(lvalue: Place, value: Place): void { - const resolved: Dependency = this.properties.get(value.identifier) ?? { - identifier: value.identifier, - path: [], - }; - this.properties.set(lvalue.identifier, resolved); + alias(lvalue: Identifier, value: Identifier): void { + this.properties.set(lvalue.id, this.properties.get(value.id) ?? value); } } export default function analyseFunctions(func: HIRFunction): void { - const state = new IdentifierState(); - for (const [_, block] of func.body.blocks) { for (const instr of block.instructions) { switch (instr.value.kind) { case 'ObjectMethod': case 'FunctionExpression': { lower(instr.value.loweredFunc.func); - infer(instr.value.loweredFunc, state, func.context); - break; - } - case 'PropertyLoad': { - state.declareProperty( - instr.lvalue, - instr.value.object, - instr.value.property, - ); - break; - } - case 'ComputedLoad': { - /* - * The path is set to an empty string as the path doesn't really - * matter for a computed load. + infer(instr.value.loweredFunc); + + /** + * Reset mutable range for outer inferReferenceEffects */ - state.declareProperty(instr.lvalue, instr.value.object, ''); - break; - } - case 'LoadLocal': - case 'LoadContext': { - if (instr.lvalue.identifier.name === null) { - state.declareTemporary(instr.lvalue, instr.value.place); + for (const operand of instr.value.loweredFunc.func.context) { + operand.identifier.mutableRange.start = makeInstructionId(0); + operand.identifier.mutableRange.end = makeInstructionId(0); + operand.identifier.scope = null; } break; } @@ -110,7 +70,6 @@ function lower(func: HIRFunction): void { inferMutableRanges(func); rewriteInstructionKindsBasedOnReassignment(func); inferReactiveScopeVariables(func); - inferMutableContextVariables(func); func.env.logger?.debugLogIRs?.({ kind: 'hir', name: 'AnalyseFunction (inner)', @@ -118,32 +77,16 @@ function lower(func: HIRFunction): void { }); } -function infer( - loweredFunc: LoweredFunction, - state: IdentifierState, - context: Array, -): void { - const mutations = new Map(); +function infer(loweredFunc: LoweredFunction): void { + const knownMutated = inferMutableContextVariables(loweredFunc.func); for (const operand of loweredFunc.func.context) { - if ( - isMutatedOrReassigned(operand.identifier) && - operand.identifier.name !== null - ) { - mutations.set(operand.identifier.name.value, operand.effect); - } - } - - for (const dep of loweredFunc.dependencies) { - let name: IdentifierName | null = null; - - if (state.properties.has(dep.identifier)) { - const receiver = state.properties.get(dep.identifier)!; - name = receiver.identifier.name; - } else { - name = dep.identifier.name; - } - - if (isRefOrRefValue(dep.identifier)) { + const identifier = operand.identifier; + CompilerError.invariant(operand.effect === Effect.Unknown, { + reason: + '[AnalyseFunctions] Expected Function context effects to not have been set', + loc: operand.loc, + }); + if (isRefOrRefValue(identifier)) { /* * TODO: this is a hack to ensure we treat functions which reference refs * as having a capture and therefore being considered mutable. this ensures @@ -151,43 +94,16 @@ function infer( * could be called, and allows us to help ensure it isn't called during * render */ - dep.effect = Effect.Capture; - } else if (name !== null) { - const effect = mutations.get(name.value); - if (effect !== undefined) { - dep.effect = effect === Effect.Unknown ? Effect.Capture : effect; - } - } - } - - /* - * This could potentially add duplicate deps to mutatedDeps in the case of - * mutating a context ref in the child function and in this parent function. - * It might be useful to dedupe this. - * - * In practice this never really matters because the Component function has no - * context refs, so it will never have duplicate deps. - */ - for (const place of context) { - CompilerError.invariant(place.identifier.name !== null, { - reason: 'context refs should always have a name', - description: null, - loc: place.loc, - suggestions: null, - }); - - const effect = mutations.get(place.identifier.name.value); - if (effect !== undefined) { - place.effect = effect === Effect.Unknown ? Effect.Capture : effect; - loweredFunc.dependencies.push(place); + 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 + operand.effect = Effect.Capture; + } else { + operand.effect = Effect.Read; } } - - for (const operand of loweredFunc.func.context) { - operand.identifier.mutableRange.start = makeInstructionId(0); - operand.identifier.mutableRange.end = makeInstructionId(0); - operand.identifier.scope = null; - } } function isMutatedOrReassigned(id: Identifier): boolean { diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutableContextVariables.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutableContextVariables.ts index 67babf43db22f..0025472721542 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutableContextVariables.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutableContextVariables.ts @@ -55,32 +55,21 @@ import {IdentifierState} from './AnalyseFunctions'; * fn(); * ``` */ -export function inferMutableContextVariables(fn: HIRFunction): void { +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': { - state.declareProperty( - instr.lvalue, - instr.value.object, - instr.value.property, - ); - break; - } + case 'PropertyLoad': case 'ComputedLoad': { - /* - * The path is set to an empty string as the path doesn't really - * matter for a computed load. - */ - state.declareProperty(instr.lvalue, instr.value.object, ''); + state.alias(instr.lvalue.identifier, instr.value.object.identifier); break; } case 'LoadLocal': case 'LoadContext': { if (instr.lvalue.identifier.name === null) { - state.declareTemporary(instr.lvalue, instr.value.place); + state.alias(instr.lvalue.identifier, instr.value.place.identifier); } break; } @@ -95,11 +84,13 @@ export function inferMutableContextVariables(fn: HIRFunction): void { visitOperand(state, knownMutatedIdentifiers, operand); } } + const results = new Set(); for (const operand of fn.context) { if (knownMutatedIdentifiers.has(operand.identifier)) { - operand.effect = Effect.Mutate; + results.add(operand); } } + return results; } function visitOperand( diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts index 0afe479df08dd..6420e74a2fd20 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts @@ -390,29 +390,31 @@ class InferenceState { freezeValues(values: Set, reason: Set): void { for (const value of values) { + if (value.kind === 'DeclareContext') { + /** + * Avoid freezing hoisted context declarations + * function Component() { + * const cb = useBar(() => foo(2)); // produces a hoisted context declaration + * const foo = useFoo(); // reassigns to the context variable + * return ; + * } + */ + continue; + } this.#values.set(value, { kind: ValueKind.Frozen, reason, context: new Set(), }); - if (value.kind === 'FunctionExpression') { - if ( - this.#env.config.enablePreserveExistingMemoizationGuarantees || - this.#env.config.enableTransitivelyFreezeFunctionExpressions - ) { - if (value.kind === 'FunctionExpression') { - /* - * We want to freeze the captured values, not mark the operands - * themselves as frozen. There could be mutations that occur - * before the freeze we are processing, and it would be invalid - * to overwrite those mutations as a freeze. - */ - for (const operand of eachInstructionValueOperand(value)) { - const operandValues = this.#variables.get(operand.identifier.id); - if (operandValues !== undefined) { - this.freezeValues(operandValues, reason); - } - } + if ( + value.kind === 'FunctionExpression' && + (this.#env.config.enablePreserveExistingMemoizationGuarantees || + this.#env.config.enableTransitivelyFreezeFunctionExpressions) + ) { + for (const operand of value.loweredFunc.func.context) { + const operandValues = this.#variables.get(operand.identifier.id); + if (operandValues !== undefined) { + this.freezeValues(operandValues, reason); } } } @@ -1143,17 +1145,17 @@ function inferBlock( case 'ObjectMethod': case 'FunctionExpression': { let hasMutableOperand = false; - const mutableOperands: Array = []; for (const operand of eachInstructionOperand(instr)) { + CompilerError.invariant(operand.effect !== Effect.Unknown, { + reason: 'Expected fn effects to be populated', + loc: operand.loc, + }); state.referenceAndRecordEffects( freezeActions, operand, - operand.effect === Effect.Unknown ? Effect.Read : operand.effect, + operand.effect, ValueReason.Other, ); - if (isMutableEffect(operand.effect, operand.loc)) { - mutableOperands.push(operand); - } hasMutableOperand ||= isMutableEffect(operand.effect, operand.loc); } /* diff --git a/compiler/packages/babel-plugin-react-compiler/src/Optimization/LowerContextAccess.ts b/compiler/packages/babel-plugin-react-compiler/src/Optimization/LowerContextAccess.ts index e27b8f952148a..5b700b23b4049 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Optimization/LowerContextAccess.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Optimization/LowerContextAccess.ts @@ -270,7 +270,6 @@ function emitSelectorFn(env: Environment, keys: Array): Instruction { name: null, loweredFunc: { func: fn, - dependencies: [], }, type: 'ArrowFunctionExpression', loc: GeneratedSource, diff --git a/compiler/packages/babel-plugin-react-compiler/src/Optimization/OutlineFunctions.ts b/compiler/packages/babel-plugin-react-compiler/src/Optimization/OutlineFunctions.ts index 7a1473be40c87..0e6d1fd59201f 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Optimization/OutlineFunctions.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Optimization/OutlineFunctions.ts @@ -24,7 +24,6 @@ export function outlineFunctions( } if ( value.kind === 'FunctionExpression' && - value.loweredFunc.dependencies.length === 0 && value.loweredFunc.func.context.length === 0 && // TODO: handle outlining named functions value.loweredFunc.func.id === null && diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts index 1108422f070d7..1f104d8592fab 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts @@ -379,6 +379,14 @@ export function findDisjointMutableValues( */ operand.identifier.mutableRange.start > 0 ) { + if ( + instr.value.kind === 'FunctionExpression' || + instr.value.kind === 'ObjectMethod' + ) { + if (operand.identifier.type.kind === 'Primitive') { + continue; + } + } operands.push(operand.identifier); } } diff --git a/compiler/packages/babel-plugin-react-compiler/src/SSA/EliminateRedundantPhi.ts b/compiler/packages/babel-plugin-react-compiler/src/SSA/EliminateRedundantPhi.ts index bae038f9bd9df..12c8c0e2e6221 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/SSA/EliminateRedundantPhi.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/SSA/EliminateRedundantPhi.ts @@ -13,6 +13,8 @@ import { eachTerminalOperand, } from '../HIR/visitors'; +const DEBUG = false; + /* * Pass to eliminate redundant phi nodes: * - all operands are the same identifier, ie `x2 = phi(x1, x1, x1)`. @@ -141,6 +143,23 @@ export function eliminateRedundantPhi( * have already propagated forwards since we visit in reverse postorder. */ } while (rewrites.size > size && hasBackEdge); + + if (DEBUG) { + for (const [, block] of ir.blocks) { + for (const phi of block.phis) { + CompilerError.invariant(!rewrites.has(phi.place.identifier), { + reason: '[EliminateRedundantPhis]: rewrite not complete', + loc: phi.place.loc, + }); + for (const [, operand] of phi.operands) { + CompilerError.invariant(!rewrites.has(operand.identifier), { + reason: '[EliminateRedundantPhis]: rewrite not complete', + loc: phi.place.loc, + }); + } + } + } + } } function rewritePlace( diff --git a/compiler/packages/babel-plugin-react-compiler/src/SSA/EnterSSA.ts b/compiler/packages/babel-plugin-react-compiler/src/SSA/EnterSSA.ts index caba0d3c36992..820f7388dc41b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/SSA/EnterSSA.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/SSA/EnterSSA.ts @@ -301,9 +301,6 @@ function enterSSAImpl( entry.preds.add(blockId); builder.defineFunction(loweredFunc); builder.enter(() => { - loweredFunc.context = loweredFunc.context.map(p => - builder.getPlace(p), - ); loweredFunc.params = loweredFunc.params.map(param => { if (param.kind === 'Identifier') { return builder.definePlace(param); diff --git a/compiler/packages/babel-plugin-react-compiler/src/Transform/TransformFire.ts b/compiler/packages/babel-plugin-react-compiler/src/Transform/TransformFire.ts index a35c4ddb0182c..a480b5d7c78de 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Transform/TransformFire.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Transform/TransformFire.ts @@ -319,51 +319,6 @@ function visitFunctionExpressionAndPropagateFireDependencies( replaceFireFunctions(fnExpr.loweredFunc.func, context), ); - /* - * Make a mapping from each dependency to the corresponding LoadLocal for it so that - * we can replace the loaded place with the generated fire function binding - */ - const loadLocalsToDepLoads = new Map(); - for (const dep of fnExpr.loweredFunc.dependencies) { - const loadLocal = context.getLoadLocalInstr(dep.identifier.id); - if (loadLocal != null) { - loadLocalsToDepLoads.set(loadLocal.place.identifier.id, loadLocal); - } - } - - const replacedCallees = new Map(); - for (const [ - calleeIdentifierId, - loadedFireFunctionBindingPlace, - ] of calleesCapturedByFnExpression.entries()) { - /* - * Given the ids of captured fire callees, look at the deps for loads of those identifiers - * and replace them with the new fire function binding - */ - const loadLocal = loadLocalsToDepLoads.get(calleeIdentifierId); - if (loadLocal == null) { - context.pushError({ - loc: fnExpr.loc, - description: null, - severity: ErrorSeverity.Invariant, - reason: - '[InsertFire] No loadLocal found for fire call argument for lambda', - suggestions: null, - }); - continue; - } - - const oldPlaceId = loadLocal.place.identifier.id; - loadLocal.place = { - ...loadedFireFunctionBindingPlace.fireFunctionBinding, - }; - - replacedCallees.set( - oldPlaceId, - loadedFireFunctionBindingPlace.fireFunctionBinding, - ); - } - // For each replaced callee, update the context of the function expression to track it for ( let contextIdx = 0; @@ -371,9 +326,13 @@ function visitFunctionExpressionAndPropagateFireDependencies( contextIdx++ ) { const contextItem = fnExpr.loweredFunc.func.context[contextIdx]; - const replacedCallee = replacedCallees.get(contextItem.identifier.id); + const replacedCallee = calleesCapturedByFnExpression.get( + contextItem.identifier.id, + ); if (replacedCallee != null) { - fnExpr.loweredFunc.func.context[contextIdx] = replacedCallee; + fnExpr.loweredFunc.func.context[contextIdx] = { + ...replacedCallee.fireFunctionBinding, + }; } } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/aliased-nested-scope-fn-expr.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/aliased-nested-scope-fn-expr.expect.md new file mode 100644 index 0000000000000..b30ee7e0d6817 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/aliased-nested-scope-fn-expr.expect.md @@ -0,0 +1,120 @@ + +## Input + +```javascript +// @enableTransitivelyFreezeFunctionExpressions:false +import { + Stringify, + mutate, + identity, + setPropertyByKey, + shallowCopy, +} from 'shared-runtime'; +/** + * Function expression version of `aliased-nested-scope-truncated-dep`. + * + * In this fixture, the output would be invalid if propagateScopeDeps did not + * avoid adding MemberExpression dependencies which would other evaluate during + * the mutable ranges of their base objects. + * This is different from `aliased-nested-scope-truncated-dep` which *does* + * produce correct output regardless of MemberExpression dependency truncation. + * + * Note while other expressions evaluate inline, function expressions *always* + * represent deferred evaluation. This means that + * (1) it's always safe to reorder function expression creation until its + * earliest potential invocation + * (2) it's invalid to eagerly evaluate function expression dependencies during + * their respective mutable ranges. + */ + +function Component({prop}) { + let obj = shallowCopy(prop); + + const aliasedObj = identity(obj); + + // When `obj` is mutable (either directly or through aliases), taking a + // dependency on `obj.id` is invalid as it may change before getId() is invoked + const getId = () => obj.id; + + mutate(aliasedObj); + setPropertyByKey(aliasedObj, 'id', prop.id + 1); + + // Calling getId() should return prop.id + 1, not the prev + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{prop: {id: 1}}], + sequentialRenders: [{prop: {id: 1}}, {prop: {id: 1}}, {prop: {id: 2}}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @enableTransitivelyFreezeFunctionExpressions:false +import { + Stringify, + mutate, + identity, + setPropertyByKey, + shallowCopy, +} from "shared-runtime"; +/** + * Function expression version of `aliased-nested-scope-truncated-dep`. + * + * In this fixture, the output would be invalid if propagateScopeDeps did not + * avoid adding MemberExpression dependencies which would other evaluate during + * the mutable ranges of their base objects. + * This is different from `aliased-nested-scope-truncated-dep` which *does* + * produce correct output regardless of MemberExpression dependency truncation. + * + * Note while other expressions evaluate inline, function expressions *always* + * represent deferred evaluation. This means that + * (1) it's always safe to reorder function expression creation until its + * earliest potential invocation + * (2) it's invalid to eagerly evaluate function expression dependencies during + * their respective mutable ranges. + */ + +function Component(t0) { + const $ = _c(2); + const { prop } = t0; + let t1; + if ($[0] !== prop) { + const obj = shallowCopy(prop); + + const aliasedObj = identity(obj); + + const getId = () => obj.id; + + mutate(aliasedObj); + setPropertyByKey(aliasedObj, "id", prop.id + 1); + + t1 = ; + $[0] = prop; + $[1] = t1; + } else { + t1 = $[1]; + } + return t1; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ prop: { id: 1 } }], + sequentialRenders: [ + { prop: { id: 1 } }, + { prop: { id: 1 } }, + { prop: { id: 2 } }, + ], +}; + +``` + +### Eval output +(kind: ok)
{"getId":{"kind":"Function","result":2},"shouldInvokeFns":true}
+
{"getId":{"kind":"Function","result":2},"shouldInvokeFns":true}
+
{"getId":{"kind":"Function","result":3},"shouldInvokeFns":true}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/aliased-nested-scope-fn-expr.tsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/aliased-nested-scope-fn-expr.tsx new file mode 100644 index 0000000000000..40022c6f651fd --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/aliased-nested-scope-fn-expr.tsx @@ -0,0 +1,46 @@ +// @enableTransitivelyFreezeFunctionExpressions:false +import { + Stringify, + mutate, + identity, + setPropertyByKey, + shallowCopy, +} from 'shared-runtime'; +/** + * Function expression version of `aliased-nested-scope-truncated-dep`. + * + * In this fixture, the output would be invalid if propagateScopeDeps did not + * avoid adding MemberExpression dependencies which would other evaluate during + * the mutable ranges of their base objects. + * This is different from `aliased-nested-scope-truncated-dep` which *does* + * produce correct output regardless of MemberExpression dependency truncation. + * + * Note while other expressions evaluate inline, function expressions *always* + * represent deferred evaluation. This means that + * (1) it's always safe to reorder function expression creation until its + * earliest potential invocation + * (2) it's invalid to eagerly evaluate function expression dependencies during + * their respective mutable ranges. + */ + +function Component({prop}) { + let obj = shallowCopy(prop); + + const aliasedObj = identity(obj); + + // When `obj` is mutable (either directly or through aliases), taking a + // dependency on `obj.id` is invalid as it may change before getId() is invoked + const getId = () => obj.id; + + mutate(aliasedObj); + setPropertyByKey(aliasedObj, 'id', prop.id + 1); + + // Calling getId() should return prop.id + 1, not the prev + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{prop: {id: 1}}], + sequentialRenders: [{prop: {id: 1}}, {prop: {id: 1}}, {prop: {id: 2}}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/aliased-nested-scope-truncated-dep.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/aliased-nested-scope-truncated-dep.expect.md new file mode 100644 index 0000000000000..933fafff5f1ba --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/aliased-nested-scope-truncated-dep.expect.md @@ -0,0 +1,221 @@ + +## Input + +```javascript +import { + Stringify, + mutate, + identity, + shallowCopy, + setPropertyByKey, +} from 'shared-runtime'; + +/** + * This fixture is similar to `bug-aliased-capture-aliased-mutate` and + * `nonmutating-capture-in-unsplittable-memo-block`, but with a focus on + * dependency extraction. + * + * NOTE: this fixture is currently valid, but will break with optimizations: + * - Scope and mutable-range based reordering may move the array creation + * *after* the `mutate(aliasedObj)` call. This is invalid if mutate + * reassigns inner properties. + * - RecycleInto or other deeper-equality optimizations may produce invalid + * output -- it may compare the array's contents / dependencies too early. + * - Runtime validation for immutable values will break if `mutate` does + * interior mutation of the value captured into the array. + * + * Before scope block creation, HIR looks like this: + * // + * // $1 is unscoped as obj's mutable range will be + * // extended in a later pass + * // + * $1 = LoadLocal obj@0[0:12] + * $2 = PropertyLoad $1.id + * // + * // $3 gets assigned a scope as Array is an allocating + * // instruction, but this does *not* get extended or + * // merged into the later mutation site. + * // (explained in `bug-aliased-capture-aliased-mutate`) + * // + * $3@1 = Array[$2] + * ... + * $10@0 = LoadLocal shallowCopy@0[0, 12] + * $11 = LoadGlobal mutate + * $12 = $11($10@0[0, 12]) + * + * When filling in scope dependencies, we find that it's incorrect to depend on + * PropertyLoads from obj as it hasn't completed its mutable range. Following + * the immutable / mutable-new typing system, we check the identity of obj to + * detect whether it was newly created (and thus mutable) in this render pass. + * + * HIR with scopes looks like this. + * bb0: + * $1 = LoadLocal obj@0[0:12] + * $2 = PropertyLoad $1.id + * scopeTerminal deps=[obj@0] block=bb1 fallt=bb2 + * bb1: + * $3@1 = Array[$2] + * goto bb2 + * bb2: + * ... + * + * This is surprising as deps now is entirely decoupled from temporaries used + * by the block itself. scope @1's instructions now reference a value (1) + * produced outside its scope range and (2) not represented in its dependencies + * + * The right thing to do is to ensure that all Loads from a value get assigned + * the value's reactive scope. This also requires track mutating and aliasing + * separately from scope range. In this example, that would correctly merge + * the scopes of $3 with obj. + * Runtime validation and optimizations such as ReactiveGraph-based reordering + * require this as well. + * + * A tempting fix is to instead extend $3's ReactiveScope range up to include + * $2 (the PropertyLoad). This fixes dependency deduping but not reordering + * and mutability. + */ +function Component({prop}) { + let obj = shallowCopy(prop); + const aliasedObj = identity(obj); + + // [obj.id] currently is assigned its own reactive scope + const id = [obj.id]; + + // Writing to the alias may reassign to previously captured references. + // The compiler currently produces valid output, but this breaks with + // reordering, recycleInto, and other potential optimizations. + mutate(aliasedObj); + setPropertyByKey(aliasedObj, 'id', prop.id + 1); + + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{prop: {id: 1}}], + sequentialRenders: [{prop: {id: 1}}, {prop: {id: 1}}, {prop: {id: 2}}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { + Stringify, + mutate, + identity, + shallowCopy, + setPropertyByKey, +} from "shared-runtime"; + +/** + * This fixture is similar to `bug-aliased-capture-aliased-mutate` and + * `nonmutating-capture-in-unsplittable-memo-block`, but with a focus on + * dependency extraction. + * + * NOTE: this fixture is currently valid, but will break with optimizations: + * - Scope and mutable-range based reordering may move the array creation + * *after* the `mutate(aliasedObj)` call. This is invalid if mutate + * reassigns inner properties. + * - RecycleInto or other deeper-equality optimizations may produce invalid + * output -- it may compare the array's contents / dependencies too early. + * - Runtime validation for immutable values will break if `mutate` does + * interior mutation of the value captured into the array. + * + * Before scope block creation, HIR looks like this: + * // + * // $1 is unscoped as obj's mutable range will be + * // extended in a later pass + * // + * $1 = LoadLocal obj@0[0:12] + * $2 = PropertyLoad $1.id + * // + * // $3 gets assigned a scope as Array is an allocating + * // instruction, but this does *not* get extended or + * // merged into the later mutation site. + * // (explained in `bug-aliased-capture-aliased-mutate`) + * // + * $3@1 = Array[$2] + * ... + * $10@0 = LoadLocal shallowCopy@0[0, 12] + * $11 = LoadGlobal mutate + * $12 = $11($10@0[0, 12]) + * + * When filling in scope dependencies, we find that it's incorrect to depend on + * PropertyLoads from obj as it hasn't completed its mutable range. Following + * the immutable / mutable-new typing system, we check the identity of obj to + * detect whether it was newly created (and thus mutable) in this render pass. + * + * HIR with scopes looks like this. + * bb0: + * $1 = LoadLocal obj@0[0:12] + * $2 = PropertyLoad $1.id + * scopeTerminal deps=[obj@0] block=bb1 fallt=bb2 + * bb1: + * $3@1 = Array[$2] + * goto bb2 + * bb2: + * ... + * + * This is surprising as deps now is entirely decoupled from temporaries used + * by the block itself. scope @1's instructions now reference a value (1) + * produced outside its scope range and (2) not represented in its dependencies + * + * The right thing to do is to ensure that all Loads from a value get assigned + * the value's reactive scope. This also requires track mutating and aliasing + * separately from scope range. In this example, that would correctly merge + * the scopes of $3 with obj. + * Runtime validation and optimizations such as ReactiveGraph-based reordering + * require this as well. + * + * A tempting fix is to instead extend $3's ReactiveScope range up to include + * $2 (the PropertyLoad). This fixes dependency deduping but not reordering + * and mutability. + */ +function Component(t0) { + const $ = _c(4); + const { prop } = t0; + let t1; + if ($[0] !== prop) { + const obj = shallowCopy(prop); + const aliasedObj = identity(obj); + let t2; + if ($[2] !== obj) { + t2 = [obj.id]; + $[2] = obj; + $[3] = t2; + } else { + t2 = $[3]; + } + const id = t2; + + mutate(aliasedObj); + setPropertyByKey(aliasedObj, "id", prop.id + 1); + + t1 = ; + $[0] = prop; + $[1] = t1; + } else { + t1 = $[1]; + } + return t1; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ prop: { id: 1 } }], + sequentialRenders: [ + { prop: { id: 1 } }, + { prop: { id: 1 } }, + { prop: { id: 2 } }, + ], +}; + +``` + +### Eval output +(kind: ok)
{"id":[1]}
+
{"id":[1]}
+
{"id":[2]}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/aliased-nested-scope-truncated-dep.tsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/aliased-nested-scope-truncated-dep.tsx new file mode 100644 index 0000000000000..4d9d7e78fb309 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/aliased-nested-scope-truncated-dep.tsx @@ -0,0 +1,93 @@ +import { + Stringify, + mutate, + identity, + shallowCopy, + setPropertyByKey, +} from 'shared-runtime'; + +/** + * This fixture is similar to `bug-aliased-capture-aliased-mutate` and + * `nonmutating-capture-in-unsplittable-memo-block`, but with a focus on + * dependency extraction. + * + * NOTE: this fixture is currently valid, but will break with optimizations: + * - Scope and mutable-range based reordering may move the array creation + * *after* the `mutate(aliasedObj)` call. This is invalid if mutate + * reassigns inner properties. + * - RecycleInto or other deeper-equality optimizations may produce invalid + * output -- it may compare the array's contents / dependencies too early. + * - Runtime validation for immutable values will break if `mutate` does + * interior mutation of the value captured into the array. + * + * Before scope block creation, HIR looks like this: + * // + * // $1 is unscoped as obj's mutable range will be + * // extended in a later pass + * // + * $1 = LoadLocal obj@0[0:12] + * $2 = PropertyLoad $1.id + * // + * // $3 gets assigned a scope as Array is an allocating + * // instruction, but this does *not* get extended or + * // merged into the later mutation site. + * // (explained in `bug-aliased-capture-aliased-mutate`) + * // + * $3@1 = Array[$2] + * ... + * $10@0 = LoadLocal shallowCopy@0[0, 12] + * $11 = LoadGlobal mutate + * $12 = $11($10@0[0, 12]) + * + * When filling in scope dependencies, we find that it's incorrect to depend on + * PropertyLoads from obj as it hasn't completed its mutable range. Following + * the immutable / mutable-new typing system, we check the identity of obj to + * detect whether it was newly created (and thus mutable) in this render pass. + * + * HIR with scopes looks like this. + * bb0: + * $1 = LoadLocal obj@0[0:12] + * $2 = PropertyLoad $1.id + * scopeTerminal deps=[obj@0] block=bb1 fallt=bb2 + * bb1: + * $3@1 = Array[$2] + * goto bb2 + * bb2: + * ... + * + * This is surprising as deps now is entirely decoupled from temporaries used + * by the block itself. scope @1's instructions now reference a value (1) + * produced outside its scope range and (2) not represented in its dependencies + * + * The right thing to do is to ensure that all Loads from a value get assigned + * the value's reactive scope. This also requires track mutating and aliasing + * separately from scope range. In this example, that would correctly merge + * the scopes of $3 with obj. + * Runtime validation and optimizations such as ReactiveGraph-based reordering + * require this as well. + * + * A tempting fix is to instead extend $3's ReactiveScope range up to include + * $2 (the PropertyLoad). This fixes dependency deduping but not reordering + * and mutability. + */ +function Component({prop}) { + let obj = shallowCopy(prop); + const aliasedObj = identity(obj); + + // [obj.id] currently is assigned its own reactive scope + const id = [obj.id]; + + // Writing to the alias may reassign to previously captured references. + // The compiler currently produces valid output, but this breaks with + // reordering, recycleInto, and other potential optimizations. + mutate(aliasedObj); + setPropertyByKey(aliasedObj, 'id', prop.id + 1); + + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{prop: {id: 1}}], + sequentialRenders: [{prop: {id: 1}}, {prop: {id: 1}}, {prop: {id: 2}}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-ref-access-in-unused-callback-nested.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-ref-access-in-unused-callback-nested.expect.md index 37a510b8c290a..3584faf699f86 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-ref-access-in-unused-callback-nested.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-ref-access-in-unused-callback-nested.expect.md @@ -44,48 +44,44 @@ import { c as _c } from "react/compiler-runtime"; // @validateRefAccessDuringRen import { useEffect, useRef, useState } from "react"; function Component() { - const $ = _c(6); + const $ = _c(5); const ref = useRef(null); const [state, setState] = useState(false); let t0; - let t1; if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - t0 = () => {}; - - t1 = []; + t0 = []; $[0] = t0; - $[1] = t1; } else { t0 = $[0]; - t1 = $[1]; } - useEffect(t0, t1); + useEffect(_temp, t0); + let t1; let t2; - let t3; - if ($[2] === Symbol.for("react.memo_cache_sentinel")) { - t2 = () => { + if ($[1] === Symbol.for("react.memo_cache_sentinel")) { + t1 = () => { setState(true); }; - t3 = []; + t2 = []; + $[1] = t1; $[2] = t2; - $[3] = t3; } else { + t1 = $[1]; t2 = $[2]; - t3 = $[3]; } - useEffect(t2, t3); + useEffect(t1, t2); - const t4 = String(state); - let t5; - if ($[4] !== t4) { - t5 = ; + const t3 = String(state); + let t4; + if ($[3] !== t3) { + t4 = ; + $[3] = t3; $[4] = t4; - $[5] = t5; } else { - t5 = $[5]; + t4 = $[4]; } - return t5; + return t4; } +function _temp() {} function Child(t0) { const { ref } = t0; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capturing-func-mutate-2.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capturing-func-mutate-2.expect.md index c071d5d20ed99..6836544c5d337 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capturing-func-mutate-2.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capturing-func-mutate-2.expect.md @@ -27,7 +27,6 @@ export const FIXTURE_ENTRYPOINT = { import { c as _c } from "react/compiler-runtime"; function component(a, b) { const $ = _c(2); - const y = { b }; let z; if ($[0] !== a) { z = { a }; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capturing-func-no-mutate.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capturing-func-no-mutate.expect.md index aa32b3260ef50..14bf94e770a6b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capturing-func-no-mutate.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capturing-func-no-mutate.expect.md @@ -31,12 +31,20 @@ export const FIXTURE_ENTRYPOINT = { ```javascript import { c as _c } from "react/compiler-runtime"; function Component(t0) { - const $ = _c(3); + const $ = _c(5); const { a, b } = t0; let z; if ($[0] !== a || $[1] !== b) { z = { a }; - const y = { b }; + let t1; + if ($[3] !== b) { + t1 = { b }; + $[3] = b; + $[4] = t1; + } else { + t1 = $[4]; + } + const y = t1; const x = function () { z.a = 2; return Math.max(y.b, 0); diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capturing-func-simple-alias-iife.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capturing-func-simple-alias-iife.expect.md index 1b91bc1a11275..a071dddba6b4c 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capturing-func-simple-alias-iife.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capturing-func-simple-alias-iife.expect.md @@ -34,7 +34,6 @@ function component(a) { const x = { a }; y = {}; - y; y = x; mutate(y); diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capturing-function-alias-computed-load-2-iife.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capturing-function-alias-computed-load-2-iife.expect.md index f4721a507f31f..2afc5fd25dbac 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capturing-function-alias-computed-load-2-iife.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capturing-function-alias-computed-load-2-iife.expect.md @@ -31,7 +31,6 @@ function bar(a) { const x = [a]; y = {}; - y; y = x[0][1]; $[0] = a; $[1] = y; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capturing-function-alias-computed-load-3-iife.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capturing-function-alias-computed-load-3-iife.expect.md index 5c0be290a6930..3e57b7dc7c255 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capturing-function-alias-computed-load-3-iife.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capturing-function-alias-computed-load-3-iife.expect.md @@ -37,8 +37,6 @@ function bar(a, b) { let t; t = {}; - y; - t; y = x[0][1]; t = x[1][0]; $[0] = a; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capturing-function-alias-computed-load-4-iife.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capturing-function-alias-computed-load-4-iife.expect.md index 34b927d91e5f7..22728aaf4323d 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capturing-function-alias-computed-load-4-iife.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capturing-function-alias-computed-load-4-iife.expect.md @@ -31,7 +31,6 @@ function bar(a) { const x = [a]; y = {}; - y; y = x[0].a[1]; $[0] = a; $[1] = y; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capturing-function-alias-computed-load-iife.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capturing-function-alias-computed-load-iife.expect.md index 0978be54acb46..60f829cdc4d66 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capturing-function-alias-computed-load-iife.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capturing-function-alias-computed-load-iife.expect.md @@ -30,7 +30,6 @@ function bar(a) { const x = [a]; y = {}; - y; y = x[0]; $[0] = a; $[1] = y; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capturing-reference-changes-type.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capturing-reference-changes-type.expect.md index 1bdc1c09a3483..299aa5a31dff2 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capturing-reference-changes-type.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capturing-reference-changes-type.expect.md @@ -25,7 +25,6 @@ function component(a) { const x = { a }; y = 1; - y; y = x; mutate(y); diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife-reassign.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife-reassign.expect.md index d17c934b3b42c..cf85967682607 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife-reassign.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/codegen-inline-iife-reassign.expect.md @@ -38,9 +38,8 @@ function useTest() { const t1 = (w = 42); const t2 = w; - - w; let t3; + w = 999; t3 = 2; t0 = makeArray(t1, t2, t3); diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/const-propagation-into-function-expression-global.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/const-propagation-into-function-expression-global.expect.md index e42ea8ce933b8..04b6c4f17f41a 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/const-propagation-into-function-expression-global.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/const-propagation-into-function-expression-global.expect.md @@ -19,10 +19,10 @@ function foo() { import { c as _c } from "react/compiler-runtime"; function foo() { const $ = _c(1); + + const getJSX = _temp; let t0; if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - const getJSX = () => ; - t0 = getJSX(); $[0] = t0; } else { @@ -31,6 +31,9 @@ function foo() { const result = t0; return result; } +function _temp() { + return ; +} ``` \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/const-propagation-into-function-expression-primitive.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/const-propagation-into-function-expression-primitive.expect.md index 6686c0b5301bf..60fe0808d922a 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/const-propagation-into-function-expression-primitive.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/const-propagation-into-function-expression-primitive.expect.md @@ -23,13 +23,14 @@ export const FIXTURE_ENTRYPOINT = { ```javascript function foo() { - const f = () => { - console.log(42); - }; + const f = _temp; f(); return 42; } +function _temp() { + console.log(42); +} export const FIXTURE_ENTRYPOINT = { fn: foo, diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/constant-propagation-into-function-expressions.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/constant-propagation-into-function-expressions.expect.md index 8ea2190480003..8822eddcdb69f 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/constant-propagation-into-function-expressions.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/constant-propagation-into-function-expressions.expect.md @@ -18,12 +18,10 @@ function Component(props) { import { c as _c } from "react/compiler-runtime"; function Component(props) { const $ = _c(1); + + const onEvent = _temp; let t0; if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - const onEvent = () => { - console.log(42); - }; - t0 = ; $[0] = t0; } else { @@ -31,6 +29,9 @@ function Component(props) { } return t0; } +function _temp() { + console.log(42); +} ``` \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/context-variable-as-jsx-element-tag.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/context-variable-as-jsx-element-tag.expect.md index 3dc0dba27c364..da3bb94ed5ed4 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/context-variable-as-jsx-element-tag.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/context-variable-as-jsx-element-tag.expect.md @@ -34,9 +34,8 @@ function Component(props) { let Component; if ($[0] === Symbol.for("react.memo_cache_sentinel")) { Component = Stringify; - - Component; let t0; + t0 = Component; Component = t0; $[0] = Component; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hoisting-simple-function-declaration.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hoisting-simple-function-declaration.expect.md index 2045ee7901e96..1ba0d59e17265 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hoisting-simple-function-declaration.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hoisting-simple-function-declaration.expect.md @@ -24,13 +24,17 @@ export const FIXTURE_ENTRYPOINT = { ## Error ``` + 4 | } 5 | return baz(); // OK: FuncDecls are HoistableDeclarations that have both declaration and value hoisting - 6 | function baz() { +> 6 | function baz() { + | ^^^^^^^^^^^^^^^^ > 7 | return bar(); - | ^^^ Todo: Support functions with unreachable code that may contain hoisted declarations (7:7) - 8 | } + | ^^^^^^^^^^^^^^^^^ +> 8 | } + | ^^^^ Todo: Support functions with unreachable code that may contain hoisted declarations (6:8) 9 | } 10 | + 11 | export const FIXTURE_ENTRYPOINT = { ``` \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hook-call-freezes-captured-identifier.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hook-call-freezes-captured-identifier.expect.md new file mode 100644 index 0000000000000..7babe57b000e2 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hook-call-freezes-captured-identifier.expect.md @@ -0,0 +1,41 @@ + +## Input + +```javascript +// @enableTransitivelyFreezeFunctionExpressions +import {setPropertyByKey, Stringify, useIdentity} from 'shared-runtime'; + +function Foo({count}) { + const x = {value: 0}; + /** + * After this custom hook call, it's no longer valid to mutate x. + */ + const cb = useIdentity(() => { + setPropertyByKey(x, 'value', count); + }); + + x.value += count; + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{count: 1}], +}; + +``` + + +## Error + +``` + 11 | }); + 12 | +> 13 | x.value += count; + | ^ InvalidReact: This mutates a variable that React considers immutable (13:13) + 14 | return ; + 15 | } + 16 | +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hook-call-freezes-captured-identifier.tsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hook-call-freezes-captured-identifier.tsx new file mode 100644 index 0000000000000..b71626a435b78 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hook-call-freezes-captured-identifier.tsx @@ -0,0 +1,20 @@ +// @enableTransitivelyFreezeFunctionExpressions +import {setPropertyByKey, Stringify, useIdentity} from 'shared-runtime'; + +function Foo({count}) { + const x = {value: 0}; + /** + * After this custom hook call, it's no longer valid to mutate x. + */ + const cb = useIdentity(() => { + setPropertyByKey(x, 'value', count); + }); + + x.value += count; + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{count: 1}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hook-call-freezes-captured-memberexpr.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hook-call-freezes-captured-memberexpr.expect.md new file mode 100644 index 0000000000000..fcc47ddc2b14f --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hook-call-freezes-captured-memberexpr.expect.md @@ -0,0 +1,41 @@ + +## Input + +```javascript +// @enableTransitivelyFreezeFunctionExpressions +import {mutate, Stringify, useIdentity} from 'shared-runtime'; + +function Foo({count}) { + const x = {value: 0}; + /** + * After this custom hook call, it's no longer valid to mutate x. + */ + const cb = useIdentity(() => { + x.value++; + }); + + x.value += count; + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{count: 1}], +}; + +``` + + +## Error + +``` + 11 | }); + 12 | +> 13 | x.value += count; + | ^ InvalidReact: This mutates a variable that React considers immutable (13:13) + 14 | return ; + 15 | } + 16 | +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hook-call-freezes-captured-memberexpr.jsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hook-call-freezes-captured-memberexpr.jsx new file mode 100644 index 0000000000000..2a94559c1f026 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hook-call-freezes-captured-memberexpr.jsx @@ -0,0 +1,20 @@ +// @enableTransitivelyFreezeFunctionExpressions +import {mutate, Stringify, useIdentity} from 'shared-runtime'; + +function Foo({count}) { + const x = {value: 0}; + /** + * After this custom hook call, it's no longer valid to mutate x. + */ + const cb = useIdentity(() => { + x.value++; + }); + + x.value += count; + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{count: 1}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-repro-named-function-with-shadowed-local-same-name.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-repro-named-function-with-shadowed-local-same-name.expect.md index db3a192eaf604..f66b970f00dd4 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-repro-named-function-with-shadowed-local-same-name.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-repro-named-function-with-shadowed-local-same-name.expect.md @@ -22,7 +22,7 @@ function Component(props) { 7 | return hasErrors; 8 | } > 9 | return hasErrors(); - | ^^^^^^^^^ Invariant: [hoisting] Expected value for identifier to be initialized. hasErrors_0$16 (9:9) + | ^^^^^^^^^ Invariant: [hoisting] Expected value for identifier to be initialized. hasErrors_0$14 (9:9) 10 | } 11 | ``` diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-setstate-captured-indirectly-jsx.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-setstate-captured-indirectly-jsx.expect.md new file mode 100644 index 0000000000000..55cab1e9f3bd1 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-setstate-captured-indirectly-jsx.expect.md @@ -0,0 +1,81 @@ + +## Input + +```javascript +// @validatePreserveExistingMemoizationGuarantees +function useFoo() { + const onClick = response => { + setState(DISABLED_FORM); + }; + + const [state, setState] = useState(); + const handleLogout = useCallback(() => { + setState(DISABLED_FORM); + }, [setState]); + const getComponent = () => { + return handleLogout()} />; + }; + + // this `getComponent` call should not be inferred as mutating setState + return [getComponent(), onClick]; // pass onClick to avoid dce +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validatePreserveExistingMemoizationGuarantees +function useFoo() { + const $ = _c(9); + const onClick = (response) => { + setState(DISABLED_FORM); + }; + + const [, t0] = useState(); + const setState = t0; + let t1; + if ($[0] !== setState) { + t1 = () => { + setState(DISABLED_FORM); + }; + $[0] = setState; + $[1] = t1; + } else { + t1 = $[1]; + } + setState; + const handleLogout = t1; + let t2; + if ($[2] !== handleLogout) { + t2 = () => handleLogout()} />; + $[2] = handleLogout; + $[3] = t2; + } else { + t2 = $[3]; + } + const getComponent = t2; + let t3; + if ($[4] !== getComponent) { + t3 = getComponent(); + $[4] = getComponent; + $[5] = t3; + } else { + t3 = $[5]; + } + let t4; + if ($[6] !== onClick || $[7] !== t3) { + t4 = [t3, onClick]; + $[6] = onClick; + $[7] = t3; + $[8] = t4; + } else { + t4 = $[8]; + } + return t4; +} + +``` + +### Eval output +(kind: exception) Fixture not implemented \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-setstate-captured-indirectly-jsx.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-setstate-captured-indirectly-jsx.js new file mode 100644 index 0000000000000..4a44679390a80 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-setstate-captured-indirectly-jsx.js @@ -0,0 +1,17 @@ +// @validatePreserveExistingMemoizationGuarantees +function useFoo() { + const onClick = response => { + setState(DISABLED_FORM); + }; + + const [state, setState] = useState(); + const handleLogout = useCallback(() => { + setState(DISABLED_FORM); + }, [setState]); + const getComponent = () => { + return handleLogout()} />; + }; + + // this `getComponent` call should not be inferred as mutating setState + return [getComponent(), onClick]; // pass onClick to avoid dce +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-setstate.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-setstate.expect.md new file mode 100644 index 0000000000000..483d9b1a8e2da --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-setstate.expect.md @@ -0,0 +1,77 @@ + +## Input + +```javascript +import {useEffect, useState} from 'react'; +import {Stringify} from 'shared-runtime'; + +function Foo() { + /** + * Previously, this lowered to + * $1 = LoadContext capture setState + * $2 = FunctionExpression deps=$1 context=setState + * [[ at this point, we freeze the `LoadContext setState` instruction, but it will never be referenced again ]] + * + * Now, this function expression directly references `setState`, which freezes + * the source `DeclareContext HoistedConst setState`. Freezing source identifiers + * (instead of the one level removed `LoadContext`) is more semantically correct + * for everything *other* than hoisted context declarations. + * + * $2 = Function context=setState + */ + useEffect(() => setState(2), []); + + const [state, setState] = useState(0); + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{}], + sequentialRenders: [{}, {}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { useEffect, useState } from "react"; +import { Stringify } from "shared-runtime"; + +function Foo() { + const $ = _c(3); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 = []; + $[0] = t0; + } else { + t0 = $[0]; + } + useEffect(() => setState(2), t0); + + const [state, t1] = useState(0); + const setState = t1; + let t2; + if ($[1] !== state) { + t2 = ; + $[1] = state; + $[2] = t2; + } else { + t2 = $[2]; + } + return t2; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{}], + sequentialRenders: [{}, {}], +}; + +``` + +### Eval output +(kind: ok)
{"state":2}
+
{"state":2}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-setstate.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-setstate.js new file mode 100644 index 0000000000000..7b26c8d086491 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-setstate.js @@ -0,0 +1,28 @@ +import {useEffect, useState} from 'react'; +import {Stringify} from 'shared-runtime'; + +function Foo() { + /** + * Previously, this lowered to + * $1 = LoadContext capture setState + * $2 = FunctionExpression deps=$1 context=setState + * [[ at this point, we freeze the `LoadContext setState` instruction, but it will never be referenced again ]] + * + * Now, this function expression directly references `setState`, which freezes + * the source `DeclareContext HoistedConst setState`. Freezing source identifiers + * (instead of the one level removed `LoadContext`) is more semantically correct + * for everything *other* than hoisted context declarations. + * + * $2 = Function context=setState + */ + useEffect(() => setState(2), []); + + const [state, setState] = useState(0); + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{}], + sequentialRenders: [{}, {}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hook-call-freezes-captured-memberexpr.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hook-call-freezes-captured-memberexpr.expect.md new file mode 100644 index 0000000000000..957919516d09d --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hook-call-freezes-captured-memberexpr.expect.md @@ -0,0 +1,87 @@ + +## Input + +```javascript +import {useIdentity, Stringify, identity} from 'shared-runtime'; + +function Foo({val1}) { + // `x={inner: val1}` should be able to be memoized + const x = {inner: val1}; + + // Any references to `x` after this hook call should be read-only + const cb = useIdentity(() => x.inner); + + // With enableTransitivelyFreezeFunctionExpressions, it's invalid + // to write to `x` after it's been frozen. + // TODO: runtime validation for DX + const copy = identity(x); + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{val1: 1}], + sequentialRenders: [{val1: 1}, {val1: 1}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { useIdentity, Stringify, identity } from "shared-runtime"; + +function Foo(t0) { + const $ = _c(9); + const { val1 } = t0; + let t1; + if ($[0] !== val1) { + t1 = { inner: val1 }; + $[0] = val1; + $[1] = t1; + } else { + t1 = $[1]; + } + const x = t1; + let t2; + if ($[2] !== x.inner) { + t2 = () => x.inner; + $[2] = x.inner; + $[3] = t2; + } else { + t2 = $[3]; + } + const cb = useIdentity(t2); + let t3; + if ($[4] !== x) { + t3 = identity(x); + $[4] = x; + $[5] = t3; + } else { + t3 = $[5]; + } + const copy = t3; + let t4; + if ($[6] !== cb || $[7] !== copy) { + t4 = ; + $[6] = cb; + $[7] = copy; + $[8] = t4; + } else { + t4 = $[8]; + } + return t4; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{ val1: 1 }], + sequentialRenders: [{ val1: 1 }, { val1: 1 }], +}; + +``` + +### Eval output +(kind: ok)
{"copy":{"inner":1},"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}
+
{"copy":{"inner":1},"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hook-call-freezes-captured-memberexpr.tsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hook-call-freezes-captured-memberexpr.tsx new file mode 100644 index 0000000000000..68e8e034379e1 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hook-call-freezes-captured-memberexpr.tsx @@ -0,0 +1,21 @@ +import {useIdentity, Stringify, identity} from 'shared-runtime'; + +function Foo({val1}) { + // `x={inner: val1}` should be able to be memoized + const x = {inner: val1}; + + // Any references to `x` after this hook call should be read-only + const cb = useIdentity(() => x.inner); + + // With enableTransitivelyFreezeFunctionExpressions, it's invalid + // to write to `x` after it's been frozen. + // TODO: runtime validation for DX + const copy = identity(x); + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{val1: 1}], + sequentialRenders: [{val1: 1}, {val1: 1}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-local-tag-in-lambda.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-local-tag-in-lambda.expect.md index 74e01a72d52ba..a7d27bc38193f 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-local-tag-in-lambda.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-local-tag-in-lambda.expect.md @@ -25,10 +25,10 @@ import { c as _c } from "react/compiler-runtime"; import { Stringify } from "shared-runtime"; function useFoo() { const $ = _c(1); + + const callback = _temp; let t0; if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - const callback = () => ; - t0 = callback(); $[0] = t0; } else { @@ -36,6 +36,9 @@ function useFoo() { } return t0; } +function _temp() { + return ; +} export const FIXTURE_ENTRYPOINT = { fn: useFoo, diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-memberexpr-tag-in-lambda.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-memberexpr-tag-in-lambda.expect.md index 22fa3b2e2a2e3..e5ead2479dd40 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-memberexpr-tag-in-lambda.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/jsx-memberexpr-tag-in-lambda.expect.md @@ -25,10 +25,10 @@ import { c as _c } from "react/compiler-runtime"; import * as SharedRuntime from "shared-runtime"; function useFoo() { const $ = _c(1); + + const callback = _temp; let t0; if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - const callback = () => ; - t0 = callback(); $[0] = t0; } else { @@ -36,6 +36,9 @@ function useFoo() { } return t0; } +function _temp() { + return ; +} export const FIXTURE_ENTRYPOINT = { fn: useFoo, diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/lambda-mutated-non-reactive-to-reactive.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/lambda-mutated-non-reactive-to-reactive.expect.md index d34db46d6aa28..ed0ddda55b32f 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/lambda-mutated-non-reactive-to-reactive.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/lambda-mutated-non-reactive-to-reactive.expect.md @@ -26,7 +26,6 @@ function f(a) { const $ = _c(4); let x; if ($[0] !== a) { - x; x = { a }; $[0] = a; $[1] = x; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/lambda-mutated-ref-non-reactive.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/lambda-mutated-ref-non-reactive.expect.md index 2aa5d4d06dfb6..8dc4839085ee5 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/lambda-mutated-ref-non-reactive.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/lambda-mutated-ref-non-reactive.expect.md @@ -27,7 +27,6 @@ function f(a) { const $ = _c(2); let x; if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - x; x = {}; $[0] = x; } else { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nested-function-shadowed-identifiers.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nested-function-shadowed-identifiers.expect.md index 13ba6d17986bb..3c624de9ebe57 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nested-function-shadowed-identifiers.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nested-function-shadowed-identifiers.expect.md @@ -31,7 +31,7 @@ function Component(props) { let t0; if ($[0] === Symbol.for("react.memo_cache_sentinel")) { t0 = (e) => { - setX((currentX) => currentX + null); + setX(_temp); }; $[0] = t0; } else { @@ -48,6 +48,9 @@ function Component(props) { } return t1; } +function _temp(currentX) { + return currentX + null; +} export const FIXTURE_ENTRYPOINT = { fn: Component, diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useMemo-reordering-depslist-assignment.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useMemo-reordering-depslist-assignment.expect.md index e8a3e2d627c59..3fffec6a7dc20 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useMemo-reordering-depslist-assignment.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useMemo-reordering-depslist-assignment.expect.md @@ -35,7 +35,6 @@ function useFoo(arr1, arr2) { if ($[0] !== arr1 || $[1] !== arr2) { const x = [arr1]; - y; (y = x.concat(arr2)), y; $[0] = arr1; $[1] = arr2; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/rewrite-phis-in-lambda-capture-context.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/rewrite-phis-in-lambda-capture-context.expect.md index 2e451d8948988..0c66dee6a85b1 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/rewrite-phis-in-lambda-capture-context.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/rewrite-phis-in-lambda-capture-context.expect.md @@ -22,26 +22,21 @@ function Component() { ## Code ```javascript -import { c as _c } from "react/compiler-runtime"; function Component() { - const $ = _c(1); - let t0; - if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - t0 = () => { - while (bar()) { - if (baz) { - bar(); - } - } - return () => 4; - }; - $[0] = t0; - } else { - t0 = $[0]; - } - const get4 = t0; + const get4 = _temp2; return get4; } +function _temp2() { + while (bar()) { + if (baz) { + bar(); + } + } + return _temp; +} +function _temp() { + return 4; +} ``` \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/use-operator-conditional.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/use-operator-conditional.expect.md index e335273026791..6ad460347fa50 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/use-operator-conditional.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/use-operator-conditional.expect.md @@ -85,7 +85,6 @@ function Inner(props) { input = use(FooContext); } - input; input; let t0; let t1;