Add 'move to new file' refactor (#23726)

* Add 'move to new file' refactor * Code review, and support commonjs * Compute movedSymbols completely before using, and support `export import` * Fix assertion error: sort empty change before non-empty change * Remove extra newline * Add allowTextChangesInNewFiles preference * Add the new file to 'files' in tsconfig * Avoid parameter initializer * Update API baselines * Use path relative to tsconfig.json * Code review * Fix error where node in tsconfig file was missing a source file
microsoft · May 10, 2018 · 7271ec1 · 7271ec1
1 parent 6149b41
commit 7271ec1
Show file tree

Hide file tree

Showing 45 changed files with 1,254 additions and 116 deletions.
diff --git a/src/compiler/core.ts b/src/compiler/core.ts
@@ -311,8 +311,8 @@ namespace ts {
     }
 
     /** Works like Array.prototype.findIndex, returning `-1` if no element satisfying the predicate is found. */
-    export function findIndex<T>(array: ReadonlyArray<T>, predicate: (element: T, index: number) => boolean): number {
-        for (let i = 0; i < array.length; i++) {
+    export function findIndex<T>(array: ReadonlyArray<T>, predicate: (element: T, index: number) => boolean, startIndex?: number): number {
+        for (let i = startIndex || 0; i < array.length; i++) {
             if (predicate(array[i], i)) {
                 return i;
             }
@@ -2294,20 +2294,24 @@ namespace ts {
         return ["", ...relative, ...components];
     }
 
+    export function getRelativePathFromFile(from: string, to: string, getCanonicalFileName: GetCanonicalFileName) {
+        return ensurePathIsNonModuleName(getRelativePathFromDirectory(getDirectoryPath(from), to, getCanonicalFileName));
+    }
+
     /**
      * Gets a relative path that can be used to traverse between `from` and `to`.
      */
-    export function getRelativePath(from: string, to: string, ignoreCase: boolean): string;
+    export function getRelativePathFromDirectory(from: string, to: string, ignoreCase: boolean): string;
     /**
      * Gets a relative path that can be used to traverse between `from` and `to`.
      */
     // tslint:disable-next-line:unified-signatures
-    export function getRelativePath(from: string, to: string, getCanonicalFileName: GetCanonicalFileName): string;
-    export function getRelativePath(from: string, to: string, getCanonicalFileNameOrIgnoreCase: GetCanonicalFileName | boolean) {
-        Debug.assert((getRootLength(from) > 0) === (getRootLength(to) > 0), "Paths must either both be absolute or both be relative");
+    export function getRelativePathFromDirectory(fromDirectory: string, to: string, getCanonicalFileName: GetCanonicalFileName): string;
+    export function getRelativePathFromDirectory(fromDirectory: string, to: string, getCanonicalFileNameOrIgnoreCase: GetCanonicalFileName | boolean) {
+        Debug.assert((getRootLength(fromDirectory) > 0) === (getRootLength(to) > 0), "Paths must either both be absolute or both be relative");
         const getCanonicalFileName = typeof getCanonicalFileNameOrIgnoreCase === "function" ? getCanonicalFileNameOrIgnoreCase : identity;
         const ignoreCase = typeof getCanonicalFileNameOrIgnoreCase === "boolean" ? getCanonicalFileNameOrIgnoreCase : false;
-        const pathComponents = getPathComponentsRelativeTo(from, to, ignoreCase ? equateStringsCaseInsensitive : equateStringsCaseSensitive, getCanonicalFileName);
+        const pathComponents = getPathComponentsRelativeTo(fromDirectory, to, ignoreCase ? equateStringsCaseInsensitive : equateStringsCaseSensitive, getCanonicalFileName);
         return getPathFromPathComponents(pathComponents);
     }
 

diff --git a/src/compiler/diagnosticMessages.json b/src/compiler/diagnosticMessages.json
@@ -4245,5 +4245,9 @@
     "Convert all 'require' to 'import'": {
         "category": "Message",
         "code": 95048
+    },
+    "Move to a new file": {
+        "category": "Message",
+        "code": 95049
     }
 }
diff --git a/src/compiler/factory.ts b/src/compiler/factory.ts
@@ -1505,6 +1505,13 @@ namespace ts {
         return block;
     }
 
+    /* @internal */
+    export function createExpressionStatement(expression: Expression): ExpressionStatement {
+        const node = <ExpressionStatement>createSynthesizedNode(SyntaxKind.ExpressionStatement);
+        node.expression = expression;
+        return node;
+    }
+
     export function updateBlock(node: Block, statements: ReadonlyArray<Statement>) {
         return node.statements !== statements
             ? updateNode(createBlock(statements, node.multiLine), node)
@@ -1531,9 +1538,7 @@ namespace ts {
     }
 
     export function createStatement(expression: Expression) {
-        const node = <ExpressionStatement>createSynthesizedNode(SyntaxKind.ExpressionStatement);
-        node.expression = parenthesizeExpressionForExpressionStatement(expression);
-        return node;
+        return createExpressionStatement(parenthesizeExpressionForExpressionStatement(expression));
     }
 
     export function updateStatement(node: ExpressionStatement, expression: Expression) {

diff --git a/src/harness/fourslash.ts b/src/harness/fourslash.ts
@@ -3005,9 +3005,7 @@ Actual: ${stringify(fullActual)}`);
         }
 
         public verifyApplicableRefactorAvailableAtMarker(negative: boolean, markerName: string) {
-            const marker = this.getMarkerByName(markerName);
-            const applicableRefactors = this.languageService.getApplicableRefactors(this.activeFile.fileName, marker.position, ts.defaultPreferences);
-            const isAvailable = applicableRefactors && applicableRefactors.length > 0;
+            const isAvailable = this.getApplicableRefactors(this.getMarkerByName(markerName).position).length > 0;
             if (negative && isAvailable) {
                 this.raiseError(`verifyApplicableRefactorAvailableAtMarker failed - expected no refactor at marker ${markerName} but found some.`);
             }
@@ -3024,9 +3022,7 @@ Actual: ${stringify(fullActual)}`);
         }
 
         public verifyRefactorAvailable(negative: boolean, name: string, actionName?: string) {
-            const selection = this.getSelection();
-
-            let refactors = this.languageService.getApplicableRefactors(this.activeFile.fileName, selection, ts.defaultPreferences) || [];
+            let refactors = this.getApplicableRefactors(this.getSelection());
             refactors = refactors.filter(r => r.name === name && (actionName === undefined || r.actions.some(a => a.name === actionName)));
             const isAvailable = refactors.length > 0;
 
@@ -3046,10 +3042,7 @@ Actual: ${stringify(fullActual)}`);
         }
 
         public verifyRefactor({ name, actionName, refactors }: FourSlashInterface.VerifyRefactorOptions) {
-            const selection = this.getSelection();
-
-            const actualRefactors = (this.languageService.getApplicableRefactors(this.activeFile.fileName, selection, ts.defaultPreferences) || ts.emptyArray)
-                .filter(r => r.name === name && r.actions.some(a => a.name === actionName));
+            const actualRefactors = this.getApplicableRefactors(this.getSelection()).filter(r => r.name === name && r.actions.some(a => a.name === actionName));
             this.assertObjectsEqual(actualRefactors, refactors);
         }
 
@@ -3059,8 +3052,7 @@ Actual: ${stringify(fullActual)}`);
                 throw new Error("Exactly one refactor range is allowed per test.");
             }
 
-            const applicableRefactors = this.languageService.getApplicableRefactors(this.activeFile.fileName, ts.first(ranges), ts.defaultPreferences);
-            const isAvailable = applicableRefactors && applicableRefactors.length > 0;
+            const isAvailable = this.getApplicableRefactors(ts.first(ranges)).length > 0;
             if (negative && isAvailable) {
                 this.raiseError(`verifyApplicableRefactorAvailableForRange failed - expected no refactor but found some.`);
             }
@@ -3071,7 +3063,7 @@ Actual: ${stringify(fullActual)}`);
 
         public applyRefactor({ refactorName, actionName, actionDescription, newContent: newContentWithRenameMarker }: FourSlashInterface.ApplyRefactorOptions) {
             const range = this.getSelection();
-            const refactors = this.languageService.getApplicableRefactors(this.activeFile.fileName, range, ts.defaultPreferences);
+            const refactors = this.getApplicableRefactors(range);
             const refactorsWithName = refactors.filter(r => r.name === refactorName);
             if (refactorsWithName.length === 0) {
                 this.raiseError(`The expected refactor: ${refactorName} is not available at the marker location.\nAvailable refactors: ${refactors.map(r => r.name)}`);
@@ -3117,7 +3109,48 @@ Actual: ${stringify(fullActual)}`);
                     return { renamePosition, newContent };
                 }
             }
+        }
 
