feat(compiler): Add extended diagnostic to warn when missing let on n…

goldens/public-api/compiler-cli/error_code.md

@@ -46,6 +46,7 @@ export enum ErrorCode {
     INLINE_TYPE_CTOR_REQUIRED = 8901,
     INVALID_BANANA_IN_BOX = 8101,
     MISSING_CONTROL_FLOW_DIRECTIVE = 8103,
+    MISSING_NGFOROF_LET = 8105,
     MISSING_PIPE = 8004,
     MISSING_REFERENCE_TARGET = 8003,
     NGMODULE_BOOTSTRAP_IS_STANDALONE = 6009,

goldens/public-api/compiler-cli/extended_template_diagnostic_name.md

@@ -11,6 +11,8 @@ export enum ExtendedTemplateDiagnosticName {
     // (undocumented)
     MISSING_CONTROL_FLOW_DIRECTIVE = "missingControlFlowDirective",
     // (undocumented)
+    MISSING_NGFOROF_LET = "missingNgForOfLet",
+    // (undocumented)
     NULLISH_COALESCING_NOT_NULLABLE = "nullishCoalescingNotNullable",
     // (undocumented)
     TEXT_ATTRIBUTE_NOT_BINDING = "textAttributeNotBinding"

packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts

@@ -256,6 +256,17 @@ export enum ErrorCode {
    */
   TEXT_ATTRIBUTE_NOT_BINDING = 8104,
 
+  /**
+   * NgForOf is used in a template, but the user forgot to include let
+   * in their statement.
+   *
+   * For example:
+   * ```
+   * <ul><li *ngFor="item of items">{{item["name"]}};</li></ul>
+   * ```
+   */
+  MISSING_NGFOROF_LET = 8105,
+
   /**
    * 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.

packages/compiler-cli/src/ngtsc/diagnostics/src/extended_template_diagnostic_name.ts

@@ -20,4 +20,5 @@ export enum ExtendedTemplateDiagnosticName {
   NULLISH_COALESCING_NOT_NULLABLE = 'nullishCoalescingNotNullable',
   MISSING_CONTROL_FLOW_DIRECTIVE = 'missingControlFlowDirective',
   TEXT_ATTRIBUTE_NOT_BINDING = 'textAttributeNotBinding',
+  MISSING_NGFOROF_LET = 'missingNgForOfLet'
 }

packages/compiler-cli/src/ngtsc/typecheck/extended/BUILD.bazel

@@ -14,6 +14,7 @@ ts_library(
         "//packages/compiler-cli/src/ngtsc/typecheck/extended/api",
         "//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/invalid_banana_in_box",
         "//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/missing_control_flow_directive",
+        "//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/missing_ngforof_let",
         "//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/nullish_coalescing_not_nullable",
         "//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/text_attribute_not_binding",
         "@npm//typescript",

packages/compiler-cli/src/ngtsc/typecheck/extended/checks/missing_ngforof_let/BUILD.bazel

@@ -0,0 +1,17 @@
+load("//tools:defaults.bzl", "ts_library")
+
+ts_library(
+    name = "missing_ngforof_let",
+    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",
+    ],
+)

packages/compiler-cli/src/ngtsc/typecheck/extended/checks/missing_ngforof_let/index.ts

@@ -0,0 +1,53 @@
+/**
+ * @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, TmplAstNode, TmplAstTemplate} from '@angular/compiler';
+import ts from 'typescript';
+
+import {ErrorCode, ExtendedTemplateDiagnosticName} from '../../../../diagnostics';
+import {NgTemplateDiagnostic} from '../../../api';
+import {TemplateCheckFactory, TemplateCheckWithVisitor, TemplateContext} from '../../api';
+
+/**
+ * Ensures a user doesn't forget to omit `let` when using ngfor.
+ * Will return diagnostic information when `let` is missing.
+ */
+class MissingNgForOfLetCheck extends TemplateCheckWithVisitor<ErrorCode.MISSING_NGFOROF_LET> {
+  override code = ErrorCode.MISSING_NGFOROF_LET as const;
+
+  override visitNode(
+      ctx: TemplateContext<ErrorCode.MISSING_NGFOROF_LET>, component: ts.ClassDeclaration,
+      node: TmplAstNode|AST): NgTemplateDiagnostic<ErrorCode.MISSING_NGFOROF_LET>[] {
+    const isTemplate = node instanceof TmplAstTemplate;
+    if (!(node instanceof TmplAstTemplate)) {
+      return [];
+    }
+
+    if (node.templateAttrs.length === 0) {
+      return [];
+    }
+    const attr = node.templateAttrs.find(x => x.name === 'ngFor');
+    if (attr === undefined) {
+      return [];
+    }
+
+    if (node.variables.length > 0) {
+      return [];
+    }
+    const errorString = 'Your ngFor is missing a value. Did you forget to add the `let` keyword?';
+    const diagnostic = ctx.makeTemplateDiagnostic(attr.sourceSpan, errorString);
+    return [diagnostic];
+  }
+}
+
+export const factory: TemplateCheckFactory<
+    ErrorCode.MISSING_NGFOROF_LET, ExtendedTemplateDiagnosticName.MISSING_NGFOROF_LET> = {
+  code: ErrorCode.MISSING_NGFOROF_LET,
+  name: ExtendedTemplateDiagnosticName.MISSING_NGFOROF_LET,
+  create: () => new MissingNgForOfLetCheck(),
+};

packages/compiler-cli/src/ngtsc/typecheck/extended/index.ts

@@ -11,6 +11,7 @@ import {ErrorCode, ExtendedTemplateDiagnosticName} from '../../diagnostics';
 import {TemplateCheckFactory} from './api';
 import {factory as invalidBananaInBoxFactory} from './checks/invalid_banana_in_box';
 import {factory as missingControlFlowDirectiveFactory} from './checks/missing_control_flow_directive';
+import {factory as missingNgForOfLetFactory} from './checks/missing_ngforof_let';
 import {factory as nullishCoalescingNotNullableFactory} from './checks/nullish_coalescing_not_nullable';
 import {factory as textAttributeNotBindingFactory} from './checks/text_attribute_not_binding';
 
@@ -22,4 +23,5 @@ export const ALL_DIAGNOSTIC_FACTORIES:
       nullishCoalescingNotNullableFactory,
       missingControlFlowDirectiveFactory,
       textAttributeNotBindingFactory,
+      missingNgForOfLetFactory,
     ];

packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/missing_ngforof_let/BUILD.bazel

@@ -0,0 +1,27 @@
+load("//tools:defaults.bzl", "jasmine_node_test", "ts_library")
+
+ts_library(
+    name = "test_lib",
+    testonly = True,
+    srcs = ["missing_ngforof_let_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/missing_ngforof_let",
+        "//packages/compiler-cli/src/ngtsc/typecheck/testing",
+        "@npm//typescript",
+    ],
+)
+
+jasmine_node_test(
+    name = "test",
+    bootstrap = ["//tools/testing:node_no_angular_es2015"],
+    deps = [
+        ":test_lib",
+    ],
+)

packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/missing_ngforof_let/missing_ngforof_let_spec.ts

@@ -0,0 +1,83 @@
+/**
+ * @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 missingNgForOfLet} from '../../../checks/missing_ngforof_let/index';
+import {ExtendedTemplateCheckerImpl} from '../../../src/extended_template_checker';
+
+runInEachFileSystem(() => {
+  describe('TemplateChecks', () => {
+    it('binds the error code to its extended template diagnostic name', () => {
+      expect(missingNgForOfLet.code).toBe(ErrorCode.MISSING_NGFOROF_LET);
+      expect(missingNgForOfLet.name).toBe(ExtendedTemplateDiagnosticName.MISSING_NGFOROF_LET);
+    });
+
+    it('should produce missing ngforof let warning', () => {
+      const fileName = absoluteFrom('/main.ts');
+      const {program, templateTypeChecker} = setup([{
+        fileName,
+        templates: {
+          'TestCmp': '<ul><li *ngFor="thing of items">{{thing["name"]}}</li></ul>',
+        },
+        source:
+            'export class TestCmp { items: [] = [{\'name\': \'diana\'}, {\'name\': \'prince\'}] }'
+      }]);
+      const sf = getSourceFileOrError(program, fileName);
+      const component = getClass(sf, 'TestCmp');
+      const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
+          templateTypeChecker, program.getTypeChecker(), [missingNgForOfLet], {} /* options */);
+      const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
+      expect(diags.length).toBe(1);
+      expect(diags[0].category).toBe(ts.DiagnosticCategory.Warning);
+      expect(diags[0].code).toBe(ngErrorCode(ErrorCode.MISSING_NGFOROF_LET));
+      expect(getSourceCodeForDiagnostic(diags[0])).toBe('ngFor="thing ');
+    });
+
+    it('should not produce missing ngforof let warning if written correctly', () => {
+      const fileName = absoluteFrom('/main.ts');
+      const {program, templateTypeChecker} = setup([{
+        fileName,
+        templates: {
+          'TestCmp': '<ul><li *ngFor="let item of items">{{item["name"]}};</li></ul>',
+        },
+        source:
+            'export class TestCmp { items: [] = [{\'name\': \'diana\'}, {\'name\': \'prince\'}] }'
+      }]);
+      const sf = getSourceFileOrError(program, fileName);
+      const component = getClass(sf, 'TestCmp');
+      const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
+          templateTypeChecker, program.getTypeChecker(), [missingNgForOfLet], {} /* options */);
+      const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
+      expect(diags.length).toBe(0);
+    });
+
+    it('should not produce missing ngforof let warning if written correctly in longhand', () => {
+      const fileName = absoluteFrom('/main.ts');
+      const {program, templateTypeChecker} = setup([{
+        fileName,
+        templates: {
+          'TestCmp': '<ng-template ngFor let-item [ngForOf]="items">{{item["name"]}}</ng-template>',
+        },
+        source:
+            'export class TestCmp { items: [] = [{\'name\': \'diana\'}, {\'name\': \'prince\'}] }'
+      }]);
+      const sf = getSourceFileOrError(program, fileName);
+      const component = getClass(sf, 'TestCmp');
+      const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
+          templateTypeChecker, program.getTypeChecker(), [missingNgForOfLet], {} /* options */);
+      const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
+      expect(diags.length).toBe(0);
+    });
+  });
+});