From 278c7f56babc574a66392eacace297715e4d5446 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Fri, 16 Jun 2023 14:42:16 -0700 Subject: [PATCH] ValidateFrozenLambdas: visit terminals The original version of the code wasn't checking return values. I missed this since my examples were passing functions _into_ the return value, as opposed to return the functions directly. This revealed some existing test fixtures that were technically invalid, but easy to fix by changing the return value. --- .../src/HIR/ValidateFrozenLambdas.ts | 87 ++++++++++++------- .../capturing-func-mutate-2.expect.md | 14 +-- .../compiler/capturing-func-mutate-2.js | 2 +- .../capturing-func-mutate-3.expect.md | 39 ++------- .../compiler/capturing-func-mutate-3.js | 2 +- .../capturing-func-mutate-nested.expect.md | 14 +-- .../compiler/capturing-func-mutate-nested.js | 2 +- .../compiler/capturing-func-mutate.expect.md | 27 ++---- .../compiler/capturing-func-mutate.js | 2 +- ...ction-conditional-capture-mutate.expect.md | 55 ------------ ...ing-function-conditional-capture-mutate.js | 10 --- .../capturing-nested-member-call.expect.md | 18 +--- .../compiler/capturing-nested-member-call.js | 2 +- ...ction-conditional-capture-mutate.expect.md | 28 ++++++ ...ing-function-conditional-capture-mutate.js | 13 +++ 15 files changed, 134 insertions(+), 181 deletions(-) delete mode 100644 compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-function-conditional-capture-mutate.expect.md delete mode 100644 compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-function-conditional-capture-mutate.js create mode 100644 compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-capturing-function-conditional-capture-mutate.expect.md create mode 100644 compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-capturing-function-conditional-capture-mutate.js diff --git a/compiler/forget/packages/babel-plugin-react-forget/src/HIR/ValidateFrozenLambdas.ts b/compiler/forget/packages/babel-plugin-react-forget/src/HIR/ValidateFrozenLambdas.ts index 67a51109ef..6edd306c8e 100644 --- a/compiler/forget/packages/babel-plugin-react-forget/src/HIR/ValidateFrozenLambdas.ts +++ b/compiler/forget/packages/babel-plugin-react-forget/src/HIR/ValidateFrozenLambdas.ts @@ -15,11 +15,12 @@ import { FunctionExpression, HIRFunction, IdentifierId, + Place, isMutableEffect, isRefValueType, isUseRefType, } from "./HIR"; -import { eachInstructionValueOperand } from "./visitors"; +import { eachInstructionValueOperand, eachTerminalOperand } from "./visitors"; /** * Various APIs in React take ownership of the values passed to them, such that it is invalid @@ -47,56 +48,78 @@ import { eachInstructionValueOperand } from "./visitors"; * the developer fix the mistake earlier. */ export function validateFrozenLambdas(fn: HIRFunction): void { - const lambdas = new Map(); - const temporaries = new Map(); + const state = new State(); const errors = new CompilerError(); for (const [, block] of fn.body.blocks) { for (const instr of block.instructions) { if (instr.value.kind === "FunctionExpression") { - lambdas.set(instr.lvalue.identifier.id, instr.value); + state.lambdas.set(instr.lvalue.identifier.id, instr.value); } else if (instr.value.kind === "LoadLocal") { const resolvedId = - temporaries.get(instr.value.place.identifier.id) ?? + state.temporaries.get(instr.value.place.identifier.id) ?? instr.value.place.identifier.id; - temporaries.set(instr.lvalue.identifier.id, resolvedId); + state.temporaries.set(instr.lvalue.identifier.id, resolvedId); } else if (instr.value.kind === "StoreLocal") { const resolvedId = - temporaries.get(instr.value.value.identifier.id) ?? + state.temporaries.get(instr.value.value.identifier.id) ?? instr.value.value.identifier.id; - temporaries.set(instr.value.lvalue.place.identifier.id, resolvedId); + state.temporaries.set( + instr.value.lvalue.place.identifier.id, + resolvedId + ); } else { for (const operand of eachInstructionValueOperand(instr.value)) { - if (operand.effect === Effect.Freeze) { - const operandId = - temporaries.get(operand.identifier.id) ?? operand.identifier.id; - const lambda = lambdas.get(operandId); - if ( - lambda !== undefined && - lambda.dependencies.some( - (place) => - isMutableEffect(place.effect, place.loc) && - !isRefValueType(place.identifier) && - !isUseRefType(place.identifier) - ) - ) { - errors.pushErrorDetail( - new CompilerErrorDetail({ - codeframe: null, - description: null, - loc: typeof operand.loc !== "symbol" ? operand.loc : null, - reason: - "Cannot use a mutable function where an immutable value is expected", - severity: ErrorSeverity.InvalidInput, - }) - ); - } + 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 && + lambda.dependencies.some( + (place) => + isMutableEffect(place.effect, place.loc) && + !isRefValueType(place.identifier) && + !isUseRefType(place.identifier) + ) + ) { + return new CompilerErrorDetail({ + codeframe: null, + description: null, + loc: typeof operand.loc !== "symbol" ? operand.loc : null, + reason: + "Cannot use a mutable function where an immutable value is expected", + severity: ErrorSeverity.InvalidInput, + }); + } + } + return null; +} diff --git a/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-func-mutate-2.expect.md b/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-func-mutate-2.expect.md index 6bb60c91d2..dfe78b3b1c 100644 --- a/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-func-mutate-2.expect.md +++ b/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-func-mutate-2.expect.md @@ -10,7 +10,7 @@ function component(a, b) { y.b; }; x(); - return x; + return z; } ``` @@ -33,21 +33,21 @@ function component(a, b) { const y = t0; const c_2 = $[2] !== a; const c_3 = $[3] !== y.b; - let x; + let z; if (c_2 || c_3) { - const z = { a }; - x = function () { + z = { a }; + const x = function () { z.a = 2; y.b; }; x(); $[2] = a; $[3] = y.b; - $[4] = x; + $[4] = z; } else { - x = $[4]; + z = $[4]; } - return x; + return z; } ``` diff --git a/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-func-mutate-2.js b/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-func-mutate-2.js index 31cc3c63de..91b600968d 100644 --- a/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-func-mutate-2.js +++ b/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-func-mutate-2.js @@ -6,5 +6,5 @@ function component(a, b) { y.b; }; x(); - return x; + return z; } diff --git a/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-func-mutate-3.expect.md b/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-func-mutate-3.expect.md index f6e79fc390..7c0a4eef4a 100644 --- a/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-func-mutate-3.expect.md +++ b/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-func-mutate-3.expect.md @@ -9,7 +9,7 @@ function component(a, b) { z.a = 2; y.b; }; - return x; + return z; } ``` @@ -19,43 +19,18 @@ function component(a, b) { ```javascript import { unstable_useMemoCache as useMemoCache } from "react"; function component(a, b) { - const $ = useMemoCache(7); - const c_0 = $[0] !== b; + const $ = useMemoCache(2); + const c_0 = $[0] !== a; let t0; if (c_0) { - t0 = { b }; - $[0] = b; + t0 = { a }; + $[0] = a; $[1] = t0; } else { t0 = $[1]; } - const y = t0; - const c_2 = $[2] !== a; - let t1; - if (c_2) { - t1 = { a }; - $[2] = a; - $[3] = t1; - } else { - t1 = $[3]; - } - const z = t1; - const c_4 = $[4] !== z.a; - const c_5 = $[5] !== y.b; - let t2; - if (c_4 || c_5) { - t2 = function () { - z.a = 2; - y.b; - }; - $[4] = z.a; - $[5] = y.b; - $[6] = t2; - } else { - t2 = $[6]; - } - const x = t2; - return x; + const z = t0; + return z; } ``` diff --git a/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-func-mutate-3.js b/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-func-mutate-3.js index 4468bd6d61..92d4343642 100644 --- a/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-func-mutate-3.js +++ b/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-func-mutate-3.js @@ -5,5 +5,5 @@ function component(a, b) { z.a = 2; y.b; }; - return x; + return z; } diff --git a/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-func-mutate-nested.expect.md b/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-func-mutate-nested.expect.md index b440fd42fa..70ff460545 100644 --- a/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-func-mutate-nested.expect.md +++ b/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-func-mutate-nested.expect.md @@ -8,7 +8,7 @@ function component(a) { y.b.a = 2; }; x(); - return x; + return y; } ``` @@ -20,19 +20,19 @@ import { unstable_useMemoCache as useMemoCache } from "react"; function component(a) { const $ = useMemoCache(2); const c_0 = $[0] !== a; - let x; + let y; if (c_0) { - const y = { b: { a } }; - x = function () { + y = { b: { a } }; + const x = function () { y.b.a = 2; }; x(); $[0] = a; - $[1] = x; + $[1] = y; } else { - x = $[1]; + y = $[1]; } - return x; + return y; } ``` diff --git a/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-func-mutate-nested.js b/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-func-mutate-nested.js index f1ba1cd25f..8d01fd0c10 100644 --- a/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-func-mutate-nested.js +++ b/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-func-mutate-nested.js @@ -4,5 +4,5 @@ function component(a) { y.b.a = 2; }; x(); - return x; + return y; } diff --git a/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-func-mutate.expect.md b/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-func-mutate.expect.md index d4d249b3dc..2953cebf44 100644 --- a/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-func-mutate.expect.md +++ b/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-func-mutate.expect.md @@ -10,7 +10,7 @@ function component(a, b) { y.b; }; x(); - return x; + return z; } ``` @@ -20,34 +20,25 @@ function component(a, b) { ```javascript import { unstable_useMemoCache as useMemoCache } from "react"; function component(a, b) { - const $ = useMemoCache(5); + const $ = useMemoCache(3); const c_0 = $[0] !== a; const c_1 = $[1] !== b; - let x; + let z; if (c_0 || c_1) { - const z = { a }; - const c_3 = $[3] !== b; - let t0; - if (c_3) { - t0 = { b }; - $[3] = b; - $[4] = t0; - } else { - t0 = $[4]; - } - const y = t0; - x = function () { + z = { a }; + const y = { b }; + const x = function () { z.a = 2; y.b; }; x(); $[0] = a; $[1] = b; - $[2] = x; + $[2] = z; } else { - x = $[2]; + z = $[2]; } - return x; + return z; } ``` diff --git a/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-func-mutate.js b/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-func-mutate.js index 3cbb540d16..420879a6f1 100644 --- a/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-func-mutate.js +++ b/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-func-mutate.js @@ -6,5 +6,5 @@ function component(a, b) { y.b; }; x(); - return x; + return z; } diff --git a/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-function-conditional-capture-mutate.expect.md b/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-function-conditional-capture-mutate.expect.md deleted file mode 100644 index af7f7c646d..0000000000 --- a/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-function-conditional-capture-mutate.expect.md +++ /dev/null @@ -1,55 +0,0 @@ - -## Input - -```javascript -function component(a, b) { - let z = { a }; - let y = b; - let x = function () { - if (y) { - mutate(z); - } - }; - return x; -} - -``` - -## Code - -```javascript -import { unstable_useMemoCache as useMemoCache } from "react"; -function component(a, b) { - const $ = useMemoCache(5); - const c_0 = $[0] !== a; - let t0; - if (c_0) { - t0 = { a }; - $[0] = a; - $[1] = t0; - } else { - t0 = $[1]; - } - const z = t0; - const y = b; - const c_2 = $[2] !== y; - const c_3 = $[3] !== z; - let t1; - if (c_2 || c_3) { - t1 = function () { - if (y) { - mutate(z); - } - }; - $[2] = y; - $[3] = z; - $[4] = t1; - } else { - t1 = $[4]; - } - const x = t1; - return x; -} - -``` - \ No newline at end of file diff --git a/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-function-conditional-capture-mutate.js b/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-function-conditional-capture-mutate.js deleted file mode 100644 index 10bac9e4df..0000000000 --- a/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-function-conditional-capture-mutate.js +++ /dev/null @@ -1,10 +0,0 @@ -function component(a, b) { - let z = { a }; - let y = b; - let x = function () { - if (y) { - mutate(z); - } - }; - return x; -} diff --git a/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-nested-member-call.expect.md b/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-nested-member-call.expect.md index e298c2bf9a..ccc47b8511 100644 --- a/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-nested-member-call.expect.md +++ b/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-nested-member-call.expect.md @@ -7,7 +7,7 @@ function component(a) { let x = function () { z.a.a(); }; - return x; + return z; } ``` @@ -17,7 +17,7 @@ function component(a) { ```javascript import { unstable_useMemoCache as useMemoCache } from "react"; function component(a) { - const $ = useMemoCache(6); + const $ = useMemoCache(4); const c_0 = $[0] !== a; let t0; if (c_0) { @@ -37,19 +37,7 @@ function component(a) { t1 = $[3]; } const z = t1; - const c_4 = $[4] !== z.a; - let t2; - if (c_4) { - t2 = function () { - z.a.a(); - }; - $[4] = z.a; - $[5] = t2; - } else { - t2 = $[5]; - } - const x = t2; - return x; + return z; } ``` diff --git a/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-nested-member-call.js b/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-nested-member-call.js index 463211df83..5d0c832556 100644 --- a/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-nested-member-call.js +++ b/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/capturing-nested-member-call.js @@ -3,5 +3,5 @@ function component(a) { let x = function () { z.a.a(); }; - return x; + return z; } diff --git a/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-capturing-function-conditional-capture-mutate.expect.md b/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-capturing-function-conditional-capture-mutate.expect.md new file mode 100644 index 0000000000..72e81ac0a7 --- /dev/null +++ b/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-capturing-function-conditional-capture-mutate.expect.md @@ -0,0 +1,28 @@ + +## Input + +```javascript +function component(a, b) { + let z = { a }; + let y = b; + let x = function () { + if (y) { + // we don't know for sure this mutates, so we should assume + // that there is no mutation so long as `x` isn't called + // during render + maybeMutate(z); + } + }; + return x; +} + +``` + + +## Error + +``` +[ReactForget] InvalidInput: Cannot use a mutable function where an immutable value is expected (12:12) +``` + + \ No newline at end of file diff --git a/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-capturing-function-conditional-capture-mutate.js b/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-capturing-function-conditional-capture-mutate.js new file mode 100644 index 0000000000..6081eec659 --- /dev/null +++ b/compiler/forget/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-capturing-function-conditional-capture-mutate.js @@ -0,0 +1,13 @@ +function component(a, b) { + let z = { a }; + let y = b; + let x = function () { + if (y) { + // we don't know for sure this mutates, so we should assume + // that there is no mutation so long as `x` isn't called + // during render + maybeMutate(z); + } + }; + return x; +}