From 38758d856a49a2f793dfe8c042b00b99c6af1c0f Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Mon, 28 Oct 2019 15:29:03 -0700 Subject: [PATCH] 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 --- .../src/ngtsc/diagnostics/src/code.ts | 5 +++ .../src/ngtsc/typecheck/src/diagnostics.ts | 7 +++- .../src/ngtsc/typecheck/src/oob.ts | 34 +++++++++++++++++-- .../ngtsc/typecheck/src/type_check_block.ts | 14 ++++++-- .../src/ngtsc/typecheck/test/test_utils.ts | 1 + .../test/ngtsc/template_typecheck_spec.ts | 23 +++++++++++++ .../compiler/src/expression_parser/ast.ts | 7 ++-- .../compiler/src/expression_parser/parser.ts | 5 ++- 8 files changed, 86 insertions(+), 10 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts b/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts index 67f44e6e32e75..592c12059c9fa 100644 --- a/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts +++ b/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts @@ -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 { diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/diagnostics.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/diagnostics.ts index dae298b33ff26..6f761c843c57d 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/diagnostics.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/diagnostics.ts @@ -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'; @@ -55,6 +55,11 @@ export function toAbsoluteSpan(span: ParseSpan, sourceSpan: ParseSourceSpan): Ab return {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 diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts index 17fc520a857a3..4204f75b01b4d 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts @@ -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 @@ -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 { @@ -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)); + } } 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 92774e7fb747f..705de13bf6e79 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 @@ -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) !); } @@ -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))); 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 026c41236c22d..76962ea6a1754 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts @@ -387,4 +387,5 @@ export class NoopSchemaChecker implements DomSchemaChecker { export class NoopOobRecorder implements OutOfBandDiagnosticRecorder { get diagnostics(): ReadonlyArray { return []; } missingReferenceTarget(): void {} + missingPipe(): void {} } diff --git a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts index b5f3eca79160e..a7d4cb52fb1ae 100644 --- a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts +++ b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts @@ -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'; diff --git a/packages/compiler/src/expression_parser/ast.ts b/packages/compiler/src/expression_parser/ast.ts index 5323373a1959b..2b7052e7919ef 100644 --- a/packages/compiler/src/expression_parser/ast.ts +++ b/packages/compiler/src/expression_parser/ast.ts @@ -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); } @@ -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 { @@ -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; } diff --git a/packages/compiler/src/expression_parser/parser.ts b/packages/compiler/src/expression_parser/parser.ts index 4a6ca50aeed9a..cc78d116032f2 100644 --- a/packages/compiler/src/expression_parser/parser.ts +++ b/packages/compiler/src/expression_parser/parser.ts @@ -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('|')); }