fix(compiler-cli): detect missing control flow directive imports

This commit adds an extended diagnostics check that verifies that all control flow directives (such as `ngIf`, `ngFor`) have matching directives. Currently there is no diagnistics produced for such cases, which makes it harded for developers to detect and find a problem.
angular · May 26, 2022 · 5ac5ecf · 5ac5ecf
1 parent 8629f2d
commit 5ac5ecf
Show file tree

Hide file tree

Showing 10 changed files with 257 additions and 0 deletions.
diff --git a/goldens/public-api/compiler-cli/error_code.md b/goldens/public-api/compiler-cli/error_code.md
@@ -45,6 +45,7 @@ export enum ErrorCode {
     INLINE_TCB_REQUIRED = 8900,
     INLINE_TYPE_CTOR_REQUIRED = 8901,
     INVALID_BANANA_IN_BOX = 8101,
+    MISSING_CONTROL_FLOW_DIRECTIVE = 8103,
     MISSING_PIPE = 8004,
     MISSING_REFERENCE_TARGET = 8003,
     NGMODULE_BOOTSTRAP_IS_STANDALONE = 6009,

diff --git a/goldens/public-api/compiler-cli/extended_template_diagnostic_name.md b/goldens/public-api/compiler-cli/extended_template_diagnostic_name.md
@@ -9,6 +9,8 @@ export enum ExtendedTemplateDiagnosticName {
     // (undocumented)
     INVALID_BANANA_IN_BOX = "invalidBananaInBox",
     // (undocumented)
+    MISSING_CONTROL_FLOW_DIRECTIVE = "missingControlFlowDirective",
+    // (undocumented)
     NULLISH_COALESCING_NOT_NULLABLE = "nullishCoalescingNotNullable"
 }
 

diff --git a/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts b/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts
@@ -233,6 +233,12 @@ export enum ErrorCode {
    */
   NULLISH_COALESCING_NOT_NULLABLE = 8102,
 
+  /**
+   * A known control flow directive (e.g. `*ngIf`) is used in a template,
+   * but the `CommonModule` is not imported.
+   */
+  MISSING_CONTROL_FLOW_DIRECTIVE = 8103,
+
   /**
    * 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
@@ -18,4 +18,5 @@
 export enum ExtendedTemplateDiagnosticName {
   INVALID_BANANA_IN_BOX = 'invalidBananaInBox',
   NULLISH_COALESCING_NOT_NULLABLE = 'nullishCoalescingNotNullable',
+  MISSING_CONTROL_FLOW_DIRECTIVE = 'missingControlFlowDirective',
 }
diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/BUILD.bazel b/packages/compiler-cli/src/ngtsc/typecheck/extended/BUILD.bazel
@@ -13,6 +13,7 @@ ts_library(
         "//packages/compiler-cli/src/ngtsc/typecheck/api",
         "//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/nullish_coalescing_not_nullable",
         "@npm//typescript",
     ],

diff --git a/...mpiler-cli/src/ngtsc/typecheck/extended/checks/missing_control_flow_directive/BUILD.bazel b/...mpiler-cli/src/ngtsc/typecheck/extended/checks/missing_control_flow_directive/BUILD.bazel
@@ -0,0 +1,15 @@
+load("//tools:defaults.bzl", "ts_library")
+
+ts_library(
+    name = "missing_control_flow_directive",
+    srcs = ["index.ts"],
+    visibility = ["//packages/compiler-cli/src/ngtsc:__subpackages__"],
+    deps = [
+        "//packages/compiler",
+        "//packages/compiler-cli/src/ngtsc/core:api",
+        "//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/.../compiler-cli/src/ngtsc/typecheck/extended/checks/missing_control_flow_directive/index.ts b/.../compiler-cli/src/ngtsc/typecheck/extended/checks/missing_control_flow_directive/index.ts
@@ -0,0 +1,74 @@
+/**
+ * @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 {NgCompilerOptions} from '../../../../core/api';
+import {ErrorCode, ExtendedTemplateDiagnosticName} from '../../../../diagnostics';
+import {NgTemplateDiagnostic} from '../../../api';
+import {TemplateCheckFactory, TemplateCheckWithVisitor, TemplateContext} from '../../api';
+
+/**
+ * The list of known control flow directives present in the `CommonModule`.
+ *
+ * Note: there is no `ngSwitch` here since it's typically used as a regular
+ * binding (e.g. `[ngSwitch]`), however the `ngSwitchCase` and `ngSwitchDefault`
+ * are used as structural directives and a warning would be generated. Once the
+ * `CommonModule` is included, the `ngSwitch` would also be covered.
+ */
+const KNOWN_CONTROL_FLOW_DIRECTIVES = new Set(['ngIf', 'ngFor', 'ngSwitchCase', 'ngSwitchDefault']);
+
+/**
+ * Ensures that there are no known control flow directives (such as *ngIf and *ngFor)
+ * used in a template without importing a `CommonModule`. Returns diagnostics in case
+ * such a directive is detected. Note: this check only handles the cases when structural
+ * directive syntax is used (e.g. `*ngIf`). Regular binding syntax (e.g. `[ngIf]`) is
+ * handled separately in type checker and treated as a hard error instead of a warning.
+ */
+class MissingControlFlowDirectiveCheck extends
+    TemplateCheckWithVisitor<ErrorCode.MISSING_CONTROL_FLOW_DIRECTIVE> {
+  override code = ErrorCode.MISSING_CONTROL_FLOW_DIRECTIVE as const;
+
+  override visitNode(
+      ctx: TemplateContext<ErrorCode.MISSING_CONTROL_FLOW_DIRECTIVE>,
+      component: ts.ClassDeclaration,
+      node: TmplAstNode|AST): NgTemplateDiagnostic<ErrorCode.MISSING_CONTROL_FLOW_DIRECTIVE>[] {
+    if (!(node instanceof TmplAstTemplate)) return [];
+
+    const controlFlowAttr =
+        node.templateAttrs.find(attr => KNOWN_CONTROL_FLOW_DIRECTIVES.has(attr.name));
+    if (!controlFlowAttr) return [];
+
+    const symbol = ctx.templateTypeChecker.getSymbolOfNode(node, component);
+    if (symbol === null || symbol.directives.length > 0) {
+      return [];
+    }
+
+    const sourceSpan = controlFlowAttr.keySpan || controlFlowAttr.sourceSpan;
+    // TODO: get an access to the `standalone` flag and update the error message to
+    // be more precise (to either mention @NgModule.imports or @Component.imports).
+    const errorMessage = `The \`*${controlFlowAttr.name}\` directive was used in the template, ` +
+        `but the \`CommonModule\` was not imported. Please make sure that the \`CommonModule\` ` +
+        `is included into the \`@NgModule.imports\` array of an @NgModule that declares this ` +
+        `component or to the \`@Component.imports\` array of this component if it's marked as ` +
+        `standalone (if it has the \`standalone: true\` flag in the @Component decorator).`;
+    const diagnostic = ctx.makeTemplateDiagnostic(sourceSpan, errorMessage);
+    return [diagnostic];
+  }
+}
+
+export const factory: TemplateCheckFactory<
+    ErrorCode.MISSING_CONTROL_FLOW_DIRECTIVE,
+    ExtendedTemplateDiagnosticName.MISSING_CONTROL_FLOW_DIRECTIVE> = {
+  code: ErrorCode.MISSING_CONTROL_FLOW_DIRECTIVE,
+  name: ExtendedTemplateDiagnosticName.MISSING_CONTROL_FLOW_DIRECTIVE,
+  create: (options: NgCompilerOptions) => {
+    return new MissingControlFlowDirectiveCheck();
+  },
+};
diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/index.ts b/packages/compiler-cli/src/ngtsc/typecheck/extended/index.ts
@@ -10,6 +10,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 nullishCoalescingNotNullableFactory} from './checks/nullish_coalescing_not_nullable';
 
 export {ExtendedTemplateCheckerImpl} from './src/extended_template_checker';
@@ -18,4 +19,5 @@ export const ALL_DIAGNOSTIC_FACTORIES:
     readonly TemplateCheckFactory<ErrorCode, ExtendedTemplateDiagnosticName>[] = [
       invalidBananaInBoxFactory,
       nullishCoalescingNotNullableFactory,
+      missingControlFlowDirectiveFactory,
     ];
diff --git a/...r-cli/src/ngtsc/typecheck/extended/test/checks/missing_control_flow_directive/BUILD.bazel b/...r-cli/src/ngtsc/typecheck/extended/test/checks/missing_control_flow_directive/BUILD.bazel
@@ -0,0 +1,27 @@
+load("//tools:defaults.bzl", "jasmine_node_test", "ts_library")
+
+ts_library(
+    name = "test_lib",
+    testonly = True,
+    srcs = ["missing_control_flow_directive_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_control_flow_directive",
+        "//packages/compiler-cli/src/ngtsc/typecheck/testing",
+        "@npm//typescript",
+    ],
+)
+
+jasmine_node_test(
+    name = "test",
+    bootstrap = ["//tools/testing:node_no_angular_es2015"],
+    deps = [
+        ":test_lib",
+    ],
+)
diff --git a/...xtended/test/checks/missing_control_flow_directive/missing_control_flow_directive_spec.ts b/...xtended/test/checks/missing_control_flow_directive/missing_control_flow_directive_spec.ts
@@ -0,0 +1,128 @@
+/**
+ * @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 {DiagnosticCategoryLabel} from '@angular/compiler-cli/src/ngtsc/core/api';
+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 missingControlFlowDirectiveCheck} from '../../../checks/missing_control_flow_directive';
+import {ExtendedTemplateCheckerImpl} from '../../../src/extended_template_checker';
+
+const KNOWN_CONTROL_FLOW_DIRECTIVES = ['ngIf', 'ngFor', 'ngSwitchCase', 'ngSwitchDefault'];
+
+runInEachFileSystem.native(() => {
+  describe('MissingControlFlowDirectiveCheck', () => {
+    KNOWN_CONTROL_FLOW_DIRECTIVES.forEach(directive => {
+      ['div', 'ng-template', 'ng-container', 'ng-content'].forEach(element => {
+        it(`should produce a warning when the '${directive}' directive is not imported ` +
+               `(when used on the '${element}' element)`,
+           () => {
+             const fileName = absoluteFrom('/main.ts');
+             const {program, templateTypeChecker} = setup([{
+               fileName,
+               templates: {
+                 'TestCmp': `<${element} *${directive}="exp"></${element}>`,
+               },
+             }]);
+             const sf = getSourceFileOrError(program, fileName);
+             const component = getClass(sf, 'TestCmp');
+             const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
+                 templateTypeChecker, program.getTypeChecker(), [missingControlFlowDirectiveCheck],
+                 {} /* 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_CONTROL_FLOW_DIRECTIVE));
+             expect(getSourceCodeForDiagnostic(diags[0])).toBe(directive);
+           });
+
+        it(`should *not* produce a warning when the '${directive}' directive is imported ` +
+               `(when used on the '${element}' element)`,
+           () => {
+             const className = directive.charAt(0).toUpperCase() + directive.substr(1);
+             const fileName = absoluteFrom('/main.ts');
+             const {program, templateTypeChecker} = setup([{
+               fileName,
+               templates: {
+                 'TestCmp': `<${element} *${directive}="exp"></${element}>`,
+               },
+               source: `
+                export class TestCmp {}
+                export class ${className} {}
+              `,
+               declarations: [{
+                 type: 'directive',
+                 name: className,
+                 selector: `[${directive}]`,
+               }],
+             }]);
+             const sf = getSourceFileOrError(program, fileName);
+             const component = getClass(sf, 'TestCmp');
+             const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
+                 templateTypeChecker, program.getTypeChecker(), [missingControlFlowDirectiveCheck],
+                 {strictNullChecks: true} /* options */);
+             const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
+             // No diagnostic messages are expected.
+             expect(diags.length).toBe(0);
+           });
+
+        it(`should *not* produce a warning when the '${directive}' directive selector attribute ` +
+               `is present on the '${element}' element, but the directive is not imported`,
+           () => {
+             const className = directive.charAt(0).toUpperCase() + directive.substr(1);
+             const fileName = absoluteFrom('/main.ts');
+             const {program, templateTypeChecker} = setup([{
+               fileName,
+               templates: {
+                 'TestCmp': `<${element} *${directive}="exp"></${element}>`,
+               },
+               source: `
+                export class TestCmp {}
+                export class ${className} {}
+              `,
+               declarations: [{
+                 type: 'directive',
+                 name: className,
+                 selector: `[${directive}]`,
+               }],
+             }]);
+             const sf = getSourceFileOrError(program, fileName);
+             const component = getClass(sf, 'TestCmp');
+             const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
+                 templateTypeChecker, program.getTypeChecker(), [missingControlFlowDirectiveCheck],
+                 {strictNullChecks: true} /* options */);
+             const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
+             // No diagnostic messages are expected.
+             expect(diags.length).toBe(0);
+           });
+      });
+    });
+
+    it(`should *not* produce a warning for other missing structural directives`, () => {
+      const fileName = absoluteFrom('/main.ts');
+      const {program, templateTypeChecker} = setup([{
+        fileName,
+        templates: {
+          'TestCmp': `<div *foo="exp"></div>`,
+        },
+      }]);
+      const sf = getSourceFileOrError(program, fileName);
+      const component = getClass(sf, 'TestCmp');
+      const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
+          templateTypeChecker, program.getTypeChecker(), [missingControlFlowDirectiveCheck],
+          {strictNullChecks: true} /* options */);
+      const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
+      // No diagnostic messages are expected.
+      expect(diags.length).toBe(0);
+    });
+  });
+});