Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Avoid repeating codefix work when resolving auto-import specifiers for completions #49442

Merged
merged 1 commit into from Jun 9, 2022
Merged

Avoid repeating codefix work when resolving auto-import specifiers for completions #49442

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
106 changes: 70 additions & 36 deletions src/services/codefixes/importFixes.ts
View file
Open in desktop
Expand Up @@ -40,6 +40,9 @@ namespace ts.codefix {
},
});

/**
* Computes multiple import additions to a file and writes them to a ChangeTracker.
*/
export interface ImportAdder {
hasFixes(): boolean;
addImportFromDiagnostic: (diagnostic: DiagnosticWithLocation, context: CodeFixContextBase) => void;
Expand Down Expand Up @@ -235,6 +238,47 @@ namespace ts.codefix {
}
}

/**
* Computes module specifiers for multiple import additions to a file.
*/
export interface ImportSpecifierResolver {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like I'm missing something obvious, but why not make this a function type?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really thought it was going to contain more than one method.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be fair in some languages they're the same. Just not in our language. :D

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I thought about moving more (all?) of the exported functions into here so that other things that want to call importFixes.ts logic don’t make the same mistake. E.g., I’m pretty sure the same inefficiency happens with the “add missing properties/methods” codefix when it needs to import more than one type to write out signatures. I decided against a bigger refactoring since we were considering this for a patch, but in the future I would like this object to hold all the importing-file-specific info involved with computing module specifiers that could get reused between resolving multiple module specifiers. That will also help with the growing issue in this file that every function takes about a dozen parameters that just get fed through from the top. It will be much easier to read these function signatures if we just close over importingFile, program, host, preferences, formatContext, packageJsonImportFilter, exportInfoMap, .... So that’s why I don’t want to replace this with createGetModuleSpecifierForBestExportInfo that just returns the one function.

getModuleSpecifierForBestExportInfo(
exportInfo: readonly SymbolExportInfo[],
symbolName: string,
position: number,
isValidTypeOnlyUseSite: boolean,
fromCacheOnly?: boolean
): { exportInfo?: SymbolExportInfo, moduleSpecifier: string, computedWithoutCacheCount: number } | undefined;
}

export function createImportSpecifierResolver(importingFile: SourceFile, program: Program, host: LanguageServiceHost, preferences: UserPreferences): ImportSpecifierResolver {
const packageJsonImportFilter = createPackageJsonImportFilter(importingFile, preferences, host);
const importMap = createExistingImportMap(program.getTypeChecker(), importingFile, program.getCompilerOptions());
amcasey marked this conversation as resolved.
Show resolved Hide resolved
return { getModuleSpecifierForBestExportInfo };

function getModuleSpecifierForBestExportInfo(
exportInfo: readonly SymbolExportInfo[],
symbolName: string,
position: number,
isValidTypeOnlyUseSite: boolean,
fromCacheOnly?: boolean,
): { exportInfo?: SymbolExportInfo, moduleSpecifier: string, computedWithoutCacheCount: number } | undefined {
const { fixes, computedWithoutCacheCount } = getImportFixes(
exportInfo,
{ symbolName, position },
isValidTypeOnlyUseSite,
/*useRequire*/ false,
program,
importingFile,
host,
preferences,
importMap,
fromCacheOnly);
const result = getBestFix(fixes, importingFile, program, packageJsonImportFilter, host);
return result && { ...result, computedWithoutCacheCount };
}
}

// Sorted with the preferred fix coming first.
const enum ImportFixKind { UseNamespace, JsdocTypeImport, AddToExisting, AddNew, PromoteTypeOnly }
// These should not be combined as bitflags, but are given powers of 2 values to
Expand Down Expand Up @@ -394,32 +438,6 @@ namespace ts.codefix {
}
}

export function getModuleSpecifierForBestExportInfo(
exportInfo: readonly SymbolExportInfo[],
symbolName: string,
position: number,
isValidTypeOnlyUseSite: boolean,
importingFile: SourceFile,
program: Program,
host: LanguageServiceHost,
preferences: UserPreferences,
packageJsonImportFilter?: PackageJsonImportFilter,
fromCacheOnly?: boolean,
): { exportInfo?: SymbolExportInfo, moduleSpecifier: string, computedWithoutCacheCount: number } | undefined {
const { fixes, computedWithoutCacheCount } = getImportFixes(
exportInfo,
{ symbolName, position },
isValidTypeOnlyUseSite,
/*useRequire*/ false,
program,
importingFile,
host,
preferences,
fromCacheOnly);
const result = getBestFix(fixes, importingFile, program, packageJsonImportFilter || createPackageJsonImportFilter(importingFile, preferences, host), host);
return result && { ...result, computedWithoutCacheCount };
}

