Skip to content

Commit

Permalink
fix(compiler): unclear lexer error when using private identifier in e…
Browse files Browse the repository at this point in the history
…xpressions (#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
  • Loading branch information
devversion authored and atscott committed May 18, 2021
1 parent 63383c1 commit 3c726c3
Show file tree
Hide file tree
Showing 5 changed files with 97 additions and 7 deletions.
23 changes: 23 additions & 0 deletions packages/compiler/src/expression_parser/lexer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import * as chars from '../chars';
export enum TokenType {
Character,
Identifier,
PrivateIdentifier,
Keyword,
String,
Operator,
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
Expand All @@ -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);
}
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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.
Expand Down
31 changes: 29 additions & 2 deletions packages/compiler/src/expression_parser/parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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();
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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.
*
Expand Down
36 changes: 32 additions & 4 deletions packages/compiler/test/expression_parser/lexer_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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, '?.');
});
Expand Down
11 changes: 11 additions & 0 deletions packages/compiler/test/expression_parser/parser_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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');
});
});

Expand All @@ -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', () => {
Expand Down Expand Up @@ -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', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1622,7 +1622,8 @@ Reference "#a" is defined several times ("<div #a></div><div [ERROR ->]#a></div>
describe('inline templates', () => {
it('should report an error on variables declared with #', () => {
expect(() => humanizeTplAst(parse('<div *ngIf="#a=b">', [])))
.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 ...', () => {
Expand Down

0 comments on commit 3c726c3

Please sign in to comment.