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

Insert auto-imports in sorted order #39394

Merged
merged 3 commits into from
Jul 8, 2020
Merged

Insert auto-imports in sorted order #39394

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
14e20e6
Sort auto-imports
andrewbranch Jul 2, 2020
c92b071
Avoid re-checking sort all the time
andrewbranch Jul 2, 2020
7b9407e
Add comment
andrewbranch Jul 8, 2020
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
12 changes: 12 additions & 0 deletions src/compiler/core.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -819,6 +819,18 @@ namespace ts {
return deduplicateSorted(sort(array, comparer), equalityComparer || comparer || compareStringsCaseSensitive as any as Comparer<T>);
}

export function arrayIsSorted<T>(array: readonly T[], comparer: Comparer<T>) {
if (array.length < 2) return true;
let prevElement = array[0];
for (const element of array.slice(1)) {
if (comparer(prevElement, element) === Comparison.GreaterThan) {
return false;
}
prevElement = element;
}
return true;
}

export function arrayIsEqualTo<T>(array1: readonly T[] | undefined, array2: readonly T[] | undefined, equalityComparer: (a: T, b: T, index: number) => boolean = equateValues): boolean {
if (!array1 || !array2) {
return array1 === array2;
Expand Down
14 changes: 13 additions & 1 deletion src/compiler/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4336,6 +4336,9 @@ namespace ts {
/* @internal */
export type AnyImportOrRequire = AnyImportSyntax | RequireVariableDeclaration;

/* @internal */
export type AnyImportOrRequireStatement = AnyImportSyntax | RequireVariableStatement;


/* @internal */
export type AnyImportOrReExport = AnyImportSyntax | ExportDeclaration;
Expand All @@ -4357,8 +4360,17 @@ namespace ts {

/* @internal */
export interface RequireVariableDeclaration extends VariableDeclaration {
readonly initializer: RequireOrImportCall;
}

initializer: RequireOrImportCall;
/* @internal */
export interface RequireVariableStatement extends VariableStatement {
readonly declarationList: RequireVariableDeclarationList;
}

/* @internal */
export interface RequireVariableDeclarationList extends VariableDeclarationList {
readonly declarations: NodeArray<RequireVariableDeclaration>;
}

/* @internal */
Expand Down
6 changes: 4 additions & 2 deletions src/compiler/utilities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1934,8 +1934,10 @@ namespace ts {
return isVariableDeclaration(node) && !!node.initializer && isRequireCall(node.initializer, requireStringLiteralLikeArgument);
}

export function isRequireVariableDeclarationStatement(node: Node, requireStringLiteralLikeArgument = true): node is VariableStatement {
return isVariableStatement(node) && every(node.declarationList.declarations, decl => isRequireVariableDeclaration(decl, requireStringLiteralLikeArgument));
export function isRequireVariableStatement(node: Node, requireStringLiteralLikeArgument = true): node is RequireVariableStatement {
return isVariableStatement(node)
&& node.declarationList.declarations.length > 0
&& every(node.declarationList.declarations, decl => isRequireVariableDeclaration(decl, requireStringLiteralLikeArgument));
}

export function isSingleOrDoubleQuote(charCode: number) {
Expand Down
46 changes: 33 additions & 13 deletions src/services/codefixes/importFixes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ namespace ts.codefix {
doAddExistingFix(changeTracker, sourceFile, importClauseOrBindingPattern, defaultImport, namedImports, canUseTypeOnlyImport);
});

let newDeclarations: Statement | readonly Statement[] | undefined;
let newDeclarations: AnyImportOrRequireStatement | readonly AnyImportOrRequireStatement[] | undefined;
newImports.forEach(({ useRequire, ...imports }, moduleSpecifier) => {
const getDeclarations = useRequire ? getNewRequires : getNewImports;
newDeclarations = combine(newDeclarations, getDeclarations(moduleSpecifier, quotePreference, imports));
Expand Down Expand Up @@ -671,15 +671,35 @@ namespace ts.codefix {
}

if (namedImports.length) {
const specifiers = namedImports.map(name => factory.createImportSpecifier(/*propertyName*/ undefined, factory.createIdentifier(name)));
if (clause.namedBindings && cast(clause.namedBindings, isNamedImports).elements.length) {
for (const spec of specifiers) {
changes.insertNodeInListAfter(sourceFile, last(cast(clause.namedBindings, isNamedImports).elements), spec);
const existingSpecifiers = clause.namedBindings && cast(clause.namedBindings, isNamedImports).elements;
const newSpecifiers = stableSort(
namedImports.map(name => factory.createImportSpecifier(/*propertyName*/ undefined, factory.createIdentifier(name))),
OrganizeImports.compareImportOrExportSpecifiers);

if (existingSpecifiers?.length && OrganizeImports.importSpecifiersAreSorted(existingSpecifiers)) {
for (const spec of newSpecifiers) {
const insertionIndex = OrganizeImports.getImportSpecifierInsertionIndex(existingSpecifiers, spec);
const prevSpecifier = (clause.namedBindings as NamedImports).elements[insertionIndex - 1];
if (prevSpecifier) {
changes.insertNodeInListAfter(sourceFile, prevSpecifier, spec);
}
else {
changes.insertNodeBefore(
sourceFile,
existingSpecifiers[0],
spec,
!positionsAreOnSameLine(existingSpecifiers[0].getStart(), clause.parent.getStart(), sourceFile));
}
}
}
else if (existingSpecifiers?.length) {
for (const spec of newSpecifiers) {
changes.insertNodeAtEndOfList(sourceFile, existingSpecifiers, spec);
}
}
else {
if (specifiers.length) {
const namedImports = factory.createNamedImports(specifiers);
if (newSpecifiers.length) {
const namedImports = factory.createNamedImports(newSpecifiers);
if (clause.namedBindings) {
changes.replaceNode(sourceFile, clause.namedBindings, namedImports);
}
Expand Down Expand Up @@ -727,9 +747,9 @@ namespace ts.codefix {
readonly name: string;
};
}
function getNewImports(moduleSpecifier: string, quotePreference: QuotePreference, imports: ImportsCollection): Statement | readonly Statement[] {
function getNewImports(moduleSpecifier: string, quotePreference: QuotePreference, imports: ImportsCollection): AnyImportSyntax | readonly AnyImportSyntax[] {
const quotedModuleSpecifier = makeStringLiteral(moduleSpecifier, quotePreference);
let statements: Statement | readonly Statement[] | undefined;
let statements: AnyImportSyntax | readonly AnyImportSyntax[] | undefined;
if (imports.defaultImport !== undefined || imports.namedImports?.length) {
statements = combine(statements, makeImport(
imports.defaultImport === undefined ? undefined : factory.createIdentifier(imports.defaultImport),
Expand All @@ -756,9 +776,9 @@ namespace ts.codefix {
return Debug.checkDefined(statements);
}

function getNewRequires(moduleSpecifier: string, quotePreference: QuotePreference, imports: ImportsCollection): Statement | readonly Statement[] {
function getNewRequires(moduleSpecifier: string, quotePreference: QuotePreference, imports: ImportsCollection): RequireVariableStatement | readonly RequireVariableStatement[] {
const quotedModuleSpecifier = makeStringLiteral(moduleSpecifier, quotePreference);
let statements: Statement | readonly Statement[] | undefined;
let statements: RequireVariableStatement | readonly RequireVariableStatement[] | undefined;
// const { default: foo, bar, etc } = require('./mod');
if (imports.defaultImport || imports.namedImports?.length) {
const bindingElements = imports.namedImports?.map(name => factory.createBindingElement(/*dotDotDotToken*/ undefined, /*propertyName*/ undefined, name)) || [];
Expand All @@ -776,7 +796,7 @@ namespace ts.codefix {
return Debug.checkDefined(statements);
}

function createConstEqualsRequireDeclaration(name: string | ObjectBindingPattern, quotedModuleSpecifier: StringLiteral): VariableStatement {
function createConstEqualsRequireDeclaration(name: string | ObjectBindingPattern, quotedModuleSpecifier: StringLiteral): RequireVariableStatement {
return factory.createVariableStatement(
/*modifiers*/ undefined,
factory.createVariableDeclarationList([
Expand All @@ -785,7 +805,7 @@ namespace ts.codefix {
/*exclamationToken*/ undefined,
/*type*/ undefined,
factory.createCallExpression(factory.createIdentifier("require"), /*typeArguments*/ undefined, [quotedModuleSpecifier]))],
NodeFlags.Const));
NodeFlags.Const)) as RequireVariableStatement;
}

function symbolHasMeaning({ declarations }: Symbol, meaning: SemanticMeaning): boolean {
Expand Down
80 changes: 72 additions & 8 deletions src/services/organizeImports.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@ namespace ts.OrganizeImports {

const changeTracker = textChanges.ChangeTracker.fromContext({ host, formatContext, preferences });

const coalesceAndOrganizeImports = (importGroup: readonly ImportDeclaration[]) => coalesceImports(removeUnusedImports(importGroup, sourceFile, program));
const coalesceAndOrganizeImports = (importGroup: readonly ImportDeclaration[]) => stableSort(
coalesceImports(removeUnusedImports(importGroup, sourceFile, program)),
(s1, s2) => compareImportsOrRequireStatements(s1, s2));

// All of the old ImportDeclarations in the file, in syntactic order.
const topLevelImportDecls = sourceFile.statements.filter(isImportDeclaration);
Expand Down Expand Up @@ -55,7 +57,7 @@ namespace ts.OrganizeImports {
suppressLeadingTrivia(oldImportDecls[0]);

const oldImportGroups = group(oldImportDecls, importDecl => getExternalModuleName(importDecl.moduleSpecifier!)!);
const sortedImportGroups = stableSort(oldImportGroups, (group1, group2) => compareModuleSpecifiers(group1[0].moduleSpecifier!, group2[0].moduleSpecifier!));
const sortedImportGroups = stableSort(oldImportGroups, (group1, group2) => compareModuleSpecifiers(group1[0].moduleSpecifier, group2[0].moduleSpecifier));
const newImportDecls = flatMap(sortedImportGroups, importGroup =>
getExternalModuleName(importGroup[0].moduleSpecifier!)
? coalesce(importGroup)
Expand Down Expand Up @@ -395,15 +397,18 @@ namespace ts.OrganizeImports {
}

function sortSpecifiers<T extends ImportOrExportSpecifier>(specifiers: readonly T[]) {
return stableSort(specifiers, (s1, s2) =>
compareIdentifiers(s1.propertyName || s1.name, s2.propertyName || s2.name) ||
compareIdentifiers(s1.name, s2.name));
return stableSort(specifiers, compareImportOrExportSpecifiers);
}

export function compareImportOrExportSpecifiers<T extends ImportOrExportSpecifier>(s1: T, s2: T) {
return compareIdentifiers(s1.propertyName || s1.name, s2.propertyName || s2.name)
|| compareIdentifiers(s1.name, s2.name);
}

/* internal */ // Exported for testing
export function compareModuleSpecifiers(m1: Expression, m2: Expression) {
const name1 = getExternalModuleName(m1);
const name2 = getExternalModuleName(m2);
export function compareModuleSpecifiers(m1: Expression | undefined, m2: Expression | undefined) {
const name1 = m1 === undefined ? undefined : getExternalModuleName(m1);
const name2 = m2 === undefined ? undefined : getExternalModuleName(m2);
return compareBooleans(name1 === undefined, name2 === undefined) ||
compareBooleans(isExternalModuleNameRelative(name1!), isExternalModuleNameRelative(name2!)) ||
compareStringsCaseInsensitive(name1!, name2!);
Expand All @@ -412,4 +417,63 @@ namespace ts.OrganizeImports {
function compareIdentifiers(s1: Identifier, s2: Identifier) {
return compareStringsCaseInsensitive(s1.text, s2.text);
}

function getModuleSpecifierExpression(declaration: AnyImportOrRequireStatement): Expression | undefined {
switch (declaration.kind) {
case SyntaxKind.ImportEqualsDeclaration:
return tryCast(declaration.moduleReference, isExternalModuleReference)?.expression;
case SyntaxKind.ImportDeclaration:
return declaration.moduleSpecifier;
case SyntaxKind.VariableStatement:
return declaration.declarationList.declarations[0].initializer.arguments[0];
}
}

export function importsAreSorted(imports: readonly AnyImportOrRequireStatement[]): imports is SortedReadonlyArray<AnyImportOrRequireStatement> {
return arrayIsSorted(imports, compareImportsOrRequireStatements);
}

export function importSpecifiersAreSorted(imports: readonly ImportSpecifier[]): imports is SortedReadonlyArray<ImportSpecifier> {
return arrayIsSorted(imports, compareImportOrExportSpecifiers);
}

export function getImportDeclarationInsertionIndex(sortedImports: SortedReadonlyArray<AnyImportOrRequireStatement>, newImport: AnyImportOrRequireStatement) {
const index = binarySearch(sortedImports, newImport, identity, compareImportsOrRequireStatements);
return index < 0 ? ~index : index;
}

export function getImportSpecifierInsertionIndex(sortedImports: SortedReadonlyArray<ImportSpecifier>, newImport: ImportSpecifier) {
const index = binarySearch(sortedImports, newImport, identity, compareImportOrExportSpecifiers);
return index < 0 ? ~index : index;
}

export function compareImportsOrRequireStatements(s1: AnyImportOrRequireStatement, s2: AnyImportOrRequireStatement) {
return compareModuleSpecifiers(getModuleSpecifierExpression(s1), getModuleSpecifierExpression(s2)) || compareImportKind(s1, s2);
}

function compareImportKind(s1: AnyImportOrRequireStatement, s2: AnyImportOrRequireStatement) {
return compareValues(getImportKindOrder(s1), getImportKindOrder(s2));
}

// 1. Side-effect imports
// 2. Type-only imports
// 3. Namespace imports
// 4. Default imports
// 5. Named imports
// 6. ImportEqualsDeclarations
// 7. Require variable statements
function getImportKindOrder(s1: AnyImportOrRequireStatement) {
switch (s1.kind) {
case SyntaxKind.ImportDeclaration:
if (!s1.importClause) return 0;
if (s1.importClause.isTypeOnly) return 1;
if (s1.importClause.namedBindings?.kind === SyntaxKind.NamespaceImport) return 2;
if (s1.importClause.name) return 3;
return 4;
case SyntaxKind.ImportEqualsDeclaration:
return 5;
case SyntaxKind.VariableStatement:
return 6;
}
}
}
6 changes: 3 additions & 3 deletions src/services/refactors/moveToNewFile.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ namespace ts.refactor {
| ImportEqualsDeclaration
| VariableStatement;

function createOldFileImportsFromNewFile(newFileNeedExport: ReadonlySymbolSet, newFileNameWithExtension: string, useEs6Imports: boolean, quotePreference: QuotePreference): Statement | undefined {
function createOldFileImportsFromNewFile(newFileNeedExport: ReadonlySymbolSet, newFileNameWithExtension: string, useEs6Imports: boolean, quotePreference: QuotePreference): AnyImportOrRequireStatement | undefined {
let defaultImport: Identifier | undefined;
const imports: string[] = [];
newFileNeedExport.forEach(symbol => {
Expand All @@ -283,7 +283,7 @@ namespace ts.refactor {
return makeImportOrRequire(defaultImport, imports, newFileNameWithExtension, useEs6Imports, quotePreference);
}

function makeImportOrRequire(defaultImport: Identifier | undefined, imports: readonly string[], path: string, useEs6Imports: boolean, quotePreference: QuotePreference): Statement | undefined {
function makeImportOrRequire(defaultImport: Identifier | undefined, imports: readonly string[], path: string, useEs6Imports: boolean, quotePreference: QuotePreference): AnyImportOrRequireStatement | undefined {
path = ensurePathIsNonModuleName(path);
if (useEs6Imports) {
const specifiers = imports.map(i => factory.createImportSpecifier(/*propertyName*/ undefined, factory.createIdentifier(i)));
Expand All @@ -293,7 +293,7 @@ namespace ts.refactor {
Debug.assert(!defaultImport, "No default import should exist"); // If there's a default export, it should have been an es6 module.
const bindingElements = imports.map(i => factory.createBindingElement(/*dotDotDotToken*/ undefined, /*propertyName*/ undefined, i));
return bindingElements.length
? makeVariableStatement(factory.createObjectBindingPattern(bindingElements), /*type*/ undefined, createRequireCall(factory.createStringLiteral(path)))
? makeVariableStatement(factory.createObjectBindingPattern(bindingElements), /*type*/ undefined, createRequireCall(factory.createStringLiteral(path))) as RequireVariableStatement
: undefined;
}
}
Expand Down
7 changes: 5 additions & 2 deletions src/services/textChanges.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,9 +472,9 @@ namespace ts.textChanges {
this.insertNodesAt(sourceFile, start, typeParameters, { prefix: "<", suffix: ">" });
}

private getOptionsForInsertNodeBefore(before: Node, inserted: Node, doubleNewlines: boolean): InsertNodeOptions {
private getOptionsForInsertNodeBefore(before: Node, inserted: Node, blankLineBetween: boolean): InsertNodeOptions {
if (isStatement(before) || isClassElement(before)) {
return { suffix: doubleNewlines ? this.newLineCharacter + this.newLineCharacter : this.newLineCharacter };
return { suffix: blankLineBetween ? this.newLineCharacter + this.newLineCharacter : this.newLineCharacter };
}
else if (isVariableDeclaration(before)) { // insert `x = 1, ` into `const x = 1, y = 2;
return { suffix: ", " };
Expand All @@ -485,6 +485,9 @@ namespace ts.textChanges {
else if (isStringLiteral(before) && isImportDeclaration(before.parent) || isNamedImports(before)) {
return { suffix: ", " };
}
else if (isImportSpecifier(before)) {
return { suffix: "," + (blankLineBetween ? this.newLineCharacter : " ") };
}
return Debug.failBadSyntaxKind(before); // We haven't handled this kind of node yet -- add it
}

Expand Down
Loading