Skip to content

Proposal: simplify auto import descriptions #47631

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 9 additions & 13 deletions src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -6354,18 +6354,14 @@
"category": "Message",
"code": 90012
},
"Import '{0}' from module \"{1}\"": {
"Import '{0}' from \"{1}\"": {
Copy link
Member

Choose a reason for hiding this comment

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

This is used for both add and update? Also, nitpicky, but why one set of single quotes and one set of double quotes?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think I even see this message used the diff. Am I missing it?

Copy link
Member

Choose a reason for hiding this comment

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

I missed it too - it's referred to by its property name in the tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

@amcasey yes, both add and update. We always use double quotes for module specifiers for some reason, and single quotes for other code quoting 🤷‍♂️

"category": "Message",
"code": 90013
},
"Change '{0}' to '{1}'": {
"category": "Message",
"code": 90014
},
"Add '{0}' to existing import declaration from \"{1}\"": {
"category": "Message",
"code": 90015
},
"Declare property '{0}'": {
"category": "Message",
"code": 90016
Expand Down Expand Up @@ -6430,14 +6426,6 @@
"category": "Message",
"code": 90031
},
"Import default '{0}' from module \"{1}\"": {
"category": "Message",
"code": 90032
},
"Add default import '{0}' to existing import declaration from \"{1}\"": {
"category": "Message",
"code": 90033
},
"Add parameter name": {
"category": "Message",
"code": 90034
Expand Down Expand Up @@ -6482,6 +6470,14 @@
"category": "Message",
"code": 90056
},
"Add import from \"{0}\"": {
"category": "Message",
"code": 90057
},
"Update import from \"{0}\"": {
"category": "Message",
"code": 90058
},

"Convert function to an ES2015 class": {
"category": "Message",
Expand Down
43 changes: 26 additions & 17 deletions src/services/codefixes/importFixes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,16 @@ namespace ts.codefix {
const { errorCode, preferences, sourceFile, span, program } = context;
const info = getFixesInfo(context, errorCode, span.start, /*useAutoImportProvider*/ true);
if (!info) return undefined;
const { fixes, symbolName } = info;
const { fixes, symbolName, errorIdentifierText } = info;
const quotePreference = getQuotePreference(sourceFile, preferences);
return fixes.map(fix => codeActionForFix(context, sourceFile, symbolName, fix, quotePreference, program.getCompilerOptions()));
return fixes.map(fix => codeActionForFix(
context,
sourceFile,
symbolName,
fix,
/*includeSymbolNameInDescription*/ symbolName !== errorIdentifierText,
quotePreference,
program.getCompilerOptions()));
},
fixIds: [importFixId],
getAllCodeActions: context => {
Expand Down Expand Up @@ -78,7 +85,7 @@ namespace ts.codefix {
const useRequire = shouldUseRequire(sourceFile, program);
const fix = getImportFixForSymbol(sourceFile, exportInfos, moduleSymbol, symbolName, program, /*position*/ undefined, !!isValidTypeOnlyUseSite, useRequire, host, preferences);
if (fix) {
addImport({ fixes: [fix], symbolName });
addImport({ fixes: [fix], symbolName, errorIdentifierText: undefined });
}
}

Expand Down Expand Up @@ -308,6 +315,7 @@ namespace ts.codefix {
sourceFile,
symbolName,
fix,
/*includeSymbolNameInDescription*/ false,
getQuotePreference(sourceFile, preferences), compilerOptions))
};
}
Expand All @@ -316,7 +324,8 @@ namespace ts.codefix {
const compilerOptions = program.getCompilerOptions();
const symbolName = getSymbolName(sourceFile, program.getTypeChecker(), symbolToken, compilerOptions);
const fix = getTypeOnlyPromotionFix(sourceFile, symbolToken, symbolName, program);
return fix && codeFixActionToCodeAction(codeActionForFix({ host, formatContext, preferences }, sourceFile, symbolName, fix, QuotePreference.Double, compilerOptions));
const includeSymbolNameInDescription = symbolName !== symbolToken.text;
return fix && codeFixActionToCodeAction(codeActionForFix({ host, formatContext, preferences }, sourceFile, symbolName, fix, includeSymbolNameInDescription, QuotePreference.Double, compilerOptions));
}

