Introduce an organizeImports command #21909

amcasey · 2018-02-12T23:09:21Z

In phase 1, it coalesces imports from the same module and sorts the
results, but does not remove unused imports.

Some trivia is lost during coalescing, but none should be duplicated.

amcasey · 2018-02-12T23:10:00Z

This will need to be reconciled with #21876.

amcasey · 2018-02-12T23:31:47Z

src/services/services.ts

+                }
+            }
+
+            const sortedImportSpecifiers = stableSort(newImportSpecifiers, (s1, s2) => {


This comparison should be shared with #21876.

amcasey · 2018-02-12T23:53:08Z

src/services/services.ts

+                if (isImportDeclaration(node)) {
+                    oldImportDecls.push(node);
+                }
+                // TODO (https://github.com/Microsoft/TypeScript/issues/10020): sort *within* ambient modules (find using isAmbientModule)


Oops, forgot to mention this other TODO in the description.

amcasey · 2018-02-13T02:22:29Z

I don't understand the build failures. I ran jake runtests-parallel locally and it claims to have run the same link command as the server. It reported no errors. However, when I ran the same lint command manually, its output matched the Jenkins console output. Is it possible that runtests-parallel is swallowing (or not waiting for) the lint errors? /cc @weswigham

amcasey · 2018-02-13T02:25:42Z

e:\ts_gh>jake runtests-parallel
Discovered 100 unittest suites.
Discovering runner-based tests...
Discovered 11829 test files in 504ms.
Starting to run tests using 8 threads...
Batching initial test lists...
Batched into 8 groups with approximate total time of 1m34s in each group. (90.0% of total tests batched)

  [▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬] √ 59191 passing (2m9s)


  59191 passing (2m)

Linting: node node_modules/tslint/bin/tslint Gulpfile.ts scripts/generateLocalizedDiagnosticMessages.ts "scripts/tslint/**/*.ts" "src/**/*.ts" --exclude "src/lib/*.d.ts" --formatters-dir ./built/local/tslint/formatters --format autolinkableStylish
rm -rf tests\baselines\local\projectOutput\

e:\ts_gh>

weswigham · 2018-02-13T05:00:55Z

I think our mistake is that windows shells don't expand globbing patterns natively? Probably? I'm not sure.

ghost · 2018-02-13T16:36:02Z

src/harness/unittests/organizeImports.ts

+            }
+        }
+
+        function assertListEqual(list1: ReadonlyArray<Node>, list2: ReadonlyArray<Node>) {


I think it would be simpler to emit and compare strings, vs parsing and comparing ASTs. That way there's no need to write assertEqual and we also test if we are correctly formatting the result. Also would make error messages more readable.

Suits me. How do I stringify them?

I'm attempting to do this, but the emitter works best if the ast it's working on is either entirely synthesized, or not at all. I'm debugging and it looks like there are some nodes with pos set as children of a synthesized node, which causes the emitter to look at the source file for comments, only there is no source text. Some part of organizeImports might be forgetting to clone a node?
On this subject, we should have fourslash tests of this feature to ensure we're testing the whole thing, including emit.

Regarding testing with text, it seems not to have been simpler. I'm going to leave things as they are until we have a concrete problem to correct or improvement to make.

I was under the impression that the baseline tests were doing an end-to-end test. Did you mean something else?

No, baseline tests will do.

ghost · 2018-02-13T22:10:54Z

src/services/services.ts

@@ -1907,6 +1907,58 @@ namespace ts {
            return codefix.getAllFixes({ fixId, sourceFile, program, host, cancellationToken, formatContext });
        }

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


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

ghost · 2018-02-14T19:14:47Z

src/harness/unittests/organizeImports.ts

+                assertListEqual(expectedCoalescedImports, actualCoalescedImports);
+            });
+
+            it("Combine namespace imports", () => {


Name is "Combine namespace imports" but we're actually testing that we don't combine them

The name describes the action taken, rather than the outcome. Combining namespace imports happens to be a no-op.

ghost · 2018-02-14T19:17:42Z

src/harness/unittests/organizeImports.ts

+            });
+
+            it("Combine namespace import with default import", () => {
+                const sortedImports = parseImports(


This pattern occurs a lot, could there be a helper so this can be written:

assertCoalesce([`import * as x from "lib";`, `import y from "lib";`], `import y, * as x from "lib";`)

I left it expanded because I thought it made the test more readable. A list of unnamed strings can be hard to follow.

ghost · 2018-02-14T19:21:14Z

src/harness/unittests/organizeImports.ts

+                assert.equal(testPath, changes[0].fileName);
+
+                Harness.Baseline.runBaseline(baselinePath, () => {
+                    const data: string[] = [];


Since the things we push to data are the same, this could just be an array literal ["// ==ORIGINAL==", testContent, "// ==ORGANIZED==", textChanges.applyChanges(testContent, changes[0].textChanges)].join(newLineCharacter).

Frankly, I didn't understand why we didn't just build a string directly. Is the array literal preferable?

Well, it lets you avoid writing ${newLineCharacter} a lot... though that's hardcoded to \n anyway, so you could just write \n.

In older JS runtimes before everyone switched to ropes, repeated string concat was expensive. string[] -> one string via join is basically the equivalent of using a StringBuilder in C#

ghost · 2018-02-14T19:31:06Z

src/services/organizeImports.ts

+        cancellationToken: CancellationToken) {
+
+        // All of the (old) ImportDeclarations in the file, in syntactic order.
+        const oldImportDecls: ImportDeclaration[] = [];


const oldImportDecls: ReadonlyArray<ImportDeclaration> = sourceFile.statements.filter(isImportDeclaration); -- should be fast enough that you don't need to check cancellationToken inside the filter.
Actually, I would just remove cancellationToken unless we actually see a perf problem...

You could also filter out invalid imports here, and be able to assume that getExternalModuleName returns a defined result everywhere else.

Fair. It was mostly there so I could test the wait dialog behavior.

ghost · 2018-02-14T19:31:48Z

src/services/organizeImports.ts

+        cancellationToken.throwIfCancellationRequested();
+
+        // All of the (new) ImportDeclarations in the file, in sorted order.
+        const newImportDecls = coalescedImportDecls;


Just name coalescedImportDecls newImportDecls?

Are locals expensive in TS? I like naming things - it makes code easier to read.

I'm sure modern JS engines know that these are the same thing. I normally agree that giving a descriptive name for a variable is useful, but const coalescedImportDecls = coalesceImports(sortedImportDecls); just repeats the same information in two different names, since the function name already tells you what it's doing -- same for the above lines.

In the debugger, it's easy to inspect each stage individually. If you'd prefer not to have any redundant text, arguably they should be combined into a single expression with only final value stored in a named variable.

For pure functions, you can enter e.g. coalesceImports(sortedImportDecls) into the watch list for inspection without needing a named local variable.

ghost · 2018-02-14T19:33:20Z

src/services/organizeImports.ts

+        }
+        else {
+            // Delete the surrounding trivia because it will have been retained in newImportDecls.
+            const replaceOptions = {


Inline replaceOptions, that way we get a contextual type and can warn on misnamed properties.

ghost · 2018-02-14T19:33:48Z

src/services/organizeImports.ts

+            changeTracker.replaceNodeWithNodes(sourceFile, oldImportDecls[0], newImportDecls, replaceOptions);
+        }
+
+        const changes = changeTracker.getChanges();


return changeTracker.getChanges(); (remember the debugger will let you see the return value even if it is not named.)

ghost · 2018-02-14T19:38:11Z

src/services/organizeImports.ts

+    }
+
+    function getExternalModuleName(specifier: Expression) {
+        return isStringLiteral(specifier) || isNoSubstitutionTemplateLiteral(specifier)


#21953 would be useful here.

ghost · 2018-02-14T20:05:40Z

src/services/organizeImports.ts

+
+    /* @internal */ // Internal for testing
+    export function sortImports(oldImports: ReadonlyArray<ImportDeclaration>) {
+        if (oldImports.length < 2) {


In core.ts:

export function compareBooleans(a: boolean, b: boolean): Comparison { return compareValues(a ? 1 : 0, b ? 1 : 0); }

Perform length < 2 optimization inside stableSort

Here:

return stableSort(oldImports, ({ moduleSpecifier: m1 }, { moduleSpecifier: m2 }) => { const name1 = getExternalModuleName(m1) || m1.getText(); const name2 = getExternalModuleName(m2) || m2.getText(); return compareBooleans(isExternalModuleNameRelative(name1), isExternalModuleNameRelative(name2)) || compareStringsCaseSensitive(name1, name2); });

This would break the non-relative vs invalid and relative vs invalid tests, but I don't think it really matters how we sort invalid expressions in import positions.

I'm not sure I understand your concerns about the current implementation.

It's not that I spotted any bugs, just noticed that it could be done in 4 lines.

It hadn't occurred to me to combine Comparisons with || - applying that should clean things up a bit.

One of the reasons I pulled out the struct was so that the same work wouldn't be repeated for every comparison in which a given import participated. If that's not interesting, it can obviously be simplified.

If you filter out invalid imports at the top, getExternalModuleName is just a property lookup. isExternalModuleNameRelative isn't very complicated either -- some simple regex and indexOf tests.

RyanCavanaugh · 2018-02-14T20:20:38Z

src/services/organizeImports.ts

+            return [];
+        }
+
+        const usedImportDecls = removeUnusedImports(oldImportDecls);


Probably too many calls to throwIfCancellationRequested here - in general you only need to call this every ~50-200ms, and I don't think sortImports or coalesceImports could ever be that slow.

ghost · 2018-02-14T20:43:22Z

src/services/organizeImports.ts

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


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

I believe this also runs after unused import removal.

ghost · 2018-02-14T20:49:47Z

src/services/organizeImports.ts

+     * @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);


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; }

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

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

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

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?

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?

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?

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

ghost · 2018-02-14T20:52:33Z

src/services/organizeImports.ts

+        cancellationToken: CancellationToken) {
+
+        // All of the (old) ImportDeclarations in the file, in syntactic order.
+        const oldImportDecls: ImportDeclaration[] = [];


You could also filter out invalid imports here, and be able to assume that getExternalModuleName returns a defined result everywhere else.

ghost · 2018-02-14T20:57:43Z

src/services/organizeImports.ts

+        return coalescedImports;
+
+        // `undefined` is the min value.
+        function compareIdentifiers(s1: Identifier | undefined, s2: Identifier | undefined) {


return compareStringsCaseSensitive(s1 && s1.text, s2 && s2.text);. Also, these functions don't use any closure variables.

I'm accustomed to syntactic scopes conveying encapsulation. If it is more idiomatic for them to express closure, then I agree that these functions should not be nested.

Opinions on that vary around the team, so whichever is fine.

ghost · 2018-02-14T21:05:03Z

src/services/organizeImports.ts

+                }
+            }
+
+            for (const namedImport of namedImports) {


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

Thanks, I figured there had to be a function that did that. I was looking for "selectMany".

ghost · 2018-02-14T21:06:03Z

src/services/organizeImports.ts

+        const groupedImports = groupSortedImports(sortedImports);
+        for (const importGroup of groupedImports) {
+
+            let seenImportWithoutClause = false;


Think this could be its own function:

function getImportParts(importGroup: ReadonlyArray<ImportDeclaration> ): { importWithoutClause: ImportDeclaration | undefined, defaultImports: ReadonlyArray<Identifier>, namespaceImports: ReadonlyArray<NamespaceImport>, namedImports: ReadonlyArray<NamedImports>, } { let importWithoutClause: ImportDeclaration; 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 = 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 }; }

then:

const { importWithoutClause, defaultImports, namedImports, namespaceImports } = getImportParts(importGroup); if (importWithoutClause) { coalescedImports.push(importWithoutClause); } ...proceed as before...

ghost · 2018-02-14T21:07:39Z

src/services/organizeImports.ts

+                }
+            }
+
+            const sortedImportSpecifiers = stableSort(newImportSpecifiers, (s1, s2) => {


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

ghost · 2018-02-14T21:09:09Z

src/services/organizeImports.ts

+                continue;
+            }
+
+            let newDefaultImport: Identifier = undefined;


Identifier | undefined not Identifier = undefined

ghost · 2018-02-14T21:42:04Z

src/services/organizeImports.ts

+        const changeTracker = textChanges.ChangeTracker.fromContext({ host, formatContext });
+
+        // NB: Stopping before i === 0
+        for (let i = oldImportDecls.length - 1; i > 0; i--) {


Could you add a comment explaining why this is in reverse order? I assume this has something to do with trivia?

It's not - I was thinking of something else.

ghost · 2018-02-15T16:26:40Z

src/services/organizeImports.ts

+        }
+
+        const oldValidImportDecls = oldImportDecls.filter(importDecl => getExternalModuleName(importDecl.moduleSpecifier));
+        const oldInvalidImportDecls = oldImportDecls.filter(importDecl => !getExternalModuleName(importDecl.moduleSpecifier));


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.

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

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

ghost · 2018-02-15T16:31:21Z

src/services/organizeImports.ts

+        });
+    }
+
+    function getExternalModuleName(specifier: Expression) {


Could have a ValidImportDeclaration type that always has a valid specifier, and then not need to use this function or worry about undefined results.

interface ValidImportDeclaration extends ImportDeclaration { moduleSpecifier: StringLiteral; } function isValidImportDeclaration(node: Node): node is ValidImportDeclaration { return isImportDeclaration(node) && isStringLiteral(node.moduleSpecifier); }

then:

const oldImportDecls = sourceFile.statements.filter(isValidImportDeclaration);

(We could technically allow NoSubstitutionTemplateLiterals here, but they're actually compile errors so not really valid.)

amcasey · 2018-02-15T23:50:45Z

Restarting tests since lint error line numbers didn't match current source.

amcasey · 2018-02-16T00:05:34Z

I am unable to repro the failure The baseline file organizeImports/CoalesceMultipleModules.ts has changed.

ghost · 2018-02-16T00:10:42Z

@amcasey Might have to do with newlines?

amcasey · 2018-02-16T00:11:59Z

@andy-ms Conceivably, but it would be strange to have only this one failure.

Edit: They're hard-coded to use \n.

amcasey · 2018-02-16T18:44:49Z

We have many files with mixed line endings, but my current hypothesis is that the problem was introduced when I renamed the baseline output directory from "OrganizeImports" to "organizeImports" for consistency with the others. I'll try to purge the old name locally and then force push.

amcasey · 2018-02-16T18:45:16Z

Other than this irritating issue, is this more or less ready to merge?

In phase 1, it coalesces imports from the same module and sorts the results, but does not remove unused imports. Some trivia is lost during coalescing, but none should be duplicated.

ghost · 2018-02-16T18:53:43Z

src/server/protocol.ts

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

+    /**


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

I can move it when I implement remove-unused.

Eliminate cancellation token Add organizeImports.ts to tsconfig.json Simplify ts.OrganizeImports.organizeImports Simplify sortImports Semantic change: all invalid module specifiers are now considered to be equal. Simplify comparisons using || Pull out imports with invalid modules specifiers ...for separate processing. They are tacked on to the end of the organized imports in their original order. Bonus: downstream functions can now assume imports have valid module specifiers. Rename baseline folder with leading lowercase Simplify coalesceImports Remove some unnecessary null checks Simplify baseline generation

amcasey · 2018-02-16T18:59:00Z

I've revised history so that the baseline folder has always been lowercase. Hopefully, that will fix the tests.

MgSam · 2018-04-05T03:27:20Z

How do you invoke this command in Visual Studio? I've been completely unable to get it to appear anywhere since upgrading to 2.8.

mhegazy · 2018-04-05T04:03:11Z

This is a new command, so requires update to VS as well. Should light up in your next VS update.

amcasey requested review from mhegazy and a user February 12, 2018 23:09

mhegazy mentioned this pull request Feb 12, 2018

Added alphabetization logic to missing import quick fixes #21876

Closed

amcasey commented Feb 12, 2018

View reviewed changes

ghost reviewed Feb 13, 2018

View reviewed changes

ghost reviewed Feb 14, 2018

View reviewed changes

RyanCavanaugh reviewed Feb 14, 2018

View reviewed changes

ghost reviewed Feb 14, 2018

View reviewed changes

ghost reviewed Feb 15, 2018

View reviewed changes

amcasey force-pushed the OrganizeImports branch from a398e71 to 7498043 Compare February 15, 2018 23:17

amcasey closed this Feb 15, 2018

amcasey reopened this Feb 15, 2018

Introduce an organizeImports command

5656f35

In phase 1, it coalesces imports from the same module and sorts the results, but does not remove unused imports. Some trivia is lost during coalescing, but none should be duplicated.

ghost approved these changes Feb 16, 2018

View reviewed changes

amcasey added 2 commits February 16, 2018 10:52

Fix lint errors

979b146

Separate OrganizeImports into its own namespace and file

f4141ac

ghost reviewed Feb 16, 2018

View reviewed changes

amcasey force-pushed the OrganizeImports branch from 0c2babc to 5c278ce Compare February 16, 2018 18:57

amcasey merged commit 70e9a5e into microsoft:master Feb 16, 2018

amcasey deleted the OrganizeImports branch February 16, 2018 20:49

This was referenced Feb 16, 2018

Group imports before sorting and coalescing #22001

Merged

Support 'Organize Imports' feature #10020

Closed

josteink mentioned this pull request Mar 1, 2018

add support for organizeImports command ananthakumaran/tide#241

Merged

microsoft locked and limited conversation to collaborators Jul 25, 2018

Introduce an organizeImports command #21909

Introduce an organizeImports command #21909

Conversation

amcasey commented Feb 12, 2018

amcasey commented Feb 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcasey commented Feb 13, 2018

amcasey commented Feb 13, 2018

weswigham commented Feb 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Feb 13, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcasey Feb 14, 2018 • edited Loading

Choose a reason for hiding this comment

ghost Feb 14, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Feb 14, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcasey commented Feb 15, 2018

amcasey commented Feb 16, 2018

ghost commented Feb 16, 2018

amcasey commented Feb 16, 2018 • edited Loading

amcasey commented Feb 16, 2018

ghost Feb 13, 2018 •

edited by ghost

Loading

amcasey Feb 14, 2018 •

edited

Loading

ghost Feb 14, 2018 •

edited by ghost

Loading

ghost Feb 14, 2018 •

edited by ghost

Loading

amcasey commented Feb 16, 2018 •

edited

Loading