Skip to content

Commit 37af6ab

Browse files
JoostKdylhunn
JoostK
authored and
dylhunn
committed
fix(compiler): allow banana-in-a-box bindings to end with non-null assertion (#37809)
For two-way-bindings that use the banana-in-a-box syntax, the compiler synthesizes an event assignment expression from the primary expression. It is valid for the primary expression to be terminated by the non-null operator, however naive string substitution is used for the synthesized expression, such that the `!` would immediately precede the `=` token, resulting in the valid `!=` operator token. The expression would still parse correctly but it doesn't implement the proper semantics, resulting in incorrect runtime behavior. Changing the expression substitution to force a space between the primary expression and the assignment avoids this mistake, but it uncovers a new issue. The grammar does not allow for the LHS of an assignment to be the non-null operator, so the synthesized expression would fail to parse. To alleviate this, the synthesized expression is parsed with a special parser flag to allow for this syntax. Fixes #36551 PR Close #37809
1 parent 604a67f commit 37af6ab

File tree

5 files changed

+99
-36
lines changed

5 files changed

+99
-36
lines changed

‎packages/compiler/src/expression_parser/parser.ts‎

Lines changed: 56 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -29,19 +29,41 @@ export class TemplateBindingParseResult {
2929
public errors: ParserError[]) {}
3030
}
3131