function getImportFixForSymbol(sourceFile: SourceFile, exportInfos: readonly SymbolExportInfo[], moduleSymbol: Symbol, symbolName: string, program: Program, position: number | undefined, isValidTypeOnlyUseSite: boolean, useRequire: boolean, host: LanguageServiceHost, preferences: UserPreferences) {
Expand Down Expand Up @@ -666,7 +675,7 @@ namespace ts.codefix {
}
}

interface FixesInfo { readonly fixes: readonly ImportFix[]; readonly symbolName: string; }
interface FixesInfo { readonly fixes: readonly ImportFix[]; readonly symbolName: string; readonly errorIdentifierText: string | undefined; }
function getFixesInfo(context: CodeFixContextBase, errorCode: number, pos: number, useAutoImportProvider: boolean): FixesInfo | undefined {
const symbolToken = getTokenAtPosition(context.sourceFile, pos);
let info;
Expand All @@ -679,7 +688,7 @@ namespace ts.codefix {
else if (errorCode === Diagnostics._0_cannot_be_used_as_a_value_because_it_was_imported_using_import_type.code) {
const symbolName = getSymbolName(context.sourceFile, context.program.getTypeChecker(), symbolToken, context.program.getCompilerOptions());
const fix = getTypeOnlyPromotionFix(context.sourceFile, symbolToken, symbolName, context.program);
return fix && { fixes: [fix], symbolName };
return fix && { fixes: [fix], symbolName, errorIdentifierText: symbolToken.text };
}
else {
info = getFixesInfoForNonUMDImport(context, symbolToken, useAutoImportProvider);
Expand Down Expand Up @@ -736,7 +745,7 @@ namespace ts.codefix {
const exportInfos: readonly SymbolExportInfo[] = [{ symbol: umdSymbol, moduleSymbol: symbol, moduleFileName: undefined, exportKind: ExportKind.UMD, targetFlags: symbol.flags, isFromPackageJson: false }];
const useRequire = shouldUseRequire(sourceFile, program);
const fixes = getImportFixes(exportInfos, symbolName, isIdentifier(token) ? token.getStart(sourceFile) : undefined, /*isValidTypeOnlyUseSite*/ false, useRequire, program, sourceFile, host, preferences);
return { fixes, symbolName };
return { fixes, symbolName, errorIdentifierText: tryCast(token, isIdentifier)?.text };
}
function getUmdSymbol(token: Node, checker: TypeChecker): Symbol | undefined {
// try the identifier to see if it is the umd symbol
Expand Down Expand Up @@ -808,7 +817,7 @@ namespace ts.codefix {
const exportInfos = getExportInfos(symbolName, isJSXTagName(symbolToken), getMeaningFromLocation(symbolToken), cancellationToken, sourceFile, program, useAutoImportProvider, host, preferences);
const fixes = arrayFrom(flatMapIterator(exportInfos.entries(), ([_, exportInfos]) =>
getImportFixes(exportInfos, symbolName, symbolToken.getStart(sourceFile), isValidTypeOnlyUseSite, useRequire, program, sourceFile, host, preferences)));
return { fixes, symbolName };
return { fixes, symbolName, errorIdentifierText: symbolToken.text };
}

function getTypeOnlyPromotionFix(sourceFile: SourceFile, symbolToken: Identifier, symbolName: string, program: Program): FixPromoteTypeOnlyImport | undefined {
Expand Down Expand Up @@ -920,14 +929,14 @@ namespace ts.codefix {
return allowSyntheticDefaults ? ImportKind.Default : ImportKind.CommonJS;
}

function codeActionForFix(context: textChanges.TextChangesContext, sourceFile: SourceFile, symbolName: string, fix: ImportFix, quotePreference: QuotePreference, compilerOptions: CompilerOptions): CodeFixAction {
function codeActionForFix(context: textChanges.TextChangesContext, sourceFile: SourceFile, symbolName: string, fix: ImportFix, includeSymbolNameInDescription: boolean, quotePreference: QuotePreference, compilerOptions: CompilerOptions): CodeFixAction {
let diag!: DiagnosticAndArguments;
const changes = textChanges.ChangeTracker.with(context, tracker => {
diag = codeActionForFixWorker(tracker, sourceFile, symbolName, fix, quotePreference, compilerOptions);
diag = codeActionForFixWorker(tracker, sourceFile, symbolName, fix, includeSymbolNameInDescription, quotePreference, compilerOptions);
});
return createCodeFixAction(importFixName, changes, diag, importFixId, Diagnostics.Add_all_missing_imports);
}
function codeActionForFixWorker(changes: textChanges.ChangeTracker, sourceFile: SourceFile, symbolName: string, fix: ImportFix, quotePreference: QuotePreference, compilerOptions: CompilerOptions): DiagnosticAndArguments {
function codeActionForFixWorker(changes: textChanges.ChangeTracker, sourceFile: SourceFile, symbolName: string, fix: ImportFix, includeSymbolNameInDescription: boolean, quotePreference: QuotePreference, compilerOptions: CompilerOptions): DiagnosticAndArguments {
switch (fix.kind) {
case ImportFixKind.UseNamespace:
addNamespaceQualifier(changes, sourceFile, fix);
Expand All @@ -945,11 +954,9 @@ namespace ts.codefix {
importKind === ImportKind.Named ? [{ name: symbolName, addAsTypeOnly }] : emptyArray,
compilerOptions);
const moduleSpecifierWithoutQuotes = stripQuotes(moduleSpecifier);
return [
importKind === ImportKind.Default ? Diagnostics.Add_default_import_0_to_existing_import_declaration_from_1 : Diagnostics.Add_0_to_existing_import_declaration_from_1,
symbolName,
moduleSpecifierWithoutQuotes
]; // you too!
Copy link
Member

Choose a reason for hiding this comment

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

you too!

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 left it in for so long, never knowing what it was referencing.

return includeSymbolNameInDescription
? [Diagnostics.Import_0_from_1, symbolName, moduleSpecifierWithoutQuotes]
: [Diagnostics.Update_import_from_0, moduleSpecifierWithoutQuotes];
}
case ImportFixKind.AddNew: {
const { importKind, moduleSpecifier, addAsTypeOnly, useRequire } = fix;
Expand All @@ -958,7 +965,9 @@ namespace ts.codefix {
const namedImports: Import[] | undefined = importKind === ImportKind.Named ? [{ name: symbolName, addAsTypeOnly }] : undefined;
const namespaceLikeImport = importKind === ImportKind.Namespace || importKind === ImportKind.CommonJS ? { importKind, name: symbolName, addAsTypeOnly } : undefined;
insertImports(changes, sourceFile, getDeclarations(moduleSpecifier, quotePreference, defaultImport, namedImports, namespaceLikeImport), /*blankLineBetween*/ true);
return [importKind === ImportKind.Default ? Diagnostics.Import_default_0_from_module_1 : Diagnostics.Import_0_from_module_1, symbolName, moduleSpecifier];
return includeSymbolNameInDescription
? [Diagnostics.Import_0_from_1, symbolName, moduleSpecifier]
: [Diagnostics.Add_import_from_0, moduleSpecifier];
}
case ImportFixKind.PromoteTypeOnly: {
const { typeOnlyAliasDeclaration } = fix;
Expand Down
4 changes: 2 additions & 2 deletions src/testRunner/unittests/tsserver/completions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ namespace ts.projectSystem {
{
codeActions: [
{
description: `Import 'foo' from module "./a"`,
description: `Add import from "./a"`,
changes: [
{
fileName: "/b.ts",
Expand Down Expand Up @@ -118,7 +118,7 @@ namespace ts.projectSystem {
{
codeActions: [
{
description: `Import 'foo' from module "./a"`,
description: `Add import from "./a"`,
changes: [
{
fileName: "/b.ts",
Expand Down
2 changes: 1 addition & 1 deletion src/testRunner/unittests/tsserver/duplicatePackages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ namespace ts.projectSystem {
});
assert.deepEqual<readonly protocol.CodeFixAction[] | undefined>(response, [
{
description: `Import 'foo' from module "foo"`,
description: `Add import from "foo"`,
fixName: "import",
changes: [{
fileName: user.path,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,4 +98,4 @@ response:{"responseRequired":false}
request:{"command":"getCodeFixes","arguments":{"file":"/user/username/projects/myproject/app/src/program/index.ts","startLine":1,"startOffset":1,"endLine":1,"endOffset":4,"errorCodes":[2304]},"seq":1,"type":"request"}
DirectoryWatcher:: Added:: WatchInfo: /user/username/projects/myproject/node_modules 1 undefined WatchType: node_modules for closed script infos and package.jsons affecting module specifier cache
Elapsed:: *ms DirectoryWatcher:: Added:: WatchInfo: /user/username/projects/myproject/node_modules 1 undefined WatchType: node_modules for closed script infos and package.jsons affecting module specifier cache
response:{"response":[{"fixName":"import","description":"Import 'foo' from module \"shared\"","changes":[{"fileName":"/user/username/projects/myproject/app/src/program/index.ts","textChanges":[{"start":{"line":1,"offset":1},"end":{"line":1,"offset":1},"newText":"import { foo } from \"shared\";\n\n"}]}]}],"responseRequired":true}
response:{"response":[{"fixName":"import","description":"Add import from \"shared\"","changes":[{"fileName":"/user/username/projects/myproject/app/src/program/index.ts","textChanges":[{"start":{"line":1,"offset":1},"end":{"line":1,"offset":1},"newText":"import { foo } from \"shared\";\n\n"}]}]}],"responseRequired":true}
Original file line number Diff line number Diff line change
Expand Up @@ -97,4 +97,4 @@ response:{"responseRequired":false}
request:{"command":"getCodeFixes","arguments":{"file":"/user/username/projects/myproject/app/src/program/index.ts","startLine":1,"startOffset":1,"endLine":1,"endOffset":4,"errorCodes":[2304]},"seq":1,"type":"request"}
DirectoryWatcher:: Added:: WatchInfo: /user/username/projects/myproject/node_modules 1 undefined WatchType: node_modules for closed script infos and package.jsons affecting module specifier cache
Elapsed:: *ms DirectoryWatcher:: Added:: WatchInfo: /user/username/projects/myproject/node_modules 1 undefined WatchType: node_modules for closed script infos and package.jsons affecting module specifier cache
response:{"response":[{"fixName":"import","description":"Import 'foo' from module \"shared\"","changes":[{"fileName":"/user/username/projects/myproject/app/src/program/index.ts","textChanges":[{"start":{"line":1,"offset":1},"end":{"line":1,"offset":1},"newText":"import { foo } from \"shared\";\n\n"}]}]}],"responseRequired":true}
response:{"response":[{"fixName":"import","description":"Add import from \"shared\"","changes":[{"fileName":"/user/username/projects/myproject/app/src/program/index.ts","textChanges":[{"start":{"line":1,"offset":1},"end":{"line":1,"offset":1},"newText":"import { foo } from \"shared\";\n\n"}]}]}],"responseRequired":true}
Original file line number Diff line number Diff line change
Expand Up @@ -97,4 +97,4 @@ response:{"responseRequired":false}
request:{"command":"getCodeFixes","arguments":{"file":"/user/username/projects/myproject/app/src/program/index.ts","startLine":1,"startOffset":1,"endLine":1,"endOffset":4,"errorCodes":[2304]},"seq":1,"type":"request"}
DirectoryWatcher:: Added:: WatchInfo: /user/username/projects/myproject/node_modules 1 undefined WatchType: node_modules for closed script infos and package.jsons affecting module specifier cache
Elapsed:: *ms DirectoryWatcher:: Added:: WatchInfo: /user/username/projects/myproject/node_modules 1 undefined WatchType: node_modules for closed script infos and package.jsons affecting module specifier cache
response:{"response":[{"fixName":"import","description":"Import 'foo' from module \"shared\"","changes":[{"fileName":"/user/username/projects/myproject/app/src/program/index.ts","textChanges":[{"start":{"line":1,"offset":1},"end":{"line":1,"offset":1},"newText":"import { foo } from \"shared\";\n\n"}]}]}],"responseRequired":true}
response:{"response":[{"fixName":"import","description":"Add import from \"shared\"","changes":[{"fileName":"/user/username/projects/myproject/app/src/program/index.ts","textChanges":[{"start":{"line":1,"offset":1},"end":{"line":1,"offset":1},"newText":"import { foo } from \"shared\";\n\n"}]}]}],"responseRequired":true}
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

goTo.file("/b.ts");
verify.codeFixAvailable([
{ description: `Import 'foo' from module "./a"` },
{ description: `Add import from "./a"` },
{ description: "Change spelling to 'fooo'" },
{ description: "Add 'this.' to unresolved variable" },
]);
Expand Down
2 changes: 1 addition & 1 deletion tests/cases/fourslash/codeFixSpellingVsImport.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,6 @@

goTo.file("/b.ts");
verify.codeFixAvailable([
{ description: `Import 'foo' from module "./a"` },
{ description: `Add import from "./a"` },
{ description: "Change spelling to 'foof'" },
]);
2 changes: 1 addition & 1 deletion tests/cases/fourslash/completionsImportFromJSXTag.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
verify.applyCodeActionFromCompletion("", {
name: "Box",
source: "/Box",
description: `Import 'Box' from module "./Box"`,
description: `Add import from "./Box"`,
newFileContent: `import { Box } from "./Box";

export function App() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
verify.applyCodeActionFromCompletion("", {
name: "Abcde",
source: "/test",
description: `Import 'Abcde' from module "./test"`,
description: `Add import from "./test"`,
newFileContent: `import { Abcde } from "./test";

export {};
Expand Down
2 changes: 1 addition & 1 deletion tests/cases/fourslash/completionsImportYieldExpression.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
verify.applyCodeActionFromCompletion("", {
name: "a",
source: "/a",
description: `Import 'a' from module "./a"`,
description: `Add import from "./a"`,
newFileContent: `import { a } from "./a";

function *f() {
Expand Down
2 changes: 1 addition & 1 deletion tests/cases/fourslash/completionsImport_46332.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ verify.completions({
verify.applyCodeActionFromCompletion("", {
name: "ref",
source: "vue",
description: `Add 'ref' to existing import declaration from "vue"`,
description: `Update import from "vue"`,
data: {
exportName: "ref",
fileName: "/node_modules/vue/dist/vue.d.ts",
Expand Down
2 changes: 1 addition & 1 deletion tests/cases/fourslash/completionsImport_ambient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ verify.completions({
verify.applyCodeActionFromCompletion("", {
name: "Bar",
source: "path2longer",
description: `Import 'Bar' from module "path2longer"`,
description: `Add import from "path2longer"`,
newFileContent: `import { Bar } from "path2longer";\n\nBa`,
preferences: {
includeCompletionsForModuleExports: true
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ verify.applyCodeActionFromCompletion("", {
name: "someModule",
source: "/someModule",
data: { exportName: "default", fileName: "/someModule.ts" },
description: `Import default 'someModule' from module "./someModule"`,
description: `Add import from "./someModule"`,
newFileContent: `import someModule from "./someModule";

someMo`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ verify.completions({
verify.applyCodeActionFromCompletion("", {
name: "concat",
source: "/node_modules/bar/concat",
description: `Import 'concat' from module "bar/concat"`,
description: `Add import from "bar/concat"`,
newFileContent:
`import { concat } from "bar/concat";

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ verify.completions({
verify.applyCodeActionFromCompletion("", {
name: "foo",
source: "/a",
description: `Add default import 'foo' to existing import declaration from "./a"`,
description: `Update import from "./a"`,
newFileContent: `import foo, { x } from "./a";
f;`,
});
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ verify.completions({
verify.applyCodeActionFromCompletion("", {
name: "foo",
source: "/a",
description: `Add default import 'foo' to existing import declaration from "./a"`,
description: `Update import from "./a"`,
newFileContent: `import foo, * as a from "./a";
f;`,
});
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ verify.completions({
verify.applyCodeActionFromCompletion("", {
name: "foo",
source: "/a",
description: `Import default 'foo' from module "./a"`,
description: `Add import from "./a"`,
newFileContent: `import foo from "./a";
import f_o_o from "./a";
f;`,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ verify.completions(
verify.applyCodeActionFromCompletion("1", {
name: "fooBar",
source: "/src/foo-bar",
description: `Import default 'fooBar' from module "./foo-bar"`,
description: `Add import from "./foo-bar"`,
newFileContent: `import fooBar from "./foo-bar"

def
Expand Down
Loading