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

…gForOf

In the case that a user accidentally forgot the let keyword, they dont get a very clear indicator of there being a problem.
They get an issue in the template iteration at runtime. This diagnostic will warn the user when the let keyword is missing.

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

@@ -13,7 +13,9 @@ export enum ExtendedTemplateDiagnosticName {
     // (undocumented)
     NULLISH_COALESCING_NOT_NULLABLE = "nullishCoalescingNotNullable",
     // (undocumented)
-    TEXT_ATTRIBUTE_NOT_BINDING = "textAttributeNotBinding"
+    TEXT_ATTRIBUTE_NOT_BINDING = "textAttributeNotBinding",
+    // (undocumented)
+    MISSING_NGFOROF_LET = "missingNgForOfLet"
 }
 
 // (No @packageDocumentation comment for this package)

packages/common/test/directives/ng_for_spec.ts

@@ -95,6 +95,18 @@ let thisArg: any;
          detectChangesAndExpectText('shyam;');
        }));
 
+    it('should throw with useful error when forgetting let keyword', waitForAsync(() => {
+         const template = '<ul><li *ngFor="item of items">{{item["name"]}}</li></ul>';
+         fixture = createTestComponent(template);
+
+         // INIT
+         getComponent().items = [{'name': 'misko'}, {'name': 'shyam'}];
+         fixture.detectChanges();
+         expect(() => fixture.detectChanges())
+             .toThrowError(
+                 `NG02200: Cannot find a differ supporting object '\[object Object\]' of type 'object'. NgFor only supports binding to Iterables, such as Arrays. Did you mean to use the keyvalue pipe? Find more at https://angular.io/errors/NG02200`);
+       }));
+
     it('should gracefully handle nulls', waitForAsync(() => {
          const template = '<ul><li *ngFor="let item of null">{{item}};</li></ul>';
          fixture = createTestComponent(template);

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,43 @@
+/**
+ * @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 (!(isTemplate) || (isTemplate && !(node.templateAttrs.length))) return [];
+    const attr = node.templateAttrs.find(x => x.name === 'ngForOf');
+    if (attr == null) return [];
+
+    if (node.variables.length > 0) return [];
+    const errorString = 'Your ngForOf is missing a value. Did you forget to add the `let` keyword?';
+    const diagnostic = ctx.makeTemplateDiagnostic(node.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,66 @@
+/**
+ * @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('<li *ngFor="thing of items">{{thing["name"]}}</li>');
+    });
+
+    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);
+    });
+  });
+});