fix(compiler): give precedence to local let declarations over parent …

…ones (#56752) Currently the logic that maps a name to a variable looks at the variables in their definition order. This means that `@let` declarations from parent views will always come before local ones, because the local ones are declared inline whereas the parent ones are hoisted to the top of the function. These changes resolve the issue by giving precedence to the local variables. Fixes #56737. PR Close #56752
angular · Jul 1, 2024 · 2a1291e · 2a1291e
1 parent 4d18c5b
commit 2a1291e
Show file tree

Hide file tree

Showing 9 changed files with 146 additions and 1 deletion.
diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_let/GOLDEN_PARTIAL.js b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_let/GOLDEN_PARTIAL.js
@@ -715,3 +715,43 @@ export declare class MyApp {
     static ɵcmp: i0.ɵɵComponentDeclaration<MyApp, "ng-component", never, {}, {}, never, never, true, never>;
 }
 
+/****************************************************************************************************
+ * PARTIAL FILE: shadowed_let.js
+ ****************************************************************************************************/
+import { Component } from '@angular/core';
+import * as i0 from "@angular/core";
+export class MyApp {
+}
+MyApp.ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: MyApp, deps: [], target: i0.ɵɵFactoryTarget.Component });
+MyApp.ɵcmp = i0.ɵɵngDeclareComponent({ minVersion: "17.0.0", version: "0.0.0-PLACEHOLDER", type: MyApp, isStandalone: true, selector: "ng-component", ngImport: i0, template: `
+    @let value = 'parent';
+
+    @if (true) {
+      @let value = 'local';
+      The value comes from {{value}}
+    }
+  `, isInline: true });
+i0.ɵɵngDeclareClassMetadata({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: MyApp, decorators: [{
+            type: Component,
+            args: [{
+                    template: `
+    @let value = 'parent';
+
+    @if (true) {
+      @let value = 'local';
+      The value comes from {{value}}
+    }
+  `,
+                    standalone: true,
+                }]
+        }] });
+
+/****************************************************************************************************
+ * PARTIAL FILE: shadowed_let.d.ts
+ ****************************************************************************************************/
+import * as i0 from "@angular/core";
+export declare class MyApp {
+    static ɵfac: i0.ɵɵFactoryDeclaration<MyApp, never>;
+    static ɵcmp: i0.ɵɵComponentDeclaration<MyApp, "ng-component", never, {}, {}, never, never, true, never>;
+}
+
diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_let/TEST_CASES.json b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_let/TEST_CASES.json
@@ -275,6 +275,23 @@
           "failureMessage": "Incorrect template"
         }
       ]
+    },
+    {
+      "description": "should give precedence to local @let definition over one from a parent view",
+      "inputFiles": [
+        "shadowed_let.ts"
+      ],
+      "expectations": [
+        {
+          "files": [
+            {
+              "expected": "shadowed_let_template.js",
+              "generated": "shadowed_let.js"
+            }
+          ],
+          "failureMessage": "Incorrect template"
+        }
+      ]
     }
   ]
 }
diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_let/shadowed_let.ts b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_let/shadowed_let.ts
@@ -0,0 +1,14 @@
+import {Component} from '@angular/core';
+
+@Component({
+  template: `
+    @let value = 'parent';
+
+    @if (true) {
+      @let value = 'local';
+      The value comes from {{value}}
+    }
+  `,
+  standalone: true,
+})
+export class MyApp {}
diff --git a/...ges/compiler-cli/test/compliance/test_cases/r3_view_compiler_let/shadowed_let_template.js b/...ges/compiler-cli/test/compliance/test_cases/r3_view_compiler_let/shadowed_let_template.js
@@ -0,0 +1,31 @@
+function MyApp_Conditional_1_Template(rf, ctx) {
+  if (rf & 1) {
+    $r3$.ɵɵdeclareLet(0);
+    $r3$.ɵɵtext(1);
+  }
+  if (rf & 2) {
+    const $value_r1$ = "local";
+    $r3$.ɵɵadvance();
+    $r3$.ɵɵtextInterpolate1(" The value comes from ", $value_r1$, " ");
+  }
+}
+
+…
+
+$r3$.ɵɵdefineComponent({
+  …
+  decls: 2,
+  vars: 1,
+  template: function MyApp_Template(rf, ctx) {
+    if (rf & 1) {
+      $r3$.ɵɵdeclareLet(0);
+      $r3$.ɵɵtemplate(1, MyApp_Conditional_1_Template, 2, 1);
+    }
+    if (rf & 2) {
+      "parent";
+      $r3$.ɵɵadvance();
+      $r3$.ɵɵconditional(true ? 1 : -1);
+    }
+  },
+  …
+});
diff --git a/packages/compiler/src/template/pipeline/ir/src/variable.ts b/packages/compiler/src/template/pipeline/ir/src/variable.ts
@@ -56,6 +56,11 @@ export interface IdentifierVariable extends SemanticVariableBase {
    * The identifier whose value in the template is tracked in this variable.
    */
   identifier: string;
+
+  /**
+   * Whether the variable was declared locally within the same view or somewhere else.
+   */
+  local: boolean;
 }
 
 /**

diff --git a/packages/compiler/src/template/pipeline/src/phases/generate_local_let_references.ts b/packages/compiler/src/template/pipeline/src/phases/generate_local_let_references.ts
@@ -25,6 +25,7 @@ export function generateLocalLetReferences(job: ComponentCompilationJob): void {
         kind: ir.SemanticVariableKind.Identifier,
         name: null,
         identifier: op.declaredName,
+        local: true,
       };
 
       ir.OpList.replace<ir.UpdateOp>(

diff --git a/packages/compiler/src/template/pipeline/src/phases/generate_variables.ts b/packages/compiler/src/template/pipeline/src/phases/generate_variables.ts
@@ -167,6 +167,7 @@ function getScopeForView(view: ViewCompilationUnit, parent: Scope | null): Scope
       kind: ir.SemanticVariableKind.Identifier,
       name: null,
       identifier,
+      local: false,
     });
   }
 
@@ -189,6 +190,7 @@ function getScopeForView(view: ViewCompilationUnit, parent: Scope | null): Scope
               kind: ir.SemanticVariableKind.Identifier,
               name: null,
               identifier: op.localRefs[offset].name,
+              local: false,
             },
           });
         }
@@ -202,6 +204,7 @@ function getScopeForView(view: ViewCompilationUnit, parent: Scope | null): Scope
             kind: ir.SemanticVariableKind.Identifier,
             name: null,
             identifier: op.declaredName,
+            local: false,
           },
         });
         break;

diff --git a/packages/compiler/src/template/pipeline/src/phases/resolve_names.ts b/packages/compiler/src/template/pipeline/src/phases/resolve_names.ts
@@ -36,6 +36,9 @@ function processLexicalScope(
   // identifiers from parent templates) only local variables need be considered here.
   const scope = new Map<string, ir.XrefId>();
 
+  // Symbols defined within the current scope. They take precedence over ones defined outside.
+  const localDefinitions = new Map<string, ir.XrefId>();
+
   // First, step through the operations list and:
   // 1) build up the `scope` mapping
   // 2) recurse into any listener functions
@@ -44,6 +47,16 @@ function processLexicalScope(
       case ir.OpKind.Variable:
         switch (op.variable.kind) {
           case ir.SemanticVariableKind.Identifier:
+            if (op.variable.local) {
+              if (localDefinitions.has(op.variable.identifier)) {
+                continue;
+              }
+              localDefinitions.set(op.variable.identifier, op.xref);
+            } else if (scope.has(op.variable.identifier)) {
+              continue;
+            }
+            scope.set(op.variable.identifier, op.xref);
+            break;
           case ir.SemanticVariableKind.Alias:
             // This variable represents some kind of identifier which can be used in the template.
             if (scope.has(op.variable.identifier)) {
@@ -85,7 +98,9 @@ function processLexicalScope(
           // `expr` is a read of a name within the lexical scope of this view.
           // Either that name is defined within the current view, or it represents a property from the
           // main component context.
-          if (scope.has(expr.name)) {
+          if (localDefinitions.has(expr.name)) {
+            return new ir.ReadVariableExpr(localDefinitions.get(expr.name)!);
+          } else if (scope.has(expr.name)) {
             // This was a defined variable in the current scope.
             return new ir.ReadVariableExpr(scope.get(expr.name)!);
           } else {

diff --git a/packages/core/test/acceptance/let_spec.ts b/packages/core/test/acceptance/let_spec.ts
@@ -426,6 +426,25 @@ describe('@let declarations', () => {
     expect(fixture.nativeElement.textContent).toContain('The value comes from @let');
   });
 
+  it('should give precedence to local @let definition over one from a parent view', () => {
+    @Component({
+      standalone: true,
+      template: `
+        @let value = 'parent';
+
+        @if (true) {
+          @let value = 'local';
+          The value comes from {{value}}
+        }
+      `,
+    })
+    class TestComponent {}
+
+    const fixture = TestBed.createComponent(TestComponent);
+    fixture.detectChanges();
+    expect(fixture.nativeElement.textContent).toContain('The value comes from local');
+  });
+
   it('should be able to use @for loop variables in @let declarations', () => {
     @Component({
       standalone: true,