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.
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
Avoid repeating codefix work when resolving auto-import specifiers for completions #49442
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -235,6 +238,47 @@ namespace ts.codefix { | |
} | ||
} | ||
|
||
/** | ||
* Computes module specifiers for multiple import additions to a file. | ||
*/ | ||
export interface ImportSpecifierResolver { | ||
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 | ||
|
@@ -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: { | ||
|
@@ -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) { | ||
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Something about the name of this feels off - what does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Import declarations that already exist in the file There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Huh? It’s not exports that are There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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 { | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 withcreateGetModuleSpecifierForBestExportInfo
that just returns the one function.