Skip to content
Permalink
Browse files

fix(language-service): correctly parse expressions in an attribute (#…

…34517)

Currently, the language service provides completions in a template node
attribute by first checking if the attribute contains template bindings
to provide completions for, and then providing completions for the
expression in the attribute.

In the latter case, the expression AST was being constructed
"synthetically" inside the language service, in particular declaring the
expression to be a `PropertyRead` with an implicit receiver.
Unfortunately, this AST can be incorrect if the expression is actually a
property read on a component property receiver (e.g. when reading
`key` in the expression `obj.key`, `obj` is the receiver).

The fix is pretty simple - rather than a synthetic construction of the
AST, ask the expression parser to parse the expression in the attribute.

Fixes angular/vscode-ng-language-service#523

PR Close #34517
  • Loading branch information
ayazhafiz authored and alxhub committed Dec 20, 2019
1 parent 1660095 commit 7a0d6e7cba634e8f6346a258907962ba3a2f92cb
Showing with 32 additions and 13 deletions.
  1. +8 −13 packages/language-service/src/completions.ts
  2. +24 −0 packages/language-service/test/completions_spec.ts
@@ -419,11 +419,9 @@ class ExpressionVisitor extends NullTemplateVisitor {
}

visitAttr(ast: AttrAst) {
// The attribute value is a template expression but the expression AST
// was not produced when the TemplateAst was produced so do that here.
// First, verify the attribute consists of some binding we can give completions for.
const {templateBindings} = this.info.expressionParser.parseTemplateBindings(
ast.name, ast.value, ast.sourceSpan.toString(), ast.sourceSpan.start.offset);

// Find where the cursor is relative to the start of the attribute value.
const valueRelativePosition = this.position - ast.sourceSpan.start.offset;
// Find the template binding that contains the position
@@ -436,12 +434,11 @@ class ExpressionVisitor extends NullTemplateVisitor {
if (ast.name.startsWith('*')) {
this.microSyntaxInAttributeValue(ast, binding);
} else {
// If the position is in the expression or after the key or there is no key,
// return the expression completions
const span = new ParseSpan(0, ast.value.length);
const offset = ast.sourceSpan.start.offset;
const receiver = new ImplicitReceiver(span, span.toAbsolute(offset));
const expressionAst = new PropertyRead(span, span.toAbsolute(offset), receiver, '');
// If the position is in the expression or after the key or there is no key, return the
// expression completions.
// The expression must be reparsed to get a valid AST rather than only template bindings.
const expressionAst = this.info.expressionParser.parseBinding(
ast.value, ast.sourceSpan.toString(), ast.sourceSpan.start.offset);
this.addAttributeValuesToCompletions(expressionAst);
}
}
@@ -542,10 +539,8 @@ class ExpressionVisitor extends NullTemplateVisitor {
const KW_OF = ' of ';
const ofLocation = attr.value.indexOf(KW_OF);
if (ofLocation > 0 && valueRelativePosition >= ofLocation + KW_OF.length) {
const span = new ParseSpan(0, attr.value.length);
const offset = attr.sourceSpan.start.offset;
const receiver = new ImplicitReceiver(span, span.toAbsolute(offset));
const expressionAst = new PropertyRead(span, span.toAbsolute(offset), receiver, '');
const expressionAst = this.info.expressionParser.parseBinding(
attr.value, attr.sourceSpan.toString(), attr.sourceSpan.start.offset);
this.addAttributeValuesToCompletions(expressionAst);
}
}
@@ -320,6 +320,13 @@ describe('completions', () => {
expectContain(completions, CompletionKind.VARIABLE, ['x']);
});

it('should include expression completions', () => {
mockHost.override(TEST_TEMPLATE, `<div *ngFor="let x of hero.~{expr-property-read}"></div>`);
const marker = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'expr-property-read');
const completions = ngLS.getCompletionsAt(TEST_TEMPLATE, marker.start);
expectContain(completions, CompletionKind.PROPERTY, ['name']);
});

it('should include variable in the let scope in interpolation', () => {
mockHost.override(TEST_TEMPLATE, `
<div *ngFor="let h of heroes">
@@ -370,6 +377,13 @@ describe('completions', () => {
expectContain(completions, CompletionKind.PROPERTY, ['test']);
});

it('should be able to complete property read', () => {
mockHost.override(TEST_TEMPLATE, `<h1 [model]="hero.~{property-read}"></h1>`);
const marker = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'property-read');
const completions = ngLS.getCompletionsAt(TEST_TEMPLATE, marker.start);
expectContain(completions, CompletionKind.PROPERTY, ['id', 'name']);
});

it('should be able to complete an event', () => {
const marker = mockHost.getLocationMarkerFor(PARSING_CASES, 'event-binding-model');
const completions = ngLS.getCompletionsAt(PARSING_CASES, marker.start);
@@ -471,6 +485,16 @@ describe('completions', () => {
expectContain(completions, CompletionKind.PROPERTY, ['name', 'testEvent']);
});

it('should get reference property completions in a data binding', () => {
mockHost.override(TEST_TEMPLATE, `
<test-comp #test></test-comp>
<div (click)="test.~{property-read}"></div>
`);
const marker = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'property-read');
const completions = ngLS.getCompletionsAt(TEST_TEMPLATE, marker.start);
expectContain(completions, CompletionKind.PROPERTY, ['name', 'testEvent']);
});

// TODO: Enable when we have a flag that indicates the project targets the DOM
// it('should reference the element if no component', () => {
// const marker = mockHost.getLocationMarkerFor(PARSING_CASES, 'test-comp-after-div');

0 comments on commit 7a0d6e7

Please sign in to comment.
You can’t perform that action at this time.