refactor(compiler-cli): add diagnostic for duplicate let declarations (#56199)

crisbeto · thePunderWoman · commit 9aea8a0576a0 · 2024-06-04T17:28:03.000Z
Adds a template diagnostic that will flag cases where multiple `@let` declarations use the same name. PR Close #56199
diff --git a/goldens/public-api/compiler-cli/error_code.api.md b/goldens/public-api/compiler-cli/error_code.api.md
@@ -42,6 +42,7 @@ export enum ErrorCode {
     DIRECTIVE_INHERITS_UNDECORATED_CTOR = 2006,
     // (undocumented)
     DIRECTIVE_MISSING_SELECTOR = 2004,
+    DUPLICATE_LET_DECLARATION = 8017,
     DUPLICATE_VARIABLE_DECLARATION = 8006,
     HOST_BINDING_PARSE_ERROR = 5001,
     HOST_DIRECTIVE_COMPONENT = 2015,
diff --git a/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts b/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts
@@ -364,6 +364,9 @@ export enum ErrorCode {
   /** An expression is trying to read an `@let` before it has been defined. */
   LET_USED_BEFORE_DEFINITION = 8016,
 
+  /** Multiple `@let` declarations were defined with the same name within a scope. */
+  DUPLICATE_LET_DECLARATION = 8017,
+
   /**
    * A two way binding in a template has an incorrect syntax,
    * parentheses outside brackets. For example:
diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts
@@ -185,6 +185,15 @@ export interface OutOfBandDiagnosticRecorder {
     node: PropertyRead,
     target: TmplAstLetDeclaration,
   ): void;
+
+  /**
+   * Reports a duplicate `@let` declaration within the same scope.
+   *
+   * @param templateId the template type-checking ID of the template which contains the duplicate
+   * declaration.
+   * @param current the `TmplAstLetDeclaration` which duplicates a previous declaration.
+   */
+  duplicateLetDeclaration(templateId: TemplateId, current: TmplAstLetDeclaration): void;
 }
 
 export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecorder {
@@ -640,6 +649,22 @@ export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecor
       ),
     );
   }
+
+  duplicateLetDeclaration(templateId: TemplateId, current: TmplAstLetDeclaration): void {
+    const mapping = this.resolver.getSourceMapping(templateId);
+    const errorMsg = `Cannot declare @let called '${current.name}' as there is another @let declaration with the same name.`;
+
+    this._diagnostics.push(
+      makeTemplateDiagnostic(
+        templateId,
+        mapping,
+        current.sourceSpan,
+        ts.DiagnosticCategory.Error,
+        ngErrorCode(ErrorCode.DUPLICATE_LET_DECLARATION),
+        errorMsg,
+      ),
+    );
+  }
 }
 
 function makeInlineDiagnostic(
diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts
@@ -2021,9 +2021,11 @@ class Scope {
   private varMap = new Map<TmplAstVariable, number | ts.Identifier>();
 
   /**
-   * A map of `TmplAstLetDeclaration`s to the index of their op in the `opQueue`.
+   * A map of the names of `TmplAstLetDeclaration`s to the index of their op in the `opQueue`.
+   *
+   * Assumes that there won't be duplicated `@let` declarations within the same scope.
    */
-  private letDeclOpMap = new Map<TmplAstLetDeclaration, number>();
+  private letDeclOpMap = new Map<string, number>();
 
   /**
    * Statements for this template.
@@ -2132,7 +2134,11 @@ class Scope {
 
       if (node instanceof TmplAstLetDeclaration) {
         const opIndex = scope.opQueue.push(new TcbLetDeclarationOp(tcb, scope, node)) - 1;
-        scope.letDeclOpMap.set(node, opIndex);
+        if (scope.letDeclOpMap.has(node.name)) {
+          tcb.oobRecorder.duplicateLetDeclaration(tcb.id, node);
+        } else {
+          scope.letDeclOpMap.set(node.name, opIndex);
+        }
       }
     }
     return scope;
@@ -2255,7 +2261,7 @@ class Scope {
       return this.varMap.has(node);
     }
     if (node instanceof TmplAstLetDeclaration) {
-      return this.letDeclOpMap.has(node);
+      return this.letDeclOpMap.has(node.name);
     }
     return this.referenceOpMap.has(node);
   }
@@ -2266,8 +2272,8 @@ class Scope {
   ): ts.Expression | null {
     if (ref instanceof TmplAstReference && this.referenceOpMap.has(ref)) {
       return this.resolveOp(this.referenceOpMap.get(ref)!);
-    } else if (ref instanceof TmplAstLetDeclaration && this.letDeclOpMap.has(ref)) {
-      return this.resolveOp(this.letDeclOpMap.get(ref)!);
+    } else if (ref instanceof TmplAstLetDeclaration && this.letDeclOpMap.has(ref.name)) {
+      return this.resolveOp(this.letDeclOpMap.get(ref.name)!);
     } else if (ref instanceof TmplAstVariable && this.varMap.has(ref)) {
       // Resolving a context variable for this template.
       // Execute the `TcbVariableOp` associated with the `TmplAstVariable`.
diff --git a/packages/compiler-cli/src/ngtsc/typecheck/testing/index.ts b/packages/compiler-cli/src/ngtsc/typecheck/testing/index.ts
@@ -1039,4 +1039,5 @@ export class NoopOobRecorder implements OutOfBandDiagnosticRecorder {
     node: PropertyRead,
     target: TmplAstLetDeclaration,
   ): void {}
+  duplicateLetDeclaration(templateId: TemplateId, current: TmplAstLetDeclaration): void {}
 }
diff --git a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts
@@ -6876,6 +6876,34 @@ suppress
         );
       });
 
+      it('should not allow multiple @let declarations with the same name within a scope', () => {
+        env.write(
+          'test.ts',
+          `
+          import {Component} from '@angular/core';
+
+          @Component({
+            template: \`
+              @if (true) {
+                @let value = 1;
+                @let value = 'one';
+                {{value}}
+              }
+            \`,
+            standalone: true,
+          })
+          export class Main {
+          }
+        `,
+        );
+
+        const diags = env.driveDiagnostics();
+        expect(diags.length).toBe(1);
+        expect(diags[0].messageText).toBe(
+          `Cannot declare @let called 'value' as there is another @let declaration with the same name.`,
+        );
+      });
+
       it('should not allow a let declaration to be referenced before it is defined', () => {
         env.write(
           'test.ts',

Original file line number	Diff line number	Diff line change
`@@ -1039,4 +1039,5 @@ export class NoopOobRecorder implements OutOfBandDiagnosticRecorder {`
`1039`	`1039`	`node: PropertyRead,`
`1040`	`1040`	`target: TmplAstLetDeclaration,`
`1041`	`1041`	`): void {}`
	`1042`	`+ duplicateLetDeclaration(templateId: TemplateId, current: TmplAstLetDeclaration): void {}`
`1042`	`1043`	`}`