From 732c1c4c1cfa89c0c064e2aa326dfda61d1a8285 Mon Sep 17 00:00:00 2001 From: Andrew Kushnir Date: Wed, 25 May 2022 16:58:13 -0700 Subject: [PATCH] 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. --- goldens/public-api/compiler-cli/error_code.md | 1 + .../extended_template_diagnostic_name.md | 2 + .../src/ngtsc/diagnostics/src/error_code.ts | 6 + .../src/extended_template_diagnostic_name.ts | 1 + .../src/ngtsc/typecheck/extended/BUILD.bazel | 1 + .../BUILD.bazel | 15 ++ .../missing_control_flow_directive/index.ts | 72 ++++++++++ .../src/ngtsc/typecheck/extended/index.ts | 2 + .../BUILD.bazel | 27 ++++ .../missing_control_flow_directive_spec.ts | 128 ++++++++++++++++++ 10 files changed, 255 insertions(+) create mode 100644 packages/compiler-cli/src/ngtsc/typecheck/extended/checks/missing_control_flow_directive/BUILD.bazel create mode 100644 packages/compiler-cli/src/ngtsc/typecheck/extended/checks/missing_control_flow_directive/index.ts create mode 100644 packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/missing_control_flow_directive/BUILD.bazel create mode 100644 packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/missing_control_flow_directive/missing_control_flow_directive_spec.ts diff --git a/goldens/public-api/compiler-cli/error_code.md b/goldens/public-api/compiler-cli/error_code.md index e8f2ee7635a60..46612ddeac3b3 100644 --- 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 index 0a99fb4dca154..14c2369348ea1 100644 --- 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 index cf6a07ef4d9aa..486b8c3dfc22f 100644 --- 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 index 57a6df6c4fa7b..4f58aeb142103 100644 --- 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 index 8323abb4b0306..1490aaaaf4877 100644 --- 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 new file mode 100644 index 0000000000000..d3471e56fafc1 --- /dev/null +++ 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 new file mode 100644 index 0000000000000..7b5dbbf7d3a97 --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/missing_control_flow_directive/index.ts @@ -0,0 +1,72 @@ +/** + * @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 { + override code = ErrorCode.MISSING_CONTROL_FLOW_DIRECTIVE as const; + + override visitNode( + ctx: TemplateContext, + component: ts.ClassDeclaration, + node: TmplAstNode|AST): NgTemplateDiagnostic[] { + 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 \`@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 index 0717e4634d6c7..9378767b36078 100644 --- 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[] = [ 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 new file mode 100644 index 0000000000000..776d44c49c31f --- /dev/null +++ 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 new file mode 100644 index 0000000000000..76f1884a468e5 --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/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">`, + }, + }]); + 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">`, + }, + 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">`, + }, + 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': `
`, + }, + }]); + 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); + }); + }); +});