honeymoose
/
angular-cn
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix(compiler): record correct end of expression (#34690) 

This commit fixes a bug with the expression parser wherein the end index
of an expression node was recorded as the start index of the next token,
not the end index of the current token.

Closes #33477
Closes https://github.com/angular/vscode-ng-language-service/issues/433

PR Close #34690
This commit is contained in:
ayazhafiz 2019-12-18 19:30:56 -06:00 committed by Misko Hevery
parent 47bfec4e46
commit df890d7629
9 changed files with 87 additions and 92 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

38
packages/compiler-cli/src/ngtsc/typecheck/test/span_comments_spec.ts
View File

@ -12,17 +12,17 @@ describe('type check blocks diagnostics', () => {
describe('parse spans', () => {
it('should annotate binary ops', () => {
expect(tcbWithSpans('{{ a + b }}'))
.toContain('"" + (((ctx).a /*3,5*/) + ((ctx).b /*7,9*/) /*3,9*/);');
.toContain('"" + (((ctx).a /*3,4*/) + ((ctx).b /*7,8*/) /*3,8*/);');
});
it('should annotate conditions', () => {
expect(tcbWithSpans('{{ a ? b : c }}'))
.toContain('((ctx).a /*3,5*/ ? (ctx).b /*7,9*/ : (ctx).c /*11,13*/) /*3,13*/;');
.toContain('((ctx).a /*3,4*/ ? (ctx).b /*7,8*/ : (ctx).c /*11,12*/) /*3,12*/;');
});
it('should annotate interpolations', () => {
expect(tcbWithSpans('{{ hello }} {{ world }}'))
.toContain('"" + (ctx).hello /*3,9*/ + (ctx).world /*15,21*/;');
.toContain('"" + (ctx).hello /*3,8*/ + (ctx).world /*15,20*/;');
});
it('should annotate literal map expressions', () => {
@ -35,46 +35,46 @@ describe('type check blocks diagnostics', () => {
it('should annotate literal array expressions', () => {
const TEMPLATE = '{{ [a, b] }}';
expect(tcbWithSpans(TEMPLATE)).toContain('[(ctx).a /*4,5*/, (ctx).b /*7,8*/] /*3,10*/;');
expect(tcbWithSpans(TEMPLATE)).toContain('[(ctx).a /*4,5*/, (ctx).b /*7,8*/] /*3,9*/;');
});
it('should annotate literals', () => {
const TEMPLATE = '{{ 123 }}';
expect(tcbWithSpans(TEMPLATE)).toContain('123 /*3,7*/;');
expect(tcbWithSpans(TEMPLATE)).toContain('123 /*3,6*/;');
});
it('should annotate non-null assertions', () => {
const TEMPLATE = `{{ a! }}`;
expect(tcbWithSpans(TEMPLATE)).toContain('(((ctx).a /*3,4*/)! /*3,6*/);');
expect(tcbWithSpans(TEMPLATE)).toContain('(((ctx).a /*3,4*/)! /*3,5*/);');
});
it('should annotate prefix not', () => {
const TEMPLATE = `{{ !a }}`;
expect(tcbWithSpans(TEMPLATE)).toContain('!((ctx).a /*4,6*/) /*3,6*/;');
expect(tcbWithSpans(TEMPLATE)).toContain('!((ctx).a /*4,5*/) /*3,5*/;');
});
it('should annotate method calls', () => {
const TEMPLATE = `{{ method(a, b) }}`;
expect(tcbWithSpans(TEMPLATE))
.toContain('(ctx).method((ctx).a /*10,11*/, (ctx).b /*13,14*/) /*3,16*/;');
.toContain('(ctx).method((ctx).a /*10,11*/, (ctx).b /*13,14*/) /*3,15*/;');
});
it('should annotate method calls of variables', () => {
const TEMPLATE = `<ng-template let-method>{{ method(a, b) }}</ng-template>`;
expect(tcbWithSpans(TEMPLATE))
.toContain('(_t2 /*27,40*/).method((ctx).a /*34,35*/, (ctx).b /*37,38*/) /*27,40*/;');
.toContain('(_t2 /*27,39*/).method((ctx).a /*34,35*/, (ctx).b /*37,38*/) /*27,39*/;');
});
it('should annotate function calls', () => {
const TEMPLATE = `{{ method(a)(b, c) }}`;
expect(tcbWithSpans(TEMPLATE))
.toContain(
'((ctx).method((ctx).a /*10,11*/) /*3,12*/)((ctx).b /*13,14*/, (ctx).c /*16,17*/) /*3,19*/;');
'((ctx).method((ctx).a /*10,11*/) /*3,12*/)((ctx).b /*13,14*/, (ctx).c /*16,17*/) /*3,18*/;');
});
it('should annotate property access', () => {
const TEMPLATE = `{{ a.b.c }}`;
expect(tcbWithSpans(TEMPLATE)).toContain('(((ctx).a /*3,4*/).b /*3,6*/).c /*3,9*/;');
expect(tcbWithSpans(TEMPLATE)).toContain('(((ctx).a /*3,4*/).b /*3,6*/).c /*3,8*/;');
});
it('should annotate property writes', () => {
@ -85,7 +85,7 @@ describe('type check blocks diagnostics', () => {
it('should annotate keyed property access', () => {
const TEMPLATE = `{{ a[b] }}`;
expect(tcbWithSpans(TEMPLATE)).toContain('((ctx).a /*3,4*/)[(ctx).b /*5,6*/] /*3,8*/;');
expect(tcbWithSpans(TEMPLATE)).toContain('((ctx).a /*3,4*/)[(ctx).b /*5,6*/] /*3,7*/;');
});
it('should annotate keyed property writes', () => {
@ -97,19 +97,19 @@ describe('type check blocks diagnostics', () => {
it('should annotate safe property access', () => {
const TEMPLATE = `{{ a?.b }}`;
expect(tcbWithSpans(TEMPLATE))
.toContain('((null as any) ? ((ctx).a /*3,4*/)!.b : undefined) /*3,8*/');
.toContain('((null as any) ? ((ctx).a /*3,4*/)!.b : undefined) /*3,7*/');
});
it('should annotate safe method calls', () => {
const TEMPLATE = `{{ a?.method(b) }}`;
expect(tcbWithSpans(TEMPLATE))
.toContain(
'((null as any) ? ((ctx).a /*3,4*/)!.method((ctx).b /*13,14*/) : undefined) /*3,16*/');
'((null as any) ? ((ctx).a /*3,4*/)!.method((ctx).b /*13,14*/) : undefined) /*3,15*/');
});
it('should annotate $any casts', () => {
const TEMPLATE = `{{ $any(a) }}`;
expect(tcbWithSpans(TEMPLATE)).toContain('((ctx).a /*8,9*/ as any) /*3,11*/;');
expect(tcbWithSpans(TEMPLATE)).toContain('((ctx).a /*8,9*/ as any) /*3,10*/;');
});
it('should annotate chained expressions', () => {
@ -127,17 +127,17 @@ describe('type check blocks diagnostics', () => {
}];
const block = tcbWithSpans(TEMPLATE, PIPES);
expect(block).toContain(
'(null as TestPipe).transform((ctx).a /*3,5*/, (ctx).b /*12,14*/) /*3,14*/;');
'(null as TestPipe).transform((ctx).a /*3,4*/, (ctx).b /*12,13*/) /*3,13*/;');
});
describe('attaching multiple comments for multiple references', () => {
it('should be correct for element refs', () => {
const TEMPLATE = `<span #a></span>{{ a || a }}`;
expect(tcbWithSpans(TEMPLATE)).toContain('((_t1 /*19,21*/) || (_t1 /*24,26*/) /*19,26*/);');
expect(tcbWithSpans(TEMPLATE)).toContain('((_t1 /*19,20*/) || (_t1 /*24,25*/) /*19,25*/);');
});
it('should be correct for template vars', () => {
const TEMPLATE = `<ng-template let-a="b">{{ a || a }}</ng-template>`;
expect(tcbWithSpans(TEMPLATE)).toContain('((_t2 /*26,28*/) || (_t2 /*31,33*/) /*26,33*/);');
expect(tcbWithSpans(TEMPLATE)).toContain('((_t2 /*26,27*/) || (_t2 /*31,32*/) /*26,32*/);');
});
it('should be correct for directive refs', () => {
const DIRECTIVES: TestDeclaration[] = [{
@ -148,7 +148,7 @@ describe('type check blocks diagnostics', () => {
}];
const TEMPLATE = `<my-cmp #a></my-cmp>{{ a || a }}`;
expect(tcbWithSpans(TEMPLATE, DIRECTIVES))
.toContain('((_t2 /*23,25*/) || (_t2 /*28,30*/) /*23,30*/);');
.toContain('((_t2 /*23,24*/) || (_t2 /*28,29*/) /*23,29*/);');
});
});
});

4
packages/compiler-cli/test/ngtsc/template_mapping_spec.ts
View File

@ -176,9 +176,9 @@ runInEachFileSystem((os) => {
expect(mappings).toContain(
{source: 'items.push(', generated: 'ctx.items.push(', sourceUrl: '../test.ts'});
expect(mappings).toContain(
{source: `'item' `, generated: `"item"`, sourceUrl: '../test.ts'});
{source: `'item'`, generated: `"item"`, sourceUrl: '../test.ts'});
expect(mappings).toContain({
source: '+ items.length)',
source: ' + items.length)',
generated: ' + ctx.items.length)',
sourceUrl: '../test.ts'
});

16
packages/compiler/src/expression_parser/parser.ts
View File

@ -312,7 +312,14 @@ export class _ParseAST {
*/
get currentAbsoluteOffset(): number { return this.absoluteOffset + this.inputIndex; }
span(start: number) { return new ParseSpan(start, this.inputIndex); }
span(start: number) {
// `end` is either the
// - end index of the current token
// - start of the first token (this can happen e.g. when creating an implicit receiver)
const curToken = this.peek(-1);
const end = this.index > 0 ? curToken.end + this.offset : this.inputIndex;
return new ParseSpan(start, end);
}
sourceSpan(start: number): AbsoluteSourceSpan {
const serial = `${start}@${this.inputIndex}`;
@ -740,7 +747,6 @@ export class _ParseAST {
const value = this.parseConditional();
return new PropertyWrite(this.span(start), this.sourceSpan(start), receiver, id, value);
} else {
const span = this.span(start);
return new PropertyRead(this.span(start), this.sourceSpan(start), receiver, id);
}
}
@ -887,11 +893,7 @@ export class _ParseAST {
return null;
}
const ast = this.parsePipe(); // example: "condition | async"
const {start} = ast.span;
// Getting the end of the last token removes trailing whitespace.
// If ast has the correct end span then no need to peek at last token.
// TODO(ayazhafiz): Remove this in https://github.com/angular/angular/pull/34690
const {end} = this.peek(-1);
const {start, end} = ast.span;
const value = this.input.substring(start, end);
return new ASTWithSource(ast, value, this.location, this.absoluteOffset + start, this.errors);
}

6
packages/compiler/test/expression_parser/parser_spec.ts
View File

@ -516,7 +516,7 @@ describe('parser', () => {
const bindings = parseTemplateBindings(attr);
expect(humanizeSpans(bindings, attr)).toEqual([
// source span, key span, value span
['ngIf="cond | pipe ', 'ngIf', 'cond | pipe '],
['ngIf="cond | pipe ', 'ngIf', 'cond | pipe'],
['ngIf="cond | pipe as foo, ', 'foo', 'ngIf'],
['let x; ', 'x', null],
['ngIf as y', 'y', 'ngIf'],
@ -531,7 +531,7 @@ describe('parser', () => {
// source span, key span, value span
['ngFor="', 'ngFor', null],
['let item; ', 'item', null],
['of items | slice:0:1 ', 'of', 'items | slice:0:1 '],
['of items | slice:0:1 ', 'of', 'items | slice:0:1'],
['of items | slice:0:1 as collection, ', 'collection', 'of'],
['trackBy: func; ', 'trackBy', 'func'],
['index as i', 'i', 'index'],
@ -545,7 +545,7 @@ describe('parser', () => {
// source span, key span, value span
['ngFor="', 'ngFor', null],
['let item, ', 'item', null],
['of: [1,2,3] | pipe ', 'of', '[1,2,3] | pipe '],
['of: [1,2,3] | pipe ', 'of', '[1,2,3] | pipe'],
['of: [1,2,3] | pipe as items; ', 'items', 'of'],
['let i=index, ', 'i', 'index'],
['count as len, ', 'len', 'count'],

22
packages/compiler/test/render3/r3_ast_absolute_span_spec.ts
View File

@ -60,9 +60,7 @@ describe('expression AST absolute source spans', () => {
it('should provide absolute offsets of expressions in a binary expression', () => {
expect(humanizeExpressionSource(parse('<div>{{1 + 2}}<div>').nodes))
.toEqual(jasmine.arrayContaining([
// TODO(ayazhafiz): The expression parser includes an extra whitespace on a expressions
// with trailing whitespace in a binary expression. Look into fixing this.
['1', new AbsoluteSourceSpan(7, 9)],
['1', new AbsoluteSourceSpan(7, 8)],
['2', new AbsoluteSourceSpan(11, 12)],
]));
});
@ -78,10 +76,8 @@ describe('expression AST absolute source spans', () => {
it('should provide absolute offsets of expressions in a conditional', () => {
expect(humanizeExpressionSource(parse('<div>{{bool ? 1 : 0}}<div>').nodes))
.toEqual(jasmine.arrayContaining([
// TODO(ayazhafiz): The expression parser includes an extra whitespace on a expressions
// with trailing whitespace in a conditional expression. Look into fixing this.
['bool', new AbsoluteSourceSpan(7, 12)],
['1', new AbsoluteSourceSpan(14, 16)],
['bool', new AbsoluteSourceSpan(7, 11)],
['1', new AbsoluteSourceSpan(14, 15)],
['0', new AbsoluteSourceSpan(18, 19)],
]));
});
@ -133,9 +129,7 @@ describe('expression AST absolute source spans', () => {
it('should provide absolute offsets of expressions in an interpolation', () => {
expect(humanizeExpressionSource(parse('<div>{{1 + 2}}<div>').nodes))
.toEqual(jasmine.arrayContaining([
// TODO(ayazhafiz): The expression parser includes an extra whitespace on a expressions
// with trailing whitespace in a conditional expression. Look into fixing this.
['1', new AbsoluteSourceSpan(7, 9)],
['1', new AbsoluteSourceSpan(7, 8)],
['2', new AbsoluteSourceSpan(11, 12)],
]));
});
@ -197,9 +191,7 @@ describe('expression AST absolute source spans', () => {
describe('literal map', () => {
it('should provide absolute offsets of a literal map', () => {
expect(humanizeExpressionSource(parse('<div>{{ {a: 0} }}<div>').nodes)).toContain([
// TODO(ayazhafiz): The expression parser includes an extra whitespace on a expressions
// with trailing whitespace in a literal map. Look into fixing this.
'{a: 0}', new AbsoluteSourceSpan(8, 15)
'{a: 0}', new AbsoluteSourceSpan(8, 14)
]);
});
@ -248,9 +240,7 @@ describe('expression AST absolute source spans', () => {
it('should provide absolute offsets expressions in a pipe', () => {
expect(humanizeExpressionSource(parse('<div>{{prop | pipe}}<div>').nodes)).toContain([
// TODO(ayazhafiz): The expression parser includes an extra whitespace on a expressions
// with trailing whitespace in a pipe. Look into fixing this.
'prop', new AbsoluteSourceSpan(7, 12)
'prop', new AbsoluteSourceSpan(7, 11)
]);
});
});

24
packages/compiler/test/template_parser/template_parser_absolute_span_spec.ts
View File

@ -94,9 +94,7 @@ describe('expression AST absolute source spans', () => {
it('should provide absolute offsets of expressions in a binary expression', () => {
expect(humanizeExpressionSource(parse('<div>{{1 + 2}}<div>')))
.toEqual(jasmine.arrayContaining([
// TODO(ayazhafiz): The expression parser includes an extra whitespace on a expressions
// with trailing whitespace in a binary expression. Look into fixing this.
['1', new AbsoluteSourceSpan(7, 9)],
['1', new AbsoluteSourceSpan(7, 8)],
['2', new AbsoluteSourceSpan(11, 12)],
]));
});
@ -112,10 +110,8 @@ describe('expression AST absolute source spans', () => {
it('should provide absolute offsets of expressions in a conditional', () => {
expect(humanizeExpressionSource(parse('<div>{{bool ? 1 : 0}}<div>')))
.toEqual(jasmine.arrayContaining([
// TODO(ayazhafiz): The expression parser includes an extra whitespace on a expressions
// with trailing whitespace in a conditional expression. Look into fixing this.
['bool', new AbsoluteSourceSpan(7, 12)],
['1', new AbsoluteSourceSpan(14, 16)],
['bool', new AbsoluteSourceSpan(7, 11)],
['1', new AbsoluteSourceSpan(14, 15)],
['0', new AbsoluteSourceSpan(18, 19)],
]));
});
@ -167,9 +163,7 @@ describe('expression AST absolute source spans', () => {
it('should provide absolute offsets of expressions in an interpolation', () => {
expect(humanizeExpressionSource(parse('<div>{{1 + 2}}<div>')))
.toEqual(jasmine.arrayContaining([
// TODO(ayazhafiz): The expression parser includes an extra whitespace on a expressions
// with trailing whitespace in a conditional expression. Look into fixing this.
['1', new AbsoluteSourceSpan(7, 9)],
['1', new AbsoluteSourceSpan(7, 8)],
['2', new AbsoluteSourceSpan(11, 12)],
]));
});
@ -231,9 +225,7 @@ describe('expression AST absolute source spans', () => {
describe('literal map', () => {
it('should provide absolute offsets of a literal map', () => {
expect(humanizeExpressionSource(parse('<div>{{ {a: 0} }}<div>'))).toContain([
// TODO(ayazhafiz): The expression parser includes an extra whitespace on a expressions
// with trailing whitespace in a literal map. Look into fixing this.
'{a: 0}', new AbsoluteSourceSpan(8, 15)
'{a: 0}', new AbsoluteSourceSpan(8, 14)
]);
});
@ -287,11 +279,7 @@ describe('expression AST absolute source spans', () => {
it('should provide absolute offsets expressions in a pipe', () => {
expect(humanizeExpressionSource(parse('<div>{{prop | test}}<div>', [], [testPipe])))
.toContain([
// TODO(ayazhafiz): The expression parser includes an extra whitespace on a expressions
// with trailing whitespace in a pipe. Look into fixing this.
'prop', new AbsoluteSourceSpan(7, 12)
]);
.toContain(['prop', new AbsoluteSourceSpan(7, 11)]);
});
});

32
packages/language-service/src/completions.ts
View File

@ -6,7 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/
import {AST, ASTWithSource, AstPath, AttrAst, Attribute, BoundDirectivePropertyAst, BoundElementPropertyAst, BoundEventAst, BoundTextAst, Element, ElementAst, HtmlAstPath, NAMED_ENTITIES, Node as HtmlAst, NullTemplateVisitor, ReferenceAst, TagContentType, TemplateBinding, Text, VariableBinding, getHtmlTagDefinition} from '@angular/compiler';
import {AST, AbsoluteSourceSpan, AstPath, AttrAst, Attribute, BoundDirectivePropertyAst, BoundElementPropertyAst, BoundEventAst, BoundTextAst, Element, ElementAst, EmptyExpr, ExpressionBinding, HtmlAstPath, NAMED_ENTITIES, Node as HtmlAst, NullTemplateVisitor, ParseSpan, ReferenceAst, TagContentType, TemplateBinding, Text, VariableBinding, getHtmlTagDefinition} from '@angular/compiler';
import {$$, $_, isAsciiLetter, isDigit} from '@angular/compiler/src/chars';
import {AstResult} from './common';
@ -575,21 +575,21 @@ class ExpressionVisitor extends NullTemplateVisitor {
}
}
}
}
else if (inSpan(valueRelativePosition, binding.value?.ast.span)) {
this.processExpressionCompletions(binding.value !.ast);
return;
}
// If the expression is incomplete, for example *ngFor="let x of |"
// binding.expression is null. We could still try to provide suggestions
// by looking for symbols that are in scope.
const KW_OF = ' of ';
const ofLocation = attr.value.indexOf(KW_OF);
if (ofLocation > 0 && valueRelativePosition >= ofLocation + KW_OF.length) {
const expressionAst = this.info.expressionParser.parseBinding(
attr.value, attr.sourceSpan.toString(), attr.sourceSpan.start.offset);
this.processExpressionCompletions(expressionAst);
} else if (binding instanceof ExpressionBinding) {
if (inSpan(this.position, binding.value?.ast.sourceSpan)) {
this.processExpressionCompletions(binding.value !.ast);
return;
} else if (!binding.value && this.position > binding.key.span.end) {
// No expression is defined for the value of the key expression binding, but the cursor is
// in a location where the expression would be defined. This can happen in a case like
// let i of |
// ^-- cursor
// In this case, backfill the value to be an empty expression and retrieve completions.
this.processExpressionCompletions(new EmptyExpr(
new ParseSpan(valueRelativePosition, valueRelativePosition),
new AbsoluteSourceSpan(this.position, this.position)));
return;
}
}
}
}

33
packages/language-service/test/completions_spec.ts
View File

@ -310,15 +310,30 @@ describe('completions', () => {
expect(completions).toBeUndefined();
});
it('should include field reference', () => {
mockHost.override(TEST_TEMPLATE, `<div *ngFor="let x of ~{cursor}"></div>`);
const marker = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'cursor');
const completions = ngLS.getCompletionsAtPosition(TEST_TEMPLATE, marker.start);
expectContain(completions, CompletionKind.PROPERTY, ['title', 'heroes', 'league']);
// the symbol 'x' declared in *ngFor is also in scope. This asserts that
// we are actually taking the AST into account and not just referring to
// the symbol table of the Component.
expectContain(completions, CompletionKind.VARIABLE, ['x']);
describe('template binding: key expression', () => {
it('should complete the RHS of a template key expression without an expression value', () => {
mockHost.override(
TEST_TEMPLATE, `<div *ngFor="let x of ~{cursor}"></div>`); // value is undefined
const marker = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'cursor');
const completions = ngLS.getCompletionsAtPosition(TEST_TEMPLATE, marker.start);
expectContain(completions, CompletionKind.PROPERTY, ['title', 'heroes', 'league']);
// the symbol 'x' declared in *ngFor is also in scope. This asserts that
// we are actually taking the AST into account and not just referring to
// the symbol table of the Component.
expectContain(completions, CompletionKind.VARIABLE, ['x']);
});
it('should complete the RHS of a template key expression with an expression value', () => {
mockHost.override(
TEST_TEMPLATE, `<div *ngFor="let x of t~{cursor}"></div>`); // value is defined
const marker = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'cursor');
const completions = ngLS.getCompletionsAtPosition(TEST_TEMPLATE, marker.start);
expectContain(completions, CompletionKind.PROPERTY, ['title', 'heroes', 'league']);
// the symbol 'x' declared in *ngFor is also in scope. This asserts that
// we are actually taking the AST into account and not just referring to
// the symbol table of the Component.
expectContain(completions, CompletionKind.VARIABLE, ['x']);
});
});
it('should include expression completions', () => {

4
packages/language-service/test/diagnostics_spec.ts
View File

@ -328,7 +328,7 @@ describe('diagnostics', () => {
it('report an unknown field in $implicit context', () => {
mockHost.override(TEST_TEMPLATE, `
<div *withContext="let myVar">
{{ ~{start-emb}myVar.missingField ~{end-emb}}}
{{ ~{start-emb}myVar.missingField~{end-emb} }}
</div>
`);
const diags = ngLS.getSemanticDiagnostics(TEST_TEMPLATE);
@ -346,7 +346,7 @@ describe('diagnostics', () => {
it('report an unknown field in non implicit context', () => {
mockHost.override(TEST_TEMPLATE, `
<div *withContext="let myVar = nonImplicitPerson">
{{ ~{start-emb}myVar.missingField ~{end-emb}}}
{{ ~{start-emb}myVar.missingField~{end-emb} }}
</div>
`);
const diags = ngLS.getSemanticDiagnostics(TEST_TEMPLATE);