From 9db59d010d07172ad093a76c990541c78db12e42 Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Mon, 28 Oct 2019 15:27:55 -0700 Subject: [PATCH] fix(ivy): don't crash on an unknown localref target (#33454) Previously the template binder would crash when encountering an unknown localref (# reference) such as `
` when no directive has `exportAs: "foo"`. With this commit, the compiler instead generates a template diagnostic error informing the user about the invalid reference. PR Close #33454 --- .../src/ngtsc/diagnostics/src/code.ts | 5 ++ .../src/ngtsc/typecheck/src/context.ts | 15 +++-- .../src/ngtsc/typecheck/src/oob.ts | 55 +++++++++++++++++++ .../ngtsc/typecheck/src/type_check_block.ts | 41 +++++++++++--- .../ngtsc/typecheck/src/type_check_file.ts | 6 +- .../src/ngtsc/typecheck/test/test_utils.ts | 10 +++- .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 18 ++++++ .../test/ngtsc/template_typecheck_spec.ts | 21 +++++++ .../compiler/src/render3/view/t2_binder.ts | 10 ++-- 9 files changed, 161 insertions(+), 20 deletions(-) create mode 100644 packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts diff --git a/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts b/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts index cbad3b5f84b8d..67f44e6e32e75 100644 --- a/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts +++ b/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts @@ -84,6 +84,11 @@ export enum ErrorCode { * An element's attribute name failed validation against the DOM schema. */ SCHEMA_INVALID_ATTRIBUTE = 8002, + + /** + * No matching directive was found for a `#ref="target"` expression. + */ + MISSING_REFERENCE_TARGET = 8003, } export function ngErrorCode(code: ErrorCode): number { diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts index d7c281983a410..92aa179984c18 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts @@ -19,6 +19,7 @@ import {shouldReportDiagnostic, translateDiagnostic} from './diagnostics'; import {DomSchemaChecker, RegistryDomSchemaChecker} from './dom'; import {Environment} from './environment'; import {TypeCheckProgramHost} from './host'; +import {OutOfBandDiagnosticRecorder, OutOfBandDiagnosticRecorderImpl} from './oob'; import {TcbSourceManager} from './source'; import {generateTypeCheckBlock, requiresInlineTypeCheckBlock} from './type_check_block'; import {TypeCheckFile} from './type_check_file'; @@ -58,6 +59,8 @@ export class TypeCheckContext { private domSchemaChecker = new RegistryDomSchemaChecker(this.sourceManager); + private oobRecorder = new OutOfBandDiagnosticRecorderImpl(this.sourceManager); + /** * Record a template for the given component `node`, with a `SelectorMatcher` for directive * matching. @@ -102,7 +105,8 @@ export class TypeCheckContext { this.addInlineTypeCheckBlock(ref, tcbMetadata); } else { // The class can be type-checked externally as normal. - this.typeCheckFile.addTypeCheckBlock(ref, tcbMetadata, this.domSchemaChecker); + this.typeCheckFile.addTypeCheckBlock( + ref, tcbMetadata, this.domSchemaChecker, this.oobRecorder); } } @@ -219,6 +223,7 @@ export class TypeCheckContext { } diagnostics.push(...this.domSchemaChecker.diagnostics); + diagnostics.push(...this.oobRecorder.diagnostics); return { diagnostics, @@ -234,7 +239,7 @@ export class TypeCheckContext { this.opMap.set(sf, []); } const ops = this.opMap.get(sf) !; - ops.push(new TcbOp(ref, tcbMeta, this.config, this.domSchemaChecker)); + ops.push(new TcbOp(ref, tcbMeta, this.config, this.domSchemaChecker, this.oobRecorder)); } } @@ -266,7 +271,8 @@ class TcbOp implements Op { constructor( readonly ref: Reference>, readonly meta: TypeCheckBlockMetadata, readonly config: TypeCheckingConfig, - readonly domSchemaChecker: DomSchemaChecker) {} + readonly domSchemaChecker: DomSchemaChecker, + readonly oobRecorder: OutOfBandDiagnosticRecorder) {} /** * Type check blocks are inserted immediately after the end of the component class. @@ -277,7 +283,8 @@ class TcbOp implements Op { string { const env = new Environment(this.config, im, refEmitter, sf); const fnName = ts.createIdentifier(`_tcb_${this.ref.node.pos}`); - const fn = generateTypeCheckBlock(env, this.ref, fnName, this.meta, this.domSchemaChecker); + const fn = generateTypeCheckBlock( + env, this.ref, fnName, this.meta, this.domSchemaChecker, this.oobRecorder); return printer.printNode(ts.EmitHint.Unspecified, fn, sf); } } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts new file mode 100644 index 0000000000000..17fc520a857a3 --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts @@ -0,0 +1,55 @@ +/** + * @license + * Copyright Google Inc. 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 {TmplAstReference} from '@angular/compiler'; +import * as ts from 'typescript'; + +import {ErrorCode, ngErrorCode} from '../../diagnostics'; + +import {TcbSourceResolver, makeTemplateDiagnostic} from './diagnostics'; + +/** + * Collects `ts.Diagnostic`s on problems which occur in the template which aren't directly sourced + * from Type Check Blocks. + * + * During the creation of a Type Check Block, the template is traversed and the + * `OutOfBandDiagnosticRecorder` is called to record cases when a correct interpretation for the + * template cannot be found. These operations create `ts.Diagnostic`s which are stored by the + * recorder for later display. + */ +export interface OutOfBandDiagnosticRecorder { + readonly diagnostics: ReadonlyArray; + + /** + * Reports a `#ref="target"` expression in the template for which a target directive could not be + * found. + * + * @param templateId the template type-checking ID of the template which contains the broken + * reference. + * @param ref the `TmplAstReference` which could not be matched to a directive. + */ + missingReferenceTarget(templateId: string, ref: TmplAstReference): void; +} + +export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecorder { + private _diagnostics: ts.Diagnostic[] = []; + + constructor(private resolver: TcbSourceResolver) {} + + get diagnostics(): ReadonlyArray { return this._diagnostics; } + + missingReferenceTarget(templateId: string, ref: TmplAstReference): void { + const mapping = this.resolver.getSourceMapping(templateId); + const value = ref.value.trim(); + + const errorMsg = `No directive found with exportAs '${value}'.`; + this._diagnostics.push(makeTemplateDiagnostic( + mapping, ref.valueSpan || ref.sourceSpan, ts.DiagnosticCategory.Error, + ngErrorCode(ErrorCode.MISSING_REFERENCE_TARGET), errorMsg)); + } +} 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 384dfa1d8ff47..92774e7fb747f 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 @@ -17,6 +17,7 @@ import {addParseSpanInfo, addSourceId, toAbsoluteSpan, wrapForDiagnostics} from import {DomSchemaChecker} from './dom'; import {Environment} from './environment'; import {NULL_AS_ANY, astToTypescript} from './expression'; +import {OutOfBandDiagnosticRecorder} from './oob'; import {checkIfClassIsExported, checkIfGenericTypesAreUnbound, tsCallMethod, tsCastToAny, tsCreateElement, tsCreateVariable, tsDeclareVariable} from './ts_util'; @@ -28,15 +29,27 @@ import {checkIfClassIsExported, checkIfGenericTypesAreUnbound, tsCallMethod, tsC * When passed through TypeScript's TypeChecker, type errors that arise within the type check block * function indicate issues in the template itself. * - * @param node the TypeScript node for the component class. + * As a side effect of generating a TCB for the component, `ts.Diagnostic`s may also be produced + * directly for issues within the template which are identified during generation. These issues are + * recorded in either the `domSchemaChecker` (which checks usage of DOM elements and bindings) as + * well as the `oobRecorder` (which records errors when the type-checking code generator is unable + * to sufficiently understand a template). + * + * @param env an `Environment` into which type-checking code will be generated. + * @param ref a `Reference` to the component class which should be type-checked. + * @param name a `ts.Identifier` to use for the generated `ts.FunctionDeclaration`. * @param meta metadata about the component's template and the function being generated. - * @param importManager an `ImportManager` for the file into which the TCB will be written. + * @param domSchemaChecker used to check and record errors regarding improper usage of DOM elements + * and bindings. + * @param oobRecorder used to record errors regarding template elements which could not be correctly + * translated into types during TCB generation. */ export function generateTypeCheckBlock( env: Environment, ref: Reference>, name: ts.Identifier, - meta: TypeCheckBlockMetadata, domSchemaChecker: DomSchemaChecker): ts.FunctionDeclaration { - const tcb = - new Context(env, domSchemaChecker, meta.id, meta.boundTarget, meta.pipes, meta.schemas); + meta: TypeCheckBlockMetadata, domSchemaChecker: DomSchemaChecker, + oobRecorder: OutOfBandDiagnosticRecorder): ts.FunctionDeclaration { + const tcb = new Context( + env, domSchemaChecker, oobRecorder, meta.id, meta.boundTarget, meta.pipes, meta.schemas); const scope = Scope.forNodes(tcb, null, tcb.boundTarget.target.template !); const ctxRawType = env.referenceType(ref); if (!ts.isTypeReferenceNode(ctxRawType)) { @@ -562,7 +575,8 @@ export class Context { private nextId = 1; constructor( - readonly env: Environment, readonly domSchemaChecker: DomSchemaChecker, readonly id: string, + readonly env: Environment, readonly domSchemaChecker: DomSchemaChecker, + readonly oobRecorder: OutOfBandDiagnosticRecorder, readonly id: string, readonly boundTarget: BoundTarget, private pipes: Map>>, readonly schemas: SchemaMetadata[]) {} @@ -797,6 +811,7 @@ class Scope { for (const child of node.children) { this.appendNode(child); } + this.checkReferencesOfNode(node); } else if (node instanceof TmplAstTemplate) { // Template children are rendered in a child scope. this.appendDirectivesAndInputsOfNode(node); @@ -806,11 +821,20 @@ class Scope { this.templateCtxOpMap.set(node, ctxIndex); this.opQueue.push(new TcbTemplateBodyOp(this.tcb, this, node)); } + this.checkReferencesOfNode(node); } else if (node instanceof TmplAstBoundText) { this.opQueue.push(new TcbTextInterpolationOp(this.tcb, this, node)); } } + private checkReferencesOfNode(node: TmplAstElement|TmplAstTemplate): void { + for (const ref of node.references) { + if (this.tcb.boundTarget.getReferenceTarget(ref) === null) { + this.tcb.oobRecorder.missingReferenceTarget(this.tcb.id, ref); + } + } + } + private appendDirectivesAndInputsOfNode(node: TmplAstElement|TmplAstTemplate): void { // Collect all the inputs on the element. const claimedInputs = new Set(); @@ -1025,7 +1049,10 @@ class TcbExpressionTranslator { } else if (binding instanceof TmplAstReference) { const target = this.tcb.boundTarget.getReferenceTarget(binding); if (target === null) { - throw new Error(`Unbound reference? ${binding.name}`); + // This reference is unbound. Traversal of the `TmplAstReference` itself should have + // recorded the error in the `OutOfBandDiagnosticRecorder`. + // Still check the rest of the expression if possible by using an `any` value. + return NULL_AS_ANY; } // The reference is either to an element, an node, or to a directive on an diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_file.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_file.ts index 5875911fd23ac..3a08bde37fb99 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_file.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_file.ts @@ -15,9 +15,11 @@ import {ImportManager} from '../../translator'; import {TypeCheckBlockMetadata, TypeCheckingConfig} from './api'; import {DomSchemaChecker} from './dom'; import {Environment} from './environment'; +import {OutOfBandDiagnosticRecorder} from './oob'; import {generateTypeCheckBlock} from './type_check_block'; + /** * An `Environment` representing the single type-checking file into which most (if not all) Type * Check Blocks (TCBs) will be generated. @@ -38,9 +40,9 @@ export class TypeCheckFile extends Environment { addTypeCheckBlock( ref: Reference>, meta: TypeCheckBlockMetadata, - domSchemaChecker: DomSchemaChecker): void { + domSchemaChecker: DomSchemaChecker, oobRecorder: OutOfBandDiagnosticRecorder): void { const fnId = ts.createIdentifier(`_tcb${this.nextTcbId++}`); - const fn = generateTypeCheckBlock(this, ref, fnId, meta, domSchemaChecker); + const fn = generateTypeCheckBlock(this, ref, fnId, meta, domSchemaChecker, oobRecorder); this.tcbStatements.push(fn); } 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 3945b02d1d6fb..026c41236c22d 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {CssSelector, ParseSourceFile, ParseSourceSpan, R3TargetBinder, SchemaMetadata, SelectorMatcher, TmplAstElement, Type, parseTemplate} from '@angular/compiler'; +import {CssSelector, ParseSourceFile, ParseSourceSpan, R3TargetBinder, SchemaMetadata, SelectorMatcher, TmplAstElement, TmplAstReference, Type, parseTemplate} from '@angular/compiler'; import * as ts from 'typescript'; import {AbsoluteFsPath, LogicalFileSystem, absoluteFrom} from '../../file_system'; @@ -19,6 +19,7 @@ import {TemplateSourceMapping, TypeCheckBlockMetadata, TypeCheckableDirectiveMet import {TypeCheckContext} from '../src/context'; import {DomSchemaChecker} from '../src/dom'; import {Environment} from '../src/environment'; +import {OutOfBandDiagnosticRecorder} from '../src/oob'; import {generateTypeCheckBlock} from '../src/type_check_block'; export function typescriptLibDts(): TestFile { @@ -220,7 +221,7 @@ export function tcb( const tcb = generateTypeCheckBlock( FakeEnvironment.newFake(config), new Reference(clazz), ts.createIdentifier('Test_TCB'), meta, - new NoopSchemaChecker()); + new NoopSchemaChecker(), new NoopOobRecorder()); const removeComments = !options.emitSpans; const res = ts.createPrinter({removeComments}).printNode(ts.EmitHint.Unspecified, tcb, sf); @@ -382,3 +383,8 @@ export class NoopSchemaChecker implements DomSchemaChecker { id: string, element: TmplAstElement, name: string, span: ParseSourceSpan, schemas: SchemaMetadata[]): void {} } + +export class NoopOobRecorder implements OutOfBandDiagnosticRecorder { + get diagnostics(): ReadonlyArray { return []; } + missingReferenceTarget(): void {} +} diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index 49e46d5b84d08..a8abb01bd08b6 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -3137,6 +3137,24 @@ runInEachFileSystem(os => { }); }); + describe('local refs', () => { + it('should not generate an error when a local ref is unresolved' + + ' (outside of template type-checking)', + () => { + + env.write('test.ts', ` + import {Component} from '@angular/core'; + + @Component({ + template: '
', + }) + export class TestCmp {} + `); + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(0); + }); + }); + describe('multiple local refs', () => { const getComponentScript = (template: string): string => ` import {Component, Directive, NgModule} from '@angular/core'; diff --git a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts index 9522cf8a82e2b..b5f3eca79160e 100644 --- a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts +++ b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts @@ -714,6 +714,27 @@ export declare class AnimationEvent { env.driveMain(); }); + it('should report an error with an unknown local ref target', () => { + env.write('test.ts', ` + import {Component, NgModule} from '@angular/core'; + + @Component({ + selector: 'test', + template: '
', + }) + class TestCmp {} + + @NgModule({ + declarations: [TestCmp], + }) + class Module {} + `); + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(1); + expect(diags[0].messageText).toBe(`No directive found with exportAs 'unknownTarget'.`); + expect(getSourceCodeForDiagnostic(diags[0])).toBe('unknownTarget'); + }); + it('should report an error with pipe bindings', () => { env.write('test.ts', ` import {CommonModule} from '@angular/common'; diff --git a/packages/compiler/src/render3/view/t2_binder.ts b/packages/compiler/src/render3/view/t2_binder.ts index a1eb09df4b0cc..e684b29e0a164 100644 --- a/packages/compiler/src/render3/view/t2_binder.ts +++ b/packages/compiler/src/render3/view/t2_binder.ts @@ -260,16 +260,16 @@ class DirectiveBinder implements Visitor { // This could be a reference to a component if there is one. dirTarget = directives.find(dir => dir.isComponent) || null; } else { - // This is a reference to a directive exported via exportAs. One should exist. + // This should be a reference to a directive exported via exportAs. dirTarget = directives.find( dir => dir.exportAs !== null && dir.exportAs.some(value => value === ref.value)) || null; - - // Check if a matching directive was found, and error if it wasn't. + // Check if a matching directive was found. if (dirTarget === null) { - // TODO(alxhub): Return an error value here that can be used for template validation. - throw new Error(`Assertion error: failed to find directive with exportAs: ${ref.value}`); + // No matching directive was found - this reference points to an unknown target. Leave it + // unmapped. + return; } }