Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(language-service): incorrect autocomplete results on unknown symbol #37518

Closed
wants to merge 1 commit into from
Closed

fix(language-service): incorrect autocomplete results on unknown symbol #37518

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
158 changes: 82 additions & 76 deletions packages/language-service/src/completions.ts
View file
Open in desktop
Expand Up @@ -6,7 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/

import {AbsoluteSourceSpan, AST, AstPath, AttrAst, Attribute, BoundDirectivePropertyAst, BoundElementPropertyAst, BoundEventAst, BoundTextAst, Element, ElementAst, EmptyExpr, ExpressionBinding, getHtmlTagDefinition, HtmlAstPath, Node as HtmlAst, NullTemplateVisitor, ParseSpan, ReferenceAst, TagContentType, TemplateBinding, Text, VariableBinding} from '@angular/compiler';
import {AbsoluteSourceSpan, AST, AstPath, AttrAst, Attribute, BoundDirectivePropertyAst, BoundElementPropertyAst, BoundEventAst, BoundTextAst, Element, ElementAst, EmptyExpr, ExpressionBinding, getHtmlTagDefinition, HtmlAstPath, Node as HtmlAst, NullTemplateVisitor, ParseSpan, ReferenceAst, TagContentType, TemplateBinding, Text, VariableBinding, Visitor} from '@angular/compiler';
import {$$, $_, isAsciiLetter, isDigit} from '@angular/compiler/src/chars';

