Fix Add all missing imports when ordered alphabetically #43453
Conversation
typescript-bot
added
the
For Milestone Bug
src/services/textChanges.ts
Outdated
| @@ -916,7 +927,7 @@ namespace ts.textChanges { | |||
| const normalized = stableSort(changesInFile, (a, b) => (a.range.pos - b.range.pos) || (a.range.end - b.range.end)); | |||
| // verify that change intervals do not overlap, except possibly at end points. | |||
| for (let i = 0; i < normalized.length - 1; i++) { | |||
| Debug.assert(normalized[i].range.end <= normalized[i + 1].range.pos, "Changes overlap", () => | |||
| Debug.assert(normalized[i].range.end <= normalized[i + 1].range.pos || normalized[i].kind === ChangeKind.ReplaceWithList, "Changes overlap", () => | |||
There was a problem hiding this comment.
Could you paste the two JSON/debugger output of the two changes (normalized[i] and normalized[i + 1]) that trigger this assertion? I have some thoughts, but want to make sure I understand what’s happening correctly.
There was a problem hiding this comment.
They are pretty big nodes. But here it goes:
normalized[i] = {
"kind": 4,
"sourceFile": {},
"range": {
"pos": 15,
"end": 16
},
"options": {
"prefix": " ",
"suffix": ", "
},
"node": {
"pos": -1,
"end": -1,
"flags": 8,
"modifierFlagsCache": 0,
"transformFlags": 0,
"kind": 266,
"name": {
"pos": -1,
"end": -1,
"flags": 8,
"modifierFlagsCache": 0,
"transformFlags": 0,
"kind": 78,
"escapedText": "Test2"
}
}
}normalized[i + 1] = {
"kind": 4,
"sourceFile": {},
"range": {
"pos": 15,
"end": 16
},
"options": {
"prefix": " ",
"suffix": ", "
},
"node": {
"pos": -1,
"end": -1,
"flags": 8,
"modifierFlagsCache": 0,
"transformFlags": 0,
"kind": 266,
"name": {
"pos": -1,
"end": -1,
"flags": 8,
"modifierFlagsCache": 0,
"transformFlags": 0,
"kind": 78,
"escapedText": "Test3"
}
}
}There was a problem hiding this comment.
Edited ^ to remove the sourceFile node so we can look at this. The source file text was:
import { Test1, Test4 } from './file1';
interface Testing {
test1: Test1;
test2: Test2;
test3: Test3;
test4: Test4;
}There was a problem hiding this comment.
What’s happening is that both fixes are attempting to delete the space between Test1, and Test4 with their inserted node. They make up for the deletion of the space by including it in the prefix of their insertion. But it’s the deletion of the space that’s the alleged problem, because overlapping deletes might be problematic. It seems like the simplest fix would just be to ensure that this insertion is performed with an empty range, so it doesn’t try to delete anything.
src/services/textChanges.ts
Outdated
| @@ -916,7 +927,7 @@ namespace ts.textChanges { | |||
| const normalized = stableSort(changesInFile, (a, b) => (a.range.pos - b.range.pos) || (a.range.end - b.range.end)); | |||
| // verify that change intervals do not overlap, except possibly at end points. | |||
| for (let i = 0; i < normalized.length - 1; i++) { | |||
| Debug.assert(normalized[i].range.end <= normalized[i + 1].range.pos, "Changes overlap", () => | |||
| Debug.assert(normalized[i].range.end <= normalized[i + 1].range.pos || normalized[i].kind === ChangeKind.ReplaceWithList, "Changes overlap", () => | |||
There was a problem hiding this comment.
What’s happening is that both fixes are attempting to delete the space between Test1, and Test4 with their inserted node. They make up for the deletion of the space by including it in the prefix of their insertion. But it’s the deletion of the space that’s the alleged problem, because overlapping deletes might be problematic. It seems like the simplest fix would just be to ensure that this insertion is performed with an empty range, so it doesn’t try to delete anything.
src/services/textChanges.ts
Outdated
| @@ -769,7 +780,7 @@ namespace ts.textChanges { | |||
|
|
|||
| // write separator and leading trivia of the next element as suffix | |||
| const suffix = `${tokenToString(nextToken.kind)}${sourceFile.text.substring(nextToken.end, containingList[index + 1].getStart(sourceFile))}`; | |||
| this.replaceRange(sourceFile, createRange(startPos, containingList[index + 1].getStart(sourceFile)), newNode, { prefix, suffix }); | |||
| this.replaceRangeWithList(sourceFile, createRange(startPos, containingList[index + 1].getStart(sourceFile)), newNode, { prefix, suffix }); | |||
There was a problem hiding this comment.
This is the problematic call, because the range is non-empty. It feels like you could just move startPos forward such that prefix is never needed, and create an empty range.
armanio123
force-pushed
the
FixAddAllMissingImports
branch
from
fc47b78 to
1c66f9f
Compare
|
Done, the fix was actually super easy once you noticed that the problem is that is replacing the same text. Now instead of replacing it will insert in x position and uses the next node trivia for formatting. |
armanio123
changed the title
| else { | ||
| // next element is located on different line that separator | ||
| // let insert position be the beginning of the line that contains next element | ||
| startPos = getStartPositionOfLine(lineAndCharOfNextElement.line, sourceFile); |
There was a problem hiding this comment.
There are a couple failing tests now; I’m guessing some of this was still important. If I had to guess, I would say that this else block needs to be preserved for insertions where the next element is on a different line.
There was a problem hiding this comment.
I really thought I had run the tests. Anyway is fixed now. We still don't need all of that we just needed to consider the trivia.
Fixes #42527