From 0921eac6dc9eba0be6319dff10b85d60c90155ea Mon Sep 17 00:00:00 2001 From: Andrew Branch Date: Thu, 19 May 2022 14:33:46 -0700 Subject: [PATCH] Fix path completions for typesVersions (#49154) * Fix path completions for typesVersions * Add more tests * Fix case when * is a fragment of a path component * Once a path pattern matches, only return completions from that pattern and higher priority ones * Fix iteration order issue * Aesthetics --- src/compiler/core.ts | 2 +- src/services/stringCompletions.ts | 84 +++++++++++++------ ...tionForStringLiteralNonrelativeImport13.ts | 2 +- .../completionsPaths_pathMapping_topLevel.ts | 2 +- .../pathCompletionsTypesVersionsWildcard1.ts | 42 ++++++++++ .../pathCompletionsTypesVersionsWildcard2.ts | 34 ++++++++ .../pathCompletionsTypesVersionsWildcard3.ts | 48 +++++++++++ .../pathCompletionsTypesVersionsWildcard4.ts | 41 +++++++++ .../pathCompletionsTypesVersionsWildcard5.ts | 54 ++++++++++++ .../pathCompletionsTypesVersionsWildcard6.ts | 57 +++++++++++++ 10 files changed, 339 insertions(+), 27 deletions(-) create mode 100644 tests/cases/fourslash/pathCompletionsTypesVersionsWildcard1.ts create mode 100644 tests/cases/fourslash/pathCompletionsTypesVersionsWildcard2.ts create mode 100644 tests/cases/fourslash/pathCompletionsTypesVersionsWildcard3.ts create mode 100644 tests/cases/fourslash/pathCompletionsTypesVersionsWildcard4.ts create mode 100644 tests/cases/fourslash/pathCompletionsTypesVersionsWildcard5.ts create mode 100644 tests/cases/fourslash/pathCompletionsTypesVersionsWildcard6.ts diff --git a/src/compiler/core.ts b/src/compiler/core.ts index a3be6f976cdc9..d0eaa7fc0dc05 100644 --- a/src/compiler/core.ts +++ b/src/compiler/core.ts @@ -2299,7 +2299,7 @@ namespace ts { return startsWith(getCanonicalFileName(str), getCanonicalFileName(prefix)) ? str.substring(prefix.length) : undefined; } - function isPatternMatch({ prefix, suffix }: Pattern, candidate: string) { + export function isPatternMatch({ prefix, suffix }: Pattern, candidate: string) { return candidate.length >= prefix.length + suffix.length && startsWith(candidate, prefix) && endsWith(candidate, suffix); diff --git a/src/services/stringCompletions.ts b/src/services/stringCompletions.ts index 604a833742649..39ae23fe4b46d 100644 --- a/src/services/stringCompletions.ts +++ b/src/services/stringCompletions.ts @@ -448,10 +448,28 @@ namespace ts.Completions.StringCompletions { fragment = ensureTrailingDirectorySeparator(fragment); - // const absolutePath = normalizeAndPreserveTrailingSlash(isRootedDiskPath(fragment) ? fragment : combinePaths(scriptPath, fragment)); // TODO(rbuckton): should use resolvePaths const absolutePath = resolvePath(scriptPath, fragment); const baseDirectory = hasTrailingDirectorySeparator(absolutePath) ? absolutePath : getDirectoryPath(absolutePath); + // check for a version redirect + const packageJsonPath = findPackageJson(baseDirectory, host); + if (packageJsonPath) { + const packageJson = readJson(packageJsonPath, host as { readFile: (filename: string) => string | undefined }); + const typesVersions = (packageJson as any).typesVersions; + if (typeof typesVersions === "object") { + const versionPaths = getPackageJsonTypesVersionsPaths(typesVersions)?.paths; + if (versionPaths) { + const packageDirectory = getDirectoryPath(packageJsonPath); + const pathInPackage = absolutePath.slice(ensureTrailingDirectorySeparator(packageDirectory).length); + if (addCompletionEntriesFromPaths(result, pathInPackage, packageDirectory, extensions, versionPaths, host)) { + // A true result means one of the `versionPaths` was matched, which will block relative resolution + // to files and folders from here. All reachable paths given the pattern match are already added. + return result; + } + } + } + } + const ignoreCase = !(host.useCaseSensitiveFileNames && host.useCaseSensitiveFileNames()); if (!tryDirectoryExists(host, baseDirectory)) return result; @@ -505,37 +523,51 @@ namespace ts.Completions.StringCompletions { } } - // check for a version redirect - const packageJsonPath = findPackageJson(baseDirectory, host); - if (packageJsonPath) { - const packageJson = readJson(packageJsonPath, host as { readFile: (filename: string) => string | undefined }); - const typesVersions = (packageJson as any).typesVersions; - if (typeof typesVersions === "object") { - const versionResult = getPackageJsonTypesVersionsPaths(typesVersions); - const versionPaths = versionResult && versionResult.paths; - const rest = absolutePath.slice(ensureTrailingDirectorySeparator(baseDirectory).length); - if (versionPaths) { - addCompletionEntriesFromPaths(result, rest, baseDirectory, extensions, versionPaths, host); - } - } - } - return result; } + /** @returns whether `fragment` was a match for any `paths` (which should indicate whether any other path completions should be offered) */ function addCompletionEntriesFromPaths(result: NameAndKind[], fragment: string, baseDirectory: string, fileExtensions: readonly string[], paths: MapLike, host: LanguageServiceHost) { + let pathResults: { results: NameAndKind[], matchedPattern: boolean }[] = []; + let matchedPathPrefixLength = -1; for (const path in paths) { if (!hasProperty(paths, path)) continue; const patterns = paths[path]; if (patterns) { - for (const { name, kind, extension } of getCompletionsForPathMapping(path, patterns, fragment, baseDirectory, 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 === name)) { - result.push(nameAndKind(name, kind, extension)); - } + const pathPattern = tryParsePattern(path); + if (!pathPattern) continue; + const isMatch = typeof pathPattern === "object" && isPatternMatch(pathPattern, fragment); + const isLongestMatch = isMatch && (matchedPathPrefixLength === undefined || pathPattern.prefix.length > matchedPathPrefixLength); + if (isLongestMatch) { + // If this is a higher priority match than anything we've seen so far, previous results from matches are invalid, e.g. + // for `import {} from "some-package/|"` with a typesVersions: + // { + // "bar/*": ["bar/*"], // <-- 1. We add 'bar', but 'bar/*' doesn't match yet. + // "*": ["dist/*"], // <-- 2. We match here and add files from dist. 'bar' is still ok because it didn't come from a match. + // "foo/*": ["foo/*"] // <-- 3. We matched '*' earlier and added results from dist, but if 'foo/*' also matched, + // } results in dist would not be visible. 'bar' still stands because it didn't come from a match. + // This is especially important if `dist/foo` is a folder, because if we fail to clear results + // added by the '*' match, after typing `"some-package/foo/|"` we would get file results from both + // ./dist/foo and ./foo, when only the latter will actually be resolvable. + // See pathCompletionsTypesVersionsWildcard6.ts. + matchedPathPrefixLength = pathPattern.prefix.length; + pathResults = pathResults.filter(r => !r.matchedPattern); + } + if (typeof pathPattern === "string" || matchedPathPrefixLength === undefined || pathPattern.prefix.length >= matchedPathPrefixLength) { + pathResults.push({ + matchedPattern: isMatch, + results: getCompletionsForPathMapping(path, patterns, fragment, baseDirectory, fileExtensions, host) + .map(({ name, kind, extension }) => nameAndKind(name, kind, extension)), + }); } } } + + const equatePaths = host.useCaseSensitiveFileNames?.() ? equateStringsCaseSensitive : equateStringsCaseInsensitive; + const equateResults: EqualityComparer = (a, b) => equatePaths(a.name, b.name); + pathResults.forEach(pathResult => pathResult.results.forEach(pathResult => pushIfUnique(result, pathResult, equateResults))); + + return matchedPathPrefixLength > -1; } /** @@ -659,11 +691,15 @@ namespace ts.Completions.StringCompletions { const pathPrefix = path.slice(0, path.length - 1); const remainingFragment = tryRemovePrefix(fragment, pathPrefix); - return remainingFragment === undefined ? justPathMappingName(pathPrefix) : flatMap(patterns, pattern => - getModulesForPathsPattern(remainingFragment, baseUrl, pattern, fileExtensions, host)); + if (remainingFragment === undefined) { + const starIsFullPathComponent = path[path.length - 2] === "/"; + return starIsFullPathComponent ? justPathMappingName(pathPrefix) : flatMap(patterns, pattern => + getModulesForPathsPattern("", baseUrl, pattern, fileExtensions, host)?.map(({ name, ...rest }) => ({ name: pathPrefix + name, ...rest }))); + } + return flatMap(patterns, pattern => getModulesForPathsPattern(remainingFragment, baseUrl, pattern, fileExtensions, host)); function justPathMappingName(name: string): readonly NameAndKind[] { - return startsWith(name, fragment) ? [directoryResult(name)] : emptyArray; + return startsWith(name, fragment) ? [directoryResult(removeTrailingDirectorySeparator(name))] : emptyArray; } } diff --git a/tests/cases/fourslash/completionForStringLiteralNonrelativeImport13.ts b/tests/cases/fourslash/completionForStringLiteralNonrelativeImport13.ts index 59b2d7a953963..90cfe1ebb3c9d 100644 --- a/tests/cases/fourslash/completionForStringLiteralNonrelativeImport13.ts +++ b/tests/cases/fourslash/completionForStringLiteralNonrelativeImport13.ts @@ -31,6 +31,6 @@ verify.completions({ marker: test.markerNames(), - exact: ["aaa", "index", "ts3.1", "zzz"], + exact: ["index", "zzz"], isNewIdentifierLocation: true, }); diff --git a/tests/cases/fourslash/completionsPaths_pathMapping_topLevel.ts b/tests/cases/fourslash/completionsPaths_pathMapping_topLevel.ts index a448abe2dff78..1450247b97f89 100644 --- a/tests/cases/fourslash/completionsPaths_pathMapping_topLevel.ts +++ b/tests/cases/fourslash/completionsPaths_pathMapping_topLevel.ts @@ -15,6 +15,6 @@ verify.completions({ marker: "", - exact: ["src", "foo/"].map(name => ({ name, kind: "directory" })), + exact: ["src", "foo"].map(name => ({ name, kind: "directory" })), isNewIdentifierLocation: true, }); diff --git a/tests/cases/fourslash/pathCompletionsTypesVersionsWildcard1.ts b/tests/cases/fourslash/pathCompletionsTypesVersionsWildcard1.ts new file mode 100644 index 0000000000000..bc3d30d715caa --- /dev/null +++ b/tests/cases/fourslash/pathCompletionsTypesVersionsWildcard1.ts @@ -0,0 +1,42 @@ +/// + +// @module: commonjs + +// @Filename: /node_modules/foo/package.json +//// { +//// "types": "index.d.ts", +//// "typesVersions": { +//// "*": { +//// "*": ["dist/*"] +//// } +//// } +//// } + +// @Filename: /node_modules/foo/nope.d.ts +//// export const nope = 0; + +// @Filename: /node_modules/foo/dist/index.d.ts +//// export const index = 0; + +// @Filename: /node_modules/foo/dist/blah.d.ts +//// export const blah = 0; + +// @Filename: /node_modules/foo/dist/subfolder/one.d.ts +//// export const one = 0; + +// @Filename: /a.ts +//// import { } from "foo//**/"; + +verify.completions({ + marker: "", + isNewIdentifierLocation: true, + exact: ["blah", "index", "subfolder"], +}); + +edit.insert("subfolder/"); + +verify.completions({ + marker: "", + isNewIdentifierLocation: true, + exact: ["one"], +}); diff --git a/tests/cases/fourslash/pathCompletionsTypesVersionsWildcard2.ts b/tests/cases/fourslash/pathCompletionsTypesVersionsWildcard2.ts new file mode 100644 index 0000000000000..8c607bf1e0d6e --- /dev/null +++ b/tests/cases/fourslash/pathCompletionsTypesVersionsWildcard2.ts @@ -0,0 +1,34 @@ +/// + +// @module: commonjs + +// @Filename: /node_modules/foo/package.json +//// { +//// "types": "index.d.ts", +//// "typesVersions": { +//// "<=3.4.1": { +//// "*": ["ts-old/*"] +//// } +//// } +//// } + +// @Filename: /node_modules/foo/nope.d.ts +//// export const nope = 0; + +// @Filename: /node_modules/foo/ts-old/index.d.ts +//// export const index = 0; + +// @Filename: /node_modules/foo/ts-old/blah.d.ts +//// export const blah = 0; + +// @Filename: /node_modules/foo/ts-old/subfolder/one.d.ts +//// export const one = 0; + +// @Filename: /a.ts +//// import { } from "foo//**/"; + +verify.completions({ + marker: "", + isNewIdentifierLocation: true, + exact: ["nope", "ts-old"], +}); diff --git a/tests/cases/fourslash/pathCompletionsTypesVersionsWildcard3.ts b/tests/cases/fourslash/pathCompletionsTypesVersionsWildcard3.ts new file mode 100644 index 0000000000000..0d6d704b14a54 --- /dev/null +++ b/tests/cases/fourslash/pathCompletionsTypesVersionsWildcard3.ts @@ -0,0 +1,48 @@ +/// + +// @module: commonjs + +// @Filename: /node_modules/foo/package.json +//// { +//// "types": "index.d.ts", +//// "typesVersions": { +//// ">=4.3.5": { +//// "browser/*": ["dist/*"] +//// } +//// } +//// } + +// @Filename: /node_modules/foo/nope.d.ts +//// export const nope = 0; + +// @Filename: /node_modules/foo/dist/index.d.ts +//// export const index = 0; + +// @Filename: /node_modules/foo/dist/blah.d.ts +//// export const blah = 0; + +// @Filename: /node_modules/foo/dist/subfolder/one.d.ts +//// export const one = 0; + +// @Filename: /a.ts +//// import { } from "foo//**/"; + +verify.completions({ + marker: "", + isNewIdentifierLocation: true, + exact: ["browser", "nope", "dist"], +}); + +edit.insert("browser/"); + +verify.completions({ + isNewIdentifierLocation: true, + exact: ["blah", "index", "subfolder"], +}); + +edit.insert("subfolder/"); + +verify.completions({ + isNewIdentifierLocation: true, + exact: ["one"], +}); diff --git a/tests/cases/fourslash/pathCompletionsTypesVersionsWildcard4.ts b/tests/cases/fourslash/pathCompletionsTypesVersionsWildcard4.ts new file mode 100644 index 0000000000000..038bda22ac165 --- /dev/null +++ b/tests/cases/fourslash/pathCompletionsTypesVersionsWildcard4.ts @@ -0,0 +1,41 @@ +/// + +// @module: commonjs + +// @Filename: /node_modules/foo/package.json +//// { +//// "types": "index.d.ts", +//// "typesVersions": { +//// ">=4.3.5": { +//// "component-*": ["cjs/components/*"] +//// } +//// } +//// } + +// @Filename: /node_modules/foo/nope.d.ts +//// export const nope = 0; + +// @Filename: /node_modules/foo/cjs/components/index.d.ts +//// export const index = 0; + +// @Filename: /node_modules/foo/cjs/components/blah.d.ts +//// export const blah = 0; + +// @Filename: /node_modules/foo/cjs/components/subfolder/one.d.ts +//// export const one = 0; + +// @Filename: /a.ts +//// import { } from "foo//**/"; + +verify.completions({ + marker: "", + isNewIdentifierLocation: true, + exact: ["component-blah", "component-index", "component-subfolder", "nope", "cjs"], +}); + +edit.insert("component-subfolder/"); + +verify.completions({ + isNewIdentifierLocation: true, + exact: ["one"], +}); diff --git a/tests/cases/fourslash/pathCompletionsTypesVersionsWildcard5.ts b/tests/cases/fourslash/pathCompletionsTypesVersionsWildcard5.ts new file mode 100644 index 0000000000000..a7e8f49dad1a5 --- /dev/null +++ b/tests/cases/fourslash/pathCompletionsTypesVersionsWildcard5.ts @@ -0,0 +1,54 @@ +/// + +// @module: commonjs + +// @Filename: /node_modules/foo/package.json +//// { +//// "types": "index.d.ts", +//// "typesVersions": { +//// "*": { +//// "*": ["dist/*"], +//// "foo/*": ["dist/*"], +//// "bar/*": ["dist/*"], +//// "exact-match": ["dist/index.d.ts"] +//// } +//// } +//// } + +// @Filename: /node_modules/foo/nope.d.ts +//// export const nope = 0; + +// @Filename: /node_modules/foo/dist/index.d.ts +//// export const index = 0; + +// @Filename: /node_modules/foo/dist/blah.d.ts +//// export const blah = 0; + +// @Filename: /node_modules/foo/dist/foo/onlyInFooFolder.d.ts +//// export const foo = 0; + +// @Filename: /node_modules/foo/dist/subfolder/one.d.ts +//// export const one = 0; + +// @Filename: /a.ts +//// import { } from "foo//**/"; + +verify.completions({ + marker: "", + isNewIdentifierLocation: true, + exact: ["blah", "index", "foo", "subfolder", "bar", "exact-match"], +}); + +edit.insert("foo/"); + +verify.completions({ + isNewIdentifierLocation: true, + exact: ["blah", "index", "foo", "subfolder"], +}); + +edit.insert("foo/"); + +verify.completions({ + isNewIdentifierLocation: true, + exact: ["onlyInFooFolder"], +}); diff --git a/tests/cases/fourslash/pathCompletionsTypesVersionsWildcard6.ts b/tests/cases/fourslash/pathCompletionsTypesVersionsWildcard6.ts new file mode 100644 index 0000000000000..c3f89a4fa6c44 --- /dev/null +++ b/tests/cases/fourslash/pathCompletionsTypesVersionsWildcard6.ts @@ -0,0 +1,57 @@ +/// + +// This is the same test as pathCompletionsTypesVersionsWildcard5, but +// with the path patterns shuffled to ensure iteration order doesn't matter. + +// @module: commonjs + +// @Filename: /node_modules/foo/package.json +//// { +//// "types": "index.d.ts", +//// "typesVersions": { +//// "*": { +//// "bar/*": ["dist/*"], +//// "exact-match": ["dist/index.d.ts"], +//// "foo/*": ["dist/*"], +//// "*": ["dist/*"] +//// } +//// } +//// } + +// @Filename: /node_modules/foo/nope.d.ts +//// export const nope = 0; + +// @Filename: /node_modules/foo/dist/index.d.ts +//// export const index = 0; + +// @Filename: /node_modules/foo/dist/blah.d.ts +//// export const blah = 0; + +// @Filename: /node_modules/foo/dist/foo/onlyInFooFolder.d.ts +//// export const foo = 0; + +// @Filename: /node_modules/foo/dist/subfolder/one.d.ts +//// export const one = 0; + +// @Filename: /a.ts +//// import { } from "foo//**/"; + +verify.completions({ + marker: "", + isNewIdentifierLocation: true, + exact: ["bar", "exact-match", "foo", "blah", "index", "subfolder"], +}); + +edit.insert("foo/"); + +verify.completions({ + isNewIdentifierLocation: true, + exact: ["blah", "index", "foo", "subfolder"], +}); + +edit.insert("foo/"); + +verify.completions({ + isNewIdentifierLocation: true, + exact: ["onlyInFooFolder"], +});