getEditsForFileRename: Test both before and after the rename

Author: Andy Hanson

src/compiler/moduleSpecifiers.ts

@@ -2,13 +2,23 @@
 /* @internal */
 namespace ts.moduleSpecifiers {
     export interface ModuleSpecifierPreferences {
-        importModuleSpecifierPreference?: "relative" | "non-relative";
+        readonly importModuleSpecifierPreference?: "relative" | "non-relative";
     }
 
-    // Note: fromSourceFile is just for usesJsExtensionOnImports
-    export function getModuleSpecifier(compilerOptions: CompilerOptions, fromSourceFile: SourceFile, fromSourceFileName: string, toFileName: string, host: ModuleSpecifierResolutionHost, preferences: ModuleSpecifierPreferences = {}) {
-        const info = getInfo(compilerOptions, fromSourceFile, fromSourceFileName, host);
-        return getGlobalModuleSpecifier(toFileName, info, host, compilerOptions) ||
+    // Note: importingSourceFile is just for usesJsExtensionOnImports
+    export function getModuleSpecifier(
+        compilerOptions: CompilerOptions,
+        importingSourceFile: SourceFile,
+        importingSourceFileName: string,
+        toFileName: string,
+        host: ModuleSpecifierResolutionHost,
+        files: ReadonlyArray<SourceFile>,
+        preferences: ModuleSpecifierPreferences = {},
+    ): string {
+        const info = getInfo(compilerOptions, importingSourceFile, importingSourceFileName, host);
+        const modulePaths = getAllModulePaths(files, toFileName, info.getCanonicalFileName, host);
+        return firstDefined(modulePaths, moduleFileName => getGlobalModuleSpecifier(moduleFileName, info, host, compilerOptions)) ||
+            getGlobalModuleSpecifier(toFileName, info, host, compilerOptions) ||
             first(getLocalModuleSpecifiers(toFileName, info, compilerOptions, preferences));
     }
 
@@ -28,7 +38,7 @@ namespace ts.moduleSpecifiers {
         if (!files) {
             return Debug.fail("Files list must be present to resolve symlinks in specifier resolution");
         }
-        const modulePaths = getAllModulePaths(files, getSourceFileOfNode(moduleSymbol.valueDeclaration), info.getCanonicalFileName, host);
+        const modulePaths = getAllModulePaths(files, getSourceFileOfNode(moduleSymbol.valueDeclaration).fileName, info.getCanonicalFileName, host);
 
         const global = mapDefined(modulePaths, moduleFileName => getGlobalModuleSpecifier(moduleFileName, info, host, compilerOptions));
         return global.length ? global.map(g => [g]) : modulePaths.map(moduleFileName =>
@@ -177,14 +187,14 @@ namespace ts.moduleSpecifiers {
     }
 
     /**
-     * Looks for a existing imports that use symlinks to this module.
+     * Looks for existing imports that use symlinks to this module.
      * Only if no symlink is available, the real path will be used.
      */
-    function getAllModulePaths(files: ReadonlyArray<SourceFile>, { fileName }: SourceFile, getCanonicalFileName: (file: string) => string, host: ModuleSpecifierResolutionHost): ReadonlyArray<string> {
+    function getAllModulePaths(files: ReadonlyArray<SourceFile>, importedFileName: string, getCanonicalFileName: (file: string) => string, host: ModuleSpecifierResolutionHost): ReadonlyArray<string> {
         const symlinks = mapDefined(files, sf =>
             sf.resolvedModules && firstDefinedIterator(sf.resolvedModules.values(), res =>
-                res && res.resolvedFileName === fileName ? res.originalPath : undefined));
-        return symlinks.length === 0 ? getAllModulePathsUsingIndirectSymlinks(files, getNormalizedAbsolutePath(fileName, host.getCurrentDirectory ? host.getCurrentDirectory() : ""), getCanonicalFileName, host) : symlinks;
+                res && res.resolvedFileName === importedFileName ? res.originalPath : undefined));
+        return symlinks.length === 0 ? getAllModulePathsUsingIndirectSymlinks(files, getNormalizedAbsolutePath(importedFileName, host.getCurrentDirectory ? host.getCurrentDirectory() : ""), getCanonicalFileName, host) : symlinks;
     }
 
     function getRelativePathNParents(relativePath: string): number {

src/harness/fourslash.ts

@@ -391,8 +391,11 @@ namespace FourSlash {
         }
 
         private getFileContent(fileName: string): string {
-            const script = this.languageServiceAdapterHost.getScriptInfo(fileName)!;
-            return script.content;
+            return ts.Debug.assertDefined(this.tryGetFileContent(fileName));
+        }
+        private tryGetFileContent(fileName: string): string | undefined {
+            const script = this.languageServiceAdapterHost.getScriptInfo(fileName);
+            return script && script.content;
         }
 
         // Entry points from fourslash.ts
@@ -1956,7 +1959,7 @@ Actual: ${stringify(fullActual)}`);
          * @returns The number of characters added to the file as a result of the edits.
          * May be negative.
          */
-        private applyEdits(fileName: string, edits: ts.TextChange[], isFormattingEdit: boolean): number {
+        private applyEdits(fileName: string, edits: ReadonlyArray<ts.TextChange>, isFormattingEdit: boolean): number {
             // We get back a set of edits, but langSvc.editScript only accepts one at a time. Use this to keep track
             // of the incremental offset from each edit to the next. We assume these edit ranges don't overlap
 
@@ -3134,30 +3137,34 @@ Actual: ${stringify(fullActual)}`);
             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, options.preferences || ts.defaultPreferences)!;
-            this.testNewFileContents(editInfo.edits, options.newFileContents);
+            this.testNewFileContents(editInfo.edits, options.newFileContents, "move to new file");
         }
 
-        private testNewFileContents(edits: ReadonlyArray<ts.FileTextChanges>, newFileContents: { [fileName: string]: string }): void {
-            for (const edit of edits) {
-                const newContent = newFileContents[edit.fileName];
+        private testNewFileContents(edits: ReadonlyArray<ts.FileTextChanges>, newFileContents: { [fileName: string]: string }, description: string): void {
+            for (const { fileName, textChanges } of edits) {
+                const newContent = newFileContents[fileName];
                 if (newContent === undefined) {
-                    this.raiseError(`There was an edit in ${edit.fileName} but new content was not specified.`);
+                    this.raiseError(`${description} - There was an edit in ${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);
+
+                const fileContent = this.tryGetFileContent(fileName);
+                if (fileContent !== undefined) {
+                    const actualNewContent = ts.textChanges.applyChanges(fileContent, textChanges);
+                    assert.equal(actualNewContent, newContent, `new content for ${fileName}`);
                 }
                 else {
-                    assert(edit.textChanges.length === 1);
-                    const change = ts.first(edit.textChanges);
+                    // Creates a new file.
+                    assert(textChanges.length === 1);
+                    const change = ts.first(textChanges);
                     assert.deepEqual(change.span, ts.createTextSpan(0, 0));
-                    assert.equal(change.newText, newContent, `Content for ${edit.fileName}`);
+                    assert.equal(change.newText, newContent, `${description} - Content for ${fileName}`);
                 }
             }
 
             for (const fileName in newFileContents) {
-                assert(edits.some(e => e.fileName === fileName));
+                if (!edits.some(e => e.fileName === fileName)) {
+                    ts.Debug.fail(`${description} - Asserted new contents of ${fileName} but there were no edits`);
+                }
             }
         }
 
@@ -3292,7 +3299,7 @@ Actual: ${stringify(fullActual)}`);
             eq(item.replacementSpan, options && options.replacementSpan && ts.createTextSpanFromRange(options.replacementSpan), "replacementSpan");
         }
 
-        private findFile(indexOrName: string | number) {
+        private findFile(indexOrName: string | number): FourSlashFile {
             if (typeof indexOrName === "number") {
                 const index = indexOrName;
                 if (index >= this.testData.files.length) {
@@ -3303,32 +3310,39 @@ Actual: ${stringify(fullActual)}`);
                 }
             }
             else if (ts.isString(indexOrName)) {
-                let name = ts.normalizePath(indexOrName);
-
-                // names are stored in the compiler with this relative path, this allows people to use goTo.file on just the fileName
-                name = name.indexOf("/") === -1 ? (this.basePath + "/" + name) : name;
-
-                const availableNames: string[] = [];
-                const result = ts.forEach(this.testData.files, file => {
-                    const fn = ts.normalizePath(file.fileName);
-                    if (fn) {
-                        if (fn === name) {
-                            return file;
-                        }
-                        availableNames.push(fn);
-                    }
-                });
-
-                if (!result) {
-                    throw new Error(`No test file named "${name}" exists. Available file names are: ${availableNames.join(", ")}`);
+                const { file, availableNames } = this.tryFindFileWorker(indexOrName);
+                if (!file) {
+                    throw new Error(`No test file named "${indexOrName}" exists. Available file names are: ${availableNames.join(", ")}`);
                 }
-                return result;
+                return file;
             }
             else {
                 return ts.Debug.assertNever(indexOrName);
             }
         }
 
+        private tryFindFileWorker(name: string): { readonly file: FourSlashFile | undefined; readonly availableNames: ReadonlyArray<string>; } {
+            name = ts.normalizePath(name);
+            // names are stored in the compiler with this relative path, this allows people to use goTo.file on just the fileName
+            name = name.indexOf("/") === -1 ? (this.basePath + "/" + name) : name;
+
+            const availableNames: string[] = [];
+            const file = ts.forEach(this.testData.files, file => {
+                const fn = ts.normalizePath(file.fileName);
+                if (fn) {
+                    if (fn === name) {
+                        return file;
+                    }
+                    availableNames.push(fn);
+                }
+            });
+            return { file, availableNames };
+        }
+
+        private hasFile(name: string): boolean {
+            return this.tryFindFileWorker(name).file !== undefined;
+        }
+
         private getLineColStringAtPosition(position: number) {
             const pos = this.languageServiceAdapterHost.positionToLineAndCharacter(this.activeFile.fileName, position);
             return `line ${(pos.line + 1)}, col ${pos.character}`;
@@ -3366,16 +3380,35 @@ Actual: ${stringify(fullActual)}`);
             return !!a && !!b && a.start === b.start && a.length === b.length;
         }
 
-        public getEditsForFileRename(options: FourSlashInterface.GetEditsForFileRenameOptions): void {
-            const changes = this.languageService.getEditsForFileRename(options.oldPath, options.newPath, this.formatCodeSettings, ts.defaultPreferences);
-            this.testNewFileContents(changes, options.newFileContents);
+        public getEditsForFileRename({ oldPath, newPath, newFileContents }: FourSlashInterface.GetEditsForFileRenameOptions): void {
+            const test = (fileContents: { readonly [fileName: string]: string }, description: string): void => {
+                const changes = this.languageService.getEditsForFileRename(oldPath, newPath, this.formatCodeSettings, ts.defaultPreferences);
+                this.testNewFileContents(changes, fileContents, description);
+            };
+
+            ts.Debug.assert(!this.hasFile(newPath), "initially, newPath should not exist");
+
+            test(newFileContents, "with file not yet moved");
+
+            this.languageServiceAdapterHost.renameFileOrDirectory(oldPath, newPath);
+            this.languageService.cleanupSemanticCache();
+            const pathUpdater = ts.getPathUpdater(oldPath, newPath, ts.createGetCanonicalFileName(/*useCaseSensitiveFileNames*/ false));
+            test(renameKeys(newFileContents, key => pathUpdater(key) || key), "with file moved");
         }
 
         private getApplicableRefactors(positionOrRange: number | ts.TextRange, preferences = ts.defaultPreferences): ReadonlyArray<ts.ApplicableRefactorInfo> {
             return this.languageService.getApplicableRefactors(this.activeFile.fileName, positionOrRange, preferences) || ts.emptyArray;
         }
     }
 
+    function renameKeys<T>(obj: { readonly [key: string]: T }, renameKey: (key: string) => string): { readonly [key: string]: T } {
+        const res: { [key: string]: T } = {};
+        for (const key in obj) {
+            res[renameKey(key)] = obj[key];
+        }
+        return res;
+    }
+
     export function runFourSlashTest(basePath: string, testType: FourSlashTestType, fileName: string) {
         const content = Harness.IO.readFile(fileName)!;
         runFourSlashTestContent(basePath, testType, content, fileName);

src/harness/harnessLanguageService.ts

@@ -151,6 +151,21 @@ namespace Harness.LanguageService {
             this.scriptInfos.set(vpath.resolve(this.vfs.cwd(), fileName), new ScriptInfo(fileName, content, isRootFile));
         }
 
+        public renameFileOrDirectory(oldPath: string, newPath: string): void {
+            this.vfs.mkdirpSync(ts.getDirectoryPath(newPath));
+            this.vfs.renameSync(oldPath, newPath);
+
+            const updater = ts.getPathUpdater(oldPath, newPath, ts.createGetCanonicalFileName(/*useCaseSensitiveFileNames*/ false));
+            this.scriptInfos.forEach((scriptInfo, key) => {
+                const newFileName = updater(key);
+                if (newFileName !== undefined) {
+                    this.scriptInfos.delete(key);
+                    this.scriptInfos.set(newFileName, scriptInfo);
+                    scriptInfo.fileName = newFileName;
+                }
+            });
+        }
+
         public editScript(fileName: string, start: number, end: number, newText: string) {
             const script = this.getScriptInfo(fileName);
             if (script) {

src/server/session.ts

@@ -129,13 +129,8 @@ namespace ts.server {
         project: Project;
     }
 
-    function allEditsBeforePos(edits: TextChange[], pos: number) {
-        for (const edit of edits) {
-            if (textSpanEnd(edit.span) >= pos) {
-                return false;
-            }
-        }
-        return true;
+    function allEditsBeforePos(edits: ReadonlyArray<TextChange>, pos: number): boolean {
+        return edits.every(edit => textSpanEnd(edit.span) < pos);
     }
 
     // CommandNames used to be exposed before TS 2.4 as a namespace

src/services/getEditsForFileRename.ts

@@ -13,7 +13,8 @@ namespace ts {
 
     /** If 'path' refers to an old directory, returns path in the new directory. */
     type PathUpdater = (path: string) => string | undefined;
-    function getPathUpdater(oldFileOrDirPath: string, newFileOrDirPath: string, getCanonicalFileName: GetCanonicalFileName): PathUpdater {
+    // exported for tests
+    export function getPathUpdater(oldFileOrDirPath: string, newFileOrDirPath: string, getCanonicalFileName: GetCanonicalFileName): PathUpdater {
         const canonicalOldPath = getCanonicalFileName(oldFileOrDirPath);
         return path => {
             const canonicalPath = getCanonicalFileName(path);
@@ -102,7 +103,8 @@ namespace ts {
         getCanonicalFileName: GetCanonicalFileName,
         preferences: UserPreferences,
     ): void {
-        for (const sourceFile of program.getSourceFiles()) {
+        const allFiles = program.getSourceFiles();
+        for (const sourceFile of allFiles) {
             const newFromOld = oldToNew(sourceFile.fileName);
             const newImportFromPath = newFromOld !== undefined ? newFromOld : sourceFile.fileName;
             const newImportFromDirectory = getDirectoryPath(newImportFromPath);
@@ -121,15 +123,19 @@ namespace ts {
                     return newAbsolute === undefined ? undefined : ensurePathIsNonModuleName(getRelativePathFromDirectory(newImportFromDirectory, newAbsolute, getCanonicalFileName));
                 },
                 importLiteral => {
+                    const importedModuleSymbol = program.getTypeChecker().getSymbolAtLocation(importLiteral);
+                    // No need to update if it's an ambient module^M
+                    if (importedModuleSymbol && importedModuleSymbol.declarations.some(d => isAmbientModule(d))) return undefined;
+
                     const toImport = oldFromNew !== undefined
                         // If we're at the new location (file was already renamed), need to redo module resolution starting from the old location.
                         // TODO:GH#18217
                         ? getSourceFileToImportFromResolved(resolveModuleName(importLiteral.text, oldImportFromPath, program.getCompilerOptions(), host as ModuleResolutionHost), oldToNew, program)
-                        : getSourceFileToImport(importLiteral, sourceFile, program, host, oldToNew);
+                        : getSourceFileToImport(importedModuleSymbol, importLiteral, sourceFile, program, host, oldToNew);
                     // If neither the importing source file nor the imported file moved, do nothing.
                     return toImport === undefined || !toImport.updated && !importingSourceFileMoved
                         ? undefined
-                        : moduleSpecifiers.getModuleSpecifier(program.getCompilerOptions(), sourceFile, newImportFromPath, toImport.newFileName, host, preferences);
+                        : moduleSpecifiers.getModuleSpecifier(program.getCompilerOptions(), sourceFile, newImportFromPath, toImport.newFileName, host, allFiles, preferences);
                 });
         }
     }
@@ -146,11 +152,17 @@ namespace ts {
         /** True if the imported file was renamed. */
         readonly updated: boolean;
     }
-    function getSourceFileToImport(importLiteral: StringLiteralLike, importingSourceFile: SourceFile, program: Program, host: LanguageServiceHost, oldToNew: PathUpdater): ToImport | undefined {
-        const symbol = program.getTypeChecker().getSymbolAtLocation(importLiteral);
-        if (symbol) {
-            if (symbol.declarations.some(d => isAmbientModule(d))) return undefined; // No need to update if it's an ambient module
-            const oldFileName = find(symbol.declarations, isSourceFile)!.fileName;
+    function getSourceFileToImport(
+        importedModuleSymbol: Symbol | undefined,
+        importLiteral: StringLiteralLike,
+        importingSourceFile: SourceFile,
+        program: Program,
+        host: LanguageServiceHost,
+        oldToNew: PathUpdater,
+    ): ToImport | undefined {
+        if (importedModuleSymbol) {
+            // `find` should succeed because we checked for ambient modules before calling this function.
+            const oldFileName = find(importedModuleSymbol.declarations, isSourceFile)!.fileName;
             const newFileName = oldToNew(oldFileName);
             return newFileName === undefined ? { newFileName: oldFileName, updated: false } : { newFileName, updated: true };
         }