Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
fix(ivy): don't crash on unknown pipe (#33454)
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.

PR Close #33454
  • Loading branch information
alxhub authored and atscott committed Oct 31, 2019
1 parent 9db59d0 commit 38758d8
Show file tree
Hide file tree
Showing 8 changed files with 86 additions and 10 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
34 changes: 32 additions & 2 deletions packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts
Expand Up @@ -6,12 +6,14 @@
* found in the LICENSE file at https://angular.io/license
*/

import {TmplAstReference} from '@angular/compiler';
import {AbsoluteSourceSpan, BindingPipe, TmplAstReference} from '@angular/compiler';
import * as ts from 'typescript';

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

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



/**
* Collects `ts.Diagnostic`s on problems which occur in the template which aren't directly sourced
Expand All @@ -34,6 +36,19 @@ export interface OutOfBandDiagnosticRecorder {
* @param ref the `TmplAstReference` which could not be matched to a directive.
*/
missingReferenceTarget(templateId: string, ref: TmplAstReference): void;

/**
* Reports usage of a `| pipe` expression in the template for which the named pipe could not be
* found.
*
* @param templateId the template type-checking ID of the template which contains the unknown
* pipe.
* @param ast the `BindingPipe` invocation of the pipe which could not be found.
* @param sourceSpan the `AbsoluteSourceSpan` of the pipe invocation (ideally, this should be the
* source span of the pipe's name). This depends on the source span of the `BindingPipe` itself
* plus span of the larger expression context.
*/
missingPipe(templateId: string, ast: BindingPipe, sourceSpan: AbsoluteSourceSpan): void;
}

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

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

const location = absoluteSourceSpanToSourceLocation(templateId, absSpan);
const sourceSpan = this.resolver.sourceLocationToSpan(location);
if (sourceSpan === null) {
throw new Error(
`Assertion failure: no SourceLocation found for usage of pipe '${ast.name}'.`);
}
this._diagnostics.push(makeTemplateDiagnostic(
mapping, sourceSpan, ts.DiagnosticCategory.Error, ngErrorCode(ErrorCode.MISSING_PIPE),
errorMsg));
}
}
14 changes: 11 additions & 3 deletions packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts
Expand Up @@ -589,9 +589,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 @@ -988,9 +988,17 @@ 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;
if (this.tcb.env.config.checkTypeOfPipes) {
pipe = this.tcb.getPipeByName(ast.name);
if (pipe === null) {
// No pipe by that name exists in scope. Record this as an error.
const nameAbsoluteSpan = toAbsoluteSpan(ast.nameSpan, this.sourceSpan);
this.tcb.oobRecorder.missingPipe(this.tcb.id, ast, nameAbsoluteSpan);

// Return an 'any' value to at least allow the rest of the expression to be checked.
pipe = NULL_AS_ANY;
}
} else {
pipe = ts.createParen(ts.createAsExpression(
ts.createNull(), ts.createKeywordTypeNode(ts.SyntaxKind.AnyKeyword)));
Expand Down
Expand Up @@ -387,4 +387,5 @@ export class NoopSchemaChecker implements DomSchemaChecker {
export class NoopOobRecorder implements OutOfBandDiagnosticRecorder {
get diagnostics(): ReadonlyArray<ts.Diagnostic> { return []; }
missingReferenceTarget(): void {}
missingPipe(): void {}
}
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 38758d8

Please sign in to comment.