Odd quick fix ordering for misspelled identifiers

[DanielRosenwasser]

I believe that "misspelled identifier" quick fixes should precede the "remove unused declarations".

If anyone wants to take this on, this should just be a matter of

1. Making a test case for this, and
2. Shifting the order of which quick fix is registered first.

[ghost]

The ordering of fixes is currently determined by the load order of files, since they each immediately alter global state the moment they're loaded. I think we should change these files to export an object instead of calling registerCodeFix, make registerCodeFix private, and have codeFixProvider import from all code fixes and register them in an order it chooses.

[jrkong]

I'd like to give this one a shot if no one's working on it. This is my first issue so I would really appreciate it if someone could direct me to the files I should look at to fix this.

[RyanCavanaugh]

@jrkong you just have to change the ordering of the codefix files in the various tsconfig.json files under src

[jrkong]

@RyanCavanaugh thanks a lot! That really helps, I'll do some digging.

[jrkong]

So after spending a bit of time with the code I've discovered some odd inconsistencies with the behavior for this particular bug:

When I attempt to reproduce the bug with the given line in the issue this is what I see:

However if I added the lines:

export {};
function f() {}

the bug is reproduced.

@DanielRosenwasser could you verify if I've reproduced the bug properly?

Here's where things got really odd for me:

Looking at the src/services/tsconfig.json the spelling code fixes are imported on line 54 and the code fixes for removing imports and unused dependencies are imported on line 62.

So in theory Change spelling to 'assertNever' should be loaded before Remove import from './Utilities' and Delete all unused declarations. What's more, the tests seem to corroborate that the source code is loading the spelling codefixes first:
Here's a screenshot of the test I've been working on and how it currently fails.

I deliberately tried testing against the incorrect output to verify the behavior of the bug before I made any further changes but the tests seem to imply that Change spelling to 'assertNever' should already. So does anyone have any idea what's going on?

[ghost]

I looked at the tsserver logs and it looks like the editor is making two separate calls:

Info 83   [17:4:3.734] request:
    {"seq":12,"type":"request","command":"getCodeFixes","arguments":{"file":"/home/andy/sample/ts/src/a.ts","startLine":1,"startOffset":10,"endLine":1,"endOffset":16,"errorCodes":[2724]}}
Perf 84   [17:4:3.793] 12::getCodeFixes: elapsed time (in milliseconds) 58.5188
Info 85   [17:4:3.793] response:
    {"seq":0,"type":"response","command":"getCodeFixes","request_seq":12,"success":true,"body":[{"fixName":"spelling","description":"Change spelling to 'fooBar'","changes":[{"fileName":"/home/andy/sample/ts/src/a.ts","textChanges":[{"start":{"line":1,"offset":10},"end":{"line":1,"offset":16},"newText":"fooBar"}]}],"fixId":"fixSpelling","fixAllDescription":"Fix all detected spelling errors"}]}
Info 86   [17:4:3.794] request:
    {"seq":13,"type":"request","command":"getCodeFixes","arguments":{"file":"/home/andy/sample/ts/src/a.ts","startLine":1,"startOffset":1,"endLine":1,"endOffset":30,"errorCodes":[6133]}}
Perf 87   [17:4:3.795] 13::getCodeFixes: elapsed time (in milliseconds) 1.0016
Info 88   [17:4:3.795] response:
    {"seq":0,"type":"response","command":"getCodeFixes","request_seq":13,"success":true,"body":[{"fixName":"unusedIdentifier","description":"Remove import from './b'","changes":[{"fileName":"/home/andy/sample/ts/src/a.ts","textChanges":[{"start":{"line":1,"offset":1},"end":{"line":2,"offset":1},"newText":""}]}],"fixId":"unusedIdentifier_delete","fixAllDescription":"Delete all unused declarations"}]}

So the ordering of our results is irrelevant because we're only returning one result at a time.
Our API is designed to take a range and a list of error codes, so perhaps it should be the editor's responsibility to make one combined call when ranges overlap, and then we could order the results in tsserver. Another option would to be return fixes with some kind of priority, but that would be irrelevant if they aren't actually overlapping. @mjbvz

[shonnungar]

Is this issue still up for grabs? @DanielRosenwasser

[DanielRosenwasser]

I think from the conversation above, this issue is too involved and needs some actual discussion at an editor sync first.

[DanielRosenwasser]

One idea we had in the editor sync: an editor could ask about diagnostics with narrower spans than wider spans.

Another: an editor could provide multiple diagnostics and ranges it's interested in fixing in one call; that way the server could fix this.

Another-nother idea: editor provides a range, TypeScript just tries to fix all error codes.

Another-nother-nother idea: because error spans can drift, it might be better to just ask for the error code.

But overall, we need to get a better idea from how editors have implemented this.

There might be some inspiration from how refactorings work.