From a92d97cda7a0f694ad1cb928a909e659a8c9650f Mon Sep 17 00:00:00 2001 From: ayazhafiz Date: Sat, 28 Dec 2019 18:02:09 -0600 Subject: [PATCH] fix(compiler): report errors for missing binding names (#34595) Currently, would-be binding attributes that are missing binding names are not parsed as bindings, and fall through as regular attributes. In some cases, this can lead to a runtime error; trying to assign `#` as a DOM attribute in an element like in `
` fails because `#` is not a valid attribute name. Attributes composed of binding prefixes but not defining a binding should be considered invalid, as this almost certainly indicates an unintentional elision of a binding by the developer. This commit introduces error reporting for attributes with a binding name prefix but no actual binding name. Closes https://github.com/angular/vscode-ng-language-service/issues/293. PR Close #34595 --- .../src/render3/r3_template_transform.ts | 7 +++- .../src/template_parser/binding_parser.ts | 12 ++++++ .../src/template_parser/template_parser.ts | 6 ++- .../render3/r3_template_transform_spec.ts | 39 +++++++++++++++++++ .../template_parser/template_parser_spec.ts | 30 ++++++++++++++ 5 files changed, 92 insertions(+), 2 deletions(-) diff --git a/packages/compiler/src/render3/r3_template_transform.ts b/packages/compiler/src/render3/r3_template_transform.ts index 8739b66fbf..c55364beb9 100644 --- a/packages/compiler/src/render3/r3_template_transform.ts +++ b/packages/compiler/src/render3/r3_template_transform.ts @@ -21,7 +21,7 @@ import * as t from './r3_ast'; import {I18N_ICU_VAR_PREFIX, isI18nRootNode} from './view/i18n/util'; const BIND_NAME_REGEXP = - /^(?:(?:(?:(bind-)|(let-)|(ref-|#)|(on-)|(bindon-)|(@))(.+))|\[\(([^\)]+)\)\]|\[([^\]]+)\]|\(([^\)]+)\))$/; + /^(?:(?:(?:(bind-)|(let-)|(ref-|#)|(on-)|(bindon-)|(@))(.*))|\[\(([^\)]+)\)\]|\[([^\]]+)\]|\(([^\)]+)\))$/; // Group 1 = "bind-" const KW_BIND_IDX = 1; @@ -399,7 +399,10 @@ class HtmlAstToIvyAst implements html.Visitor { valueSpan: ParseSourceSpan|undefined, variables: t.Variable[]) { if (identifier.indexOf('-') > -1) { this.reportError(`"-" is not allowed in variable names`, sourceSpan); + } else if (identifier.length === 0) { + this.reportError(`Variable does not have a name`, sourceSpan); } + variables.push(new t.Variable(identifier, value, sourceSpan, valueSpan)); } @@ -408,6 +411,8 @@ class HtmlAstToIvyAst implements html.Visitor { valueSpan: ParseSourceSpan|undefined, references: t.Reference[]) { if (identifier.indexOf('-') > -1) { this.reportError(`"-" is not allowed in reference names`, sourceSpan); + } else if (identifier.length === 0) { + this.reportError(`Reference does not have a name`, sourceSpan); } references.push(new t.Reference(identifier, value, sourceSpan, valueSpan)); diff --git a/packages/compiler/src/template_parser/binding_parser.ts b/packages/compiler/src/template_parser/binding_parser.ts index f6258db5e3..6ea53eab0e 100644 --- a/packages/compiler/src/template_parser/binding_parser.ts +++ b/packages/compiler/src/template_parser/binding_parser.ts @@ -203,6 +203,10 @@ export class BindingParser { name: string, expression: string, isHost: boolean, sourceSpan: ParseSourceSpan, absoluteOffset: number, valueSpan: ParseSourceSpan|undefined, targetMatchableAttrs: string[][], targetProps: ParsedProperty[]) { + if (name.length === 0) { + this._reportError(`Property name is missing in binding`, sourceSpan); + } + let isAnimationProp = false; if (name.startsWith(ANIMATE_PROP_PREFIX)) { isAnimationProp = true; @@ -248,6 +252,10 @@ export class BindingParser { name: string, expression: string|null, sourceSpan: ParseSourceSpan, absoluteOffset: number, valueSpan: ParseSourceSpan|undefined, targetMatchableAttrs: string[][], targetProps: ParsedProperty[]) { + if (name.length === 0) { + this._reportError('Animation trigger is missing', sourceSpan); + } + // This will occur when a @trigger is not paired with an expression. // For animations it is valid to not have an expression since */void // states will be applied by angular when the element is attached/detached @@ -343,6 +351,10 @@ export class BindingParser { parseEvent( name: string, expression: string, sourceSpan: ParseSourceSpan, handlerSpan: ParseSourceSpan, targetMatchableAttrs: string[][], targetEvents: ParsedEvent[]) { + if (name.length === 0) { + this._reportError(`Event name is missing in binding`, sourceSpan); + } + if (isAnimationLabel(name)) { name = name.substr(1); this._parseAnimationEvent(name, expression, sourceSpan, handlerSpan, targetEvents); diff --git a/packages/compiler/src/template_parser/template_parser.ts b/packages/compiler/src/template_parser/template_parser.ts index 85c35a6f98..453a32b29e 100644 --- a/packages/compiler/src/template_parser/template_parser.ts +++ b/packages/compiler/src/template_parser/template_parser.ts @@ -31,7 +31,7 @@ import * as t from './template_ast'; import {PreparsedElementType, preparseElement} from './template_preparser'; const BIND_NAME_REGEXP = - /^(?:(?:(?:(bind-)|(let-)|(ref-|#)|(on-)|(bindon-)|(@))(.+))|\[\(([^\)]+)\)\]|\[([^\]]+)\]|\(([^\)]+)\))$/; + /^(?:(?:(?:(bind-)|(let-)|(ref-|#)|(on-)|(bindon-)|(@))(.*))|\[\(([^\)]+)\)\]|\[([^\]]+)\]|\(([^\)]+)\))$/; // Group 1 = "bind-" const KW_BIND_IDX = 1; @@ -501,6 +501,8 @@ class TemplateParseVisitor implements html.Visitor { identifier: string, value: string, sourceSpan: ParseSourceSpan, targetVars: t.VariableAst[]) { if (identifier.indexOf('-') > -1) { this._reportError(`"-" is not allowed in variable names`, sourceSpan); + } else if (identifier.length === 0) { + this._reportError(`Variable does not have a name`, sourceSpan); } targetVars.push(new t.VariableAst(identifier, value, sourceSpan)); @@ -511,6 +513,8 @@ class TemplateParseVisitor implements html.Visitor { targetRefs: ElementOrDirectiveRef[]) { if (identifier.indexOf('-') > -1) { this._reportError(`"-" is not allowed in reference names`, sourceSpan); + } else if (identifier.length === 0) { + this._reportError(`Reference does not have a name`, sourceSpan); } targetRefs.push(new ElementOrDirectiveRef(identifier, value, sourceSpan)); diff --git a/packages/compiler/test/render3/r3_template_transform_spec.ts b/packages/compiler/test/render3/r3_template_transform_spec.ts index eff0b5da3f..1bcadadd4c 100644 --- a/packages/compiler/test/render3/r3_template_transform_spec.ts +++ b/packages/compiler/test/render3/r3_template_transform_spec.ts @@ -167,6 +167,10 @@ describe('R3 template transform', () => { ]); }); + it('should report missing property names in bind- syntax', () => { + expect(() => parse('
')).toThrowError(/Property name is missing in binding/); + }); + it('should parse bound properties via {{...}}', () => { expectFromHtml('
').toEqual([ ['Element', 'div'], @@ -339,6 +343,10 @@ describe('R3 template transform', () => { ]); }); + it('should report missing event names in on- syntax', () => { + expect(() => parse('
')).toThrowError(/Event name is missing in binding/); + }); + it('should parse bound events and properties via [(...)]', () => { expectFromHtml('
').toEqual([ ['Element', 'div'], @@ -355,12 +363,29 @@ describe('R3 template transform', () => { ]); }); + it('should report missing property names in bindon- syntax', () => { + expect(() => parse('
')) + .toThrowError(/Property name is missing in binding/); + }); + it('should report an error on empty expression', () => { expect(() => parse('
')).toThrowError(/Empty expressions are not allowed/); expect(() => parse('
')).toThrowError(/Empty expressions are not allowed/); }); }); + describe('variables', () => { + it('should report variables not on template elements', () => { + expect(() => parse('
')) + .toThrowError(/"let-" is only supported on ng-template elements./); + }); + + it('should report missing variable names', () => { + expect(() => parse('')) + .toThrowError(/Variable does not have a name/); + }); + }); + describe('references', () => { it('should parse references via #...', () => { expectFromHtml('
').toEqual([ @@ -382,6 +407,20 @@ describe('R3 template transform', () => { ['Reference', 'someA', ''], ]); }); + + it('should report invalid reference names', () => { + expect(() => parse('
')).toThrowError(/"-" is not allowed in reference names/); + }); + + it('should report missing reference names', () => { + expect(() => parse('
')).toThrowError(/Reference does not have a name/); + }); + }); + + describe('literal attribute', () => { + it('should report missing animation trigger in @ syntax', () => { + expect(() => parse('
')).toThrowError(/Animation trigger is missing/); + }); }); describe('ng-content', () => { diff --git a/packages/compiler/test/template_parser/template_parser_spec.ts b/packages/compiler/test/template_parser/template_parser_spec.ts index 782b7d5733..af03ee7ca7 100644 --- a/packages/compiler/test/template_parser/template_parser_spec.ts +++ b/packages/compiler/test/template_parser/template_parser_spec.ts @@ -658,6 +658,11 @@ Binding to attribute 'onEvent' is disallowed for security reasons (" { + expect(() => parse('
', [])).toThrowError(`Template parse errors: +Property name is missing in binding ("
]bind->
"): TestComp@0:5`); + }); + it('should parse bound properties via {{...}} and not report them as attributes', () => { expect(humanizeTplAst(parse('
', []))).toEqual([ [ElementAst, 'div'], @@ -684,6 +689,11 @@ Binding to attribute 'onEvent' is disallowed for security reasons ("\]@someAnimation="value2">"\): TestComp@0:5/); }); + it('should report missing animation trigger in @ syntax', () => { + expect(() => parse('
', [])).toThrowError(`Template parse errors: +Animation trigger is missing ("
]@>
"): TestComp@0:5`); + }); + it('should not issue a warning when host attributes contain a valid property-bound animation trigger', () => { const animationEntries = ['prop']; @@ -804,6 +814,11 @@ Binding to attribute 'onEvent' is disallowed for security reasons (" { + expect(() => parse('
', [])) + .toThrowError(/Event name is missing in binding/); + }); + it('should allow events on explicit embedded templates that are emitted by a directive', () => { const dirA = @@ -840,6 +855,10 @@ Binding to attribute 'onEvent' is disallowed for security reasons (" { + expect(() => parse('
', [])) + .toThrowError(/Property name is missing in binding/); + }); }); describe('directives', () => { @@ -1372,11 +1391,22 @@ There is no directive with "exportAs" set to "dirA" ("
]#a="dirA">< "-" is not allowed in reference names ("
]#a-b>
"): TestComp@0:5`); }); + it('should report missing reference names', () => { + expect(() => parse('
', [])).toThrowError(`Template parse errors: +Reference does not have a name ("
]#>
"): TestComp@0:5`); + }); + it('should report variables as errors', () => { expect(() => parse('
', [])).toThrowError(`Template parse errors: "let-" is only supported on ng-template elements. ("
]let-a>
"): TestComp@0:5`); }); + it('should report missing variable names', () => { + expect(() => parse('', [])) + .toThrowError(`Template parse errors: +Variable does not have a name ("]let->"): TestComp@0:13`); + }); + it('should report duplicate reference names', () => { expect(() => parse('
', [])) .toThrowError(`Template parse errors: