maxmustermann/TypeScript
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

importFixes: When one file redirects to another, consider both for global import specifiers (#25834)

Browse source 
* importFixes: When one file redirects to another, consider both for global import specifiers

* Add test for #26044

* Avoid a symlinked package globally importing itself (fixes another case of #26044)

* Compare to node_modules with getCanonicalFileName
This commit is contained in:
Andy 2018-07-31 17:28:56 -07:00 committed by GitHub
parent f326b4b7cb
commit 9c9f3e3cf9
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
11 changed files with 187 additions and 63 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

7
src/compiler/checker.ts
View file

@ -3888,14 +3888,15 @@ namespace ts {
const links = getSymbolLinks(symbol);
let specifier = links.specifierCache && links.specifierCache.get(contextFile.path);
if (!specifier) {
specifier = flatten(moduleSpecifiers.getModuleSpecifiers(
specifier = moduleSpecifiers.getModuleSpecifierForDeclarationFile(
symbol,
compilerOptions,
contextFile,
context.tracker.moduleResolverHost,
context.tracker.moduleResolverHost.getSourceFiles!(), // TODO: GH#18217
{ importModuleSpecifierPreference: "non-relative" }
))[0];
{ importModuleSpecifierPreference: "non-relative" },
host.redirectTargetsMap,
);
links.specifierCache = links.specifierCache || createMap();
links.specifierCache.set(contextFile.path, specifier);
}

120
src/compiler/moduleSpecifiers.ts
View file

@ -1,4 +1,4 @@
// Used by importFixes to synthesize import module specifiers.
// Used by importFixes, getEditsForFileRename, and declaration emit to synthesize import module specifiers.
/* @internal */
namespace ts.moduleSpecifiers {
export interface ModuleSpecifierPreferences {
@ -14,13 +14,26 @@ namespace ts.moduleSpecifiers {
host: ModuleSpecifierResolutionHost,
files: ReadonlyArray<SourceFile>,
preferences: ModuleSpecifierPreferences = {},
redirectTargetsMap: RedirectTargetsMap,
): string {
const info = getInfo(compilerOptions, importingSourceFile, importingSourceFileName, host);
const modulePaths = getAllModulePaths(files, toFileName, info.getCanonicalFileName, host);
const modulePaths = getAllModulePaths(files, importingSourceFileName, toFileName, info.getCanonicalFileName, host, redirectTargetsMap);
return firstDefined(modulePaths, moduleFileName => getGlobalModuleSpecifier(moduleFileName, info, host, compilerOptions)) ||
first(getLocalModuleSpecifiers(toFileName, info, compilerOptions, preferences));
}
export function getModuleSpecifierForDeclarationFile(
moduleSymbol: Symbol,
compilerOptions: CompilerOptions,
importingSourceFile: SourceFile,
host: ModuleSpecifierResolutionHost,
files: ReadonlyArray<SourceFile>,
preferences: ModuleSpecifierPreferences,
redirectTargetsMap: RedirectTargetsMap,
): string {
return first(first(getModuleSpecifiers(moduleSymbol, compilerOptions, importingSourceFile, host, files, preferences, redirectTargetsMap)));
}
// For each symlink/original for a module, returns a list of ways to import that file.
export function getModuleSpecifiers(
moduleSymbol: Symbol,
@ -29,6 +42,7 @@ namespace ts.moduleSpecifiers {
host: ModuleSpecifierResolutionHost,
files: ReadonlyArray<SourceFile>,
preferences: ModuleSpecifierPreferences,
redirectTargetsMap: RedirectTargetsMap,
): ReadonlyArray<ReadonlyArray<string>> {
const ambient = tryGetModuleNameFromAmbientModule(moduleSymbol);
if (ambient) return [[ambient]];
@ -37,7 +51,8 @@ namespace ts.moduleSpecifiers {
if (!files) {
return Debug.fail("Files list must be present to resolve symlinks in specifier resolution");
}
const modulePaths = getAllModulePaths(files, getSourceFileOfNode(moduleSymbol.valueDeclaration).fileName, info.getCanonicalFileName, host);
const moduleSourceFile = getSourceFileOfNode(moduleSymbol.valueDeclaration);
const modulePaths = getAllModulePaths(files, importingSourceFile.path, moduleSourceFile.fileName, info.getCanonicalFileName, host, redirectTargetsMap);
const global = mapDefined(modulePaths, moduleFileName => getGlobalModuleSpecifier(moduleFileName, info, host, compilerOptions));
return global.length ? global.map(g => [g]) : modulePaths.map(moduleFileName =>
@ -142,64 +157,69 @@ namespace ts.moduleSpecifiers {
return firstDefined(imports, ({ text }) => pathIsRelative(text) ? fileExtensionIs(text, Extension.Js) : undefined) || false;
}
function discoverProbableSymlinks(files: ReadonlyArray<SourceFile>, getCanonicalFileName: (file: string) => string, host: ModuleSpecifierResolutionHost) {
function stringsEqual(a: string, b: string, getCanonicalFileName: GetCanonicalFileName): boolean {
return getCanonicalFileName(a) === getCanonicalFileName(b);
}
// KLUDGE: Don't assume one 'node_modules' links to another. More likely a single directory inside the node_modules is the symlink.
// ALso, don't assume that an `@foo` directory is linked. More likely the contents of that are linked.
function isNodeModulesOrScopedPackageDirectory(s: string, getCanonicalFileName: GetCanonicalFileName): boolean {
return getCanonicalFileName(s) === "node_modules" || startsWith(s, "@");
}
function guessDirectorySymlink(a: string, b: string, cwd: string, getCanonicalFileName: GetCanonicalFileName): [string, string] {
const aParts = getPathComponents(toPath(a, cwd, getCanonicalFileName));
const bParts = getPathComponents(toPath(b, cwd, getCanonicalFileName));
while (!isNodeModulesOrScopedPackageDirectory(aParts[aParts.length - 2], getCanonicalFileName) &&
!isNodeModulesOrScopedPackageDirectory(bParts[bParts.length - 2], getCanonicalFileName) &&
stringsEqual(aParts[aParts.length - 1], bParts[bParts.length - 1], getCanonicalFileName)) {
aParts.pop();
bParts.pop();
}
return [getPathFromPathComponents(aParts), getPathFromPathComponents(bParts)];
}
function discoverProbableSymlinks(files: ReadonlyArray<SourceFile>, getCanonicalFileName: GetCanonicalFileName, cwd: string): ReadonlyMap<string> {
const result = createMap<string>();
const symlinks = mapDefined(files, sf =>
sf.resolvedModules && firstDefinedIterator(sf.resolvedModules.values(), res =>
res && res.originalPath && res.resolvedFileName !== res.originalPath ? [res.resolvedFileName, res.originalPath] : undefined));
const result = createMap<string>();
if (symlinks) {
const currentDirectory = host.getCurrentDirectory ? host.getCurrentDirectory() : "";
const compareStrings = (!host.useCaseSensitiveFileNames || host.useCaseSensitiveFileNames()) ? compareStringsCaseSensitive : compareStringsCaseInsensitive;
for (const [resolvedPath, originalPath] of symlinks) {
const resolvedParts = getPathComponents(toPath(resolvedPath, currentDirectory, getCanonicalFileName));
const originalParts = getPathComponents(toPath(originalPath, currentDirectory, getCanonicalFileName));
while (compareStrings(resolvedParts[resolvedParts.length - 1], originalParts[originalParts.length - 1]) === Comparison.EqualTo) {
resolvedParts.pop();
originalParts.pop();
}
result.set(getPathFromPathComponents(originalParts), getPathFromPathComponents(resolvedParts));
}
for (const [resolvedPath, originalPath] of symlinks) {
const [commonResolved, commonOriginal] = guessDirectorySymlink(resolvedPath, originalPath, cwd, getCanonicalFileName);
result.set(commonOriginal, commonResolved);
}
return result;
}
function getAllModulePathsUsingIndirectSymlinks(files: ReadonlyArray<SourceFile>, target: string, getCanonicalFileName: (file: string) => string, host: ModuleSpecifierResolutionHost) {
const links = discoverProbableSymlinks(files, getCanonicalFileName, host);
const paths = arrayFrom(links.keys());
let options: string[] | undefined;
const compareStrings = (!host.useCaseSensitiveFileNames || host.useCaseSensitiveFileNames()) ? compareStringsCaseSensitive : compareStringsCaseInsensitive;
for (const path of paths) {
const resolved = links.get(path)!;
if (compareStrings(target.slice(0, resolved.length + 1), resolved + "/") === Comparison.EqualTo) {
const relative = getRelativePathFromDirectory(resolved, target, getCanonicalFileName);
const option = resolvePath(path, relative);
if (!host.fileExists || host.fileExists(option)) {
if (!options) options = [];
options.push(option);
}
}
}
if (options) {
options.push(target); // Since these are speculative, we also include the original resolved name as a possibility
return options;
}
return [target];
}
/**
* Looks for existing imports that use symlinks to this module.
* Only if no symlink is available, the real path will be used.
* Symlinks will be returned first so they are preferred over the real path.
*/
function getAllModulePaths(files: ReadonlyArray<SourceFile>, importedFileName: string, getCanonicalFileName: (file: string) => string, host: ModuleSpecifierResolutionHost): ReadonlyArray<string> {
const symlinks = mapDefined(files, sf =>
sf.resolvedModules && firstDefinedIterator(sf.resolvedModules.values(), res =>
res && res.resolvedFileName === importedFileName ? res.originalPath : undefined));
function getAllModulePaths(files: ReadonlyArray<SourceFile>, importingFileName: string, importedFileName: string, getCanonicalFileName: GetCanonicalFileName, host: ModuleSpecifierResolutionHost, redirectTargetsMap: RedirectTargetsMap): ReadonlyArray<string> {
const redirects = redirectTargetsMap.get(importedFileName);
const importedFileNames = redirects ? [...redirects, importedFileName] : [importedFileName];
const cwd = host.getCurrentDirectory ? host.getCurrentDirectory() : "";
const baseOptions = getAllModulePathsUsingIndirectSymlinks(files, getNormalizedAbsolutePath(importedFileName, cwd), getCanonicalFileName, host);
if (symlinks.length === 0) {
return baseOptions;
}
return deduplicate(concatenate(baseOptions, flatMap(symlinks, importedFileName => getAllModulePathsUsingIndirectSymlinks(files, getNormalizedAbsolutePath(importedFileName, cwd), getCanonicalFileName, host))));
const targets = importedFileNames.map(f => getNormalizedAbsolutePath(f, cwd));
const links = discoverProbableSymlinks(files, getCanonicalFileName, cwd);
const result: string[] = [];
const compareStrings = (!host.useCaseSensitiveFileNames || host.useCaseSensitiveFileNames()) ? compareStringsCaseSensitive : compareStringsCaseInsensitive;
links.forEach((resolved, path) => {
if (startsWithDirectory(importingFileName, resolved, getCanonicalFileName)) {
return; // Don't want to a package to globally import from itself
}
const target = targets.find(t => compareStrings(t.slice(0, resolved.length + 1), resolved + "/") === Comparison.EqualTo);
if (target === undefined) return;
const relative = getRelativePathFromDirectory(resolved, target, getCanonicalFileName);
const option = resolvePath(path, relative);
if (!host.fileExists || host.fileExists(option)) {
result.push(option);
}
});
result.push(...targets);
return result;
}
function getRelativePathNParents(relativePath: string): number {

11
src/compiler/program.ts
View file

@ -634,7 +634,8 @@ namespace ts {
const packageIdToSourceFile = createMap<SourceFile>();
// Maps from a SourceFile's `.path` to the name of the package it was imported with.
let sourceFileToPackageName = createMap<string>();
let redirectTargetsSet = createMap<true>();
// Key is a file name. Value is the (non-empty, or undefined) list of files that redirect to it.
let redirectTargetsMap = createMultiMap<string>();
const filesByName = createMap<SourceFile | undefined>();
let missingFilePaths: ReadonlyArray<Path> | undefined;
@ -752,7 +753,7 @@ namespace ts {
getSourceFileFromReference,
getLibFileFromReference,
sourceFileToPackageName,
redirectTargetsSet,
redirectTargetsMap,
isEmittedFile,
getConfigFileParsingDiagnostics,
getResolvedModuleWithFailedLookupLocationsFromCache,
@ -1073,7 +1074,7 @@ namespace ts {
fileChanged = false;
newSourceFile = oldSourceFile; // Use the redirect.
}
else if (oldProgram.redirectTargetsSet.has(oldSourceFile.path)) {
else if (oldProgram.redirectTargetsMap.has(oldSourceFile.path)) {
// If a redirected-to source file changes, the redirect may be broken.
if (newSourceFile !== oldSourceFile) {
return oldProgram.structureIsReused = StructureIsReused.Not;
@ -1220,7 +1221,7 @@ namespace ts {
resolvedProjectReferences = oldProgram.getProjectReferences();
sourceFileToPackageName = oldProgram.sourceFileToPackageName;
redirectTargetsSet = oldProgram.redirectTargetsSet;
redirectTargetsMap = oldProgram.redirectTargetsMap;
return oldProgram.structureIsReused = StructureIsReused.Completely;
}
@ -2079,7 +2080,7 @@ namespace ts {
// Some other SourceFile already exists with this package name and version.
// Instead of creating a duplicate, just redirect to the existing one.
const dupFile = createRedirectSourceFile(fileFromPackageId, file!, fileName, path); // TODO: GH#18217
redirectTargetsSet.set(fileFromPackageId.path, true);
redirectTargetsMap.add(fileFromPackageId.path, fileName);
filesByName.set(path, dupFile);
sourceFileToPackageName.set(path, packageId.name);
processingOtherFiles!.push(dupFile);

9
src/compiler/types.ts
View file

@ -2551,7 +2551,7 @@ namespace ts {
/**
* If two source files are for the same version of the same package, one will redirect to the other.
* (See `createRedirectSourceFile` in program.ts.)
* The redirect will have this set. The redirected-to source file will be in `redirectTargetsSet`.
* The redirect will have this set. The redirected-to source file will be in `redirectTargetsMap`.
*/
/* @internal */ redirectInfo?: RedirectInfo;
@ -2798,7 +2798,7 @@ namespace ts {
/** Given a source file, get the name of the package it was imported from. */
/* @internal */ sourceFileToPackageName: Map<string>;
/** Set of all source files that some other source file redirects to. */
/* @internal */ redirectTargetsSet: Map<true>;
/* @internal */ redirectTargetsMap: MultiMap<string>;
/** Is the file emitted file */
/* @internal */ isEmittedFile(file: string): boolean;
@ -2807,6 +2807,9 @@ namespace ts {
getProjectReferences(): (ResolvedProjectReference | undefined)[] | undefined;
}
/* @internal */
export type RedirectTargetsMap = ReadonlyMap<ReadonlyArray<string>>;
export interface ResolvedProjectReference {
commandLine: ParsedCommandLine;
sourceFile: SourceFile;
@ -2884,6 +2887,8 @@ namespace ts {
getSourceFiles(): ReadonlyArray<SourceFile>;
getSourceFile(fileName: string): SourceFile | undefined;
getResolvedTypeReferenceDirectives(): ReadonlyMap<ResolvedTypeReferenceDirective>;
readonly redirectTargetsMap: RedirectTargetsMap;
}
export interface TypeChecker {

6
src/compiler/utilities.ts
View file

@ -7157,6 +7157,12 @@ namespace ts {
return path.slice(0, Math.max(rootLength, path.lastIndexOf(directorySeparator)));
}
export function startsWithDirectory(fileName: string, directoryName: string, getCanonicalFileName: GetCanonicalFileName): boolean {
const canonicalFileName = getCanonicalFileName(fileName);
const canonicalDirectoryName = getCanonicalFileName(directoryName);
return startsWith(canonicalFileName, canonicalDirectoryName + "/") || startsWith(canonicalFileName, canonicalDirectoryName + "\\");
}
export function isUrl(path: string) {
return getEncodedRootLength(path) < 0;
}

2
src/services/codefixes/importFixes.ts
View file

@ -275,7 +275,7 @@ namespace ts.codefix {
): ReadonlyArray<FixAddNewImport | FixUseImportType> {
const isJs = isSourceFileJavaScript(sourceFile);
const choicesForEachExportingModule = flatMap<SymbolExportInfo, ReadonlyArray<FixAddNewImport | FixUseImportType>>(moduleSymbols, ({ moduleSymbol, importKind, exportedSymbolIsTypeOnly }) => {
const modulePathsGroups = moduleSpecifiers.getModuleSpecifiers(moduleSymbol, program.getCompilerOptions(), sourceFile, host, program.getSourceFiles(), preferences);
const modulePathsGroups = moduleSpecifiers.getModuleSpecifiers(moduleSymbol, program.getCompilerOptions(), sourceFile, host, program.getSourceFiles(), preferences, program.redirectTargetsMap);
return modulePathsGroups.map(group => group.map((moduleSpecifier): FixAddNewImport | FixUseImportType =>
// `position` should only be undefined at a missing jsx namespace, in which case we shouldn't be looking for pure types.
exportedSymbolIsTypeOnly && isJs ? { kind: ImportFixKind.ImportType, moduleSpecifier, position: Debug.assertDefined(position) } : { kind: ImportFixKind.AddNew, moduleSpecifier, importKind }));

2
src/services/documentHighlights.ts
View file

@ -28,7 +28,7 @@ namespace ts.DocumentHighlights {
const map = arrayToMultiMap(referenceEntries.map(FindAllReferences.toHighlightSpan), e => e.fileName, e => e.span);
return arrayFrom(map.entries(), ([fileName, highlightSpans]) => {
if (!sourceFilesSet.has(fileName)) {
Debug.assert(program.redirectTargetsSet.has(fileName));
Debug.assert(program.redirectTargetsMap.has(fileName));
const redirectTarget = program.getSourceFile(fileName);
const redirect = find(sourceFilesToSearch, f => !!f.redirectInfo && f.redirectInfo.redirectTarget === redirectTarget)!;
fileName = redirect.fileName;

2
src/services/getEditsForFileRename.ts
View file

@ -156,7 +156,7 @@ namespace ts {
// Need an update if the imported file moved, or the importing file moved and was using a relative path.
return toImport !== undefined && (toImport.updated || (importingSourceFileMoved && pathIsRelative(importLiteral.text)))
? moduleSpecifiers.getModuleSpecifier(program.getCompilerOptions(), sourceFile, newImportFromPath, toImport.newFileName, host, allFiles, preferences)
? moduleSpecifiers.getModuleSpecifier(program.getCompilerOptions(), sourceFile, newImportFromPath, toImport.newFileName, host, allFiles, preferences, program.redirectTargetsMap)
: undefined;
});
}

53
src/testRunner/unittests/tsserverProjectSystem.ts
View file

@ -9535,6 +9535,59 @@ export function Test2() {
});
});
describe("duplicate packages", () => {
// Tests that 'moduleSpecifiers.ts' will import from the redirecting file, and not from the file it redirects to, if that can provide a global module specifier.
it("works with import fixes", () => {
const packageContent = "export const foo: number;";
const packageJsonContent = JSON.stringify({ name: "foo", version: "1.2.3" });
const aFooIndex: File = { path: "/a/node_modules/foo/index.d.ts", content: packageContent };
const aFooPackage: File = { path: "/a/node_modules/foo/package.json", content: packageJsonContent };
const bFooIndex: File = { path: "/b/node_modules/foo/index.d.ts", content: packageContent };
const bFooPackage: File = { path: "/b/node_modules/foo/package.json", content: packageJsonContent };
const userContent = 'import("foo");\nfoo';
const aUser: File = { path: "/a/user.ts", content: userContent };
const bUser: File = { path: "/b/user.ts", content: userContent };
const tsconfig: File = {
path: "/tsconfig.json",
content: "{}",
};
const host = createServerHost([aFooIndex, aFooPackage, bFooIndex, bFooPackage, aUser, bUser, tsconfig]);
const session = createSession(host);
openFilesForSession([aUser, bUser], session);
for (const user of [aUser, bUser]) {
const response = executeSessionRequest<protocol.CodeFixRequest, protocol.CodeFixResponse>(session, protocol.CommandTypes.GetCodeFixes, {
file: user.path,
startLine: 2,
startOffset: 1,
endLine: 2,
endOffset: 4,
errorCodes: [Diagnostics.Cannot_find_name_0.code],
});
assert.deepEqual<ReadonlyArray<protocol.CodeFixAction> | undefined>(response, [
{
description: `Import 'foo' from module "foo"`,
fixName: "import",
fixId: "fixMissingImport",
fixAllDescription: "Add all missing imports",
changes: [{
fileName: user.path,
textChanges: [{
start: { line: 1, offset: 1 },
end: { line: 1, offset: 1 },
newText: 'import { foo } from "foo";\n\n',
}],
}],
commands: undefined,
},
]);
}
});
});
describe("tsserverProjectSystem Untitled files", () => {
it("Can convert positions to locations", () => {
const aTs: File = { path: "/proj/a.ts", content: "" };

21
tests/cases/fourslash/importNameCodeFix_symlink_own_package.ts Normal file
View file

@ -0,0 +1,21 @@
/// <reference path="fourslash.ts" />
// @Filename: /packages/b/b0.ts
// @Symlink: /node_modules/b/b0.ts
////x;
// @Filename: /packages/b/b1.ts
// @Symlink: /node_modules/b/b1.ts
////import { a } from "a";
////export const x = 0;
// @Filename: /packages/a/index.d.ts
// @Symlink: /node_modules/a/index.d.ts
////export const a: number;
goTo.file("/packages/b/b0.ts");
verify.importFixAtPosition([
`import { x } from "./b1";
x;`,
]);

17
tests/cases/fourslash/importNameCodeFix_symlink_own_package_2.ts Normal file
View file

@ -0,0 +1,17 @@
/// <reference path="fourslash.ts" />
// @Filename: /packages/a/test.ts
// @Symlink: /node_modules/a/test.ts
////x;
// @Filename: /packages/a/utils.ts
// @Symlink: /node_modules/a/utils.ts
////import {} from "a/utils";
////export const x = 0;
goTo.file("/packages/a/test.ts");
verify.importFixAtPosition([
`import { x } from "./utils";
x;`,
]);