Skip to content

Commit d7e437a

Browse files
Have the ChangeTracker filter out edits that are no-ops (#38123)
* Filter out edits that are no-ops in 'organize imports'. * Updated tests for 'organize imports'. * Always remove no-op changes from the change tracker. * Add a new `stringContainsAt` helper function to avoid traversing the entire file contents. * Combine `map`/`filter` sequence into `mapDefined`. * Fix up documentation.
1 parent 9569e8a commit d7e437a

File tree

3 files changed

+87
-15
lines changed

3 files changed

+87
-15
lines changed

src/services/textChanges.ts

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -887,7 +887,7 @@ namespace ts.textChanges {
887887

888888
namespace changesToText {
889889
export function getTextChangesFromChanges(changes: readonly Change[], newLineCharacter: string, formatContext: formatting.FormatContext, validate: ValidateNonFormattedText | undefined): FileTextChanges[] {
890-
return group(changes, c => c.sourceFile.path).map(changesInFile => {
890+
return mapDefined(group(changes, c => c.sourceFile.path), changesInFile => {
891891
const sourceFile = changesInFile[0].sourceFile;
892892
// order changes by start position
893893
// If the start position is the same, put the shorter range first, since an empty range (x, x) may precede (x, y) but not vice-versa.
@@ -897,9 +897,20 @@ namespace ts.textChanges {
897897
Debug.assert(normalized[i].range.end <= normalized[i + 1].range.pos, "Changes overlap", () =>
898898
`${JSON.stringify(normalized[i].range)} and ${JSON.stringify(normalized[i + 1].range)}`);
899899
}
900-
const textChanges = normalized.map(c =>
901-
createTextChange(createTextSpanFromRange(c.range), computeNewText(c, sourceFile, newLineCharacter, formatContext, validate)));
902-
return { fileName: sourceFile.fileName, textChanges };
900+
901+
const textChanges = mapDefined(normalized, c => {
902+
const span = createTextSpanFromRange(c.range);
903+
const newText = computeNewText(c, sourceFile, newLineCharacter, formatContext, validate);
904+
905+
// Filter out redundant changes.
906+
if (span.length === newText.length && stringContainsAt(sourceFile.text, newText, span.start)) {
907+
return undefined;
908+
}
909+
910+
return createTextChange(span, newText);
911+
});
912+
913+
return textChanges.length > 0 ? { fileName: sourceFile.fileName, textChanges } : undefined;
903914
});
904915
}
905916

src/services/utilities.ts

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2827,6 +2827,35 @@ namespace ts {
28272827
return symbol.name;
28282828
}
28292829

2830+
/**
2831+
* Useful to check whether a string contains another string at a specific index
2832+
* without allocating another string or traversing the entire contents of the outer string.
2833+
*
2834+
* This function is useful in place of either of the following:
2835+
*
2836+
* ```ts
2837+
* // Allocates
2838+
* haystack.substr(startIndex, needle.length) === needle
2839+
*
2840+
* // Full traversal
2841+
* haystack.indexOf(needle, startIndex) === startIndex
2842+
* ```
2843+
*
2844+
* @param haystack The string that potentially contains `needle`.
2845+
* @param needle The string whose content might sit within `haystack`.
2846+
* @param startIndex The index within `haystack` to start searching for `needle`.
2847+
*/
2848+
export function stringContainsAt(haystack: string, needle: string, startIndex: number) {
2849+
const needleLength = needle.length;
2850+
if (needleLength + startIndex > haystack.length) {
2851+
return false;
2852+
}
2853+
for (let i = 0; i < needleLength; i++) {
2854+
if (needle.charCodeAt(i) !== haystack.charCodeAt(i + startIndex)) return false;
2855+
}
2856+
return true;
2857+
}
2858+
28302859
export function startsWithUnderscore(name: string): boolean {
28312860
return name.charCodeAt(0) === CharacterCodes._;
28322861
}

src/testRunner/unittests/services/organizeImports.ts

Lines changed: 43 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -285,6 +285,7 @@ namespace ts {
285285
});
286286
});
287287

288+
288289
describe("Baselines", () => {
289290

290291
const libFile = {
@@ -327,6 +328,16 @@ export const Other = 1;
327328
assert.isEmpty(changes);
328329
});
329330

331+
it("doesn't return any changes when the text would be identical", () => {
332+
const testFile = {
333+
path: "/a.ts",
334+
content: `import { f } from 'foo';\nf();`
335+
};
336+
const languageService = makeLanguageService(testFile);
337+
const changes = languageService.organizeImports({ type: "file", fileName: testFile.path }, testFormatSettings, emptyOptions);
338+
assert.isEmpty(changes);
339+
});
340+
330341
testOrganizeImports("Renamed_used",
331342
{
332343
path: "/test.ts",
@@ -366,6 +377,16 @@ D();
366377
},
367378
libFile);
368379

380+
it("doesn't return any changes when the text would be identical", () => {
381+
const testFile = {
382+
path: "/a.ts",
383+
content: `import { f } from 'foo';\nf();`
384+
};
385+
const languageService = makeLanguageService(testFile);
386+
const changes = languageService.organizeImports({ type: "file", fileName: testFile.path }, testFormatSettings, emptyOptions);
387+
assert.isEmpty(changes);
388+
});
389+
369390
testOrganizeImports("Unused_All",
370391
{
371392
path: "/test.ts",
@@ -377,14 +398,17 @@ import D from "lib";
377398
},
378399
libFile);
379400

380-
testOrganizeImports("Unused_Empty",
381-
{
401+
it("Unused_Empty", () => {
402+
const testFile = {
382403
path: "/test.ts",
383404
content: `
384405
import { } from "lib";
385406
`,
386-
},
387-
libFile);
407+
};
408+
const languageService = makeLanguageService(testFile);
409+
const changes = languageService.organizeImports({ type: "file", fileName: testFile.path }, testFormatSettings, emptyOptions);
410+
assert.isEmpty(changes);
411+
});
388412

389413
testOrganizeImports("Unused_false_positive_module_augmentation",
390414
{
@@ -414,25 +438,33 @@ declare module 'caseless' {
414438
test(name: KeyType): boolean;
415439
}
416440
}`
417-
});
441+
});
418442

419-
testOrganizeImports("Unused_false_positive_shorthand_assignment",
420-
{
443+
it("Unused_false_positive_shorthand_assignment", () => {
444+
const testFile = {
421445
path: "/test.ts",
422446
content: `
423447
import { x } from "a";
424448
const o = { x };
425449
`
426-
});
450+
};
451+
const languageService = makeLanguageService(testFile);
452+
const changes = languageService.organizeImports({ type: "file", fileName: testFile.path }, testFormatSettings, emptyOptions);
453+
assert.isEmpty(changes);
454+
});
427455

428-
testOrganizeImports("Unused_false_positive_export_shorthand",
429-
{
456+
it("Unused_false_positive_export_shorthand", () => {
457+
const testFile = {
430458
path: "/test.ts",
431459
content: `
432460
import { x } from "a";
433461
export { x };
434462
`
435-
});
463+
};
464+
const languageService = makeLanguageService(testFile);
465+
const changes = languageService.organizeImports({ type: "file", fileName: testFile.path }, testFormatSettings, emptyOptions);
466+
assert.isEmpty(changes);
467+
});
436468

437469
testOrganizeImports("MoveToTop",
438470
{

0 commit comments

Comments
 (0)