feat(compiler-cli): detect missing control flow directive imports in standalone components (#46146)

AndrewKushnir · thePunderWoman · commit 131d029da16b · 2022-06-10T21:22:53.000Z
This commit adds an extended diagnostics check that verifies that all control flow directives (such as `ngIf`, `ngFor`) have the necessary directives imported in standalone components. Currently there is no diagnostics produced for such cases, which makes it harder to detect and find problems. PR Close #46146
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/api/api.ts b/packages/compiler-cli/src/ngtsc/typecheck/api/api.ts
@@ -25,6 +25,7 @@ export interface TypeCheckableDirectiveMeta extends DirectiveMeta, DirectiveType
   queries: string[];
   inputs: ClassPropertyMapping;
   outputs: ClassPropertyMapping;
+  isStandalone: boolean;
 }
 
 export type TemplateId = string&{__brand: 'TemplateId'};
diff --git a/packages/compiler-cli/src/ngtsc/typecheck/api/scope.ts b/packages/compiler-cli/src/ngtsc/typecheck/api/scope.ts
@@ -7,6 +7,7 @@
  */
 
 import ts from 'typescript';
+
 import {ClassDeclaration} from '../../reflection';
 import {SymbolWithValueDeclaration} from '../../util/src/typescript';
 
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/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/missing_control_flow_directive/BUILD.bazel b/packages/compiler-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/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/missing_control_flow_directive/index.ts b/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/missing_control_flow_directive/index.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 {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 of a *standalone* component 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 run(
+      ctx: TemplateContext<ErrorCode.MISSING_CONTROL_FLOW_DIRECTIVE>,
+      component: ts.ClassDeclaration, template: TmplAstNode[]) {
+    const componentMetadata = ctx.templateTypeChecker.getDirectiveMetadata(component);
+    // Avoid running this check for non-standalone components.
+    if (!componentMetadata || !componentMetadata.isStandalone) {
+      return [];
+    }
+    return super.run(ctx, component, template);
+  }
+
+  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;
+    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 \`@Component.imports\` array of this component.`;
+    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/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/missing_control_flow_directive/BUILD.bazel b/packages/compiler-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/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/missing_control_flow_directive/missing_control_flow_directive_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/missing_control_flow_directive/missing_control_flow_directive_spec.ts
@@ -0,0 +1,143 @@
+/**
+ * @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, 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(() => {
+  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}>`,
+               },
+               declarations: [{
+                 name: 'TestCmp',
+                 type: 'directive',
+                 selector: `[test-cmp]`,
+                 isStandalone: true,
+               }]
+             }]);
+             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 not imported ` +
+               `into a non-standalone component scope (when used on the '${element}' element)`,
+           () => {
+             const fileName = absoluteFrom('/main.ts');
+             const {program, templateTypeChecker} = setup([{
+               fileName,
+               templates: {
+                 'TestCmp': `<${element} *${directive}="exp"></${element}>`,
+               },
+               declarations: [{
+                 name: 'TestCmp',
+                 type: 'directive',
+                 selector: `[test-cmp]`,
+                 isStandalone: false,
+               }]
+             }]);
+             const sf = getSourceFileOrError(program, fileName);
+             const component = getClass(sf, 'TestCmp');
+             const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
+                 templateTypeChecker, program.getTypeChecker(), [missingControlFlowDirectiveCheck],
+                 {} /* 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 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}]`,
+                 },
+                 {
+                   name: 'TestCmp',
+                   type: 'directive',
+                   selector: `[test-cmp]`,
+                   isStandalone: true,
+                 }
+               ],
+             }]);
+             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>`,
+        },
+        declarations: [{
+          name: 'TestCmp',
+          type: 'directive',
+          selector: `[test-cmp]`,
+          isStandalone: true,
+        }]
+      }]);
+      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);
+    });
+  });
+});
diff --git a/packages/compiler-cli/src/ngtsc/typecheck/testing/index.ts b/packages/compiler-cli/src/ngtsc/typecheck/testing/index.ts

Original file line number	Diff line number	Diff line change
`@@ -9,6 +9,8 @@ export enum ExtendedTemplateDiagnosticName {`
`9`	`9`	`// (undocumented)`
`10`	`10`	`INVALID_BANANA_IN_BOX = "invalidBananaInBox",`
`11`	`11`	`// (undocumented)`
	`12`	`+ MISSING_CONTROL_FLOW_DIRECTIVE = "missingControlFlowDirective",`
	`13`	`+ // (undocumented)`
`12`	`14`	`NULLISH_COALESCING_NOT_NULLABLE = "nullishCoalescingNotNullable"`
`13`	`15`	`}`
`14`	`16`
Original file line number	Diff line number	Diff line change
`@@ -18,4 +18,5 @@`
`18`	`18`	`export enum ExtendedTemplateDiagnosticName {`
`19`	`19`	`INVALID_BANANA_IN_BOX = 'invalidBananaInBox',`
`20`	`20`	`NULLISH_COALESCING_NOT_NULLABLE = 'nullishCoalescingNotNullable',`
	`21`	`+ MISSING_CONTROL_FLOW_DIRECTIVE = 'missingControlFlowDirective',`
`21`	`22`	`}`
Original file line number	Diff line number	Diff line change
`@@ -25,6 +25,7 @@ export interface TypeCheckableDirectiveMeta extends DirectiveMeta, DirectiveType`
`25`	`25`	`queries: string[];`
`26`	`26`	`inputs: ClassPropertyMapping;`
`27`	`27`	`outputs: ClassPropertyMapping;`
	`28`	`+ isStandalone: boolean;`
`28`	`29`	`}`
`29`	`30`
`30`	`31`	`export type TemplateId = string&{__brand: 'TemplateId'};`