From edeaaaf38aea54152f93a6136fdef15c49a2fc82 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Tue, 17 Jan 2023 09:40:49 -0800 Subject: [PATCH] Add PropertyCall/ComputedCall instructions Adds `PropertyCall` and `ComputedCall`, which are a combination of CallExpression and PropertyLoad/ComputedLoad, respectively. The goal is to ensure that we correctly model the receiver of a call where the callee is a member expression, and also accurately record scope dependencies in for both the computed and non-computed (property) cases. An alternative that I tried first was to add a `receiver: Place | null` to CallExpression. That works well for HIR construction, but it's then very difficult at codegen time to correctly reconstruct the original call: if the receiver and callee share part of their structure then we can transform back to a non-computed member expression, otherwise it has to be computed. Eg we have to distinguish `a.b.c[d.foo]()` from `a.b.c[a.b.c.foo]()`. Given that our target is high-level code, it seems reasonable to have a higher-level representation for these cases. I'm open to feedback but this feels pretty reasonable in terms of complexity / precision of modeling. --- compiler/forget/src/HIR/BuildHIR.ts | 116 ++++++++++++------ compiler/forget/src/HIR/HIR.ts | 18 ++- compiler/forget/src/HIR/PrintHIR.ts | 12 ++ compiler/forget/src/HIR/visitors.ts | 22 ++++ .../src/Inference/InferMutableLifetimes.ts | 4 +- .../src/Inference/InferReferenceEffects.ts | 43 +++++++ .../ReactiveScopes/CodegenReactiveFunction.ts | 18 +++ .../InferReactiveScopeVariables.ts | 2 + .../PropagateScopeDependencies.ts | 9 +- .../fixtures/hir/component.expect.md | 58 +++++---- .../hir/method-call-computed.expect.md | 70 +++++++++++ .../fixtures/hir/method-call-computed.js | 13 ++ .../hir/method-call-fn-call.expect.md | 54 ++++++++ .../fixtures/hir/method-call-fn-call.js | 10 ++ .../fixtures/hir/method-call.expect.md | 50 ++++++++ .../src/__tests__/fixtures/hir/method-call.js | 9 ++ 16 files changed, 431 insertions(+), 77 deletions(-) create mode 100644 compiler/forget/src/__tests__/fixtures/hir/method-call-computed.expect.md create mode 100644 compiler/forget/src/__tests__/fixtures/hir/method-call-computed.js create mode 100644 compiler/forget/src/__tests__/fixtures/hir/method-call-fn-call.expect.md create mode 100644 compiler/forget/src/__tests__/fixtures/hir/method-call-fn-call.js create mode 100644 compiler/forget/src/__tests__/fixtures/hir/method-call.expect.md create mode 100644 compiler/forget/src/__tests__/fixtures/hir/method-call.js diff --git a/compiler/forget/src/HIR/BuildHIR.ts b/compiler/forget/src/HIR/BuildHIR.ts index d094049dd8..131903a844 100644 --- a/compiler/forget/src/HIR/BuildHIR.ts +++ b/compiler/forget/src/HIR/BuildHIR.ts @@ -902,21 +902,51 @@ function lowerExpression( calleePath.isExpression(), "Call expressions only support callees that are expressions (v8 intrinsics not supported)" ); - const callee = lowerExpressionToPlace(builder, calleePath); - const argPaths = expr.get("arguments"); - const args = argPaths.map((arg) => { - todoInvariant( - arg.isExpression(), - "todo: support non-expression call arguments" + if (calleePath.isMemberExpression()) { + const { object, property, value } = lowerMemberExpression( + builder, + calleePath ); - return lowerExpressionToPlace(builder, arg); - }); - return { - kind: "CallExpression", - callee, - args, - loc: exprLoc, - }; + const args = expr.get("arguments").map((arg) => { + todoInvariant( + arg.isExpression(), + "todo: support non-expression call arguments" + ); + return lowerExpressionToPlace(builder, arg); + }); + if (typeof property === "string") { + return { + kind: "PropertyCall", + receiver: object, + property, + args, + loc: exprLoc, + }; + } else { + return { + kind: "ComputedCall", + receiver: object, + property, + args, + loc: exprLoc, + }; + } + } else { + const callee = lowerExpressionToPlace(builder, calleePath); + const args = expr.get("arguments").map((arg) => { + todoInvariant( + arg.isExpression(), + "todo: support non-expression call arguments" + ); + return lowerExpressionToPlace(builder, arg); + }); + return { + kind: "CallExpression", + callee, + args, + loc: exprLoc, + }; + } } case "BinaryExpression": { const expr = exprPath as NodePath; @@ -1141,31 +1171,7 @@ function lowerExpression( } case "MemberExpression": { const expr = exprPath as NodePath; - const object = lowerExpressionToPlace(builder, expr.get("object")); - invariant(object.kind === "Identifier", "scope cannot appear here"); - const property = expr.get("property"); - let value: InstructionValue; - if (!expr.node.computed) { - todoInvariant(property.isIdentifier(), "Support private names"); - value = { - kind: "PropertyLoad", - object, - property: property.node.name, - loc: exprLoc, - }; - } else { - invariant( - property.isExpression(), - "Expected private names to be non-computed" - ); - const propertyPlace = lowerExpressionToPlace(builder, property); - value = { - kind: "ComputedLoad", - object, - property: propertyPlace, - loc: exprLoc, - }; - } + const { value } = lowerMemberExpression(builder, expr); const place: Place = buildTemporaryPlace(builder, exprLoc); builder.push({ id: makeInstructionId(0), @@ -1258,6 +1264,38 @@ function lowerExpression( } } +function lowerMemberExpression( + builder: HIRBuilder, + expr: NodePath +): { object: Place; property: Place | string; value: InstructionValue } { + const exprLoc = expr.node.loc ?? GeneratedSource; + const object = lowerExpressionToPlace(builder, expr.get("object")); + const property = expr.get("property"); + if (!expr.node.computed) { + todoInvariant(property.isIdentifier(), "Support private names"); + const value: InstructionValue = { + kind: "PropertyLoad", + object: { ...object }, + property: property.node.name, + loc: exprLoc, + }; + return { object, property: property.node.name, value }; + } else { + invariant( + property.isExpression(), + "Expected private names to be non-computed" + ); + const propertyPlace = lowerExpressionToPlace(builder, property); + const value: InstructionValue = { + kind: "ComputedLoad", + object: { ...object }, + property: { ...propertyPlace }, + loc: exprLoc, + }; + return { object, property: propertyPlace, value }; + } +} + function lowerConditional( builder: HIRBuilder, test: Place, diff --git a/compiler/forget/src/HIR/HIR.ts b/compiler/forget/src/HIR/HIR.ts index 15b1f26822..3287f6e60d 100644 --- a/compiler/forget/src/HIR/HIR.ts +++ b/compiler/forget/src/HIR/HIR.ts @@ -355,7 +355,23 @@ export type InstructionData = right: Place; } | { kind: "NewExpression"; callee: Place; args: Array } - | { kind: "CallExpression"; callee: Place; args: Array } + | { + kind: "CallExpression"; + callee: Place; + args: Array; + } + | { + kind: "PropertyCall"; + receiver: Place; + property: string; + args: Array; + } + | { + kind: "ComputedCall"; + receiver: Place; + property: Place; + args: Array; + } | { kind: "UnaryExpression"; operator: string; value: Place } | { kind: "JsxExpression"; diff --git a/compiler/forget/src/HIR/PrintHIR.ts b/compiler/forget/src/HIR/PrintHIR.ts index 6c2725a87a..d21717c8c8 100644 --- a/compiler/forget/src/HIR/PrintHIR.ts +++ b/compiler/forget/src/HIR/PrintHIR.ts @@ -229,6 +229,18 @@ export function printInstructionValue(instrValue: InstructionValue): string { .join(", ")})`; break; } + case "PropertyCall": { + value = `PropertyCall ${printPlace(instrValue.receiver)}.${ + instrValue.property + }(${instrValue.args.map((arg) => printPlace(arg)).join(", ")})`; + break; + } + case "ComputedCall": { + value = `ComputedCall ${printPlace(instrValue.receiver)}[${printPlace( + instrValue.property + )}](${instrValue.args.map((arg) => printPlace(arg)).join(", ")})`; + break; + } case "JSXText": case "Primitive": { value = JSON.stringify(instrValue.value); diff --git a/compiler/forget/src/HIR/visitors.ts b/compiler/forget/src/HIR/visitors.ts index bb795cb38e..54f7508eac 100644 --- a/compiler/forget/src/HIR/visitors.ts +++ b/compiler/forget/src/HIR/visitors.ts @@ -37,6 +37,17 @@ export function* eachInstructionValueOperand( yield instrValue.right; break; } + case "PropertyCall": { + yield instrValue.receiver; + yield* instrValue.args; + break; + } + case "ComputedCall": { + yield instrValue.receiver; + yield instrValue.property; + yield* instrValue.args; + break; + } case "Identifier": { yield instrValue; break; @@ -146,6 +157,17 @@ export function mapInstructionOperands( instrValue.args = instrValue.args.map((arg) => fn(arg)); break; } + case "PropertyCall": { + instrValue.receiver = fn(instrValue.receiver); + instrValue.args = instrValue.args.map((arg) => fn(arg)); + break; + } + case "ComputedCall": { + instrValue.receiver = fn(instrValue.receiver); + instrValue.property = fn(instrValue.property); + instrValue.args = instrValue.args.map((arg) => fn(arg)); + break; + } case "UnaryExpression": { instrValue.value = fn(instrValue.value); break; diff --git a/compiler/forget/src/Inference/InferMutableLifetimes.ts b/compiler/forget/src/Inference/InferMutableLifetimes.ts index 1af0efa1ce..145c2ee8e3 100644 --- a/compiler/forget/src/Inference/InferMutableLifetimes.ts +++ b/compiler/forget/src/Inference/InferMutableLifetimes.ts @@ -6,7 +6,6 @@ */ import invariant from "invariant"; -import { assertExhaustive } from "../Utils/utils"; import { Effect, HIRFunction, @@ -16,6 +15,7 @@ import { } from "../HIR/HIR"; import { printInstruction, printPlace } from "../HIR/PrintHIR"; import { eachInstructionOperand } from "../HIR/visitors"; +import { assertExhaustive } from "../Utils/utils"; /** * For each usage of a value in the given function, determines if the usage @@ -72,7 +72,7 @@ function inferPlace( switch (place.effect) { case Effect.Unknown: { throw new Error( - `Found an unkown place ${printPlace(place)} at ${printInstruction( + `Found an unknown place ${printPlace(place)} at ${printInstruction( instr )}!` ); diff --git a/compiler/forget/src/Inference/InferReferenceEffects.ts b/compiler/forget/src/Inference/InferReferenceEffects.ts index e6c90eb73b..2a17e0e93c 100644 --- a/compiler/forget/src/Inference/InferReferenceEffects.ts +++ b/compiler/forget/src/Inference/InferReferenceEffects.ts @@ -583,6 +583,49 @@ function inferBlock(env: Environment, block: BasicBlock) { valueKind = ValueKind.Immutable; break; } + case "PropertyCall": { + if (!env.isDefined(instrValue.receiver)) { + // TODO @josephsavona: improve handling of globals + const value: InstructionValue = { + kind: "Primitive", + loc: instrValue.loc, + value: undefined, + }; + env.initialize(value, ValueKind.Frozen); + env.define(instrValue.receiver, value); + } + + env.reference(instrValue.receiver, Effect.Mutate); + for (const arg of instrValue.args) { + env.reference(arg, Effect.Mutate); + } + env.initialize(instrValue, ValueKind.Mutable); + env.define(instr.lvalue.place, instrValue); + instr.lvalue.place.effect = Effect.Mutate; + continue; + } + case "ComputedCall": { + if (!env.isDefined(instrValue.receiver)) { + // TODO @josephsavona: improve handling of globals + const value: InstructionValue = { + kind: "Primitive", + loc: instrValue.loc, + value: undefined, + }; + env.initialize(value, ValueKind.Frozen); + env.define(instrValue.receiver, value); + } + + env.reference(instrValue.receiver, Effect.Mutate); + env.reference(instrValue.property, Effect.Read); + for (const arg of instrValue.args) { + env.reference(arg, Effect.Mutate); + } + env.initialize(instrValue, ValueKind.Mutable); + env.define(instr.lvalue.place, instrValue); + instr.lvalue.place.effect = Effect.Mutate; + continue; + } case "PropertyStore": { const effect = isObjectType(instrValue.object.identifier) ? Effect.Store diff --git a/compiler/forget/src/ReactiveScopes/CodegenReactiveFunction.ts b/compiler/forget/src/ReactiveScopes/CodegenReactiveFunction.ts index c73a2c0810..9442b79911 100644 --- a/compiler/forget/src/ReactiveScopes/CodegenReactiveFunction.ts +++ b/compiler/forget/src/ReactiveScopes/CodegenReactiveFunction.ts @@ -518,6 +518,24 @@ function codegenInstructionValue( value = createCallExpression(instrValue.loc, callee, args); break; } + case "PropertyCall": { + const receiver = codegenPlace(temp, instrValue.receiver); + const callee = t.memberExpression( + receiver, + t.identifier(instrValue.property) + ); + const args = instrValue.args.map((arg) => codegenPlace(temp, arg)); + value = createCallExpression(instrValue.loc, callee, args); + break; + } + case "ComputedCall": { + const receiver = codegenPlace(temp, instrValue.receiver); + const property = codegenPlace(temp, instrValue.property); + const callee = t.memberExpression(receiver, property, true); + const args = instrValue.args.map((arg) => codegenPlace(temp, arg)); + value = createCallExpression(instrValue.loc, callee, args); + break; + } case "NewExpression": { const callee = codegenPlace(temp, instrValue.callee); const args = instrValue.args.map((arg) => codegenPlace(temp, arg)); diff --git a/compiler/forget/src/ReactiveScopes/InferReactiveScopeVariables.ts b/compiler/forget/src/ReactiveScopes/InferReactiveScopeVariables.ts index ceca43e1cc..74e809f156 100644 --- a/compiler/forget/src/ReactiveScopes/InferReactiveScopeVariables.ts +++ b/compiler/forget/src/ReactiveScopes/InferReactiveScopeVariables.ts @@ -175,6 +175,8 @@ function mayAllocate(value: InstructionValue): boolean { case "Primitive": { return false; } + case "PropertyCall": + case "ComputedCall": case "PropertyStore": case "ComputedStore": case "ArrayExpression": diff --git a/compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts b/compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts index 4c29c6d41c..cb392edf37 100644 --- a/compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts +++ b/compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts @@ -271,11 +271,10 @@ function visitInstructionValue( value: InstructionValue, lvalue: LValue | null ): void { - for (const operand of eachInstructionValueOperand(value)) { - // check for method invocation, we want to depend on the callee, not the method - if (value.kind === "PropertyLoad" && lvalue !== null) { - context.declareProperty(lvalue.place, value.object, value.property); - } else { + if (value.kind === "PropertyLoad" && lvalue !== null) { + context.declareProperty(lvalue.place, value.object, value.property); + } else { + for (const operand of eachInstructionValueOperand(value)) { context.visitOperand(operand); } } diff --git a/compiler/forget/src/__tests__/fixtures/hir/component.expect.md b/compiler/forget/src/__tests__/fixtures/hir/component.expect.md index 4a0f4c75f6..46382a4f92 100644 --- a/compiler/forget/src/__tests__/fixtures/hir/component.expect.md +++ b/compiler/forget/src/__tests__/fixtures/hir/component.expect.md @@ -39,21 +39,20 @@ function Component(props) { const items = props.items; const maxItems = props.maxItems; const c_0 = $[0] !== maxItems; - const c_1 = $[1] !== items.length; - const c_2 = $[2] !== items.at; + const c_1 = $[1] !== items; let renderedItems; - if (c_0 || c_1 || c_2) { + if (c_0 || c_1) { renderedItems = []; const seen = new Set(); - const c_4 = $[4] !== maxItems; + const c_3 = $[3] !== maxItems; let max; - if (c_4) { + if (c_3) { max = Math.max(0, maxItems); - $[4] = maxItems; - $[5] = max; + $[3] = maxItems; + $[4] = max; } else { - max = $[5]; + max = $[4]; } for (let i = 0; i < items.length; i = i + 1, i) { @@ -76,44 +75,43 @@ function Component(props) { } $[0] = maxItems; - $[1] = items.length; - $[2] = items.at; - $[3] = renderedItems; + $[1] = items; + $[2] = renderedItems; } else { - renderedItems = $[3]; + renderedItems = $[2]; } const count = renderedItems.length; - const c_6 = $[6] !== count; - let t7; + const c_5 = $[5] !== count; + let t6; - if (c_6) { - t7 =

{count} Items

; - $[6] = count; - $[7] = t7; + if (c_5) { + t6 =

{count} Items

; + $[5] = count; + $[6] = t6; } else { - t7 = $[7]; + t6 = $[6]; } - const c_8 = $[8] !== t7; - const c_9 = $[9] !== renderedItems; - let t10; + const c_7 = $[7] !== t6; + const c_8 = $[8] !== renderedItems; + let t9; - if (c_8 || c_9) { - t10 = ( + if (c_7 || c_8) { + t9 = (
- {t7} + {t6} {renderedItems}
); - $[8] = t7; - $[9] = renderedItems; - $[10] = t10; + $[7] = t6; + $[8] = renderedItems; + $[9] = t9; } else { - t10 = $[10]; + t9 = $[9]; } - return t10; + return t9; } ``` diff --git a/compiler/forget/src/__tests__/fixtures/hir/method-call-computed.expect.md b/compiler/forget/src/__tests__/fixtures/hir/method-call-computed.expect.md new file mode 100644 index 0000000000..ada6bcb228 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/hir/method-call-computed.expect.md @@ -0,0 +1,70 @@ + +## Input + +```javascript +function foo(a, b, c) { + // Construct and freeze x, y + const x = makeObject(a); + const y = makeObject(a); +
+ {x} + {y} +
; + + // z should depend on `x`, `y.method`, and `b` + const z = x[y.method](b); + return z; +} + +``` + +## Code + +```javascript +function foo(a, b, c) { + const $ = React.useMemoCache(); + const c_0 = $[0] !== a; + let x; + if (c_0) { + x = makeObject(a); + $[0] = a; + $[1] = x; + } else { + x = $[1]; + } + + const c_2 = $[2] !== a; + let y; + + if (c_2) { + y = makeObject(a); + $[2] = a; + $[3] = y; + } else { + y = $[3]; + } + +
+ {x} + {y} +
; + const c_4 = $[4] !== x; + const c_5 = $[5] !== y.method; + const c_6 = $[6] !== b; + let z; + + if (c_4 || c_5 || c_6) { + z = x[y.method](b); + $[4] = x; + $[5] = y.method; + $[6] = b; + $[7] = z; + } else { + z = $[7]; + } + + return z; +} + +``` + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/hir/method-call-computed.js b/compiler/forget/src/__tests__/fixtures/hir/method-call-computed.js new file mode 100644 index 0000000000..5aaf78027e --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/hir/method-call-computed.js @@ -0,0 +1,13 @@ +function foo(a, b, c) { + // Construct and freeze x, y + const x = makeObject(a); + const y = makeObject(a); +
+ {x} + {y} +
; + + // z should depend on `x`, `y.method`, and `b` + const z = x[y.method](b); + return z; +} diff --git a/compiler/forget/src/__tests__/fixtures/hir/method-call-fn-call.expect.md b/compiler/forget/src/__tests__/fixtures/hir/method-call-fn-call.expect.md new file mode 100644 index 0000000000..891a0a2134 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/hir/method-call-fn-call.expect.md @@ -0,0 +1,54 @@ + +## Input + +```javascript +function foo(a, b, c) { + // Construct and freeze x + const x = makeObject(a); +
{x}
; + + // y should depend on `x` and `b` + const method = x.method; + const y = method.call(x, b); + return y; +} + +``` + +## Code + +```javascript +function foo(a, b, c) { + const $ = React.useMemoCache(); + const c_0 = $[0] !== a; + let x; + if (c_0) { + x = makeObject(a); + $[0] = a; + $[1] = x; + } else { + x = $[1]; + } + +
{x}
; + const method = x.method; + const c_2 = $[2] !== method; + const c_3 = $[3] !== x; + const c_4 = $[4] !== b; + let y; + + if (c_2 || c_3 || c_4) { + y = method.call(x, b); + $[2] = method; + $[3] = x; + $[4] = b; + $[5] = y; + } else { + y = $[5]; + } + + return y; +} + +``` + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/hir/method-call-fn-call.js b/compiler/forget/src/__tests__/fixtures/hir/method-call-fn-call.js new file mode 100644 index 0000000000..5289caae26 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/hir/method-call-fn-call.js @@ -0,0 +1,10 @@ +function foo(a, b, c) { + // Construct and freeze x + const x = makeObject(a); +
{x}
; + + // y should depend on `x` and `b` + const method = x.method; + const y = method.call(x, b); + return y; +} diff --git a/compiler/forget/src/__tests__/fixtures/hir/method-call.expect.md b/compiler/forget/src/__tests__/fixtures/hir/method-call.expect.md new file mode 100644 index 0000000000..8221e84c9d --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/hir/method-call.expect.md @@ -0,0 +1,50 @@ + +## Input + +```javascript +function foo(a, b, c) { + // Construct and freeze x + const x = makeObject(a); +
{x}
; + + // y should depend on `x` and `b` + const y = x.foo(b); + return y; +} + +``` + +## Code + +```javascript +function foo(a, b, c) { + const $ = React.useMemoCache(); + const c_0 = $[0] !== a; + let x; + if (c_0) { + x = makeObject(a); + $[0] = a; + $[1] = x; + } else { + x = $[1]; + } + +
{x}
; + const c_2 = $[2] !== x; + const c_3 = $[3] !== b; + let y; + + if (c_2 || c_3) { + y = x.foo(b); + $[2] = x; + $[3] = b; + $[4] = y; + } else { + y = $[4]; + } + + return y; +} + +``` + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/hir/method-call.js b/compiler/forget/src/__tests__/fixtures/hir/method-call.js new file mode 100644 index 0000000000..8978b40850 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/hir/method-call.js @@ -0,0 +1,9 @@ +function foo(a, b, c) { + // Construct and freeze x + const x = makeObject(a); +
{x}
; + + // y should depend on `x` and `b` + const y = x.foo(b); + return y; +}