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.

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

Introduce an organizeImports command #21909

Merged
merged 4 commits into from
Feb 16, 2018
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
1 change: 1 addition & 0 deletions Jakefile.js
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ var harnessSources = harnessCoreSources.concat([
"typingsInstaller.ts",
"projectErrors.ts",
"matchFiles.ts",
"organizeImports.ts",
"initializeTSConfig.ts",
"extractConstants.ts",
"extractFunctions.ts",
Expand Down
5 changes: 5 additions & 0 deletions src/compiler/core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1905,6 +1905,11 @@ namespace ts {
Comparison.EqualTo;
}

/** True is greater than false. */
export function compareBooleans(a: boolean, b: boolean): Comparison {
return compareValues(a ? 1 : 0, b ? 1 : 0);
}

function compareMessageText(text1: string | DiagnosticMessageChain, text2: string | DiagnosticMessageChain): Comparison {
while (text1 && text2) {
// We still have both chains.
Expand Down
3 changes: 3 additions & 0 deletions src/harness/harnessLanguageService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -522,6 +522,9 @@ namespace Harness.LanguageService {
getApplicableRefactors(): ts.ApplicableRefactorInfo[] {
throw new Error("Not supported on the shim.");
}
organizeImports(_scope: ts.OrganizeImportsScope, _formatOptions: ts.FormatCodeSettings): ReadonlyArray<ts.FileTextChanges> {
throw new Error("Not supported on the shim.");
}
getEmitOutput(fileName: string): ts.EmitOutput {
return unwrapJSONCallResult(this.shim.getEmitOutput(fileName));
}
Expand Down
1 change: 1 addition & 0 deletions src/harness/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@
"./unittests/tsserverProjectSystem.ts",
"./unittests/tscWatchMode.ts",
"./unittests/matchFiles.ts",
"./unittests/organizeImports.ts",
"./unittests/initializeTSConfig.ts",
"./unittests/compileOnSave.ts",
"./unittests/typingsInstaller.ts",
Expand Down
426 changes: 426 additions & 0 deletions src/harness/unittests/organizeImports.ts

Large diffs are not rendered by default.

2 changes: 2 additions & 0 deletions src/harness/unittests/session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,8 @@ namespace ts.server {
CommandNames.GetApplicableRefactors,
CommandNames.GetEditsForRefactor,
CommandNames.GetEditsForRefactorFull,
CommandNames.OrganizeImports,
CommandNames.OrganizeImportsFull,
];

it("should not throw when commands are executed with invalid arguments", () => {
Expand Down
4 changes: 4 additions & 0 deletions src/server/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -629,6 +629,10 @@ namespace ts.server {
};
}

organizeImports(_scope: OrganizeImportsScope, _formatOptions: FormatCodeSettings): ReadonlyArray<FileTextChanges> {
return notImplemented();
}

private convertCodeEditsToTextChanges(edits: protocol.FileCodeEdits[]): FileTextChanges[] {
return edits.map(edit => {
const fileName = edit.fileName;
Expand Down
25 changes: 25 additions & 0 deletions src/server/protocol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,10 @@ namespace ts.server.protocol {
/* @internal */
GetEditsForRefactorFull = "getEditsForRefactor-full",

OrganizeImports = "organizeImports",
/* @internal */
OrganizeImportsFull = "organizeImports-full",

// NOTE: If updating this, be sure to also update `allCommandNames` in `harness/unittests/session.ts`.
}

Expand Down Expand Up @@ -547,6 +551,27 @@ namespace ts.server.protocol {
renameFilename?: string;
}

/**
Copy link

Choose a reason for hiding this comment

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

This comment might be better in organizeImports.ts (the non-test one).

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 can move it when I implement remove-unused.

* Organize imports by:
* 1) Removing unused imports
* 2) Coalescing imports from the same module
* 3) Sorting imports
*/
export interface OrganizeImportsRequest extends Request {
command: CommandTypes.OrganizeImports;
arguments: OrganizeImportsRequestArgs;
}

export type OrganizeImportsScope = GetCombinedCodeFixScope;

export interface OrganizeImportsRequestArgs {
scope: OrganizeImportsScope;
}

export interface OrganizeImportsResponse extends Response {
edits: ReadonlyArray<FileCodeEdits>;
}

/**
* Request for the available codefixes at a specific position.
*/
Expand Down
19 changes: 19 additions & 0 deletions src/server/session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1597,6 +1597,19 @@ namespace ts.server {
}
}

private organizeImports({ scope }: protocol.OrganizeImportsRequestArgs, simplifiedResult: boolean): ReadonlyArray<protocol.FileCodeEdits> | ReadonlyArray<FileTextChanges> {
Debug.assert(scope.type === "file");
const { file, project } = this.getFileAndProject(scope.args);
const formatOptions = this.projectService.getFormatCodeOptions(file);
const changes = project.getLanguageService().organizeImports({ type: "file", fileName: file }, formatOptions);
if (simplifiedResult) {
return this.mapTextChangesToCodeEdits(project, changes);
}
else {
return changes;
}
}

private getCodeFixes(args: protocol.CodeFixRequestArgs, simplifiedResult: boolean): ReadonlyArray<protocol.CodeAction> | ReadonlyArray<CodeAction> {
if (args.errorCodes.length === 0) {
return undefined;
Expand Down Expand Up @@ -2041,6 +2054,12 @@ namespace ts.server {
},
[CommandNames.GetEditsForRefactorFull]: (request: protocol.GetEditsForRefactorRequest) => {
return this.requiredResponse(this.getEditsForRefactor(request.arguments, /*simplifiedResult*/ false));
},
[CommandNames.OrganizeImports]: (request: protocol.OrganizeImportsRequest) => {
return this.requiredResponse(this.organizeImports(request.arguments, /*simplifiedResult*/ true));
},
[CommandNames.OrganizeImportsFull]: (request: protocol.OrganizeImportsRequest) => {
return this.requiredResponse(this.organizeImports(request.arguments, /*simplifiedResult*/ false));
}
});

Expand Down
234 changes: 234 additions & 0 deletions src/services/organizeImports.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,234 @@
/* @internal */
namespace ts.OrganizeImports {
export function organizeImports(
sourceFile: SourceFile,
formatContext: formatting.FormatContext,
host: LanguageServiceHost) {

// TODO (https://github.com/Microsoft/TypeScript/issues/10020): sort *within* ambient modules (find using isAmbientModule)

// All of the old ImportDeclarations in the file, in syntactic order.
const oldImportDecls = sourceFile.statements.filter(isImportDeclaration);

if (oldImportDecls.length === 0) {
return [];
}

const oldValidImportDecls = oldImportDecls.filter(importDecl => getExternalModuleName(importDecl.moduleSpecifier));
const oldInvalidImportDecls = oldImportDecls.filter(importDecl => !getExternalModuleName(importDecl.moduleSpecifier));
Copy link

Choose a reason for hiding this comment

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

Not sure this is needed -- we're basically just deleting these and adding them back, could just ignore them entirely and make oldImportDecls only be the valid ones.

Copy link
Member Author

Choose a reason for hiding this comment

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

If they are scattered throughout the file, this will group them together at the top.

Copy link

Choose a reason for hiding this comment

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

OK. For the case where you do keep the invalid ones, could use a helper function and write const [oldValidImportDecls, oldInvalidImportDecls] = partition(oldImportDecls, isValidImportDeclaration);


// All of the new ImportDeclarations in the file, in sorted order.
const newImportDecls = coalesceImports(sortImports(removeUnusedImports(oldValidImportDecls))).concat(oldInvalidImportDecls);

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

// Delete or replace the first import.
if (newImportDecls.length === 0) {
changeTracker.deleteNode(sourceFile, oldImportDecls[0]);
}
else {
// Note: Delete the surrounding trivia because it will have been retained in newImportDecls.
changeTracker.replaceNodeWithNodes(sourceFile, oldImportDecls[0], newImportDecls, {
useNonAdjustedStartPosition: false,
useNonAdjustedEndPosition: false,
suffix: getNewLineOrDefaultFromHost(host, formatContext.options),
});
}

// Delete any subsequent imports.
for (let i = 1; i < oldImportDecls.length; i++) {
changeTracker.deleteNode(sourceFile, oldImportDecls[i]);
}

return changeTracker.getChanges();
}

function removeUnusedImports(oldImports: ReadonlyArray<ImportDeclaration>) {
return oldImports; // TODO (https://github.com/Microsoft/TypeScript/issues/10020)
}

/* @internal */ // Internal for testing
export function sortImports(oldImports: ReadonlyArray<ImportDeclaration>) {
return stableSort(oldImports, (import1, import2) => {
const name1 = getExternalModuleName(import1.moduleSpecifier);
const name2 = getExternalModuleName(import2.moduleSpecifier);
Debug.assert(name1 !== undefined);
Debug.assert(name2 !== undefined);
return compareBooleans(isExternalModuleNameRelative(name1), isExternalModuleNameRelative(name2)) ||
compareStringsCaseSensitive(name1, name2);
});
}

function getExternalModuleName(specifier: Expression) {
return isStringLiteral(specifier) || isNoSubstitutionTemplateLiteral(specifier)
? specifier.text
: undefined;
}

/**
* @param sortedImports a non-empty list of ImportDeclarations, sorted by module name.
*/
function groupSortedImports(sortedImports: ReadonlyArray<ImportDeclaration>): ReadonlyArray<ReadonlyArray<ImportDeclaration>> {
Debug.assert(length(sortedImports) > 0);
Copy link

Choose a reason for hiding this comment

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

function groupSortedImports(sortedImports: ReadonlyArray<ImportDeclaration>): ReadonlyArray<ReadonlyArray<ImportDeclaration>> {
    return groupBy(sortedImports, i => getExternalModuleName(i.moduleSpecifier));
}

function groupBy<T>(values: ReadonlyArray<T>, by: (value: T) => string): ReadonlyArray<ReadonlyArray<T>> {
    Debug.assert(values.length !== 0);
    const groups: T[][] = [];
    let groupName = by(values[0]);
    let group: T[] = [];
    for (const value of values) {
        const b = by(value);
        if (b === groupName) {
            group.push(value);
        }
        else {
            if (group.length) {
                groups.push(group);
            }
            groupName = b;
            group = [value];
        }
    }
    if (group.length) {
        groups.push(group);
    }
    return groups;
}

Copy link

Choose a reason for hiding this comment

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

Maybe better:

function groupSortedImports(sortedImports: ReadonlyArray<ImportDeclaration>): ReadonlyArray<ReadonlyArray<ImportDeclaration>> {
    return groupBy(sortedImports, (a, b) => getExternalModuleName(a.moduleSpecifier) === getExternalModuleName(b.moduleSpecifier));
}

function groupBy<T>(values: ReadonlyArray<T>, areSameGroup: (a: T, b: T) => boolean): ReadonlyArray<ReadonlyArray<T>> {
    Debug.assert(values.length !== 0);
    const groups: T[][] = [];
    let group: T[] = [];
    for (const value of values) {
        if (group.length && !areSameGroup(value, group[0])) {
            groups.push(group);
            group = [];
        }
        group.push(value);
    }
    if (group.length) {
        groups.push(group);
    }
    return groups;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to confirm, your improvement was pulling out a generic helper? The actual behavior is the same?

Copy link

@ghost ghost Feb 14, 2018

Choose a reason for hiding this comment

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

Yes, assuming invalid imports are filtered out (so getExternalModuleName returns a defined result).

Copy link

Choose a reason for hiding this comment

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

Just noticed we don't need Debug.assert(values.length !== 0); in the "maybe better" version.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that this isn't true group-by - it requires that the input be sorted. Is it still worth pulling out a generic helper for specialized functionality?

Copy link

Choose a reason for hiding this comment

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

Actually, this doesn't just rely on it being sorted, but on it being sorted by the same mechanism as areSameGroup. Maybe it would be better to group first (not relying on sorting) and then sort the groups?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's why it's a private helper function. At the moment, there doesn't seem to be a need for something more general, does there?

Copy link
Member Author

Choose a reason for hiding this comment

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

As you requested, we are now grouping before performing the other operations.


const groups: ImportDeclaration[][] = [];

let groupName: string | undefined = getExternalModuleName(sortedImports[0].moduleSpecifier);
Debug.assert(groupName !== undefined);
let group: ImportDeclaration[] = [];

for (const importDeclaration of sortedImports) {
const moduleName = getExternalModuleName(importDeclaration.moduleSpecifier);
Debug.assert(moduleName !== undefined);
if (moduleName === groupName) {
group.push(importDeclaration);
}
else if (group.length) {
groups.push(group);

groupName = moduleName;
group = [importDeclaration];
}
}

if (group.length) {
groups.push(group);
}

return groups;
}

/* @internal */ // Internal for testing
/**
* @param sortedImports a list of ImportDeclarations, sorted by module name.
*/
export function coalesceImports(sortedImports: ReadonlyArray<ImportDeclaration>) {
if (sortedImports.length === 0) {
Copy link

Choose a reason for hiding this comment

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

You already tested for length at the top of organizeImports, and sorting wouldn't have changed the length.

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 believe this also runs after unused import removal.

return sortedImports;
}

const coalescedImports: ImportDeclaration[] = [];

const groupedImports = groupSortedImports(sortedImports);
for (const importGroup of groupedImports) {

const { importWithoutClause, defaultImports, namespaceImports, namedImports } = getImportParts(importGroup);

if (importWithoutClause) {
coalescedImports.push(importWithoutClause);
}

// Normally, we don't combine default and namespace imports, but it would be silly to
// produce two import declarations in this special case.
if (defaultImports.length === 1 && namespaceImports.length === 1 && namedImports.length === 0) {
// Add the namespace import to the existing default ImportDeclaration.
const defaultImportClause = defaultImports[0].parent as ImportClause;
coalescedImports.push(
updateImportDeclarationAndClause(defaultImportClause, defaultImportClause.name, namespaceImports[0]));

continue;
}

const sortedNamespaceImports = stableSort(namespaceImports, (n1, n2) => compareIdentifiers(n1.name, n2.name));

for (const namespaceImport of sortedNamespaceImports) {
// Drop the name, if any
coalescedImports.push(
updateImportDeclarationAndClause(namespaceImport.parent, /*name*/ undefined, namespaceImport));
}

if (defaultImports.length === 0 && namedImports.length === 0) {
continue;
}

let newDefaultImport: Identifier | undefined;
const newImportSpecifiers: ImportSpecifier[] = [];
if (defaultImports.length === 1) {
newDefaultImport = defaultImports[0];
}
else {
for (const defaultImport of defaultImports) {
newImportSpecifiers.push(
createImportSpecifier(createIdentifier("default"), defaultImport));
}
}

newImportSpecifiers.push(...flatMap(namedImports, n => n.elements));

const sortedImportSpecifiers = stableSort(newImportSpecifiers, (s1, s2) =>
compareIdentifiers(s1.propertyName || s1.name, s2.propertyName || s2.name) ||
compareIdentifiers(s1.name, s2.name));

const importClause = defaultImports.length > 0
? defaultImports[0].parent as ImportClause
: namedImports[0].parent;

const newNamedImports = sortedImportSpecifiers.length === 0
? undefined
: namedImports.length === 0
? createNamedImports(sortedImportSpecifiers)
: updateNamedImports(namedImports[0], sortedImportSpecifiers);

coalescedImports.push(
updateImportDeclarationAndClause(importClause, newDefaultImport, newNamedImports));
}

return coalescedImports;

function getImportParts(importGroup: ReadonlyArray<ImportDeclaration>) {
let importWithoutClause: ImportDeclaration | undefined;
const defaultImports: Identifier[] = [];
const namespaceImports: NamespaceImport[] = [];
const namedImports: NamedImports[] = [];

for (const importDeclaration of importGroup) {
if (importDeclaration.importClause === undefined) {
// Only the first such import is interesting - the others are redundant.
// Note: Unfortunately, we will lose trivia that was on this node.
importWithoutClause = importWithoutClause || importDeclaration;
continue;
}

const { name, namedBindings } = importDeclaration.importClause;

if (name) {
defaultImports.push(name);
}

if (namedBindings) {
if (isNamespaceImport(namedBindings)) {
namespaceImports.push(namedBindings);
}
else {
namedImports.push(namedBindings);
}
}
}

return {
importWithoutClause,
defaultImports,
namespaceImports,
namedImports,
};
}

function compareIdentifiers(s1: Identifier, s2: Identifier) {
return compareStringsCaseSensitive(s1.text, s2.text);
}

function updateImportDeclarationAndClause(
importClause: ImportClause,
name: Identifier | undefined,
namedBindings: NamedImportBindings | undefined) {

const importDeclaration = importClause.parent;
return updateImportDeclaration(
importDeclaration,
importDeclaration.decorators,
importDeclaration.modifiers,
updateImportClause(importClause, name, namedBindings),
importDeclaration.moduleSpecifier);
}
}
}
11 changes: 11 additions & 0 deletions src/services/services.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
/// <reference path='jsTyping.ts' />
/// <reference path='navigateTo.ts' />
/// <reference path='navigationBar.ts' />
/// <reference path='organizeImports.ts' />
/// <reference path='outliningElementsCollector.ts' />
/// <reference path='patternMatcher.ts' />
/// <reference path='preProcess.ts' />
Expand Down Expand Up @@ -1848,6 +1849,15 @@ namespace ts {
return codefix.getAllFixes({ fixId, sourceFile, program, host, cancellationToken, formatContext });
}

function organizeImports(scope: OrganizeImportsScope, formatOptions: FormatCodeSettings): ReadonlyArray<FileTextChanges> {
Copy link

Choose a reason for hiding this comment

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

I think these changes are substantial enough to deserve an organizeImports.ts file, just like we have for breakpoints/classifier/etc.

synchronizeHostData();
Debug.assert(scope.type === "file");
const sourceFile = getValidSourceFile(scope.fileName);
const formatContext = formatting.getFormatContext(formatOptions);

return OrganizeImports.organizeImports(sourceFile, formatContext, host);
}

function applyCodeActionCommand(action: CodeActionCommand): Promise<ApplyCodeActionCommandResult>;
function applyCodeActionCommand(action: CodeActionCommand[]): Promise<ApplyCodeActionCommandResult[]>;
function applyCodeActionCommand(action: CodeActionCommand | CodeActionCommand[]): Promise<ApplyCodeActionCommandResult | ApplyCodeActionCommandResult[]>;
Expand Down Expand Up @@ -2143,6 +2153,7 @@ namespace ts {
getCodeFixesAtPosition,
getCombinedCodeFix,
applyCodeActionCommand,
organizeImports,
getEmitOutput,
getNonBoundSourceFile,
getSourceFile,
Expand Down
Loading