From abd1bc8039ade4324f91fd616c82e52f05bdbb70 Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Fri, 21 Jan 2022 13:08:48 -0800 Subject: [PATCH] fix(compiler): correct spans when parsing bindings with comments (#44785) The previous fix for correcting spans with comments in https://github.com/angular/angular/commit/59eef29a6c5d568ca80595cd7018e21ad406c85d had the unfortunate side effect of _breaking_ the spans with comments when there was leading whitespace. This happened because the previous fix was testing one without a comment, identifying that the offset shouldn't have anything added to it, and then removing that offset adjustment (`offsets[i] + (expressionText.length - sourceToLex.length)`). Upon further investigation, this offset adjustment _was actually necessary_ for when the input had comments, but this was only because the `stripComments` function used `trim` to remove whitespace for these cases. This is the real problem -- not only does it create a ton of confusion but also it means that the behavior of the lexer and resulting spans is different between inputs with comments and inputs without comments. After reviewing how the `inputLength` of `_ParseAST` was used, it appears that the correct behavior would be to _not_ trim the input. The `inputLength` is used to advance the current index beyond points which have been processed. This _should_ include any whitespace. Additionally, `inputLength` doesn't appear to be needed at all. When there was no comment in the input, it was always equal to the `input.length` anyways. When there _is_ a comment, it should include that comment anyways to advance the index beyond the comment. PR Close #44785 --- .../src/ngtsc/indexer/test/template_spec.ts | 13 ++++++++ .../compiler/src/expression_parser/parser.ts | 32 ++++++++----------- .../test/render3/r3_ast_absolute_span_spec.ts | 11 +++++++ 3 files changed, 38 insertions(+), 18 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/indexer/test/template_spec.ts b/packages/compiler-cli/src/ngtsc/indexer/test/template_spec.ts index eb05a494e9b..1e168755e86 100644 --- a/packages/compiler-cli/src/ngtsc/indexer/test/template_spec.ts +++ b/packages/compiler-cli/src/ngtsc/indexer/test/template_spec.ts @@ -62,6 +62,19 @@ runInEachFileSystem(() => { }); }); + it('should handle whitespace and comments in interpolations', () => { + const template = '{{ foo // comment }}'; + const refs = getTemplateIdentifiers(bind(template)); + + const [ref] = Array.from(refs); + expect(ref).toEqual({ + name: 'foo', + kind: IdentifierKind.Property, + span: new AbsoluteSourceSpan(5, 8), + target: null, + }); + }); + it('should generate nothing in empty template', () => { const template = ''; const refs = getTemplateIdentifiers(bind(template)); diff --git a/packages/compiler/src/expression_parser/parser.ts b/packages/compiler/src/expression_parser/parser.ts index 4a168c9be66..8121550104c 100644 --- a/packages/compiler/src/expression_parser/parser.ts +++ b/packages/compiler/src/expression_parser/parser.ts @@ -41,9 +41,7 @@ export class Parser { const sourceToLex = this._stripComments(input); const tokens = this._lexer.tokenize(sourceToLex); const ast = - new _ParseAST( - input, location, absoluteOffset, tokens, sourceToLex.length, true, this.errors, 0) - .parseChain(); + new _ParseAST(input, location, absoluteOffset, tokens, true, this.errors, 0).parseChain(); return new ASTWithSource(ast, input, location, absoluteOffset, this.errors); } @@ -90,8 +88,7 @@ export class Parser { this._checkNoInterpolation(input, location, interpolationConfig); const sourceToLex = this._stripComments(input); const tokens = this._lexer.tokenize(sourceToLex); - return new _ParseAST( - input, location, absoluteOffset, tokens, sourceToLex.length, false, this.errors, 0) + return new _ParseAST(input, location, absoluteOffset, tokens, false, this.errors, 0) .parseChain(); } @@ -138,8 +135,8 @@ export class Parser { absoluteValueOffset: number): TemplateBindingParseResult { const tokens = this._lexer.tokenize(templateValue); const parser = new _ParseAST( - templateValue, templateUrl, absoluteValueOffset, tokens, templateValue.length, - false /* parseAction */, this.errors, 0 /* relative offset */); + templateValue, templateUrl, absoluteValueOffset, tokens, false /* parseAction */, + this.errors, 0 /* relative offset */); return parser.parseTemplateBindings({ source: templateKey, span: new AbsoluteSourceSpan(absoluteKeyOffset, absoluteKeyOffset + templateKey.length), @@ -159,10 +156,9 @@ export class Parser { const expressionText = expressions[i].text; const sourceToLex = this._stripComments(expressionText); const tokens = this._lexer.tokenize(sourceToLex); - const ast = new _ParseAST( - input, location, absoluteOffset, tokens, sourceToLex.length, false, - this.errors, offsets[i]) - .parseChain(); + const ast = + new _ParseAST(input, location, absoluteOffset, tokens, false, this.errors, offsets[i]) + .parseChain(); expressionNodes.push(ast); } @@ -180,7 +176,7 @@ export class Parser { const sourceToLex = this._stripComments(expression); const tokens = this._lexer.tokenize(sourceToLex); const ast = new _ParseAST( - expression, location, absoluteOffset, tokens, sourceToLex.length, + expression, location, absoluteOffset, tokens, /* parseAction */ false, this.errors, 0) .parseChain(); const strings = ['', '']; // The prefix and suffix strings are both empty @@ -275,7 +271,7 @@ export class Parser { private _stripComments(input: string): string { const i = this._commentStart(input); - return i != null ? input.substring(0, i).trim() : input; + return i != null ? input.substring(0, i) : input; } private _commentStart(input: string): number|null { @@ -392,8 +388,8 @@ export class _ParseAST { constructor( public input: string, public location: string, public absoluteOffset: number, - public tokens: Token[], public inputLength: number, public parseAction: boolean, - private errors: ParserError[], private offset: number) {} + public tokens: Token[], public parseAction: boolean, private errors: ParserError[], + private offset: number) {} peek(offset: number): Token { const i = this.index + offset; @@ -429,7 +425,7 @@ export class _ParseAST { // No tokens have been processed yet; return the next token's start or the length of the input // if there is no token. if (this.tokens.length === 0) { - return this.inputLength + this.offset; + return this.input.length + this.offset; } return this.next.index + this.offset; } @@ -587,7 +583,7 @@ export class _ParseAST { if (exprs.length == 0) { // We have no expressions so create an empty expression that spans the entire input length const artificialStart = this.offset; - const artificialEnd = this.offset + this.inputLength; + const artificialEnd = this.offset + this.input.length; return new EmptyExpr( this.span(artificialStart, artificialEnd), this.sourceSpan(artificialStart, artificialEnd)); @@ -623,7 +619,7 @@ export class _ParseAST { // // Therefore, we push the end of the `ParseSpan` for this pipe all the way up to the // beginning of the next token, or until the end of input if the next token is EOF. - fullSpanEnd = this.next.index !== -1 ? this.next.index : this.inputLength + this.offset; + fullSpanEnd = this.next.index !== -1 ? this.next.index : this.input.length + this.offset; // The `nameSpan` for an empty pipe name is zero-length at the end of any whitespace // beyond the pipe character. diff --git a/packages/compiler/test/render3/r3_ast_absolute_span_spec.ts b/packages/compiler/test/render3/r3_ast_absolute_span_spec.ts index df707989260..56bee6cbaca 100644 --- a/packages/compiler/test/render3/r3_ast_absolute_span_spec.ts +++ b/packages/compiler/test/render3/r3_ast_absolute_span_spec.ts @@ -17,6 +17,17 @@ describe('expression AST absolute source spans', () => { .toContain(['foo', new AbsoluteSourceSpan(2, 5)]); }); + it('should handle whitespace in interpolation', () => { + expect(humanizeExpressionSource(parse('{{ foo }}', {preserveWhitespaces: true}).nodes)) + .toContain(['foo', new AbsoluteSourceSpan(4, 7)]); + }); + + it('should handle whitespace and comment in interpolation', () => { + expect(humanizeExpressionSource( + parse('{{ foo // comment }}', {preserveWhitespaces: true}).nodes)) + .toContain(['foo', new AbsoluteSourceSpan(4, 7)]); + }); + it('should handle comment in an action binding', () => { expect(humanizeExpressionSource(parse('', { preserveWhitespaces: true