From 9322b9a06040e3401377e2123691230e1e7e62e8 Mon Sep 17 00:00:00 2001 From: crisbeto Date: Sat, 4 Jul 2020 09:21:40 +0200 Subject: [PATCH] fix(compiler): check more cases for pipe usage inside host bindings (#37883) Builds on top of #34655 to support more cases that could be using a pipe inside host bindings (e.g. ternary expressions or function calls). Fixes #37610. PR Close #37883 --- .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 38 ---------- .../compiler/src/expression_parser/parser.ts | 25 +++---- .../test/expression_parser/parser_spec.ts | 73 ++++++++++++++++++- 3 files changed, 83 insertions(+), 53 deletions(-) diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index 7e1ae0cde2..9ad1a3a678 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -3071,44 +3071,6 @@ runInEachFileSystem(os => { .toContain('Host binding expression cannot contain pipes'); }); - it('should throw in case pipes are used in host bindings (defined as `!(value | pipe)`)', - () => { - env.write(`test.ts`, ` - import {Component} from '@angular/core'; - - @Component({ - selector: 'test', - template: '...', - host: { - '[id]': '!(id | myPipe)' - } - }) - class FooCmp {} - `); - const errors = env.driveDiagnostics(); - expect(trim(errors[0].messageText as string)) - .toContain('Host binding expression cannot contain pipes'); - }); - - it('should throw in case pipes are used in host bindings (defined as `(value | pipe) === X`)', - () => { - env.write(`test.ts`, ` - import {Component} from '@angular/core'; - - @Component({ - selector: 'test', - template: '...', - host: { - '[id]': '(id | myPipe) === true' - } - }) - class FooCmp {} - `); - const errors = env.driveDiagnostics(); - expect(trim(errors[0].messageText as string)) - .toContain('Host binding expression cannot contain pipes'); - }); - it('should generate host bindings for directives', () => { env.write(`test.ts`, ` import {Component, HostBinding, HostListener, TemplateRef} from '@angular/core'; diff --git a/packages/compiler/src/expression_parser/parser.ts b/packages/compiler/src/expression_parser/parser.ts index 743205b920..8f97b71ff3 100644 --- a/packages/compiler/src/expression_parser/parser.ts +++ b/packages/compiler/src/expression_parser/parser.ts @@ -10,7 +10,7 @@ import * as chars from '../chars'; import {DEFAULT_INTERPOLATION_CONFIG, InterpolationConfig} from '../ml_parser/interpolation_config'; import {escapeRegExp} from '../util'; -import {AbsoluteSourceSpan, AST, AstVisitor, ASTWithSource, Binary, BindingPipe, Chain, Conditional, EmptyExpr, ExpressionBinding, FunctionCall, ImplicitReceiver, Interpolation, KeyedRead, KeyedWrite, LiteralArray, LiteralMap, LiteralMapKey, LiteralPrimitive, MethodCall, NonNullAssert, ParserError, ParseSpan, PrefixNot, PropertyRead, PropertyWrite, Quote, SafeMethodCall, SafePropertyRead, TemplateBinding, TemplateBindingIdentifier, VariableBinding} from './ast'; +import {AbsoluteSourceSpan, AST, AstVisitor, ASTWithSource, Binary, BindingPipe, Chain, Conditional, EmptyExpr, ExpressionBinding, FunctionCall, ImplicitReceiver, Interpolation, KeyedRead, KeyedWrite, LiteralArray, LiteralMap, LiteralMapKey, LiteralPrimitive, MethodCall, NonNullAssert, ParserError, ParseSpan, PrefixNot, PropertyRead, PropertyWrite, Quote, RecursiveAstVisitor, SafeMethodCall, SafePropertyRead, TemplateBinding, TemplateBindingIdentifier, VariableBinding} from './ast'; import {EOF, isIdentifier, isQuote, Lexer, Token, TokenType} from './lexer'; export class SplitInterpolation { @@ -1052,11 +1052,11 @@ class SimpleExpressionChecker implements AstVisitor { visitFunctionCall(ast: FunctionCall, context: any) {} visitLiteralArray(ast: LiteralArray, context: any) { - this.visitAll(ast.expressions); + this.visitAll(ast.expressions, context); } visitLiteralMap(ast: LiteralMap, context: any) { - this.visitAll(ast.values); + this.visitAll(ast.values, context); } visitBinary(ast: Binary, context: any) {} @@ -1075,8 +1075,8 @@ class SimpleExpressionChecker implements AstVisitor { visitKeyedWrite(ast: KeyedWrite, context: any) {} - visitAll(asts: any[]): any[] { - return asts.map(node => node.visit(this)); + visitAll(asts: any[], context: any): any[] { + return asts.map(node => node.visit(this, context)); } visitChain(ast: Chain, context: any) {} @@ -1085,19 +1085,16 @@ class SimpleExpressionChecker implements AstVisitor { } /** - * This class extends SimpleExpressionChecker used in View Engine and performs more strict checks to - * make sure host bindings do not contain pipes. In View Engine, having pipes in host bindings is + * This class implements SimpleExpressionChecker used in View Engine and performs more strict checks + * to make sure host bindings do not contain pipes. In View Engine, having pipes in host bindings is * not supported as well, but in some cases (like `!(value | async)`) the error is not triggered at * compile time. In order to preserve View Engine behavior, more strict checks are introduced for * Ivy mode only. */ -class IvySimpleExpressionChecker extends SimpleExpressionChecker { - visitBinary(ast: Binary, context: any) { - ast.left.visit(this); - ast.right.visit(this); - } +class IvySimpleExpressionChecker extends RecursiveAstVisitor implements SimpleExpressionChecker { + errors: string[] = []; - visitPrefixNot(ast: PrefixNot, context: any) { - ast.expression.visit(this); + visitPipe() { + this.errors.push('pipes'); } } diff --git a/packages/compiler/test/expression_parser/parser_spec.ts b/packages/compiler/test/expression_parser/parser_spec.ts index f11479b465..e58f3a7efe 100644 --- a/packages/compiler/test/expression_parser/parser_spec.ts +++ b/packages/compiler/test/expression_parser/parser_spec.ts @@ -8,7 +8,7 @@ import {ASTWithSource, BindingPipe, Interpolation, ParserError, TemplateBinding, VariableBinding} from '@angular/compiler/src/expression_parser/ast'; import {Lexer} from '@angular/compiler/src/expression_parser/lexer'; -import {Parser, SplitInterpolation} from '@angular/compiler/src/expression_parser/parser'; +import {IvyParser, Parser, SplitInterpolation} from '@angular/compiler/src/expression_parser/parser'; import {expect} from '@angular/platform-browser/testing/src/matchers'; @@ -740,6 +740,68 @@ describe('parser', () => { it('should report when encountering field write', () => { expectError(validate(parseSimpleBinding('a = b')), 'Bindings cannot contain assignments'); }); + + describe('Ivy-only validations', () => { + it('should throw if a pipe is used inside a conditional', () => { + expectError( + validate(parseSimpleBindingIvy('(hasId | myPipe) ? "my-id" : ""')), + 'Host binding expression cannot contain pipes'); + }); + + it('should throw if a pipe is used inside a function call', () => { + expectError( + validate(parseSimpleBindingIvy('getId(true, id | myPipe)')), + 'Host binding expression cannot contain pipes'); + }); + + it('should throw if a pipe is used inside a method call', () => { + expectError( + validate(parseSimpleBindingIvy('idService.getId(true, id | myPipe)')), + 'Host binding expression cannot contain pipes'); + }); + + it('should throw if a pipe is used inside a safe method call', () => { + expectError( + validate(parseSimpleBindingIvy('idService?.getId(true, id | myPipe)')), + 'Host binding expression cannot contain pipes'); + }); + + it('should throw if a pipe is used inside a property access', () => { + expectError( + validate(parseSimpleBindingIvy('a[id | myPipe]')), + 'Host binding expression cannot contain pipes'); + }); + + it('should throw if a pipe is used inside a keyed read expression', () => { + expectError( + validate(parseSimpleBindingIvy('a[id | myPipe].b')), + 'Host binding expression cannot contain pipes'); + }); + + it('should throw if a pipe is used inside a safe property read', () => { + expectError( + validate(parseSimpleBindingIvy('(id | myPipe)?.id')), + 'Host binding expression cannot contain pipes'); + }); + + it('should throw if a pipe is used inside a non-null assertion', () => { + expectError( + validate(parseSimpleBindingIvy('[id | myPipe]!')), + 'Host binding expression cannot contain pipes'); + }); + + it('should throw if a pipe is used inside a prefix not expression', () => { + expectError( + validate(parseSimpleBindingIvy('!(id | myPipe)')), + 'Host binding expression cannot contain pipes'); + }); + + it('should throw if a pipe is used inside a binary expression', () => { + expectError( + validate(parseSimpleBindingIvy('(id | myPipe) === true')), + 'Host binding expression cannot contain pipes'); + }); + }); }); describe('wrapLiteralPrimitive', () => { @@ -781,6 +843,10 @@ function createParser() { return new Parser(new Lexer()); } +function createIvyParser() { + return new IvyParser(new Lexer()); +} + function parseAction(text: string, location: any = null, offset: number = 0): ASTWithSource { return createParser().parseAction(text, location, offset); } @@ -816,6 +882,11 @@ function parseSimpleBinding(text: string, location: any = null, offset: number = return createParser().parseSimpleBinding(text, location, offset); } +function parseSimpleBindingIvy( + text: string, location: any = null, offset: number = 0): ASTWithSource { + return createIvyParser().parseSimpleBinding(text, location, offset); +} + function checkInterpolation(exp: string, expected?: string) { const ast = parseInterpolation(exp)!; if (expected == null) expected = exp;