angular · crisbeto · Jul 23, 2024 · Jul 23, 2024
diff --git a/goldens/public-api/compiler-cli/error_code.api.md b/goldens/public-api/compiler-cli/error_code.api.md
@@ -105,6 +105,7 @@ export enum ErrorCode {
     UNDECORATED_PROVIDER = 2005,
     UNINVOKED_FUNCTION_IN_EVENT_BINDING = 8111,
     UNSUPPORTED_INITIALIZER_API_USAGE = 8110,
+    UNUSED_LET_DECLARATION = 8112,
     // (undocumented)
     VALUE_HAS_WRONG_TYPE = 1010,
     // (undocumented)

diff --git a/goldens/public-api/compiler-cli/extended_template_diagnostic_name.api.md b/goldens/public-api/compiler-cli/extended_template_diagnostic_name.api.md
@@ -27,7 +27,9 @@ export enum ExtendedTemplateDiagnosticName {
     // (undocumented)
     TEXT_ATTRIBUTE_NOT_BINDING = "textAttributeNotBinding",
     // (undocumented)
-    UNINVOKED_FUNCTION_IN_EVENT_BINDING = "uninvokedFunctionInEventBinding"
+    UNINVOKED_FUNCTION_IN_EVENT_BINDING = "uninvokedFunctionInEventBinding",
+    // (undocumented)
+    UNUSED_LET_DECLARATION = "unusedLetDeclaration"
 }
 
 // (No @packageDocumentation comment for this package)

diff --git a/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts b/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts
@@ -495,6 +495,19 @@ export enum ErrorCode {
    */
   UNINVOKED_FUNCTION_IN_EVENT_BINDING = 8111,
 
+  /**
+   * A `@let` declaration in a template isn't used.
+   *
+   * For example:
+   * ```
+   * @let used = 1; <!-- Not an error -->
+   * @let notUsed = 2; <!-- Error -->
+   *
+   * {{used}}
+   * ```
+   */
+  UNUSED_LET_DECLARATION = 8112,
+
   /**
    * The template type-checking engine would need to generate an inline type check block for a
    * component, but the current type-checking environment doesn't support it.

diff --git a/packages/compiler-cli/src/ngtsc/diagnostics/src/extended_template_diagnostic_name.ts b/packages/compiler-cli/src/ngtsc/diagnostics/src/extended_template_diagnostic_name.ts
@@ -27,4 +27,5 @@ export enum ExtendedTemplateDiagnosticName {
   SKIP_HYDRATION_NOT_STATIC = 'skipHydrationNotStatic',
   INTERPOLATED_SIGNAL_NOT_INVOKED = 'interpolatedSignalNotInvoked',
   CONTROL_FLOW_PREVENTING_CONTENT_PROJECTION = 'controlFlowPreventingContentProjection',
+  UNUSED_LET_DECLARATION = 'unusedLetDeclaration',
 }
diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/BUILD.bazel b/packages/compiler-cli/src/ngtsc/typecheck/extended/BUILD.bazel
@@ -22,6 +22,7 @@ ts_library(
         "//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/suffix_not_supported",
         "//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/text_attribute_not_binding",
         "//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/uninvoked_function_in_event_binding",
+        "//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/unused_let_declaration",
         "@npm//typescript",
     ],
 )
diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/api/api.ts b/packages/compiler-cli/src/ngtsc/typecheck/extended/api/api.ts
@@ -109,6 +109,17 @@ export interface TemplateCheckFactory<
 export abstract class TemplateCheckWithVisitor<Code extends ErrorCode>
   implements TemplateCheck<Code>
 {
+  /**
+   * When extended diagnostics were first introduced, the visitor wasn't implemented correctly
+   * which meant that it wasn't visiting the `templateAttrs` of structural directives (e.g.
+   * the expression of `*ngIf`). Fixing the issue causes a lot of internal breakages and will likely
+   * need to be done in a major version to avoid external breakages. This flag is used to opt out
+   * pre-existing diagnostics from the correct behavior until the breakages have been fixed while
+   * ensuring that newly-written diagnostics are correct from the beginning.
+   * TODO(crisbeto): remove this flag and fix the internal brekages.
+   */
+  readonly canVisitStructuralAttributes: boolean = true;
+
   abstract code: Code;
 
   /**
@@ -179,15 +190,23 @@ class TemplateVisitor<Code extends ErrorCode>
   }
 
   visitTemplate(template: TmplAstTemplate) {
+    const isInlineTemplate = template.tagName === 'ng-template';
     this.visitAllNodes(template.attributes);
-    if (template.tagName === 'ng-template') {
-      // Only visit input/outputs/templateAttrs if this isn't an inline template node
-      // generated for a structural directive (like `<div *ngIf></div>`). These nodes
-      // would be visited when the underlying element of an inline template node is processed.
+
+    if (isInlineTemplate) {
+      // Only visit input/outputs if this isn't an inline template node generated for a structural
+      // directive (like `<div *ngIf></div>`). These nodes would be visited when the underlying
+      // element of an inline template node is processed.
       this.visitAllNodes(template.inputs);
       this.visitAllNodes(template.outputs);
+    }
+
+    // TODO(crisbeto): remove this condition when deleting `canVisitStructuralAttributes`.
+    if (this.check.canVisitStructuralAttributes || isInlineTemplate) {
+      // `templateAttrs` aren't transferred over to the inner element so we always have to visit them.
       this.visitAllNodes(template.templateAttrs);
     }
+
     this.visitAllNodes(template.variables);
     this.visitAllNodes(template.references);
     this.visitAllNodes(template.children);

diff --git a/...compiler-cli/src/ngtsc/typecheck/extended/checks/nullish_coalescing_not_nullable/index.ts b/...compiler-cli/src/ngtsc/typecheck/extended/checks/nullish_coalescing_not_nullable/index.ts
@@ -21,6 +21,7 @@ import {TemplateCheckFactory, TemplateCheckWithVisitor, TemplateContext} from '.
  * otherwise it would produce inaccurate results.
  */
 class NullishCoalescingNotNullableCheck extends TemplateCheckWithVisitor<ErrorCode.NULLISH_COALESCING_NOT_NULLABLE> {
+  override readonly canVisitStructuralAttributes = false;
   override code = ErrorCode.NULLISH_COALESCING_NOT_NULLABLE as const;
 
   override visitNode(

diff --git a/...ges/compiler-cli/src/ngtsc/typecheck/extended/checks/optional_chain_not_nullable/index.ts b/...ges/compiler-cli/src/ngtsc/typecheck/extended/checks/optional_chain_not_nullable/index.ts
@@ -21,6 +21,7 @@ import {TemplateCheckFactory, TemplateCheckWithVisitor, TemplateContext} from '.
  * otherwise it would produce inaccurate results.
  */
 class OptionalChainNotNullableCheck extends TemplateCheckWithVisitor<ErrorCode.OPTIONAL_CHAIN_NOT_NULLABLE> {
+  override readonly canVisitStructuralAttributes = false;
   override code = ErrorCode.OPTIONAL_CHAIN_NOT_NULLABLE as const;
 
   override visitNode(

diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/unused_let_declaration/BUILD.bazel b/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/unused_let_declaration/BUILD.bazel
@@ -0,0 +1,17 @@
+load("//tools:defaults.bzl", "ts_library")
+
+ts_library(
+    name = "unused_let_declaration",
+    srcs = ["index.ts"],
+    visibility = [
+        "//packages/compiler-cli/src/ngtsc:__subpackages__",
+        "//packages/compiler-cli/test/ngtsc:__pkg__",
+    ],
+    deps = [
+        "//packages/compiler",
+        "//packages/compiler-cli/src/ngtsc/diagnostics",
+        "//packages/compiler-cli/src/ngtsc/typecheck/api",
+        "//packages/compiler-cli/src/ngtsc/typecheck/extended/api",
+        "@npm//typescript",
+    ],
+)
diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/unused_let_declaration/index.ts b/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/unused_let_declaration/index.ts
@@ -0,0 +1,87 @@
+/**
+ * @license
+ * Copyright Google LLC All Rights Reserved.
+ *
+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.io/license
+ */
+
+import {AST, ASTWithSource, TmplAstLetDeclaration, TmplAstNode} from '@angular/compiler';
+import ts from 'typescript';
+
+import {ErrorCode, ExtendedTemplateDiagnosticName} from '../../../../diagnostics';
+import {NgTemplateDiagnostic} from '../../../api';
+import {TemplateCheckFactory, TemplateCheckWithVisitor, TemplateContext} from '../../api';
+
+interface ClassAnalysis {
+  allLetDeclarations: Set<TmplAstLetDeclaration>;
+  usedLetDeclarations: Set<TmplAstLetDeclaration>;
+}
+
+/**
+ * Ensures that all `@let` declarations in a template are used.
+ */
+class UnusedLetDeclarationCheck extends TemplateCheckWithVisitor<ErrorCode.UNUSED_LET_DECLARATION> {
+  override code = ErrorCode.UNUSED_LET_DECLARATION as const;
+  private analysis = new Map<ts.ClassDeclaration, ClassAnalysis>();
+
+  override run(
+    ctx: TemplateContext<ErrorCode.UNUSED_LET_DECLARATION>,
+    component: ts.ClassDeclaration,
+    template: TmplAstNode[],
+  ): NgTemplateDiagnostic<ErrorCode.UNUSED_LET_DECLARATION>[] {
+    super.run(ctx, component, template);
+
+    const diagnostics: NgTemplateDiagnostic<ErrorCode.UNUSED_LET_DECLARATION>[] = [];
+    const {allLetDeclarations, usedLetDeclarations} = this.getAnalysis(component);
+
+    for (const decl of allLetDeclarations) {
+      if (!usedLetDeclarations.has(decl)) {
+        diagnostics.push(
+          ctx.makeTemplateDiagnostic(
+            decl.sourceSpan,
+            `@let ${decl.name} is declared but its value is never read.`,
+          ),
+        );
+      }
+    }
+
+    this.analysis.clear();
+    return diagnostics;
+  }
+
+  override visitNode(
+    ctx: TemplateContext<ErrorCode.UNUSED_LET_DECLARATION>,
+    component: ts.ClassDeclaration,
+    node: TmplAstNode | AST,
+  ): NgTemplateDiagnostic<ErrorCode.UNUSED_LET_DECLARATION>[] {
+    if (node instanceof TmplAstLetDeclaration) {
+      this.getAnalysis(component).allLetDeclarations.add(node);
+    } else if (node instanceof AST) {
+      const unwrappedNode = node instanceof ASTWithSource ? node.ast : node;
+      const target = ctx.templateTypeChecker.getExpressionTarget(unwrappedNode, component);
+
+      if (target !== null && target instanceof TmplAstLetDeclaration) {
+        this.getAnalysis(component).usedLetDeclarations.add(target);
+      }
+    }
+
+    return [];
+  }
+
+  private getAnalysis(node: ts.ClassDeclaration): ClassAnalysis {
+    if (!this.analysis.has(node)) {
+      this.analysis.set(node, {allLetDeclarations: new Set(), usedLetDeclarations: new Set()});
+    }
+    return this.analysis.get(node)!;
+  }
+}
+
+export const factory: TemplateCheckFactory<
+  ErrorCode.UNUSED_LET_DECLARATION,
+  ExtendedTemplateDiagnosticName.UNUSED_LET_DECLARATION
+> = {
+  code: ErrorCode.UNUSED_LET_DECLARATION,
+  name: ExtendedTemplateDiagnosticName.UNUSED_LET_DECLARATION,
+  create: () => new UnusedLetDeclarationCheck(),
+};
diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/index.ts b/packages/compiler-cli/src/ngtsc/typecheck/extended/index.ts
@@ -18,6 +18,7 @@ import {factory as optionalChainNotNullableFactory} from './checks/optional_chai
 import {factory as suffixNotSupportedFactory} from './checks/suffix_not_supported';
 import {factory as textAttributeNotBindingFactory} from './checks/text_attribute_not_binding';
 import {factory as uninvokedFunctionInEventBindingFactory} from './checks/uninvoked_function_in_event_binding';
+import {factory as unusedLetDeclarationFactory} from './checks/unused_let_declaration';
 
 export {ExtendedTemplateCheckerImpl} from './src/extended_template_checker';
 
@@ -34,6 +35,7 @@ export const ALL_DIAGNOSTIC_FACTORIES: readonly TemplateCheckFactory<
   suffixNotSupportedFactory,
   interpolatedSignalNotInvoked,
   uninvokedFunctionInEventBindingFactory,
+  unusedLetDeclarationFactory,
 ];
 
 export const SUPPORTED_DIAGNOSTIC_NAMES = new Set<string>([

diff --git a/.../compiler-cli/src/ngtsc/typecheck/extended/test/checks/unused_let_declaration/BUILD.bazel b/.../compiler-cli/src/ngtsc/typecheck/extended/test/checks/unused_let_declaration/BUILD.bazel
@@ -0,0 +1,30 @@
+load("//tools:defaults.bzl", "jasmine_node_test", "ts_library")
+
+ts_library(
+    name = "test_lib",
+    testonly = True,
+    srcs = ["unused_let_declaration_spec.ts"],
+    deps = [
+        "//packages/compiler",
+        "//packages/compiler-cli/src/ngtsc/core:api",
+        "//packages/compiler-cli/src/ngtsc/diagnostics",
+        "//packages/compiler-cli/src/ngtsc/file_system",
+        "//packages/compiler-cli/src/ngtsc/file_system/testing",
+        "//packages/compiler-cli/src/ngtsc/testing",
+        "//packages/compiler-cli/src/ngtsc/typecheck/extended",
+        "//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/unused_let_declaration",
+        "//packages/compiler-cli/src/ngtsc/typecheck/testing",
+        "@npm//typescript",
+    ],
+)
+
+jasmine_node_test(
+    name = "test",
+    bootstrap = ["//tools/testing:node_no_angular"],
+    data = [
+        "//packages/core:npm_package",
+    ],
+    deps = [
+        ":test_lib",
+    ],
+)
diff --git a/...gtsc/typecheck/extended/test/checks/unused_let_declaration/unused_let_declaration_spec.ts b/...gtsc/typecheck/extended/test/checks/unused_let_declaration/unused_let_declaration_spec.ts
@@ -0,0 +1,112 @@
+/**
+ * @license
+ * Copyright Google LLC All Rights Reserved.
+ *
+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.io/license
+ */
+
+import ts from 'typescript';
+
+import {ErrorCode, ExtendedTemplateDiagnosticName, ngErrorCode} from '../../../../../diagnostics';
+import {absoluteFrom, getSourceFileOrError} from '../../../../../file_system';
+import {runInEachFileSystem} from '../../../../../file_system/testing';
+import {getSourceCodeForDiagnostic} from '../../../../../testing';
+import {getClass, setup} from '../../../../testing';
+import {factory as unusedLetDeclarationFactory} from '../../../checks/unused_let_declaration/index';
+import {ExtendedTemplateCheckerImpl} from '../../../src/extended_template_checker';
+
+runInEachFileSystem(() => {
+  describe('UnusedLetDeclarationCheck', () => {
+    function diagnose(template: string) {
+      const fileName = absoluteFrom('/main.ts');
+      const {program, templateTypeChecker} = setup([
+        {
+          fileName,
+          templates: {
+            'TestCmp': template,
+          },
+          source: `
+            export class TestCmp {
+              eventCallback(value: any) {}
+            }
+          `,
+        },
+      ]);
+      const sf = getSourceFileOrError(program, fileName);
+      const component = getClass(sf, 'TestCmp');
+      const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
+        templateTypeChecker,
+        program.getTypeChecker(),
+        [unusedLetDeclarationFactory],
+        {},
+      );
+      return extendedTemplateChecker.getDiagnosticsForComponent(component);
+    }
+
+    it('binds the error code to its extended template diagnostic name', () => {
+      expect(unusedLetDeclarationFactory.code).toBe(ErrorCode.UNUSED_LET_DECLARATION);
+      expect(unusedLetDeclarationFactory.name).toBe(
+        ExtendedTemplateDiagnosticName.UNUSED_LET_DECLARATION,
+      );
+    });
+
+    it('should report a @let declaration that is not used', () => {
+      const diags = diagnose(`
+        @let used = 1;
+        @let unused = 2;
+        {{used}}
+      `);
+
+      expect(diags.length).toBe(1);
+      expect(diags[0].category).toBe(ts.DiagnosticCategory.Warning);
+      expect(diags[0].code).toBe(ngErrorCode(ErrorCode.UNUSED_LET_DECLARATION));
+      expect(getSourceCodeForDiagnostic(diags[0])).toBe('@let unused = 2');
+    });
+
+    it('should report a @let declaration that is not used', () => {
+      const diags = diagnose(`
+        @let foo = 1;
+
+        @if (true) {
+          @let foo = 2;
+          {{foo}}
+        }
+      `);
+
+      expect(diags.length).toBe(1);
+      expect(diags[0].category).toBe(ts.DiagnosticCategory.Warning);
+      expect(diags[0].code).toBe(ngErrorCode(ErrorCode.UNUSED_LET_DECLARATION));
+      expect(getSourceCodeForDiagnostic(diags[0])).toBe('@let foo = 1');
+    });
+
+    it('should not report a @let declaration that is only used in other @let declarations', () => {
+      const diags = diagnose(`
+        @let one = 1;
+        @let two = 2;
+        @let three = one + two;
+        {{three}}
+      `);
+
+      expect(diags.length).toBe(0);
+    });
+
+    it('should not report a @let declaration that is only used in an event listener', () => {
+      const diags = diagnose(`
+        @let foo = 1;
+        <button (click)="eventCallback(foo + 1)">Click me</button>
+      `);
+
+      expect(diags.length).toBe(0);
+    });
+
+    it('should not report a @let declaration that is only used in a structural directive', () => {
+      const diags = diagnose(`
+        @let foo = null;
+        <div *ngIf="foo"></div>
+      `);
+
+      expect(diags.length).toBe(0);
+    });
+  });
+});