From 3c726c35165a99fdb857bbe48a01705b6806f2ba Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Mon, 10 May 2021 18:16:45 +0200 Subject: [PATCH] fix(compiler): unclear lexer error when using private identifier in expressions (#42027) TypeScript supports ECMAScript private identifiers. It can happen that developers intend to access such members from within an expression. This currently results in an unclear error from the lexer. e.g. ``` 'Parser Error: Unexpected token # at column 1 in [{{#myField}}] in C:/test.ts@5:2 ``` We could improve such errors by tokenizing private identifiers similar to how the TypeScript scanner processes them. Later we can report better errors in the expression parser or in the typecheck block. This commit causes all private identifier tokens to be disallowed, so it never reaches the type checker. This is done intentionally as private identifiers should not be considered valid Angular syntax, especially because private fields are not guaranteed to be accessible from within a component/directive definition (e.g. there cases where a template function is generated outside of the class; which results in private members not being accessible; and this results in mixed/confusing behavior). Fixes #36003. PR Close #42027 --- .../compiler/src/expression_parser/lexer.ts | 23 ++++++++++++ .../compiler/src/expression_parser/parser.ts | 31 ++++++++++++++-- .../test/expression_parser/lexer_spec.ts | 36 ++++++++++++++++--- .../test/expression_parser/parser_spec.ts | 11 ++++++ .../template_parser/template_parser_spec.ts | 3 +- 5 files changed, 97 insertions(+), 7 deletions(-) diff --git a/packages/compiler/src/expression_parser/lexer.ts b/packages/compiler/src/expression_parser/lexer.ts index 7346dd8609..99c5a5bcd5 100644 --- a/packages/compiler/src/expression_parser/lexer.ts +++ b/packages/compiler/src/expression_parser/lexer.ts @@ -11,6 +11,7 @@ import * as chars from '../chars'; export enum TokenType { Character, Identifier, + PrivateIdentifier, Keyword, String, Operator, @@ -58,6 +59,10 @@ export class Token { return this.type == TokenType.Identifier; } + isPrivateIdentifier(): boolean { + return this.type == TokenType.PrivateIdentifier; + } + isKeyword(): boolean { return this.type == TokenType.Keyword; } @@ -104,6 +109,7 @@ export class Token { case TokenType.Identifier: case TokenType.Keyword: case TokenType.Operator: + case TokenType.PrivateIdentifier: case TokenType.String: case TokenType.Error: return this.strValue; @@ -123,6 +129,10 @@ function newIdentifierToken(index: number, end: number, text: string): Token { return new Token(index, end, TokenType.Identifier, 0, text); } +function newPrivateIdentifierToken(index: number, end: number, text: string): Token { + return new Token(index, end, TokenType.PrivateIdentifier, 0, text); +} + function newKeywordToken(index: number, end: number, text: string): Token { return new Token(index, end, TokenType.Keyword, 0, text); } @@ -204,6 +214,7 @@ class _Scanner { case chars.$DQ: return this.scanString(); case chars.$HASH: + return this.scanPrivateIdentifier(); case chars.$PLUS: case chars.$MINUS: case chars.$STAR: @@ -279,6 +290,18 @@ class _Scanner { newIdentifierToken(start, this.index, str); } + /** Scans an ECMAScript private identifier. */ + scanPrivateIdentifier(): Token { + const start: number = this.index; + this.advance(); + if (!isIdentifierStart(this.peek)) { + return this.error('Invalid character [#]', -1); + } + while (isIdentifierPart(this.peek)) this.advance(); + const identifierName: string = this.input.substring(start, this.index); + return newPrivateIdentifierToken(start, this.index, identifierName); + } + scanNumber(start: number): Token { let simple: boolean = (this.index === start); this.advance(); // Skip initial digit. diff --git a/packages/compiler/src/expression_parser/parser.ts b/packages/compiler/src/expression_parser/parser.ts index 18a516649e..01dc54a1a9 100644 --- a/packages/compiler/src/expression_parser/parser.ts +++ b/packages/compiler/src/expression_parser/parser.ts @@ -548,7 +548,11 @@ export class _ParseAST { expectIdentifierOrKeyword(): string|null { const n = this.next; if (!n.isIdentifier() && !n.isKeyword()) { - this.error(`Unexpected ${this.prettyPrintToken(n)}, expected identifier or keyword`); + if (n.isPrivateIdentifier()) { + this._reportErrorForPrivateIdentifier(n, 'expected identifier or keyword'); + } else { + this.error(`Unexpected ${this.prettyPrintToken(n)}, expected identifier or keyword`); + } return null; } this.advance(); @@ -558,7 +562,12 @@ export class _ParseAST { expectIdentifierOrKeywordOrString(): string { const n = this.next; if (!n.isIdentifier() && !n.isKeyword() && !n.isString()) { - this.error(`Unexpected ${this.prettyPrintToken(n)}, expected identifier, keyword, or string`); + if (n.isPrivateIdentifier()) { + this._reportErrorForPrivateIdentifier(n, 'expected identifier, keyword or string'); + } else { + this.error( + `Unexpected ${this.prettyPrintToken(n)}, expected identifier, keyword, or string`); + } return ''; } this.advance(); @@ -899,6 +908,10 @@ export class _ParseAST { this.advance(); return new LiteralPrimitive(this.span(start), this.sourceSpan(start), literalValue); + } else if (this.next.isPrivateIdentifier()) { + this._reportErrorForPrivateIdentifier(this.next, null); + return new EmptyExpr(this.span(start), this.sourceSpan(start)); + } else if (this.index >= this.tokens.length) { this.error(`Unexpected end of expression: ${this.input}`); return new EmptyExpr(this.span(start), this.sourceSpan(start)); @@ -1209,6 +1222,20 @@ export class _ParseAST { `at the end of the expression`; } + /** + * Records an error for an unexpected private identifier being discovered. + * @param token Token representing a private identifier. + * @param extraMessage Optional additional message being appended to the error. + */ + private _reportErrorForPrivateIdentifier(token: Token, extraMessage: string|null) { + let errorMessage = + `Private identifiers are not supported. Unexpected private identifier: ${token}`; + if (extraMessage !== null) { + errorMessage += `, ${extraMessage}`; + } + this.error(errorMessage); + } + /** * Error recovery should skip tokens until it encounters a recovery point. * diff --git a/packages/compiler/test/expression_parser/lexer_spec.ts b/packages/compiler/test/expression_parser/lexer_spec.ts index 664529f9ef..6946ff3d84 100644 --- a/packages/compiler/test/expression_parser/lexer_spec.ts +++ b/packages/compiler/test/expression_parser/lexer_spec.ts @@ -47,6 +47,13 @@ function expectIdentifierToken(token: any, index: number, end: number, identifie expect(token.toString()).toEqual(identifier); } +function expectPrivateIdentifierToken(token: any, index: number, end: number, identifier: string) { + expectToken(token, index, end); + expect(token.isPrivateIdentifier()).toBe(true); + expect(token.toString()).toEqual(identifier); +} + + function expectKeywordToken(token: any, index: number, end: number, keyword: string) { expectToken(token, index, end); expect(token.isKeyword()).toBe(true); @@ -82,6 +89,31 @@ function expectErrorToken(token: Token, index: any, end: number, message: string expectIdentifierToken(tokens[2], 2, 3, 'k'); }); + it('should tokenize a private identifier', () => { + const tokens: number[] = lex('#a'); + expect(tokens.length).toEqual(1); + expectPrivateIdentifierToken(tokens[0], 0, 2, '#a'); + }); + + it('should tokenize a property access with private identifier', () => { + const tokens: number[] = lex('j.#k'); + expect(tokens.length).toEqual(3); + expectIdentifierToken(tokens[0], 0, 1, 'j'); + expectCharacterToken(tokens[1], 1, 2, '.'); + expectPrivateIdentifierToken(tokens[2], 2, 4, '#k'); + }); + + it('should throw an invalid character error when a hash character is discovered but ' + + 'not indicating a private identifier', + () => { + expectErrorToken( + lex('#')[0], 0, 1, + `Lexer Error: Invalid character [#] at column 0 in expression [#]`); + expectErrorToken( + lex('#0')[0], 0, 1, + `Lexer Error: Invalid character [#] at column 0 in expression [#0]`); + }); + it('should tokenize an operator', () => { const tokens: number[] = lex('j-k'); expect(tokens.length).toEqual(3); @@ -248,10 +280,6 @@ function expectErrorToken(token: Token, index: any, end: number, message: string 'Lexer Error: Invalid unicode escape [\\u1\'\'b] at column 2 in expression [\'\\u1\'\'bla\']'); }); - it('should tokenize hash as operator', () => { - expectOperatorToken(lex('#')[0], 0, 1, '#'); - }); - it('should tokenize ?. as operator', () => { expectOperatorToken(lex('?.')[0], 0, 2, '?.'); }); diff --git a/packages/compiler/test/expression_parser/parser_spec.ts b/packages/compiler/test/expression_parser/parser_spec.ts index 5e9b149c83..f813421059 100644 --- a/packages/compiler/test/expression_parser/parser_spec.ts +++ b/packages/compiler/test/expression_parser/parser_spec.ts @@ -120,6 +120,7 @@ describe('parser', () => { it('should only allow identifier, string, or keyword as map key', () => { expectActionError('{(:0}', 'expected identifier, keyword, or string'); expectActionError('{1234:0}', 'expected identifier, keyword, or string'); + expectActionError('{#myField:0}', 'expected identifier, keyword or string'); }); }); @@ -130,11 +131,20 @@ describe('parser', () => { checkAction('a.a'); }); + it('should error for private identifiers with implicit receiver', () => { + checkActionWithError( + '#privateField', '', + 'Private identifiers are not supported. Unexpected private identifier: #privateField at column 1'); + }); + it('should only allow identifier or keyword as member names', () => { checkActionWithError('x.', 'x.', 'identifier or keyword'); checkActionWithError('x.(', 'x.', 'identifier or keyword'); checkActionWithError('x. 1234', 'x.', 'identifier or keyword'); checkActionWithError('x."foo"', 'x.', 'identifier or keyword'); + checkActionWithError( + 'x.#privateField', 'x.', + 'Private identifiers are not supported. Unexpected private identifier: #privateField, expected identifier or keyword'); }); it('should parse safe field access', () => { @@ -511,6 +521,7 @@ describe('parser', () => { expectBindingError('"Foo"|(', 'identifier or keyword'); expectBindingError('"Foo"|1234', 'identifier or keyword'); expectBindingError('"Foo"|"uppercase"', 'identifier or keyword'); + expectBindingError('"Foo"|#privateIdentifier"', 'identifier or keyword'); }); it('should parse quoted expressions', () => { diff --git a/packages/compiler/test/template_parser/template_parser_spec.ts b/packages/compiler/test/template_parser/template_parser_spec.ts index 69a0c3e754..1c7a9116dc 100644 --- a/packages/compiler/test/template_parser/template_parser_spec.ts +++ b/packages/compiler/test/template_parser/template_parser_spec.ts @@ -1622,7 +1622,8 @@ Reference "#a" is defined several times ("
]#a>
describe('inline templates', () => { it('should report an error on variables declared with #', () => { expect(() => humanizeTplAst(parse('
', []))) - .toThrowError(/Parser Error: Unexpected token # at column 1/); + .toThrowError( + /Parser Error: Private identifiers are not supported\. Unexpected private identifier: #a at column 1/); }); it('should parse variables via let ...', () => {