import {ATTR, getBindingDescriptor} from './binding_utils';
Expand Down Expand Up @@ -127,79 +127,97 @@ function getBoundedWordSpan(

export function getTemplateCompletions(
templateInfo: ng.AstResult, position: number): ng.CompletionEntry[] {
let result: ng.CompletionEntry[] = [];
const {htmlAst, template} = templateInfo;
// The templateNode starts at the delimiter character so we add 1 to skip it.
// Calculate the position relative to the start of the template. This is needed
// because spans in HTML AST are relative. Inline template has non-zero start position.
const templatePosition = position - template.span.start;
const path = getPathToNodeAtPosition(htmlAst, templatePosition);
const mostSpecific = path.tail;
if (path.empty || !mostSpecific) {
result = elementCompletions(templateInfo);
} else {
const astPosition = templatePosition - mostSpecific.sourceSpan.start.offset;
mostSpecific.visit(
{
visitElement(ast) {
const startTagSpan = spanOf(ast.sourceSpan);
const tagLen = ast.name.length;
// + 1 for the opening angle bracket
if (templatePosition <= startTagSpan.start + tagLen + 1) {
// If we are in the tag then return the element completions.
result = elementCompletions(templateInfo);
} else if (templatePosition < startTagSpan.end) {
// We are in the attribute section of the element (but not in an attribute).
// Return the attribute completions.
result = attributeCompletionsForElement(templateInfo, ast.name);
}
},
visitAttribute(ast: Attribute) {
// An attribute consists of two parts, LHS="RHS".
// Determine if completions are requested for LHS or RHS
if (ast.valueSpan && inSpan(templatePosition, spanOf(ast.valueSpan))) {
// RHS completion
result = attributeValueCompletions(templateInfo, path);
} else {
// LHS completion
result = attributeCompletions(templateInfo, path);
}
},
visitText(ast) {
result = interpolationCompletions(templateInfo, templatePosition);
if (result.length) return result;
const element = path.first(Element);
if (element) {
const definition = getHtmlTagDefinition(element.name);
if (definition.contentType === TagContentType.PARSABLE_DATA) {
result = voidElementAttributeCompletions(templateInfo, path);
if (!result.length) {
// If the element can hold content, show element completions.
result = elementCompletions(templateInfo);
}
}
} else {
// If no element container, implies parsable data so show elements.
result = voidElementAttributeCompletions(templateInfo, path);
if (!result.length) {
result = elementCompletions(templateInfo);
}
}
},
visitComment() {},
visitExpansion() {},
visitExpansionCase() {}
},
null);
}

const htmlPath: HtmlAstPath = getPathToNodeAtPosition(htmlAst, templatePosition);
const mostSpecific = htmlPath.tail;
const visitor = new HtmlVisitor(templateInfo, htmlPath);
const results: ng.CompletionEntry[] = mostSpecific ?
mostSpecific.visit(visitor, null /* context */) :
elementCompletions(templateInfo);
const replacementSpan = getBoundedWordSpan(templateInfo, position, mostSpecific);
return result.map(entry => {
return results.map(entry => {
return {
...entry,
replacementSpan,
};
});
}

class HtmlVisitor implements Visitor {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole section is mostly a refactoring, except for visitText(), where I've moved the logic around a little.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(meta point about walkers, not actionable but you might find this interesting)
Angular's AST walkers as classes might be an anti-pattern; see the TypeScript lead's issue on TSLint regarding their walkers of the TS AST: palantir/tslint#2522

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially I implemented the visitor as a plain object:

function createVisitor(): Visitor {
  return {
    // ...
    visitText() { /* ... */ },
    // ...
  };
}

But it masked a bug that was not caught by the type system. This is because Angular's own typing for Visitor is not great:

angular/packages/compiler/src/ml_parser/ast.ts

Lines 83 to 94 in 20346ff

export interface Visitor {
// Returning a truthy value from `visit()` will prevent `visitAll()` from the call to the typed
// method and result returned will become the result included in `visitAll()`s result array.
visit?(node: Node, context: any): any;
visitElement(element: Element, context: any): any;
visitAttribute(attribute: Attribute, context: any): any;
visitText(text: Text, context: any): any;
visitComment(comment: Comment, context: any): any;
visitExpansion(expansion: Expansion, context: any): any;
visitExpansionCase(expansionCase: ExpansionCase, context: any): any;
}

In this use case, all methods need to explicitly return ng.CompletionEntry[], but visitComment() et al. return undefined. It'd cause a crash at runtime.
I wanted a way to implement the Visitor interface yet I don't want to inherit the default signatures. In the end I thought implementing it as a class is the best compromise.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, your solution is better. The issue I linked to basically says the problem is with classes in the first place, so the visitor classes in the compiler package would have to be reconsidered

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah I see. So essentially a imperative visitor with if-else statements is more performant than a class-based one.
I think that is still achievable here with the .visit() method. I'll definitely keep this in mind when working on Ivy LS.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for discussion sake, if we go with the if-else approach, don't we lose the benefits provided by the type system? The type system guarantees that every "branch" has an appropriate handler/visitor.
Maybe a switch statement could provide the same benefit.

/**
* Position relative to the start of the template.
*/
private readonly relativePosition: number;
constructor(private readonly templateInfo: ng.AstResult, private readonly htmlPath: HtmlAstPath) {
this.relativePosition = htmlPath.position;
}
// Note that every visitor method must explicitly specify return type because
// Visitor returns `any` for all methods.
visitElement(ast: Element): ng.CompletionEntry[] {
const startTagSpan = spanOf(ast.sourceSpan);
const tagLen = ast.name.length;
// + 1 for the opening angle bracket
if (this.relativePosition <= startTagSpan.start + tagLen + 1) {
// If we are in the tag then return the element completions.
return elementCompletions(this.templateInfo);
}
if (this.relativePosition < startTagSpan.end) {
// We are in the attribute section of the element (but not in an attribute).
// Return the attribute completions.
return attributeCompletionsForElement(this.templateInfo, ast.name);
}
return [];
}
visitAttribute(ast: Attribute): ng.CompletionEntry[] {
// An attribute consists of two parts, LHS="RHS".
// Determine if completions are requested for LHS or RHS
if (ast.valueSpan && inSpan(this.relativePosition, spanOf(ast.valueSpan))) {
// RHS completion
return attributeValueCompletions(this.templateInfo, this.htmlPath);
}
// LHS completion
return attributeCompletions(this.templateInfo, this.htmlPath);
}
visitText(): ng.CompletionEntry[] {
const templatePath = findTemplateAstAt(this.templateInfo.templateAst, this.relativePosition);
if (templatePath.tail instanceof BoundTextAst) {
// If we know that this is an interpolation then do not try other scenarios.
const visitor = new ExpressionVisitor(
this.templateInfo, this.relativePosition,
() =>
getExpressionScope(diagnosticInfoFromTemplateInfo(this.templateInfo), templatePath));
templatePath.tail?.visit(visitor, null);
return visitor.results;
}
// TODO(kyliau): Not sure if this check is really needed since we don't have
// any test cases for it.
const element = this.htmlPath.first(Element);
if (element &&
getHtmlTagDefinition(element.name).contentType !== TagContentType.PARSABLE_DATA) {
return [];
}
// This is to account for cases like <h1> <a> text | </h1> where the
// closest element has no closing tag and thus is considered plain text.
const results = voidElementAttributeCompletions(this.templateInfo, this.htmlPath);
if (results.length) {
return results;
}
return elementCompletions(this.templateInfo);
}
visitComment(): ng.CompletionEntry[] {
return [];
}
visitExpansion(): ng.CompletionEntry[] {
return [];
}
visitExpansionCase(): ng.CompletionEntry[] {
return [];
}
}

function attributeCompletions(info: ng.AstResult, path: AstPath<HtmlAst>): ng.CompletionEntry[] {
const attr = path.tail;
const elem = path.parentOf(attr);
Expand Down Expand Up @@ -356,18 +374,6 @@ function elementCompletions(info: ng.AstResult): ng.CompletionEntry[] {
return results;
}

function interpolationCompletions(info: ng.AstResult, position: number): ng.CompletionEntry[] {
// Look for an interpolation in at the position.
const templatePath = findTemplateAstAt(info.templateAst, position);
if (!templatePath.tail) {
return [];
}
const visitor = new ExpressionVisitor(
info, position, () => getExpressionScope(diagnosticInfoFromTemplateInfo(info), templatePath));
templatePath.tail.visit(visitor, null);
return visitor.results;
}

// There is a special case of HTML where text that contains a unclosed tag is treated as
// text. For exaple '<h1> Some <a text </h1>' produces a text nodes inside of the H1
// element "Some <a text". We, however, want to treat this as if the user was requesting
Expand Down
7 changes: 7 additions & 0 deletions packages/language-service/test/completions_spec.ts
View file
Open in desktop
Expand Up @@ -841,6 +841,13 @@ describe('completions', () => {
'trim',
]);
});

it('should not return any results for unknown symbol', () => {
mockHost.override(TEST_TEMPLATE, '{{ doesnotexist.~{cursor} }}');
const marker = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'cursor');
const completions = ngLS.getCompletionsAtPosition(TEST_TEMPLATE, marker.start);
expect(completions).toBeUndefined();
});
kyliau marked this conversation as resolved.
Show resolved Hide resolved
});

function expectContain(
Expand Down