Skip to content

Commit

Permalink
fix(ivy): don't crash on an unknown localref target
Browse files Browse the repository at this point in the history
Previously the template binder would crash when encountering an unknown
localref (# reference) such as `<div #ref="foo">` when no directive has
`exportAs: "foo"`.

With this commit, the compiler instead generates a template diagnostic error
informing the user about the invalid reference.
  • Loading branch information
alxhub committed Oct 28, 2019
1 parent e483aca commit c54ce09
Show file tree
Hide file tree
Showing 8 changed files with 119 additions and 12 deletions.
5 changes: 5 additions & 0 deletions packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts
Expand Up @@ -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 {
Expand Down
15 changes: 11 additions & 4 deletions packages/compiler-cli/src/ngtsc/typecheck/src/context.ts
Expand Up @@ -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';
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -219,6 +223,7 @@ export class TypeCheckContext {
}

diagnostics.push(...this.domSchemaChecker.diagnostics);
diagnostics.push(...this.oobRecorder.diagnostics);

return {
diagnostics,
Expand All @@ -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));
}
}

Expand Down Expand Up @@ -266,7 +271,8 @@ class TcbOp implements Op {
constructor(
readonly ref: Reference<ClassDeclaration<ts.ClassDeclaration>>,
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.
Expand All @@ -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);
}
}
Expand Down
39 changes: 39 additions & 0 deletions packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts
@@ -0,0 +1,39 @@
/**
* @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 {AbsoluteSourceSpan, BindingPipe, TmplAstReference} from '@angular/compiler';
import * as ts from 'typescript';

import {ErrorCode, ngErrorCode} from '../../diagnostics';

import {TcbSourceResolver, absoluteSourceSpanToSourceLocation, makeTemplateDiagnostic} from './diagnostics';

/**
* Collects reports on template diagnostics which are
*/
export interface OutOfBandDiagnosticRecorder {
readonly diagnostics: ts.Diagnostic[];

missingReferenceTarget(id: string, ref: TmplAstReference): void;
}

export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecorder {
readonly diagnostics: ts.Diagnostic[] = [];

constructor(private resolver: TcbSourceResolver) {}

missingReferenceTarget(id: string, ref: TmplAstReference): void {
const mapping = this.resolver.getSourceMapping(id);
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));
}
}
19 changes: 17 additions & 2 deletions packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts
Expand Up @@ -562,7 +562,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<TypeCheckableDirectiveMeta>,
private pipes: Map<string, Reference<ClassDeclaration<ts.ClassDeclaration>>>,
readonly schemas: SchemaMetadata[]) {}
Expand Down Expand Up @@ -797,6 +798,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);
Expand All @@ -806,11 +808,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<string>();
Expand Down Expand Up @@ -1025,7 +1036,11 @@ 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.
this.tcb.oobRecorder.missingReferenceTarget(this.tcb.id, binding);

// Still check the rest of the expression if possible.
return NULL_AS_ANY;
}

// The reference is either to an element, an <ng-template> node, or to a directive on an
Expand Down
Expand Up @@ -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.
Expand All @@ -38,9 +40,9 @@ export class TypeCheckFile extends Environment {

addTypeCheckBlock(
ref: Reference<ClassDeclaration<ts.ClassDeclaration>>, 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);
}

Expand Down
18 changes: 18 additions & 0 deletions packages/compiler-cli/test/ngtsc/ngtsc_spec.ts
Expand Up @@ -3103,6 +3103,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: '<div #ref="unknownTarget"></div>',
})
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';
Expand Down
21 changes: 21 additions & 0 deletions packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts
Expand Up @@ -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: '<div #ref="unknownTarget"></div>',
})
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';
Expand Down
8 changes: 4 additions & 4 deletions packages/compiler/src/render3/view/t2_binder.ts
Expand Up @@ -265,11 +265,11 @@ class DirectiveBinder<DirectiveT extends DirectiveMeta> implements Visitor {
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.
// Don't map it.
return;
}
}

Expand Down

0 comments on commit c54ce09

Please sign in to comment.