From ee77d91ca2a3862cd60b8f59391a9a13241607ea Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Thu, 7 Sep 2023 16:32:55 -0700 Subject: [PATCH] TryStatement: handle catch clause params It's possible that the value thrown during a `try` block actually is a reference to some value defined outside the scope of the try block. If the catch clause param is also mutated, that means the mutable range of the variable would have to include the entire try/catch. We handle this by emitting a DeclareLocal temporary for the catch param prior to the try/catch. If it is modified during the catch block, that will extend its mutable range to cover the full try/catch. If any values are mutated inside the try, their range will also (naturally) extend around the full try/catch block. These ranges will overlap and be merged, ensuring that we capture the possibility that the value is mutated via the catch param. See unit test. --- .../src/HIR/BuildHIR.ts | 55 ++++++++++++++++--- .../babel-plugin-react-forget/src/HIR/HIR.ts | 6 ++ .../src/HIR/PrintHIR.ts | 3 + .../src/HIR/visitors.ts | 10 +++- .../ReactiveScopes/BuildReactiveFunction.ts | 1 + .../ReactiveScopes/CodegenReactiveFunction.ts | 17 +++++- .../try-catch-with-catch-param.expect.md | 45 +++++++++++++++ .../compiler/try-catch-with-catch-param.js | 12 ++++ .../packages/sprout/src/SproutTodoFilter.ts | 1 + 9 files changed, 140 insertions(+), 10 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/try-catch-with-catch-param.expect.md create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/try-catch-with-catch-param.js diff --git a/compiler/packages/babel-plugin-react-forget/src/HIR/BuildHIR.ts b/compiler/packages/babel-plugin-react-forget/src/HIR/BuildHIR.ts index 6d15722873..48f925f673 100644 --- a/compiler/packages/babel-plugin-react-forget/src/HIR/BuildHIR.ts +++ b/compiler/packages/babel-plugin-react-forget/src/HIR/BuildHIR.ts @@ -941,7 +941,52 @@ function lowerStatement( }); return; } + if (stmt.get("finalizer").node != null) { + builder.errors.push({ + reason: `(BuildHIR::lowerStatement) Handle TryStatement with a finalizer ('finally') clause`, + severity: ErrorSeverity.Todo, + loc: stmt.node.loc ?? null, + suggestions: null, + }); + } + + const handlerBindingPath = handlerPath.get("param"); + let handlerBinding: { + place: Place; + path: NodePath; + } | null = null; + if (handlerBindingPath.node != null && handlerBindingPath.hasNode()) { + const place: Place = { + kind: "Identifier", + identifier: builder.makeTemporary(), + effect: Effect.Unknown, + loc: handlerBindingPath.node.loc ?? GeneratedSource, + }; + lowerValueToTemporary(builder, { + kind: "DeclareLocal", + lvalue: { + kind: InstructionKind.Catch, + place: { ...place }, + }, + loc: handlerBindingPath.node.loc ?? GeneratedSource, + }); + + handlerBinding = { + path: handlerBindingPath, + place, + }; + } + const handler = builder.enter("block", (_blockId) => { + if (handlerBinding !== null) { + lowerAssignment( + builder, + handlerBinding.path.node.loc ?? GeneratedSource, + InstructionKind.Catch, + handlerBinding.path, + { ...handlerBinding.place } + ); + } lowerStatement(builder, handlerPath.get("body")); return { kind: "goto", @@ -951,14 +996,6 @@ function lowerStatement( loc: handlerPath.node.loc ?? GeneratedSource, }; }); - if (stmt.get("finalizer").node != null) { - builder.errors.push({ - reason: `(BuildHIR::lowerStatement) Handle TryStatement with a finalizer ('finally') clause`, - severity: ErrorSeverity.Todo, - loc: stmt.node.loc ?? null, - suggestions: null, - }); - } const block = builder.enter("block", (_blockId) => { const block = stmt.get("block"); @@ -978,6 +1015,8 @@ function lowerStatement( { kind: "try", block, + handlerBinding: + handlerBinding !== null ? { ...handlerBinding.place } : null, handler, fallthrough: continuationBlock.id, id: makeInstructionId(0), diff --git a/compiler/packages/babel-plugin-react-forget/src/HIR/HIR.ts b/compiler/packages/babel-plugin-react-forget/src/HIR/HIR.ts index 9e1a51b014..5216fd625d 100644 --- a/compiler/packages/babel-plugin-react-forget/src/HIR/HIR.ts +++ b/compiler/packages/babel-plugin-react-forget/src/HIR/HIR.ts @@ -213,6 +213,7 @@ export type ReactiveLabelTerminal = { export type ReactiveTryTerminal = { kind: "try"; block: ReactiveBlock; + handlerBinding: Place | null; handler: ReactiveBlock; id: InstructionId; }; @@ -457,6 +458,7 @@ export type SequenceTerminal = { export type TryTerminal = { kind: "try"; block: BlockId; + handlerBinding: Place | null; handler: BlockId; // TODO: support `finally` fallthrough: BlockId | null; @@ -547,6 +549,10 @@ export enum InstructionKind { * assing a new value to a let binding */ Reassign = "Reassign", + /** + * catch clause binding + */ + Catch = "Catch", } function _staticInvariantInstructionValueHasLocation( diff --git a/compiler/packages/babel-plugin-react-forget/src/HIR/PrintHIR.ts b/compiler/packages/babel-plugin-react-forget/src/HIR/PrintHIR.ts index 2405a1b82c..534801c659 100644 --- a/compiler/packages/babel-plugin-react-forget/src/HIR/PrintHIR.ts +++ b/compiler/packages/babel-plugin-react-forget/src/HIR/PrintHIR.ts @@ -590,6 +590,9 @@ export function printLValue(lval: LValue): string { case InstructionKind.Reassign: { return `Reassign ${lvalue}`; } + case InstructionKind.Catch: { + return `Catch ${lvalue}`; + } default: { assertExhaustive(lval.kind, `Unexpected lvalue kind '${lval.kind}'`); } diff --git a/compiler/packages/babel-plugin-react-forget/src/HIR/visitors.ts b/compiler/packages/babel-plugin-react-forget/src/HIR/visitors.ts index d92e5eb329..d263b75077 100644 --- a/compiler/packages/babel-plugin-react-forget/src/HIR/visitors.ts +++ b/compiler/packages/babel-plugin-react-forget/src/HIR/visitors.ts @@ -750,6 +750,7 @@ export function mapTerminalSuccessors( return { kind: "try", block, + handlerBinding: terminal.handlerBinding, handler, fallthrough, id: makeInstructionId(0), @@ -1001,7 +1002,14 @@ export function mapTerminalOperands( terminal.value = fn(terminal.value); break; } - case "try": + case "try": { + if (terminal.handlerBinding !== null) { + terminal.handlerBinding = fn(terminal.handlerBinding); + } else { + terminal.handlerBinding = null; + } + break; + } case "maybe-throw": case "sequence": case "label": diff --git a/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/BuildReactiveFunction.ts b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/BuildReactiveFunction.ts index 626233a837..3806fc2304 100644 --- a/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/BuildReactiveFunction.ts +++ b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/BuildReactiveFunction.ts @@ -676,6 +676,7 @@ class Driver { terminal: { kind: "try", block, + handlerBinding: terminal.handlerBinding, handler, id: terminal.id, }, diff --git a/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/CodegenReactiveFunction.ts b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/CodegenReactiveFunction.ts index 1af001f1ed..912cbee894 100644 --- a/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/CodegenReactiveFunction.ts +++ b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/CodegenReactiveFunction.ts @@ -454,6 +454,13 @@ function codegenTerminal( loc: iterableItem.loc, suggestions: null, }); + case InstructionKind.Catch: + CompilerError.invariant(false, { + reason: "Unexpected catch variable as for-of collection", + description: null, + loc: iterableItem.loc, + suggestions: null, + }); default: assertExhaustive( iterableItem.value.lvalue.kind, @@ -516,9 +523,14 @@ function codegenTerminal( return codegenBlock(cx, terminal.block); } case "try": { + let catchParam = null; + if (terminal.handlerBinding !== null) { + catchParam = convertIdentifier(terminal.handlerBinding.identifier); + cx.temp.set(terminal.handlerBinding.identifier.id, null); + } return t.tryStatement( codegenBlock(cx, terminal.block), - t.catchClause(null, codegenBlock(cx, terminal.handler)) + t.catchClause(catchParam, codegenBlock(cx, terminal.handler)) ); } default: { @@ -638,6 +650,9 @@ function codegenInstructionNullable( return createExpressionStatement(instr.loc, expr); } } + case InstructionKind.Catch: { + return t.emptyStatement(); + } default: { assertExhaustive(kind, `Unexpected instruction kind '${kind}'`); } diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/try-catch-with-catch-param.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/try-catch-with-catch-param.expect.md new file mode 100644 index 0000000000..cc881c39b1 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/try-catch-with-catch-param.expect.md @@ -0,0 +1,45 @@ + +## Input + +```javascript +function Component(props) { + let x = []; + try { + // foo could throw its argument... + foo(x); + } catch (e) { + // ... in which case this could be mutating `x`! + e.push(null); + return e; + } + return x; +} + +``` + +## Code + +```javascript +import { unstable_useMemoCache as useMemoCache } from "react"; +function Component(props) { + const $ = useMemoCache(1); + let x; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + x = []; + try { + foo(x); + } catch (t22) { + const e = t22; + + e.push(null); + return e; + } + $[0] = x; + } else { + x = $[0]; + } + return x; +} + +``` + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/try-catch-with-catch-param.js b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/try-catch-with-catch-param.js new file mode 100644 index 0000000000..cdaf1fe0fb --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/try-catch-with-catch-param.js @@ -0,0 +1,12 @@ +function Component(props) { + let x = []; + try { + // foo could throw its argument... + foo(x); + } catch (e) { + // ... in which case this could be mutating `x`! + e.push(null); + return e; + } + return x; +} diff --git a/compiler/packages/sprout/src/SproutTodoFilter.ts b/compiler/packages/sprout/src/SproutTodoFilter.ts index 7398ebbcd8..5206dd62cb 100644 --- a/compiler/packages/sprout/src/SproutTodoFilter.ts +++ b/compiler/packages/sprout/src/SproutTodoFilter.ts @@ -416,6 +416,7 @@ const skipFilter = new Set([ "try-catch-within-mutable-range", "try-catch", "try-catch-with-return", + "try-catch-with-catch-param", // TODO: 🌲 "forest-basic",