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

[kyliau]

This commit fixes a bug whereby the language service would incorrectly
return HTML elements if autocomplete is requested for an unknown symbol.
This is because we walk through every possible scenario, and fallback to
element autocomplete if none of the scenarios match. See screenshot below.

The fix here is to return results only for interpolation if we know for sure
we are in a bound text. This means we will now return an empty results if
there is no suggestions.

This commit also refactors the code a little to make it easier to
understand.

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

• Bugfix
• Feature
• Code style update (formatting, local variables)
• Refactoring (no functional changes, no api changes)
• Build related changes
• CI related changes
• Documentation content changes
• angular.io application / infrastructure changes
• Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

• Yes
• No

Other information

[kyliau]

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

[ayazhafiz]

(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

[kyliau]

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.

[ayazhafiz]

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

[kyliau]

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.

[kyliau]

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.

[ayazhafiz]

(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

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}