From 43a35bad2ea910aabfcd49a48a06b57f8002c61c Mon Sep 17 00:00:00 2001 From: Andy Date: Thu, 30 Nov 2017 09:36:17 -0800 Subject: [PATCH] Miscellaneous fixes to avoid duplicate completions (#20349) * Miscellaneous fixes to avoid duplicate completions * Move typeHasCallOrConstructSignatures to utility --- src/compiler/checker.ts | 22 +++++----- src/compiler/utilities.ts | 9 ++++ src/harness/fourslash.ts | 3 +- src/services/codefixes/importFixes.ts | 3 +- src/services/completions.ts | 43 +++++++++---------- src/services/pathCompletions.ts | 41 ++++++++---------- ...etionForStringLiteralNonrelativeImport8.ts | 2 +- .../fourslash/completionInJSDocFunctionNew.ts | 4 +- .../completionInJSDocFunctionThis.ts | 4 +- .../fourslash/completionListPrimitives.ts | 2 +- tests/cases/fourslash/fourslash.ts | 2 +- .../fourslash/getJavaScriptCompletions2.ts | 2 +- 12 files changed, 69 insertions(+), 68 deletions(-) diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index 27b7cfea68f..c6f5fda7f3b 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -6245,12 +6245,12 @@ namespace ts { function getAllPossiblePropertiesOfTypes(types: Type[]): Symbol[] { const unionType = getUnionType(types); if (!(unionType.flags & TypeFlags.Union)) { - return getPropertiesOfType(unionType); + return getAugmentedPropertiesOfType(unionType); } const props = createSymbolTable(); for (const memberType of types) { - for (const { escapedName } of getPropertiesOfType(memberType)) { + for (const { escapedName } of getAugmentedPropertiesOfType(memberType)) { if (!props.has(escapedName)) { props.set(escapedName, createUnionOrIntersectionProperty(unionType as UnionType, escapedName)); } @@ -9380,9 +9380,7 @@ namespace ts { !(target.flags & TypeFlags.Union) && !isIntersectionConstituent && source !== globalObjectType && - (getPropertiesOfType(source).length > 0 || - getSignaturesOfType(source, SignatureKind.Call).length > 0 || - getSignaturesOfType(source, SignatureKind.Construct).length > 0) && + (getPropertiesOfType(source).length > 0 || typeHasCallOrConstructSignatures(source)) && isWeakType(target) && !hasCommonProperties(source, target)) { if (reportErrors) { @@ -10770,8 +10768,7 @@ namespace ts { */ function isObjectTypeWithInferableIndex(type: Type) { return type.symbol && (type.symbol.flags & (SymbolFlags.ObjectLiteral | SymbolFlags.TypeLiteral | SymbolFlags.ValueModule)) !== 0 && - getSignaturesOfType(type, SignatureKind.Call).length === 0 && - getSignaturesOfType(type, SignatureKind.Construct).length === 0; + !typeHasCallOrConstructSignatures(type); } function createSymbolWithType(source: Symbol, type: Type) { @@ -18351,10 +18348,7 @@ namespace ts { error(left, Diagnostics.The_left_hand_side_of_an_instanceof_expression_must_be_of_type_any_an_object_type_or_a_type_parameter); } // NOTE: do not raise error if right is unknown as related error was already reported - if (!(isTypeAny(rightType) || - getSignaturesOfType(rightType, SignatureKind.Call).length || - getSignaturesOfType(rightType, SignatureKind.Construct).length || - isTypeSubtypeOf(rightType, globalFunctionType))) { + if (!(isTypeAny(rightType) || typeHasCallOrConstructSignatures(rightType) || isTypeSubtypeOf(rightType, globalFunctionType))) { error(right, Diagnostics.The_right_hand_side_of_an_instanceof_expression_must_be_of_type_any_or_of_a_type_assignable_to_the_Function_interface_type); } return booleanType; @@ -24431,7 +24425,7 @@ namespace ts { function getAugmentedPropertiesOfType(type: Type): Symbol[] { type = getApparentType(type); const propsByName = createSymbolTable(getPropertiesOfType(type)); - if (getSignaturesOfType(type, SignatureKind.Call).length || getSignaturesOfType(type, SignatureKind.Construct).length) { + if (typeHasCallOrConstructSignatures(type)) { forEach(getPropertiesOfType(globalFunctionType), p => { if (!propsByName.has(p.escapedName)) { propsByName.set(p.escapedName, p); @@ -24441,6 +24435,10 @@ namespace ts { return getNamedMembers(propsByName); } + function typeHasCallOrConstructSignatures(type: Type): boolean { + return ts.typeHasCallOrConstructSignatures(type, checker); + } + function getRootSymbols(symbol: Symbol): Symbol[] { if (getCheckFlags(symbol) & CheckFlags.Synthetic) { const symbols: Symbol[] = []; diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts index 9426cd77839..05bd2c53d96 100644 --- a/src/compiler/utilities.ts +++ b/src/compiler/utilities.ts @@ -1969,6 +1969,11 @@ namespace ts { return isKeyword(token) && !isContextualKeyword(token); } + export function isStringANonContextualKeyword(name: string) { + const token = stringToToken(name); + return token !== undefined && isNonContextualKeyword(token); + } + export function isTrivia(token: SyntaxKind) { return SyntaxKind.FirstTriviaToken <= token && token <= SyntaxKind.LastTriviaToken; } @@ -3664,6 +3669,10 @@ namespace ts { directory = parentPath; } } + + export function typeHasCallOrConstructSignatures(type: Type, checker: TypeChecker) { + return checker.getSignaturesOfType(type, SignatureKind.Call).length !== 0 || checker.getSignaturesOfType(type, SignatureKind.Construct).length !== 0; + } } namespace ts { diff --git a/src/harness/fourslash.ts b/src/harness/fourslash.ts index cd992464a65..0e37dd51c15 100644 --- a/src/harness/fourslash.ts +++ b/src/harness/fourslash.ts @@ -3081,7 +3081,7 @@ Actual: ${stringify(fullActual)}`); const itemsString = items.map(item => stringify({ name: item.name, source: item.source, kind: item.kind })).join(",\n"); this.raiseError(`Expected "${stringify({ entryId, text, documentation, kind })}" to be in list [${itemsString}]`); } - else if (matchingItems.length > 1 && !(options && options.allowDuplicate)) { + else if (matchingItems.length > 1) { this.raiseError(`Found duplicate completion items for ${stringify(entryId)}`); } const item = matchingItems[0]; @@ -4558,7 +4558,6 @@ namespace FourSlashInterface { export interface VerifyCompletionListContainsOptions extends ts.GetCompletionsAtPositionOptions { sourceDisplay: string; - allowDuplicate: boolean; // TODO: GH#20042 } export interface NewContentOptions { diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts index 41515265862..5124b47f475 100644 --- a/src/services/codefixes/importFixes.ts +++ b/src/services/codefixes/importFixes.ts @@ -814,7 +814,6 @@ namespace ts.codefix { lastCharWasValid = isValid; } // Need `|| "_"` to ensure result isn't empty. - const token = stringToToken(res); - return token === undefined || !isNonContextualKeyword(token) ? res || "_" : `_${res}`; + return !isStringANonContextualKeyword(res) ? res || "_" : `_${res}`; } } diff --git a/src/services/completions.ts b/src/services/completions.ts index a3b94acd4c0..083ddea37db 100644 --- a/src/services/completions.ts +++ b/src/services/completions.ts @@ -112,7 +112,7 @@ namespace ts.Completions { } const realName = unescapeLeadingUnderscores(name); - if (uniqueNames.has(realName)) { + if (uniqueNames.has(realName) || isStringANonContextualKeyword(realName)) { return; } @@ -552,8 +552,6 @@ namespace ts.Completions { options: GetCompletionsAtPositionOptions, target: ScriptTarget, ): CompletionData | undefined { - const isJavaScriptFile = isSourceFileJavaScript(sourceFile); - let request: Request | undefined; let start = timestamp(); @@ -832,23 +830,21 @@ namespace ts.Completions { } } - function addTypeProperties(type: Type) { - // Filter private properties - for (const symbol of type.getApparentProperties()) { - if (typeChecker.isValidPropertyAccess((node.parent), symbol.name)) { - symbols.push(symbol); - } - } - - if (isJavaScriptFile && type.flags & TypeFlags.Union) { + function addTypeProperties(type: Type): void { + if (isSourceFileJavaScript(sourceFile)) { // In javascript files, for union types, we don't just get the members that // the individual types have in common, we also include all the members that - // each individual type has. This is because we're going to add all identifiers - // anyways. So we might as well elevate the members that were at least part + // each individual type has. This is because we're going to add all identifiers + // anyways. So we might as well elevate the members that were at least part // of the individual types to a higher status since we know what they are. - const unionType = type; - for (const elementType of unionType.types) { - addTypeProperties(elementType); + symbols.push(...getPropertiesForCompletion(type, typeChecker, /*isForAccess*/ true)); + } + else { + // Filter private properties + for (const symbol of type.getApparentProperties()) { + if (typeChecker.isValidPropertyAccess((node.parent), symbol.name)) { + symbols.push(symbol); + } } } } @@ -1239,7 +1235,7 @@ namespace ts.Completions { isNewIdentifierLocation = true; const typeForObject = typeChecker.getContextualType(objectLikeContainer); if (!typeForObject) return false; - typeMembers = getPropertiesForCompletion(typeForObject, typeChecker); + typeMembers = getPropertiesForCompletion(typeForObject, typeChecker, /*isForAccess*/ false); existingMembers = (objectLikeContainer).properties; } else { @@ -1952,6 +1948,8 @@ namespace ts.Completions { function getAllKeywordCompletions() { const allKeywordsCompletions: CompletionEntry[] = []; for (let i = SyntaxKind.FirstKeyword; i <= SyntaxKind.LastKeyword; i++) { + // "undefined" is a global variable, so don't need a keyword completion for it. + if (i === SyntaxKind.UndefinedKeyword) continue; allKeywordsCompletions.push({ name: tokenToString(i), kind: ScriptElementKind.keyword, @@ -2050,14 +2048,15 @@ namespace ts.Completions { * tries to only include those types which declare properties, not methods. * This ensures that we don't try providing completions for all the methods on e.g. Array. */ - function getPropertiesForCompletion(type: Type, checker: TypeChecker): Symbol[] { + function getPropertiesForCompletion(type: Type, checker: TypeChecker, isForAccess: boolean): Symbol[] { if (!(type.flags & TypeFlags.Union)) { - return checker.getPropertiesOfType(type); + return type.getApparentProperties(); } const { types } = type as UnionType; - const filteredTypes = types.filter(memberType => !(memberType.flags & TypeFlags.Primitive || checker.isArrayLikeType(memberType))); - // If there are no property-only types, just provide completions for every type as usual. + // If we're providing completions for an object literal, skip primitive, array-like, or callable types since those shouldn't be implemented by object literals. + const filteredTypes = isForAccess ? types : types.filter(memberType => + !(memberType.flags & TypeFlags.Primitive || checker.isArrayLikeType(memberType) || typeHasCallOrConstructSignatures(memberType, checker))); return checker.getAllPossiblePropertiesOfTypes(filteredTypes); } } diff --git a/src/services/pathCompletions.ts b/src/services/pathCompletions.ts index a6dfb53da80..15c1ff99758 100644 --- a/src/services/pathCompletions.ts +++ b/src/services/pathCompletions.ts @@ -138,39 +138,36 @@ namespace ts.Completions.PathCompletions { function getCompletionEntriesForNonRelativeModules(fragment: string, scriptPath: string, span: TextSpan, compilerOptions: CompilerOptions, host: LanguageServiceHost, typeChecker: TypeChecker): CompletionEntry[] { const { baseUrl, paths } = compilerOptions; - let result: CompletionEntry[]; + const result: CompletionEntry[] = []; const fileExtensions = getSupportedExtensions(compilerOptions); if (baseUrl) { const projectDir = compilerOptions.project || host.getCurrentDirectory(); const absolute = isRootedDiskPath(baseUrl) ? baseUrl : combinePaths(projectDir, baseUrl); - result = getCompletionEntriesForDirectoryFragment(fragment, normalizePath(absolute), fileExtensions, /*includeExtensions*/ false, span, host); + getCompletionEntriesForDirectoryFragment(fragment, normalizePath(absolute), fileExtensions, /*includeExtensions*/ false, span, host, /*exclude*/ undefined, result); - if (paths) { - for (const path in paths) { - if (paths.hasOwnProperty(path)) { - if (path === "*") { - if (paths[path]) { - for (const pattern of paths[path]) { - for (const match of getModulesForPathsPattern(fragment, baseUrl, pattern, fileExtensions, host)) { - result.push(createCompletionEntryForModule(match, ScriptElementKind.externalModuleName, span)); - } - } - } - } - else if (startsWith(path, fragment)) { - const entry = paths[path] && paths[path].length === 1 && paths[path][0]; - if (entry) { - result.push(createCompletionEntryForModule(path, ScriptElementKind.externalModuleName, span)); - } + for (const path in paths) { + if (!paths.hasOwnProperty(path)) continue; + const patterns = paths[path]; + if (!patterns) continue; + + if (path === "*") { + for (const pattern of patterns) { + for (const match of getModulesForPathsPattern(fragment, baseUrl, pattern, fileExtensions, host)) { + // Path mappings may provide a duplicate way to get to something we've already added, so don't add again. + if (result.some(entry => entry.name === match)) continue; + result.push(createCompletionEntryForModule(match, ScriptElementKind.externalModuleName, span)); } } } + else if (startsWith(path, fragment)) { + if (patterns.length === 1) { + if (result.some(entry => entry.name === path)) continue; + result.push(createCompletionEntryForModule(path, ScriptElementKind.externalModuleName, span)); + } + } } } - else { - result = []; - } if (compilerOptions.moduleResolution === ts.ModuleResolutionKind.NodeJs) { forEachAncestorDirectory(scriptPath, ancestor => { diff --git a/tests/cases/fourslash/completionForStringLiteralNonrelativeImport8.ts b/tests/cases/fourslash/completionForStringLiteralNonrelativeImport8.ts index 6ba94510e90..603e8d1104d 100644 --- a/tests/cases/fourslash/completionForStringLiteralNonrelativeImport8.ts +++ b/tests/cases/fourslash/completionForStringLiteralNonrelativeImport8.ts @@ -51,5 +51,5 @@ for (const kind of kinds) { verify.completionListContains("1test"); goTo.marker(kind + "2"); - verify.completionListContains("2test", undefined, undefined, undefined, undefined, undefined, { allowDuplicate: true }); // TODO: GH#20042 + verify.completionListContains("2test"); } diff --git a/tests/cases/fourslash/completionInJSDocFunctionNew.ts b/tests/cases/fourslash/completionInJSDocFunctionNew.ts index a3fff51a931..6a06ec76fe5 100644 --- a/tests/cases/fourslash/completionInJSDocFunctionNew.ts +++ b/tests/cases/fourslash/completionInJSDocFunctionNew.ts @@ -6,5 +6,5 @@ ////var f = function () { return new/**/; } goTo.marker(); -verify.completionListCount(118); -verify.completionListContains('new', undefined, undefined, undefined, undefined, undefined, { allowDuplicate: true }); // TODO: GH#20042 +verify.completionListCount(116); +verify.completionListContains('new'); diff --git a/tests/cases/fourslash/completionInJSDocFunctionThis.ts b/tests/cases/fourslash/completionInJSDocFunctionThis.ts index 8f62680205c..0fd771f272b 100644 --- a/tests/cases/fourslash/completionInJSDocFunctionThis.ts +++ b/tests/cases/fourslash/completionInJSDocFunctionThis.ts @@ -5,5 +5,5 @@ ////var f = function (s) { return this/**/; } goTo.marker(); -verify.completionListCount(119); -verify.completionListContains('this', undefined, undefined, undefined, undefined, undefined, { allowDuplicate: true }); // TODO: GH#20042 +verify.completionListCount(117); +verify.completionListContains('this') diff --git a/tests/cases/fourslash/completionListPrimitives.ts b/tests/cases/fourslash/completionListPrimitives.ts index 7442746fa0c..54c518108de 100644 --- a/tests/cases/fourslash/completionListPrimitives.ts +++ b/tests/cases/fourslash/completionListPrimitives.ts @@ -8,5 +8,5 @@ verify.completionListContains("boolean"); verify.completionListContains("null"); verify.completionListContains("number"); verify.completionListContains("string"); -verify.completionListContains("undefined", undefined, undefined, undefined, undefined, undefined, { allowDuplicate: true }); // TODO: GH#20042 +verify.completionListContains("undefined"); verify.completionListContains("void"); diff --git a/tests/cases/fourslash/fourslash.ts b/tests/cases/fourslash/fourslash.ts index b38a7f453bc..44c270de4db 100644 --- a/tests/cases/fourslash/fourslash.ts +++ b/tests/cases/fourslash/fourslash.ts @@ -148,7 +148,7 @@ declare namespace FourSlashInterface { kind?: string, spanIndex?: number, hasAction?: boolean, - options?: { includeExternalModuleExports?: boolean, sourceDisplay?: string, allowDuplicate?: boolean }, + options?: { includeExternalModuleExports?: boolean, sourceDisplay?: string }, ): void; completionListItemsCountIsGreaterThan(count: number): void; completionListIsEmpty(): void; diff --git a/tests/cases/fourslash/getJavaScriptCompletions2.ts b/tests/cases/fourslash/getJavaScriptCompletions2.ts index 736762cd169..b5aab81b434 100644 --- a/tests/cases/fourslash/getJavaScriptCompletions2.ts +++ b/tests/cases/fourslash/getJavaScriptCompletions2.ts @@ -7,4 +7,4 @@ ////v./**/ goTo.marker(); -verify.completionListContains("valueOf", /*displayText:*/ undefined, /*documentation*/ undefined, "method", undefined, undefined, { allowDuplicate: true }); // TODO: GH#20042 +verify.completionListContains("valueOf", /*displayText:*/ undefined, /*documentation*/ undefined, "method");