From b0f3ec43aed25b90228fd9c255593e6c74d008ae Mon Sep 17 00:00:00 2001 From: ayazhafiz Date: Fri, 25 Sep 2020 18:52:06 -0500 Subject: [PATCH 1/5] feat(compiler): Recover on malformed keyed reads and keyed writes This patch adds support for recovering well-formed (and near-complete) ASTs for semantically malformed keyed reads and keyed writes. See the added tests for details on the types of semantics we can now recover; in particular, notice that some assumptions are made about the form of a keyed read/write intended by a user. For example, in the malformed expression `a[1 + = 2`, we assume that the user meant to write a binary expression for the key of `a`, and assign that key the value `2`. In particular, we now parse this as `a[1 + ] = 2`. There are some different interpretations that can be made here, but I think this is reasonable. The actual changes in the parser code are fairly minimal (a nice surprise!); the biggest addition is a `writeContext` that marks whether the `=` operator can serve as a recovery point after error detection. Part of #38596 --- .../compiler/src/expression_parser/parser.ts | 52 +++++++---- .../test/expression_parser/parser_spec.ts | 87 +++++++++++++++++++ 2 files changed, 124 insertions(+), 15 deletions(-) diff --git a/packages/compiler/src/expression_parser/parser.ts b/packages/compiler/src/expression_parser/parser.ts index 090e236135184..33efb94ffda73 100644 --- a/packages/compiler/src/expression_parser/parser.ts +++ b/packages/compiler/src/expression_parser/parser.ts @@ -292,6 +292,7 @@ export class _ParseAST { private rparensExpected = 0; private rbracketsExpected = 0; private rbracesExpected = 0; + private writeContext = false; // Cache of expression start and input indeces to the absolute source span they map to, used to // prevent creating superfluous source spans in `sourceSpan`. @@ -384,6 +385,12 @@ export class _ParseAST { return this.next.isKeywordAs(); } + /** + * Consumes an expected character, otherwise emits an error about the missing expected character + * and skips over the token stream until reaching a recoverable point. + * + * See `this.error` and `this.skip` for more details. + */ expectCharacter(code: number) { if (this.consumeOptionalCharacter(code)) return; this.error(`Missing expected ${String.fromCharCode(code)}`); @@ -632,7 +639,11 @@ export class _ParseAST { } else if (this.consumeOptionalCharacter(chars.$LBRACKET)) { this.rbracketsExpected++; + this.writeContext = true; const key = this.parsePipe(); + if (key instanceof EmptyExpr) { + this.error(`Key access cannot be empty`) + } this.rbracketsExpected--; this.expectCharacter(chars.$RBRACKET); if (this.consumeOptionalOperator('=')) { @@ -642,7 +653,7 @@ export class _ParseAST { } else { result = new KeyedRead(this.span(resultStart), this.sourceSpan(resultStart), result, key); } - + this.writeContext = false; } else if (this.consumeOptionalCharacter(chars.$LPAREN)) { this.rparensExpected++; const args = this.parseCallArguments(); @@ -994,6 +1005,10 @@ export class _ParseAST { this.consumeOptionalCharacter(chars.$SEMICOLON) || this.consumeOptionalCharacter(chars.$COMMA); } + /** + * Records an error and skips over the token stream until reaching a recoverable point. See + * `this.skip` for more details on token skipping. + */ error(message: string, index: number|null = null) { this.errors.push(new ParserError(message, this.input, this.locationText(index), this.location)); this.skip(); @@ -1005,25 +1020,32 @@ export class _ParseAST { `at the end of the expression`; } - // Error recovery should skip tokens until it encounters a recovery point. skip() treats - // the end of input and a ';' as unconditionally a recovery point. It also treats ')', - // '}' and ']' as conditional recovery points if one of calling productions is expecting - // one of these symbols. This allows skip() to recover from errors such as '(a.) + 1' allowing - // more of the AST to be retained (it doesn't skip any tokens as the ')' is retained because - // of the '(' begins an '(' ')' production). The recovery points of grouping symbols - // must be conditional as they must be skipped if none of the calling productions are not - // expecting the closing token else we will never make progress in the case of an - // extraneous group closing symbol (such as a stray ')'). This is not the case for ';' because - // parseChain() is always the root production and it expects a ';'. - - // If a production expects one of these token it increments the corresponding nesting count, - // and then decrements it just prior to checking if the token is in the input. + /** + * Error recovery should skip tokens until it encounters a recovery point. skip() treats + * the end of input and a ';' as unconditionally a recovery point. It also treats ')', + * '}' and ']' as conditional recovery points if one of calling productions is expecting + * one of these symbols. This allows skip() to recover from errors such as '(a.) + 1' allowing + * more of the AST to be retained (it doesn't skip any tokens as the ')' is retained because + * of the '(' begins an '(' ')' production). The recovery points of grouping symbols + * must be conditional as they must be skipped if none of the calling productions are not + * expecting the closing token else we will never make progress in the case of an + * extraneous group closing symbol (such as a stray ')'). This is not the case for ';' because + * parseChain() is always the root production and it expects a ';'. + * + * Furthermore, the presence of a stateful context can add more recovery points. + * - in a `writeContext`, we are able to recover after seeing the `=` operator, which signals + * the presence of an independent rvalue expression following the `=` operator. + * + * If a production expects one of these token it increments the corresponding nesting count, + * and then decrements it just prior to checking if the token is in the input. + */ private skip() { let n = this.next; while (this.index < this.tokens.length && !n.isCharacter(chars.$SEMICOLON) && (this.rparensExpected <= 0 || !n.isCharacter(chars.$RPAREN)) && (this.rbracesExpected <= 0 || !n.isCharacter(chars.$RBRACE)) && - (this.rbracketsExpected <= 0 || !n.isCharacter(chars.$RBRACKET))) { + (this.rbracketsExpected <= 0 || !n.isCharacter(chars.$RBRACKET)) && + !(this.writeContext && n.isOperator('='))) { if (this.next.isError()) { this.errors.push( new ParserError(this.next.toString()!, this.input, this.locationText(), this.location)); diff --git a/packages/compiler/test/expression_parser/parser_spec.ts b/packages/compiler/test/expression_parser/parser_spec.ts index bbc58d16f7ae1..8c9f85651eb0e 100644 --- a/packages/compiler/test/expression_parser/parser_spec.ts +++ b/packages/compiler/test/expression_parser/parser_spec.ts @@ -154,6 +154,84 @@ describe('parser', () => { }); }); + describe('keyed read', () => { + it('should parse keyed reads', () => { + checkAction('a["a"]'); + checkAction('this.a["a"]', 'a["a"]'); + checkAction('a.a["a"]'); + }); + + describe('malformed keyed reads', () => { + it('should recover on missing keys', () => { + checkActionWithError('a[]', 'a[]', 'Key access cannot be empty'); + }); + + it('should recover on incomplete expression keys', () => { + checkActionWithError('a[1 + ]', 'a[1 + ]', 'Unexpected token ]'); + }); + + it('should recover on unterminated keys', () => { + checkActionWithError( + 'a[1 + 2', 'a[1 + 2]', 'Missing expected ] at the end of the expression'); + }); + + it('should recover on incomplete and unterminated keys', () => { + checkActionWithError( + 'a[1 + ', 'a[1 + ]', 'Missing expected ] at the end of the expression'); + }); + }); + }); + + describe('keyed write', () => { + it('should parse keyed writes', () => { + checkAction('a["a"] = 1 + 2'); + checkAction('this.a["a"] = 1 + 2', 'a["a"] = 1 + 2'); + checkAction('a.a["a"] = 1 + 2'); + }); + + describe('malformed keyed writes', () => { + it('should recover on empty rvalues', () => { + checkActionWithError('a["a"] = ', 'a["a"] = ', 'Unexpected end of expression'); + }); + + it('should recover on incomplete rvalues', () => { + checkActionWithError('a["a"] = 1 + ', 'a["a"] = 1 + ', 'Unexpected end of expression'); + }); + + it('should recover on missing keys', () => { + checkActionWithError('a[] = 1', 'a[] = 1', 'Key access cannot be empty'); + }); + + it('should recover on incomplete expression keys', () => { + checkActionWithError('a[1 + ] = 1', 'a[1 + ] = 1', 'Unexpected token ]'); + }); + + it('should recover on unterminated keys', () => { + checkActionWithError('a[1 + 2 = 1', 'a[1 + 2] = 1', 'Missing expected ]'); + }); + + it('should recover on incomplete and unterminated keys', () => { + const ast = parseAction('a[1 + = 1'); + expect(unparse(ast)).toEqual('a[1 + ] = 1'); + validate(ast); + + const errors = ast.errors.map(e => e.message); + expect(errors.length).toBe(2); + expect(errors[0]).toContain('Unexpected token ='); + expect(errors[1]).toContain('Missing expected ]'); + }); + + it('should error on writes after a keyed write', () => { + const ast = parseAction('a[1] = 1 = 2'); + expect(unparse(ast)).toEqual('a[1] = 1'); + validate(ast); + + expect(ast.errors.length).toBe(1); + expect(ast.errors[0].message).toContain('Unexpected token \'=\''); + }); + }); + }); + describe('conditional', () => { it('should parse ternary/conditional expressions', () => { checkAction('7 == 3 + 4 ? 10 : 20'); @@ -926,3 +1004,12 @@ function expectActionError(text: string, message: string) { function expectBindingError(text: string, message: string) { expectError(validate(parseBinding(text)), message); } + +/** + * Check that an malformed action parses to a recovered AST while emitting an + * error. + */ +function checkActionWithError(text: string, expected: string, error: string) { + checkAction(text, expected); + expectActionError(text, error); +} From 7cc63ceceabbc6542463d46813ac66cac86d01ef Mon Sep 17 00:00:00 2001 From: ayazhafiz Date: Fri, 25 Sep 2020 20:05:18 -0500 Subject: [PATCH 2/5] fixup! feat(compiler): Recover on malformed keyed reads and keyed writes --- packages/compiler/src/expression_parser/parser.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/compiler/src/expression_parser/parser.ts b/packages/compiler/src/expression_parser/parser.ts index 33efb94ffda73..644f26b470304 100644 --- a/packages/compiler/src/expression_parser/parser.ts +++ b/packages/compiler/src/expression_parser/parser.ts @@ -642,7 +642,7 @@ export class _ParseAST { this.writeContext = true; const key = this.parsePipe(); if (key instanceof EmptyExpr) { - this.error(`Key access cannot be empty`) + this.error(`Key access cannot be empty`); } this.rbracketsExpected--; this.expectCharacter(chars.$RBRACKET); From f6f8017ac5577e839a4febff306075f952bd5404 Mon Sep 17 00:00:00 2001 From: ayazhafiz Date: Thu, 1 Oct 2020 17:20:27 -0500 Subject: [PATCH 3/5] fixup! feat(compiler): Recover on malformed keyed reads and keyed writes --- .../compiler/src/expression_parser/parser.ts | 57 +++++++++++++------ 1 file changed, 40 insertions(+), 17 deletions(-) diff --git a/packages/compiler/src/expression_parser/parser.ts b/packages/compiler/src/expression_parser/parser.ts index 644f26b470304..6834cfd381f79 100644 --- a/packages/compiler/src/expression_parser/parser.ts +++ b/packages/compiler/src/expression_parser/parser.ts @@ -288,11 +288,23 @@ export class IvyParser extends Parser { simpleExpressionChecker = IvySimpleExpressionChecker; // } +/** Describes a stateful context an expression parser is in. */ +enum ParseContext { + /** + * A Writable context is one in which a value may be written to an lvalue. + * For example, after we see a property access, we may expect a write to the + * property via the "=" operator. + * a.b + * ^ possible "=" after + */ + Writable = 1, +} + export class _ParseAST { private rparensExpected = 0; private rbracketsExpected = 0; private rbracesExpected = 0; - private writeContext = false; + private context: ParseContext = 0; // Cache of expression start and input indeces to the absolute source span they map to, used to // prevent creating superfluous source spans in `sourceSpan`. @@ -369,6 +381,16 @@ export class _ParseAST { this.index++; } + /** + * Executes a callback in the provided context. + */ + private withContext any>(context: ParseContext, cb: T): ReturnType { + this.context |= context; + const ret = cb(); + this.context ^= context; + return ret; + } + consumeOptionalCharacter(code: number): boolean { if (this.next.isCharacter(code)) { this.advance(); @@ -639,21 +661,22 @@ export class _ParseAST { } else if (this.consumeOptionalCharacter(chars.$LBRACKET)) { this.rbracketsExpected++; - this.writeContext = true; - const key = this.parsePipe(); - if (key instanceof EmptyExpr) { - this.error(`Key access cannot be empty`); - } - this.rbracketsExpected--; - this.expectCharacter(chars.$RBRACKET); - if (this.consumeOptionalOperator('=')) { - const value = this.parseConditional(); - result = new KeyedWrite( - this.span(resultStart), this.sourceSpan(resultStart), result, key, value); - } else { - result = new KeyedRead(this.span(resultStart), this.sourceSpan(resultStart), result, key); - } - this.writeContext = false; + this.withContext(ParseContext.Writable, () => { + const key = this.parsePipe(); + if (key instanceof EmptyExpr) { + this.error(`Key access cannot be empty`); + } + this.rbracketsExpected--; + this.expectCharacter(chars.$RBRACKET); + if (this.consumeOptionalOperator('=')) { + const value = this.parseConditional(); + result = new KeyedWrite( + this.span(resultStart), this.sourceSpan(resultStart), result, key, value); + } else { + result = + new KeyedRead(this.span(resultStart), this.sourceSpan(resultStart), result, key); + } + }); } else if (this.consumeOptionalCharacter(chars.$LPAREN)) { this.rparensExpected++; const args = this.parseCallArguments(); @@ -1045,7 +1068,7 @@ export class _ParseAST { (this.rparensExpected <= 0 || !n.isCharacter(chars.$RPAREN)) && (this.rbracesExpected <= 0 || !n.isCharacter(chars.$RBRACE)) && (this.rbracketsExpected <= 0 || !n.isCharacter(chars.$RBRACKET)) && - !(this.writeContext && n.isOperator('='))) { + (!(this.context & ParseContext.Writable) || !n.isOperator('='))) { if (this.next.isError()) { this.errors.push( new ParserError(this.next.toString()!, this.input, this.locationText(), this.location)); From c93b55048e5679ff48e5e244cd943b46f0f2d9b1 Mon Sep 17 00:00:00 2001 From: ayazhafiz Date: Fri, 2 Oct 2020 08:46:35 -0500 Subject: [PATCH 4/5] fixup! feat(compiler): Recover on malformed keyed reads and keyed writes --- .../compiler/src/expression_parser/parser.ts | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/packages/compiler/src/expression_parser/parser.ts b/packages/compiler/src/expression_parser/parser.ts index 6834cfd381f79..111da18e99752 100644 --- a/packages/compiler/src/expression_parser/parser.ts +++ b/packages/compiler/src/expression_parser/parser.ts @@ -289,13 +289,14 @@ export class IvyParser extends Parser { } /** Describes a stateful context an expression parser is in. */ -enum ParseContext { +enum ParseContextFlags { + None = 0, /** * A Writable context is one in which a value may be written to an lvalue. * For example, after we see a property access, we may expect a write to the * property via the "=" operator. - * a.b - * ^ possible "=" after + * prop + * ^ possible "=" after */ Writable = 1, } @@ -304,7 +305,7 @@ export class _ParseAST { private rparensExpected = 0; private rbracketsExpected = 0; private rbracesExpected = 0; - private context: ParseContext = 0; + private context = ParseContextFlags.None; // Cache of expression start and input indeces to the absolute source span they map to, used to // prevent creating superfluous source spans in `sourceSpan`. @@ -384,7 +385,7 @@ export class _ParseAST { /** * Executes a callback in the provided context. */ - private withContext any>(context: ParseContext, cb: T): ReturnType { + private withContext(context: ParseContextFlags, cb: () => T): T { this.context |= context; const ret = cb(); this.context ^= context; @@ -660,8 +661,8 @@ export class _ParseAST { result = this.parseAccessMemberOrMethodCall(result, true); } else if (this.consumeOptionalCharacter(chars.$LBRACKET)) { - this.rbracketsExpected++; - this.withContext(ParseContext.Writable, () => { + this.withContext(ParseContextFlags.Writable, () => { + this.rbracketsExpected++; const key = this.parsePipe(); if (key instanceof EmptyExpr) { this.error(`Key access cannot be empty`); @@ -1068,7 +1069,7 @@ export class _ParseAST { (this.rparensExpected <= 0 || !n.isCharacter(chars.$RPAREN)) && (this.rbracesExpected <= 0 || !n.isCharacter(chars.$RBRACE)) && (this.rbracketsExpected <= 0 || !n.isCharacter(chars.$RBRACKET)) && - (!(this.context & ParseContext.Writable) || !n.isOperator('='))) { + (!(this.context & ParseContextFlags.Writable) || !n.isOperator('='))) { if (this.next.isError()) { this.errors.push( new ParserError(this.next.toString()!, this.input, this.locationText(), this.location)); From 80cee1b0dc5817101a301a6569599be816e60900 Mon Sep 17 00:00:00 2001 From: ayazhafiz Date: Fri, 2 Oct 2020 08:49:01 -0500 Subject: [PATCH 5/5] fixup! feat(compiler): Recover on malformed keyed reads and keyed writes --- packages/compiler/src/expression_parser/parser.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/compiler/src/expression_parser/parser.ts b/packages/compiler/src/expression_parser/parser.ts index 111da18e99752..ccdb08d4563cc 100644 --- a/packages/compiler/src/expression_parser/parser.ts +++ b/packages/compiler/src/expression_parser/parser.ts @@ -1057,8 +1057,8 @@ export class _ParseAST { * parseChain() is always the root production and it expects a ';'. * * Furthermore, the presence of a stateful context can add more recovery points. - * - in a `writeContext`, we are able to recover after seeing the `=` operator, which signals - * the presence of an independent rvalue expression following the `=` operator. + * - in a `Writable` context, we are able to recover after seeing the `=` operator, which + * signals the presence of an independent rvalue expression following the `=` operator. * * If a production expects one of these token it increments the corresponding nesting count, * and then decrements it just prior to checking if the token is in the input.