diff --git a/compiler/forget/src/CompilerPipeline.ts b/compiler/forget/src/CompilerPipeline.ts index 3ca516125b..fda4f4c7a5 100644 --- a/compiler/forget/src/CompilerPipeline.ts +++ b/compiler/forget/src/CompilerPipeline.ts @@ -23,6 +23,7 @@ import { inferReactiveScopeVariables, propagateScopeDependencies, pruneUnusedLabels, + pruneUnusedLValues, pruneUnusedScopes, renameVariables, } from "./ReactiveScopes"; @@ -82,6 +83,9 @@ export default function ( pruneUnusedScopes(reactiveFunction); logReactiveFunction("pruneUnusedScopes", reactiveFunction); + pruneUnusedLValues(reactiveFunction); + logReactiveFunction("pruneUnusedLValues", reactiveFunction); + renameVariables(reactiveFunction); logReactiveFunction("renameVariables", reactiveFunction); diff --git a/compiler/forget/src/HIR/BuildHIR.ts b/compiler/forget/src/HIR/BuildHIR.ts index 4310f3f21c..3a33048e51 100644 --- a/compiler/forget/src/HIR/BuildHIR.ts +++ b/compiler/forget/src/HIR/BuildHIR.ts @@ -649,13 +649,17 @@ function lowerStatement( const stmt = stmtPath as NodePath; const expression = stmt.get("expression"); const value = lowerExpression(builder, expression); - if (expression.isAssignmentExpression()) { - // instruction already emitted via lowerExpression() + if (expression.isAssignmentExpression() && value.kind === "Identifier") { + // already lowered to a place return; } + const place = buildTemporaryPlace( + builder, + stmt.node.loc ?? GeneratedSource + ); builder.push({ id: makeInstructionId(0), - lvalue: null, + lvalue: { kind: InstructionKind.Const, place }, value, loc: stmt.node.loc ?? GeneratedSource, }); @@ -885,13 +889,7 @@ function lowerExpression( // tmp != null ? tmp : const left = lowerExpressionToPlace(builder, leftPath); - const nullPlace: Place = { - kind: "Identifier", - identifier: builder.makeTemporary(), - memberPath: null, - effect: Effect.Unknown, - loc: left.loc, - }; + const nullPlace: Place = buildTemporaryPlace(builder, left.loc); builder.push({ id: makeInstructionId(0), value: { @@ -903,13 +901,7 @@ function lowerExpression( lvalue: { place: { ...nullPlace }, kind: InstructionKind.Const }, }); - const condPlace: Place = { - kind: "Identifier", - identifier: builder.makeTemporary(), - memberPath: null, - effect: Effect.Unknown, - loc: left.loc, - }; + const condPlace: Place = buildTemporaryPlace(builder, left.loc); builder.push({ id: makeInstructionId(0), lvalue: { @@ -960,36 +952,23 @@ function lowerExpression( } case "MemberExpression": { const leftExpr = left as NodePath; - const object = lowerExpressionToPlace( - builder, - leftExpr.get("object") - ); const property = leftExpr.get("property"); invariant( property.isIdentifier(), "Assignment expression to dynamic properties is not yet supported" ); const right = lowerExpressionToPlace(builder, expr.get("right")); - const place: Place = { - kind: "Identifier", - identifier: builder.makeTemporary(), - memberPath: null, - effect: Effect.Read, - loc: exprLoc, + const object = lowerExpressionToPlace( + builder, + leftExpr.get("object") + ); + return { + kind: "PropertyStore", + object, + property: property.node.name, + value: right, + loc: leftNode.loc ?? GeneratedSource, }; - builder.push({ - id: makeInstructionId(0), - lvalue: { place: { ...place }, kind: InstructionKind.Const }, - value: { - kind: "PropertyStore", - object, - property: property.node.name, - value: right, - loc: leftNode.loc ?? GeneratedSource, - }, - loc: exprLoc, - }); - return place; } default: { todoInvariant( @@ -1056,13 +1035,10 @@ function lowerExpression( "Assignment expression to dynamic properties is not yet supported" ); // Store the previous value to a temporary - const previousValuePlace: Place = { - kind: "Identifier", - identifier: builder.makeTemporary(), - memberPath: null, - effect: Effect.Read, - loc: exprLoc, - }; + const previousValuePlace: Place = buildTemporaryPlace( + builder, + exprLoc + ); builder.push({ id: makeInstructionId(0), lvalue: { @@ -1078,13 +1054,7 @@ function lowerExpression( loc: leftExpr.node.loc ?? GeneratedSource, }); // Store the new value to a temporary - const newValuePlace: Place = { - kind: "Identifier", - identifier: builder.makeTemporary(), - memberPath: null, - effect: Effect.Read, - loc: exprLoc, - }; + const newValuePlace: Place = buildTemporaryPlace(builder, exprLoc); builder.push({ id: makeInstructionId(0), lvalue: { @@ -1102,29 +1072,13 @@ function lowerExpression( }); // Save the result back to the property - const place: Place = { - kind: "Identifier", - identifier: builder.makeTemporary(), - memberPath: null, - effect: Effect.Read, - loc: exprLoc, - }; - builder.push({ - id: makeInstructionId(0), - lvalue: { - place: { ...place }, - kind: InstructionKind.Const, - }, - value: { - kind: "PropertyStore", - object: { ...object }, - property: property.node.name, - value: { ...newValuePlace }, - loc: leftExpr.node.loc ?? GeneratedSource, - }, + return { + kind: "PropertyStore", + object: { ...object }, + property: property.node.name, + value: { ...newValuePlace }, loc: leftExpr.node.loc ?? GeneratedSource, - }); - return place; + }; } default: { invariant( @@ -1149,13 +1103,7 @@ function lowerExpression( property: property.node.name, loc: exprLoc, }; - const place: Place = { - kind: "Identifier", - identifier: builder.makeTemporary(), - memberPath: null, - effect: Effect.Read, - loc: exprLoc, - }; + const place: Place = buildTemporaryPlace(builder, exprLoc); builder.push({ id: makeInstructionId(0), lvalue: { place: { ...place }, kind: InstructionKind.Const }, @@ -1231,13 +1179,7 @@ function lowerConditional( consequent: () => InstructionValue, alternate: () => InstructionValue ): Place { - const place: Place = { - kind: "Identifier", - identifier: builder.makeTemporary(), - memberPath: null, - effect: Effect.Read, - loc, - }; + const place: Place = buildTemporaryPlace(builder, loc); // Block for code following the if const continuationBlock = builder.reserve(); // Block for the consequent (if the test is truthy) @@ -1311,13 +1253,7 @@ function lowerJsxElementName( }; return place; } else { - const place: Place = { - kind: "Identifier", - identifier: builder.makeTemporary(), - memberPath: null, - effect: Effect.Unknown, - loc: exprLoc, - }; + const place: Place = buildTemporaryPlace(builder, exprLoc); builder.push({ id: makeInstructionId(0), value: { @@ -1351,13 +1287,7 @@ function lowerJsxElement( todoInvariant(expression.isExpression(), "handle empty expressions"); return lowerExpressionToPlace(builder, expression); } else if (exprPath.isJSXText()) { - const place: Place = { - kind: "Identifier", - identifier: builder.makeTemporary(), - memberPath: null, - effect: Effect.Unknown, - loc: exprLoc, - }; + const place: Place = buildTemporaryPlace(builder, exprLoc); builder.push({ id: makeInstructionId(0), value: { @@ -1374,13 +1304,7 @@ function lowerJsxElement( t.isJSXFragment(exprNode) || t.isJSXSpreadChild(exprNode), "Expected refinement to work" ); - const place: Place = { - kind: "Identifier", - identifier: builder.makeTemporary(), - memberPath: null, - effect: Effect.Unknown, - loc: exprLoc, - }; + const place: Place = buildTemporaryPlace(builder, exprLoc); builder.push({ id: makeInstructionId(0), value: { @@ -1404,13 +1328,7 @@ function lowerExpressionToPlace( return instr; } const exprLoc = exprPath.node.loc ?? GeneratedSource; - const place: Place = { - kind: "Identifier", - identifier: builder.makeTemporary(), - memberPath: null, - effect: Effect.Unknown, - loc: exprLoc, - }; + const place: Place = buildTemporaryPlace(builder, exprLoc); builder.push({ id: makeInstructionId(0), value: instr, @@ -1496,6 +1414,20 @@ function lowerLVal(builder: HIRBuilder, exprPath: NodePath): Place { } } +/** + * Creates a temporary Identifier and Place referencing that identifier. + */ +function buildTemporaryPlace(builder: HIRBuilder, loc: SourceLocation): Place { + const place: Place = { + kind: "Identifier", + identifier: builder.makeTemporary(), + memberPath: null, + effect: Effect.Unknown, + loc, + }; + return place; +} + function lowerAssignment( builder: HIRBuilder, loc: SourceLocation, diff --git a/compiler/forget/src/HIR/Codegen.ts b/compiler/forget/src/HIR/Codegen.ts index 640ec791b8..2f19e26955 100644 --- a/compiler/forget/src/HIR/Codegen.ts +++ b/compiler/forget/src/HIR/Codegen.ts @@ -302,14 +302,7 @@ export function codegenInstruction( if (instr.lvalue === null) { return t.expressionStatement(value); } - if (instr.value.kind === "PropertyStore") { - invariant( - instr.lvalue.place.identifier.name === null, - "Expected property stores to be lowered to a temporary" - ); - temp.set(instr.lvalue.place.identifier.id, value); - return t.expressionStatement(value); - } else if ( + if ( instr.lvalue.place.memberPath === null && instr.lvalue.place.identifier.name === null ) { diff --git a/compiler/forget/src/ReactiveScopes/PruneTemporaryLValues.ts b/compiler/forget/src/ReactiveScopes/PruneTemporaryLValues.ts new file mode 100644 index 0000000000..9ce26cf09f --- /dev/null +++ b/compiler/forget/src/ReactiveScopes/PruneTemporaryLValues.ts @@ -0,0 +1,41 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +import { + Identifier, + Instruction, + InstructionKind, + ReactiveFunction, +} from "../HIR/HIR"; +import { visitFunction } from "./visitors"; + +/** + * Nulls out lvalues for temporary variables that are never accessed later. This only + * nulls out the lvalue itself, it does not remove the corresponding instructions. + */ +export function pruneTemporaryLValues(fn: ReactiveFunction): void { + const lvalues = new Map(); + visitFunction(fn, { + visitInstruction: (instr) => { + if ( + instr.lvalue !== null && + instr.lvalue.kind === InstructionKind.Const && + instr.lvalue.place.identifier.name === null + ) { + lvalues.set(instr.lvalue.place.identifier, instr); + } + }, + visitValue: (value) => { + if (value.kind === "Identifier") { + lvalues.delete(value.identifier); + } + }, + }); + for (const [, instr] of lvalues) { + instr.lvalue = null; + } +} diff --git a/compiler/forget/src/ReactiveScopes/index.ts b/compiler/forget/src/ReactiveScopes/index.ts index acc1f021f9..2905926ece 100644 --- a/compiler/forget/src/ReactiveScopes/index.ts +++ b/compiler/forget/src/ReactiveScopes/index.ts @@ -12,6 +12,7 @@ export { inferReactiveScopes } from "./InferReactiveScopes"; export { inferReactiveScopeVariables } from "./InferReactiveScopeVariables"; export { printReactiveFunction } from "./PrintReactiveFunction"; export { propagateScopeDependencies } from "./PropagateScopeDependencies"; +export { pruneTemporaryLValues as pruneUnusedLValues } from "./PruneTemporaryLValues"; export { pruneUnusedLabels } from "./PruneUnusedLabels"; export { pruneUnusedScopes } from "./PruneUnusedScopes"; export { renameVariables } from "./RenameVariables"; diff --git a/compiler/forget/src/ReactiveScopes/visitors.ts b/compiler/forget/src/ReactiveScopes/visitors.ts index c46c3a7c7d..75c34f1b9e 100644 --- a/compiler/forget/src/ReactiveScopes/visitors.ts +++ b/compiler/forget/src/ReactiveScopes/visitors.ts @@ -5,9 +5,82 @@ * LICENSE file in the root directory of this source tree. */ -import { ReactiveBlock, ReactiveTerminal } from "../HIR/HIR"; +import { + Instruction, + InstructionValue, + Place, + ReactiveBlock, + ReactiveFunction, + ReactiveScope, + ReactiveTerminal, + ReactiveValueBlock, +} from "../HIR/HIR"; +import { eachInstructionValueOperand } from "../HIR/visitors"; import { assertExhaustive } from "../Utils/utils"; +export function visitFunction( + fn: ReactiveFunction, + visitors: { + visitValue?: (value: InstructionValue) => void; + visitInstruction?: (instr: Instruction) => void; + visitTerminal?: (terminal: ReactiveTerminal) => void; + visitScope?: (scope: ReactiveScope) => void; + } +): void { + const { visitValue, visitInstruction, visitTerminal, visitScope } = visitors; + function visitBlock(block: ReactiveBlock): void { + for (const item of block) { + switch (item.kind) { + case "instruction": { + if (visitValue) { + for (const operand of eachInstructionValueOperand( + item.instruction.value + )) { + visitValue(operand); + } + } + if (visitInstruction) { + visitInstruction(item.instruction); + } + break; + } + case "terminal": { + if (visitValue) { + eachTerminalOperand(item.terminal, (operand) => { + visitValue(operand); + }); + } + if (visitTerminal) { + visitTerminal(item.terminal); + } + eachTerminalBlock(item.terminal, visitBlock, visitValueBlock); + break; + } + case "scope": { + if (visitScope) { + visitScope(item.scope); + } + visitBlock(item.instructions); + break; + } + default: { + assertExhaustive( + item, + `Unexpected item kind '${(item as any).kind}'` + ); + } + } + } + } + function visitValueBlock(block: ReactiveValueBlock): void { + visitBlock(block.instructions); + if (block.value !== null && visitValue) { + visitValue(block.value); + } + } + visitBlock(fn.body); +} + export function mapTerminalBlocks( terminal: ReactiveTerminal, fn: (block: ReactiveBlock) => ReactiveBlock @@ -50,3 +123,98 @@ export function mapTerminalBlocks( } } } + +export function eachTerminalBlock( + terminal: ReactiveTerminal, + visitBlock: (block: ReactiveBlock) => void, + visitValueBlock: (block: ReactiveValueBlock) => void +): void { + switch (terminal.kind) { + case "break": + case "continue": + case "return": + case "throw": { + break; + } + case "for": { + visitValueBlock(terminal.init); + visitValueBlock(terminal.test); + visitValueBlock(terminal.update); + visitBlock(terminal.loop); + break; + } + case "while": { + visitValueBlock(terminal.test); + visitBlock(terminal.loop); + break; + } + case "if": { + visitBlock(terminal.consequent); + if (terminal.alternate !== null) { + visitBlock(terminal.alternate); + } + break; + } + case "switch": { + for (const case_ of terminal.cases) { + if (case_.block !== undefined) { + visitBlock(case_.block); + } + } + break; + } + default: { + assertExhaustive( + terminal, + `Unexpected terminal kind '${(terminal as any).kind}'` + ); + } + } +} + +export function eachTerminalOperand( + terminal: ReactiveTerminal, + fn: (place: Place) => void +): void { + switch (terminal.kind) { + case "break": + case "continue": { + break; + } + case "return": { + if (terminal.value !== null) { + fn(terminal.value); + } + break; + } + case "throw": { + fn(terminal.value); + break; + } + case "for": { + break; + } + case "while": { + break; + } + case "if": { + fn(terminal.test); + break; + } + case "switch": { + fn(terminal.test); + for (const case_ of terminal.cases) { + if (case_.test !== null) { + fn(case_.test); + } + } + break; + } + default: { + assertExhaustive( + terminal, + `Unexpected terminal kind '${(terminal as any).kind}'` + ); + } + } +} diff --git a/compiler/forget/src/__tests__/fixtures/hir/_bug_chained-assignment-expressions.expect.md b/compiler/forget/src/__tests__/fixtures/hir/_bug_chained-assignment-expressions.expect.md deleted file mode 100644 index 9bdf873991..0000000000 --- a/compiler/forget/src/__tests__/fixtures/hir/_bug_chained-assignment-expressions.expect.md +++ /dev/null @@ -1,28 +0,0 @@ - -## Input - -```javascript -function foo() { - const x = { y: 0 }; - const y = { z: 0 }; - x.y += y.z *= 1; -} - -``` - -## Code - -```javascript -function foo() { - const x = { - y: 0, - }; - const y = { - z: 0, - }; - y.z = y.z * 1; - x.y = x.y + (y.z = y.z * 1); -} - -``` - \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/hir/_bug_chained-assignment-expressions.js b/compiler/forget/src/__tests__/fixtures/hir/_bug_chained-assignment-expressions.js deleted file mode 100644 index b6e035f072..0000000000 --- a/compiler/forget/src/__tests__/fixtures/hir/_bug_chained-assignment-expressions.js +++ /dev/null @@ -1,5 +0,0 @@ -function foo() { - const x = { y: 0 }; - const y = { z: 0 }; - x.y += y.z *= 1; -} diff --git a/compiler/forget/src/__tests__/fixtures/hir/chained-assignment-expressions.expect.md b/compiler/forget/src/__tests__/fixtures/hir/chained-assignment-expressions.expect.md new file mode 100644 index 0000000000..8629728d77 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/hir/chained-assignment-expressions.expect.md @@ -0,0 +1,35 @@ + +## Input + +```javascript +function foo() { + const x = { x: 0 }; + const y = { z: 0 }; + const z = { z: 0 }; + x.x += y.y *= 1; + z.z += y.y *= x.x &= 3; + return z; +} + +``` + +## Code + +```javascript +function foo() { + const x = { + x: 0, + }; + const y = { + z: 0, + }; + const z = { + z: 0, + }; + x.x = x.x + (y.y = y.y * 1); + z.z = z.z + (y.y = y.y * (x.x = x.x & 3)); + return z; +} + +``` + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/hir/chained-assignment-expressions.js b/compiler/forget/src/__tests__/fixtures/hir/chained-assignment-expressions.js new file mode 100644 index 0000000000..fb4f27844c --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/hir/chained-assignment-expressions.js @@ -0,0 +1,8 @@ +function foo() { + const x = { x: 0 }; + const y = { z: 0 }; + const z = { z: 0 }; + x.x += y.y *= 1; + z.z += y.y *= x.x &= 3; + return z; +}