function getImportFixes(
exportInfos: readonly SymbolExportInfo[],
useNamespaceInfo: {
Expand All @@ -433,10 +451,11 @@ namespace ts.codefix {
sourceFile: SourceFile,
host: LanguageServiceHost,
preferences: UserPreferences,
importMap = createExistingImportMap(program.getTypeChecker(), sourceFile, program.getCompilerOptions()),
fromCacheOnly?: boolean,
): { computedWithoutCacheCount: number, fixes: readonly ImportFixWithModuleSpecifier[] } {
const checker = program.getTypeChecker();
const existingImports = flatMap(exportInfos, info => getExistingImportDeclarations(info, checker, sourceFile, program.getCompilerOptions()));
const existingImports = flatMap(exportInfos, importMap.getImportsForExportInfo);
const useNamespace = useNamespaceInfo && tryUseExistingNamespaceImport(existingImports, useNamespaceInfo.symbolName, useNamespaceInfo.position, checker);
const addToExisting = tryAddToExistingImport(existingImports, isValidTypeOnlyUseSite, checker, program.getCompilerOptions());
if (addToExisting) {
Expand Down Expand Up @@ -587,19 +606,34 @@ namespace ts.codefix {
});
}

function getExistingImportDeclarations({ moduleSymbol, exportKind, targetFlags, symbol }: SymbolExportInfo, checker: TypeChecker, importingFile: SourceFile, compilerOptions: CompilerOptions): readonly FixAddToExistingImportInfo[] {
// Can't use an es6 import for a type in JS.
if (!(targetFlags & SymbolFlags.Value) && isSourceFileJS(importingFile)) return emptyArray;
const importKind = getImportKind(importingFile, exportKind, compilerOptions);
return mapDefined(importingFile.imports, (moduleSpecifier): FixAddToExistingImportInfo | undefined => {
function createExistingImportMap(checker: TypeChecker, importingFile: SourceFile, compilerOptions: CompilerOptions) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something about the name of this feels off - what does existing refer to?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Import declarations that already exist in the file

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

createImportMapForExistingExports then probably

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh? It’s not exports that are existing, it’s the imports. See historical usage of ExistingImports all over this file 😄

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The purpose of this module is to add imports to a file. By definition, those imports do not exist yet. Contrast them with imports that do exist in the file, which can be used in various ways in the process of adding new ones. Those are what this map contains. It’s a map of imports that exist in the file. We need to specify that they exist because so much in the module deals with hypothetical imports that we might choose to write into existence. Everywhere an existing import is mentioned, it’s called an “existing import,” e.g. the type FixAddToExistingImport (a type of fix that adds a new specifier to an existing import declaration). So, I think it’s fair and consistent to say this is a map of existing imports. (More specifically, the keys are the symbol ids of other modules, and the values are the local import declarations that resolve to those modules.)

let importMap: MultiMap<SymbolId, AnyImportOrRequire> | undefined;
for (const moduleSpecifier of importingFile.imports) {
const i = importFromModuleSpecifier(moduleSpecifier);
if (isVariableDeclarationInitializedToRequire(i.parent)) {
return checker.resolveExternalModuleName(moduleSpecifier) === moduleSymbol ? { declaration: i.parent, importKind, symbol, targetFlags } : undefined;
const moduleSymbol = checker.resolveExternalModuleName(moduleSpecifier);
if (moduleSymbol) {
(importMap ||= createMultiMap()).add(getSymbolId(moduleSymbol), i.parent);
}
}
if (i.kind === SyntaxKind.ImportDeclaration || i.kind === SyntaxKind.ImportEqualsDeclaration) {
return checker.getSymbolAtLocation(moduleSpecifier) === moduleSymbol ? { declaration: i, importKind, symbol, targetFlags } : undefined;
else if (i.kind === SyntaxKind.ImportDeclaration || i.kind === SyntaxKind.ImportEqualsDeclaration) {
const moduleSymbol = checker.getSymbolAtLocation(moduleSpecifier);
if (moduleSymbol) {
(importMap ||= createMultiMap()).add(getSymbolId(moduleSymbol), i);
}
}
});
}

return {
getImportsForExportInfo: ({ moduleSymbol, exportKind, targetFlags, symbol }: SymbolExportInfo): readonly FixAddToExistingImportInfo[] => {
// Can't use an es6 import for a type in JS.
if (!(targetFlags & SymbolFlags.Value) && isSourceFileJS(importingFile)) return emptyArray;
const matchingDeclarations = importMap?.get(getSymbolId(moduleSymbol));
if (!matchingDeclarations) return emptyArray;
const importKind = getImportKind(importingFile, exportKind, compilerOptions);
return matchingDeclarations.map(declaration => ({ declaration, importKind, symbol, targetFlags }));
}
};
}

function shouldUseRequire(sourceFile: SourceFile, program: Program): boolean {
Expand Down
16 changes: 8 additions & 8 deletions src/services/completions.ts
View file
Open in desktop
Expand Up @@ -184,16 +184,15 @@ namespace ts.Completions {
function resolvingModuleSpecifiers<TReturn>(
logPrefix: string,
host: LanguageServiceHost,
resolver: codefix.ImportSpecifierResolver,
program: Program,
sourceFile: SourceFile,
position: number,
preferences: UserPreferences,
isForImportStatementCompletion: boolean,
isValidTypeOnlyUseSite: boolean,
cb: (context: ModuleSpecifierResolutioContext) => TReturn,
): TReturn {
const start = timestamp();
const packageJsonImportFilter = createPackageJsonImportFilter(sourceFile, preferences, host);
// Under `--moduleResolution nodenext`, we have to resolve module specifiers up front, because
// package.json exports can mean we *can't* resolve a module specifier (that doesn't include a
// relative path into node_modules), and we want to filter those completions out entirely.
Expand Down Expand Up @@ -221,7 +220,7 @@ namespace ts.Completions {

function tryResolve(exportInfo: readonly SymbolExportInfo[], symbolName: string, isFromAmbientModule: boolean): ModuleSpecifierResolutionResult {
if (isFromAmbientModule) {
const result = codefix.getModuleSpecifierForBestExportInfo(exportInfo, symbolName, position, isValidTypeOnlyUseSite, sourceFile, program, host, preferences);
const result = resolver.getModuleSpecifierForBestExportInfo(exportInfo, symbolName, position, isValidTypeOnlyUseSite);
if (result) {
ambientCount++;
}
Expand All @@ -230,7 +229,7 @@ namespace ts.Completions {
const shouldResolveModuleSpecifier = needsFullResolution || preferences.allowIncompleteCompletions && resolvedCount < moduleSpecifierResolutionLimit;
const shouldGetModuleSpecifierFromCache = !shouldResolveModuleSpecifier && preferences.allowIncompleteCompletions && cacheAttemptCount < moduleSpecifierResolutionCacheAttemptLimit;
const result = (shouldResolveModuleSpecifier || shouldGetModuleSpecifierFromCache)
? codefix.getModuleSpecifierForBestExportInfo(exportInfo, symbolName, position, isValidTypeOnlyUseSite, sourceFile, program, host, preferences, packageJsonImportFilter, shouldGetModuleSpecifierFromCache)
? resolver.getModuleSpecifierForBestExportInfo(exportInfo, symbolName, position, isValidTypeOnlyUseSite, shouldGetModuleSpecifierFromCache)
: undefined;

if (!shouldResolveModuleSpecifier && !shouldGetModuleSpecifierFromCache || shouldGetModuleSpecifierFromCache && !result) {
Expand Down Expand Up @@ -371,8 +370,8 @@ namespace ts.Completions {
const newEntries = resolvingModuleSpecifiers(
"continuePreviousIncompleteResponse",
host,
codefix.createImportSpecifierResolver(file, program, host, preferences),
andrewbranch marked this conversation as resolved.
Show resolved Hide resolved
program,
file,
location.getStart(),
preferences,
/*isForImportStatementCompletion*/ false,
Expand Down Expand Up @@ -2206,6 +2205,7 @@ namespace ts.Completions {
let hasUnresolvedAutoImports = false;
// This also gets mutated in nested-functions after the return
let symbols: Symbol[] = [];
let importSpecifierResolver: codefix.ImportSpecifierResolver | undefined;
const symbolToOriginInfoMap: SymbolOriginInfoMap = [];
const symbolToSortTextMap: SymbolSortTextMap = [];
const seenPropertySymbols = new Map<SymbolId, true>();
Expand Down Expand Up @@ -2448,14 +2448,14 @@ namespace ts.Completions {
}
else {
const fileName = isExternalModuleNameRelative(stripQuotes(moduleSymbol.name)) ? getSourceFileOfModule(moduleSymbol)?.fileName : undefined;
const { moduleSpecifier } = codefix.getModuleSpecifierForBestExportInfo([{
const { moduleSpecifier } = (importSpecifierResolver ||= codefix.createImportSpecifierResolver(sourceFile, program, host, preferences)).getModuleSpecifierForBestExportInfo([{
andrewbranch marked this conversation as resolved.
Show resolved Hide resolved
exportKind: ExportKind.Named,
moduleFileName: fileName,
isFromPackageJson: false,
moduleSymbol,
symbol: firstAccessibleSymbol,
targetFlags: skipAlias(firstAccessibleSymbol, typeChecker).flags,
}], firstAccessibleSymbol.name, position, isValidTypeOnlyAliasUseSite(location), sourceFile, program, host, preferences) || {};
}], firstAccessibleSymbol.name, position, isValidTypeOnlyAliasUseSite(location)) || {};

if (moduleSpecifier) {
const origin: SymbolOriginInfoResolvedExport = {
Expand Down Expand Up @@ -2733,8 +2733,8 @@ namespace ts.Completions {
resolvingModuleSpecifiers(
"collectAutoImports",
host,
importSpecifierResolver ||= codefix.createImportSpecifierResolver(sourceFile, program, host, preferences),
program,
sourceFile,
position,
preferences,
!!importCompletionNode,
Expand Down