From 1d2579204fe271de0e2e722b4a712c37736581cc Mon Sep 17 00:00:00 2001 From: Andrew Branch Date: Fri, 11 Nov 2022 13:52:14 -0800 Subject: [PATCH 1/2] Fix auto-import when `paths` points to project reference redirect --- src/compiler/moduleSpecifiers.ts | 22 +++++----- src/harness/fourslashImpl.ts | 4 ++ .../autoImportCrossProject_baseUrl_toDist.ts | 41 ++++++++++++++++++ .../autoImportCrossProject_paths_toDist2.ts | 43 +++++++++++++++++++ 4 files changed, 98 insertions(+), 12 deletions(-) create mode 100644 tests/cases/fourslash/server/autoImportCrossProject_baseUrl_toDist.ts create mode 100644 tests/cases/fourslash/server/autoImportCrossProject_paths_toDist2.ts diff --git a/src/compiler/moduleSpecifiers.ts b/src/compiler/moduleSpecifiers.ts index a322e648f8b03..45ad64ef7142a 100644 --- a/src/compiler/moduleSpecifiers.ts +++ b/src/compiler/moduleSpecifiers.ts @@ -285,12 +285,12 @@ function computeModuleSpecifiers( return nodeModulesSpecifiers!; } - if (!specifier && !modulePath.isRedirect) { + if (!specifier) { const local = getLocalModuleSpecifier(modulePath.path, info, compilerOptions, host, options.overrideImportMode || importingSourceFile.impliedNodeFormat, preferences); if (pathIsBareSpecifier(local)) { pathsSpecifiers = append(pathsSpecifiers, local); } - else if (!importedFileIsInNodeModules || modulePath.isInNodeModules) { + else if (!modulePath.isRedirect && (!importedFileIsInNodeModules || modulePath.isInNodeModules)) { // Why this extra conditional, not just an `else`? If some path to the file contained // 'node_modules', but we can't create a non-relative specifier (e.g. "@foo/bar/path/to/file"), // that means we had to go through a *sibling's* node_modules, not one we can access directly. @@ -338,16 +338,16 @@ function getLocalModuleSpecifier(moduleFileName: string, info: Info, compilerOpt } const fromPaths = paths && tryGetModuleNameFromPaths(relativeToBaseUrl, paths, getAllowedEndings(ending, compilerOptions, importMode), host, compilerOptions); - const nonRelative = fromPaths === undefined && baseUrl !== undefined ? removeExtensionAndIndexPostFix(relativeToBaseUrl, ending, compilerOptions) : fromPaths; - if (!nonRelative) { + const maybeNonRelative = fromPaths === undefined && baseUrl !== undefined ? removeExtensionAndIndexPostFix(relativeToBaseUrl, ending, compilerOptions) : fromPaths; + if (!maybeNonRelative) { return relativePath; } - if (relativePreference === RelativePreference.NonRelative) { - return nonRelative; + if (relativePreference === RelativePreference.NonRelative && !pathIsRelative(maybeNonRelative)) { + return maybeNonRelative; } - if (relativePreference === RelativePreference.ExternalNonRelative) { + if (relativePreference === RelativePreference.ExternalNonRelative && !pathIsRelative(maybeNonRelative)) { const projectDirectory = compilerOptions.configFilePath ? toPath(getDirectoryPath(compilerOptions.configFilePath), host.getCurrentDirectory(), info.getCanonicalFileName) : info.getCanonicalFileName(host.getCurrentDirectory()); @@ -363,7 +363,7 @@ function getLocalModuleSpecifier(moduleFileName: string, info: Info, compilerOpt // lib/ | (path crosses tsconfig.json) // imported.ts <--- // - return nonRelative; + return maybeNonRelative; } const nearestTargetPackageJson = getNearestAncestorDirectoryWithPackageJson(host, getDirectoryPath(modulePath)); @@ -378,16 +378,14 @@ function getLocalModuleSpecifier(moduleFileName: string, info: Info, compilerOpt // package.json | // component.ts <--- // - return nonRelative; + return maybeNonRelative; } return relativePath; } - if (relativePreference !== RelativePreference.Shortest) Debug.assertNever(relativePreference); - // Prefer a relative import over a baseUrl import if it has fewer components. - return isPathRelativeToParent(nonRelative) || countPathComponents(relativePath) < countPathComponents(nonRelative) ? relativePath : nonRelative; + return isPathRelativeToParent(maybeNonRelative) || countPathComponents(relativePath) < countPathComponents(maybeNonRelative) ? relativePath : maybeNonRelative; } /** @internal */ diff --git a/src/harness/fourslashImpl.ts b/src/harness/fourslashImpl.ts index 7e1e40fe17120..ac7c0c40ad3b5 100644 --- a/src/harness/fourslashImpl.ts +++ b/src/harness/fourslashImpl.ts @@ -3041,6 +3041,10 @@ export class TestState { * @param fileName Path to file where error should be retrieved from. */ private getCodeFixes(fileName: string, errorCode?: number, preferences: ts.UserPreferences = ts.emptyOptions, position?: number): readonly ts.CodeFixAction[] { + if (this.testType === FourSlashTestType.Server) { + this.configure(preferences); + } + const diagnosticsForCodeFix = this.getDiagnostics(fileName, /*includeSuggestions*/ true).map(diagnostic => ({ start: diagnostic.start, length: diagnostic.length, diff --git a/tests/cases/fourslash/server/autoImportCrossProject_baseUrl_toDist.ts b/tests/cases/fourslash/server/autoImportCrossProject_baseUrl_toDist.ts new file mode 100644 index 0000000000000..d6b6689d4e1a7 --- /dev/null +++ b/tests/cases/fourslash/server/autoImportCrossProject_baseUrl_toDist.ts @@ -0,0 +1,41 @@ +/// + +// @Filename: /common/tsconfig.json +//// { +//// "compilerOptions": { +//// "module": "commonjs", +//// "outDir": "dist", +//// "composite": true +//// }, +//// "include": ["src"] +//// } + +// @Filename: /common/src/MyModule.ts +//// export function square(n: number) { +//// return n * 2; +//// } + +// @Filename: /web/tsconfig.json +//// { +//// "compilerOptions": { +//// "module": "esnext", +//// "moduleResolution": "node", +//// "noEmit": true, +//// "baseUrl": "." +//// }, +//// "include": ["src"], +//// "references": [{ "path": "../common" }] +//// } + +// @Filename: /web/src/MyApp.ts +//// import { square } from "../../common/dist/src/MyModule"; + +// @Filename: /web/src/Helper.ts +//// export function saveMe() { +//// square/**/(2); +//// } + +goTo.file("/web/src/Helper.ts"); +verify.importFixModuleSpecifiers("", ["../../common/src/MyModule"], { + importModuleSpecifierPreference: "non-relative" +}); diff --git a/tests/cases/fourslash/server/autoImportCrossProject_paths_toDist2.ts b/tests/cases/fourslash/server/autoImportCrossProject_paths_toDist2.ts new file mode 100644 index 0000000000000..4a7f7af5d14b3 --- /dev/null +++ b/tests/cases/fourslash/server/autoImportCrossProject_paths_toDist2.ts @@ -0,0 +1,43 @@ +/// + +// @Filename: /common/tsconfig.json +//// { +//// "compilerOptions": { +//// "module": "commonjs", +//// "outDir": "dist", +//// "composite": true +//// }, +//// "include": ["src"] +//// } + +// @Filename: /common/src/MyModule.ts +//// export function square(n: number) { +//// return n * 2; +//// } + +// @Filename: /web/tsconfig.json +//// { +//// "compilerOptions": { +//// "module": "esnext", +//// "moduleResolution": "node", +//// "noEmit": true, +//// "paths": { +//// "@common/*": ["../common/dist/src/*"] +//// } +//// }, +//// "include": ["src"], +//// "references": [{ "path": "../common" }] +//// } + +// @Filename: /web/src/MyApp.ts +//// import { square } from "@common/MyModule"; + +// @Filename: /web/src/Helper.ts +//// export function saveMe() { +//// square/**/(2); +//// } + +goTo.file("/web/src/Helper.ts"); +verify.importFixModuleSpecifiers("", ["@common/MyModule"], { + importModuleSpecifierPreference: "non-relative" +}); From 4f5d243fca14f7fa5b1466eb334f55ccc8674a8a Mon Sep 17 00:00:00 2001 From: Andrew Branch Date: Fri, 11 Nov 2022 14:32:54 -0800 Subject: [PATCH 2/2] Put paths specifiers to redirects in lower priority bucket --- src/compiler/moduleSpecifiers.ts | 40 ++++++++++++++++++++++++++------ 1 file changed, 33 insertions(+), 7 deletions(-) diff --git a/src/compiler/moduleSpecifiers.ts b/src/compiler/moduleSpecifiers.ts index 45ad64ef7142a..21482e577a383 100644 --- a/src/compiler/moduleSpecifiers.ts +++ b/src/compiler/moduleSpecifiers.ts @@ -275,6 +275,7 @@ function computeModuleSpecifiers( // 4. Relative paths let nodeModulesSpecifiers: string[] | undefined; let pathsSpecifiers: string[] | undefined; + let redirectPathsSpecifiers: string[] | undefined; let relativeSpecifiers: string[] | undefined; for (const modulePath of modulePaths) { const specifier = tryGetModuleNameAsNodeModule(modulePath, info, importingSourceFile, host, compilerOptions, userPreferences, /*packageNameOnly*/ undefined, options.overrideImportMode); @@ -286,11 +287,25 @@ function computeModuleSpecifiers( } if (!specifier) { - const local = getLocalModuleSpecifier(modulePath.path, info, compilerOptions, host, options.overrideImportMode || importingSourceFile.impliedNodeFormat, preferences); - if (pathIsBareSpecifier(local)) { + const local = getLocalModuleSpecifier( + modulePath.path, + info, + compilerOptions, + host, + options.overrideImportMode || importingSourceFile.impliedNodeFormat, + preferences, + /*pathsOnly*/ modulePath.isRedirect, + ); + if (!local) { + continue; + } + if (modulePath.isRedirect) { + redirectPathsSpecifiers = append(redirectPathsSpecifiers, local); + } + else if (pathIsBareSpecifier(local)) { pathsSpecifiers = append(pathsSpecifiers, local); } - else if (!modulePath.isRedirect && (!importedFileIsInNodeModules || modulePath.isInNodeModules)) { + else if (!importedFileIsInNodeModules || modulePath.isInNodeModules) { // Why this extra conditional, not just an `else`? If some path to the file contained // 'node_modules', but we can't create a non-relative specifier (e.g. "@foo/bar/path/to/file"), // that means we had to go through a *sibling's* node_modules, not one we can access directly. @@ -306,8 +321,9 @@ function computeModuleSpecifiers( } return pathsSpecifiers?.length ? pathsSpecifiers : + redirectPathsSpecifiers?.length ? redirectPathsSpecifiers : nodeModulesSpecifiers?.length ? nodeModulesSpecifiers : - Debug.checkDefined(relativeSpecifiers); + Debug.checkDefined(relativeSpecifiers); } interface Info { @@ -322,22 +338,32 @@ function getInfo(importingSourceFileName: Path, host: ModuleSpecifierResolutionH return { getCanonicalFileName, importingSourceFileName, sourceDirectory }; } -function getLocalModuleSpecifier(moduleFileName: string, info: Info, compilerOptions: CompilerOptions, host: ModuleSpecifierResolutionHost, importMode: ResolutionMode, { ending, relativePreference }: Preferences): string { +function getLocalModuleSpecifier(moduleFileName: string, info: Info, compilerOptions: CompilerOptions, host: ModuleSpecifierResolutionHost, importMode: ResolutionMode, { ending, relativePreference }: Preferences): string; +function getLocalModuleSpecifier(moduleFileName: string, info: Info, compilerOptions: CompilerOptions, host: ModuleSpecifierResolutionHost, importMode: ResolutionMode, { ending, relativePreference }: Preferences, pathsOnly?: boolean): string | undefined; +function getLocalModuleSpecifier(moduleFileName: string, info: Info, compilerOptions: CompilerOptions, host: ModuleSpecifierResolutionHost, importMode: ResolutionMode, { ending, relativePreference }: Preferences, pathsOnly?: boolean): string | undefined { const { baseUrl, paths, rootDirs } = compilerOptions; + if (pathsOnly && !paths) { + return undefined; + } + const { sourceDirectory, getCanonicalFileName } = info; const relativePath = rootDirs && tryGetModuleNameFromRootDirs(rootDirs, moduleFileName, sourceDirectory, getCanonicalFileName, ending, compilerOptions) || removeExtensionAndIndexPostFix(ensurePathIsNonModuleName(getRelativePathFromDirectory(sourceDirectory, moduleFileName, getCanonicalFileName)), ending, compilerOptions); if (!baseUrl && !paths || relativePreference === RelativePreference.Relative) { - return relativePath; + return pathsOnly ? undefined : relativePath; } const baseDirectory = getNormalizedAbsolutePath(getPathsBasePath(compilerOptions, host) || baseUrl!, host.getCurrentDirectory()); const relativeToBaseUrl = getRelativePathIfInDirectory(moduleFileName, baseDirectory, getCanonicalFileName); if (!relativeToBaseUrl) { - return relativePath; + return pathsOnly ? undefined : relativePath; } const fromPaths = paths && tryGetModuleNameFromPaths(relativeToBaseUrl, paths, getAllowedEndings(ending, compilerOptions, importMode), host, compilerOptions); + if (pathsOnly) { + return fromPaths; + } + const maybeNonRelative = fromPaths === undefined && baseUrl !== undefined ? removeExtensionAndIndexPostFix(relativeToBaseUrl, ending, compilerOptions) : fromPaths; if (!maybeNonRelative) { return relativePath;