From 9a12e8ba58a14288359036b130c1e86ae579da84 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Tue, 2 May 2023 15:00:11 -0700 Subject: [PATCH] Support OptionalCallExpression as LHS of LogicalExpression MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A common idiom is to map over some possibly-missing list of items from a data payload and fall back to an empty array: ```javascript const renderedItems = data?.items?.map(renderItem) ?? []; ``` The way we were lowering OptionalCallExpression meant that in this case, we'd end up with an OptionalCallTerminal as the terminal of the logical expression's test block, which violates our internal invariant. Logical test blocks must end in a Branch! This PR fixes the immediate issue, which is that the callee - in this case `data?.items?.map` — was being lowered prior to the OptionalCallTerminal instead of inside its test block. Changing that fixes the shape of the IR and makes this example work. As part of investigating this I realized that the way I originally handled lowering of optional call isn't quite right. The difference isn't observable unless we did more sophisticated DCE but we don't correctly model the fact that if `data.items` is null that the `map()` call won't occur. That is technically fine bc we do model the fact that the `map()` call is conditional, and notably its arguments are only conditional dependencies. So it's good enough. But in a follow-up I'll change to model the fact that `data.items` is null, that the map call isn't reachable at all. --- compiler/forget/src/HIR/BuildHIR.ts | 107 +++++++++--------- compiler/forget/src/HIR/HIRBuilder.ts | 28 +++-- .../ReactiveScopes/BuildReactiveFunction.ts | 16 ++- .../ReactiveScopes/PrintReactiveFunction.ts | 8 ++ .../ReactiveScopes/PruneNonEscapingScopes.ts | 5 +- .../compiler/optional-call-chained.expect.md | 32 ++++++ .../compiler/optional-call-chained.js | 4 + .../compiler/optional-call-logical.expect.md | 21 ++++ .../compiler/optional-call-logical.js | 4 + .../compiler/optional-call-simple.expect.md | 30 +++++ .../fixtures/compiler/optional-call-simple.js | 3 + 11 files changed, 195 insertions(+), 63 deletions(-) create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/optional-call-chained.expect.md create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/optional-call-chained.js create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/optional-call-logical.expect.md create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/optional-call-logical.js create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/optional-call-simple.expect.md create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/optional-call-simple.js diff --git a/compiler/forget/src/HIR/BuildHIR.ts b/compiler/forget/src/HIR/BuildHIR.ts index eab63c1042..063306d93b 100644 --- a/compiler/forget/src/HIR/BuildHIR.ts +++ b/compiler/forget/src/HIR/BuildHIR.ts @@ -1093,6 +1093,29 @@ function lowerExpression( const loc = expr.node.loc ?? GeneratedSource; const place = buildTemporaryPlace(builder, loc); const continuationBlock = builder.reserve(builder.currentBlockKind()); + const consequent = builder.reserve("value"); + + // block to evaluate if the callee is null/undefined, this sets the result of the call to undefined. + const alternate = builder.enter("value", () => { + const temp = lowerValueToTemporary(builder, { + kind: "Primitive", + value: undefined, + loc, + }); + lowerValueToTemporary(builder, { + kind: "StoreLocal", + lvalue: { kind: InstructionKind.Const, place: { ...place } }, + value: { ...temp }, + loc, + }); + return { + kind: "goto", + variant: GotoVariant.Break, + block: continuationBlock.id, + id: makeInstructionId(0), + loc, + }; + }); // Lower the callee in the current block: the callee is always unconditionally evaluated // The test block's branch will test on this value to determine whether to evaluate the call (consequent) @@ -1100,27 +1123,42 @@ function lowerExpression( let callee: | { kind: "CallExpression"; callee: Place } | { kind: "MethodCall"; receiver: Place; property: Place }; - if ( - calleePath.isMemberExpression() || - calleePath.isOptionalMemberExpression() - ) { - const memberExpr = lowerMemberExpression(builder, calleePath); - const propertyPlace = lowerValueToTemporary(builder, memberExpr.value); - callee = { - kind: "MethodCall", - receiver: memberExpr.object, - property: propertyPlace, + const testBlock = builder.enter("value", () => { + if ( + calleePath.isMemberExpression() || + calleePath.isOptionalMemberExpression() + ) { + const memberExpr = lowerMemberExpression(builder, calleePath); + const propertyPlace = lowerValueToTemporary( + builder, + memberExpr.value + ); + callee = { + kind: "MethodCall", + receiver: memberExpr.object, + property: propertyPlace, + }; + } else { + callee = { + kind: "CallExpression", + callee: lowerExpressionToTemporary(builder, calleePath), + }; + } + const testPlace = + callee.kind === "CallExpression" ? callee.callee : callee.property; + return { + kind: "branch", + test: { ...testPlace }, + consequent: consequent.id, + alternate, + id: makeInstructionId(0), + loc, }; - } else { - callee = { - kind: "CallExpression", - callee: lowerExpressionToTemporary(builder, calleePath), - }; - } + }); // block to evaluate if the callee is non-null/undefined. arguments are lowered in this block to preserve // the semantic of conditional evaluation depending on the callee - const consequent = builder.enter("value", () => { + builder.enterReserved(consequent, () => { const args = lowerArguments(builder, expr.get("arguments")); const temp = buildTemporaryPlace(builder, loc); if (callee.kind === "CallExpression") { @@ -1164,41 +1202,6 @@ function lowerExpression( }; }); - // block to evaluate if the callee is null/undefined, this sets the result of the call to undefined. - const alternate = builder.enter("value", () => { - const temp = lowerValueToTemporary(builder, { - kind: "Primitive", - value: undefined, - loc, - }); - lowerValueToTemporary(builder, { - kind: "StoreLocal", - lvalue: { kind: InstructionKind.Const, place: { ...place } }, - value: { ...temp }, - loc, - }); - return { - kind: "goto", - variant: GotoVariant.Break, - block: continuationBlock.id, - id: makeInstructionId(0), - loc, - }; - }); - - const testBlock = builder.enter("value", () => { - const testPlace = - callee.kind === "CallExpression" ? callee.callee : callee.property; - return { - kind: "branch", - test: { ...testPlace }, - consequent, - alternate, - id: makeInstructionId(0), - loc, - }; - }); - builder.terminateWithContinuation( { kind: "optional-call", diff --git a/compiler/forget/src/HIR/HIRBuilder.ts b/compiler/forget/src/HIR/HIRBuilder.ts index c5ff24bd03..aa5dc76b6f 100644 --- a/compiler/forget/src/HIR/HIRBuilder.ts +++ b/compiler/forget/src/HIR/HIRBuilder.ts @@ -340,16 +340,13 @@ export default class HIRBuilder { } /** - * Create a new block and execute the provided callback with the new block - * set as the current, resetting to the previously active block upon exit. - * The lambda must return a terminal node, which is used to terminate the - * newly constructed block. + * Sets the given wip block as the current block, executes the provided callback to populate the block + * up to its terminal, and then resets the previous actively block. */ - enter(nextBlockKind: BlockKind, fn: (blockId: BlockId) => Terminal): BlockId { + enterReserved(wip: WipBlock, fn: () => Terminal): void { const current = this.#current; - const nextId = this.#env.nextBlockId; - this.#current = newBlock(nextId, nextBlockKind); - const terminal = fn(nextId); + this.#current = wip; + const terminal = fn(); const { id: blockId, kind, instructions } = this.#current; this.#completed.set(blockId, { kind, @@ -360,7 +357,20 @@ export default class HIRBuilder { phis: new Set(), }); this.#current = current; - return nextId; + } + + /** + * Create a new block and execute the provided callback with the new block + * set as the current, resetting to the previously active block upon exit. + * The lambda must return a terminal node, which is used to terminate the + * newly constructed block. + */ + enter(nextBlockKind: BlockKind, fn: (blockId: BlockId) => Terminal): BlockId { + const wip = this.reserve(nextBlockKind); + this.enterReserved(wip, () => { + return fn(wip.id); + }); + return wip.id; } label(label: string, breakBlock: BlockId, fn: () => T): T { diff --git a/compiler/forget/src/ReactiveScopes/BuildReactiveFunction.ts b/compiler/forget/src/ReactiveScopes/BuildReactiveFunction.ts index bb373124da..ac14e933ee 100644 --- a/compiler/forget/src/ReactiveScopes/BuildReactiveFunction.ts +++ b/compiler/forget/src/ReactiveScopes/BuildReactiveFunction.ts @@ -768,12 +768,26 @@ class Driver { testBlock.terminal.consequent, terminal.loc ); + const call: ReactiveSequenceValue = { + kind: "SequenceExpression", + instructions: [ + { + id: test.id, + loc: testBlock.terminal.loc, + lvalue: test.place, + value: test.value, + }, + ], + id: consequent.id, + value: consequent.value, + loc: terminal.loc, + }; return { place: { ...consequent.place }, value: { kind: "OptionalCall", optional: terminal.optional, - call: consequent.value, + call: call, id: terminal.id, loc: terminal.loc, }, diff --git a/compiler/forget/src/ReactiveScopes/PrintReactiveFunction.ts b/compiler/forget/src/ReactiveScopes/PrintReactiveFunction.ts index ee3c490f69..dc50874395 100644 --- a/compiler/forget/src/ReactiveScopes/PrintReactiveFunction.ts +++ b/compiler/forget/src/ReactiveScopes/PrintReactiveFunction.ts @@ -151,6 +151,14 @@ function printReactiveValue(writer: Writer, value: ReactiveValue): void { }); break; } + case "OptionalCall": { + writer.append(`OptionalCall optional=${value.optional}`); + writer.newline(); + writer.indented(() => { + printReactiveValue(writer, value.call); + }); + break; + } default: { writer.append(printInstructionValue(value)); } diff --git a/compiler/forget/src/ReactiveScopes/PruneNonEscapingScopes.ts b/compiler/forget/src/ReactiveScopes/PruneNonEscapingScopes.ts index a3f9464c9c..1cdd9bd3e8 100644 --- a/compiler/forget/src/ReactiveScopes/PruneNonEscapingScopes.ts +++ b/compiler/forget/src/ReactiveScopes/PruneNonEscapingScopes.ts @@ -262,7 +262,10 @@ function computeMemoizedIdentifiers(state: State): Set { // Visit an identifier, optionally forcing it to be memoized function visit(id: IdentifierId, forceMemoize: boolean = false): boolean { const node = state.identifiers.get(id); - invariant(node !== undefined, "Expected a node for all identifiers"); + invariant( + node !== undefined, + `Expected a node for all identifiers, none found for '${id}'` + ); if (node.seen) { return node.memoized; } diff --git a/compiler/forget/src/__tests__/fixtures/compiler/optional-call-chained.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/optional-call-chained.expect.md new file mode 100644 index 0000000000..1732d047b3 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/optional-call-chained.expect.md @@ -0,0 +1,32 @@ + +## Input + +```javascript +function Component(props) { + const object = makeObject(); + return object.a?.b?.c(props); +} + +``` + +## Code + +```javascript +import { unstable_useMemoCache as useMemoCache } from "react"; +function Component(props) { + const $ = useMemoCache(2); + const c_0 = $[0] !== props; + let t0; + if (c_0) { + const object = makeObject(); + t0 = object.a?.b?.c(props); + $[0] = props; + $[1] = t0; + } else { + t0 = $[1]; + } + return t0; +} + +``` + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/optional-call-chained.js b/compiler/forget/src/__tests__/fixtures/compiler/optional-call-chained.js new file mode 100644 index 0000000000..6f41a207d5 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/optional-call-chained.js @@ -0,0 +1,4 @@ +function Component(props) { + const object = makeObject(); + return object.a?.b?.c(props); +} diff --git a/compiler/forget/src/__tests__/fixtures/compiler/optional-call-logical.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/optional-call-logical.expect.md new file mode 100644 index 0000000000..f0b8d40ff3 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/optional-call-logical.expect.md @@ -0,0 +1,21 @@ + +## Input + +```javascript +function Component(props) { + const item = useFragment(graphql`...`, props.item); + return item.items?.map((item) => renderItem(item)) ?? []; +} + +``` + +## Code + +```javascript +function Component(props) { + const item = useFragment(graphql`...`, props.item); + return item.items?.map((item_0) => renderItem(item_0)) ?? []; +} + +``` + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/optional-call-logical.js b/compiler/forget/src/__tests__/fixtures/compiler/optional-call-logical.js new file mode 100644 index 0000000000..41a1d86734 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/optional-call-logical.js @@ -0,0 +1,4 @@ +function Component(props) { + const item = useFragment(graphql`...`, props.item); + return item.items?.map((item) => renderItem(item)) ?? []; +} diff --git a/compiler/forget/src/__tests__/fixtures/compiler/optional-call-simple.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/optional-call-simple.expect.md new file mode 100644 index 0000000000..62e8b17cfd --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/optional-call-simple.expect.md @@ -0,0 +1,30 @@ + +## Input + +```javascript +function Component(props) { + return foo?.(props); +} + +``` + +## Code + +```javascript +import { unstable_useMemoCache as useMemoCache } from "react"; +function Component(props) { + const $ = useMemoCache(2); + const c_0 = $[0] !== props; + let t0; + if (c_0) { + t0 = foo?.(props); + $[0] = props; + $[1] = t0; + } else { + t0 = $[1]; + } + return t0; +} + +``` + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/optional-call-simple.js b/compiler/forget/src/__tests__/fixtures/compiler/optional-call-simple.js new file mode 100644 index 0000000000..6590cb5b78 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/optional-call-simple.js @@ -0,0 +1,3 @@ +function Component(props) { + return foo?.(props); +}