Skip to content

Commit

Permalink
fix(ivy): don't crash on unknown pipe
Browse files Browse the repository at this point in the history
Previously the compiler would crash if a pipe was encountered which did not
match any pipe in the scope of a template.

This commit introduces a new diagnostic error for unknown pipes instead.
  • Loading branch information
alxhub committed Oct 29, 2019
1 parent 14e4b09 commit fd267e8
Show file tree
Hide file tree
Showing 7 changed files with 73 additions and 11 deletions.
5 changes: 5 additions & 0 deletions packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts
Expand Up @@ -89,6 +89,11 @@ export enum ErrorCode {
* No matching directive was found for a `#ref="target"` expression.
*/
MISSING_REFERENCE_TARGET = 8003,

/**
* No matching pipe was found for a
*/
MISSING_PIPE = 8004,
}

export function ngErrorCode(code: ErrorCode): number {
Expand Down
7 changes: 6 additions & 1 deletion packages/compiler-cli/src/ngtsc/typecheck/src/diagnostics.ts
Expand Up @@ -5,7 +5,7 @@
* 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 {ParseSourceSpan, ParseSpan, Position} from '@angular/compiler';
import {AbsoluteSourceSpan, ParseSourceSpan, ParseSpan, Position} from '@angular/compiler';
import * as ts from 'typescript';

import {getTokenAtPosition} from '../../util/src/typescript';
Expand Down Expand Up @@ -55,6 +55,11 @@ export function toAbsoluteSpan(span: ParseSpan, sourceSpan: ParseSourceSpan): Ab
return <AbsoluteSpan>{start: span.start + offset, end: span.end + offset};
}

export function absoluteSourceSpanToSourceLocation(
id: string, span: AbsoluteSourceSpan): SourceLocation {
return {id, ...span};
}