32+
/**
33+
* Represents the possible parse modes to be used as a bitmask.
34+
*/
35+
export const enum ParseFlags {
36+
None = 0,
37+
38+
/**
39+
* Whether an output binding is being parsed.
40+
*/
41+
Action = 1 << 0,
42+
43+
/**
44+
* Whether an assignment event is being parsed, i.e. an expression originating from
45+
* two-way-binding aka banana-in-a-box syntax.
46+
*/
47+
AssignmentEvent = 1 << 1,
48+
}
49+
3250
export class Parser {
3351
private errors: ParserError[] = [];
3452

3553
constructor(private _lexer: Lexer) {}
3654

3755
parseAction(
38-
input: string, location: string, absoluteOffset: number,
56+
input: string, isAssignmentEvent: boolean, location: string, absoluteOffset: number,
3957
interpolationConfig: InterpolationConfig = DEFAULT_INTERPOLATION_CONFIG): ASTWithSource {
4058
this._checkNoInterpolation(input, location, interpolationConfig);
4159
const sourceToLex = this._stripComments(input);
4260
const tokens = this._lexer.tokenize(sourceToLex);
61+
let flags = ParseFlags.Action;
62+
if (isAssignmentEvent) {
63+
flags |= ParseFlags.AssignmentEvent;
64+
}
4365
const ast =
44-
new _ParseAST(input, location, absoluteOffset, tokens, true, this.errors, 0).parseChain();
66+
new _ParseAST(input, location, absoluteOffset, tokens, flags, this.errors, 0).parseChain();
4567
return new ASTWithSource(ast, input, location, absoluteOffset, this.errors);
4668
}
4769

@@ -88,7 +110,7 @@ export class Parser {
88110
this._checkNoInterpolation(input, location, interpolationConfig);
89111
const sourceToLex = this._stripComments(input);
90112
const tokens = this._lexer.tokenize(sourceToLex);
91-
return new _ParseAST(input, location, absoluteOffset, tokens, false, this.errors, 0)
113+
return new _ParseAST(input, location, absoluteOffset, tokens, ParseFlags.None, this.errors, 0)
92114
.parseChain();
93115
}
94116

@@ -135,8 +157,8 @@ export class Parser {
135157
absoluteValueOffset: number): TemplateBindingParseResult {
136158
const tokens = this._lexer.tokenize(templateValue);
137159
const parser = new _ParseAST(
138-
templateValue, templateUrl, absoluteValueOffset, tokens, false /* parseAction */,
139-
this.errors, 0 /* relative offset */);
160+
templateValue, templateUrl, absoluteValueOffset, tokens, ParseFlags.None, this.errors,
161+
0 /* relative offset */);
140162
return parser.parseTemplateBindings({
141163
source: templateKey,
142164
span: new AbsoluteSourceSpan(absoluteKeyOffset, absoluteKeyOffset + templateKey.length),
@@ -157,7 +179,8 @@ export class Parser {
157179
const sourceToLex = this._stripComments(expressionText);
158180
const tokens = this._lexer.tokenize(sourceToLex);
159181
const ast =
160-
new _ParseAST(input, location, absoluteOffset, tokens, false, this.errors, offsets[i])
182+
new _ParseAST(
183+
input, location, absoluteOffset, tokens, ParseFlags.None, this.errors, offsets[i])
161184
.parseChain();
162185
expressionNodes.push(ast);
163186
}
@@ -175,10 +198,9 @@ export class Parser {
175198
ASTWithSource {
176199
const sourceToLex = this._stripComments(expression);
177200
const tokens = this._lexer.tokenize(sourceToLex);
178-
const ast = new _ParseAST(
179-
expression, location, absoluteOffset, tokens,
180-
/* parseAction */ false, this.errors, 0)
181-
.parseChain();
201+
const ast =
202+
new _ParseAST(expression, location, absoluteOffset, tokens, ParseFlags.None, this.errors, 0)
203+
.parseChain();
182204
const strings = ['', '']; // The prefix and suffix strings are both empty
183205
return this.createInterpolationAst(strings, [ast], expression, location, absoluteOffset);
184206
}
@@ -388,7 +410,7 @@ export class _ParseAST {
388410

389411
constructor(
390412
public input: string, public location: string, public absoluteOffset: number,
391-
public tokens: Token[], public parseAction: boolean, private errors: ParserError[],
413+
public tokens: Token[], public parseFlags: ParseFlags, private errors: ParserError[],
392414
private offset: number) {}
393415

394416
peek(offset: number): Token {
@@ -571,7 +593,7 @@ export class _ParseAST {
571593
exprs.push(expr);
572594

573595
if (this.consumeOptionalCharacter(chars.$SEMICOLON)) {
574-
if (!this.parseAction) {
596+
if (!(this.parseFlags & ParseFlags.Action)) {
575597
this.error('Binding expression cannot contain chained expression');
576598
}
577599
while (this.consumeOptionalCharacter(chars.$SEMICOLON)) {
@@ -596,7 +618,7 @@ export class _ParseAST {
596618
const start = this.inputIndex;
597619
let result = this.parseExpression();
598620
if (this.consumeOptionalOperator('|')) {
599-
if (this.parseAction) {
621+
if (this.parseFlags & ParseFlags.Action) {
600622
this.error('Cannot have a pipe in an action expression');
601623
}
602624

@@ -952,16 +974,16 @@ export class _ParseAST {
952974
let receiver: AST;
953975

954976
if (isSafe) {
955-
if (this.consumeOptionalOperator('=')) {
977+
if (this.consumeOptionalAssignment()) {
956978
this.error('The \'?.\' operator cannot be used in the assignment');
957979
receiver = new EmptyExpr(this.span(start), this.sourceSpan(start));
958980
} else {
959981
receiver = new SafePropertyRead(
960982
this.span(start), this.sourceSpan(start), nameSpan, readReceiver, id);
961983
}
962984
} else {
963-
if (this.consumeOptionalOperator('=')) {
964-
if (!this.parseAction) {
985+
if (this.consumeOptionalAssignment()) {
986+
if (!(this.parseFlags & ParseFlags.Action)) {
965987
this.error('Bindings cannot contain assignments');
966988
return new EmptyExpr(this.span(start), this.sourceSpan(start));
967989
}
@@ -991,6 +1013,24 @@ export class _ParseAST {
9911013
new Call(span, sourceSpan, receiver, args, argumentSpan);
9921014
}
9931015

1016+
private consumeOptionalAssignment(): boolean {
1017+
// When parsing assignment events (originating from two-way-binding aka banana-in-a-box syntax),
1018+
// it is valid for the primary expression to be terminated by the non-null operator. This
1019+
// primary expression is substituted as LHS of the assignment operator to achieve
1020+
// two-way-binding, such that the LHS could be the non-null operator. The grammar doesn't
1021+
// naturally allow for this syntax, so assignment events are parsed specially.
1022+
if ((this.parseFlags & ParseFlags.AssignmentEvent) && this.next.isOperator('!') &&
1023+
this.peek(1).isOperator('=')) {
1024+
// First skip over the ! operator.
1025+
this.advance();
1026+
// Then skip over the = operator, to fully consume the optional assignment operator.
1027+
this.advance();
1028+
return true;
1029+
}
1030+
1031+
return this.consumeOptionalOperator('=');
1032+
}
1033+
9941034
parseCallArguments(): BindingPipe[] {
9951035
if (this.next.isCharacter(chars.$RPAREN)) return [];
9961036
const positionals: AST[] = [];

‎packages/compiler/src/render3/r3_template_transform.ts‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -379,8 +379,8 @@ class HtmlAstToIvyAst implements html.Visitor {
379379
const identifier = bindParts[IDENT_KW_IDX];
380380
const keySpan = createKeySpan(srcSpan, bindParts[KW_ON_IDX], identifier);
381381
this.bindingParser.parseEvent(
382-
identifier, value, srcSpan, attribute.valueSpan || srcSpan, matchableAttributes, events,
383-
keySpan);
382+
identifier, value, /* isAssignmentEvent */ false, srcSpan,
383+
attribute.valueSpan || srcSpan, matchableAttributes, events, keySpan);
384384
addEvents(events, boundEvents);
385385
} else if (bindParts[KW_BINDON_IDX]) {
386386
const identifier = bindParts[IDENT_KW_IDX];
@@ -432,8 +432,8 @@ class HtmlAstToIvyAst implements html.Visitor {
432432
} else {
433433
const events: ParsedEvent[] = [];
434434
this.bindingParser.parseEvent(
435-
identifier, value, srcSpan, attribute.valueSpan || srcSpan, matchableAttributes, events,
436-
keySpan);
435+
identifier, value, /* isAssignmentEvent */ false, srcSpan,
436+
attribute.valueSpan || srcSpan, matchableAttributes, events, keySpan);
437437
addEvents(events, boundEvents);
438438
}
439439

@@ -486,8 +486,8 @@ class HtmlAstToIvyAst implements html.Visitor {
486486
boundEvents: t.BoundEvent[], keySpan: ParseSourceSpan) {
487487
const events: ParsedEvent[] = [];
488488
this.bindingParser.parseEvent(
489-
`${name}Change`, `${expression}=$event`, sourceSpan, valueSpan || sourceSpan,
490-
targetMatchableAttrs, events, keySpan);
489+
`${name}Change`, `${expression} =$event`, /* isAssignmentEvent */ true, sourceSpan,
490+
valueSpan || sourceSpan, targetMatchableAttrs, events, keySpan);
491491
addEvents(events, boundEvents);
492492
}
493493

‎packages/compiler/src/template_parser/binding_parser.ts‎

Lines changed: 20 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,9 @@ export class BindingParser {
8181
// Regardless, neither of these values are used in Ivy but are only here to satisfy the
8282
// function signature. This should likely be refactored in the future so that `sourceSpan`
8383
// isn't being used inaccurately.
84-
this.parseEvent(propName, expression, sourceSpan, sourceSpan, [], targetEvents, sourceSpan);
84+
this.parseEvent(
85+
propName, expression, /* isAssignmentEvent */ false, sourceSpan, sourceSpan, [],
86+
targetEvents, sourceSpan);
8587
} else {
8688
this._reportError(
8789
`Value of the host listener "${
@@ -393,8 +395,9 @@ export class BindingParser {
393395

394396
// TODO: keySpan should be required but was made optional to avoid changing VE parser.
395397
parseEvent(
396-
name: string, expression: string, sourceSpan: ParseSourceSpan, handlerSpan: ParseSourceSpan,
397-
targetMatchableAttrs: string[][], targetEvents: ParsedEvent[], keySpan: ParseSourceSpan) {
398+
name: string, expression: string, isAssignmentEvent: boolean, sourceSpan: ParseSourceSpan,
399+
handlerSpan: ParseSourceSpan, targetMatchableAttrs: string[][], targetEvents: ParsedEvent[],
400+
keySpan: ParseSourceSpan) {
398401
if (name.length === 0) {
399402
this._reportError(`Event name is missing in binding`, sourceSpan);
400403
}
@@ -405,10 +408,12 @@ export class BindingParser {
405408
keySpan = moveParseSourceSpan(
406409
keySpan, new AbsoluteSourceSpan(keySpan.start.offset + 1, keySpan.end.offset));
407410
}
408-
this._parseAnimationEvent(name, expression, sourceSpan, handlerSpan, targetEvents, keySpan);
411+
this._parseAnimationEvent(
412+
name, expression, isAssignmentEvent, sourceSpan, handlerSpan, targetEvents, keySpan);
409413
} else {
410414
this._parseRegularEvent(
411-
name, expression, sourceSpan, handlerSpan, targetMatchableAttrs, targetEvents, keySpan);
415+
name, expression, isAssignmentEvent, sourceSpan, handlerSpan, targetMatchableAttrs,
416+
targetEvents, keySpan);
412417
}
413418
}
414419

@@ -419,12 +424,12 @@ export class BindingParser {
419424
}
420425

421426
private _parseAnimationEvent(
422-
name: string, expression: string, sourceSpan: ParseSourceSpan, handlerSpan: ParseSourceSpan,
423-
targetEvents: ParsedEvent[], keySpan: ParseSourceSpan) {
427+
name: string, expression: string, isAssignmentEvent: boolean, sourceSpan: ParseSourceSpan,
428+
handlerSpan: ParseSourceSpan, targetEvents: ParsedEvent[], keySpan: ParseSourceSpan) {
424429
const matches = splitAtPeriod(name, [name, '']);
425430
const eventName = matches[0];
426431
const phase = matches[1].toLowerCase();
427-
const ast = this._parseAction(expression, handlerSpan);
432+
const ast = this._parseAction(expression, isAssignmentEvent, handlerSpan);
428433
targetEvents.push(new ParsedEvent(
429434
eventName, phase, ParsedEventType.Animation, ast, sourceSpan, handlerSpan, keySpan));
430435

@@ -447,25 +452,27 @@ export class BindingParser {
447452
}
448453

449454
private _parseRegularEvent(
450-
name: string, expression: string, sourceSpan: ParseSourceSpan, handlerSpan: ParseSourceSpan,
451-
targetMatchableAttrs: string[][], targetEvents: ParsedEvent[], keySpan: ParseSourceSpan) {
455+
name: string, expression: string, isAssignmentEvent: boolean, sourceSpan: ParseSourceSpan,
456+
handlerSpan: ParseSourceSpan, targetMatchableAttrs: string[][], targetEvents: ParsedEvent[],
457+
keySpan: ParseSourceSpan) {
452458
// long format: 'target: eventName'
453459
const [target, eventName] = splitAtColon(name, [null!, name]);
454-
const ast = this._parseAction(expression, handlerSpan);
460+
const ast = this._parseAction(expression, isAssignmentEvent, handlerSpan);
455461
targetMatchableAttrs.push([name!, ast.source!]);
456462
targetEvents.push(new ParsedEvent(
457463
eventName, target, ParsedEventType.Regular, ast, sourceSpan, handlerSpan, keySpan));
458464
// Don't detect directives for event names for now,
459465
// so don't add the event name to the matchableAttrs
460466
}
461467

462-
private _parseAction(value: string, sourceSpan: ParseSourceSpan): ASTWithSource {
468+
private _parseAction(value: string, isAssignmentEvent: boolean, sourceSpan: ParseSourceSpan):
469+
ASTWithSource {
463470
const sourceInfo = (sourceSpan && sourceSpan.start || '(unknown').toString();
464471
const absoluteOffset = (sourceSpan && sourceSpan.start) ? sourceSpan.start.offset : 0;
465472

466473
try {
467474
const ast = this._exprParser.parseAction(
468-
value, sourceInfo, absoluteOffset, this._interpolationConfig);
475+
value, isAssignmentEvent, sourceInfo, absoluteOffset, this._interpolationConfig);
469476
if (ast) {
470477
this._reportExpressionParserErrors(ast.errors, sourceSpan);
471478
}

‎packages/compiler/test/expression_parser/parser_spec.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1180,7 +1180,7 @@ function createParser() {
11801180
}
11811181

11821182
function parseAction(text: string, location: any = null, offset: number = 0): ASTWithSource {
1183-
return createParser().parseAction(text, location, offset);
1183+
return createParser().parseAction(text, /* isAssignmentEvent */ false, location, offset);
11841184
}
11851185

11861186
function parseBinding(text: string, location: any = null, offset: number = 0): ASTWithSource {

‎packages/compiler/test/render3/r3_template_transform_spec.ts‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -421,6 +421,22 @@ describe('R3 template transform', () => {
421421
]);
422422
});
423423

424+
it('should parse bound events and properties via [(...)] with non-null operator', () => {
425+
expectFromHtml('<div [(prop)]="v!"></div>').toEqual([
426+
['Element', 'div'],
427+
['BoundAttribute', BindingType.Property, 'prop', 'v!'],
428+
['BoundEvent', 'propChange', null, 'v = $event'],
429+
]);
430+
});
431+
432+
it('should report an error for assignments into non-null asserted expressions', () => {
433+
// TODO(joost): this syntax is allowed in TypeScript. Consider changing the grammar to
434+
// allow this syntax, or improve the error message.
435+
// See https://github.com/angular/angular/pull/37809
436+
expect(() => parse('<div (prop)="v! = $event"></div>'))
437+
.toThrowError(/Unexpected token '=' at column 4/);
438+
});
439+
424440
it('should report missing property names in bindon- syntax', () => {
425441
expect(() => parse('<div bindon-></div>'))
426442
.toThrowError(/Property name is missing in binding/);

0 commit comments

Comments
 (0)