From 76cedb8bf3890a2c85288299dcd512f3c79f53bf Mon Sep 17 00:00:00 2001 From: Andrew Kushnir Date: Thu, 24 Jan 2019 17:25:46 -0800 Subject: [PATCH] fix(ivy): verify Host Bindings and Host Listeners before compiling them (#28356) Prior to this change we may encounter some errors (like pipes being used where they should not be used) while compiling Host Bindings and Listeners. With this update we move validation logic to the analyze phase and throw an error if something is wrong. This also aligns error messages between Ivy and VE. PR Close #28356 --- .../src/ngtsc/annotations/src/directive.ts | 20 ++++++--- .../src/ngtsc/diagnostics/src/code.ts | 6 +++ .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 38 +++++++++++++++++ packages/compiler/src/compiler.ts | 2 +- .../compiler/src/compiler_facade_interface.ts | 10 ++++- packages/compiler/src/jit_compiler_facade.ts | 29 +++++++++---- packages/compiler/src/parse_util.ts | 16 +++++++ .../compiler/src/render3/view/compiler.ts | 37 ++++++++++++---- .../src/compiler/compiler_facade_interface.ts | 10 ++++- packages/core/src/render3/jit/directive.ts | 14 +++++-- packages/core/test/linker/integration_spec.ts | 42 +++++++++---------- 11 files changed, 175 insertions(+), 49 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts b/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts index 27f569af19..4d23588ce5 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {ConstantPool, Expression, R3DirectiveMetadata, R3QueryMetadata, Statement, WrappedNodeExpr, compileDirectiveFromMetadata, makeBindingParser, parseHostBindings} from '@angular/compiler'; +import {ConstantPool, Expression, ParseError, R3DirectiveMetadata, R3QueryMetadata, Statement, WrappedNodeExpr, compileDirectiveFromMetadata, makeBindingParser, parseHostBindings, verifyHostBindings} from '@angular/compiler'; import * as ts from 'typescript'; import {ErrorCode, FatalDiagnosticError} from '../../diagnostics'; @@ -437,7 +437,17 @@ function extractHostBindings( }); } - const {attributes, listeners, properties} = parseHostBindings(hostMetadata); + const bindings = parseHostBindings(hostMetadata); + + // TODO: create and provide proper sourceSpan to make error message more descriptive (FW-995) + const errors = verifyHostBindings(bindings, /* sourceSpan */ null !); + if (errors.length > 0) { + throw new FatalDiagnosticError( + // TODO: provide more granular diagnostic and output specific host expression that triggered + // an error instead of the whole host object + ErrorCode.HOST_BINDING_PARSE_ERROR, metadata.get('host') !, + errors.map((error: ParseError) => error.msg).join('\n')); + } filterToMembersWithDecorator(members, 'HostBinding', coreModule) .forEach(({member, decorators}) => { @@ -456,7 +466,7 @@ function extractHostBindings( hostPropertyName = resolved; } - properties[hostPropertyName] = member.name; + bindings.properties[hostPropertyName] = member.name; }); }); @@ -492,10 +502,10 @@ function extractHostBindings( } } - listeners[eventName] = `${member.name}(${args.join(',')})`; + bindings.listeners[eventName] = `${member.name}(${args.join(',')})`; }); }); - return {attributes, properties, listeners}; + return bindings; } const QUERY_TYPES = new Set([ diff --git a/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts b/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts index c65416334d..5b3338859d 100644 --- a/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts +++ b/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts @@ -24,6 +24,12 @@ export enum ErrorCode { SYMBOL_EXPORTED_UNDER_DIFFERENT_NAME = 3002, CONFIG_FLAT_MODULE_NO_INDEX = 4001, + + /** + * Raised when a host expression has a parse error, such as a host listener or host binding + * expression containing a pipe. + */ + HOST_BINDING_PARSE_ERROR = 5001, } export function ngErrorCode(code: ErrorCode): number { diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index e7a7f570df..2f46985930 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -827,6 +827,44 @@ describe('ngtsc behavioral tests', () => { `Unexpected global target 'UnknownTarget' defined for 'click' event. Supported list of global targets: window,document,body.`); }); + it('should throw in case pipes are used in host listeners', () => { + env.tsconfig(); + env.write(`test.ts`, ` + import {Component} from '@angular/core'; + + @Component({ + selector: 'test', + template: '...', + host: { + '(click)': 'doSmth() | myPipe' + } + }) + class FooCmp {} + `); + const errors = env.driveDiagnostics(); + expect(trim(errors[0].messageText as string)) + .toContain('Cannot have a pipe in an action expression'); + }); + + it('should throw in case pipes are used in host listeners', () => { + env.tsconfig(); + 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 generate host bindings for directives', () => { env.tsconfig(); env.write(`test.ts`, ` diff --git a/packages/compiler/src/compiler.ts b/packages/compiler/src/compiler.ts index e84e6b8575..692de99a14 100644 --- a/packages/compiler/src/compiler.ts +++ b/packages/compiler/src/compiler.ts @@ -98,7 +98,7 @@ export {compileInjector, compileNgModule, R3InjectorMetadata, R3NgModuleMetadata export {compilePipeFromMetadata, R3PipeMetadata} from './render3/r3_pipe_compiler'; export {makeBindingParser, parseTemplate} from './render3/view/template'; export {R3Reference} from './render3/util'; -export {compileBaseDefFromMetadata, R3BaseRefMetaData, compileComponentFromMetadata, compileDirectiveFromMetadata, parseHostBindings} from './render3/view/compiler'; +export {compileBaseDefFromMetadata, R3BaseRefMetaData, compileComponentFromMetadata, compileDirectiveFromMetadata, parseHostBindings, verifyHostBindings} from './render3/view/compiler'; export {publishFacade} from './jit_compiler_facade'; // This file only reexports content of the `src` folder. Keep it that way. diff --git a/packages/compiler/src/compiler_facade_interface.ts b/packages/compiler/src/compiler_facade_interface.ts index 5e659a47af..b375c16592 100644 --- a/packages/compiler/src/compiler_facade_interface.ts +++ b/packages/compiler/src/compiler_facade_interface.ts @@ -38,6 +38,8 @@ export interface CompilerFacade { compileComponent( angularCoreEnv: CoreEnvironment, sourceMapUrl: string, meta: R3ComponentMetadataFacade): any; + createParseSourceSpan(kind: string, typeName: string, sourceUrl: string): ParseSourceSpan; + R3ResolvedDependencyType: typeof R3ResolvedDependencyType; } @@ -109,7 +111,7 @@ export interface R3DirectiveMetadataFacade { name: string; type: any; typeArgumentCount: number; - typeSourceSpan: null; + typeSourceSpan: ParseSourceSpan; deps: R3DependencyMetadataFacade[]|null; selector: string|null; queries: R3QueryMetadataFacade[]; @@ -148,3 +150,9 @@ export interface R3QueryMetadataFacade { descendants: boolean; read: any|null; } + +export interface ParseSourceSpan { + start: any; + end: any; + details: any; +} \ No newline at end of file diff --git a/packages/compiler/src/jit_compiler_facade.ts b/packages/compiler/src/jit_compiler_facade.ts index c79ba07621..d7470f4baa 100644 --- a/packages/compiler/src/jit_compiler_facade.ts +++ b/packages/compiler/src/jit_compiler_facade.ts @@ -13,13 +13,14 @@ import {HostBinding, HostListener, Input, Output, Type} from './core'; import {compileInjectable} from './injectable_compiler_2'; import {DEFAULT_INTERPOLATION_CONFIG, InterpolationConfig} from './ml_parser/interpolation_config'; import {Expression, LiteralExpr, WrappedNodeExpr} from './output/output_ast'; +import {ParseError, ParseSourceSpan, r3JitTypeSourceSpan} from './parse_util'; import {R3DependencyMetadata, R3ResolvedDependencyType} from './render3/r3_factory'; import {jitExpression} from './render3/r3_jit'; import {R3InjectorMetadata, R3NgModuleMetadata, compileInjector, compileNgModule} from './render3/r3_module_compiler'; import {compilePipeFromMetadata} from './render3/r3_pipe_compiler'; import {R3Reference} from './render3/util'; import {R3DirectiveMetadata, R3QueryMetadata} from './render3/view/api'; -import {compileComponentFromMetadata, compileDirectiveFromMetadata, parseHostBindings} from './render3/view/compiler'; +import {compileComponentFromMetadata, compileDirectiveFromMetadata, parseHostBindings, verifyHostBindings} from './render3/view/compiler'; import {makeBindingParser, parseTemplate} from './render3/view/template'; import {DomElementSchemaRegistry} from './schema/dom_element_schema_registry'; @@ -142,6 +143,10 @@ export class CompilerFacadeImpl implements CompilerFacade { return jitExpression(res.expression, angularCoreEnv, sourceMapUrl, preStatements); } + + createParseSourceSpan(kind: string, typeName: string, sourceUrl: string): ParseSourceSpan { + return r3JitTypeSourceSpan(kind, typeName, sourceUrl); + } } // This seems to be needed to placate TS v3.0 only @@ -189,10 +194,10 @@ function convertDirectiveFacadeToMetadata(facade: R3DirectiveMetadataFacade): R3 return { ...facade as R3DirectiveMetadataFacadeNoPropAndWhitespace, - typeSourceSpan: null !, + typeSourceSpan: facade.typeSourceSpan, type: new WrappedNodeExpr(facade.type), deps: convertR3DependencyMetadataArray(facade.deps), - host: extractHostBindings(facade.host, facade.propMetadata), + host: extractHostBindings(facade.host, facade.propMetadata, facade.typeSourceSpan), inputs: {...inputsFromMetadata, ...inputsFromType}, outputs: {...outputsFromMetadata, ...outputsFromType}, queries: facade.queries.map(convertToR3QueryMetadata), @@ -244,28 +249,36 @@ function convertR3DependencyMetadataArray(facades: R3DependencyMetadataFacade[] return facades == null ? null : facades.map(convertR3DependencyMetadata); } -function extractHostBindings(host: {[key: string]: string}, propMetadata: {[key: string]: any[]}): { +function extractHostBindings( + host: {[key: string]: string}, propMetadata: {[key: string]: any[]}, + sourceSpan: ParseSourceSpan): { attributes: StringMap, listeners: StringMap, properties: StringMap, } { // First parse the declarations from the metadata. - const {attributes, listeners, properties} = parseHostBindings(host || {}); + const bindings = parseHostBindings(host || {}); + + // After that check host bindings for errors + const errors = verifyHostBindings(bindings, sourceSpan); + if (errors.length) { + throw new Error(errors.map((error: ParseError) => error.msg).join('\n')); + } // Next, loop over the properties of the object, looking for @HostBinding and @HostListener. for (const field in propMetadata) { if (propMetadata.hasOwnProperty(field)) { propMetadata[field].forEach(ann => { if (isHostBinding(ann)) { - properties[ann.hostPropertyName || field] = field; + bindings.properties[ann.hostPropertyName || field] = field; } else if (isHostListener(ann)) { - listeners[ann.eventName || field] = `${field}(${(ann.args || []).join(',')})`; + bindings.listeners[ann.eventName || field] = `${field}(${(ann.args || []).join(',')})`; } }); } } - return {attributes, listeners, properties}; + return bindings; } function isHostBinding(value: any): value is HostBinding { diff --git a/packages/compiler/src/parse_util.ts b/packages/compiler/src/parse_util.ts index 12dd1fb4e5..4e1fc7d5c5 100644 --- a/packages/compiler/src/parse_util.ts +++ b/packages/compiler/src/parse_util.ts @@ -139,3 +139,19 @@ export function typeSourceSpan(kind: string, type: CompileIdentifierMetadata): P return new ParseSourceSpan( new ParseLocation(sourceFile, -1, -1, -1), new ParseLocation(sourceFile, -1, -1, -1)); } + +/** + * Generates Source Span object for a given R3 Type for JIT mode. + * + * @param kind Component or Directive. + * @param typeName name of the Component or Directive. + * @param sourceUrl reference to Component or Directive source. + * @returns instance of ParseSourceSpan that represent a given Component or Directive. + */ +export function r3JitTypeSourceSpan( + kind: string, typeName: string, sourceUrl: string): ParseSourceSpan { + const sourceFileName = `in ${kind} ${typeName} in ${sourceUrl}`; + const sourceFile = new ParseSourceFile('', sourceFileName); + return new ParseSourceSpan( + new ParseLocation(sourceFile, -1, -1, -1), new ParseLocation(sourceFile, -1, -1, -1)); +} \ No newline at end of file diff --git a/packages/compiler/src/render3/view/compiler.ts b/packages/compiler/src/render3/view/compiler.ts index 7801a3ff61..b6e97bc134 100644 --- a/packages/compiler/src/render3/view/compiler.ts +++ b/packages/compiler/src/render3/view/compiler.ts @@ -16,7 +16,7 @@ import {AST, ParsedEvent, ParsedEventType, ParsedProperty} from '../../expressio import {LifecycleHooks} from '../../lifecycle_reflector'; import {DEFAULT_INTERPOLATION_CONFIG} from '../../ml_parser/interpolation_config'; import * as o from '../../output/output_ast'; -import {typeSourceSpan} from '../../parse_util'; +import {ParseError, ParseSourceSpan, typeSourceSpan} from '../../parse_util'; import {CssSelector, SelectorMatcher} from '../../selector'; import {ShadowCss} from '../../shadow_css'; import {CONTENT_ATTR, HOST_ATTR} from '../../style_compiler'; @@ -30,7 +30,7 @@ import {prepareSyntheticListenerFunctionName, prepareSyntheticPropertyName, type import {R3ComponentDef, R3ComponentMetadata, R3DirectiveDef, R3DirectiveMetadata, R3QueryMetadata} from './api'; import {Instruction, StylingBuilder} from './styling_builder'; -import {BindingScope, TemplateDefinitionBuilder, ValueConverter, prepareEventListenerParameters, renderFlagCheckIfStmt, resolveSanitizationFn} from './template'; +import {BindingScope, TemplateDefinitionBuilder, ValueConverter, makeBindingParser, prepareEventListenerParameters, renderFlagCheckIfStmt, resolveSanitizationFn} from './template'; import {CONTEXT_NAME, DefinitionMap, RENDER_FLAGS, TEMPORARY_NAME, asLiteral, conditionallyCreateMapObjectLiteral, getQueryPredicate, temporaryAllocator} from './util'; const EMPTY_ARRAY: any[] = []; @@ -865,11 +865,15 @@ const enum HostBindingGroup { Event = 2, } -export function parseHostBindings(host: {[key: string]: string}): { - attributes: {[key: string]: string}, - listeners: {[key: string]: string}, - properties: {[key: string]: string}, -} { +// Defines Host Bindings structure that contains attributes, listeners, and properties, +// parsed from the `host` object defined for a Type. +export interface ParsedHostBindings { + attributes: {[key: string]: string}; + listeners: {[key: string]: string}; + properties: {[key: string]: string}; +} + +export function parseHostBindings(host: {[key: string]: string}): ParsedHostBindings { const attributes: {[key: string]: string} = {}; const listeners: {[key: string]: string} = {}; const properties: {[key: string]: string} = {}; @@ -892,6 +896,25 @@ export function parseHostBindings(host: {[key: string]: string}): { return {attributes, listeners, properties}; } +/** + * Verifies host bindings and returns the list of errors (if any). Empty array indicates that a + * given set of host bindings has no errors. + * + * @param bindings set of host bindings to verify. + * @param sourceSpan source span where host bindings were defined. + * @returns array of errors associated with a given set of host bindings. + */ +export function verifyHostBindings( + bindings: ParsedHostBindings, sourceSpan: ParseSourceSpan): ParseError[] { + const summary = metadataAsSummary({ host: bindings } as any); + // TODO: abstract out host bindings verification logic and use it instead of + // creating events and properties ASTs to detect errors (FW-996) + const bindingParser = makeBindingParser(); + bindingParser.createDirectiveHostEventAsts(summary, sourceSpan); + bindingParser.createBoundHostProperties(summary, sourceSpan); + return bindingParser.errors; +} + function compileStyles(styles: string[], selector: string, hostSelector: string): string[] { const shadowCss = new ShadowCss(); return styles.map(style => { return shadowCss !.shimCssText(style, selector, hostSelector); }); diff --git a/packages/core/src/compiler/compiler_facade_interface.ts b/packages/core/src/compiler/compiler_facade_interface.ts index 8c424454b3..d921bc85ae 100644 --- a/packages/core/src/compiler/compiler_facade_interface.ts +++ b/packages/core/src/compiler/compiler_facade_interface.ts @@ -38,6 +38,8 @@ export interface CompilerFacade { compileComponent( angularCoreEnv: CoreEnvironment, sourceMapUrl: string, meta: R3ComponentMetadataFacade): any; + createParseSourceSpan(kind: string, typeName: string, sourceUrl: string): ParseSourceSpan; + R3ResolvedDependencyType: typeof R3ResolvedDependencyType; } @@ -109,7 +111,7 @@ export interface R3DirectiveMetadataFacade { name: string; type: any; typeArgumentCount: number; - typeSourceSpan: null; + typeSourceSpan: ParseSourceSpan; deps: R3DependencyMetadataFacade[]|null; selector: string|null; queries: R3QueryMetadataFacade[]; @@ -148,3 +150,9 @@ export interface R3QueryMetadataFacade { descendants: boolean; read: any|null; } + +export interface ParseSourceSpan { + start: any; + end: any; + details: any; +} \ No newline at end of file diff --git a/packages/core/src/render3/jit/directive.ts b/packages/core/src/render3/jit/directive.ts index 375e232205..d81f138411 100644 --- a/packages/core/src/render3/jit/directive.ts +++ b/packages/core/src/render3/jit/directive.ts @@ -54,8 +54,11 @@ export function compileComponent(type: Type, metadata: Component): void { throw new Error(error.join('\n')); } + const sourceMapUrl = `ng://${renderStringify(type)}/template.html`; const meta: R3ComponentMetadataFacade = { ...directiveMetadata(type, metadata), + typeSourceSpan: + compiler.createParseSourceSpan('Component', renderStringify(type), sourceMapUrl), template: metadata.template || '', preserveWhitespaces: metadata.preserveWhitespaces || false, styles: metadata.styles || EMPTY_ARRAY, @@ -68,8 +71,7 @@ export function compileComponent(type: Type, metadata: Component): void { interpolation: metadata.interpolation, viewProviders: metadata.viewProviders || null, }; - ngComponentDef = compiler.compileComponent( - angularCoreEnv, `ng://${renderStringify(type)}/template.html`, meta); + ngComponentDef = compiler.compileComponent(angularCoreEnv, sourceMapUrl, meta); // When NgModule decorator executed, we enqueued the module definition such that // it would only dequeue and add itself as module scope to all of its declarations, @@ -111,9 +113,13 @@ export function compileDirective(type: Type, directive: Directive): void { Object.defineProperty(type, NG_DIRECTIVE_DEF, { get: () => { if (ngDirectiveDef === null) { + const name = type && type.name; + const sourceMapUrl = `ng://${name}/ngDirectiveDef.js`; + const compiler = getCompilerFacade(); const facade = directiveMetadata(type as ComponentType, directive); - ngDirectiveDef = getCompilerFacade().compileDirective( - angularCoreEnv, `ng://${type && type.name}/ngDirectiveDef.js`, facade); + facade.typeSourceSpan = + compiler.createParseSourceSpan('Directive', renderStringify(type), sourceMapUrl); + ngDirectiveDef = compiler.compileDirective(angularCoreEnv, sourceMapUrl, facade); } return ngDirectiveDef; }, diff --git a/packages/core/test/linker/integration_spec.ts b/packages/core/test/linker/integration_spec.ts index 9205e85b3e..612402757e 100644 --- a/packages/core/test/linker/integration_spec.ts +++ b/packages/core/test/linker/integration_spec.ts @@ -943,18 +943,17 @@ function declareTests(config?: {useJit: boolean}) { expect(tc.properties['title']).toBe(undefined); }); - fixmeIvy('FW-725: Pipes in host bindings fail with a cryptic error') - .it('should not allow pipes in hostProperties', () => { - @Directive({selector: '[host-properties]', host: {'[id]': 'id | uppercase'}}) - class DirectiveWithHostProps { - } + it('should not allow pipes in hostProperties', () => { + @Directive({selector: '[host-properties]', host: {'[id]': 'id | uppercase'}}) + class DirectiveWithHostProps { + } - TestBed.configureTestingModule({declarations: [MyComp, DirectiveWithHostProps]}); - const template = '
'; - TestBed.overrideComponent(MyComp, {set: {template}}); - expect(() => TestBed.createComponent(MyComp)) - .toThrowError(/Host binding expression cannot contain pipes/); - }); + TestBed.configureTestingModule({declarations: [MyComp, DirectiveWithHostProps]}); + const template = '
'; + TestBed.overrideComponent(MyComp, {set: {template}}); + expect(() => TestBed.createComponent(MyComp)) + .toThrowError(/Host binding expression cannot contain pipes/); + }); it('should not use template variables for expressions in hostListeners', () => { @Directive({selector: '[host-listener]', host: {'(click)': 'doIt(id, unknownProp)'}}) @@ -979,18 +978,17 @@ function declareTests(config?: {useJit: boolean}) { expect(dir.receivedArgs).toEqual(['one', undefined]); }); - fixmeIvy('FW-742: Pipes in host listeners should throw a descriptive error') - .it('should not allow pipes in hostListeners', () => { - @Directive({selector: '[host-listener]', host: {'(click)': 'doIt() | somePipe'}}) - class DirectiveWithHostListener { - } + it('should not allow pipes in hostListeners', () => { + @Directive({selector: '[host-listener]', host: {'(click)': 'doIt() | somePipe'}}) + class DirectiveWithHostListener { + } - TestBed.configureTestingModule({declarations: [MyComp, DirectiveWithHostListener]}); - const template = '
'; - TestBed.overrideComponent(MyComp, {set: {template}}); - expect(() => TestBed.createComponent(MyComp)) - .toThrowError(/Cannot have a pipe in an action expression/); - }); + TestBed.configureTestingModule({declarations: [MyComp, DirectiveWithHostListener]}); + const template = '
'; + TestBed.overrideComponent(MyComp, {set: {template}}); + expect(() => TestBed.createComponent(MyComp)) + .toThrowError(/Cannot have a pipe in an action expression/); + });