From e192930cd4b4d8e19627db1c33dbc313767f03d8 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Wed, 3 May 2023 17:10:27 -0700 Subject: [PATCH] Remove unnecessary `optional` property on loads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Now that _all_ optional expression types use the new representation, the optionality of all PropertyLoad and ComputedLoad is modeled via control flow (in HIR) and the structure of OptionalExpression (in ReactiveFunction). Thus we no longer need the `optional` properties on these load instructions — they're optional if they're part of an OptionalExpression. --- compiler/forget/src/HIR/BuildHIR.ts | 7 ----- compiler/forget/src/HIR/HIR.ts | 2 -- compiler/forget/src/HIR/PrintHIR.ts | 12 +++---- .../src/Optimization/ConstantPropagation.ts | 10 ------ .../ReactiveScopes/CodegenReactiveFunction.ts | 31 ++++--------------- .../PropagateScopeDependencies.ts | 22 ++++--------- 6 files changed, 18 insertions(+), 66 deletions(-) diff --git a/compiler/forget/src/HIR/BuildHIR.ts b/compiler/forget/src/HIR/BuildHIR.ts index 2539141fa2..12880fdac3 100644 --- a/compiler/forget/src/HIR/BuildHIR.ts +++ b/compiler/forget/src/HIR/BuildHIR.ts @@ -2157,7 +2157,6 @@ function lowerMemberExpression( object: { ...object }, property: propertyNode.node.name, loc: exprLoc, - optional: expr.node.optional ?? false, }; return { object, property: propertyNode.node.name, value }; } else { @@ -2177,26 +2176,21 @@ function lowerMemberExpression( }, }; } - let optional; let property: Place; - // See "PropertyLoad" for the difference between optionalMemberExpr() // and node.optional here if (expr.isOptionalMemberExpression()) { // if expr is in an optional chain, evaluation of `property` is // conditional on whether expr is nullish property = lowerReorderableExpression(builder, propertyNode); - optional = expr.node.optional ?? false; } else { property = lowerExpressionToTemporary(builder, propertyNode); - optional = false; } const value: InstructionValue = { kind: "ComputedLoad", object: { ...object }, property: { ...property }, loc: exprLoc, - optional, }; return { object, property, value }; } @@ -2280,7 +2274,6 @@ function lowerJsxMemberExpression( kind: "PropertyLoad", object: objectPlace, property, - optional: false, loc, }); } diff --git a/compiler/forget/src/HIR/HIR.ts b/compiler/forget/src/HIR/HIR.ts index 1b6dcaefca..a0f3c62e47 100644 --- a/compiler/forget/src/HIR/HIR.ts +++ b/compiler/forget/src/HIR/HIR.ts @@ -645,7 +645,6 @@ export type InstructionValue = kind: "PropertyLoad"; object: Place; property: string; - optional: boolean; loc: SourceLocation; } // `delete object.property` @@ -669,7 +668,6 @@ export type InstructionValue = kind: "ComputedLoad"; object: Place; property: Place; - optional: boolean; loc: SourceLocation; } // `delete object[property]` diff --git a/compiler/forget/src/HIR/PrintHIR.ts b/compiler/forget/src/HIR/PrintHIR.ts index 53840e5861..7b3c1a55a1 100644 --- a/compiler/forget/src/HIR/PrintHIR.ts +++ b/compiler/forget/src/HIR/PrintHIR.ts @@ -367,9 +367,9 @@ export function printInstructionValue(instrValue: ReactiveValue): string { break; } case "PropertyLoad": { - value = `PropertyLoad ${printPlace(instrValue.object)}${ - instrValue.optional ? "?" : "" - }.${instrValue.property}`; + value = `PropertyLoad ${printPlace(instrValue.object)}.${ + instrValue.property + }`; break; } case "PropertyStore": { @@ -385,9 +385,9 @@ export function printInstructionValue(instrValue: ReactiveValue): string { break; } case "ComputedLoad": { - value = `ComputedLoad ${printPlace(instrValue.object)}${ - instrValue.optional ? "?" : "" - }[${printPlace(instrValue.property)}]`; + value = `ComputedLoad ${printPlace(instrValue.object)}[${printPlace( + instrValue.property + )}]`; break; } case "ComputedStore": { diff --git a/compiler/forget/src/Optimization/ConstantPropagation.ts b/compiler/forget/src/Optimization/ConstantPropagation.ts index ba7bbc98e0..f2aa229f9c 100644 --- a/compiler/forget/src/Optimization/ConstantPropagation.ts +++ b/compiler/forget/src/Optimization/ConstantPropagation.ts @@ -6,7 +6,6 @@ */ import { isValidIdentifier } from "@babel/types"; -import invariant from "invariant"; import { GotoVariant, HIRFunction, @@ -180,16 +179,7 @@ function evaluateInstruction( loc: value.loc, property: property.value, object: value.object, - optional: value.optional, }; - // Future-proofing: when we add support for optional computed properties, - // we'll need to copy the value here - if ((value as any).optional) { - invariant( - false, - "TODO: translate optional computed load to optional property load" - ); - } instr.value = nextValue; } return null; diff --git a/compiler/forget/src/ReactiveScopes/CodegenReactiveFunction.ts b/compiler/forget/src/ReactiveScopes/CodegenReactiveFunction.ts index 129986cb70..c3a8a5acfc 100644 --- a/compiler/forget/src/ReactiveScopes/CodegenReactiveFunction.ts +++ b/compiler/forget/src/ReactiveScopes/CodegenReactiveFunction.ts @@ -853,21 +853,11 @@ function codegenInstructionValue( const object = codegenPlace(cx, instrValue.object); // We currently only lower single chains of optional memberexpr. // (See BuildHIR.ts for more detail.) - if (t.isOptionalMemberExpression(object) || instrValue.optional) { - value = t.optionalMemberExpression( - object, - t.identifier(instrValue.property), - undefined, - instrValue.optional - ); - } else { - value = t.memberExpression( - object, - t.identifier(instrValue.property), - undefined, - instrValue.optional - ); - } + value = t.memberExpression( + object, + t.identifier(instrValue.property), + undefined + ); break; } case "PropertyDelete": { @@ -895,16 +885,7 @@ function codegenInstructionValue( case "ComputedLoad": { const object = codegenPlace(cx, instrValue.object); const property = codegenPlace(cx, instrValue.property); - if (t.isOptionalMemberExpression(object) || instrValue.optional) { - value = t.optionalMemberExpression( - object, - property, - true, - instrValue.optional - ); - } else { - value = t.memberExpression(object, property, true, instrValue.optional); - } + value = t.memberExpression(object, property, true); break; } case "ComputedDelete": { diff --git a/compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts b/compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts index a00f9f927f..7aaf4a980a 100644 --- a/compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts +++ b/compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts @@ -306,13 +306,8 @@ class Context { return objectDependency; } - declareProperty( - lvalue: Place, - object: Place, - property: string, - isConditional: boolean - ): void { - const nextDependency = this.#getProperty(object, property, isConditional); + declareProperty(lvalue: Place, object: Place, property: string): void { + const nextDependency = this.#getProperty(object, property, false); this.#properties.set(lvalue.identifier, nextDependency); } @@ -363,8 +358,8 @@ class Context { this.visitDependency(dependency); } - visitProperty(object: Place, property: string, isConditional: boolean): void { - const nextDependency = this.#getProperty(object, property, isConditional); + visitProperty(object: Place, property: string): void { + const nextDependency = this.#getProperty(object, property, false); this.visitDependency(nextDependency); } @@ -529,14 +524,9 @@ class PropagationVisitor extends ReactiveFunctionVisitor { } } else if (value.kind === "PropertyLoad") { if (lvalue !== null && !context.isUsedOutsideDeclaringScope(lvalue)) { - context.declareProperty( - lvalue, - value.object, - value.property, - value.optional - ); + context.declareProperty(lvalue, value.object, value.property); } else { - context.visitProperty(value.object, value.property, value.optional); + context.visitProperty(value.object, value.property); } } else if (value.kind === "StoreLocal") { context.visitOperand(value.value);