From 22b97005eec83c0da431ccf799ff4104c583ac69 Mon Sep 17 00:00:00 2001 From: Mike Vitousek Date: Thu, 1 Aug 2024 11:24:45 -0700 Subject: [PATCH] [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 6c31306274..192ad03671 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 0000000000..480057495a --- /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; + } +}