From f6e88cd65945d8abead74d322caf0d93b9743062 Mon Sep 17 00:00:00 2001 From: Keen Yee Liau Date: Wed, 28 Aug 2019 10:18:18 -0700 Subject: [PATCH] fix(language-service): Use ts.CompletionEntry for completions (#32375) This is a prerequisite to fix a bug in template completions whereby completion of the string `ti` for the variable `title` results in `tititle`. This is because the position where the completion is requested is used to insert the completion text. This is incorrect. Instead, a `replacementSpan` should be used to indicate the span of text that needs to be replaced. Angular's own `Completion` interface is insufficient to hold this information. Instead, we should just use ts.CompletionEntry. Also added string enum for `CompletionKind`, which is similar to ts.ScriptElementKind but contains more info about HTML entities. PR Close #32375 --- .../goldens/completionInfo.json | 44 +---- packages/language-service/src/completions.ts | 184 +++++++++++------- .../language-service/src/language_service.ts | 4 +- packages/language-service/src/types.ts | 18 ++ packages/language-service/src/utils.ts | 17 -- .../language-service/test/completions_spec.ts | 5 +- 6 files changed, 138 insertions(+), 134 deletions(-) diff --git a/integration/language_service_plugin/goldens/completionInfo.json b/integration/language_service_plugin/goldens/completionInfo.json index 3c503000c84c45..14ea173b55b533 100644 --- a/integration/language_service_plugin/goldens/completionInfo.json +++ b/integration/language_service_plugin/goldens/completionInfo.json @@ -12,255 +12,213 @@ { "name": "anchor", "kind": "method", - "kindModifiers": "", "sortText": "anchor" }, { "name": "big", "kind": "method", - "kindModifiers": "", "sortText": "big" }, { "name": "blink", "kind": "method", - "kindModifiers": "", "sortText": "blink" }, { "name": "bold", "kind": "method", - "kindModifiers": "", "sortText": "bold" }, { "name": "charAt", "kind": "method", - "kindModifiers": "", "sortText": "charAt" }, { "name": "charCodeAt", "kind": "method", - "kindModifiers": "", "sortText": "charCodeAt" }, { "name": "codePointAt", "kind": "method", - "kindModifiers": "", "sortText": "codePointAt" }, { "name": "concat", "kind": "method", - "kindModifiers": "", "sortText": "concat" }, { "name": "endsWith", "kind": "method", - "kindModifiers": "", "sortText": "endsWith" }, { "name": "fixed", "kind": "method", - "kindModifiers": "", "sortText": "fixed" }, { "name": "fontcolor", "kind": "method", - "kindModifiers": "", "sortText": "fontcolor" }, { "name": "fontsize", "kind": "method", - "kindModifiers": "", "sortText": "fontsize" }, { "name": "includes", "kind": "method", - "kindModifiers": "", "sortText": "includes" }, { "name": "indexOf", "kind": "method", - "kindModifiers": "", "sortText": "indexOf" }, { "name": "italics", "kind": "method", - "kindModifiers": "", "sortText": "italics" }, { "name": "lastIndexOf", "kind": "method", - "kindModifiers": "", "sortText": "lastIndexOf" }, { "name": "length", "kind": "property", - "kindModifiers": "", "sortText": "length" }, { "name": "link", "kind": "method", - "kindModifiers": "", "sortText": "link" }, { "name": "localeCompare", "kind": "method", - "kindModifiers": "", "sortText": "localeCompare" }, { "name": "match", "kind": "method", - "kindModifiers": "", "sortText": "match" }, { "name": "normalize", "kind": "method", - "kindModifiers": "", "sortText": "normalize" }, { "name": "repeat", "kind": "method", - "kindModifiers": "", "sortText": "repeat" }, { "name": "replace", "kind": "method", - "kindModifiers": "", "sortText": "replace" }, { "name": "search", "kind": "method", - "kindModifiers": "", "sortText": "search" }, { "name": "slice", "kind": "method", - "kindModifiers": "", "sortText": "slice" }, { "name": "small", "kind": "method", - "kindModifiers": "", "sortText": "small" }, { "name": "split", "kind": "method", - "kindModifiers": "", "sortText": "split" }, { "name": "startsWith", "kind": "method", - "kindModifiers": "", "sortText": "startsWith" }, { "name": "strike", "kind": "method", - "kindModifiers": "", "sortText": "strike" }, { "name": "sub", "kind": "method", - "kindModifiers": "", "sortText": "sub" }, { "name": "substr", "kind": "method", - "kindModifiers": "", "sortText": "substr" }, { "name": "substring", "kind": "method", - "kindModifiers": "", "sortText": "substring" }, { "name": "sup", "kind": "method", - "kindModifiers": "", "sortText": "sup" }, { "name": "toLocaleLowerCase", "kind": "method", - "kindModifiers": "", "sortText": "toLocaleLowerCase" }, { "name": "toLocaleUpperCase", "kind": "method", - "kindModifiers": "", "sortText": "toLocaleUpperCase" }, { "name": "toLowerCase", "kind": "method", - "kindModifiers": "", "sortText": "toLowerCase" }, { "name": "toString", "kind": "method", - "kindModifiers": "", "sortText": "toString" }, { "name": "toUpperCase", "kind": "method", - "kindModifiers": "", "sortText": "toUpperCase" }, { "name": "trim", "kind": "method", - "kindModifiers": "", "sortText": "trim" }, { "name": "trimLeft", "kind": "method", - "kindModifiers": "", "sortText": "trimLeft" }, { "name": "trimRight", "kind": "method", - "kindModifiers": "", "sortText": "trimRight" }, { "name": "valueOf", "kind": "method", - "kindModifiers": "", "sortText": "valueOf" } ] } -} \ No newline at end of file +} diff --git a/packages/language-service/src/completions.ts b/packages/language-service/src/completions.ts index de12e06c9f317e..f9366be2062dc9 100644 --- a/packages/language-service/src/completions.ts +++ b/packages/language-service/src/completions.ts @@ -12,8 +12,8 @@ import {getExpressionScope} from '@angular/compiler-cli/src/language_services'; import {AstResult, AttrInfo} from './common'; import {getExpressionCompletions} from './expressions'; import {attributeNames, elementNames, eventNames, propertyNames} from './html_info'; -import {Completion, Completions, Span, Symbol, SymbolTable, TemplateSource} from './types'; -import {diagnosticInfoFromTemplateInfo, findTemplateAstAt, flatten, getSelectors, hasTemplateReference, inSpan, removeSuffix, spanOf, uniqueByName} from './utils'; +import {CompletionKind, Span, Symbol, SymbolTable, TemplateSource} from './types'; +import {diagnosticInfoFromTemplateInfo, findTemplateAstAt, flatten, getSelectors, hasTemplateReference, inSpan, removeSuffix, spanOf} from './utils'; const TEMPLATE_ATTR_PREFIX = '*'; @@ -28,9 +28,9 @@ const hiddenHtmlElements = { link: true, }; -export function getTemplateCompletions(templateInfo: AstResult, position: number): Completions| - undefined { - let result: Completions|undefined = undefined; +export function getTemplateCompletions( + templateInfo: AstResult, position: number): ts.CompletionEntry[] { + let result: ts.CompletionEntry[] = []; let {htmlAst, template} = templateInfo; // The templateNode starts at the delimiter character so we add 1 to skip it. let templatePosition = position - template.span.start; @@ -45,8 +45,8 @@ export function getTemplateCompletions(templateInfo: AstResult, position: number visitElement(ast) { let startTagSpan = spanOf(ast.sourceSpan); let tagLen = ast.name.length; - if (templatePosition <= - startTagSpan.start + tagLen + 1 /* 1 for the opening angle bracket */) { + // + 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, path); } else if (templatePosition < startTagSpan.end) { @@ -66,15 +66,15 @@ export function getTemplateCompletions(templateInfo: AstResult, position: number visitText(ast) { // Check if we are in a entity. result = entityCompletions(getSourceText(template, spanOf(ast)), astPosition); - if (result) return result; + if (result.length) return result; result = interpolationCompletions(templateInfo, templatePosition); - if (result) return result; + if (result.length) return result; let element = path.first(Element); if (element) { let definition = getHtmlTagDefinition(element.name); if (definition.contentType === TagContentType.PARSABLE_DATA) { result = voidElementAttributeCompletions(templateInfo, path); - if (!result) { + if (!result.length) { // If the element can hold content, show element completions. result = elementCompletions(templateInfo, path); } @@ -82,7 +82,7 @@ export function getTemplateCompletions(templateInfo: AstResult, position: number } else { // If no element container, implies parsable data so show elements. result = voidElementAttributeCompletions(templateInfo, path); - if (!result) { + if (!result.length) { result = elementCompletions(templateInfo, path); } } @@ -96,24 +96,29 @@ export function getTemplateCompletions(templateInfo: AstResult, position: number return result; } -function attributeCompletions(info: AstResult, path: AstPath): Completions|undefined { +function attributeCompletions(info: AstResult, path: AstPath): ts.CompletionEntry[] { let item = path.tail instanceof Element ? path.tail : path.parentOf(path.tail); if (item instanceof Element) { return attributeCompletionsForElement(info, item.name, item); } - return undefined; + return []; } function attributeCompletionsForElement( - info: AstResult, elementName: string, element?: Element): Completions { + info: AstResult, elementName: string, element?: Element): ts.CompletionEntry[] { const attributes = getAttributeInfosForElement(info, elementName, element); // Map all the attributes to a completion - return attributes.map(attr => ({ - kind: attr.fromHtml ? 'html attribute' : 'attribute', - name: nameOfAttr(attr), - sort: attr.name - })); + return attributes.map(attr => { + const kind = attr.fromHtml ? CompletionKind.HTML_ATTRIBUTE : CompletionKind.ATTRIBUTE; + return { + name: nameOfAttr(attr), + // Need to cast to unknown because Angular's CompletionKind includes HTML + // entites. + kind: kind as unknown as ts.ScriptElementKind, + sortText: attr.name, + }; + }); } function getAttributeInfosForElement( @@ -188,30 +193,30 @@ function getAttributeInfosForElement( return attributes; } -function attributeValueCompletions(info: AstResult, position: number, attr: Attribute): Completions| - undefined { +function attributeValueCompletions( + info: AstResult, position: number, attr: Attribute): ts.CompletionEntry[] { const path = findTemplateAstAt(info.templateAst, position); - const mostSpecific = path.tail; + if (!path.tail) { + return []; + } const dinfo = diagnosticInfoFromTemplateInfo(info); - if (mostSpecific) { - const visitor = - new ExpressionVisitor(info, position, attr, () => getExpressionScope(dinfo, path, false)); - mostSpecific.visit(visitor, null); - if (!visitor.result || !visitor.result.length) { - // Try allwoing widening the path - const widerPath = findTemplateAstAt(info.templateAst, position, /* allowWidening */ true); - if (widerPath.tail) { - const widerVisitor = new ExpressionVisitor( - info, position, attr, () => getExpressionScope(dinfo, widerPath, false)); - widerPath.tail.visit(widerVisitor, null); - return widerVisitor.result; - } + const visitor = + new ExpressionVisitor(info, position, attr, () => getExpressionScope(dinfo, path, false)); + path.tail.visit(visitor, null); + if (!visitor.result || !visitor.result.length) { + // Try allwoing widening the path + const widerPath = findTemplateAstAt(info.templateAst, position, /* allowWidening */ true); + if (widerPath.tail) { + const widerVisitor = new ExpressionVisitor( + info, position, attr, () => getExpressionScope(dinfo, widerPath, false)); + widerPath.tail.visit(widerVisitor, null); + return widerVisitor.result || []; } - return visitor.result; } + return visitor.result || []; } -function elementCompletions(info: AstResult, path: AstPath): Completions|undefined { +function elementCompletions(info: AstResult, path: AstPath): ts.CompletionEntry[] { let htmlNames = elementNames().filter(name => !(name in hiddenHtmlElements)); // Collect the elements referenced by the selectors @@ -219,41 +224,79 @@ function elementCompletions(info: AstResult, path: AstPath): Completion .selectors.map(selector => selector.element) .filter(name => !!name) as string[]; - let components = - directiveElements.map(name => ({kind: 'component', name, sort: name})); - let htmlElements = htmlNames.map(name => ({kind: 'element', name: name, sort: name})); + let components = directiveElements.map(name => { + return { + name, + // Need to cast to unknown because Angular's CompletionKind includes HTML + // entites. + kind: CompletionKind.COMPONENT as unknown as ts.ScriptElementKind, + sortText: name, + }; + }); + let htmlElements = htmlNames.map(name => { + return { + name, + // Need to cast to unknown because Angular's CompletionKind includes HTML + // entites. + kind: CompletionKind.ELEMENT as unknown as ts.ScriptElementKind, + sortText: name, + }; + }); // Return components and html elements return uniqueByName(htmlElements.concat(components)); } -function entityCompletions(value: string, position: number): Completions|undefined { +/** + * Filter the specified `entries` by unique name. + * @param entries Completion Entries + */ +function uniqueByName(entries: ts.CompletionEntry[]) { + const results = []; + const set = new Set(); + for (const entry of entries) { + if (!set.has(entry.name)) { + set.add(entry.name); + results.push(entry); + } + } + return results; +} + +function entityCompletions(value: string, position: number): ts.CompletionEntry[] { // Look for entity completions const re = /&[A-Za-z]*;?(?!\d)/g; let found: RegExpExecArray|null; - let result: Completions|undefined = undefined; + let result: ts.CompletionEntry[] = []; while (found = re.exec(value)) { let len = found[0].length; if (position >= found.index && position < (found.index + len)) { - result = Object.keys(NAMED_ENTITIES) - .map(name => ({kind: 'entity', name: `&${name};`, sort: name})); + result = Object.keys(NAMED_ENTITIES).map(name => { + return { + name: `&${name};`, + // Need to cast to unknown because Angular's CompletionKind includes + // HTML entites. + kind: CompletionKind.ENTITY as unknown as ts.ScriptElementKind, + sortText: name, + }; + }); break; } } return result; } -function interpolationCompletions(info: AstResult, position: number): Completions|undefined { +function interpolationCompletions(info: AstResult, position: number): ts.CompletionEntry[] { // Look for an interpolation in at the position. const templatePath = findTemplateAstAt(info.templateAst, position); - const mostSpecific = templatePath.tail; - if (mostSpecific) { - let visitor = new ExpressionVisitor( - info, position, undefined, - () => getExpressionScope(diagnosticInfoFromTemplateInfo(info), templatePath, false)); - mostSpecific.visit(visitor, null); - return uniqueByName(visitor.result); + if (!templatePath.tail) { + return []; } + let visitor = new ExpressionVisitor( + info, position, undefined, + () => getExpressionScope(diagnosticInfoFromTemplateInfo(info), templatePath, false)); + templatePath.tail.visit(visitor, null); + return uniqueByName(visitor.result || []); } // There is a special case of HTML where text that contains a unclosed tag is treated as @@ -262,8 +305,8 @@ function interpolationCompletions(info: AstResult, position: number): Completion // the attributes of an "a" element, not requesting completion in the a text element. This // code checks for this case and returns element completions if it is detected or undefined // if it is not. -function voidElementAttributeCompletions(info: AstResult, path: AstPath): Completions| - undefined { +function voidElementAttributeCompletions( + info: AstResult, path: AstPath): ts.CompletionEntry[] { let tail = path.tail; if (tail instanceof Text) { let match = tail.value.match(/<(\w(\w|\d|-)*:)?(\w(\w|\d|-)*)\s/); @@ -274,11 +317,12 @@ function voidElementAttributeCompletions(info: AstResult, path: AstPath return attributeCompletionsForElement(info, match[3]); } } + return []; } class ExpressionVisitor extends NullTemplateVisitor { private getExpressionScope: () => SymbolTable; - result: Completion[]|undefined; + result: ts.CompletionEntry[]|undefined; constructor( private info: AstResult, private position: number, private attr?: Attribute, @@ -331,7 +375,15 @@ class ExpressionVisitor extends NullTemplateVisitor { .map(name => lowerName(name.substr(key.length))); } keys.push('let'); - this.result = keys.map(key => {kind: 'key', name: key, sort: key}); + this.result = keys.map(key => { + return { + name: key, + // Need to cast to unknown because Angular's CompletionKind includes + // HTML entites. + kind: CompletionKind.KEY as unknown as ts.ScriptElementKind, + sortText: key, + }; + }); }; if (!binding || (binding.key == key && !binding.expression)) { @@ -394,9 +446,14 @@ class ExpressionVisitor extends NullTemplateVisitor { } } - private symbolsToCompletions(symbols: Symbol[]): Completions { - return symbols.filter(s => !s.name.startsWith('__') && s.public) - .map(symbol => {kind: symbol.kind, name: symbol.name, sort: symbol.name}); + private symbolsToCompletions(symbols: Symbol[]): ts.CompletionEntry[] { + return symbols.filter(s => !s.name.startsWith('__') && s.public).map(symbol => { + return { + name: symbol.name, + kind: symbol.kind as ts.ScriptElementKind, + sortText: symbol.name, + }; + }); } private get attributeValuePosition() { @@ -497,12 +554,3 @@ function expandedAttr(attr: AttrInfo): AttrInfo[] { function lowerName(name: string): string { return name && (name[0].toLowerCase() + name.substr(1)); } - -export function ngCompletionToTsCompletionEntry(completion: Completion): ts.CompletionEntry { - return { - name: completion.name, - kind: completion.kind as ts.ScriptElementKind, - kindModifiers: '', - sortText: completion.sort, - }; -} diff --git a/packages/language-service/src/language_service.ts b/packages/language-service/src/language_service.ts index 35b48ca4d96255..d9c24d2078624d 100644 --- a/packages/language-service/src/language_service.ts +++ b/packages/language-service/src/language_service.ts @@ -9,7 +9,7 @@ import * as tss from 'typescript/lib/tsserverlibrary'; import {isAstResult} from './common'; -import {getTemplateCompletions, ngCompletionToTsCompletionEntry} from './completions'; +import {getTemplateCompletions} from './completions'; import {getDefinitionAndBoundSpan, getTsDefinitionAndBoundSpan} from './definitions'; import {getDeclarationDiagnostics, getTemplateDiagnostics, ngDiagnosticToTsDiagnostic, uniqueBySpan} from './diagnostics'; import {getHover} from './hover'; @@ -70,7 +70,7 @@ class LanguageServiceImpl implements LanguageService { isGlobalCompletion: false, isMemberCompletion: false, isNewIdentifierLocation: false, - entries: results.map(ngCompletionToTsCompletionEntry), + entries: results, }; } diff --git a/packages/language-service/src/types.ts b/packages/language-service/src/types.ts index 45e5d954c20231..0d64630209fbba 100644 --- a/packages/language-service/src/types.ts +++ b/packages/language-service/src/types.ts @@ -258,6 +258,24 @@ export enum DirectiveKind { EVENT = 'event', } +/** + * ScriptElementKind for completion. + */ +export enum CompletionKind { + ATTRIBUTE = 'attribute', + COMPONENT = 'component', + ELEMENT = 'element', + ENTITY = 'entity', + HTML_ATTRIBUTE = 'html attribute', + KEY = 'key', + METHOD = 'method', + PIPE = 'pipe', + PROPERTY = 'property', + REFERENCE = 'reference', + TYPE = 'type', + VARIABLE = 'variable', +} + /** * A template diagnostics message chain. This is similar to the TypeScript * DiagnosticMessageChain. The messages are intended to be formatted as separate diff --git a/packages/language-service/src/utils.ts b/packages/language-service/src/utils.ts index 5899f98f8c021b..933d87514d91cf 100644 --- a/packages/language-service/src/utils.ts +++ b/packages/language-service/src/utils.ts @@ -86,23 +86,6 @@ export function removeSuffix(value: string, suffix: string) { return value; } -export function uniqueByName < T extends { - name: string; -} -> (elements: T[] | undefined): T[]|undefined { - if (elements) { - const result: T[] = []; - const set = new Set(); - for (const element of elements) { - if (!set.has(element.name)) { - set.add(element.name); - result.push(element); - } - } - return result; - } -} - export function isTypescriptVersion(low: string, high?: string) { const version = ts.version; diff --git a/packages/language-service/test/completions_spec.ts b/packages/language-service/test/completions_spec.ts index cac0f71f25948c..03c9cb3a46e03f 100644 --- a/packages/language-service/test/completions_spec.ts +++ b/packages/language-service/test/completions_spec.ts @@ -6,20 +6,17 @@ * found in the LICENSE file at https://angular.io/license */ -import 'reflect-metadata'; import * as ts from 'typescript'; import {createLanguageService} from '../src/language_service'; -import {Completion} from '../src/types'; import {TypeScriptServiceHost} from '../src/typescript_host'; import {toh} from './test_data'; import {MockTypescriptHost} from './test_utils'; describe('completions', () => { - let documentRegistry = ts.createDocumentRegistry(); let mockHost = new MockTypescriptHost(['/app/main.ts', '/app/parsing-cases.ts'], toh); - let service = ts.createLanguageService(mockHost, documentRegistry); + let service = ts.createLanguageService(mockHost); let ngHost = new TypeScriptServiceHost(mockHost, service); let ngService = createLanguageService(ngHost);