Skip to content

Commit 7800978

Browse files
iisaduaniisaduan
authored
Organize type imports (microsoft#55269)
1 parent 2c14a1c commit 7800978

25 files changed

+838
-53
lines changed

‎src/compiler/types.ts

+1
Original file line numberDiff line numberDiff line change
@@ -10074,6 +10074,7 @@ export interface UserPreferences {
1007410074
readonly organizeImportsNumericCollation?: boolean;
1007510075
readonly organizeImportsAccentCollation?: boolean;
1007610076
readonly organizeImportsCaseFirst?: "upper" | "lower" | false;
10077+
readonly organizeImportsTypeOrder?: "first" | "last" | "inline";
1007710078
readonly excludeLibrarySymbolsInNavTo?: boolean;
1007810079
}
1007910080

‎src/harness/fourslashImpl.ts

+6-4
Original file line numberDiff line numberDiff line change
@@ -3521,10 +3521,12 @@ export class TestState {
35213521
actualTextArray.push(text);
35223522

35233523
// Undo changes to perform next fix
3524-
const span = change.textChanges[0].span;
3525-
const deletedText = originalContent.substr(span.start, change.textChanges[0].span.length);
3526-
const insertedText = change.textChanges[0].newText;
3527-
this.editScriptAndUpdateMarkers(fileName, span.start, span.start + insertedText.length, deletedText);
3524+
for (const textChange of change.textChanges) {
3525+
const span = textChange.span;
3526+
const deletedText = originalContent.slice(span.start, span.start + textChange.span.length);
3527+
const insertedText = textChange.newText;
3528+
this.editScriptAndUpdateMarkers(fileName, span.start, span.start + insertedText.length, deletedText);
3529+
}
35283530
}
35293531
if (expectedTextArray.length !== actualTextArray.length) {
35303532
this.raiseError(`Expected ${expectedTextArray.length} import fixes, got ${actualTextArray.length}:\n\n${actualTextArray.join("\n\n" + "-".repeat(20) + "\n\n")}`);

‎src/server/protocol.ts

+7
Original file line numberDiff line numberDiff line change
@@ -3643,6 +3643,13 @@ export interface UserPreferences {
36433643
* Default: `false`
36443644
*/
36453645
readonly organizeImportsCaseFirst?: "upper" | "lower" | false;
3646+
/**
3647+
* Indicates where named type-only imports should sort. "inline" sorts named imports without regard to if the import is
3648+
* type-only.
3649+
*
3650+
* Default: `last`
3651+
*/
3652+
readonly organizeImportsTypeOrder?: "last" | "first" | "inline";
36463653

36473654
/**
36483655
* Indicates whether {@link ReferencesResponseItem.lineText} is supported.

‎src/services/codefixes/importFixes.ts

+5-4
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ import {
5353
getSourceFileOfNode,
5454
getSymbolId,
5555
getTokenAtPosition,
56+
getTokenPosOfNode,
5657
getTypeKeywordOfTypeOnlyImport,
5758
getUniqueSymbolId,
5859
hostGetCanonicalFileName,
@@ -1406,14 +1407,14 @@ function promoteFromTypeOnly(
14061407
if (aliasDeclaration.parent.elements.length > 1 && sortKind) {
14071408
const newSpecifier = factory.updateImportSpecifier(aliasDeclaration, /*isTypeOnly*/ false, aliasDeclaration.propertyName, aliasDeclaration.name);
14081409
const comparer = OrganizeImports.getOrganizeImportsComparer(preferences, sortKind === SortKind.CaseInsensitive);
1409-
const insertionIndex = OrganizeImports.getImportSpecifierInsertionIndex(aliasDeclaration.parent.elements, newSpecifier, comparer);
1410-
if (aliasDeclaration.parent.elements.indexOf(aliasDeclaration) !== insertionIndex) {
1410+
const insertionIndex = OrganizeImports.getImportSpecifierInsertionIndex(aliasDeclaration.parent.elements, newSpecifier, comparer, preferences);
1411+
if (insertionIndex !== aliasDeclaration.parent.elements.indexOf(aliasDeclaration)) {
14111412
changes.delete(sourceFile, aliasDeclaration);
14121413
changes.insertImportSpecifierAtIndex(sourceFile, newSpecifier, aliasDeclaration.parent, insertionIndex);
14131414
return aliasDeclaration;
14141415
}
14151416
}
1416-
changes.deleteRange(sourceFile, aliasDeclaration.getFirstToken()!);
1417+
changes.deleteRange(sourceFile, { pos: getTokenPosOfNode(aliasDeclaration.getFirstToken()!), end: getTokenPosOfNode(aliasDeclaration.propertyName ?? aliasDeclaration.name) });
14171418
return aliasDeclaration;
14181419
}
14191420
else {
@@ -1538,7 +1539,7 @@ function doAddExistingFix(
15381539
// type-only, there's no need to ask for the insertion index - it's 0.
15391540
const insertionIndex = promoteFromTypeOnly && !spec.isTypeOnly
15401541
? 0
1541-
: OrganizeImports.getImportSpecifierInsertionIndex(existingSpecifiers, spec, comparer);
1542+
: OrganizeImports.getImportSpecifierInsertionIndex(existingSpecifiers, spec, comparer, preferences);
15421543
changes.insertImportSpecifierAtIndex(sourceFile, spec, clause.namedBindings as NamedImports, insertionIndex);
15431544
}
15441545
}

‎src/services/organizeImports.ts

+68-20
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,10 @@ import {
2323
flatMap,
2424
formatting,
2525
getNewLineOrDefaultFromHost,
26+
getStringComparer,
2627
getUILocale,
2728
group,
29+
groupBy,
2830
Identifier,
2931
identity,
3032
ImportClause,
@@ -94,7 +96,7 @@ export function organizeImports(
9496

9597
const processImportsOfSameModuleSpecifier = (importGroup: readonly ImportDeclaration[]) => {
9698
if (shouldRemove) importGroup = removeUnusedImports(importGroup, sourceFile, program);
97-
if (shouldCombine) importGroup = coalesceImportsWorker(importGroup, comparer, sourceFile);
99+
if (shouldCombine) importGroup = coalesceImportsWorker(importGroup, comparer, sourceFile, preferences);
98100
if (shouldSort) importGroup = stableSort(importGroup, (s1, s2) => compareImportsOrRequireStatements(s1, s2, comparer));
99101
return importGroup;
100102
};
@@ -104,7 +106,7 @@ export function organizeImports(
104106
// Exports are always used
105107
if (mode !== OrganizeImportsMode.RemoveUnused) {
106108
// All of the old ExportDeclarations in the file, in syntactic order.
107-
getTopLevelExportGroups(sourceFile).forEach(exportGroupDecl => organizeImportsWorker(exportGroupDecl, group => coalesceExportsWorker(group, comparer)));
109+
getTopLevelExportGroups(sourceFile).forEach(exportGroupDecl => organizeImportsWorker(exportGroupDecl, group => coalesceExportsWorker(group, comparer, preferences)));
108110
}
109111

110112
for (const ambientModule of sourceFile.statements.filter(isAmbientModule)) {
@@ -116,7 +118,7 @@ export function organizeImports(
116118
// Exports are always used
117119
if (mode !== OrganizeImportsMode.RemoveUnused) {
118120
const ambientModuleExportDecls = ambientModule.body.statements.filter(isExportDeclaration);
119-
organizeImportsWorker(ambientModuleExportDecls, group => coalesceExportsWorker(group, comparer));
121+
organizeImportsWorker(ambientModuleExportDecls, group => coalesceExportsWorker(group, comparer, preferences));
120122
}
121123
}
122124

@@ -310,12 +312,12 @@ function getExternalModuleName(specifier: Expression | undefined) {
310312
* @deprecated Only used for testing
311313
* @internal
312314
*/
313-
export function coalesceImports(importGroup: readonly ImportDeclaration[], ignoreCase: boolean, sourceFile?: SourceFile): readonly ImportDeclaration[] {
315+
export function coalesceImports(importGroup: readonly ImportDeclaration[], ignoreCase: boolean, sourceFile?: SourceFile, preferences?: UserPreferences): readonly ImportDeclaration[] {
314316
const comparer = getOrganizeImportsOrdinalStringComparer(ignoreCase);
315-
return coalesceImportsWorker(importGroup, comparer, sourceFile);
317+
return coalesceImportsWorker(importGroup, comparer, sourceFile, preferences);
316318
}
317319

318-
function coalesceImportsWorker(importGroup: readonly ImportDeclaration[], comparer: Comparer<string>, sourceFile?: SourceFile): readonly ImportDeclaration[] {
320+
function coalesceImportsWorker(importGroup: readonly ImportDeclaration[], comparer: Comparer<string>, sourceFile?: SourceFile, preferences?: UserPreferences): readonly ImportDeclaration[] {
319321
if (importGroup.length === 0) {
320322
return importGroup;
321323
}
@@ -374,7 +376,7 @@ function coalesceImportsWorker(importGroup: readonly ImportDeclaration[], compar
374376
newImportSpecifiers.push(...getNewImportSpecifiers(namedImports));
375377

376378
const sortedImportSpecifiers = factory.createNodeArray(
377-
sortSpecifiers(newImportSpecifiers, comparer),
379+
sortSpecifiers(newImportSpecifiers, comparer, preferences),
378380
firstNamedImport?.importClause.namedBindings.elements.hasTrailingComma,
379381
);
380382

@@ -491,18 +493,17 @@ function getCategorizedImports(importGroup: readonly ImportDeclaration[]) {
491493
* @deprecated Only used for testing
492494
* @internal
493495
*/
494-
export function coalesceExports(exportGroup: readonly ExportDeclaration[], ignoreCase: boolean) {
496+
export function coalesceExports(exportGroup: readonly ExportDeclaration[], ignoreCase: boolean, preferences?: UserPreferences) {
495497
const comparer = getOrganizeImportsOrdinalStringComparer(ignoreCase);
496-
return coalesceExportsWorker(exportGroup, comparer);
498+
return coalesceExportsWorker(exportGroup, comparer, preferences);
497499
}
498500

499-
function coalesceExportsWorker(exportGroup: readonly ExportDeclaration[], comparer: Comparer<string>) {
501+
function coalesceExportsWorker(exportGroup: readonly ExportDeclaration[], comparer: Comparer<string>, preferences?: UserPreferences) {
500502
if (exportGroup.length === 0) {
501503
return exportGroup;
502504
}
503505

504506
const { exportWithoutClause, namedExports, typeOnlyExports } = getCategorizedExports(exportGroup);
505-
506507
const coalescedExports: ExportDeclaration[] = [];
507508

508509
if (exportWithoutClause) {
@@ -516,7 +517,7 @@ function coalesceExportsWorker(exportGroup: readonly ExportDeclaration[], compar
516517
const newExportSpecifiers: ExportSpecifier[] = [];
517518
newExportSpecifiers.push(...flatMap(exportGroup, i => i.exportClause && isNamedExports(i.exportClause) ? i.exportClause.elements : emptyArray));
518519

519-
const sortedExportSpecifiers = sortSpecifiers(newExportSpecifiers, comparer);
520+
const sortedExportSpecifiers = sortSpecifiers(newExportSpecifiers, comparer, preferences);
520521

521522
const exportDecl = exportGroup[0];
522523
coalescedExports.push(
@@ -583,13 +584,20 @@ function updateImportDeclarationAndClause(
583584
);
584585
}
585586

586-
function sortSpecifiers<T extends ImportOrExportSpecifier>(specifiers: readonly T[], comparer: Comparer<string>) {
587-
return stableSort(specifiers, (s1, s2) => compareImportOrExportSpecifiers(s1, s2, comparer));
587+
function sortSpecifiers<T extends ImportOrExportSpecifier>(specifiers: readonly T[], comparer: Comparer<string>, preferences?: UserPreferences): readonly T[] {
588+
return stableSort(specifiers, (s1, s2) => compareImportOrExportSpecifiers(s1, s2, comparer, preferences));
588589
}
589590

590591
/** @internal */
591-
export function compareImportOrExportSpecifiers<T extends ImportOrExportSpecifier>(s1: T, s2: T, comparer: Comparer<string>): Comparison {
592-
return compareBooleans(s1.isTypeOnly, s2.isTypeOnly) || comparer(s1.name.text, s2.name.text);
592+
export function compareImportOrExportSpecifiers<T extends ImportOrExportSpecifier>(s1: T, s2: T, comparer: Comparer<string>, preferences?: UserPreferences): Comparison {
593+
switch (preferences?.organizeImportsTypeOrder) {
594+
case "first":
595+
return compareBooleans(s2.isTypeOnly, s1.isTypeOnly) || comparer(s1.name.text, s2.name.text);
596+
case "inline":
597+
return comparer(s1.name.text, s2.name.text);
598+
default:
599+
return compareBooleans(s1.isTypeOnly, s2.isTypeOnly) || comparer(s1.name.text, s2.name.text);
600+
}
593601
}
594602

595603
/**
@@ -721,11 +729,51 @@ class ImportSpecifierSortingCache implements MemoizeCache<[readonly ImportSpecif
721729

722730
/** @internal */
723731
export const detectImportSpecifierSorting = memoizeCached((specifiers: readonly ImportSpecifier[], preferences: UserPreferences): SortKind => {
724-
if (!arrayIsSorted(specifiers, (s1, s2) => compareBooleans(s1.isTypeOnly, s2.isTypeOnly))) {
725-
return SortKind.None;
732+
// If types are not sorted as specified, then imports are assumed to be unsorted.
733+
// If there is no type sorting specification, we default to "last" and move on to case sensitivity detection.
734+
switch (preferences.organizeImportsTypeOrder) {
735+
case "first":
736+
if (!arrayIsSorted(specifiers, (s1, s2) => compareBooleans(s2.isTypeOnly, s1.isTypeOnly))) return SortKind.None;
737+
break;
738+
case "inline":
739+
if (
740+
!arrayIsSorted(specifiers, (s1, s2) => {
741+
const comparer = getStringComparer(/*ignoreCase*/ true);
742+
return comparer(s1.name.text, s2.name.text);
743+
})
744+
) {
745+
return SortKind.None;
746+
}
747+
break;
748+
default:
749+
if (!arrayIsSorted(specifiers, (s1, s2) => compareBooleans(s1.isTypeOnly, s2.isTypeOnly))) return SortKind.None;
750+
break;
726751
}
752+
727753
const collateCaseSensitive = getOrganizeImportsComparer(preferences, /*ignoreCase*/ false);
728754
const collateCaseInsensitive = getOrganizeImportsComparer(preferences, /*ignoreCase*/ true);
755+
756+
if (preferences.organizeImportsTypeOrder !== "inline") {
757+
const { type: regularImports, regular: typeImports } = groupBy(specifiers, s => s.isTypeOnly ? "type" : "regular");
758+
const regularCaseSensitivity = regularImports?.length
759+
? detectSortCaseSensitivity(regularImports, specifier => specifier.name.text, collateCaseSensitive, collateCaseInsensitive)
760+
: undefined;
761+
const typeCaseSensitivity = typeImports?.length
762+
? detectSortCaseSensitivity(typeImports, specifier => specifier.name.text ?? "", collateCaseSensitive, collateCaseInsensitive)
763+
: undefined;
764+
if (regularCaseSensitivity === undefined) {
765+
return typeCaseSensitivity ?? SortKind.None;
766+
}
767+
if (typeCaseSensitivity === undefined) {
768+
return regularCaseSensitivity;
769+
}
770+
if (regularCaseSensitivity === SortKind.None || typeCaseSensitivity === SortKind.None) {
771+
return SortKind.None;
772+
}
773+
return typeCaseSensitivity & regularCaseSensitivity;
774+
}
775+
776+
// else inline
729777
return detectSortCaseSensitivity(specifiers, specifier => specifier.name.text, collateCaseSensitive, collateCaseInsensitive);
730778
}, new ImportSpecifierSortingCache());
731779

@@ -736,8 +784,8 @@ export function getImportDeclarationInsertionIndex(sortedImports: readonly AnyIm
736784
}
737785

738786
/** @internal */
739-
export function getImportSpecifierInsertionIndex(sortedImports: readonly ImportSpecifier[], newImport: ImportSpecifier, comparer: Comparer<string>) {
740-
const index = binarySearch(sortedImports, newImport, identity, (s1, s2) => compareImportOrExportSpecifiers(s1, s2, comparer));
787+
export function getImportSpecifierInsertionIndex(sortedImports: readonly ImportSpecifier[], newImport: ImportSpecifier, comparer: Comparer<string>, preferences: UserPreferences) {
788+
const index = binarySearch(sortedImports, newImport, identity, (s1, s2) => compareImportOrExportSpecifiers(s1, s2, comparer, preferences));
741789
return index < 0 ? ~index : index;
742790
}
743791

‎src/testRunner/unittests/services/organizeImports.ts

+3-3
Original file line numberDiff line numberDiff line change
@@ -239,10 +239,10 @@ describe("unittests:: services:: organizeImports", () => {
239239
assertListEqual(actualCoalescedExports, expectedCoalescedExports);
240240
});
241241

242-
it("Sort specifiers - type-only", () => {
242+
it("Sort specifiers - type-only-inline", () => {
243243
const sortedImports = parseImports(`import { type z, y, type x, c, type b, a } from "lib";`);
244-
const actualCoalescedImports = ts.OrganizeImports.coalesceImports(sortedImports, /*ignoreCase*/ true);
245-
const expectedCoalescedImports = parseImports(`import { a, c, y, type b, type x, type z } from "lib";`);
244+
const actualCoalescedImports = ts.OrganizeImports.coalesceImports(sortedImports, /*ignoreCase*/ true, ts.getSourceFileOfNode(sortedImports[0]), { organizeImportsTypeOrder: "inline" });
245+
const expectedCoalescedImports = parseImports(`import { a, type b, c, type x, y, type z } from "lib";`);
246246
assertListEqual(actualCoalescedImports, expectedCoalescedImports);
247247
});
248248

‎tests/baselines/reference/api/typescript.d.ts

+8
Original file line numberDiff line numberDiff line change
@@ -2933,6 +2933,13 @@ declare namespace ts {
29332933
* Default: `false`
29342934
*/
29352935
readonly organizeImportsCaseFirst?: "upper" | "lower" | false;
2936+
/**
2937+
* Indicates where named type-only imports should sort. "inline" sorts named imports without regard to if the import is
2938+
* type-only.
2939+
*
2940+
* Default: `last`
2941+
*/
2942+
readonly organizeImportsTypeOrder?: "last" | "first" | "inline";
29362943
/**
29372944
* Indicates whether {@link ReferencesResponseItem.lineText} is supported.
29382945
*/
@@ -8784,6 +8791,7 @@ declare namespace ts {
87848791
readonly organizeImportsNumericCollation?: boolean;
87858792
readonly organizeImportsAccentCollation?: boolean;
87868793
readonly organizeImportsCaseFirst?: "upper" | "lower" | false;
8794+
readonly organizeImportsTypeOrder?: "first" | "last" | "inline";
87878795
readonly excludeLibrarySymbolsInNavTo?: boolean;
87888796
}
87898797
/** Represents a bigint literal value without requiring bigint support */

‎tests/cases/fourslash/autoImportTypeImport1.ts

+46
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
/// <reference path="fourslash.ts" />
2+
3+
// @verbatimModuleSyntax: true
4+
// @target: esnext
5+
6+
// @Filename: /foo.ts
7+
//// export const A = 1;
8+
//// export type B = { x: number };
9+
//// export type C = 1;
10+
//// export class D = { y: string };
11+
12+
// @Filename: /test.ts
13+
//// import { A, D, type C } from './foo';
14+
//// const b: B/**/ | C;
15+
//// console.log(A, D);
16+
17+
goTo.marker("");
18+
19+
// importFixes should only place the import in sorted position if the existing imports are sorted as specified,
20+
// otherwise the import should be placed at the end
21+
verify.importFixAtPosition([
22+
`import { A, D, type C, type B } from './foo';
23+
const b: B | C;
24+
console.log(A, D);`],
25+
/*errorCode*/ undefined,
26+
{ organizeImportsTypeOrder: "inline" }
27+
// `type B` is added to the end since the existing imports are not sorted as specified
28+
);
29+
30+
verify.importFixAtPosition([
31+
`import { A, D, type B, type C } from './foo';
32+
const b: B | C;
33+
console.log(A, D);`],
34+
/*errorCode*/ undefined,
35+
{ organizeImportsTypeOrder: "last" }
36+
// `type B` is added to the sorted position since the existing imports *are* sorted as specified
37+
);
38+
39+
verify.importFixAtPosition([
40+
`import { A, D, type C, type B } from './foo';
41+
const b: B | C;
42+
console.log(A, D);`],
43+
/*errorCode*/ undefined,
44+
{ organizeImportsTypeOrder: "first" }
45+
// `type B` is added to the end (default behavior) since the existing imports are not sorted as specified
46+
);

‎tests/cases/fourslash/autoImportTypeImport2.ts

+43
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
/// <reference path="fourslash.ts" />
2+
3+
// @verbatimModuleSyntax: true
4+
// @target: esnext
5+
6+
// @Filename: /foo.ts
7+
//// export const A = 1;
8+
//// export type B = { x: number };
9+
//// export type C = 1;
10+
//// export class D = { y: string };
11+
12+
// @Filename: /test.ts
13+
//// import { A, type C, D } from './foo';
14+
//// const b: B/**/ | C;
15+
//// console.log(A, D);
16+
17+
goTo.marker("");
18+
19+
// importFixes should only place the import in sorted position if the existing imports are sorted as specified,
20+
// otherwise the import should be placed at the end
21+
verify.importFixAtPosition([
22+
`import { A, type B, type C, D } from './foo';
23+
const b: B | C;
24+
console.log(A, D);`],
25+
/*errorCode*/ undefined,
26+
{ organizeImportsTypeOrder: "inline" }
27+
);
28+
29+
verify.importFixAtPosition([
30+
`import { A, type C, D, type B } from './foo';
31+
const b: B | C;
32+
console.log(A, D);`],
33+
/*errorCode*/ undefined,
34+
{ organizeImportsTypeOrder: "last" }
35+
);
36+
37+
verify.importFixAtPosition([
38+
`import { A, type C, D, type B } from './foo';
39+
const b: B | C;
40+
console.log(A, D);`],
41+
/*errorCode*/ undefined,
42+
{ organizeImportsTypeOrder: "first" }
43+
);

0 commit comments

Comments
 (0)