From 70bd31a49ae4452dd13f4653513f2e9b350a1236 Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Tue, 18 Feb 2025 09:16:26 -0700 Subject: [PATCH] [compiler] Remove redundant InferMutableContextVariables This removes special casing for `PropertyStore` mutability inference within FunctionExpressions. --- .../src/Inference/AnalyseFunctions.ts | 27 +---- .../Inference/InferMutableContextVariables.ts | 105 ------------------ ...mutate-global-in-effect-fixpoint.expect.md | 66 ++++++----- .../allow-mutate-global-in-effect-fixpoint.js | 7 +- .../reanimated-shared-value-writes.expect.md | 23 ++-- 5 files changed, 54 insertions(+), 174 deletions(-) delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutableContextVariables.ts 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 5381262874369..a439b4cd01232 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts @@ -10,7 +10,6 @@ import { Effect, HIRFunction, Identifier, - IdentifierId, LoweredFunction, isRefOrRefValue, makeInstructionId, @@ -18,27 +17,9 @@ import { import {deadCodeElimination} from '../Optimization'; import {inferReactiveScopeVariables} from '../ReactiveScopes'; import {rewriteInstructionKindsBasedOnReassignment} from '../SSA'; -import {inferMutableContextVariables} from './InferMutableContextVariables'; import {inferMutableRanges} from './InferMutableRanges'; import inferReferenceEffects from './InferReferenceEffects'; -// Helper class to track indirections such as LoadLocal and PropertyLoad. -export class IdentifierState { - properties: Map = new Map(); - - resolve(identifier: Identifier): Identifier { - const resolved = this.properties.get(identifier.id); - if (resolved !== undefined) { - return resolved; - } - return identifier; - } - - alias(lvalue: Identifier, value: Identifier): void { - this.properties.set(lvalue.id, this.properties.get(value.id) ?? value); - } -} - export default function analyseFunctions(func: HIRFunction): void { for (const [_, block] of func.body.blocks) { for (const instr of block.instructions) { @@ -78,7 +59,6 @@ function lower(func: HIRFunction): void { } function infer(loweredFunc: LoweredFunction): void { - const knownMutated = inferMutableContextVariables(loweredFunc.func); for (const operand of loweredFunc.func.context) { const identifier = operand.identifier; CompilerError.invariant(operand.effect === Effect.Unknown, { @@ -95,10 +75,11 @@ function infer(loweredFunc: LoweredFunction): void { * render */ operand.effect = Effect.Capture; - } else if (knownMutated.has(operand)) { - operand.effect = Effect.Mutate; } else if (isMutatedOrReassigned(identifier)) { - // Note that this also reflects if identifier is ConditionallyMutated + /** + * Reflects direct reassignments, PropertyStores, and ConditionallyMutate + * (directly or through maybe-aliases) + */ operand.effect = Effect.Capture; } else { operand.effect = Effect.Read; diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutableContextVariables.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutableContextVariables.ts deleted file mode 100644 index 0025472721542..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutableContextVariables.ts +++ /dev/null @@ -1,105 +0,0 @@ -/** - * Copyright (c) Meta Platforms, Inc. and affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - */ - -import {Effect, HIRFunction, Identifier, Place} from '../HIR'; -import { - eachInstructionValueOperand, - eachTerminalOperand, -} from '../HIR/visitors'; -import {IdentifierState} from './AnalyseFunctions'; - -/* - * This pass infers which of the given function's context (free) variables - * are definitively mutated by the function. This analysis is *partial*, - * and only annotates provable mutations, and may miss mutations via indirections. - * The intent of this pass is to drive validations, rejecting known-bad code - * while avoiding false negatives, and the inference should *not* be used to - * drive changes in output. - * - * Note that a complete analysis is possible but would have too many false negatives. - * The approach would be to run LeaveSSA and InferReactiveScopeVariables in order to - * find all possible aliases of a context variable which may be mutated. However, this - * can lead to false negatives: - * - * ``` - * const [x, setX] = useState(null); // x is frozen - * const fn = () => { // context=[x] - * const z = {}; // z is mutable - * foo(z, x); // potentially mutate z and x - * z.a = true; // definitively mutate z - * } - * fn(); - * ``` - * - * When we analyze function expressions we assume that context variables are mutable, - * so we assume that `x` is mutable. We infer that `foo(z, x)` could be mutating the - * two variables to alias each other, such that `z.a = true` could be mutating `x`, - * and we would infer that `x` is definitively mutated. Then when we run - * InferReferenceEffects on the outer code we'd reject it, since there is a definitive - * mutation of a frozen value. - * - * Thus the actual implementation looks at only basic aliasing. The above example would - * pass, since z does not directly alias `x`. However, mutations through trivial aliases - * are detected: - * - * ``` - * const [x, setX] = useState(null); // x is frozen - * const fn = () => { // context=[x] - * const z = x; - * z.a = true; // ERROR: mutates x - * } - * fn(); - * ``` - */ -export function inferMutableContextVariables(fn: HIRFunction): Set { - const state = new IdentifierState(); - const knownMutatedIdentifiers = new Set(); - for (const [, block] of fn.body.blocks) { - for (const instr of block.instructions) { - switch (instr.value.kind) { - case 'PropertyLoad': - case 'ComputedLoad': { - state.alias(instr.lvalue.identifier, instr.value.object.identifier); - break; - } - case 'LoadLocal': - case 'LoadContext': { - if (instr.lvalue.identifier.name === null) { - state.alias(instr.lvalue.identifier, instr.value.place.identifier); - } - break; - } - default: { - for (const operand of eachInstructionValueOperand(instr.value)) { - visitOperand(state, knownMutatedIdentifiers, operand); - } - } - } - } - for (const operand of eachTerminalOperand(block.terminal)) { - visitOperand(state, knownMutatedIdentifiers, operand); - } - } - const results = new Set(); - for (const operand of fn.context) { - if (knownMutatedIdentifiers.has(operand.identifier)) { - results.add(operand); - } - } - return results; -} - -function visitOperand( - state: IdentifierState, - knownMutatedIdentifiers: Set, - operand: Place, -): void { - const resolved = state.resolve(operand.identifier); - if (operand.effect === Effect.Mutate || operand.effect === Effect.Store) { - knownMutatedIdentifiers.add(resolved); - } -} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutate-global-in-effect-fixpoint.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutate-global-in-effect-fixpoint.expect.md index 942daec1dd08c..76e4432fe90d9 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutate-global-in-effect-fixpoint.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutate-global-in-effect-fixpoint.expect.md @@ -19,9 +19,14 @@ function Component() { // capture into a separate variable that is not a context variable. const y = x; + /** + * Note that this fixture currently produces a stale effect closure if `y = x + * = someGlobal` changes between renders. Under current compiler assumptions, + * that would be a rule of react violation. + */ useEffect(() => { y.value = 'hello'; - }, []); + }); useEffect(() => { setState(someGlobal.value); @@ -46,57 +51,50 @@ import { useEffect, useState } from "react"; let someGlobal = { value: null }; function Component() { - const $ = _c(7); + const $ = _c(5); const [state, setState] = useState(someGlobal); + + let x = someGlobal; + while (x == null) { + x = someGlobal; + } + + const y = x; let t0; - let t1; - let t2; if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - let x = someGlobal; - while (x == null) { - x = someGlobal; - } - - const y = x; - t0 = useEffect; - t1 = () => { + t0 = () => { y.value = "hello"; }; - t2 = []; $[0] = t0; + } else { + t0 = $[0]; + } + useEffect(t0); + let t1; + let t2; + if ($[1] === Symbol.for("react.memo_cache_sentinel")) { + t1 = () => { + setState(someGlobal.value); + }; + t2 = [someGlobal]; $[1] = t1; $[2] = t2; } else { - t0 = $[0]; t1 = $[1]; t2 = $[2]; } - t0(t1, t2); - let t3; + useEffect(t1, t2); + + const t3 = String(state); let t4; - if ($[3] === Symbol.for("react.memo_cache_sentinel")) { - t3 = () => { - setState(someGlobal.value); - }; - t4 = [someGlobal]; + if ($[3] !== t3) { + t4 =
{t3}
; $[3] = t3; $[4] = t4; } else { - t3 = $[3]; t4 = $[4]; } - useEffect(t3, t4); - - const t5 = String(state); - let t6; - if ($[5] !== t5) { - t6 =
{t5}
; - $[5] = t5; - $[6] = t6; - } else { - t6 = $[6]; - } - return t6; + return t4; } export const FIXTURE_ENTRYPOINT = { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutate-global-in-effect-fixpoint.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutate-global-in-effect-fixpoint.js index 84bd42aaefd18..6e44adf204101 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutate-global-in-effect-fixpoint.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutate-global-in-effect-fixpoint.js @@ -15,9 +15,14 @@ function Component() { // capture into a separate variable that is not a context variable. const y = x; + /** + * Note that this fixture currently produces a stale effect closure if `y = x + * = someGlobal` changes between renders. Under current compiler assumptions, + * that would be a rule of react violation. + */ useEffect(() => { y.value = 'hello'; - }, []); + }); useEffect(() => { setState(someGlobal.value); diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reanimated-shared-value-writes.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reanimated-shared-value-writes.expect.md index 8f808c94b3043..0a19a85428939 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reanimated-shared-value-writes.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reanimated-shared-value-writes.expect.md @@ -36,21 +36,22 @@ import { useSharedValue } from "react-native-reanimated"; * of render */ function SomeComponent() { - const $ = _c(3); + const $ = _c(2); const sharedVal = useSharedValue(0); - - const T0 = Button; - const t0 = () => (sharedVal.value = Math.random()); - let t1; - if ($[0] !== T0 || $[1] !== t0) { - t1 = ; - $[0] = T0; + let t0; + if ($[0] !== sharedVal) { + t0 = ( +