From 22b97005eec83c0da431ccf799ff4104c583ac69 Mon Sep 17 00:00:00 2001 From: Mike Vitousek Date: Thu, 1 Aug 2024 11:24:45 -0700 Subject: [PATCH 1/6] [compiler] Bail out and log calls that likely have side effects [ghstack-poisoned] --- .../src/Entrypoint/Pipeline.ts | 3 + .../ReactiveScopes/ValidateNoMutationCalls.ts | 127 ++++++++++++++++++ 2 files changed, 130 insertions(+) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/ValidateNoMutationCalls.ts diff --git a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts index 6c313062744..192ad036715 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts @@ -98,6 +98,7 @@ import { } from '../Validation'; import {validateLocalsNotReassignedAfterRender} from '../Validation/ValidateLocalsNotReassignedAfterRender'; import {outlineFunctions} from '../Optimization/OutlineFunctions'; +import {validateNoMutationCalls} from '../ReactiveScopes/ValidateNoMutationCalls'; export type CompilerPipelineValue = | {kind: 'ast'; name: string; value: CodegenFunction} @@ -484,6 +485,8 @@ function* runWithEnvironment( validatePreservedManualMemoization(reactiveFunction); } + validateNoMutationCalls(reactiveFunction); + const ast = codegenFunction(reactiveFunction, { uniqueIdentifiers, fbtOperands, diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/ValidateNoMutationCalls.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/ValidateNoMutationCalls.ts new file mode 100644 index 00000000000..480057495a7 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/ValidateNoMutationCalls.ts @@ -0,0 +1,127 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +import {CompilerError, ErrorSeverity} from '../CompilerError'; +import {Environment} from '../HIR'; +import { + Effect, + Identifier, + IdentifierId, + ReactiveFunction, + ReactiveInstruction, + getHookKind, + isSetStateType, +} from '../HIR/HIR'; +import { + ReactiveFunctionVisitor, + eachReactiveValueOperand, + visitReactiveFunction, +} from './visitors'; + +const allowedNames = new Set(['invariant']); + +class Visitor extends ReactiveFunctionVisitor { + #env: Environment; + #names: Map = new Map(); + + constructor(env: Environment) { + super(); + this.#env = env; + } + + getName(id: Identifier): string | undefined { + if (id.name?.kind === 'named') { + return id.name.value; + } else { + return this.#names.get(id.id); + } + } + + setName(id: Identifier, name: string | undefined): void { + if (name != null) { + this.#names.set(id.id, name); + } + } + + override visitInstruction( + instr: ReactiveInstruction, + state: CompilerError, + ): void { + if (instr.lvalue != null) { + switch (instr.value.kind) { + case 'LoadGlobal': + this.setName(instr.lvalue.identifier, instr.value.binding.name); + break; + case 'LoadLocal': + case 'LoadContext': + this.setName( + instr.lvalue.identifier, + this.getName(instr.value.place.identifier), + ); + break; + case 'PropertyLoad': { + const name = this.getName(instr.value.object.identifier); + if (name != null) { + this.setName( + instr.lvalue.identifier, + name + '.' + instr.value.property, + ); + } + break; + } + case 'ComputedLoad': { + const name = this.getName(instr.value.object.identifier); + if (name != null) { + this.setName(instr.lvalue.identifier, name + '[...]'); + } + break; + } + } + } + + if ( + instr.value.kind === 'CallExpression' || + instr.value.kind === 'MethodCall' + ) { + let isException = false; + let name = '(unknown)'; + if (instr.value.kind === 'CallExpression') { + isException = + getHookKind(this.#env, instr.value.callee.identifier) != null || + isSetStateType(instr.value.callee.identifier); + name = this.getName(instr.value.callee.identifier) ?? name; + } else { + isException = + getHookKind(this.#env, instr.value.property.identifier) != null || + isSetStateType(instr.value.property.identifier); + name = this.getName(instr.value.property.identifier) ?? name; + } + if (instr.lvalue === null && !isException && allowedNames.has(name)) { + let allReads = true; + for (const operand of eachReactiveValueOperand(instr.value)) { + allReads &&= operand.effect === Effect.Read; + } + if (allReads) { + state.push({ + reason: `Likely illegal mutation call \`${name}\``, + loc: instr.loc, + severity: ErrorSeverity.Todo, + }); + } + } + } + super.visitInstruction(instr, state); + } +} + +export function validateNoMutationCalls(fn: ReactiveFunction): void { + const error = new CompilerError(); + visitReactiveFunction(fn, new Visitor(fn.env), error); + if (error.hasErrors()) { + throw error; + } +} From 129abed386dadeda6e4299f373215ec6a5493040 Mon Sep 17 00:00:00 2001 From: Mike Vitousek Date: Thu, 1 Aug 2024 11:35:26 -0700 Subject: [PATCH 2/6] Update on "[compiler] Bail out and log calls that likely have side effects" [ghstack-poisoned] --- .../src/ReactiveScopes/ValidateNoMutationCalls.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/ValidateNoMutationCalls.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/ValidateNoMutationCalls.ts index 480057495a7..8d7f7e253bc 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/ValidateNoMutationCalls.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/ValidateNoMutationCalls.ts @@ -100,7 +100,7 @@ class Visitor extends ReactiveFunctionVisitor { isSetStateType(instr.value.property.identifier); name = this.getName(instr.value.property.identifier) ?? name; } - if (instr.lvalue === null && !isException && allowedNames.has(name)) { + if (instr.lvalue === null && !isException && !allowedNames.has(name)) { let allReads = true; for (const operand of eachReactiveValueOperand(instr.value)) { allReads &&= operand.effect === Effect.Read; From 3affe0339f62c7f41741252dd76b6b4ec61005a2 Mon Sep 17 00:00:00 2001 From: Mike Vitousek Date: Thu, 1 Aug 2024 13:25:32 -0700 Subject: [PATCH 3/6] Update on "[compiler] Bail out and log calls that likely have side effects" [ghstack-poisoned] --- .../ReactiveScopes/ValidateNoMutationCalls.ts | 77 +++++++++++++++---- 1 file changed, 64 insertions(+), 13 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/ValidateNoMutationCalls.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/ValidateNoMutationCalls.ts index 8d7f7e253bc..194f4af2eab 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/ValidateNoMutationCalls.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/ValidateNoMutationCalls.ts @@ -11,22 +11,26 @@ import { Effect, Identifier, IdentifierId, + InstructionId, + Place, ReactiveFunction, ReactiveInstruction, getHookKind, isSetStateType, } from '../HIR/HIR'; +import { eachInstructionValueLValue } from '../HIR/visitors'; import { ReactiveFunctionVisitor, eachReactiveValueOperand, visitReactiveFunction, } from './visitors'; -const allowedNames = new Set(['invariant']); +const allowedNames = new Set(['invariant', 'recoverableViolation']); class Visitor extends ReactiveFunctionVisitor { #env: Environment; - #names: Map = new Map(); + #names = new Map(); + #functions = new Map() constructor(env: Environment) { super(); @@ -47,6 +51,10 @@ class Visitor extends ReactiveFunctionVisitor { } } + override visitReactiveFunctionValue(): void { + CompilerError.invariant(false, { reason: 'visitReactiveFunctionValue should not be called', loc: null }) + } + override visitInstruction( instr: ReactiveInstruction, state: CompilerError, @@ -55,6 +63,7 @@ class Visitor extends ReactiveFunctionVisitor { switch (instr.value.kind) { case 'LoadGlobal': this.setName(instr.lvalue.identifier, instr.value.binding.name); + super.visitInstruction(instr, state); break; case 'LoadLocal': case 'LoadContext': @@ -62,6 +71,7 @@ class Visitor extends ReactiveFunctionVisitor { instr.lvalue.identifier, this.getName(instr.value.place.identifier), ); + super.visitInstruction(instr, state); break; case 'PropertyLoad': { const name = this.getName(instr.value.object.identifier); @@ -71,6 +81,7 @@ class Visitor extends ReactiveFunctionVisitor { name + '.' + instr.value.property, ); } + super.visitInstruction(instr, state); break; } case 'ComputedLoad': { @@ -78,28 +89,69 @@ class Visitor extends ReactiveFunctionVisitor { if (name != null) { this.setName(instr.lvalue.identifier, name + '[...]'); } + super.visitInstruction(instr, state); + break; + } + case 'FunctionExpression': { + this.setName(instr.lvalue.identifier, instr.value.name ?? undefined); + const state = new CompilerError(); + this.visitHirFunction(instr.value.loweredFunc.func, state); + if (state.hasErrors()) { + this.#functions.set(instr.lvalue.identifier.id, state); + } + break; + } + case 'ReactiveFunctionValue': { + this.setName(instr.lvalue.identifier, instr.value.fn.id ?? undefined); + const state = new CompilerError(); + this.visitBlock(instr.value.fn.body, state); + if (state.hasErrors()) { + this.#functions.set(instr.lvalue.identifier.id, state); + } + break; + } + default: { + super.visitInstruction(instr, state); break; } } } + let hookKind = null; + let callee = null; if ( instr.value.kind === 'CallExpression' || instr.value.kind === 'MethodCall' ) { - let isException = false; - let name = '(unknown)'; if (instr.value.kind === 'CallExpression') { - isException = - getHookKind(this.#env, instr.value.callee.identifier) != null || - isSetStateType(instr.value.callee.identifier); - name = this.getName(instr.value.callee.identifier) ?? name; + callee = instr.value.callee; } else { - isException = - getHookKind(this.#env, instr.value.property.identifier) != null || - isSetStateType(instr.value.property.identifier); - name = this.getName(instr.value.property.identifier) ?? name; + callee = instr.value.property; } + hookKind = getHookKind(this.#env, callee.identifier); + } + + if (hookKind !== 'useEffect' && hookKind !== 'useLayoutEffect' && hookKind !== 'useInsertionEffect' && instr.value.kind !== "JsxExpression") { + for (const operand of eachReactiveValueOperand(instr.value)) { + const errors = this.#functions.get(operand.identifier.id); + if (errors != null) { + for (const lval of eachInstructionValueLValue(instr.value)) { + const existing = this.#functions.get(lval.identifier.id) ?? new CompilerError(); + errors.details.forEach(detail => existing.pushErrorDetail(detail)); + this.#functions.set(lval.identifier.id, existing); + } + } + } + } + + if (callee != null) { + const isException = + hookKind != null || + isSetStateType(callee.identifier); + const name = this.getName(callee.identifier) ?? "(unknown)"; + + this.#functions.get(callee.identifier.id)?.details?.forEach(detail => state.pushErrorDetail(detail)); + if (instr.lvalue === null && !isException && !allowedNames.has(name)) { let allReads = true; for (const operand of eachReactiveValueOperand(instr.value)) { @@ -114,7 +166,6 @@ class Visitor extends ReactiveFunctionVisitor { } } } - super.visitInstruction(instr, state); } } From 2e114b0b2ef258ef56e5794746637ddacbd29a68 Mon Sep 17 00:00:00 2001 From: Mike Vitousek Date: Thu, 1 Aug 2024 14:05:37 -0700 Subject: [PATCH 4/6] Update on "[compiler] Bail out and log calls that likely have side effects" [ghstack-poisoned] --- .../ReactiveScopes/PruneTemporaryLValues.ts | 3 +++ .../ReactiveScopes/ValidateNoMutationCalls.ts | 12 +++++++---- .../block-scoping-switch-variable-scoping.js | 21 +++++++------------ 3 files changed, 18 insertions(+), 18 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneTemporaryLValues.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneTemporaryLValues.ts index 535a4e5e911..330cb3455a6 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneTemporaryLValues.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneTemporaryLValues.ts @@ -36,6 +36,9 @@ class Visitor extends ReactiveFunctionVisitor { instruction: ReactiveInstruction, state: LValues, ): void { + if (instruction.value.kind === 'FunctionExpression') { + this.visitHirFunction(instruction.value.loweredFunc.func, state) + } this.traverseInstruction(instruction, state); if ( instruction.lvalue !== null && diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/ValidateNoMutationCalls.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/ValidateNoMutationCalls.ts index 194f4af2eab..ecade61f143 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/ValidateNoMutationCalls.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/ValidateNoMutationCalls.ts @@ -18,7 +18,7 @@ import { getHookKind, isSetStateType, } from '../HIR/HIR'; -import { eachInstructionValueLValue } from '../HIR/visitors'; +import { eachInstructionLValue, eachInstructionValueLValue } from '../HIR/visitors'; import { ReactiveFunctionVisitor, eachReactiveValueOperand, @@ -115,6 +115,8 @@ class Visitor extends ReactiveFunctionVisitor { break; } } + } else { + super.visitInstruction(instr, state); } let hookKind = null; @@ -131,11 +133,11 @@ class Visitor extends ReactiveFunctionVisitor { hookKind = getHookKind(this.#env, callee.identifier); } - if (hookKind !== 'useEffect' && hookKind !== 'useLayoutEffect' && hookKind !== 'useInsertionEffect' && instr.value.kind !== "JsxExpression") { + if (instr.value.kind !== 'JsxExpression') { for (const operand of eachReactiveValueOperand(instr.value)) { const errors = this.#functions.get(operand.identifier.id); if (errors != null) { - for (const lval of eachInstructionValueLValue(instr.value)) { + for (const lval of eachInstructionLValue(instr)) { const existing = this.#functions.get(lval.identifier.id) ?? new CompilerError(); errors.details.forEach(detail => existing.pushErrorDetail(detail)); this.#functions.set(lval.identifier.id, existing); @@ -150,7 +152,9 @@ class Visitor extends ReactiveFunctionVisitor { isSetStateType(callee.identifier); const name = this.getName(callee.identifier) ?? "(unknown)"; - this.#functions.get(callee.identifier.id)?.details?.forEach(detail => state.pushErrorDetail(detail)); + if (hookKind !== 'useEffect' && hookKind !== 'useLayoutEffect' && hookKind !== 'useInsertionEffect') { + [...eachReactiveValueOperand(instr.value)].forEach(operand => this.#functions.get(operand.identifier.id)?.details?.forEach(detail => state.pushErrorDetail(detail))); + } if (instr.lvalue === null && !isException && !allowedNames.has(name)) { let allReads = true; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/block-scoping-switch-variable-scoping.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/block-scoping-switch-variable-scoping.js index 6b005c0e046..4dc4f84b8c6 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/block-scoping-switch-variable-scoping.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/block-scoping-switch-variable-scoping.js @@ -1,19 +1,12 @@ -import {useMemo} from 'react'; +import {useEffect, useMemo} from 'react'; function Component(props) { - const outerHandlers = useMemo(() => { - let handlers = {value: props.value}; - switch (props.test) { - case true: { - console.log(handlers.value); - break; - } - default: { - } - } - return handlers; - }); - return outerHandlers; + function foo() { + mutate(); + } + const h = [foo]; + useEffect(() => { a(h) }); + return lengh(); } export const FIXTURE_ENTRYPOINT = { From d4ce56ffc6ee1abf7fccb1bb27476e9b45197464 Mon Sep 17 00:00:00 2001 From: Mike Vitousek Date: Thu, 1 Aug 2024 15:38:56 -0700 Subject: [PATCH 5/6] Update on "[compiler] Bail out and log calls that likely have side effects" [ghstack-poisoned] --- .../src/Entrypoint/Pipeline.ts | 3 - .../src/HIR/HIR.ts | 5 + .../src/Inference/InferReferenceEffects.ts | 70 ++++++- .../ReactiveScopes/PruneTemporaryLValues.ts | 3 - .../ReactiveScopes/ValidateNoMutationCalls.ts | 182 ------------------ .../block-scoping-switch-variable-scoping.js | 21 +- 6 files changed, 86 insertions(+), 198 deletions(-) delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/ValidateNoMutationCalls.ts diff --git a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts index 192ad036715..6c313062744 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts @@ -98,7 +98,6 @@ import { } from '../Validation'; import {validateLocalsNotReassignedAfterRender} from '../Validation/ValidateLocalsNotReassignedAfterRender'; import {outlineFunctions} from '../Optimization/OutlineFunctions'; -import {validateNoMutationCalls} from '../ReactiveScopes/ValidateNoMutationCalls'; export type CompilerPipelineValue = | {kind: 'ast'; name: string; value: CodegenFunction} @@ -485,8 +484,6 @@ function* runWithEnvironment( validatePreservedManualMemoization(reactiveFunction); } - validateNoMutationCalls(reactiveFunction); - const ast = codegenFunction(reactiveFunction, { uniqueIdentifiers, fbtOperands, 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 b94bffa040f..b2de996f502 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts @@ -294,6 +294,11 @@ export type HIRFunction = { }; export type FunctionEffect = + | { + kind: 'GlobalFunctionCall', + error: CompilerErrorDetailOptions; + lvalue: IdentifierId | null; + } | { kind: 'GlobalMutation'; error: CompilerErrorDetailOptions; 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 f250361e3b2..abee2e2dcab 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts @@ -238,6 +238,7 @@ export default function inferReferenceEffects( } if (!options.isFunctionExpression) { + let usedIdentifiers: null | Set = null; functionEffects.forEach(eff => { switch (eff.kind) { case 'ReactMutation': @@ -251,6 +252,28 @@ export default function inferReferenceEffects( loc: eff.loc, }); } + case "GlobalFunctionCall": { + if (eff.lvalue == null) { + functionEffects.push(eff); + } else { + if (usedIdentifiers === null) { + usedIdentifiers = new Set(); + for (const [,block] of fn.body.blocks) { + for (const instr of block.instructions) { + Array.from(eachInstructionOperand(instr)).forEach(operand => usedIdentifiers?.add(operand.identifier.id)); + } + for (const phi of block.phis) { + Array.from(phi.operands.values()).forEach(operand => usedIdentifiers?.add(operand.id)); + } + Array.from(eachTerminalOperand(block.terminal)).forEach(operand => usedIdentifiers?.add(operand.identifier.id)); + } + } + if (!usedIdentifiers.has(eff.lvalue)) { + CompilerError.throw(eff.error); + } + } + break; + } default: assertExhaustive( eff, @@ -406,6 +429,7 @@ class InferenceState { value.kind === 'ObjectMethod') && value.loweredFunc.func.effects != null ) { + let usedIdentifiers: null | Set = null; for (const effect of value.loweredFunc.func.effects) { if ( effect.kind === 'GlobalMutation' || @@ -413,7 +437,27 @@ class InferenceState { ) { // Known effects are always propagated upwards functionEffects.push(effect); - } else { + } else if (effect.kind === "GlobalFunctionCall") { + if (effect.lvalue == null) { + functionEffects.push(effect); + } else { + if (usedIdentifiers === null) { + usedIdentifiers = new Set(); + for (const [,block] of value.loweredFunc.func.body.blocks) { + for (const instr of block.instructions) { + Array.from(eachInstructionOperand(instr)).forEach(operand => usedIdentifiers?.add(operand.identifier.id)); + } + for (const phi of block.phis) { + Array.from(phi.operands.values()).forEach(operand => usedIdentifiers?.add(operand.id)); + } + Array.from(eachTerminalOperand(block.terminal)).forEach(operand => usedIdentifiers?.add(operand.identifier.id)); + } + } + if (!usedIdentifiers.has(effect.lvalue)) { + functionEffects.push({ ...effect, lvalue: null }); + } + } + } else if (effect.kind === 'ContextMutation') { /** * Contextual effects need to be replayed against the current inference * state, which may know more about the value to which the effect applied. @@ -444,6 +488,8 @@ class InferenceState { } // else case 2, local mutable value so this effect was fine } } + } else { + assertExhaustive(effect, `Unexpected function effect kind`); } } } @@ -1357,7 +1403,7 @@ function inferBlock( functionEffects.push( ...argumentEffects.filter( argEffect => - !isUseEffect || i !== 0 || argEffect.kind !== 'GlobalMutation', + !isUseEffect || i !== 0 || (argEffect.kind !== 'GlobalMutation' && argEffect.kind !== 'GlobalFunctionCall'), ), ); hasCaptureArgument ||= place.effect === Effect.Capture; @@ -1379,6 +1425,24 @@ function inferBlock( } hasCaptureArgument ||= instrValue.callee.effect === Effect.Capture; + const calleeValue = state.kind(instrValue.callee); + if (calleeValue.kind === ValueKind.Global && signature?.hookKind == null) { + const allRead = instrValue.args.every(arg => { + switch (arg.kind) { + case "Identifier": + return arg.effect === Effect.Read; + case "Spread": + return arg.place.effect === Effect.Read; + default: + assertExhaustive(arg, 'Unexpected arg kind'); + } + }); + if (allRead) { + functionEffects.push({kind: "GlobalFunctionCall", lvalue: instr.lvalue.identifier.id, error: {reason: "External function is called with arguments that React Compiler expects to be immutable and return value is ignored. This call is likely to perform unsafe side effects, which violates the rules of React.", loc: instrValue.loc, severity: ErrorSeverity.InvalidReact}}); + } + } + + state.initialize(instrValue, returnValueKind); state.define(instr.lvalue, instrValue); instr.lvalue.effect = hasCaptureArgument @@ -1486,7 +1550,7 @@ function inferBlock( functionEffects.push( ...argumentEffects.filter( argEffect => - !isUseEffect || i !== 0 || argEffect.kind !== 'GlobalMutation', + !isUseEffect || i !== 0 || (argEffect.kind !== 'GlobalMutation' && argEffect.kind !== 'GlobalFunctionCall'), ), ); hasCaptureArgument ||= place.effect === Effect.Capture; diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneTemporaryLValues.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneTemporaryLValues.ts index 330cb3455a6..535a4e5e911 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneTemporaryLValues.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneTemporaryLValues.ts @@ -36,9 +36,6 @@ class Visitor extends ReactiveFunctionVisitor { instruction: ReactiveInstruction, state: LValues, ): void { - if (instruction.value.kind === 'FunctionExpression') { - this.visitHirFunction(instruction.value.loweredFunc.func, state) - } this.traverseInstruction(instruction, state); if ( instruction.lvalue !== null && diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/ValidateNoMutationCalls.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/ValidateNoMutationCalls.ts deleted file mode 100644 index ecade61f143..00000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/ValidateNoMutationCalls.ts +++ /dev/null @@ -1,182 +0,0 @@ -/** - * Copyright (c) Meta Platforms, Inc. and affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - */ - -import {CompilerError, ErrorSeverity} from '../CompilerError'; -import {Environment} from '../HIR'; -import { - Effect, - Identifier, - IdentifierId, - InstructionId, - Place, - ReactiveFunction, - ReactiveInstruction, - getHookKind, - isSetStateType, -} from '../HIR/HIR'; -import { eachInstructionLValue, eachInstructionValueLValue } from '../HIR/visitors'; -import { - ReactiveFunctionVisitor, - eachReactiveValueOperand, - visitReactiveFunction, -} from './visitors'; - -const allowedNames = new Set(['invariant', 'recoverableViolation']); - -class Visitor extends ReactiveFunctionVisitor { - #env: Environment; - #names = new Map(); - #functions = new Map() - - constructor(env: Environment) { - super(); - this.#env = env; - } - - getName(id: Identifier): string | undefined { - if (id.name?.kind === 'named') { - return id.name.value; - } else { - return this.#names.get(id.id); - } - } - - setName(id: Identifier, name: string | undefined): void { - if (name != null) { - this.#names.set(id.id, name); - } - } - - override visitReactiveFunctionValue(): void { - CompilerError.invariant(false, { reason: 'visitReactiveFunctionValue should not be called', loc: null }) - } - - override visitInstruction( - instr: ReactiveInstruction, - state: CompilerError, - ): void { - if (instr.lvalue != null) { - switch (instr.value.kind) { - case 'LoadGlobal': - this.setName(instr.lvalue.identifier, instr.value.binding.name); - super.visitInstruction(instr, state); - break; - case 'LoadLocal': - case 'LoadContext': - this.setName( - instr.lvalue.identifier, - this.getName(instr.value.place.identifier), - ); - super.visitInstruction(instr, state); - break; - case 'PropertyLoad': { - const name = this.getName(instr.value.object.identifier); - if (name != null) { - this.setName( - instr.lvalue.identifier, - name + '.' + instr.value.property, - ); - } - super.visitInstruction(instr, state); - break; - } - case 'ComputedLoad': { - const name = this.getName(instr.value.object.identifier); - if (name != null) { - this.setName(instr.lvalue.identifier, name + '[...]'); - } - super.visitInstruction(instr, state); - break; - } - case 'FunctionExpression': { - this.setName(instr.lvalue.identifier, instr.value.name ?? undefined); - const state = new CompilerError(); - this.visitHirFunction(instr.value.loweredFunc.func, state); - if (state.hasErrors()) { - this.#functions.set(instr.lvalue.identifier.id, state); - } - break; - } - case 'ReactiveFunctionValue': { - this.setName(instr.lvalue.identifier, instr.value.fn.id ?? undefined); - const state = new CompilerError(); - this.visitBlock(instr.value.fn.body, state); - if (state.hasErrors()) { - this.#functions.set(instr.lvalue.identifier.id, state); - } - break; - } - default: { - super.visitInstruction(instr, state); - break; - } - } - } else { - super.visitInstruction(instr, state); - } - - let hookKind = null; - let callee = null; - if ( - instr.value.kind === 'CallExpression' || - instr.value.kind === 'MethodCall' - ) { - if (instr.value.kind === 'CallExpression') { - callee = instr.value.callee; - } else { - callee = instr.value.property; - } - hookKind = getHookKind(this.#env, callee.identifier); - } - - if (instr.value.kind !== 'JsxExpression') { - for (const operand of eachReactiveValueOperand(instr.value)) { - const errors = this.#functions.get(operand.identifier.id); - if (errors != null) { - for (const lval of eachInstructionLValue(instr)) { - const existing = this.#functions.get(lval.identifier.id) ?? new CompilerError(); - errors.details.forEach(detail => existing.pushErrorDetail(detail)); - this.#functions.set(lval.identifier.id, existing); - } - } - } - } - - if (callee != null) { - const isException = - hookKind != null || - isSetStateType(callee.identifier); - const name = this.getName(callee.identifier) ?? "(unknown)"; - - if (hookKind !== 'useEffect' && hookKind !== 'useLayoutEffect' && hookKind !== 'useInsertionEffect') { - [...eachReactiveValueOperand(instr.value)].forEach(operand => this.#functions.get(operand.identifier.id)?.details?.forEach(detail => state.pushErrorDetail(detail))); - } - - if (instr.lvalue === null && !isException && !allowedNames.has(name)) { - let allReads = true; - for (const operand of eachReactiveValueOperand(instr.value)) { - allReads &&= operand.effect === Effect.Read; - } - if (allReads) { - state.push({ - reason: `Likely illegal mutation call \`${name}\``, - loc: instr.loc, - severity: ErrorSeverity.Todo, - }); - } - } - } - } -} - -export function validateNoMutationCalls(fn: ReactiveFunction): void { - const error = new CompilerError(); - visitReactiveFunction(fn, new Visitor(fn.env), error); - if (error.hasErrors()) { - throw error; - } -} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/block-scoping-switch-variable-scoping.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/block-scoping-switch-variable-scoping.js index 4dc4f84b8c6..6b005c0e046 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/block-scoping-switch-variable-scoping.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/block-scoping-switch-variable-scoping.js @@ -1,12 +1,19 @@ -import {useEffect, useMemo} from 'react'; +import {useMemo} from 'react'; function Component(props) { - function foo() { - mutate(); - } - const h = [foo]; - useEffect(() => { a(h) }); - return lengh(); + const outerHandlers = useMemo(() => { + let handlers = {value: props.value}; + switch (props.test) { + case true: { + console.log(handlers.value); + break; + } + default: { + } + } + return handlers; + }); + return outerHandlers; } export const FIXTURE_ENTRYPOINT = { From 40529371953b710f55869c4aec66805c4cdbff85 Mon Sep 17 00:00:00 2001 From: Mike Vitousek Date: Thu, 1 Aug 2024 16:53:17 -0700 Subject: [PATCH 6/6] Update on "[compiler] Bail out and log calls that likely have side effects" [ghstack-poisoned] --- .../src/HIR/HIR.ts | 8 +- .../src/Inference/InferReferenceEffects.ts | 226 +++++++++++++----- 2 files changed, 177 insertions(+), 57 deletions(-) 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 b2de996f502..db9e2a0f5b2 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts @@ -295,9 +295,11 @@ export type HIRFunction = { export type FunctionEffect = | { - kind: 'GlobalFunctionCall', - error: CompilerErrorDetailOptions; - lvalue: IdentifierId | null; + kind: 'ImmutableFunctionCall'; + loc: SourceLocation; + lvalue: IdentifierId; + callee: IdentifierId; + global: boolean; } | { kind: 'GlobalMutation'; 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 abee2e2dcab..e23fe036973 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts @@ -30,6 +30,7 @@ import { isMutableEffect, isObjectType, isRefValueType, + isSetStateType, isUseRefType, } from '../HIR/HIR'; import {FunctionSignature} from '../HIR/ObjectShape'; @@ -238,7 +239,6 @@ export default function inferReferenceEffects( } if (!options.isFunctionExpression) { - let usedIdentifiers: null | Set = null; functionEffects.forEach(eff => { switch (eff.kind) { case 'ReactMutation': @@ -252,26 +252,8 @@ export default function inferReferenceEffects( loc: eff.loc, }); } - case "GlobalFunctionCall": { - if (eff.lvalue == null) { - functionEffects.push(eff); - } else { - if (usedIdentifiers === null) { - usedIdentifiers = new Set(); - for (const [,block] of fn.body.blocks) { - for (const instr of block.instructions) { - Array.from(eachInstructionOperand(instr)).forEach(operand => usedIdentifiers?.add(operand.identifier.id)); - } - for (const phi of block.phis) { - Array.from(phi.operands.values()).forEach(operand => usedIdentifiers?.add(operand.id)); - } - Array.from(eachTerminalOperand(block.terminal)).forEach(operand => usedIdentifiers?.add(operand.identifier.id)); - } - } - if (!usedIdentifiers.has(eff.lvalue)) { - CompilerError.throw(eff.error); - } - } + case 'ImmutableFunctionCall': { + // Handled below break; } default: @@ -281,9 +263,122 @@ export default function inferReferenceEffects( ); } }); - } else { - fn.effects = functionEffects; + + if (functionEffects.length > 0) { + const error = new CompilerError(); + let usedIdentifiers = new Set(); + let names = new Map(); + + function visitFunction(fn: HIRFunction): void { + for (const [, block] of fn.body.blocks) { + for (const instr of block.instructions) { + switch (instr.value.kind) { + case 'FunctionExpression': + case 'ObjectMethod': + names.set( + instr.lvalue.identifier.id, + instr.value.loweredFunc.func.id ?? '(anonymous function)', + ); + visitFunction(instr.value.loweredFunc.func); + break; + case 'LoadGlobal': + names.set(instr.lvalue.identifier.id, instr.value.binding.name); + break; + case 'LoadLocal': + case 'LoadContext': + names.set( + instr.lvalue.identifier.id, + instr.value.place.identifier.name?.value ?? + names.get(instr.value.place.identifier.id), + ); + break; + case 'StoreContext': + case 'StoreLocal': + names.set( + instr.lvalue.identifier.id, + instr.value.value.identifier.name?.value ?? + names.get(instr.value.value.identifier.id), + ); + names.set( + instr.value.lvalue.place.identifier.id, + instr.value.value.identifier.name?.value ?? + names.get(instr.value.value.identifier.id), + ); + break; + case 'PropertyLoad': + names.set( + instr.lvalue.identifier.id, + `${instr.value.object.identifier.name?.value ?? names.get(instr.value.object.identifier.id) ?? '(unknown)'}.${instr.value.property}`, + ); + break; + case 'ComputedLoad': + names.set( + instr.lvalue.identifier.id, + `${instr.value.object.identifier.name?.value ?? names.get(instr.value.object.identifier.id) ?? '(unknown)'}[...]`, + ); + break; + case 'Destructure': { + const destructuredName = + instr.value.value.identifier.name?.value ?? + names.get(instr.value.value.identifier.id); + const destructuredMsg = destructuredName + ? `(destructured from \`${destructuredName}\`)` + : '(destructured)'; + Array.from( + eachPatternOperand(instr.value.lvalue.pattern), + ).forEach(place => + names.set( + place.identifier.id, + `${place.identifier.name?.value ?? 'value'} ${destructuredMsg}`, + ), + ); + } + } + Array.from(eachInstructionOperand(instr)).forEach(operand => + usedIdentifiers.add(operand.identifier.id), + ); + } + for (const phi of block.phis) { + Array.from(phi.operands.values()).forEach(operand => + usedIdentifiers.add(operand.id), + ); + } + Array.from(eachTerminalOperand(block.terminal)).forEach(operand => + usedIdentifiers.add(operand.identifier.id), + ); + } + } + visitFunction(fn); + + const allowedNames = new Set(['invariant', 'recoverableViolation']); + + for (const effect of functionEffects) { + CompilerError.invariant(effect.kind === 'ImmutableFunctionCall', { + reason: + 'All effects other than ImmutableFunctionCall should have been handled earlier', + loc: null, + }); + if ( + !usedIdentifiers.has(effect.lvalue) && + (!effect.global || + !names.has(effect.callee) || + !allowedNames.has(names.get(effect.callee)!)) + ) { + const name = names.get(effect.callee) ?? '(unknown)'; + error.push({ + reason: `Function \'${name}\' is called with arguments that React Compiler expects to be immutable and its return value is ignored. This call is likely to perform unsafe side effects, which violates the rules of React.`, + loc: effect.loc, + severity: ErrorSeverity.InvalidReact, + }); + } + } + + if (error.hasErrors()) { + throw error; + } + } } + fn.effects = functionEffects; } // Maintains a mapping of top-level variables to the kind of value they hold @@ -429,34 +524,14 @@ class InferenceState { value.kind === 'ObjectMethod') && value.loweredFunc.func.effects != null ) { - let usedIdentifiers: null | Set = null; for (const effect of value.loweredFunc.func.effects) { if ( effect.kind === 'GlobalMutation' || - effect.kind === 'ReactMutation' + effect.kind === 'ReactMutation' || + effect.kind === 'ImmutableFunctionCall' ) { // Known effects are always propagated upwards functionEffects.push(effect); - } else if (effect.kind === "GlobalFunctionCall") { - if (effect.lvalue == null) { - functionEffects.push(effect); - } else { - if (usedIdentifiers === null) { - usedIdentifiers = new Set(); - for (const [,block] of value.loweredFunc.func.body.blocks) { - for (const instr of block.instructions) { - Array.from(eachInstructionOperand(instr)).forEach(operand => usedIdentifiers?.add(operand.identifier.id)); - } - for (const phi of block.phis) { - Array.from(phi.operands.values()).forEach(operand => usedIdentifiers?.add(operand.id)); - } - Array.from(eachTerminalOperand(block.terminal)).forEach(operand => usedIdentifiers?.add(operand.identifier.id)); - } - } - if (!usedIdentifiers.has(effect.lvalue)) { - functionEffects.push({ ...effect, lvalue: null }); - } - } } else if (effect.kind === 'ContextMutation') { /** * Contextual effects need to be replayed against the current inference @@ -1197,7 +1272,9 @@ function inferBlock( ); functionEffects.push( ...propEffects.filter( - propEffect => propEffect.kind !== 'GlobalMutation', + propEffect => + propEffect.kind !== 'GlobalMutation' && + propEffect.kind !== 'ImmutableFunctionCall', ), ); } @@ -1403,7 +1480,10 @@ function inferBlock( functionEffects.push( ...argumentEffects.filter( argEffect => - !isUseEffect || i !== 0 || (argEffect.kind !== 'GlobalMutation' && argEffect.kind !== 'GlobalFunctionCall'), + !isUseEffect || + i !== 0 || + (argEffect.kind !== 'GlobalMutation' && + argEffect.kind !== 'ImmutableFunctionCall'), ), ); hasCaptureArgument ||= place.effect === Effect.Capture; @@ -1425,24 +1505,32 @@ function inferBlock( } hasCaptureArgument ||= instrValue.callee.effect === Effect.Capture; - const calleeValue = state.kind(instrValue.callee); - if (calleeValue.kind === ValueKind.Global && signature?.hookKind == null) { + if ( + !isSetStateType(instrValue.callee.identifier) && + instrValue.callee.effect === Effect.Read && + signature?.hookKind == null + ) { const allRead = instrValue.args.every(arg => { switch (arg.kind) { - case "Identifier": + case 'Identifier': return arg.effect === Effect.Read; - case "Spread": + case 'Spread': return arg.place.effect === Effect.Read; default: assertExhaustive(arg, 'Unexpected arg kind'); } }); if (allRead) { - functionEffects.push({kind: "GlobalFunctionCall", lvalue: instr.lvalue.identifier.id, error: {reason: "External function is called with arguments that React Compiler expects to be immutable and return value is ignored. This call is likely to perform unsafe side effects, which violates the rules of React.", loc: instrValue.loc, severity: ErrorSeverity.InvalidReact}}); + functionEffects.push({ + kind: 'ImmutableFunctionCall', + lvalue: instr.lvalue.identifier.id, + callee: instrValue.callee.identifier.id, + loc: instrValue.loc, + global: state.kind(instrValue.callee).kind === ValueKind.Global, + }); } } - state.initialize(instrValue, returnValueKind); state.define(instr.lvalue, instrValue); instr.lvalue.effect = hasCaptureArgument @@ -1550,7 +1638,10 @@ function inferBlock( functionEffects.push( ...argumentEffects.filter( argEffect => - !isUseEffect || i !== 0 || (argEffect.kind !== 'GlobalMutation' && argEffect.kind !== 'GlobalFunctionCall'), + !isUseEffect || + i !== 0 || + (argEffect.kind !== 'GlobalMutation' && + argEffect.kind !== 'ImmutableFunctionCall'), ), ); hasCaptureArgument ||= place.effect === Effect.Capture; @@ -1572,6 +1663,33 @@ function inferBlock( } hasCaptureArgument ||= instrValue.receiver.effect === Effect.Capture; + if ( + !isSetStateType(instrValue.property.identifier) && + instrValue.receiver.effect === Effect.Read && + instrValue.property.effect === Effect.Read && + signature?.hookKind == null + ) { + const allRead = instrValue.args.every(arg => { + switch (arg.kind) { + case 'Identifier': + return arg.effect === Effect.Read; + case 'Spread': + return arg.place.effect === Effect.Read; + default: + assertExhaustive(arg, 'Unexpected arg kind'); + } + }); + if (allRead) { + functionEffects.push({ + kind: 'ImmutableFunctionCall', + lvalue: instr.lvalue.identifier.id, + callee: instrValue.property.identifier.id, + loc: instrValue.loc, + global: state.kind(instrValue.property).kind === ValueKind.Global, + }); + } + } + state.initialize(instrValue, returnValueKind); state.define(instr.lvalue, instrValue); instr.lvalue.effect = hasCaptureArgument