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/); + });