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.

src/services/textChanges.ts

@@ -887,7 +887,7 @@ namespace ts.textChanges {
 
     namespace changesToText {
         export function getTextChangesFromChanges(changes: readonly Change[], newLineCharacter: string, formatContext: formatting.FormatContext, validate: ValidateNonFormattedText | undefined): FileTextChanges[] {
-            return group(changes, c => c.sourceFile.path).map(changesInFile => {
+            return mapDefined(group(changes, c => c.sourceFile.path), changesInFile => {
                 const sourceFile = changesInFile[0].sourceFile;
                 // order changes by start position
                 // 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 {
                     Debug.assert(normalized[i].range.end <= normalized[i + 1].range.pos, "Changes overlap", () =>
                         `${JSON.stringify(normalized[i].range)} and ${JSON.stringify(normalized[i + 1].range)}`);
                 }
-                const textChanges = normalized.map(c =>
-                    createTextChange(createTextSpanFromRange(c.range), computeNewText(c, sourceFile, newLineCharacter, formatContext, validate)));
-                return { fileName: sourceFile.fileName, textChanges };
+
+                const textChanges = mapDefined(normalized, c => {
+                    const span = createTextSpanFromRange(c.range);
+                    const newText = computeNewText(c, sourceFile, newLineCharacter, formatContext, validate);
+
+                    // Filter out redundant changes.
+                    if (span.length === newText.length && stringContainsAt(sourceFile.text, newText, span.start)) {
+                        return undefined;
+                    }
+
+                    return createTextChange(span, newText);
+                });
+
+                return textChanges.length > 0 ? { fileName: sourceFile.fileName, textChanges } : undefined;
             });
         }
 

src/services/utilities.ts

@@ -2827,6 +2827,35 @@ namespace ts {
         return symbol.name;
     }
 
+    /**
+     * Useful to check whether a string contains another string at a specific index
+     * without allocating another string or traversing the entire contents of the outer string.
+     *
+     * This function is useful in place of either of the following:
+     *
+     * ```ts
+     * // Allocates
+     * haystack.substr(startIndex, needle.length) === needle
+     *
+     * // Full traversal
+     * haystack.indexOf(needle, startIndex) === startIndex
+     * ```
+     *
+     * @param haystack The string that potentially contains `needle`.
+     * @param needle The string whose content might sit within `haystack`.
+     * @param startIndex The index within `haystack` to start searching for `needle`.
+     */
+    export function stringContainsAt(haystack: string, needle: string, startIndex: number) {
+        const needleLength = needle.length;
+        if (needleLength + startIndex > haystack.length) {
+            return false;
+        }
+        for (let i = 0; i < needleLength; i++) {
+            if (needle.charCodeAt(i) !== haystack.charCodeAt(i + startIndex)) return false;
+        }
+        return true;
+    }
+
     export function startsWithUnderscore(name: string): boolean {
         return name.charCodeAt(0) === CharacterCodes._;
     }

src/testRunner/unittests/services/organizeImports.ts

@@ -285,6 +285,7 @@ namespace ts {
             });
         });
 
+
         describe("Baselines", () => {
 
             const libFile = {
@@ -327,6 +328,16 @@ export const Other = 1;
                 assert.isEmpty(changes);
             });
 
+            it("doesn't return any changes when the text would be identical", () => {
+                const testFile = {
+                    path: "/a.ts",
+                    content: `import { f } from 'foo';\nf();`
+                };
+                const languageService = makeLanguageService(testFile);
+                const changes = languageService.organizeImports({ type: "file", fileName: testFile.path }, testFormatSettings, emptyOptions);
+                assert.isEmpty(changes);
+            });
+
             testOrganizeImports("Renamed_used",
                 {
                     path: "/test.ts",
@@ -366,6 +377,16 @@ D();
                 },
                 libFile);
 
+                it("doesn't return any changes when the text would be identical", () => {
+                    const testFile = {
+                        path: "/a.ts",
+                        content: `import { f } from 'foo';\nf();`
+                    };
+                    const languageService = makeLanguageService(testFile);
+                    const changes = languageService.organizeImports({ type: "file", fileName: testFile.path }, testFormatSettings, emptyOptions);
+                    assert.isEmpty(changes);
+                });
+
             testOrganizeImports("Unused_All",
                 {
                     path: "/test.ts",
@@ -377,14 +398,17 @@ import D from "lib";
                 },
                 libFile);
 
-            testOrganizeImports("Unused_Empty",
-                {
+            it("Unused_Empty", () => {
+                const testFile = {
                     path: "/test.ts",
                     content: `
 import { } from "lib";
 `,
-                },
-                libFile);
+                };
+                const languageService = makeLanguageService(testFile);
+                const changes = languageService.organizeImports({ type: "file", fileName: testFile.path }, testFormatSettings, emptyOptions);
+                assert.isEmpty(changes);
+            });
 
             testOrganizeImports("Unused_false_positive_module_augmentation",
                 {
@@ -414,25 +438,33 @@ declare module 'caseless' {
         test(name: KeyType): boolean;
     }
 }`
-                });
+            });
 
-            testOrganizeImports("Unused_false_positive_shorthand_assignment",
-                {
+            it("Unused_false_positive_shorthand_assignment", () => {
+                const testFile = {
                     path: "/test.ts",
                     content: `
 import { x } from "a";
 const o = { x };
 `
-                });
+                };
+                const languageService = makeLanguageService(testFile);
+                const changes = languageService.organizeImports({ type: "file", fileName: testFile.path }, testFormatSettings, emptyOptions);
+                assert.isEmpty(changes);
+            });
 
-            testOrganizeImports("Unused_false_positive_export_shorthand",
-                {
+            it("Unused_false_positive_export_shorthand", () => {
+                const testFile = {
                     path: "/test.ts",
                     content: `
 import { x } from "a";
 export { x };
 `
-                });
+                };
+                const languageService = makeLanguageService(testFile);
+                const changes = languageService.organizeImports({ type: "file", fileName: testFile.path }, testFormatSettings, emptyOptions);
+                assert.isEmpty(changes);
+            });
 
             testOrganizeImports("MoveToTop",
                 {