From 90ed7e8228047509d5dd4c309e3dc65913a4e9b5 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Fri, 9 May 2025 10:36:17 -0700 Subject: [PATCH] [compiler] Fix for PropertyStore object effect Fix for the issue in the previous PR. Long-term the ideal thing would be to make InferMutableRanges smarter about Store effects, and recognize that they are also transitive mutations of whatever was captured into the object. So in the following: ``` const x = {y: {z: {}}}; x.y.z.key = value; ``` That the `PropertyStore z . 'key' = value` is a transitive mutation of x and all three object expressions (x, x.y, x.y.z). But for now it's simpler to stick to the original idea of Store only counting if we know that the type is an object. [ghstack-poisoned] --- .../src/HIR/PrintHIR.ts | 2 +- .../src/Inference/AnalyseFunctions.ts | 7 +++--- .../src/Inference/InferMutableRanges.ts | 20 ++++++++++++++++ .../src/Inference/InferReferenceEffects.ts | 21 ++++++++--------- .../object-access-assignment.expect.md | 23 ++----------------- 5 files changed, 37 insertions(+), 36 deletions(-) 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 93acb4c9447ff..5325494f57ab8 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts @@ -731,7 +731,7 @@ function isMutable(range: MutableRange): boolean { } const DEBUG_MUTABLE_RANGES = false; -function printMutableRange(identifier: Identifier): string { +export function printMutableRange(identifier: Identifier): string { if (DEBUG_MUTABLE_RANGES) { // if debugging, print both the identifier and scope range if they differ const range = identifier.mutableRange; 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 fbd4ce8fa05b8..6a4da94d706e2 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts @@ -85,13 +85,13 @@ function inferAliasesForCapturing( for (const block of fn.body.blocks.values()) { for (const instr of block.instructions) { const {lvalue, value} = instr; - const hasStore = + const hasCapture = lvalue.effect === Effect.Store || Iterable_some( eachInstructionValueOperand(value), - operand => operand.effect === Effect.Store, + operand => operand.effect === Effect.Capture, ); - if (!hasStore) { + if (!hasCapture) { continue; } const operands: Array = []; @@ -101,6 +101,7 @@ function inferAliasesForCapturing( for (const operand of eachInstructionValueOperand(instr.value)) { if ( operand.effect === Effect.Store || + operand.effect === Effect.Mutate || operand.effect === Effect.Capture ) { operands.push(operand.identifier); diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutableRanges.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutableRanges.ts index 1236bae7997d0..82dafab9e2f18 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutableRanges.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutableRanges.ts @@ -5,6 +5,7 @@ * LICENSE file in the root directory of this source tree. */ +import prettyFormat from 'pretty-format'; import {HIRFunction, Identifier} from '../HIR/HIR'; import DisjointSet from '../Utils/DisjointSet'; import {inferAliasForUncalledFunctions} from './InerAliasForUncalledFunctions'; @@ -16,6 +17,11 @@ import {inferMutableLifetimes} from './InferMutableLifetimes'; import {inferMutableRangesForAlias} from './InferMutableRangesForAlias'; import {inferMutableRangesForComutation} from './InferMutableRangesForComutation'; import {inferTryCatchAliases} from './InferTryCatchAliases'; +import { + printFunction, + printIdentifier, + printMutableRange, +} from '../HIR/PrintHIR'; export function inferMutableRanges(ir: HIRFunction): DisjointSet { // Infer mutable ranges for non fields @@ -96,6 +102,20 @@ export function inferMutableRanges(ir: HIRFunction): DisjointSet { return aliases; } +export function debugAliases(aliases: DisjointSet): void { + console.log( + prettyFormat( + aliases + .buildSets() + .map(set => + [...set].map( + ident => printIdentifier(ident) + printMutableRange(ident), + ), + ), + ), + ); +} + /** * Canonicalizes the alias set and mutable range information calculated at the current time. * The returned value maps each identifier in the program to the root identifier of its alias 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 01d78663d0a2b..2de141bbd3c11 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts @@ -32,6 +32,7 @@ import { isMapType, isMutableEffect, isSetType, + isObjectType, } from '../HIR/HIR'; import {FunctionSignature} from '../HIR/ObjectShape'; import { @@ -1313,7 +1314,7 @@ function inferBlock( state.referenceAndRecordEffects( freezeActions, instrValue.object, - typeof instrValue.property === 'string' + isObjectType(instrValue.object.identifier) ? Effect.Store : Effect.Mutate, ValueReason.Other, @@ -1342,25 +1343,21 @@ function inferBlock( state.referenceAndRecordEffects( freezeActions, instrValue.object, - Effect.Read, + Effect.Capture, ValueReason.Other, ); const lvalue = instr.lvalue; - lvalue.effect = Effect.ConditionallyMutate; + lvalue.effect = Effect.Store; state.initialize(instrValue, state.kind(instrValue.object)); state.define(lvalue, instrValue); continuation = {kind: 'funeffects'}; break; } case 'ComputedStore': { - const effect = - state.kind(instrValue.object).kind === ValueKind.Context - ? Effect.ConditionallyMutate - : Effect.Capture; state.referenceAndRecordEffects( freezeActions, instrValue.value, - effect, + Effect.Capture, ValueReason.Other, ); state.referenceAndRecordEffects( @@ -1372,7 +1369,9 @@ function inferBlock( state.referenceAndRecordEffects( freezeActions, instrValue.object, - Effect.Store, + isObjectType(instrValue.object.identifier) + ? Effect.Store + : Effect.Mutate, ValueReason.Other, ); @@ -1409,7 +1408,7 @@ function inferBlock( state.referenceAndRecordEffects( freezeActions, instrValue.object, - Effect.Read, + Effect.Capture, ValueReason.Other, ); state.referenceAndRecordEffects( @@ -1419,7 +1418,7 @@ function inferBlock( ValueReason.Other, ); const lvalue = instr.lvalue; - lvalue.effect = Effect.ConditionallyMutate; + lvalue.effect = Effect.Store; state.initialize(instrValue, state.kind(instrValue.object)); state.define(lvalue, instrValue); continuation = {kind: 'funeffects'}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/object-access-assignment.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/object-access-assignment.expect.md index 4294deb974bba..8b4dbc8f863d6 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/object-access-assignment.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/object-access-assignment.expect.md @@ -33,7 +33,7 @@ export const FIXTURE_ENTRYPOINT = { ```javascript import { c as _c } from "react/compiler-runtime"; function Component(t0) { - const $ = _c(10); + const $ = _c(6); const { a, b, c } = t0; let t1; if ($[0] !== a || $[1] !== b || $[2] !== c) { @@ -47,26 +47,7 @@ function Component(t0) { t2 = $[5]; } const y = t2; - let t3; - let t4; - if ($[6] === Symbol.for("react.memo_cache_sentinel")) { - t3 = {}; - t4 = {}; - $[6] = t3; - $[7] = t4; - } else { - t3 = $[6]; - t4 = $[7]; - } - let t5; - if ($[8] !== c) { - t5 = { zero: c }; - $[8] = c; - $[9] = t5; - } else { - t5 = $[9]; - } - const z = { zero: t3, one: t4, two: t5 }; + const z = { zero: {}, one: {}, two: { zero: c } }; x.zero = y.one; z.zero.zero = x.zero; t1 = { zero: x, one: z };