From 0b366e25fc300afdbf446dd655f0463120118c3f Mon Sep 17 00:00:00 2001 From: ayazhafiz Date: Fri, 20 Dec 2019 14:26:27 -0600 Subject: [PATCH 1/3] fix(language-service): correctly parse expressions in an attribute 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 https://github.com/angular/vscode-ng-language-service/issues/523 --- packages/language-service/src/completions.ts | 11 +++++------ packages/language-service/test/completions_spec.ts | 13 +++++++++++++ .../test/project/app/parsing-cases.ts | 7 ++++++- 3 files changed, 24 insertions(+), 7 deletions(-) diff --git a/packages/language-service/src/completions.ts b/packages/language-service/src/completions.ts index 79ea098da065d..8c72fd409337f 100644 --- a/packages/language-service/src/completions.ts +++ b/packages/language-service/src/completions.ts @@ -436,12 +436,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); } } diff --git a/packages/language-service/test/completions_spec.ts b/packages/language-service/test/completions_spec.ts index e94977dd9f094..c9f9eb5454b7c 100644 --- a/packages/language-service/test/completions_spec.ts +++ b/packages/language-service/test/completions_spec.ts @@ -358,6 +358,12 @@ describe('completions', () => { expectContain(completions, CompletionKind.PROPERTY, ['test']); }); + it('should be able to complete property read', () => { + const marker = mockHost.getLocationMarkerFor(PARSING_CASES, 'data-binding-property-read'); + const completions = ngLS.getCompletionsAt(PARSING_CASES, marker.start); + expectContain(completions, CompletionKind.PROPERTY, ['key']); + }); + 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); @@ -459,6 +465,13 @@ describe('completions', () => { expectContain(completions, CompletionKind.PROPERTY, ['name', 'testEvent']); }); + it('should get reference property completions in a data binding', () => { + const marker = + mockHost.getLocationMarkerFor(PARSING_CASES, 'event-binding-ref-property-read'); + const completions = ngLS.getCompletionsAt(PARSING_CASES, 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'); diff --git a/packages/language-service/test/project/app/parsing-cases.ts b/packages/language-service/test/project/app/parsing-cases.ts index 7b2b3fe36c7af..b89f69ba7efd3 100644 --- a/packages/language-service/test/project/app/parsing-cases.ts +++ b/packages/language-service/test/project/app/parsing-cases.ts @@ -53,10 +53,13 @@ export class AttributeBinding { } @Component({ - template: '

', + template: ` +

+

`, }) export class PropertyBinding { test: string = 'test'; + obj = {key: 'value'}; } @Component({ @@ -126,6 +129,8 @@ export class AsyncForUsingComponent { {{test1.~{test-comp-after-test}name}} {{div.~{test-comp-after-div}.innerText}} + +
`, }) From 984b13d29c7e3373a3d059231f7824ddfbde5eae Mon Sep 17 00:00:00 2001 From: ayazhafiz Date: Fri, 20 Dec 2019 16:28:27 -0600 Subject: [PATCH 2/3] fixup! fix(language-service): correctly parse expressions in an attribute --- packages/language-service/src/completions.ts | 6 ++--- .../language-service/test/completions_spec.ts | 23 ++++++++++++++----- .../test/project/app/parsing-cases.ts | 7 +----- 3 files changed, 20 insertions(+), 16 deletions(-) diff --git a/packages/language-service/src/completions.ts b/packages/language-service/src/completions.ts index 8c72fd409337f..33437721f9562 100644 --- a/packages/language-service/src/completions.ts +++ b/packages/language-service/src/completions.ts @@ -537,10 +537,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); } } diff --git a/packages/language-service/test/completions_spec.ts b/packages/language-service/test/completions_spec.ts index c9f9eb5454b7c..631137677b6ef 100644 --- a/packages/language-service/test/completions_spec.ts +++ b/packages/language-service/test/completions_spec.ts @@ -308,6 +308,13 @@ describe('completions', () => { expectContain(completions, CompletionKind.VARIABLE, ['x']); }); + it('should include expression completions', () => { + mockHost.override(TEST_TEMPLATE, `
`); + 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, `
@@ -359,9 +366,10 @@ describe('completions', () => { }); it('should be able to complete property read', () => { - const marker = mockHost.getLocationMarkerFor(PARSING_CASES, 'data-binding-property-read'); - const completions = ngLS.getCompletionsAt(PARSING_CASES, marker.start); - expectContain(completions, CompletionKind.PROPERTY, ['key']); + mockHost.override(TEST_TEMPLATE, `

`); + 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', () => { @@ -466,9 +474,12 @@ describe('completions', () => { }); it('should get reference property completions in a data binding', () => { - const marker = - mockHost.getLocationMarkerFor(PARSING_CASES, 'event-binding-ref-property-read'); - const completions = ngLS.getCompletionsAt(PARSING_CASES, marker.start); + mockHost.override(TEST_TEMPLATE, ` + +
+ `); + const marker = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'property-read'); + const completions = ngLS.getCompletionsAt(TEST_TEMPLATE, marker.start); expectContain(completions, CompletionKind.PROPERTY, ['name', 'testEvent']); }); diff --git a/packages/language-service/test/project/app/parsing-cases.ts b/packages/language-service/test/project/app/parsing-cases.ts index b89f69ba7efd3..7b2b3fe36c7af 100644 --- a/packages/language-service/test/project/app/parsing-cases.ts +++ b/packages/language-service/test/project/app/parsing-cases.ts @@ -53,13 +53,10 @@ export class AttributeBinding { } @Component({ - template: ` -

-

`, + template: '

', }) export class PropertyBinding { test: string = 'test'; - obj = {key: 'value'}; } @Component({ @@ -129,8 +126,6 @@ export class AsyncForUsingComponent { {{test1.~{test-comp-after-test}name}} {{div.~{test-comp-after-div}.innerText}} - -
`, }) From 333982aa830d337599dad86cbe0397555ef77dcd Mon Sep 17 00:00:00 2001 From: ayazhafiz Date: Fri, 20 Dec 2019 23:12:22 -0600 Subject: [PATCH 3/3] fixup! fix(language-service): correctly parse expressions in an attribute --- packages/language-service/src/completions.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/language-service/src/completions.ts b/packages/language-service/src/completions.ts index 33437721f9562..845dba5d97db7 100644 --- a/packages/language-service/src/completions.ts +++ b/packages/language-service/src/completions.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