/**
* Wraps the node in parenthesis such that inserted span comments become attached to the proper
* node. This is an alias for `ts.createParen` with the benefit that it signifies that the
Expand Down
18 changes: 18 additions & 0 deletions packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts
Expand Up @@ -20,12 +20,16 @@ export interface OutOfBandDiagnosticRecorder {
readonly diagnostics: ts.Diagnostic[];

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

missingPipe(id: string, ast: BindingPipe, sourceSpan: AbsoluteSourceSpan): void;
}

export class NoopOutOfBandDiagnosticRecorder implements OutOfBandDiagnosticRecorder {
get diagnostics(): ts.Diagnostic[] { return []; }

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

missingPipe(id: string, ast: BindingPipe, sourceSpan: AbsoluteSourceSpan): void {}
}

export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecorder {
Expand All @@ -42,4 +46,18 @@ export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecor
mapping, ref.valueSpan || ref.sourceSpan, ts.DiagnosticCategory.Error,
ngErrorCode(ErrorCode.MISSING_REFERENCE_TARGET), errorMsg));
}

missingPipe(id: string, ast: BindingPipe, absSpan: AbsoluteSourceSpan): void {
const mapping = this.resolver.getSourceMapping(id);
const errorMsg = `No pipe found with name '${ast.name}'.`;

const location = absoluteSourceSpanToSourceLocation(id, absSpan);
const sourceSpan = this.resolver.sourceLocationToSpan(location);
if (sourceSpan === null) {
throw new Error(`Could not map location for error`);
}
this.diagnostics.push(makeTemplateDiagnostic(
mapping, sourceSpan, ts.DiagnosticCategory.Error, ngErrorCode(ErrorCode.MISSING_PIPE),
errorMsg));
}
}
19 changes: 13 additions & 6 deletions packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts
Expand Up @@ -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';


Expand All @@ -34,9 +35,10 @@ import {checkIfClassIsExported, checkIfGenericTypesAreUnbound, tsCallMethod, tsC
*/
export function generateTypeCheckBlock(
env: Environment, ref: Reference<ClassDeclaration<ts.ClassDeclaration>>, 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)) {
Expand Down Expand Up @@ -576,9 +578,9 @@ export class Context {
*/
allocateId(): ts.Identifier { return ts.createIdentifier(`_t${this.nextId++}`); }

getPipeByName(name: string): ts.Expression {
getPipeByName(name: string): ts.Expression|null {
if (!this.pipes.has(name)) {
throw new Error(`Missing pipe: ${name}`);
return null;
}
return this.env.pipeInst(this.pipes.get(name) !);
}
Expand Down Expand Up @@ -975,9 +977,14 @@ class TcbExpressionTranslator {
return ts.createIdentifier('ctx');
} else if (ast instanceof BindingPipe) {
const expr = this.translate(ast.exp);
let pipe: ts.Expression;
let pipe: ts.Expression|null;
const nameAbsoluteSpan = toAbsoluteSpan(ast.nameSpan, this.sourceSpan);
if (this.tcb.env.config.checkTypeOfPipes) {
pipe = this.tcb.getPipeByName(ast.name);
if (pipe === null) {
this.tcb.oobRecorder.missingPipe(this.tcb.id, ast, nameAbsoluteSpan);
pipe = NULL_AS_ANY;
}
} else {
pipe = ts.createParen(ts.createAsExpression(
ts.createNull(), ts.createKeywordTypeNode(ts.SyntaxKind.AnyKeyword)));
Expand Down
23 changes: 23 additions & 0 deletions packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts
Expand Up @@ -735,6 +735,29 @@ export declare class AnimationEvent {
expect(getSourceCodeForDiagnostic(diags[0])).toBe('unknownTarget');
});

it('should report an error with an unknown pipe', () => {
env.write('test.ts', `
import {Component, NgModule} from '@angular/core';
@Component({
selector: 'test',
template: '{{expr | unknown}}',
})
class TestCmp {
expr = 3;
}
@NgModule({
declarations: [TestCmp],
})
class Module {}
`);
const diags = env.driveDiagnostics();
expect(diags.length).toBe(1);
expect(diags[0].messageText).toBe(`No pipe found with name 'unknown'.`);
expect(getSourceCodeForDiagnostic(diags[0])).toBe('unknown');
});

it('should report an error with pipe bindings', () => {
env.write('test.ts', `
import {CommonModule} from '@angular/common';
Expand Down
7 changes: 4 additions & 3 deletions packages/compiler/src/expression_parser/ast.ts
Expand Up @@ -145,7 +145,7 @@ export class KeyedWrite extends AST {
export class BindingPipe extends AST {
constructor(
span: ParseSpan, sourceSpan: AbsoluteSourceSpan, public exp: AST, public name: string,
public args: any[]) {
public args: any[], public nameSpan: ParseSpan) {
super(span, sourceSpan);
}
visit(visitor: AstVisitor, context: any = null): any { return visitor.visitPipe(this, context); }
Expand Down Expand Up @@ -484,7 +484,8 @@ export class AstTransformer implements AstVisitor {

visitPipe(ast: BindingPipe, context: any): AST {
return new BindingPipe(
ast.span, ast.sourceSpan, ast.exp.visit(this), ast.name, this.visitAll(ast.args));
ast.span, ast.sourceSpan, ast.exp.visit(this), ast.name, this.visitAll(ast.args),
ast.nameSpan);
}

visitKeyedRead(ast: KeyedRead, context: any): AST {
Expand Down Expand Up @@ -635,7 +636,7 @@ export class AstMemoryEfficientTransformer implements AstVisitor {
const exp = ast.exp.visit(this);
const args = this.visitAll(ast.args);
if (exp !== ast.exp || args !== ast.args) {
return new BindingPipe(ast.span, ast.sourceSpan, exp, ast.name, args);
return new BindingPipe(ast.span, ast.sourceSpan, exp, ast.name, args, ast.nameSpan);
}
return ast;
}
Expand Down
5 changes: 4 additions & 1 deletion packages/compiler/src/expression_parser/parser.ts
Expand Up @@ -348,13 +348,16 @@ export class _ParseAST {
}

do {
const nameStart = this.inputIndex;
const name = this.expectIdentifierOrKeyword();
const nameSpan = this.span(nameStart);
const args: AST[] = [];
while (this.optionalCharacter(chars.$COLON)) {
args.push(this.parseExpression());
}
const {start} = result.span;
result = new BindingPipe(this.span(start), this.sourceSpan(start), result, name, args);
result =
new BindingPipe(this.span(start), this.sourceSpan(start), result, name, args, nameSpan);
} while (this.optionalOperator('|'));
}

Expand Down

0 comments on commit fd267e8

Please sign in to comment.