Skip to content

Commit ab9e913

Browse files
author
Andy
authored
getEditsForFileRename: Test both before and after the rename (microsoft#25074)
1 parent 13bc46d commit ab9e913

20 files changed

+206
-123
lines changed

‎src/compiler/moduleSpecifiers.ts‎

Lines changed: 20 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,23 @@
22
/* @internal */
33
namespace ts.moduleSpecifiers {
44
export interface ModuleSpecifierPreferences {
5-
importModuleSpecifierPreference?: "relative" | "non-relative";
5+
readonly importModuleSpecifierPreference?: "relative" | "non-relative";
66
}
77

8-
// Note: fromSourceFile is just for usesJsExtensionOnImports
9-
export function getModuleSpecifier(compilerOptions: CompilerOptions, fromSourceFile: SourceFile, fromSourceFileName: string, toFileName: string, host: ModuleSpecifierResolutionHost, preferences: ModuleSpecifierPreferences = {}) {
10-
const info = getInfo(compilerOptions, fromSourceFile, fromSourceFileName, host);
11-
return getGlobalModuleSpecifier(toFileName, info, host, compilerOptions) ||
8+
// Note: importingSourceFile is just for usesJsExtensionOnImports
9+
export function getModuleSpecifier(
10+
compilerOptions: CompilerOptions,
11+
importingSourceFile: SourceFile,
12+
importingSourceFileName: string,
13+
toFileName: string,
14+
host: ModuleSpecifierResolutionHost,
15+
files: ReadonlyArray<SourceFile>,
16+
preferences: ModuleSpecifierPreferences = {},
17+
): string {
18+
const info = getInfo(compilerOptions, importingSourceFile, importingSourceFileName, host);
19+
const modulePaths = getAllModulePaths(files, toFileName, info.getCanonicalFileName, host);
20+
return firstDefined(modulePaths, moduleFileName => getGlobalModuleSpecifier(moduleFileName, info, host, compilerOptions)) ||
21+
getGlobalModuleSpecifier(toFileName, info, host, compilerOptions) ||
1222
first(getLocalModuleSpecifiers(toFileName, info, compilerOptions, preferences));
1323
}
1424

@@ -28,7 +38,7 @@ namespace ts.moduleSpecifiers {
2838
if (!files) {
2939
return Debug.fail("Files list must be present to resolve symlinks in specifier resolution");
3040
}
31-
const modulePaths = getAllModulePaths(files, getSourceFileOfNode(moduleSymbol.valueDeclaration), info.getCanonicalFileName, host);
41+
const modulePaths = getAllModulePaths(files, getSourceFileOfNode(moduleSymbol.valueDeclaration).fileName, info.getCanonicalFileName, host);
3242

3343
const global = mapDefined(modulePaths, moduleFileName => getGlobalModuleSpecifier(moduleFileName, info, host, compilerOptions));
3444
return global.length ? global.map(g => [g]) : modulePaths.map(moduleFileName =>
@@ -177,14 +187,14 @@ namespace ts.moduleSpecifiers {
177187
}
178188

179189
/**
180-
* Looks for a existing imports that use symlinks to this module.
190+
* Looks for existing imports that use symlinks to this module.
181191
* Only if no symlink is available, the real path will be used.
182192
*/
183-
function getAllModulePaths(files: ReadonlyArray<SourceFile>, { fileName }: SourceFile, getCanonicalFileName: (file: string) => string, host: ModuleSpecifierResolutionHost): ReadonlyArray<string> {
193+
function getAllModulePaths(files: ReadonlyArray<SourceFile>, importedFileName: string, getCanonicalFileName: (file: string) => string, host: ModuleSpecifierResolutionHost): ReadonlyArray<string> {
184194
const symlinks = mapDefined(files, sf =>
185195
sf.resolvedModules && firstDefinedIterator(sf.resolvedModules.values(), res =>
186-
res && res.resolvedFileName === fileName ? res.originalPath : undefined));
187-
return symlinks.length === 0 ? getAllModulePathsUsingIndirectSymlinks(files, getNormalizedAbsolutePath(fileName, host.getCurrentDirectory ? host.getCurrentDirectory() : ""), getCanonicalFileName, host) : symlinks;
196+
res && res.resolvedFileName === importedFileName ? res.originalPath : undefined));
197+
return symlinks.length === 0 ? getAllModulePathsUsingIndirectSymlinks(files, getNormalizedAbsolutePath(importedFileName, host.getCurrentDirectory ? host.getCurrentDirectory() : ""), getCanonicalFileName, host) : symlinks;
188198
}
189199

190200
function getRelativePathNParents(relativePath: string): number {

‎src/harness/fourslash.ts‎

Lines changed: 72 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -391,8 +391,11 @@ namespace FourSlash {
391391
}
392392

393393
private getFileContent(fileName: string): string {
394-
const script = this.languageServiceAdapterHost.getScriptInfo(fileName)!;
395-
return script.content;
394+
return ts.Debug.assertDefined(this.tryGetFileContent(fileName));
395+
}
396+
private tryGetFileContent(fileName: string): string | undefined {
397+
const script = this.languageServiceAdapterHost.getScriptInfo(fileName);
398+
return script && script.content;
396399
}
397400

398401
// Entry points from fourslash.ts
@@ -1935,7 +1938,7 @@ Actual: ${stringify(fullActual)}`);
19351938
* @returns The number of characters added to the file as a result of the edits.
19361939
* May be negative.
19371940
*/
1938-
private applyEdits(fileName: string, edits: ts.TextChange[], isFormattingEdit: boolean): number {
1941+
private applyEdits(fileName: string, edits: ReadonlyArray<ts.TextChange>, isFormattingEdit: boolean): number {
19391942
// We get back a set of edits, but langSvc.editScript only accepts one at a time. Use this to keep track
19401943
// of the incremental offset from each edit to the next. We assume these edit ranges don't overlap
19411944

@@ -3129,30 +3132,34 @@ Actual: ${stringify(fullActual)}`);
31293132
assert(action.name === "Move to a new file" && action.description === "Move to a new file");
31303133

31313134
const editInfo = this.languageService.getEditsForRefactor(this.activeFile.fileName, this.formatCodeSettings, range, refactor.name, action.name, options.preferences || ts.defaultPreferences)!;
3132-
this.testNewFileContents(editInfo.edits, options.newFileContents);
3135+
this.testNewFileContents(editInfo.edits, options.newFileContents, "move to new file");
31333136
}
31343137

3135-
private testNewFileContents(edits: ReadonlyArray<ts.FileTextChanges>, newFileContents: { [fileName: string]: string }): void {
3136-
for (const edit of edits) {
3137-
const newContent = newFileContents[edit.fileName];
3138+
private testNewFileContents(edits: ReadonlyArray<ts.FileTextChanges>, newFileContents: { [fileName: string]: string }, description: string): void {
3139+
for (const { fileName, textChanges } of edits) {
3140+
const newContent = newFileContents[fileName];
31383141
if (newContent === undefined) {
3139-
this.raiseError(`There was an edit in ${edit.fileName} but new content was not specified.`);
3142+
this.raiseError(`${description} - There was an edit in ${fileName} but new content was not specified.`);
31403143
}
3141-
if (this.testData.files.some(f => f.fileName === edit.fileName)) {
3142-
this.applyEdits(edit.fileName, edit.textChanges, /*isFormattingEdit*/ false);
3143-
this.openFile(edit.fileName);
3144-
this.verifyCurrentFileContent(newContent);
3144+
3145+
const fileContent = this.tryGetFileContent(fileName);
3146+
if (fileContent !== undefined) {
3147+
const actualNewContent = ts.textChanges.applyChanges(fileContent, textChanges);
3148+
assert.equal(actualNewContent, newContent, `new content for ${fileName}`);
31453149
}
31463150
else {
3147-
assert(edit.textChanges.length === 1);
3148-
const change = ts.first(edit.textChanges);
3151+
// Creates a new file.
3152+
assert(textChanges.length === 1);
3153+
const change = ts.first(textChanges);
31493154
assert.deepEqual(change.span, ts.createTextSpan(0, 0));
3150-
assert.equal(change.newText, newContent, `Content for ${edit.fileName}`);
3155+
assert.equal(change.newText, newContent, `${description} - Content for ${fileName}`);
31513156
}
31523157
}
31533158

31543159
for (const fileName in newFileContents) {
3155-
assert(edits.some(e => e.fileName === fileName));
3160+
if (!edits.some(e => e.fileName === fileName)) {
3161+
ts.Debug.fail(`${description} - Asserted new contents of ${fileName} but there were no edits`);
3162+
}
31563163
}
31573164
}
31583165

@@ -3287,7 +3294,7 @@ Actual: ${stringify(fullActual)}`);
32873294
eq(item.replacementSpan, options && options.replacementSpan && ts.createTextSpanFromRange(options.replacementSpan), "replacementSpan");
32883295
}
32893296

3290-
private findFile(indexOrName: string | number) {
3297+
private findFile(indexOrName: string | number): FourSlashFile {
32913298
if (typeof indexOrName === "number") {
32923299
const index = indexOrName;
32933300
if (index >= this.testData.files.length) {
@@ -3298,32 +3305,39 @@ Actual: ${stringify(fullActual)}`);
32983305
}
32993306
}
33003307
else if (ts.isString(indexOrName)) {
3301-
let name = ts.normalizePath(indexOrName);
3302-
3303-
// names are stored in the compiler with this relative path, this allows people to use goTo.file on just the fileName
3304-
name = name.indexOf("/") === -1 ? (this.basePath + "/" + name) : name;
3305-
3306-
const availableNames: string[] = [];
3307-
const result = ts.forEach(this.testData.files, file => {
3308-
const fn = ts.normalizePath(file.fileName);
3309-
if (fn) {
3310-
if (fn === name) {
3311-
return file;
3312-
}
3313-
availableNames.push(fn);
3314-
}
3315-
});
3316-
3317-
if (!result) {
3318-
throw new Error(`No test file named "${name}" exists. Available file names are: ${availableNames.join(", ")}`);
3308+
const { file, availableNames } = this.tryFindFileWorker(indexOrName);
3309+
if (!file) {
3310+
throw new Error(`No test file named "${indexOrName}" exists. Available file names are: ${availableNames.join(", ")}`);
33193311
}
3320-
return result;
3312+
return file;
33213313
}
33223314
else {
33233315
return ts.Debug.assertNever(indexOrName);
33243316
}
33253317
}
33263318

3319+
private tryFindFileWorker(name: string): { readonly file: FourSlashFile | undefined; readonly availableNames: ReadonlyArray<string>; } {
3320+
name = ts.normalizePath(name);
3321+
// names are stored in the compiler with this relative path, this allows people to use goTo.file on just the fileName
3322+
name = name.indexOf("/") === -1 ? (this.basePath + "/" + name) : name;
3323+
3324+
const availableNames: string[] = [];
3325+
const file = ts.forEach(this.testData.files, file => {
3326+
const fn = ts.normalizePath(file.fileName);
3327+
if (fn) {
3328+
if (fn === name) {
3329+
return file;
3330+
}
3331+
availableNames.push(fn);
3332+
}
3333+
});
3334+
return { file, availableNames };
3335+
}
3336+
3337+
private hasFile(name: string): boolean {
3338+
return this.tryFindFileWorker(name).file !== undefined;
3339+
}
3340+
33273341
private getLineColStringAtPosition(position: number) {
33283342
const pos = this.languageServiceAdapterHost.positionToLineAndCharacter(this.activeFile.fileName, position);
33293343
return `line ${(pos.line + 1)}, col ${pos.character}`;
@@ -3361,16 +3375,35 @@ Actual: ${stringify(fullActual)}`);
33613375
return !!a && !!b && a.start === b.start && a.length === b.length;
33623376
}
33633377

3364-
public getEditsForFileRename(options: FourSlashInterface.GetEditsForFileRenameOptions): void {
3365-
const changes = this.languageService.getEditsForFileRename(options.oldPath, options.newPath, this.formatCodeSettings, ts.defaultPreferences);
3366-
this.testNewFileContents(changes, options.newFileContents);
3378+
public getEditsForFileRename({ oldPath, newPath, newFileContents }: FourSlashInterface.GetEditsForFileRenameOptions): void {
3379+
const test = (fileContents: { readonly [fileName: string]: string }, description: string): void => {
3380+
const changes = this.languageService.getEditsForFileRename(oldPath, newPath, this.formatCodeSettings, ts.defaultPreferences);
3381+
this.testNewFileContents(changes, fileContents, description);
3382+
};
3383+
3384+
ts.Debug.assert(!this.hasFile(newPath), "initially, newPath should not exist");
3385+
3386+
test(newFileContents, "with file not yet moved");
3387+
3388+
this.languageServiceAdapterHost.renameFileOrDirectory(oldPath, newPath);
3389+
this.languageService.cleanupSemanticCache();
3390+
const pathUpdater = ts.getPathUpdater(oldPath, newPath, ts.createGetCanonicalFileName(/*useCaseSensitiveFileNames*/ false));
3391+
test(renameKeys(newFileContents, key => pathUpdater(key) || key), "with file moved");
33673392
}
33683393

33693394
private getApplicableRefactors(positionOrRange: number | ts.TextRange, preferences = ts.defaultPreferences): ReadonlyArray<ts.ApplicableRefactorInfo> {
33703395
return this.languageService.getApplicableRefactors(this.activeFile.fileName, positionOrRange, preferences) || ts.emptyArray;
33713396
}
33723397
}
33733398

3399+
function renameKeys<T>(obj: { readonly [key: string]: T }, renameKey: (key: string) => string): { readonly [key: string]: T } {
3400+
const res: { [key: string]: T } = {};
3401+
for (const key in obj) {
3402+
res[renameKey(key)] = obj[key];
3403+
}
3404+
return res;
3405+
}
3406+
33743407
export function runFourSlashTest(basePath: string, testType: FourSlashTestType, fileName: string) {
33753408
const content = Harness.IO.readFile(fileName)!;
33763409
runFourSlashTestContent(basePath, testType, content, fileName);

‎src/harness/harnessLanguageService.ts‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,21 @@ namespace Harness.LanguageService {
151151
this.scriptInfos.set(vpath.resolve(this.vfs.cwd(), fileName), new ScriptInfo(fileName, content, isRootFile));
152152
}
153153

154+
public renameFileOrDirectory(oldPath: string, newPath: string): void {
155+
this.vfs.mkdirpSync(ts.getDirectoryPath(newPath));
156+
this.vfs.renameSync(oldPath, newPath);
157+
158+
const updater = ts.getPathUpdater(oldPath, newPath, ts.createGetCanonicalFileName(/*useCaseSensitiveFileNames*/ false));
159+
this.scriptInfos.forEach((scriptInfo, key) => {
160+
const newFileName = updater(key);
161+
if (newFileName !== undefined) {
162+
this.scriptInfos.delete(key);
163+
this.scriptInfos.set(newFileName, scriptInfo);
164+
scriptInfo.fileName = newFileName;
165+
}
166+
});
167+
}
168+
154169
public editScript(fileName: string, start: number, end: number, newText: string) {
155170
const script = this.getScriptInfo(fileName);
156171
if (script) {

‎src/server/session.ts‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -129,13 +129,8 @@ namespace ts.server {
129129
project: Project;
130130
}
131131

132-
function allEditsBeforePos(edits: TextChange[], pos: number) {
133-
for (const edit of edits) {
134-
if (textSpanEnd(edit.span) >= pos) {
135-
return false;
136-
}
137-
}
138-
return true;
132+
function allEditsBeforePos(edits: ReadonlyArray<TextChange>, pos: number): boolean {
133+
return edits.every(edit => textSpanEnd(edit.span) < pos);
139134
}
140135

141136
// CommandNames used to be exposed before TS 2.4 as a namespace

‎src/services/getEditsForFileRename.ts‎

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,8 @@ namespace ts {
1313

1414
/** If 'path' refers to an old directory, returns path in the new directory. */
1515
type PathUpdater = (path: string) => string | undefined;
16-
function getPathUpdater(oldFileOrDirPath: string, newFileOrDirPath: string, getCanonicalFileName: GetCanonicalFileName): PathUpdater {
16+
// exported for tests
17+
export function getPathUpdater(oldFileOrDirPath: string, newFileOrDirPath: string, getCanonicalFileName: GetCanonicalFileName): PathUpdater {
1718
const canonicalOldPath = getCanonicalFileName(oldFileOrDirPath);
1819
return path => {
1920
if (getCanonicalFileName(path) === canonicalOldPath) return newFileOrDirPath;
@@ -101,7 +102,8 @@ namespace ts {
101102
getCanonicalFileName: GetCanonicalFileName,
102103
preferences: UserPreferences,
103104
): void {
104-
for (const sourceFile of program.getSourceFiles()) {
105+
const allFiles = program.getSourceFiles();
106+
for (const sourceFile of allFiles) {
105107
const newFromOld = oldToNew(sourceFile.fileName);
106108
const newImportFromPath = newFromOld !== undefined ? newFromOld : sourceFile.fileName;
107109
const newImportFromDirectory = getDirectoryPath(newImportFromPath);
@@ -120,15 +122,19 @@ namespace ts {
120122
return newAbsolute === undefined ? undefined : ensurePathIsNonModuleName(getRelativePathFromDirectory(newImportFromDirectory, newAbsolute, getCanonicalFileName));
121123
},
122124
importLiteral => {
125+
const importedModuleSymbol = program.getTypeChecker().getSymbolAtLocation(importLiteral);
126+
// No need to update if it's an ambient module^M
127+
if (importedModuleSymbol && importedModuleSymbol.declarations.some(d => isAmbientModule(d))) return undefined;
128+
123129
const toImport = oldFromNew !== undefined
124130
// If we're at the new location (file was already renamed), need to redo module resolution starting from the old location.
125131
// TODO:GH#18217
126132
? getSourceFileToImportFromResolved(resolveModuleName(importLiteral.text, oldImportFromPath, program.getCompilerOptions(), host as ModuleResolutionHost), oldToNew, program)
127-
: getSourceFileToImport(importLiteral, sourceFile, program, host, oldToNew);
133+
: getSourceFileToImport(importedModuleSymbol, importLiteral, sourceFile, program, host, oldToNew);
128134

129135
// Need an update if the imported file moved, or the importing file moved and was using a relative path.
130136
return toImport !== undefined && (toImport.updated || (importingSourceFileMoved && pathIsRelative(importLiteral.text)))
131-
? moduleSpecifiers.getModuleSpecifier(program.getCompilerOptions(), sourceFile, newImportFromPath, toImport.newFileName, host, preferences)
137+
? moduleSpecifiers.getModuleSpecifier(program.getCompilerOptions(), sourceFile, newImportFromPath, toImport.newFileName, host, allFiles, preferences)
132138
: undefined;
133139
});
134140
}
@@ -146,11 +152,17 @@ namespace ts {
146152
/** True if the imported file was renamed. */
147153
readonly updated: boolean;
148154
}
149-
function getSourceFileToImport(importLiteral: StringLiteralLike, importingSourceFile: SourceFile, program: Program, host: LanguageServiceHost, oldToNew: PathUpdater): ToImport | undefined {
150-
const symbol = program.getTypeChecker().getSymbolAtLocation(importLiteral);
151-
if (symbol) {
152-
if (symbol.declarations.some(d => isAmbientModule(d))) return undefined; // No need to update if it's an ambient module
153-
const oldFileName = find(symbol.declarations, isSourceFile)!.fileName;
155+
function getSourceFileToImport(
156+
importedModuleSymbol: Symbol | undefined,
157+
importLiteral: StringLiteralLike,
158+
importingSourceFile: SourceFile,
159+
program: Program,
160+
host: LanguageServiceHost,
161+
oldToNew: PathUpdater,
162+
): ToImport | undefined {
163+
if (importedModuleSymbol) {
164+
// `find` should succeed because we checked for ambient modules before calling this function.
165+
const oldFileName = find(importedModuleSymbol.declarations, isSourceFile)!.fileName;
154166
const newFileName = oldToNew(oldFileName);
155167
return newFileName === undefined ? { newFileName: oldFileName, updated: false } : { newFileName, updated: true };
156168
}

0 commit comments

Comments
 (0)