From 1446017e4b1a2b8a902002d09a48aebccbe9ffe6 Mon Sep 17 00:00:00 2001 From: Doug Parker Date: Mon, 6 Dec 2021 18:23:21 -0800 Subject: [PATCH] refactor(compiler-cli): call factories directly from extended template checker (#44391) Refs #42966. This moves extended template check factory invocations into the checker itself, where it can provide a more consistent API contract. Factories are called with compiler options and may return a `TemplateCheck` or `undefined` if the current options do not support that check. This allows `nullishCoalescingNotNullable` to disable itself when `strictNullChecks` is disabled without throwing errors. This gives extended template diagnostics a stronger abstraction model to define their behavior with. PR Close #44391 --- .../src/ngtsc/core/src/compiler.ts | 11 +++---- .../src/ngtsc/typecheck/extended/BUILD.bazel | 1 + .../ngtsc/typecheck/extended/api/BUILD.bazel | 1 + .../src/ngtsc/typecheck/extended/api/api.ts | 3 +- .../BUILD.bazel | 1 + .../nullish_coalescing_not_nullable/index.ts | 10 +++++- .../extended/src/extended_template_checker.ts | 14 +++++--- .../invalid_banana_in_box_spec.ts | 12 ++++--- .../nullish_coalescing_not_nullable_spec.ts | 33 ++++++++++++------- .../extended_template_diagnostics_spec.ts | 20 +---------- 10 files changed, 58 insertions(+), 48 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/core/src/compiler.ts b/packages/compiler-cli/src/ngtsc/core/src/compiler.ts index f0b07691e38..1c893b282df 100644 --- a/packages/compiler-cli/src/ngtsc/core/src/compiler.ts +++ b/packages/compiler-cli/src/ngtsc/core/src/compiler.ts @@ -29,7 +29,7 @@ import {aliasTransformFactory, CompilationMode, declarationTransformFactory, Dec import {TemplateTypeCheckerImpl} from '../../typecheck'; import {OptimizeFor, TemplateTypeChecker, TypeCheckingConfig} from '../../typecheck/api'; import {ExtendedTemplateCheckerImpl} from '../../typecheck/extended'; -import {ExtendedTemplateChecker, TemplateCheck} from '../../typecheck/extended/api'; +import {ExtendedTemplateChecker} from '../../typecheck/extended/api'; import {factory as invalidBananaInBoxFactory} from '../../typecheck/extended/checks/invalid_banana_in_box'; import {factory as NullishCoalescingNotNullableFactory} from '../../typecheck/extended/checks/nullish_coalescing_not_nullable'; import {getSourceFileOrNull, isDtsPath, toUnredirectedSourceFile} from '../../util/src/typescript'; @@ -1067,12 +1067,9 @@ export class NgCompiler { reflector, this.adapter, this.incrementalCompilation, scopeRegistry, typeCheckScopeRegistry, this.delegatingPerfRecorder); - const templateChecks: TemplateCheck[] = [invalidBananaInBoxFactory.create()]; - if (this.options.strictNullChecks) { - templateChecks.push(NullishCoalescingNotNullableFactory.create()); - } - const extendedTemplateChecker = - new ExtendedTemplateCheckerImpl(templateTypeChecker, checker, templateChecks); + const diagnosticFactories = [invalidBananaInBoxFactory, NullishCoalescingNotNullableFactory]; + const extendedTemplateChecker = new ExtendedTemplateCheckerImpl( + templateTypeChecker, checker, diagnosticFactories, this.options); return { isCore, diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/BUILD.bazel b/packages/compiler-cli/src/ngtsc/typecheck/extended/BUILD.bazel index ca8d5b1c9ff..e84bab1fe0f 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/extended/BUILD.bazel +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/BUILD.bazel @@ -7,6 +7,7 @@ ts_library( ), visibility = ["//packages/compiler-cli/src/ngtsc:__subpackages__"], deps = [ + "//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", diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/api/BUILD.bazel b/packages/compiler-cli/src/ngtsc/typecheck/extended/api/BUILD.bazel index a2174ecf816..8b2ad8e1b5e 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/extended/api/BUILD.bazel +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/api/BUILD.bazel @@ -8,6 +8,7 @@ ts_library( 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", "@npm//typescript", diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/api/api.ts b/packages/compiler-cli/src/ngtsc/typecheck/extended/api/api.ts index d39d782f9a1..45f8b57e8e2 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/extended/api/api.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/api/api.ts @@ -9,6 +9,7 @@ import {AST, ASTWithSource, RecursiveAstVisitor, TmplAstBoundAttribute, TmplAstBoundEvent, TmplAstBoundText, TmplAstContent, TmplAstElement, TmplAstIcu, TmplAstNode, TmplAstRecursiveVisitor, TmplAstReference, TmplAstTemplate, TmplAstText, TmplAstTextAttribute, TmplAstVariable} from '@angular/compiler'; import ts from 'typescript'; +import {NgCompilerOptions} from '../../../core/api'; import {ErrorCode, ExtendedTemplateDiagnosticName} from '../../../diagnostics'; import {NgTemplateDiagnostic, TemplateTypeChecker} from '../../api'; @@ -49,7 +50,7 @@ export interface TemplateCheckFactory< > { code: Code; name: Name; - create(): TemplateCheck; + create(options: NgCompilerOptions): TemplateCheck|null; } /** diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/nullish_coalescing_not_nullable/BUILD.bazel b/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/nullish_coalescing_not_nullable/BUILD.bazel index b687a466cff..58d4c7597ce 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/nullish_coalescing_not_nullable/BUILD.bazel +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/nullish_coalescing_not_nullable/BUILD.bazel @@ -6,6 +6,7 @@ ts_library( 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", diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/nullish_coalescing_not_nullable/index.ts b/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/nullish_coalescing_not_nullable/index.ts index 357e654545d..fcf8a12144a 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/nullish_coalescing_not_nullable/index.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/nullish_coalescing_not_nullable/index.ts @@ -9,6 +9,7 @@ import {AST, Binary, TmplAstNode} from '@angular/compiler'; import ts from 'typescript'; +import {NgCompilerOptions} from '../../../../core/api'; import {ErrorCode, ExtendedTemplateDiagnosticName} from '../../../../diagnostics'; import {NgTemplateDiagnostic, SymbolKind} from '../../../api'; import {TemplateCheckFactory, TemplateCheckWithVisitor, TemplateContext} from '../../api'; @@ -55,5 +56,12 @@ export const factory: TemplateCheckFactory< ExtendedTemplateDiagnosticName.NULLISH_COALESCING_NOT_NULLABLE> = { code: ErrorCode.NULLISH_COALESCING_NOT_NULLABLE, name: ExtendedTemplateDiagnosticName.NULLISH_COALESCING_NOT_NULLABLE, - create: () => new NullishCoalescingNotNullableCheck(), + create: (options: NgCompilerOptions) => { + // Require `strictNullChecks` to be enabled. + if (options.strictNullChecks === false) { + return null; + } + + return new NullishCoalescingNotNullableCheck(); + }, }; diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/src/extended_template_checker.ts b/packages/compiler-cli/src/ngtsc/typecheck/extended/src/extended_template_checker.ts index fdc69f7f189..d0360ae3327 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/extended/src/extended_template_checker.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/src/extended_template_checker.ts @@ -8,18 +8,24 @@ import ts from 'typescript'; -import {ErrorCode} from '../../../diagnostics'; +import {NgCompilerOptions} from '../../../core/api'; +import {ErrorCode, ExtendedTemplateDiagnosticName} from '../../../diagnostics'; import {TemplateDiagnostic, TemplateTypeChecker} from '../../api'; -import {ExtendedTemplateChecker, TemplateCheck, TemplateContext} from '../api'; +import {ExtendedTemplateChecker, TemplateCheck, TemplateCheckFactory, TemplateContext} from '../api'; export class ExtendedTemplateCheckerImpl implements ExtendedTemplateChecker { - private ctx: TemplateContext; + private readonly ctx: TemplateContext; + private readonly templateChecks: TemplateCheck[]; constructor( templateTypeChecker: TemplateTypeChecker, typeChecker: ts.TypeChecker, - private readonly templateChecks: TemplateCheck[]) { + templateCheckFactories: + readonly TemplateCheckFactory[], + options: NgCompilerOptions) { this.ctx = {templateTypeChecker: templateTypeChecker, typeChecker: typeChecker} as TemplateContext; + this.templateChecks = templateCheckFactories.map((factory) => factory.create(options)) + .filter((check): check is TemplateCheck => check !== null); } getDiagnosticsForComponent(component: ts.ClassDeclaration): TemplateDiagnostic[] { diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/invalid_banana_in_box/invalid_banana_in_box_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/invalid_banana_in_box/invalid_banana_in_box_spec.ts index 0a663e213eb..bcab49d7727 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/invalid_banana_in_box/invalid_banana_in_box_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/invalid_banana_in_box/invalid_banana_in_box_spec.ts @@ -36,7 +36,8 @@ runInEachFileSystem(() => { const sf = getSourceFileOrError(program, fileName); const component = getClass(sf, 'TestCmp'); const extendedTemplateChecker = new ExtendedTemplateCheckerImpl( - templateTypeChecker, program.getTypeChecker(), [invalidBananaInBoxFactory.create()]); + templateTypeChecker, program.getTypeChecker(), [invalidBananaInBoxFactory], + {} /* options */); const diags = extendedTemplateChecker.getDiagnosticsForComponent(component); expect(diags.length).toBe(1); expect(diags[0].category).toBe(ts.DiagnosticCategory.Warning); @@ -56,7 +57,8 @@ runInEachFileSystem(() => { const sf = getSourceFileOrError(program, fileName); const component = getClass(sf, 'TestCmp'); const extendedTemplateChecker = new ExtendedTemplateCheckerImpl( - templateTypeChecker, program.getTypeChecker(), [invalidBananaInBoxFactory.create()]); + templateTypeChecker, program.getTypeChecker(), [invalidBananaInBoxFactory], + {} /* options */); const diags = extendedTemplateChecker.getDiagnosticsForComponent(component); expect(diags.length).toBe(0); }); @@ -74,7 +76,8 @@ runInEachFileSystem(() => { const sf = getSourceFileOrError(program, fileName); const component = getClass(sf, 'TestCmp'); const extendedTemplateChecker = new ExtendedTemplateCheckerImpl( - templateTypeChecker, program.getTypeChecker(), [invalidBananaInBoxFactory.create()]); + templateTypeChecker, program.getTypeChecker(), [invalidBananaInBoxFactory], + {} /* options */); const diags = extendedTemplateChecker.getDiagnosticsForComponent(component); expect(diags.length).toBe(0); }); @@ -96,7 +99,8 @@ runInEachFileSystem(() => { const sf = getSourceFileOrError(program, fileName); const component = getClass(sf, 'TestCmp'); const extendedTemplateChecker = new ExtendedTemplateCheckerImpl( - templateTypeChecker, program.getTypeChecker(), [invalidBananaInBoxFactory.create()]); + templateTypeChecker, program.getTypeChecker(), [invalidBananaInBoxFactory], + {} /* options */); const diags = extendedTemplateChecker.getDiagnosticsForComponent(component); expect(diags.length).toBe(2); expect(diags[0].category).toBe(ts.DiagnosticCategory.Warning); diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/nullish_coalescing_not_nullable/nullish_coalescing_not_nullable_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/nullish_coalescing_not_nullable/nullish_coalescing_not_nullable_spec.ts index f8fc0932294..88fed5ac007 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/nullish_coalescing_not_nullable/nullish_coalescing_not_nullable_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/nullish_coalescing_not_nullable/nullish_coalescing_not_nullable_spec.ts @@ -25,6 +25,15 @@ runInEachFileSystem(() => { .toBe(ExtendedTemplateDiagnosticName.NULLISH_COALESCING_NOT_NULLABLE); }); + it('should return a check if `strictNullChecks` is enabled', () => { + expect(nullishCoalescingNotNullableFactory.create({strictNullChecks: true})).toBeDefined(); + expect(nullishCoalescingNotNullableFactory.create({})).not.toBeNull(); // Defaults enabled. + }); + + it('should not return a check if `strictNullChecks` is disabled', () => { + expect(nullishCoalescingNotNullableFactory.create({strictNullChecks: false})).toBeNull(); + }); + it('should produce nullish coalescing warning', () => { const fileName = absoluteFrom('/main.ts'); const {program, templateTypeChecker} = setup([{ @@ -37,8 +46,8 @@ runInEachFileSystem(() => { const sf = getSourceFileOrError(program, fileName); const component = getClass(sf, 'TestCmp'); const extendedTemplateChecker = new ExtendedTemplateCheckerImpl( - templateTypeChecker, program.getTypeChecker(), - [nullishCoalescingNotNullableFactory.create()]); + templateTypeChecker, program.getTypeChecker(), [nullishCoalescingNotNullableFactory], + {} /* options */); const diags = extendedTemplateChecker.getDiagnosticsForComponent(component); expect(diags.length).toBe(1); expect(diags[0].category).toBe(ts.DiagnosticCategory.Warning); @@ -58,8 +67,8 @@ runInEachFileSystem(() => { const sf = getSourceFileOrError(program, fileName); const component = getClass(sf, 'TestCmp'); const extendedTemplateChecker = new ExtendedTemplateCheckerImpl( - templateTypeChecker, program.getTypeChecker(), - [nullishCoalescingNotNullableFactory.create()]); + templateTypeChecker, program.getTypeChecker(), [nullishCoalescingNotNullableFactory], + {} /* options */); const diags = extendedTemplateChecker.getDiagnosticsForComponent(component); expect(diags.length).toBe(0); }); @@ -76,8 +85,8 @@ runInEachFileSystem(() => { const sf = getSourceFileOrError(program, fileName); const component = getClass(sf, 'TestCmp'); const extendedTemplateChecker = new ExtendedTemplateCheckerImpl( - templateTypeChecker, program.getTypeChecker(), - [nullishCoalescingNotNullableFactory.create()]); + templateTypeChecker, program.getTypeChecker(), [nullishCoalescingNotNullableFactory], + {} /* options */); const diags = extendedTemplateChecker.getDiagnosticsForComponent(component); expect(diags.length).toBe(0); }); @@ -105,8 +114,8 @@ runInEachFileSystem(() => { const sf = getSourceFileOrError(program, fileName); const component = getClass(sf, 'TestCmp'); const extendedTemplateChecker = new ExtendedTemplateCheckerImpl( - templateTypeChecker, program.getTypeChecker(), - [nullishCoalescingNotNullableFactory.create()]); + templateTypeChecker, program.getTypeChecker(), [nullishCoalescingNotNullableFactory], + {} /* options */); const diags = extendedTemplateChecker.getDiagnosticsForComponent(component); expect(diags.length).toBe(1); expect(diags[0].category).toBe(ts.DiagnosticCategory.Warning); @@ -138,8 +147,8 @@ runInEachFileSystem(() => { const sf = getSourceFileOrError(program, fileName); const component = getClass(sf, 'TestCmp'); const extendedTemplateChecker = new ExtendedTemplateCheckerImpl( - templateTypeChecker, program.getTypeChecker(), - [nullishCoalescingNotNullableFactory.create()]); + templateTypeChecker, program.getTypeChecker(), [nullishCoalescingNotNullableFactory], + {} /* options */); const diags = extendedTemplateChecker.getDiagnosticsForComponent(component); expect(diags.length).toBe(0); }); @@ -163,8 +172,8 @@ runInEachFileSystem(() => { const sf = getSourceFileOrError(program, fileName); const component = getClass(sf, 'TestCmp'); const extendedTemplateChecker = new ExtendedTemplateCheckerImpl( - templateTypeChecker, program.getTypeChecker(), - [nullishCoalescingNotNullableFactory.create()]); + templateTypeChecker, program.getTypeChecker(), [nullishCoalescingNotNullableFactory], + {} /* options */); const diags = extendedTemplateChecker.getDiagnosticsForComponent(component); expect(diags.length).toBe(0); }); diff --git a/packages/compiler-cli/test/ngtsc/extended_template_diagnostics_spec.ts b/packages/compiler-cli/test/ngtsc/extended_template_diagnostics_spec.ts index a46e766a927..17511407c45 100644 --- a/packages/compiler-cli/test/ngtsc/extended_template_diagnostics_spec.ts +++ b/packages/compiler-cli/test/ngtsc/extended_template_diagnostics_spec.ts @@ -7,6 +7,7 @@ */ import ts from 'typescript'; + import {ErrorCode, ngErrorCode} from '../../src/ngtsc/diagnostics'; import {runInEachFileSystem} from '../../src/ngtsc/file_system/testing'; import {getSourceCodeForDiagnostic, loadStandardTestFiles} from '../../src/ngtsc/testing'; @@ -122,24 +123,5 @@ runInEachFileSystem(() => { expect(diags[0].code).toBe(ngErrorCode(ErrorCode.NULLISH_COALESCING_NOT_NULLABLE)); expect(getSourceCodeForDiagnostic(diags[0])).toBe('bar ?? "foo"'); }); - - it(`should not produce nullish coalescing not nullable warning with strictNullChecks disabled`, - () => { - env.tsconfig( - {_extendedTemplateDiagnostics: true, strictTemplates: true, strictNullChecks: false}); - env.write('test.ts', ` - import {Component} from '@angular/core'; - @Component({ - selector: 'test', - template: '{{ bar ?? "foo" }}', - }) - export class TestCmp { - bar: string = undefined; - } - `); - - const diags = env.driveDiagnostics(); - expect(diags.length).toBe(0); - }); }); });