Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit d7e437a

Browse filesBrowse files
DanielRosenwasserDanielRosenwasser
authored
Have the ChangeTracker filter out edits that are no-ops (microsoft#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
Copy full SHA for d7e437a

3 files changed

+87-15Lines changed: 87 additions & 15 deletions

File tree

Expand file treeCollapse file tree
Open diff view settings
Filter options
Expand file treeCollapse file tree
Open diff view settings
Collapse file

‎src/services/textChanges.ts‎

Copy file name to clipboardExpand all lines: src/services/textChanges.ts
+15-4Lines 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

Collapse file

‎src/services/utilities.ts‎

Copy file name to clipboardExpand all lines: src/services/utilities.ts
+29Lines 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
}
Collapse file

‎src/testRunner/unittests/services/organizeImports.ts‎

Copy file name to clipboardExpand all lines: src/testRunner/unittests/services/organizeImports.ts
+43-11Lines 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 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.