From bde30f02840ad49119c2433d54ad352e95f53420 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Tue, 13 Feb 2024 16:45:16 -0800 Subject: [PATCH] [be] Remove ValidateFrozenLambdas This pass doesn't really make sense in light of `@enableTransitivelyFreezeFunctionExpressions`. The original idea of ValidateFrozenLambdas was that trying to pass a "mutable" lambda to a frozen value was invalid. But since then we've realized that the better heuristic is that freezing a lambda is transitive. --- .../src/Entrypoint/Pipeline.ts | 5 - .../src/HIR/Environment.ts | 6 - .../src/Validation/ValidateFrozenLambdas.ts | 159 ------------------ .../src/Validation/index.ts | 1 - ...valid-capture-func-passed-to-jsx.expect.md | 27 --- ...rror.invalid-capture-func-passed-to-jsx.js | 12 -- ...eze-conditionally-mutable-lambda.expect.md | 32 ---- ...lid-freeze-conditionally-mutable-lambda.js | 17 -- ...eeze-mutable-lambda-mutate-local.expect.md | 25 --- ...alid-freeze-mutable-lambda-mutate-local.js | 10 -- ...ze-mutable-lambda-reassign-local.expect.md | 23 --- ...id-freeze-mutable-lambda-reassign-local.js | 8 - .../src/__tests__/parseConfigPragma-test.ts | 6 +- 13 files changed, 3 insertions(+), 328 deletions(-) delete mode 100644 compiler/packages/babel-plugin-react-forget/src/Validation/ValidateFrozenLambdas.ts delete mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-capture-func-passed-to-jsx.expect.md delete mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-capture-func-passed-to-jsx.js delete mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-freeze-conditionally-mutable-lambda.expect.md delete mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-freeze-conditionally-mutable-lambda.js delete mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-freeze-mutable-lambda-mutate-local.expect.md delete mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-freeze-mutable-lambda-mutate-local.js delete mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-freeze-mutable-lambda-reassign-local.expect.md delete mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-freeze-mutable-lambda-reassign-local.js diff --git a/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Pipeline.ts b/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Pipeline.ts index e29e79f4e0..4fc702403f 100644 --- a/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Pipeline.ts @@ -70,7 +70,6 @@ import { import { assertExhaustive } from "../Utils/utils"; import { validateContextVariableLValues, - validateFrozenLambdas, validateHooksUsage, validateMemoizedEffectDependencies, validateNoRefAccessInRender, @@ -161,10 +160,6 @@ function* runWithEnvironment( inferReferenceEffects(hir); yield log({ kind: "hir", name: "InferReferenceEffects", value: hir }); - if (env.config.validateFrozenLambdas) { - validateFrozenLambdas(hir); - } - // Note: Has to come after infer reference effects because "dead" code may still affect inference deadCodeElimination(hir); yield log({ kind: "hir", name: "DeadCodeElimination", value: hir }); diff --git a/compiler/packages/babel-plugin-react-forget/src/HIR/Environment.ts b/compiler/packages/babel-plugin-react-forget/src/HIR/Environment.ts index b3a64b2b17..08817866b9 100644 --- a/compiler/packages/babel-plugin-react-forget/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-forget/src/HIR/Environment.ts @@ -180,12 +180,6 @@ const EnvironmentConfigSchema = z.object({ */ validateRefAccessDuringRenderFunctionExpressions: z.boolean().default(false), - /* - * Validate that mutable lambdas are not passed where a frozen value is expected, since mutable - * lambdas cannot be frozen. The only mutation allowed inside a frozen lambda is of ref values. - */ - validateFrozenLambdas: z.boolean().default(false), - /* * Validates that setState is not unconditionally called during render, as it can lead to * infinite loops. diff --git a/compiler/packages/babel-plugin-react-forget/src/Validation/ValidateFrozenLambdas.ts b/compiler/packages/babel-plugin-react-forget/src/Validation/ValidateFrozenLambdas.ts deleted file mode 100644 index aab5f06856..0000000000 --- a/compiler/packages/babel-plugin-react-forget/src/Validation/ValidateFrozenLambdas.ts +++ /dev/null @@ -1,159 +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, - CompilerErrorDetail, - ErrorSeverity, -} from "../CompilerError"; -import { - Effect, - FunctionExpression, - HIRFunction, - IdentifierId, - ObjectMethod, - Place, - isRefValueType, - isUseRefType, -} from "../HIR/HIR"; -import { - eachInstructionValueOperand, - eachTerminalOperand, -} from "../HIR/visitors"; - -/* - * Various APIs in React take ownership of the values passed to them, such that it is invalid - * to subsequently modify those values. Examples include: - * - Passing a value as a prop to JSX. Subsequently mutating this value will result in undefined - * behavior, since the mutation may or may not be observed depending on when the child re-renders. - * In addition, the value may be used as an input to memoization in children, and mutation could - * invalidate that memoization. - * - Passing a value to `useState()`, for the same reason. - * - Passing a value to a hook, for the same reason. - * - * Most "normal" data types (objects, arrays, etc) can be "frozen" when passed to a React API simply - * by not calling any mutating methods on them. However, mutable lambdas are an exception: if a lambda - * has side-effects, there is no way to do something to the lambda that would allow calling it without - * triggering those side effects. The only thing a developer could do is not call the lambda, but developers - * also have no way of knowing that they can't call the lambda. - * - * From a type system perspective, the above APIs that "take ownership" of their values really accept - * *already frozen* values as input. Thus it is invalid to pass a value that cannot be frozen to these APIs, - * and it is therefore invalid to pass a mutable lambda. - * - * This pass validates the above rule. Note that this validation can by bypassed by storing a mutable lambda - * inside some value (eg as an array element or object property). In these cases we trust that the developer - * is not breaking the rules. The goal of this validation is to find cases that are provably wrong and help - * the developer fix the mistake earlier. - */ -export function validateFrozenLambdas(fn: HIRFunction): void { - const state = new State(); - - const errors = new CompilerError(); - for (const [, block] of fn.body.blocks) { - for (const phi of block.phis) { - for (const [, operand] of phi.operands) { - const resolvedId = state.temporaries.get(operand.id) ?? operand.id; - const lambda = state.lambdas.get(resolvedId); - if (lambda !== undefined) { - state.lambdas.set(phi.id.id, lambda); - break; - } - } - } - for (const instr of block.instructions) { - switch (instr.value.kind) { - case "ObjectMethod": - case "FunctionExpression": { - if ( - instr.value.loweredFunc.dependencies.some( - (place) => - place.effect === Effect.Mutate && - !isRefValueType(place.identifier) && - !isUseRefType(place.identifier) - ) - ) { - state.lambdas.set(instr.lvalue.identifier.id, instr.value); - } - break; - } - case "LoadLocal": { - const resolvedId = - state.temporaries.get(instr.value.place.identifier.id) ?? - instr.value.place.identifier.id; - state.temporaries.set(instr.lvalue.identifier.id, resolvedId); - break; - } - case "StoreLocal": { - const resolvedId = - state.temporaries.get(instr.value.value.identifier.id) ?? - instr.value.value.identifier.id; - state.temporaries.set( - instr.value.lvalue.place.identifier.id, - resolvedId - ); - break; - } - default: { - for (const operand of eachInstructionValueOperand(instr.value)) { - const operandError = validateOperand(operand, state); - if (operandError !== null) { - errors.pushErrorDetail(operandError); - } - } - } - } - } - for (const operand of eachTerminalOperand(block.terminal)) { - const operandError = validateOperand(operand, state); - if (operandError !== null) { - errors.pushErrorDetail(operandError); - } - } - } - if (errors.hasErrors()) { - throw errors; - } -} - -class State { - lambdas: Map = new Map(); - temporaries: Map = new Map(); -} - -function validateOperand( - operand: Place, - state: State -): CompilerErrorDetail | null { - if (operand.effect === Effect.Freeze) { - const operandId = - state.temporaries.get(operand.identifier.id) ?? operand.identifier.id; - const lambda = state.lambdas.get(operandId); - if (lambda !== undefined) { - /* - * TODO: these seem to always be null, we should try to preserve original - * names from source - * TODO: figure out how to print object methods as they don't have names - */ - const description = - lambda.kind === "FunctionExpression" && - lambda.name !== null && - operand.identifier.name !== null - ? `\`${lambda.name}\` is a function that may mutate \`${operand.identifier.name}\`. If you must mutate \`${operand.identifier.name}\` try using a React API like useState and use its setter function instead` - : null; - return new CompilerErrorDetail({ - description, - loc: typeof operand.loc !== "symbol" ? operand.loc : null, - reason: - "This mutates a variable that is managed by React, where an immutable value or a function was expected", - severity: ErrorSeverity.InvalidReact, - suggestions: null, - }); - } - } - return null; -} diff --git a/compiler/packages/babel-plugin-react-forget/src/Validation/index.ts b/compiler/packages/babel-plugin-react-forget/src/Validation/index.ts index 708c8a2881..a6e30b4812 100644 --- a/compiler/packages/babel-plugin-react-forget/src/Validation/index.ts +++ b/compiler/packages/babel-plugin-react-forget/src/Validation/index.ts @@ -6,7 +6,6 @@ */ export { validateContextVariableLValues } from "./ValidateContextVariableLValues"; -export { validateFrozenLambdas } from "./ValidateFrozenLambdas"; export { validateHooksUsage } from "./ValidateHooksUsage"; export { validateMemoizedEffectDependencies } from "./ValidateMemoizedEffectDependencies"; export { validateNoRefAccessInRender } from "./ValidateNoRefAccesInRender"; diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-capture-func-passed-to-jsx.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-capture-func-passed-to-jsx.expect.md deleted file mode 100644 index 85a7e7f672..0000000000 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-capture-func-passed-to-jsx.expect.md +++ /dev/null @@ -1,27 +0,0 @@ - -## Input - -```javascript -// @validateFrozenLambdas -function component(a, b) { - let y = { b }; - let z = { a }; - let x = function () { - z.a = 2; - y.b; - }; - let t = ; - mutate(x); // x should be frozen here - return t; -} - -``` - - -## Error - -``` -[ReactForget] InvalidReact: This mutates a variable that is managed by React, where an immutable value or a function was expected (9:9) -``` - - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-capture-func-passed-to-jsx.js b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-capture-func-passed-to-jsx.js deleted file mode 100644 index 13841d6307..0000000000 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-capture-func-passed-to-jsx.js +++ /dev/null @@ -1,12 +0,0 @@ -// @validateFrozenLambdas -function component(a, b) { - let y = { b }; - let z = { a }; - let x = function () { - z.a = 2; - y.b; - }; - let t = ; - mutate(x); // x should be frozen here - return t; -} diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-freeze-conditionally-mutable-lambda.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-freeze-conditionally-mutable-lambda.expect.md deleted file mode 100644 index 6f62e04264..0000000000 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-freeze-conditionally-mutable-lambda.expect.md +++ /dev/null @@ -1,32 +0,0 @@ - -## Input - -```javascript -// @validateFrozenLambdas -function Component(props) { - const x = {}; - let fn; - if (props.cond) { - // mutable - fn = () => { - x.value = props.value; - }; - } else { - // immutable - fn = () => { - x.value; - }; - } - return fn; -} - -``` - - -## Error - -``` -[ReactForget] InvalidReact: This mutates a variable that is managed by React, where an immutable value or a function was expected (16:16) -``` - - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-freeze-conditionally-mutable-lambda.js b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-freeze-conditionally-mutable-lambda.js deleted file mode 100644 index 076e9cd6cf..0000000000 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-freeze-conditionally-mutable-lambda.js +++ /dev/null @@ -1,17 +0,0 @@ -// @validateFrozenLambdas -function Component(props) { - const x = {}; - let fn; - if (props.cond) { - // mutable - fn = () => { - x.value = props.value; - }; - } else { - // immutable - fn = () => { - x.value; - }; - } - return fn; -} diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-freeze-mutable-lambda-mutate-local.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-freeze-mutable-lambda-mutate-local.expect.md deleted file mode 100644 index a991205cf0..0000000000 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-freeze-mutable-lambda-mutate-local.expect.md +++ /dev/null @@ -1,25 +0,0 @@ - -## Input - -```javascript -// @validateFrozenLambdas -function Component(props) { - const x = {}; - const onChange = (e) => { - // INVALID! should use copy-on-write and pass the new value - x.value = e.target.value; - setX(x); - }; - return ; -} - -``` - - -## Error - -``` -[ReactForget] InvalidReact: This mutates a variable that is managed by React, where an immutable value or a function was expected (9:9) -``` - - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-freeze-mutable-lambda-mutate-local.js b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-freeze-mutable-lambda-mutate-local.js deleted file mode 100644 index 9c830542c0..0000000000 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-freeze-mutable-lambda-mutate-local.js +++ /dev/null @@ -1,10 +0,0 @@ -// @validateFrozenLambdas -function Component(props) { - const x = {}; - const onChange = (e) => { - // INVALID! should use copy-on-write and pass the new value - x.value = e.target.value; - setX(x); - }; - return ; -} diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-freeze-mutable-lambda-reassign-local.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-freeze-mutable-lambda-reassign-local.expect.md deleted file mode 100644 index 1404800c4b..0000000000 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-freeze-mutable-lambda-reassign-local.expect.md +++ /dev/null @@ -1,23 +0,0 @@ - -## Input - -```javascript -// @validateFrozenLambdas -function Component(props) { - let x = ""; - const onChange = (e) => { - x = e.target.value; - }; - return ; -} - -``` - - -## Error - -``` -[ReactForget] InvalidReact: This mutates a variable that is managed by React, where an immutable value or a function was expected (7:7) -``` - - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-freeze-mutable-lambda-reassign-local.js b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-freeze-mutable-lambda-reassign-local.js deleted file mode 100644 index f71882f091..0000000000 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-freeze-mutable-lambda-reassign-local.js +++ /dev/null @@ -1,8 +0,0 @@ -// @validateFrozenLambdas -function Component(props) { - let x = ""; - const onChange = (e) => { - x = e.target.value; - }; - return ; -} diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/parseConfigPragma-test.ts b/compiler/packages/babel-plugin-react-forget/src/__tests__/parseConfigPragma-test.ts index ae2966c153..b0aaa7e19c 100644 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/parseConfigPragma-test.ts +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/parseConfigPragma-test.ts @@ -14,16 +14,16 @@ describe("parseConfigPragma()", () => { // Validate defaults first to make sure that the parser is getting the value from the pragma, // and not just missing it and getting the default value expect(defaultConfig.enableForest).toBe(false); - expect(defaultConfig.validateFrozenLambdas).toBe(false); + expect(defaultConfig.validateRefAccessDuringRender).toBe(false); expect(defaultConfig.memoizeJsxElements).toBe(true); const config = parseConfigPragma( - "@enableForest @validateFrozenLambdas:true @memoizeJsxElements:false" + "@enableForest @validateRefAccessDuringRender:true @memoizeJsxElements:false" ); expect(config).toEqual({ ...defaultConfig, enableForest: true, - validateFrozenLambdas: true, + validateRefAccessDuringRender: true, memoizeJsxElements: false, }); });