+        public noMoveToNewFile() {
+            for (const range of this.getRanges()) {
+                for (const refactor of this.getApplicableRefactors(range, { allowTextChangesInNewFiles: true })) {
+                    if (refactor.name === "Move to a new file") {
+                        ts.Debug.fail("Did not expect to get 'move to a new file' refactor");
+                    }
+                }
+            }
+        }
+
+        public moveToNewFile(options: FourSlashInterface.MoveToNewFileOptions): void {
+            assert(this.getRanges().length === 1);
+            const range = this.getRanges()[0];
+            const refactor = ts.find(this.getApplicableRefactors(range, { allowTextChangesInNewFiles: true }), r => r.name === "Move to a new file");
+            assert(refactor.actions.length === 1);
+            const action = ts.first(refactor.actions);
+            assert(action.name === "Move to a new file" && action.description === "Move to a new file");
+
+            const editInfo = this.languageService.getEditsForRefactor(this.activeFile.fileName, this.formatCodeSettings, range, refactor.name, action.name, ts.defaultPreferences);
+            for (const edit of editInfo.edits) {
+                const newContent = options.newFileContents[edit.fileName];
+                if (newContent === undefined) {
+                    this.raiseError(`There was an edit in ${edit.fileName} but new content was not specified.`);
+                }
+                if (this.testData.files.some(f => f.fileName === edit.fileName)) {
+                    this.applyEdits(edit.fileName, edit.textChanges, /*isFormattingEdit*/ false);
+                    this.openFile(edit.fileName);
+                    this.verifyCurrentFileContent(newContent);
+                }
+                else {
+                    assert(edit.textChanges.length === 1);
+                    const change = ts.first(edit.textChanges);
+                    assert.deepEqual(change.span, ts.createTextSpan(0, 0));
+                    assert.equal(change.newText, newContent, `Content for ${edit.fileName}`);
+                }
+            }
+
+            for (const fileName in options.newFileContents) {
+                assert(editInfo.edits.some(e => e.fileName === fileName));
+            }
         }
 
         public verifyFileAfterApplyingRefactorAtMarker(
@@ -3333,6 +3366,10 @@ Actual: ${stringify(fullActual)}`);
                 this.verifyCurrentFileContent(options.newFileContents[fileName]);
             }
         }
+
+        private getApplicableRefactors(positionOrRange: number | ts.TextRange, preferences = ts.defaultPreferences): ReadonlyArray<ts.ApplicableRefactorInfo> {
+            return this.languageService.getApplicableRefactors(this.activeFile.fileName, positionOrRange, preferences) || ts.emptyArray;
+        }
     }
 
     export function runFourSlashTest(basePath: string, testType: FourSlashTestType, fileName: string) {
@@ -4430,6 +4467,13 @@ namespace FourSlashInterface {
         public getEditsForFileRename(options: GetEditsForFileRenameOptions) {
             this.state.getEditsForFileRename(options);
         }
+
+        public moveToNewFile(options: MoveToNewFileOptions): void {
+            this.state.moveToNewFile(options);
+        }
+        public noMoveToNewFile(): void {
+            this.state.noMoveToNewFile();
+        }
     }
 
     export class Edit {
@@ -4803,4 +4847,8 @@ namespace FourSlashInterface {
         readonly newPath: string;
         readonly newFileContents: { readonly [fileName: string]: string };
     }
+
+    export interface MoveToNewFileOptions {
+        readonly newFileContents: { readonly [fileName: string]: string };
+    }
 }
diff --git a/src/harness/tsconfig.json b/src/harness/tsconfig.json
@@ -116,6 +116,7 @@
         "../services/codefixes/useDefaultImport.ts",
         "../services/refactors/extractSymbol.ts",
         "../services/refactors/generateGetAccessorAndSetAccessor.ts",
+        "../services/refactors/moveToNewFile.ts",
         "../services/sourcemaps.ts",
         "../services/services.ts",
         "../services/breakpoints.ts",

diff --git a/src/harness/unittests/paths.ts b/src/harness/unittests/paths.ts
@@ -268,25 +268,25 @@ describe("core paths", () => {
         assert.strictEqual(ts.resolvePath("a", "b", "../c"), "a/c");
     });
     it("getPathRelativeTo", () => {
-        assert.strictEqual(ts.getRelativePath("/", "/", /*ignoreCase*/ false), "");
-        assert.strictEqual(ts.getRelativePath("/a", "/a", /*ignoreCase*/ false), "");
-        assert.strictEqual(ts.getRelativePath("/a/", "/a", /*ignoreCase*/ false), "");
-        assert.strictEqual(ts.getRelativePath("/a", "/", /*ignoreCase*/ false), "..");
-        assert.strictEqual(ts.getRelativePath("/a", "/b", /*ignoreCase*/ false), "../b");
-        assert.strictEqual(ts.getRelativePath("/a/b", "/b", /*ignoreCase*/ false), "../../b");
-        assert.strictEqual(ts.getRelativePath("/a/b/c", "/b", /*ignoreCase*/ false), "../../../b");
-        assert.strictEqual(ts.getRelativePath("/a/b/c", "/b/c", /*ignoreCase*/ false), "../../../b/c");
-        assert.strictEqual(ts.getRelativePath("/a/b/c", "/a/b", /*ignoreCase*/ false), "..");
-        assert.strictEqual(ts.getRelativePath("c:", "d:", /*ignoreCase*/ false), "d:/");
-        assert.strictEqual(ts.getRelativePath("file:///", "file:///", /*ignoreCase*/ false), "");
-        assert.strictEqual(ts.getRelativePath("file:///a", "file:///a", /*ignoreCase*/ false), "");
-        assert.strictEqual(ts.getRelativePath("file:///a/", "file:///a", /*ignoreCase*/ false), "");
-        assert.strictEqual(ts.getRelativePath("file:///a", "file:///", /*ignoreCase*/ false), "..");
-        assert.strictEqual(ts.getRelativePath("file:///a", "file:///b", /*ignoreCase*/ false), "../b");
-        assert.strictEqual(ts.getRelativePath("file:///a/b", "file:///b", /*ignoreCase*/ false), "../../b");
-        assert.strictEqual(ts.getRelativePath("file:///a/b/c", "file:///b", /*ignoreCase*/ false), "../../../b");
-        assert.strictEqual(ts.getRelativePath("file:///a/b/c", "file:///b/c", /*ignoreCase*/ false), "../../../b/c");
-        assert.strictEqual(ts.getRelativePath("file:///a/b/c", "file:///a/b", /*ignoreCase*/ false), "..");
-        assert.strictEqual(ts.getRelativePath("file:///c:", "file:///d:", /*ignoreCase*/ false), "file:///d:/");
+        assert.strictEqual(ts.getRelativePathFromDirectory("/", "/", /*ignoreCase*/ false), "");
+        assert.strictEqual(ts.getRelativePathFromDirectory("/a", "/a", /*ignoreCase*/ false), "");
+        assert.strictEqual(ts.getRelativePathFromDirectory("/a/", "/a", /*ignoreCase*/ false), "");
+        assert.strictEqual(ts.getRelativePathFromDirectory("/a", "/", /*ignoreCase*/ false), "..");
+        assert.strictEqual(ts.getRelativePathFromDirectory("/a", "/b", /*ignoreCase*/ false), "../b");
+        assert.strictEqual(ts.getRelativePathFromDirectory("/a/b", "/b", /*ignoreCase*/ false), "../../b");
+        assert.strictEqual(ts.getRelativePathFromDirectory("/a/b/c", "/b", /*ignoreCase*/ false), "../../../b");
+        assert.strictEqual(ts.getRelativePathFromDirectory("/a/b/c", "/b/c", /*ignoreCase*/ false), "../../../b/c");
+        assert.strictEqual(ts.getRelativePathFromDirectory("/a/b/c", "/a/b", /*ignoreCase*/ false), "..");
+        assert.strictEqual(ts.getRelativePathFromDirectory("c:", "d:", /*ignoreCase*/ false), "d:/");
+        assert.strictEqual(ts.getRelativePathFromDirectory("file:///", "file:///", /*ignoreCase*/ false), "");
+        assert.strictEqual(ts.getRelativePathFromDirectory("file:///a", "file:///a", /*ignoreCase*/ false), "");
+        assert.strictEqual(ts.getRelativePathFromDirectory("file:///a/", "file:///a", /*ignoreCase*/ false), "");
+        assert.strictEqual(ts.getRelativePathFromDirectory("file:///a", "file:///", /*ignoreCase*/ false), "..");
+        assert.strictEqual(ts.getRelativePathFromDirectory("file:///a", "file:///b", /*ignoreCase*/ false), "../b");
+        assert.strictEqual(ts.getRelativePathFromDirectory("file:///a/b", "file:///b", /*ignoreCase*/ false), "../../b");
+        assert.strictEqual(ts.getRelativePathFromDirectory("file:///a/b/c", "file:///b", /*ignoreCase*/ false), "../../../b");
+        assert.strictEqual(ts.getRelativePathFromDirectory("file:///a/b/c", "file:///b/c", /*ignoreCase*/ false), "../../../b/c");
+        assert.strictEqual(ts.getRelativePathFromDirectory("file:///a/b/c", "file:///a/b", /*ignoreCase*/ false), "..");
+        assert.strictEqual(ts.getRelativePathFromDirectory("file:///c:", "file:///d:", /*ignoreCase*/ false), "file:///d:/");
     });
 });
diff --git a/src/harness/vpath.ts b/src/harness/vpath.ts
@@ -18,7 +18,7 @@ namespace vpath {
     export import dirname = ts.getDirectoryPath;
     export import basename = ts.getBaseFileName;
     export import extname = ts.getAnyExtensionFromPath;
-    export import relative = ts.getRelativePath;
+    export import relative = ts.getRelativePathFromDirectory;
     export import beneath = ts.containsPath;
     export import changeExtension = ts.changeAnyExtension;
     export import isTypeScript = ts.hasTypeScriptFileExtension;

diff --git a/src/server/tsconfig.json b/src/server/tsconfig.json
@@ -112,6 +112,7 @@
         "../services/codefixes/useDefaultImport.ts",
         "../services/refactors/extractSymbol.ts",
         "../services/refactors/generateGetAccessorAndSetAccessor.ts",
+        "../services/refactors/moveToNewFile.ts",
         "../services/sourcemaps.ts",
         "../services/services.ts",
         "../services/breakpoints.ts",

diff --git a/src/server/tsconfig.library.json b/src/server/tsconfig.library.json
@@ -118,6 +118,7 @@
         "../services/codefixes/useDefaultImport.ts",
         "../services/refactors/extractSymbol.ts",
         "../services/refactors/generateGetAccessorAndSetAccessor.ts",
+        "../services/refactors/moveToNewFile.ts",
         "../services/sourcemaps.ts",
         "../services/services.ts",
         "../services/breakpoints.ts",

diff --git a/src/services/breakpoints.ts b/src/services/breakpoints.ts
@@ -41,7 +41,7 @@ namespace ts.BreakpointResolver {
         }
 
         function textSpanEndingAtNextToken(startNode: Node, previousTokenToFindNextEndToken: Node): TextSpan {
-            return textSpan(startNode, findNextToken(previousTokenToFindNextEndToken, previousTokenToFindNextEndToken.parent));
+            return textSpan(startNode, findNextToken(previousTokenToFindNextEndToken, previousTokenToFindNextEndToken.parent, sourceFile));
         }
 
         function spanInNodeIfStartsOnSameLine(node: Node, otherwiseOnNode?: Node): TextSpan {
@@ -60,7 +60,7 @@ namespace ts.BreakpointResolver {
         }
 
         function spanInNextNode(node: Node): TextSpan {
-            return spanInNode(findNextToken(node, node.parent));
+            return spanInNode(findNextToken(node, node.parent, sourceFile));
         }
 
         function spanInNode(node: Node): TextSpan {

diff --git a/src/services/codefixes/convertToEs6Module.ts b/src/services/codefixes/convertToEs6Module.ts
@@ -487,15 +487,6 @@ namespace ts.codefix {
             : makeImport(/*name*/ undefined, [makeImportSpecifier(propertyName, localName)], moduleSpecifier);
     }
 
-    function makeImport(name: Identifier | undefined, namedImports: ReadonlyArray<ImportSpecifier> | undefined, moduleSpecifier: StringLiteralLike): ImportDeclaration {
-        return makeImportDeclaration(name, namedImports, moduleSpecifier);
-    }
-
-    export function makeImportDeclaration(name: Identifier, namedImports: ReadonlyArray<ImportSpecifier> | undefined, moduleSpecifier: Expression) {
-        const importClause = (name || namedImports) && createImportClause(name, namedImports && createNamedImports(namedImports));
-        return createImportDeclaration(/*decorators*/ undefined, /*modifiers*/ undefined, importClause, moduleSpecifier);
-    }
-
     function makeImportSpecifier(propertyName: string | undefined, name: string): ImportSpecifier {
         return createImportSpecifier(propertyName !== undefined && propertyName !== name ? createIdentifier(propertyName) : undefined, createIdentifier(name));
     }