From fa758cc357e72c6705da956eb41858db51840bb3 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Tue, 9 Jan 2018 15:33:54 -0800 Subject: [PATCH 1/9] Tidy up code to make it harder to call incorrectly --- src/services/pathCompletions.ts | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/src/services/pathCompletions.ts b/src/services/pathCompletions.ts index e9c8b48a7c4..584a83ad95c 100644 --- a/src/services/pathCompletions.ts +++ b/src/services/pathCompletions.ts @@ -326,29 +326,28 @@ namespace ts.Completions.PathCompletions { if (typeRoots) { for (const root of typeRoots) { - getCompletionEntriesFromDirectories(host, root, span, result); + getCompletionEntriesFromDirectories(root); } } - } - if (host.getDirectories) { // Also get all @types typings installed in visible node_modules directories for (const packageJson of findPackageJsons(scriptPath, host)) { const typesDir = combinePaths(getDirectoryPath(packageJson), "node_modules/@types"); - getCompletionEntriesFromDirectories(host, typesDir, span, result); + getCompletionEntriesFromDirectories(typesDir); } } return result; - } - function getCompletionEntriesFromDirectories(host: LanguageServiceHost, directory: string, span: TextSpan, result: Push) { - if (host.getDirectories && tryDirectoryExists(host, directory)) { - const directories = tryGetDirectories(host, directory); - if (directories) { - for (let typeDirectory of directories) { - typeDirectory = normalizePath(typeDirectory); - result.push(createCompletionEntryForModule(getBaseFileName(typeDirectory), ScriptElementKind.externalModuleName, span)); + function getCompletionEntriesFromDirectories(directory: string) { + Debug.assert(!!host.getDirectories); + if (tryDirectoryExists(host, directory)) { + const directories = tryGetDirectories(host, directory); + if (directories) { + for (let typeDirectory of directories) { + typeDirectory = normalizePath(typeDirectory); + result.push(createCompletionEntryForModule(getBaseFileName(typeDirectory), ScriptElementKind.externalModuleName, span)); + } } } } From db09a593d3e4d50cf6a218192a417bc9b7ebc761 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Tue, 9 Jan 2018 15:40:32 -0800 Subject: [PATCH 2/9] Unmangle package names from typings during completion --- src/compiler/moduleNameResolver.ts | 11 ++++++++--- src/services/pathCompletions.ts | 7 +++++-- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/src/compiler/moduleNameResolver.ts b/src/compiler/moduleNameResolver.ts index 1cc11acd32d..532d076504b 100644 --- a/src/compiler/moduleNameResolver.ts +++ b/src/compiler/moduleNameResolver.ts @@ -1097,13 +1097,18 @@ namespace ts { export function getPackageNameFromAtTypesDirectory(mangledName: string): string { const withoutAtTypePrefix = removePrefix(mangledName, "@types/"); if (withoutAtTypePrefix !== mangledName) { - return stringContains(withoutAtTypePrefix, mangledScopedPackageSeparator) ? - "@" + withoutAtTypePrefix.replace(mangledScopedPackageSeparator, ts.directorySeparator) : - withoutAtTypePrefix; + return getPackageNameFromAtTypesDirectoryWithoutPrefix(withoutAtTypePrefix); } return mangledName; } + /* @internal */ + export function getPackageNameFromAtTypesDirectoryWithoutPrefix(withoutAtTypePrefix: string): string { + return stringContains(withoutAtTypePrefix, mangledScopedPackageSeparator) ? + "@" + withoutAtTypePrefix.replace(mangledScopedPackageSeparator, ts.directorySeparator) : + withoutAtTypePrefix; + } + function tryFindNonRelativeModuleNameInCache(cache: PerModuleNameCache | undefined, moduleName: string, containingDirectory: string, traceEnabled: boolean, host: ModuleResolutionHost): SearchResult { const result = cache && cache.get(containingDirectory); if (result) { diff --git a/src/services/pathCompletions.ts b/src/services/pathCompletions.ts index 584a83ad95c..f6f6667bbec 100644 --- a/src/services/pathCompletions.ts +++ b/src/services/pathCompletions.ts @@ -313,7 +313,8 @@ namespace ts.Completions.PathCompletions { function getCompletionEntriesFromTypings(host: LanguageServiceHost, options: CompilerOptions, scriptPath: string, span: TextSpan, result: CompletionEntry[] = []): CompletionEntry[] { // Check for typings specified in compiler options if (options.types) { - for (const moduleName of options.types) { + for (const typesName of options.types) { + const moduleName = getPackageNameFromAtTypesDirectoryWithoutPrefix(typesName); result.push(createCompletionEntryForModule(moduleName, ScriptElementKind.externalModuleName, span)); } } @@ -346,7 +347,9 @@ namespace ts.Completions.PathCompletions { if (directories) { for (let typeDirectory of directories) { typeDirectory = normalizePath(typeDirectory); - result.push(createCompletionEntryForModule(getBaseFileName(typeDirectory), ScriptElementKind.externalModuleName, span)); + const directoryName = getBaseFileName(typeDirectory); + const moduleName = getPackageNameFromAtTypesDirectoryWithoutPrefix(directoryName); + result.push(createCompletionEntryForModule(moduleName, ScriptElementKind.externalModuleName, span)); } } } From e48312df54be770f7c097fad1bca77dc407be260 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Tue, 9 Jan 2018 16:54:58 -0800 Subject: [PATCH 3/9] De-dup typing module completions --- src/services/pathCompletions.ts | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/services/pathCompletions.ts b/src/services/pathCompletions.ts index f6f6667bbec..98b1737adf3 100644 --- a/src/services/pathCompletions.ts +++ b/src/services/pathCompletions.ts @@ -312,10 +312,11 @@ namespace ts.Completions.PathCompletions { function getCompletionEntriesFromTypings(host: LanguageServiceHost, options: CompilerOptions, scriptPath: string, span: TextSpan, result: CompletionEntry[] = []): CompletionEntry[] { // Check for typings specified in compiler options + let seen = createMap(); if (options.types) { for (const typesName of options.types) { const moduleName = getPackageNameFromAtTypesDirectoryWithoutPrefix(typesName); - result.push(createCompletionEntryForModule(moduleName, ScriptElementKind.externalModuleName, span)); + pushResult(moduleName); } } else if (host.getDirectories) { @@ -349,11 +350,18 @@ namespace ts.Completions.PathCompletions { typeDirectory = normalizePath(typeDirectory); const directoryName = getBaseFileName(typeDirectory); const moduleName = getPackageNameFromAtTypesDirectoryWithoutPrefix(directoryName); - result.push(createCompletionEntryForModule(moduleName, ScriptElementKind.externalModuleName, span)); + pushResult(moduleName); } } } } + + function pushResult(moduleName: string) { + if (!seen.has(moduleName)) { + result.push(createCompletionEntryForModule(moduleName, ScriptElementKind.externalModuleName, span)); + seen.set(moduleName, true); + } + } } function findPackageJsons(directory: string, host: LanguageServiceHost): string[] { From 71c92bf2f6ec864e3b794c3b919915fe2c15116c Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Wed, 10 Jan 2018 13:38:25 -0800 Subject: [PATCH 4/9] Fix lint error --- src/services/pathCompletions.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/services/pathCompletions.ts b/src/services/pathCompletions.ts index 98b1737adf3..9ed612f0ef3 100644 --- a/src/services/pathCompletions.ts +++ b/src/services/pathCompletions.ts @@ -312,7 +312,7 @@ namespace ts.Completions.PathCompletions { function getCompletionEntriesFromTypings(host: LanguageServiceHost, options: CompilerOptions, scriptPath: string, span: TextSpan, result: CompletionEntry[] = []): CompletionEntry[] { // Check for typings specified in compiler options - let seen = createMap(); + const seen = createMap(); if (options.types) { for (const typesName of options.types) { const moduleName = getPackageNameFromAtTypesDirectoryWithoutPrefix(typesName); From 5d8598f2808b56bfce526defd45ea8fa0d9ec0cb Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Wed, 10 Jan 2018 13:38:32 -0800 Subject: [PATCH 5/9] Add regression test --- .../fourslash/completionListInImportClause05.ts | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) create mode 100644 tests/cases/fourslash/completionListInImportClause05.ts diff --git a/tests/cases/fourslash/completionListInImportClause05.ts b/tests/cases/fourslash/completionListInImportClause05.ts new file mode 100644 index 00000000000..b98e91c4cbb --- /dev/null +++ b/tests/cases/fourslash/completionListInImportClause05.ts @@ -0,0 +1,17 @@ +/// + +// @Filename: app.ts +//// import * as A from "[|/*1*/|]"; + +// @Filename: /node_modules/@types/a__b/index.d.ts +////declare module "@e/f" { function fun(): string; } + +// @Filename: /node_modules/@types/c__d/index.d.ts +////export declare let x: number; + +const [replacementSpan] = test.ranges(); +verify.completionsAt("1", [ + { name: "@a/b", replacementSpan }, + { name: "@c/d", replacementSpan }, + { name: "@e/f", replacementSpan }, +]); From 8275bed33fd75dcc2f1c7ebcd1e7ba08b3cbb9f5 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Wed, 10 Jan 2018 13:57:06 -0800 Subject: [PATCH 6/9] Add comment explaining test-only workaround --- tests/cases/fourslash/completionListInImportClause05.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/cases/fourslash/completionListInImportClause05.ts b/tests/cases/fourslash/completionListInImportClause05.ts index b98e91c4cbb..e1401706ea1 100644 --- a/tests/cases/fourslash/completionListInImportClause05.ts +++ b/tests/cases/fourslash/completionListInImportClause05.ts @@ -9,6 +9,11 @@ // @Filename: /node_modules/@types/c__d/index.d.ts ////export declare let x: number; +// NOTE: When performing completion, the "current directory" appears to be "/", +// which is well above "." (i.e. the directory containing "app.ts"). This issue +// is specific to the virtual file system, so just work around it by putting the +// node modules folder in "/", rather than ".". + const [replacementSpan] = test.ranges(); verify.completionsAt("1", [ { name: "@a/b", replacementSpan }, From ff102da9d1a77bd148d7c713ad109e65edfcba65 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Wed, 10 Jan 2018 15:12:00 -0800 Subject: [PATCH 7/9] Refine explanatory comment --- tests/cases/fourslash/completionListInImportClause05.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/tests/cases/fourslash/completionListInImportClause05.ts b/tests/cases/fourslash/completionListInImportClause05.ts index e1401706ea1..f7b5244d5a4 100644 --- a/tests/cases/fourslash/completionListInImportClause05.ts +++ b/tests/cases/fourslash/completionListInImportClause05.ts @@ -1,7 +1,7 @@ /// // @Filename: app.ts -//// import * as A from "[|/*1*/|]"; +////import * as A from "[|/*1*/|]"; // @Filename: /node_modules/@types/a__b/index.d.ts ////declare module "@e/f" { function fun(): string; } @@ -9,10 +9,8 @@ // @Filename: /node_modules/@types/c__d/index.d.ts ////export declare let x: number; -// NOTE: When performing completion, the "current directory" appears to be "/", -// which is well above "." (i.e. the directory containing "app.ts"). This issue -// is specific to the virtual file system, so just work around it by putting the -// node modules folder in "/", rather than ".". +// NOTE: The node_modules folder is in "/", rather than ".", because it requires +// less scaffolding to mock. In particular, "/" is where we look for type roots. const [replacementSpan] = test.ranges(); verify.completionsAt("1", [ From 211be0ae69f217db437ca13185ad76586aab5054 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Wed, 10 Jan 2018 15:12:10 -0800 Subject: [PATCH 8/9] Add regression test for de-dup'ing. --- .../fourslash/completionListInImportClause06.ts | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) create mode 100644 tests/cases/fourslash/completionListInImportClause06.ts diff --git a/tests/cases/fourslash/completionListInImportClause06.ts b/tests/cases/fourslash/completionListInImportClause06.ts new file mode 100644 index 00000000000..f6cfefa437f --- /dev/null +++ b/tests/cases/fourslash/completionListInImportClause06.ts @@ -0,0 +1,17 @@ +/// + +// @typeRoots: T1,T2 + +// @Filename: app.ts +////import * as A from "[|/*1*/|]"; + +// @Filename: T1/a__b/index.d.ts +////export declare let x: number; + +// @Filename: T2/a__b/index.d.ts +////export declare let x: number; + +// Confirm that entries are de-dup'd. +verify.completionsAt("1", [ + { name: "@a/b", replacementSpan: test.ranges()[0] }, +]); From 9a4fe8eb7e3242cbca2aa430eb7200e0ad6d0001 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Wed, 10 Jan 2018 15:17:27 -0800 Subject: [PATCH 9/9] Rename getPackageNameFromAtTypesDirectoryWithoutPrefix to getUnmangledNameForScopedPackage --- src/compiler/moduleNameResolver.ts | 10 +++++----- src/services/pathCompletions.ts | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/compiler/moduleNameResolver.ts b/src/compiler/moduleNameResolver.ts index 532d076504b..018bddd420b 100644 --- a/src/compiler/moduleNameResolver.ts +++ b/src/compiler/moduleNameResolver.ts @@ -1097,16 +1097,16 @@ namespace ts { export function getPackageNameFromAtTypesDirectory(mangledName: string): string { const withoutAtTypePrefix = removePrefix(mangledName, "@types/"); if (withoutAtTypePrefix !== mangledName) { - return getPackageNameFromAtTypesDirectoryWithoutPrefix(withoutAtTypePrefix); + return getUnmangledNameForScopedPackage(withoutAtTypePrefix); } return mangledName; } /* @internal */ - export function getPackageNameFromAtTypesDirectoryWithoutPrefix(withoutAtTypePrefix: string): string { - return stringContains(withoutAtTypePrefix, mangledScopedPackageSeparator) ? - "@" + withoutAtTypePrefix.replace(mangledScopedPackageSeparator, ts.directorySeparator) : - withoutAtTypePrefix; + export function getUnmangledNameForScopedPackage(typesPackageName: string): string { + return stringContains(typesPackageName, mangledScopedPackageSeparator) ? + "@" + typesPackageName.replace(mangledScopedPackageSeparator, ts.directorySeparator) : + typesPackageName; } function tryFindNonRelativeModuleNameInCache(cache: PerModuleNameCache | undefined, moduleName: string, containingDirectory: string, traceEnabled: boolean, host: ModuleResolutionHost): SearchResult { diff --git a/src/services/pathCompletions.ts b/src/services/pathCompletions.ts index 9ed612f0ef3..82f144ac377 100644 --- a/src/services/pathCompletions.ts +++ b/src/services/pathCompletions.ts @@ -315,7 +315,7 @@ namespace ts.Completions.PathCompletions { const seen = createMap(); if (options.types) { for (const typesName of options.types) { - const moduleName = getPackageNameFromAtTypesDirectoryWithoutPrefix(typesName); + const moduleName = getUnmangledNameForScopedPackage(typesName); pushResult(moduleName); } } @@ -349,7 +349,7 @@ namespace ts.Completions.PathCompletions { for (let typeDirectory of directories) { typeDirectory = normalizePath(typeDirectory); const directoryName = getBaseFileName(typeDirectory); - const moduleName = getPackageNameFromAtTypesDirectoryWithoutPrefix(directoryName); + const moduleName = getUnmangledNameForScopedPackage(directoryName); pushResult(moduleName); } }