{"body":"Record a variable that is declared in some other scope and that is being \nreassigned in the current one as a reassignment","parents":["e7f4eae619cd38c829c62f5a6f869b50f7789c70"],"commit":{"revision":"d47f608c612082b82d9d5c79f696f86e394a4d1a","subject":"Record reassignments","authorName":"Lauren Tan","authorEmail":"poteto@users.noreply.github.com","shortRevision":"d47f608c61","authoredAt":"2023-02-08T10:26:33-05:00"},"patch":"diff --git a\/compiler\/forget\/src\/HIR\/HIR.ts b\/compiler\/forget\/src\/HIR\/HIR.ts\nindex 85d7586b9a..e6e7b8d589 100644\n--- a\/compiler\/forget\/src\/HIR\/HIR.ts\n+++ b\/compiler\/forget\/src\/HIR\/HIR.ts\n@@ -556,6 +556,7 @@ export type ReactiveScope = {\n range: MutableRange;\n dependencies: Set;\n declarations: Set;\n+ reassignments: Set;\n };\n \n export type ReactiveScopeDependency = {\ndiff --git a\/compiler\/forget\/src\/ReactiveScopes\/CodegenReactiveFunction.ts b\/compiler\/forget\/src\/ReactiveScopes\/CodegenReactiveFunction.ts\nindex 22ac9479b7..ba790e242f 100644\n--- a\/compiler\/forget\/src\/ReactiveScopes\/CodegenReactiveFunction.ts\n+++ b\/compiler\/forget\/src\/ReactiveScopes\/CodegenReactiveFunction.ts\n@@ -76,7 +76,7 @@ export function codegenReactiveFunction(\n \n class Context {\n #nextCacheIndex: number = 0;\n- #identifiers: Set = new Set();\n+ #declarations: Set = new Set();\n temp: Temporaries = new Map();\n errors: CompilerError = new CompilerError();\n \n@@ -85,11 +85,11 @@ class Context {\n }\n \n declare(identifier: Identifier): void {\n- this.#identifiers.add(identifier);\n+ this.#declarations.add(identifier);\n }\n \n- declared(identifier: Identifier): boolean {\n- return this.#identifiers.has(identifier);\n+ hasDeclared(identifier: Identifier): boolean {\n+ return this.#declarations.has(identifier);\n }\n }\n \n@@ -179,21 +179,48 @@ function codegenReactiveScope(\n );\n }\n let firstOutputIndex: number | null = null;\n- for (const output of scope.declarations) {\n+ for (const declaration of scope.declarations) {\n const index = cx.nextCacheIndex;\n if (firstOutputIndex === null) {\n firstOutputIndex = index;\n }\n \n invariant(\n- output.name != null,\n+ declaration.name != null,\n \"Expected identifier '@%s' to be named\",\n- output.id\n+ declaration.id\n );\n \n- const name = convertIdentifier(output);\n- cx.declare(output);\n- statements.push(t.variableDeclaration(\"let\", [t.variableDeclarator(name)]));\n+ const name = convertIdentifier(declaration);\n+ if (!cx.hasDeclared(declaration)) {\n+ statements.push(\n+ t.variableDeclaration(\"let\", [t.variableDeclarator(name)])\n+ );\n+ }\n+ cacheStoreStatements.push(\n+ t.expressionStatement(\n+ t.assignmentExpression(\n+ \"=\",\n+ t.memberExpression(t.identifier(\"$\"), t.numericLiteral(index), true),\n+ name\n+ )\n+ )\n+ );\n+ cacheLoadStatements.push(\n+ t.expressionStatement(\n+ t.assignmentExpression(\n+ \"=\",\n+ name,\n+ t.memberExpression(t.identifier(\"$\"), t.numericLiteral(index), true)\n+ )\n+ )\n+ );\n+ cx.declare(declaration);\n+ }\n+ for (const reassignment of scope.reassignments) {\n+ const index = cx.nextCacheIndex;\n+ const name = convertIdentifier(reassignment);\n+\n cacheStoreStatements.push(\n t.expressionStatement(\n t.assignmentExpression(\n@@ -213,11 +240,6 @@ function codegenReactiveScope(\n )\n );\n }\n- invariant(\n- firstOutputIndex !== null,\n- \"Expected scope '@%s' to have at least one output\",\n- scope.id\n- );\n let testCondition = (changeIdentifiers as Array).reduce(\n (acc: t.Expression | null, ident: t.Expression) => {\n if (acc == null) {\n@@ -228,6 +250,11 @@ function codegenReactiveScope(\n null as t.Expression | null\n );\n if (testCondition === null) {\n+ invariant(\n+ firstOutputIndex !== null,\n+ \"Expected scope '@%s' to have at least one output\",\n+ scope.id\n+ );\n testCondition = t.binaryExpression(\n \"===\",\n t.memberExpression(\n@@ -328,7 +355,7 @@ function codegenInstructionNullable(\n value: t.Expression\n ): t.Statement | null {\n let statement;\n- if (instr.lvalue !== null && cx.declared(instr.lvalue.place.identifier)) {\n+ if (instr.lvalue !== null && cx.hasDeclared(instr.lvalue.place.identifier)) {\n statement = codegenInstruction(\n cx,\n {\ndiff --git a\/compiler\/forget\/src\/ReactiveScopes\/InferReactiveScopeVariables.ts b\/compiler\/forget\/src\/ReactiveScopes\/InferReactiveScopeVariables.ts\nindex 14be0c0a77..42674376f1 100644\n--- a\/compiler\/forget\/src\/ReactiveScopes\/InferReactiveScopeVariables.ts\n+++ b\/compiler\/forget\/src\/ReactiveScopes\/InferReactiveScopeVariables.ts\n@@ -129,6 +129,7 @@ export function inferReactiveScopeVariables(fn: HIRFunction) {\n range: identifier.mutableRange,\n dependencies: new Set(),\n declarations: new Set(),\n+ reassignments: new Set(),\n };\n scopes.set(groupIdentifier, scope);\n } else {\ndiff --git a\/compiler\/forget\/src\/ReactiveScopes\/PrintReactiveFunction.ts b\/compiler\/forget\/src\/ReactiveScopes\/PrintReactiveFunction.ts\nindex 8be8a48664..83395001be 100644\n--- a\/compiler\/forget\/src\/ReactiveScopes\/PrintReactiveFunction.ts\n+++ b\/compiler\/forget\/src\/ReactiveScopes\/PrintReactiveFunction.ts\n@@ -46,8 +46,10 @@ export function printReactiveBlock(\n }] dependencies=[${Array.from(block.scope.dependencies)\n .map((dep) => printDependency(dep))\n .join(\", \")}] declarations=[${Array.from(block.scope.declarations)\n- .map((out) => printIdentifier(out))\n- .join(\", \")}] {`\n+ .map((decl) => printIdentifier(decl))\n+ .join(\", \")}] reassignments=[${Array.from(block.scope.reassignments).map(\n+ (reassign) => printIdentifier(reassign)\n+ )}] {`\n );\n printReactiveInstructions(writer, block.instructions);\n writer.writeLine(\"}\");\ndiff --git a\/compiler\/forget\/src\/ReactiveScopes\/PropagateScopeDependencies.ts b\/compiler\/forget\/src\/ReactiveScopes\/PropagateScopeDependencies.ts\nindex aca9608203..c13dc498b2 100644\n--- a\/compiler\/forget\/src\/ReactiveScopes\/PropagateScopeDependencies.ts\n+++ b\/compiler\/forget\/src\/ReactiveScopes\/PropagateScopeDependencies.ts\n@@ -33,12 +33,17 @@ import { eachReactiveValueOperand } from \".\/visitors\";\n export function propagateScopeDependencies(fn: ReactiveFunction): void {\n const context = new Context(inferReactiveIdentifiers(fn));\n if (fn.id !== null) {\n- context.declare(fn.id, { kind: DeclKind.Const, id: makeInstructionId(0) });\n+ context.declare(fn.id, {\n+ kind: DeclKind.Const,\n+ id: makeInstructionId(0),\n+ scope: null,\n+ });\n }\n for (const param of fn.params) {\n context.declare(param.identifier, {\n kind: DeclKind.Dynamic,\n id: makeInstructionId(0),\n+ scope: null,\n });\n }\n visit(context, fn.body);\n@@ -50,7 +55,11 @@ enum DeclKind {\n }\n \n type DeclMap = Map;\n-type Decl = { kind: DeclKind; id: InstructionId };\n+type Decl = {\n+ kind: DeclKind;\n+ id: InstructionId;\n+ scope: ReactiveScope | null;\n+};\n \n type Scopes = Array;\n \n@@ -78,6 +87,12 @@ class Context {\n return scopedDependencies;\n }\n \n+ \/**\n+ * Records where a value was declared, and optionally, the scope where the value originated from.\n+ * This is later used to determine if a dependency should be added to a scope; if the current\n+ * scope we are visiting is the same scope where the value originates, it can't be a dependency\n+ * on itself.\n+ *\/\n declare(identifier: Identifier, decl: Decl): void {\n this.#declarations.set(identifier, decl);\n }\n@@ -100,8 +115,8 @@ class Context {\n return this.#scopes.indexOf(scope) !== -1;\n }\n \n- get #currentScope(): ReactiveScope {\n- return this.#scopes.at(-1)!;\n+ get currentScope(): ReactiveScope | null {\n+ return this.#scopes.at(-1) ?? null;\n }\n \n isReactive(id: Identifier): boolean {\n@@ -149,22 +164,26 @@ class Context {\n \n \/\/ Any value used after its defining scope has concluded must be added as an\n \/\/ output of its defining scope. Regardless of whether its a const or not,\n- \/\/ some later code needs access to the value.\n- if (decl !== undefined) {\n- const operandScope = maybeDependency.place.identifier.scope;\n- if (operandScope !== null && !this.#isScopeActive(operandScope)) {\n- operandScope.declarations.add(maybeDependency.place.identifier);\n- }\n+ \/\/ some later code needs access to the value. If the current\n+ \/\/ scope we are visiting is the same scope where the value originates,\n+ \/\/ it can't be a dependency on itself.\n+ if (\n+ decl !== undefined &&\n+ decl.scope !== null &&\n+ !this.#isScopeActive(decl.scope)\n+ ) {\n+ decl.scope.declarations.add(maybeDependency.place.identifier);\n }\n \n \/\/ If this operand is used in a scope, has a dynamic value, and was defined\n \/\/ before this scope, then its a dependency of the scope.\n- const currentScope = this.#currentScope;\n+ const currentScope = this.currentScope;\n if (\n+ currentScope != null &&\n decl !== undefined &&\n decl.kind !== DeclKind.Const &&\n- currentScope !== undefined &&\n- decl.id < currentScope.range.start\n+ decl.id < currentScope.range.start &&\n+ (decl.scope == null || !this.#isScopeActive(decl.scope))\n ) {\n \/\/ Check if there is an existing dependency that describes this operand\n for (const dep of this.#dependencies) {\n@@ -201,6 +220,25 @@ class Context {\n this.#dependencies.add(maybeDependency);\n }\n }\n+\n+ \/**\n+ * Record a variable that is declared in some other scope and that is being reassigned in the\n+ * current one as a {@link ReactiveScope.reassignments}\n+ *\/\n+ visitReassignment(lvalue: LValue): void {\n+ if (lvalue.kind !== InstructionKind.Reassign) {\n+ return;\n+ }\n+ const declaration = this.#declarations.get(lvalue.place.identifier);\n+ if (\n+ this.currentScope != null &&\n+ lvalue.place.identifier.scope != null &&\n+ declaration !== undefined &&\n+ declaration.scope !== lvalue.place.identifier.scope\n+ ) {\n+ this.currentScope.reassignments.add(lvalue.place.identifier);\n+ }\n+ }\n }\n \n function visit(context: Context, block: ReactiveBlock): void {\n@@ -335,14 +373,17 @@ function visitInstructionValue(\n function visitInstruction(context: Context, instr: ReactiveInstruction): void {\n const { lvalue } = instr;\n visitInstructionValue(context, instr.value, lvalue);\n- if (lvalue !== null && lvalue.kind !== InstructionKind.Reassign) {\n- \/\/ TODO: only assign Const if the value is never reassigned\n- const kind = context.isReactive(lvalue.place.identifier)\n- ? DeclKind.Dynamic\n- : DeclKind.Const;\n- context.declare(lvalue.place.identifier, {\n- kind,\n- id: lvalue.place.identifier.mutableRange.start,\n- });\n+ if (lvalue == null) {\n+ return;\n }\n+ context.visitReassignment(lvalue);\n+ \/\/ TODO: only assign Const if the value is never reassigned\n+ const kind = context.isReactive(lvalue.place.identifier)\n+ ? DeclKind.Dynamic\n+ : DeclKind.Const;\n+ context.declare(lvalue.place.identifier, {\n+ kind,\n+ id: lvalue.place.identifier.mutableRange.start,\n+ scope: context.currentScope,\n+ });\n }\ndiff --git a\/compiler\/forget\/src\/ReactiveScopes\/PruneUnusedScopes.ts b\/compiler\/forget\/src\/ReactiveScopes\/PruneUnusedScopes.ts\nindex 333dc69fde..6632981500 100644\n--- a\/compiler\/forget\/src\/ReactiveScopes\/PruneUnusedScopes.ts\n+++ b\/compiler\/forget\/src\/ReactiveScopes\/PruneUnusedScopes.ts\n@@ -30,7 +30,13 @@ function visitBlock(block: ReactiveBlock): ReactiveBlock {\n }\n case \"scope\": {\n stmt.instructions = visitBlock(stmt.instructions);\n- if (stmt.scope.declarations.size === 0) {\n+ \/\/ If a scope doesn't have declarations but reassigns a value, the scope shouldn't be pruned\n+ \/\/ as we still want to generate a memo block for that scope\n+ if (\n+ stmt.scope.declarations.size === 0 &&\n+ (stmt.scope.dependencies.size === 0 ||\n+ stmt.scope.reassignments.size === 0)\n+ ) {\n nextBlock ??= block.slice(0, i);\n nextBlock.push(...stmt.instructions);\n continue;\ndiff --git a\/compiler\/forget\/src\/__tests__\/fixtures\/hir\/ssa-leave-case.expect.md b\/compiler\/forget\/src\/__tests__\/fixtures\/hir\/ssa-leave-case.expect.md\nindex ba26e228b8..9359a8826e 100644\n--- a\/compiler\/forget\/src\/__tests__\/fixtures\/hir\/ssa-leave-case.expect.md\n+++ b\/compiler\/forget\/src\/__tests__\/fixtures\/hir\/ssa-leave-case.expect.md\n@@ -27,10 +27,11 @@ function Component(props) {\n const c_0 = $[0] !== props.p0;\n const c_1 = $[1] !== props.p1;\n let x;\n+ let y$0;\n if (c_0 || c_1) {\n x = [];\n const y = undefined;\n- let y$0 = y;\n+ y$0 = y;\n if (props.p0) {\n x.push(props.p1);\n const y$1 = x;\n@@ -39,22 +40,24 @@ function Component(props) {\n $[0] = props.p0;\n $[1] = props.p1;\n $[2] = x;\n+ $[3] = y$0;\n } else {\n x = $[2];\n+ y$0 = $[3];\n }\n- const c_3 = $[3] !== x;\n+ const c_4 = $[4] !== x;\n let t0;\n- if (c_3) {\n+ if (c_4) {\n t0 = (\n \n {x}\n {y$0}\n <\/Component>\n );\n- $[3] = x;\n- $[4] = t0;\n+ $[4] = x;\n+ $[5] = t0;\n } else {\n- t0 = $[4];\n+ t0 = $[5];\n }\n return t0;\n }\ndiff --git a\/compiler\/forget\/src\/__tests__\/fixtures\/hir\/switch-non-final-default.expect.md b\/compiler\/forget\/src\/__tests__\/fixtures\/hir\/switch-non-final-default.expect.md\nindex 9fe7363ddb..8b34a04ff3 100644\n--- a\/compiler\/forget\/src\/__tests__\/fixtures\/hir\/switch-non-final-default.expect.md\n+++ b\/compiler\/forget\/src\/__tests__\/fixtures\/hir\/switch-non-final-default.expect.md\n@@ -36,10 +36,11 @@ function Component(props) {\n const c_0 = $[0] !== props.p0;\n const c_1 = $[1] !== props.p2;\n let x;\n+ let y$0;\n if (c_0 || c_1) {\n x = [];\n const y = undefined;\n- let y$0 = y;\n+ y$0 = y;\n bb1: switch (props.p0) {\n case 1: {\n break bb1;\n@@ -47,11 +48,11 @@ function Component(props) {\n case true: {\n x.push(props.p2);\n let y$1;\n- if ($[3] === Symbol.for(\"react.memo_cache_sentinel\")) {\n+ if ($[4] === Symbol.for(\"react.memo_cache_sentinel\")) {\n y$1 = [];\n- $[3] = y$1;\n+ $[4] = y$1;\n } else {\n- y$1 = $[3];\n+ y$1 = $[4];\n }\n y$0 = y$1;\n break bb1;\n@@ -67,27 +68,29 @@ function Component(props) {\n $[0] = props.p0;\n $[1] = props.p2;\n $[2] = x;\n+ $[3] = y$0;\n } else {\n x = $[2];\n+ y$0 = $[3];\n }\n- const c_4 = $[4] !== x;\n+ const c_5 = $[5] !== x;\n let child;\n- if (c_4) {\n+ if (c_5) {\n child = <\/Component>;\n- $[4] = x;\n- $[5] = child;\n+ $[5] = x;\n+ $[6] = child;\n } else {\n- child = $[5];\n+ child = $[6];\n }\n y$0.push(props.p4);\n- const c_6 = $[6] !== child;\n+ const c_7 = $[7] !== child;\n let t0;\n- if (c_6) {\n+ if (c_7) {\n t0 = {child}<\/Component>;\n- $[6] = child;\n- $[7] = t0;\n+ $[7] = child;\n+ $[8] = t0;\n } else {\n- t0 = $[7];\n+ t0 = $[8];\n }\n return t0;\n }\ndiff --git a\/compiler\/forget\/src\/__tests__\/fixtures\/hir\/switch.expect.md b\/compiler\/forget\/src\/__tests__\/fixtures\/hir\/switch.expect.md\nindex 52cac52421..1d81a1ef5e 100644\n--- a\/compiler\/forget\/src\/__tests__\/fixtures\/hir\/switch.expect.md\n+++ b\/compiler\/forget\/src\/__tests__\/fixtures\/hir\/switch.expect.md\n@@ -32,10 +32,11 @@ function Component(props) {\n const c_1 = $[1] !== props.p2;\n const c_2 = $[2] !== props.p3;\n let x;\n+ let y$0;\n if (c_0 || c_1 || c_2) {\n x = [];\n const y = undefined;\n- let y$0 = y;\n+ y$0 = y;\n switch (props.p0) {\n case true: {\n x.push(props.p2);\n@@ -50,27 +51,29 @@ function Component(props) {\n $[1] = props.p2;\n $[2] = props.p3;\n $[3] = x;\n+ $[4] = y$0;\n } else {\n x = $[3];\n+ y$0 = $[4];\n }\n- const c_4 = $[4] !== x;\n+ const c_5 = $[5] !== x;\n let child;\n- if (c_4) {\n+ if (c_5) {\n child = <\/Component>;\n- $[4] = x;\n- $[5] = child;\n+ $[5] = x;\n+ $[6] = child;\n } else {\n- child = $[5];\n+ child = $[6];\n }\n y$0.push(props.p4);\n- const c_6 = $[6] !== child;\n+ const c_7 = $[7] !== child;\n let t0;\n- if (c_6) {\n+ if (c_7) {\n t0 = {child}<\/Component>;\n- $[6] = child;\n- $[7] = t0;\n+ $[7] = child;\n+ $[8] = t0;\n } else {\n- t0 = $[7];\n+ t0 = $[8];\n }\n return t0;\n }\n","files":[{"status":"M","path":"compiler\/forget\/src\/HIR\/HIR.ts"},{"status":"M","path":"compiler\/forget\/src\/ReactiveScopes\/CodegenReactiveFunction.ts"},{"status":"M","path":"compiler\/forget\/src\/ReactiveScopes\/InferReactiveScopeVariables.ts"},{"status":"M","path":"compiler\/forget\/src\/ReactiveScopes\/PrintReactiveFunction.ts"},{"status":"M","path":"compiler\/forget\/src\/ReactiveScopes\/PropagateScopeDependencies.ts"},{"status":"M","path":"compiler\/forget\/src\/ReactiveScopes\/PruneUnusedScopes.ts"},{"status":"M","path":"compiler\/forget\/src\/__tests__\/fixtures\/hir\/ssa-leave-case.expect.md"},{"status":"M","path":"compiler\/forget\/src\/__tests__\/fixtures\/hir\/switch-non-final-default.expect.md"},{"status":"M","path":"compiler\/forget\/src\/__tests__\/fixtures\/hir\/switch.expect.md"}]}

dillon1000/react

Files changed