From 40975e06c6805f13a87c8ca66d549adc6bf6b2ff Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Tue, 22 Sep 2020 16:18:09 -0400 Subject: [PATCH] fix(compiler-cli): perform DOM schema checks even in basic mode in g3 (#38943) In Ivy, template type-checking has 3 modes: basic, full, and strict. The primary difference between basic and full modes is that basic mode only checks the top-level template, whereas full mode descends into nested templates (embedded views like ngIfs and ngFors). Ivy applies this approach to all of its template type-checking, including the DOM schema checks which validate whether an element is a valid component/directive or not. View Engine has both the basic and the full mode, with the same distinction. However in View Engine, DOM schema checks happen for the full template even in the basic mode. Ivy's behavior here is technically a "fix" as it does not make sense for some checks to apply to the full template and others only to the top-level view. However, since g3 relies exclusively on the basic mode of checking and developers there are used to DOM checks applying throughout their template, this commit re-enables the nested schema checks even in basic mode only in g3. This is done by enabling the checks only when Closure Compiler annotations are requested. Outside of g3, it's recommended that applications use at least the full mode of checking (controlled by the `fullTemplateTypeCheck` flag), and ideally the strict mode (`strictTemplates`). PR Close #38943 --- .../src/ngtsc/core/src/compiler.ts | 4 ++ .../src/ngtsc/typecheck/api/api.ts | 6 +++ .../ngtsc/typecheck/src/type_check_block.ts | 29 +++++++++++++++ .../src/ngtsc/typecheck/test/test_utils.ts | 2 + .../typecheck/test/type_check_block_spec.ts | 1 + .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 37 +++++++++++++++++++ 6 files changed, 79 insertions(+) diff --git a/packages/compiler-cli/src/ngtsc/core/src/compiler.ts b/packages/compiler-cli/src/ngtsc/core/src/compiler.ts index c4a6637d91..48857f59b7 100644 --- a/packages/compiler-cli/src/ngtsc/core/src/compiler.ts +++ b/packages/compiler-cli/src/ngtsc/core/src/compiler.ts @@ -423,6 +423,7 @@ export class NgCompiler { applyTemplateContextGuards: strictTemplates, checkQueries: false, checkTemplateBodies: true, + alwaysCheckSchemaInTemplateBodies: true, checkTypeOfInputBindings: strictTemplates, honorAccessModifiersForInputBindings: false, strictNullInputBindings: strictTemplates, @@ -451,6 +452,9 @@ export class NgCompiler { applyTemplateContextGuards: false, checkQueries: false, checkTemplateBodies: false, + // Enable deep schema checking in "basic" template type-checking mode only if Closure + // compilation is requested, which is a good proxy for "only in google3". + alwaysCheckSchemaInTemplateBodies: this.closureCompilerEnabled, checkTypeOfInputBindings: false, strictNullInputBindings: false, honorAccessModifiersForInputBindings: false, diff --git a/packages/compiler-cli/src/ngtsc/typecheck/api/api.ts b/packages/compiler-cli/src/ngtsc/typecheck/api/api.ts index e9b3a609d1..2ff9d963df 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/api/api.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/api/api.ts @@ -230,6 +230,12 @@ export interface TypeCheckingConfig { */ checkTemplateBodies: boolean; + /** + * Whether to always apply DOM schema checks in template bodies, independently of the + * `checkTemplateBodies` setting. + */ + alwaysCheckSchemaInTemplateBodies: boolean; + /** * Whether to check resolvable queries. * diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts index dbe40a0925..1bc4af9798 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts @@ -1298,6 +1298,8 @@ class Scope { this.templateCtxOpMap.set(node, ctxIndex); if (this.tcb.env.config.checkTemplateBodies) { this.opQueue.push(new TcbTemplateBodyOp(this.tcb, this, node)); + } else if (this.tcb.env.config.alwaysCheckSchemaInTemplateBodies) { + this.appendDeepSchemaChecks(node.children); } this.checkAndAppendReferencesOfNode(node); } else if (node instanceof TmplAstBoundText) { @@ -1401,6 +1403,33 @@ class Scope { this.opQueue.push(new TcbUnclaimedOutputsOp(this.tcb, this, node, claimedOutputs)); } } + + private appendDeepSchemaChecks(nodes: TmplAstNode[]): void { + for (const node of nodes) { + if (!(node instanceof TmplAstElement || node instanceof TmplAstTemplate)) { + continue; + } + + if (node instanceof TmplAstElement) { + const claimedInputs = new Set(); + const directives = this.tcb.boundTarget.getDirectivesOfNode(node); + let hasDirectives: boolean; + if (directives === null || directives.length === 0) { + hasDirectives = false; + } else { + hasDirectives = true; + for (const dir of directives) { + for (const propertyName of dir.inputs.propertyNames) { + claimedInputs.add(propertyName); + } + } + } + this.opQueue.push(new TcbDomSchemaCheckerOp(this.tcb, node, !hasDirectives, claimedInputs)); + } + + this.appendDeepSchemaChecks(node.children); + } + } } interface TcbBoundInput { diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts index 253e5b8193..b756e306c9 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts @@ -167,6 +167,7 @@ export const ALL_ENABLED_CONFIG: TypeCheckingConfig = { applyTemplateContextGuards: true, checkQueries: false, checkTemplateBodies: true, + alwaysCheckSchemaInTemplateBodies: true, checkTypeOfInputBindings: true, honorAccessModifiersForInputBindings: true, strictNullInputBindings: true, @@ -238,6 +239,7 @@ export function tcb( checkTypeOfNonDomReferences: true, checkTypeOfPipes: true, checkTemplateBodies: true, + alwaysCheckSchemaInTemplateBodies: true, strictSafeNavigationTypes: true, useContextGenericType: true, strictLiteralTypes: true, diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts index 023503fb74..886a78f958 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts @@ -700,6 +700,7 @@ describe('type check blocks', () => { applyTemplateContextGuards: true, checkQueries: false, checkTemplateBodies: true, + alwaysCheckSchemaInTemplateBodies: true, checkTypeOfInputBindings: true, honorAccessModifiersForInputBindings: false, strictNullInputBindings: true, diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index 3fc856389f..193373668b 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -390,6 +390,43 @@ runInEachFileSystem(os => { expect(jsContents).toContain('/** @nocollapse */ TestCmp.ɵcmp'); }); + it('should still perform schema checks in embedded views', () => { + env.tsconfig({ + 'fullTemplateTypeCheck': false, + 'annotateForClosureCompiler': true, + 'ivyTemplateTypeCheck': true, + }); + env.write('test.ts', ` + import {Component, Directive, NgModule} from '@angular/core'; + + @Component({ + selector: 'test-cmp', + template: \` + + Has a directive, should be okay + Should trigger a schema error + + \` + }) + export class TestCmp {} + + @Directive({ + selector: 'some-dir', + }) + export class TestDir {} + + @NgModule({ + declarations: [TestCmp, TestDir], + }) + export class TestModule {} + `); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(1); + expect(diags[0].code).toBe(ngErrorCode(ErrorCode.SCHEMA_INVALID_ELEMENT)); + expect(ts.flattenDiagnosticMessageText(diags[0].messageText, '\n')) + .toContain('not-a-cmp'); + }); /** * The following set of tests verify that after Tsickle run we do not have cases * which trigger automatic semicolon insertion, which breaks the code. In order