From 69b00a097ab422b5f78f6901ea61eccd13f56864 Mon Sep 17 00:00:00 2001 From: Doug Parker Date: Fri, 3 Dec 2021 19:17:30 -0800 Subject: [PATCH] refactor(compiler-cli): add validation to extended template diagnostics configuration (#44391) Refs #42966. This validates the `tsconfig.json` options for extended template diagnostics. It verifies: * `strictTemplates` must be enabled if `extendedDiagnostics` have any explicit configuration. * `extendedDiagnostics.defaultCategory` must be a valid `DiagnosticCategoryLabel`. * `extendedDiagnostics.checks` keys must all be real diagnostics. * `extendedDiagnostics.checks` values must all be valid `DiagnosticCategoryLabel`s. These include new error codes, each of which prints out the exact property that was the issue and what the available options are to fix them. It does disallow the config: ```json { "angularCompilerOptions": { "strictTemplates": false, "extendedDiagnostics": { "defaultCategory": "suppress" } } } ``` Such a configuration is technically valid and could be executed, but will be rejected by this verification logic. There isn't much reason to ever do this, since users could just drop `extendedDiagnostics` altogether and get the intended effect. This is unlikely to be a significant issue for users, so it is considered invalid for now to keep the implementation simple. PR Close #44391 --- goldens/public-api/compiler-cli/error_code.md | 6 + .../src/ngtsc/core/src/compiler.ts | 107 ++++++++++++--- .../src/ngtsc/diagnostics/src/error_code.ts | 3 + .../checks/invalid_banana_in_box/BUILD.bazel | 5 +- packages/compiler-cli/test/ngtsc/BUILD.bazel | 2 + .../test/ngtsc/template_typecheck_spec.ts | 124 +++++++++++++++++- 6 files changed, 228 insertions(+), 19 deletions(-) diff --git a/goldens/public-api/compiler-cli/error_code.md b/goldens/public-api/compiler-cli/error_code.md index 0fba1f0e373..bb5dbbd8845 100644 --- a/goldens/public-api/compiler-cli/error_code.md +++ b/goldens/public-api/compiler-cli/error_code.md @@ -11,6 +11,12 @@ export enum ErrorCode { COMPONENT_MISSING_TEMPLATE = 2001, COMPONENT_RESOURCE_NOT_FOUND = 2008, // (undocumented) + CONFIG_EXTENDED_DIAGNOSTICS_IMPLIES_STRICT_TEMPLATES = 4003, + // (undocumented) + CONFIG_EXTENDED_DIAGNOSTICS_UNKNOWN_CATEGORY_LABEL = 4004, + // (undocumented) + CONFIG_EXTENDED_DIAGNOSTICS_UNKNOWN_CHECK = 4005, + // (undocumented) CONFIG_FLAT_MODULE_NO_INDEX = 4001, // (undocumented) CONFIG_STRICT_TEMPLATES_IMPLIES_FULL_TEMPLATE_TYPECHECK = 4002, diff --git a/packages/compiler-cli/src/ngtsc/core/src/compiler.ts b/packages/compiler-cli/src/ngtsc/core/src/compiler.ts index 9ffb0e98434..32eebae4cdb 100644 --- a/packages/compiler-cli/src/ngtsc/core/src/compiler.ts +++ b/packages/compiler-cli/src/ngtsc/core/src/compiler.ts @@ -32,7 +32,7 @@ import {ALL_DIAGNOSTIC_FACTORIES, ExtendedTemplateCheckerImpl} from '../../typec import {ExtendedTemplateChecker} from '../../typecheck/extended/api'; import {getSourceFileOrNull, isDtsPath, toUnredirectedSourceFile} from '../../util/src/typescript'; import {Xi18nContext} from '../../xi18n'; -import {NgCompilerAdapter, NgCompilerOptions} from '../api'; +import {DiagnosticCategoryLabel, NgCompilerAdapter, NgCompilerOptions} from '../api'; /** * State information about a compilation which is only generated once some data is requested from @@ -322,11 +322,8 @@ export class NgCompiler { 'The \'_extendedTemplateDiagnostics\' option requires \'strictTemplates\' to also be enabled.'); } - this.constructionDiagnostics.push(...this.adapter.constructionDiagnostics); - const incompatibleTypeCheckOptionsDiagnostic = verifyCompatibleTypeCheckOptions(this.options); - if (incompatibleTypeCheckOptionsDiagnostic !== null) { - this.constructionDiagnostics.push(incompatibleTypeCheckOptionsDiagnostic); - } + this.constructionDiagnostics.push( + ...this.adapter.constructionDiagnostics, ...verifyCompatibleTypeCheckOptions(this.options)); this.currentProgram = inputProgram; this.closureCompilerEnabled = !!this.options.annotateForClosureCompiler; @@ -1135,16 +1132,15 @@ function getR3SymbolsFile(program: ts.Program): ts.SourceFile|null { * "fullTemplateTypeCheck", it is required that the latter is not explicitly disabled if the * former is enabled. */ -function verifyCompatibleTypeCheckOptions(options: NgCompilerOptions): ts.Diagnostic|null { +function* + verifyCompatibleTypeCheckOptions(options: NgCompilerOptions): + Generator { if (options.fullTemplateTypeCheck === false && options.strictTemplates === true) { - return { + yield makeConfigDiagnostic({ category: ts.DiagnosticCategory.Error, - code: ngErrorCode(ErrorCode.CONFIG_STRICT_TEMPLATES_IMPLIES_FULL_TEMPLATE_TYPECHECK), - file: undefined, - start: undefined, - length: undefined, - messageText: - `Angular compiler option "strictTemplates" is enabled, however "fullTemplateTypeCheck" is disabled. + code: ErrorCode.CONFIG_STRICT_TEMPLATES_IMPLIES_FULL_TEMPLATE_TYPECHECK, + messageText: ` +Angular compiler option "strictTemplates" is enabled, however "fullTemplateTypeCheck" is disabled. Having the "strictTemplates" flag enabled implies that "fullTemplateTypeCheck" is also enabled, so the latter can not be explicitly disabled. @@ -1154,11 +1150,88 @@ One of the following actions is required: 2. Remove "strictTemplates" or set it to 'false'. More information about the template type checking compiler options can be found in the documentation: -https://angular.io/guide/template-typecheck`, - }; +https://angular.io/guide/template-typecheck + `.trim(), + }); } - return null; + if (options.extendedDiagnostics && options.strictTemplates === false) { + yield makeConfigDiagnostic({ + category: ts.DiagnosticCategory.Error, + code: ErrorCode.CONFIG_EXTENDED_DIAGNOSTICS_IMPLIES_STRICT_TEMPLATES, + messageText: ` +Angular compiler option "extendedDiagnostics" is configured, however "strictTemplates" is disabled. + +Using "extendedDiagnostics" requires that "strictTemplates" is also enabled. + +One of the following actions is required: +1. Remove "strictTemplates: false" to enable it. +2. Remove "extendedDiagnostics" configuration to disable them. + `.trim(), + }); + } + + const allowedCategoryLabels = Array.from(Object.values(DiagnosticCategoryLabel)) as string[]; + const defaultCategory = options.extendedDiagnostics?.defaultCategory; + if (defaultCategory && !allowedCategoryLabels.includes(defaultCategory)) { + yield makeConfigDiagnostic({ + category: ts.DiagnosticCategory.Error, + code: ErrorCode.CONFIG_EXTENDED_DIAGNOSTICS_UNKNOWN_CATEGORY_LABEL, + messageText: ` +Angular compiler option "extendedDiagnostics.defaultCategory" has an unknown diagnostic category: "${ + defaultCategory}". + +Allowed diagnostic categories are: +${allowedCategoryLabels.join('\n')} + `.trim(), + }); + } + + const allExtendedDiagnosticNames = + ALL_DIAGNOSTIC_FACTORIES.map((factory) => factory.name) as string[]; + for (const [checkName, category] of Object.entries(options.extendedDiagnostics?.checks ?? {})) { + if (!allExtendedDiagnosticNames.includes(checkName)) { + yield makeConfigDiagnostic({ + category: ts.DiagnosticCategory.Error, + code: ErrorCode.CONFIG_EXTENDED_DIAGNOSTICS_UNKNOWN_CHECK, + messageText: ` +Angular compiler option "extendedDiagnostics.checks" has an unknown check: "${checkName}". + +Allowed check names are: +${allExtendedDiagnosticNames.join('\n')} + `.trim(), + }); + } + + if (!allowedCategoryLabels.includes(category)) { + yield makeConfigDiagnostic({ + category: ts.DiagnosticCategory.Error, + code: ErrorCode.CONFIG_EXTENDED_DIAGNOSTICS_UNKNOWN_CATEGORY_LABEL, + messageText: ` +Angular compiler option "extendedDiagnostics.checks['${ + checkName}']" has an unknown diagnostic category: "${category}". + +Allowed diagnostic categories are: +${allowedCategoryLabels.join('\n')} + `.trim(), + }); + } + } +} + +function makeConfigDiagnostic({category, code, messageText}: { + category: ts.DiagnosticCategory, + code: ErrorCode, + messageText: string, +}): ts.Diagnostic { + return { + category, + code: ngErrorCode(code), + file: undefined, + start: undefined, + length: undefined, + messageText, + }; } class ReferenceGraphAdapter implements ReferencesRegistry { 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 b41686eee3a..ee4b4c044d3 100644 --- a/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts +++ b/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts @@ -71,6 +71,9 @@ export enum ErrorCode { CONFIG_FLAT_MODULE_NO_INDEX = 4001, CONFIG_STRICT_TEMPLATES_IMPLIES_FULL_TEMPLATE_TYPECHECK = 4002, + CONFIG_EXTENDED_DIAGNOSTICS_IMPLIES_STRICT_TEMPLATES = 4003, + CONFIG_EXTENDED_DIAGNOSTICS_UNKNOWN_CATEGORY_LABEL = 4004, + CONFIG_EXTENDED_DIAGNOSTICS_UNKNOWN_CHECK = 4005, /** * Raised when a host expression has a parse error, such as a host listener or host binding diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/invalid_banana_in_box/BUILD.bazel b/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/invalid_banana_in_box/BUILD.bazel index 4a9b4ddecad..a8d9fe22cf8 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/invalid_banana_in_box/BUILD.bazel +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/invalid_banana_in_box/BUILD.bazel @@ -3,7 +3,10 @@ load("//tools:defaults.bzl", "ts_library") ts_library( name = "invalid_banana_in_box", srcs = ["index.ts"], - visibility = ["//packages/compiler-cli/src/ngtsc:__subpackages__"], + visibility = [ + "//packages/compiler-cli/src/ngtsc:__subpackages__", + "//packages/compiler-cli/test/ngtsc:__pkg__", + ], deps = [ "//packages/compiler", "//packages/compiler-cli/src/ngtsc/diagnostics", diff --git a/packages/compiler-cli/test/ngtsc/BUILD.bazel b/packages/compiler-cli/test/ngtsc/BUILD.bazel index 04a1bbe859d..b27709f9aed 100644 --- a/packages/compiler-cli/test/ngtsc/BUILD.bazel +++ b/packages/compiler-cli/test/ngtsc/BUILD.bazel @@ -7,12 +7,14 @@ ts_library( deps = [ "//packages/compiler", "//packages/compiler-cli", + "//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/indexer", "//packages/compiler-cli/src/ngtsc/reflection", "//packages/compiler-cli/src/ngtsc/testing", + "//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/invalid_banana_in_box", "//packages/compiler-cli/src/ngtsc/util", "//packages/compiler-cli/test:test_utils", "@npm//source-map", diff --git a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts index eebcdacb53c..aa707b4ac5a 100644 --- a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts +++ b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts @@ -8,10 +8,12 @@ import ts from 'typescript'; +import {DiagnosticCategoryLabel} from '../../src/ngtsc/core/api'; import {ErrorCode, ngErrorCode} from '../../src/ngtsc/diagnostics'; -import {absoluteFrom as _, getFileSystem, getSourceFileOrError} from '../../src/ngtsc/file_system'; +import {absoluteFrom as _, getSourceFileOrError} from '../../src/ngtsc/file_system'; import {runInEachFileSystem} from '../../src/ngtsc/file_system/testing'; import {expectCompleteReuse, getSourceCodeForDiagnostic, loadStandardTestFiles} from '../../src/ngtsc/testing'; +import {factory as invalidBananaInBoxFactory} from '../../src/ngtsc/typecheck/extended/checks/invalid_banana_in_box'; import {NgtscTestEnvironment} from './env'; @@ -2454,6 +2456,126 @@ export declare class AnimationEvent { const diags = env.driveDiagnostics(); expect(diags.length).toBe(0); }); + + it('should error if "strictTemplates" is false when "extendedDiagnostics" is configured', () => { + env.tsconfig({strictTemplates: false, extendedDiagnostics: {}}); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(1); + expect(diags[0].messageText) + .toContain( + 'Angular compiler option "extendedDiagnostics" is configured, however "strictTemplates" is disabled.'); + }); + it('should not error if "strictTemplates" is true when "extendedDiagnostics" is configured', + () => { + env.tsconfig({strictTemplates: true, extendedDiagnostics: {}}); + + const diags = env.driveDiagnostics(); + expect(diags).toEqual([]); + }); + it('should not error if "strictTemplates" is false when "extendedDiagnostics" is not configured', + () => { + env.tsconfig({strictTemplates: false}); + + const diags = env.driveDiagnostics(); + expect(diags).toEqual([]); + }); + + it('should error if "extendedDiagnostics.defaultCategory" is set to an unknown value', () => { + env.tsconfig({ + extendedDiagnostics: { + defaultCategory: 'does-not-exist', + }, + }); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(1); + expect(diags[0].messageText) + .toContain( + 'Angular compiler option "extendedDiagnostics.defaultCategory" has an unknown diagnostic category: "does-not-exist".'); + expect(diags[0].messageText).toContain(` +Allowed diagnostic categories are: +warning +error +suppress + `.trim()); + }); + it('should not error if "extendedDiagnostics.defaultCategory" is set to a known value', + () => { + env.tsconfig({ + extendedDiagnostics: { + defaultCategory: DiagnosticCategoryLabel.Error, + }, + }); + + const diags = env.driveDiagnostics(); + expect(diags).toEqual([]); + }); + + it('should error if "extendedDiagnostics.checks" contains an unknown check', () => { + env.tsconfig({ + extendedDiagnostics: { + checks: { + doesNotExist: DiagnosticCategoryLabel.Error, + }, + }, + }); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(1); + expect(diags[0].messageText) + .toContain( + 'Angular compiler option "extendedDiagnostics.checks" has an unknown check: "doesNotExist".'); + }); + it('should not error if "extendedDiagnostics.checks" contains all known checks', () => { + env.tsconfig({ + extendedDiagnostics: { + checks: { + [invalidBananaInBoxFactory.name]: DiagnosticCategoryLabel.Error, + }, + }, + }); + + const diags = env.driveDiagnostics(); + expect(diags).toEqual([]); + }); + + it('should error if "extendedDiagnostics.checks" contains an unknown diagnostic category', + () => { + env.tsconfig({ + extendedDiagnostics: { + checks: { + [invalidBananaInBoxFactory.name]: 'does-not-exist', + }, + }, + }); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(1); + expect(diags[0].messageText) + .toContain(`Angular compiler option "extendedDiagnostics.checks['${ + invalidBananaInBoxFactory + .name}']" has an unknown diagnostic category: "does-not-exist".`); + expect(diags[0].messageText).toContain(` +Allowed diagnostic categories are: +warning +error +suppress + `.trim()); + }); + it('should not error if "extendedDiagnostics.checks" contains all known diagnostic categories', + () => { + env.tsconfig({ + extendedDiagnostics: { + checks: { + [invalidBananaInBoxFactory.name]: DiagnosticCategoryLabel.Error, + }, + }, + }); + + const diags = env.driveDiagnostics(); + expect(diags).toEqual([]); + }); }); describe('stability', () => {