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
This commit is contained in:
Andrew Scott
2022-01-21 13:08:48 -08:00
committed by Andrew Kushnir
parent 7316e72ec5
commit abd1bc8039
3 changed files with 38 additions and 18 deletions
@@ -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));
@@ -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.
@@ -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('<button (click)="foo = true // comment">Save</button>', {
preserveWhitespaces: true