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; + } +}