From ce80136e7b9f0024d49fce835cffa024c4505855 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Mon, 19 Jan 2026 09:19:52 +0100 Subject: [PATCH] fix(compiler): optimize away unnecessary restore/reset view calls When producing a listener, the template pipeline does the following in separate phases: 1. Generates all the variables available within its scope. 2. Adds `restoreView` and `resetView` calls if there are any referenced to local variables (e.g. `@let` or local refs). 3. Optimizes away the variables that aren't used. This means that we can end up in a situation where the references to the variables in the scope no longer exist, but we still enter and leave the view as if they're there which is unnecessary. These changes add a simple optimization pass that looks specifically for the pattern of a `restoreView` followed by a `return resetView(expr)`. Furthermore, by changing the order of some optimizations, we're able to drop the `getCurrentView` variable as well. Fixes #66286. --- .../arrow_function_defined_let.js | 6 +- .../GOLDEN_PARTIAL.js | 35 ++++++ .../r3_view_compiler_listener/TEST_CASES.json | 107 +++++++----------- .../listener_unused_let.ts | 12 ++ .../listener_unused_let_template.js | 20 ++++ .../src/phases/variable_optimization.ts | 46 ++++++-- 6 files changed, 144 insertions(+), 82 deletions(-) create mode 100644 packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/listener_unused_let.ts create mode 100644 packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/listener_unused_let_template.js diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_arrow_functions/arrow_function_defined_let.js b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_arrow_functions/arrow_function_defined_let.js index 4fef17bcaba..9303367d249 100644 --- a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_arrow_functions/arrow_function_defined_let.js +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_arrow_functions/arrow_function_defined_let.js @@ -1,8 +1,4 @@ -const arrowFn0 = (ctx, view) => (a, b) => { - // NOTE: the restoreView and resetView calls here are the result of variable optimization not picking up some cases. We can remove them once #66286 is resolved. - $r3$.ɵɵrestoreView(view); - return $r3$.ɵɵresetView(ctx.componentValue + a + b); -}; +const arrowFn0 = (ctx, view) => (a, b) => ctx.componentValue + a + b; … function TestComp_Conditional_2_Template(rf, ctx) { if (rf & 1) { diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/GOLDEN_PARTIAL.js b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/GOLDEN_PARTIAL.js index e7495f27dbd..f5fe565b683 100644 --- a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/GOLDEN_PARTIAL.js +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/GOLDEN_PARTIAL.js @@ -1042,3 +1042,38 @@ export declare class TestCmp { static ɵcmp: i0.ɵɵComponentDeclaration; } +/**************************************************************************************************** + * PARTIAL FILE: listener_unused_let.js + ****************************************************************************************************/ +import { Component } from '@angular/core'; +import * as i0 from "@angular/core"; +export class TestCmp { + noop() { } + static ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: TestCmp, deps: [], target: i0.ɵɵFactoryTarget.Component }); + static ɵcmp = i0.ɵɵngDeclareComponent({ minVersion: "14.0.0", version: "0.0.0-PLACEHOLDER", type: TestCmp, isStandalone: true, selector: "ng-component", ngImport: i0, template: ` + @let foo = 123; + + {{foo}} + `, isInline: true }); +} +i0.ɵɵngDeclareClassMetadata({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: TestCmp, decorators: [{ + type: Component, + args: [{ + template: ` + @let foo = 123; + + {{foo}} + `, + }] + }] }); + +/**************************************************************************************************** + * PARTIAL FILE: listener_unused_let.d.ts + ****************************************************************************************************/ +import * as i0 from "@angular/core"; +export declare class TestCmp { + noop(): void; + static ɵfac: i0.ɵɵFactoryDeclaration; + static ɵcmp: i0.ɵɵComponentDeclaration; +} + diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/TEST_CASES.json b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/TEST_CASES.json index 9460a3c57d6..6d668031bca 100644 --- a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/TEST_CASES.json +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/TEST_CASES.json @@ -3,9 +3,7 @@ "cases": [ { "description": "should create listener instruction on element", - "inputFiles": [ - "element_listener.ts" - ], + "inputFiles": ["element_listener.ts"], "expectations": [ { "files": [ @@ -20,9 +18,7 @@ }, { "description": "should create listener instruction on other components", - "inputFiles": [ - "component_listener.ts" - ], + "inputFiles": ["component_listener.ts"], "expectations": [ { "files": [ @@ -37,9 +33,7 @@ }, { "description": "should create multiple listener instructions that share a view snapshot", - "inputFiles": [ - "shared_snapshot_listeners.ts" - ], + "inputFiles": ["shared_snapshot_listeners.ts"], "expectations": [ { "files": [ @@ -54,9 +48,7 @@ }, { "description": "local refs in listeners defined before the local refs", - "inputFiles": [ - "local_ref_before_listener.ts" - ], + "inputFiles": ["local_ref_before_listener.ts"], "expectations": [ { "files": [ @@ -80,9 +72,7 @@ }, { "description": "should chain multiple listeners on the same element", - "inputFiles": [ - "same_element_chained_listeners.ts" - ], + "inputFiles": ["same_element_chained_listeners.ts"], "expectations": [ { "files": [ @@ -97,9 +87,7 @@ }, { "description": "should chain multiple listeners across elements", - "inputFiles": [ - "cross_element_chained_listeners.ts" - ], + "inputFiles": ["cross_element_chained_listeners.ts"], "expectations": [ { "files": [ @@ -114,9 +102,7 @@ }, { "description": "should chain multiple listeners on the same template", - "inputFiles": [ - "template_chained_listeners.ts" - ], + "inputFiles": ["template_chained_listeners.ts"], "expectations": [ { "files": [ @@ -131,9 +117,7 @@ }, { "description": "should not generate the $event argument if it is not being used in a template", - "inputFiles": [ - "no_event_arg_listener.ts" - ], + "inputFiles": ["no_event_arg_listener.ts"], "expectations": [ { "files": [ @@ -148,9 +132,7 @@ }, { "description": "should not generate the $event argument if it is not being used in a host listener", - "inputFiles": [ - "no_event_arg_host_listener.ts" - ], + "inputFiles": ["no_event_arg_host_listener.ts"], "expectations": [ { "files": [ @@ -165,9 +147,7 @@ }, { "description": "should generate the $event argument if it is being used in a host listener", - "inputFiles": [ - "has_event_arg_host_listener.ts" - ], + "inputFiles": ["has_event_arg_host_listener.ts"], "expectations": [ { "files": [ @@ -182,9 +162,7 @@ }, { "description": "should assume $event is referring to the event variable in a listener by default", - "inputFiles": [ - "event_arg_listener_implicit_meaning.ts" - ], + "inputFiles": ["event_arg_listener_implicit_meaning.ts"], "expectations": [ { "files": [ @@ -199,9 +177,7 @@ }, { "description": "should preserve accesses to $event if it is done through `this` in a listener", - "inputFiles": [ - "event_explicit_access.ts" - ], + "inputFiles": ["event_explicit_access.ts"], "expectations": [ { "files": [ @@ -215,9 +191,7 @@ }, { "description": "should not assume that $event is referring to an event object inside a property", - "inputFiles": [ - "event_in_property_binding.ts" - ], + "inputFiles": ["event_in_property_binding.ts"], "expectations": [ { "files": [ @@ -232,9 +206,7 @@ }, { "description": "should assume $event is referring to the event variable in a listener by default inside a host binding", - "inputFiles": [ - "event_arg_host_listener_implicit_meaning.ts" - ], + "inputFiles": ["event_arg_host_listener_implicit_meaning.ts"], "expectations": [ { "files": [ @@ -249,9 +221,7 @@ }, { "description": "should preserve accesses to $event if it is done through `this` in a listener inside a host binding", - "inputFiles": [ - "event_host_explicit_access.ts" - ], + "inputFiles": ["event_host_explicit_access.ts"], "expectations": [ { "files": [ @@ -266,9 +236,7 @@ }, { "description": "should generate the view restoration statements if a keyed write is used in an event listener from within an ng-template", - "inputFiles": [ - "implicit_receiver_keyed_write_inside_template.ts" - ], + "inputFiles": ["implicit_receiver_keyed_write_inside_template.ts"], "expectations": [ { "files": [ @@ -283,9 +251,7 @@ }, { "description": "should reference correct context in listener inside embedded view", - "inputFiles": [ - "embedded_view_listener_context.ts" - ], + "inputFiles": ["embedded_view_listener_context.ts"], "expectations": [ { "files": [ @@ -300,9 +266,7 @@ }, { "description": "should generate a simple two-way binding", - "inputFiles": [ - "simple_two_way.ts" - ], + "inputFiles": ["simple_two_way.ts"], "expectations": [ { "files": [ @@ -317,9 +281,7 @@ }, { "description": "should generate a nested two-way binding", - "inputFiles": [ - "nested_two_way.ts" - ], + "inputFiles": ["nested_two_way.ts"], "expectations": [ { "files": [ @@ -334,9 +296,7 @@ }, { "description": "should generate listener with multiple statements", - "inputFiles": [ - "multiple_statements.ts" - ], + "inputFiles": ["multiple_statements.ts"], "expectations": [ { "failureMessage": "Incorrect template" @@ -345,9 +305,7 @@ }, { "description": "should maintain the binding order between plain listeners and listeners part of a two-way binding", - "inputFiles": [ - "mixed_one_way_two_way_listener_order.ts" - ], + "inputFiles": ["mixed_one_way_two_way_listener_order.ts"], "expectations": [ { "failureMessage": "Incorrect template" @@ -356,9 +314,7 @@ }, { "description": "should generate a two-way binding to a @for loop variable that is a signal", - "inputFiles": [ - "two_way_binding_to_signal_loop_variable.ts" - ], + "inputFiles": ["two_way_binding_to_signal_loop_variable.ts"], "expectations": [ { "files": [ @@ -373,9 +329,7 @@ }, { "description": "should generate a two-way binding to a $any expression", - "inputFiles": [ - "two_way_to_any.ts" - ], + "inputFiles": ["two_way_to_any.ts"], "expectations": [ { "files": [ @@ -387,6 +341,21 @@ "failureMessage": "Incorrect template" } ] + }, + { + "description": "should not generate restore/reset view when listener does not use @let in the same scope", + "inputFiles": ["listener_unused_let.ts"], + "expectations": [ + { + "files": [ + { + "expected": "listener_unused_let_template.js", + "generated": "listener_unused_let.js" + } + ], + "failureMessage": "Incorrect template" + } + ] } ] } diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/listener_unused_let.ts b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/listener_unused_let.ts new file mode 100644 index 00000000000..a9248339061 --- /dev/null +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/listener_unused_let.ts @@ -0,0 +1,12 @@ +import {Component} from '@angular/core'; + +@Component({ + template: ` + @let foo = 123; + + {{foo}} + `, +}) +export class TestCmp { + noop() {} +} diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/listener_unused_let_template.js b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/listener_unused_let_template.js new file mode 100644 index 00000000000..91d46fac32a --- /dev/null +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/listener_unused_let_template.js @@ -0,0 +1,20 @@ +$r3$.ɵɵdefineComponent({ + … + decls: 2, + vars: 1, + consts: [[3, "click"]], + template: function TestCmp_Template(rf, ctx) { + if (rf & 1) { + $r3$.ɵɵdomElementStart(0, "button", 0); + $r3$.ɵɵdomListener("click", function TestCmp_Template_button_click_0_listener() { return ctx.noop(); }); + $r3$.ɵɵdomElementEnd(); + $r3$.ɵɵtext(1); + } + if (rf & 2) { + const $foo_r1$ = 123; + $r3$.ɵɵadvance(); + $r3$.ɵɵtextInterpolate1(" ", $foo_r1$, " "); + } + }, + … +}); diff --git a/packages/compiler/src/template/pipeline/src/phases/variable_optimization.ts b/packages/compiler/src/template/pipeline/src/phases/variable_optimization.ts index 3ef49a1102b..fda13e35b7c 100644 --- a/packages/compiler/src/template/pipeline/src/phases/variable_optimization.ts +++ b/packages/compiler/src/template/pipeline/src/phases/variable_optimization.ts @@ -51,16 +51,9 @@ export function optimizeVariables(job: CompilationJob): void { for (const expr of unit.functions) { optimizeVariablesInOpList(expr.ops, job.compatibility, null); + optimizeSaveRestoreView(expr.ops); } - // Note that we skip over arrow function operations, because they are considered - // separate boundaries that should not influence the surrounding create/update - // operations. This is a side-effect of not being able to control which nested - // ops `visitExpressionsInOp` will visit. Without this logic, variable references - // inside the arrow function can throw off usage counting for things like view references. - optimizeVariablesInOpList(unit.create, job.compatibility, skipArrowFunctionOps); - optimizeVariablesInOpList(unit.update, job.compatibility, skipArrowFunctionOps); - for (const op of unit.create) { if ( op.kind === ir.OpKind.Listener || @@ -69,10 +62,19 @@ export function optimizeVariables(job: CompilationJob): void { op.kind === ir.OpKind.TwoWayListener ) { optimizeVariablesInOpList(op.handlerOps, job.compatibility, skipArrowFunctionOps); + optimizeSaveRestoreView(op.handlerOps); } else if (op.kind === ir.OpKind.RepeaterCreate && op.trackByOps !== null) { optimizeVariablesInOpList(op.trackByOps, job.compatibility, skipArrowFunctionOps); } } + + // Note that we skip over arrow function operations, because they are considered + // separate boundaries that should not influence the surrounding create/update + // operations. This is a side-effect of not being able to control which nested + // ops `visitExpressionsInOp` will visit. Without this logic, variable references + // inside the arrow function can throw off usage counting for things like view references. + optimizeVariablesInOpList(unit.create, job.compatibility, skipArrowFunctionOps); + optimizeVariablesInOpList(unit.update, job.compatibility, skipArrowFunctionOps); } } @@ -563,3 +565,31 @@ function allowConservativeInlining( return true; } } + +/** + * After variables have been optimized in nested ops (e.g. handlers or functions), we may end up + * with `saveView`/`restoreView` calls that aren't necessary since all the references to the view + * were optimized away. This function removes the ops related to the view restoration. + */ +function optimizeSaveRestoreView(ops: ir.OpList): void { + const head = ops.head.next; + const tail = ops.tail.prev; + + // We can only optimize if we have two ops: + // 1. A call to `restoreView`. + // 2. A return statement with a `resetView` in it. + if ( + head !== null && + tail !== null && + head.next === tail && + head.kind === ir.OpKind.Statement && + head.statement instanceof o.ExpressionStatement && + head.statement.expr instanceof ir.RestoreViewExpr && + tail.kind === ir.OpKind.Statement && + tail.statement instanceof o.ReturnStatement && + tail.statement.value instanceof ir.ResetViewExpr + ) { + ir.OpList.remove(head); + tail.statement.value = tail.statement.value.expr; + } +}