Support a "getCombinedCodeFix" service #20338

ghost · 2017-11-29T20:20:45Z

Fixes #14549
Does not fix #20315
Each code fix may come with a groupId object. This groupId may then be passed to a getCombinedCodeFix services method which generates a code action combining all code fixes in that group.
This would be a good time to consider changes to the API, such as if we want fixes to be able to have multiple groupIds or if we want to give groupIds a human-readable description too.
Also, we might want to design the signature of getCombinedCodeFix to allow for other possible scopes in the future; such as taking { type: "file", fileName: "..." } instead of just a filename directly.

For the implementation: For the most part, services could be written so that their main body is a function taking a changeTracker, and we can then iterate over every error in the file and apply changes to the tracker. Some were more difficult due to creating strings directly instead of nodes (via checker.typeToString) -- it would probably be better to use checker.typeToTypeNode instead?

mhegazy · 2017-11-29T23:42:12Z

src/server/protocol.ts

@@ -585,6 +593,10 @@ namespace ts.server.protocol {
        errorCodes?: number[];
    }

+    export interface GetCombinedCodeFixRequestArgs extends FileRequestArgs {


Please add a GetCombinedCodeFixResponse definition

mhegazy · 2017-11-29T23:43:18Z

src/server/protocol.ts

@@ -585,6 +593,10 @@ namespace ts.server.protocol {
        errorCodes?: number[];
    }

+    export interface GetCombinedCodeFixRequestArgs extends FileRequestArgs {
+        groupId: {};


Why would not this be string? i am assuming to allow for an array, but when would you need this? u have an example in mind?

mhegazy · 2017-11-29T23:44:09Z

src/server/protocol.ts

@@ -1580,6 +1592,16 @@ namespace ts.server.protocol {
        commands?: {}[];
    }

+    export interface CodeActionAll {
+        changes: FileCodeEdits[];
+        commands: {}[] | undefined;


why not optional instead?

mhegazy · 2017-11-29T23:44:18Z

src/server/protocol.ts

+
+    export interface CodeFix extends CodeAction {
+        /** If present, one may call 'getAllCodeFixesInGroup' with this groupId. */
+        groupId: {} | undefined;


why not optional instead.

mhegazy · 2017-11-30T00:53:54Z

src/services/codeFixProvider.ts

-        getCodeActions(context: CodeFixContext): CodeAction[] | undefined;
+        getCodeActions(context: CodeFixContext): CodeFix[] | undefined;
+        groupIds: string[];
+        fixAllInGroup(context: CodeFixAllContext): CodeActionAll;


this should be optional. exsiting users like tslint for instance do not supply this function.

though this si not a public API.

Also a name that still include codeActions would be better.. something that maintains parity with getCodeActions

maybe getAllCodeActions or getAllCodeActionsInFile

mhegazy · 2017-11-30T01:08:54Z

src/services/codeFixProvider.ts

        errorCodes: number[];
-        getCodeActions(context: CodeFixContext): CodeAction[] | undefined;
+        getCodeActions(context: CodeFixContext): CodeFix[] | undefined;
+        groupIds: string[];


I think a better name is actionIds or actionCodes.

mhegazy · 2017-11-30T01:13:49Z

src/services/codeFixProvider.ts

        program: Program;
        host: LanguageServiceHost;
        cancellationToken: CancellationToken;
    }

+    export interface CodeFixAllContext extends CodeFixContextBase {
+        groupId: {};


i think we should just make this a string, unless we have a reason not to.

If we use string we are limited to treating it like an enum. With an object we could support any data as part of an action id, such as the name of a class if we wanted to limit fixes to just that. That could be done with a string, but we would have to stringify and parse the data, which is silly because we're sending this through JSON already anyway.

mhegazy · 2017-11-30T01:40:26Z

src/services/codefixes/disableJsDiagnostics.ts

+                    newText: `// @ts-nocheck${newLineCharacter}`
+                }])],
+                // groupId unnecessary because adding `// @ts-nocheck` even once will ignore every error in the file.
+                groupId: undefined,


would it be better to split this CodeFix into two, and make sure that a CodeFix is a union type, that either it has fillAllInGroup and returns groupId/groupIds or it does not.

I think it would be a pain to have to split up a codefix just to allow multiple (or optional) groupIds. If anything I think we should consolidate them more, like fixClassDoesntImplementInheritedAbstractClass and fixClassIncorrectlyImplementsInterface.

amcasey

I'm not done reviewing but I wanted to indicate that I'm looking at this.

amcasey · 2017-11-30T01:15:55Z

src/server/protocol.ts

        /* @internal */
        GetCodeFixesFull = "getCodeFixes-full",
+        GetCombinedCodeFix = "getCombinedCodeFix",
+        /* @internal */
+        GetCombinedCodeFixFull = "getCombinedCodeFix",


"getCombinedCodeFix-full"?

mhegazy · 2017-11-30T19:36:38Z

src/server/protocol.ts

@@ -1580,6 +1596,16 @@ namespace ts.server.protocol {
        commands?: {}[];
    }

+    export interface CodeActionAll {


CombinedCodeActions?

mhegazy · 2017-11-30T19:40:27Z

src/services/codeFixProvider.ts

-        getCodeActions(context: CodeFixContext): CodeAction[] | undefined;
+        getCodeActions(context: CodeFixContext): CodeFix[] | undefined;
+        actionIds: string[];
+        getAllCodeActions(context: CodeFixAllContext): CodeActionAll;


we should make this one optional

Or make CodeFixRegisraction a union type

not all codeFixes need to provide a list of applicable actionIds, and not all of them need to provide a getAllCodeActions feature and not all of them need to return an actionId in their output.

mhegazy · 2017-11-30T19:48:26Z

src/services/utilities.ts

@@ -1099,6 +1099,32 @@ namespace ts {
            return !seen[id] && (seen[id] = true);
        };
    }
+
+    /** Add a value to a set, and return true if it wasn't already present. */
+    export function addToSeenIds(seen: true[], key: number): boolean {


why not jsut use a map for these, it is a sparse array anyways..

amcasey · 2017-12-01T01:39:57Z

We've had some problems in the past when we've tried to make multiple changes to the same range in a single ChangeTracker. Do we have some reason to believe that won't occur when combining code fixes?

amcasey · 2017-12-01T01:50:53Z

src/services/codeFixProvider.ts


-        export function registerCodeFix(codeFix: CodeFix) {
-            forEach(codeFix.errorCodes, error => {
+        export function registerCodeFix(codeFix: CodeFixRegistration) {


Now that there's a CodeFix type, codeFix is a confusing name for a CodeFixRegistration.

amcasey

Is this based on Roslyn's implementation? Using theirs as a model might help us avoid making the same mistakes.

amcasey · 2017-12-01T01:54:28Z

src/services/types.ts

@@ -409,6 +410,16 @@ namespace ts {
        commands?: CodeActionCommand[];
    }

+    export interface CodeFix extends CodeAction {
+        /** If present, one may call 'getCombinedCodeFix' with this actionId. */
+        actionId?: {} | undefined;


This is on a separate type for back compat reasons? Making it optional isn't enough?

A CodeFix includes both a CodeAction and an optional action id. A CodeAction will only have an actionId if returned from a code fix, not if from a completion.

amcasey · 2017-12-01T01:59:20Z

src/services/codeFixProvider.ts

-            });
+            }
+            if (codeFix.actionIds) {
+                for (const gid of codeFix.actionIds) {


gid is out of date

ghost · 2017-12-01T16:00:19Z

We've had some problems in the past when we've tried to make multiple changes to the same range in a single ChangeTracker. Do we have some reason to believe that won't occur when combining code fixes?

We know that ChangeTracker can't handle all possible sequences of changes because some would be nonsensical -- so it's a codefix's responsibility to ensure it's doing the possible and not e.g. making any changes twice (see the many addToSeen calls).
Since ChangeTracker already has the responsibility of making multiple changes, I think it's best to make all changes through there and make improvements to ChangeTracker if any problems come up. (
There was one problem that came up when deleting the last and next-to-last items in a list, fixed in this PR.) That should be easier than trying to combine raw text changes.

amcasey · 2017-12-01T18:26:01Z

I don't want to bikeshed on naming, but it seems strange that a CodeFix is a CodeAction with an (optional) actionId. If the ID is for the action, why isn't it on the action? Maybe a CodeFixAction is a CodeAction with a fixId? @mhegazy?

amcasey · 2017-12-01T18:26:32Z

I'm not quite clear on the difference between a CombinedCodeAction and a CodeActionAll. Do we just use different names in different layers?

mhegazy · 2017-12-02T01:02:37Z

This is the extra ID I was thinking of:
http://source.roslyn.io/#Microsoft.CodeAnalysis.Workspaces/CodeActions/CodeAction.cs,48

so we should generate that from all the inputs to the codefix to make sure they are unique?

but what to do if combining the fixes is not just applying them in sequence?

amcasey · 2017-12-02T02:01:27Z

@mhegazy I think it's more or less equivalent to what we're calling action/fix ID and we have the option of incorporating the code in the future if that turns out to be interesting.

amcasey · 2017-12-06T21:51:53Z

Does VS offer all levels of repetition for all code fixes or do code fixes somehow indicate that they can be repeated? If they indicate that they can be repeated, do they somehow indicate which choices would be sensible? (e.g. don't offer 'project' if all occurrences are in 'file')

amcasey · 2017-12-06T22:06:53Z

Roslyn appears to offer all scopes when repetition is offered, but not always offer repetition (e.g. add missing member), so we might need to add a bit to CodeFixAction. Or would you just omit the fixId in that case?

ghost · 2017-12-07T15:40:17Z

@amcasey @mhegazy I've made the API changes internal for now (and marked with #20538). I'd like to get this PR in soon since it touches a lot of files.

sheetalkamat · 2017-12-07T17:14:24Z

src/server/client.ts

@@ -553,15 +553,18 @@ namespace ts.server {
            return notImplemented();
        }

-        getCodeFixesAtPosition(file: string, start: number, end: number, errorCodes: number[]): CodeAction[] {
+        getCodeFixesAtPosition(file: string, start: number, end: number, errorCodes: number[]): CodeFixAction[] {


Can the return type be ReadonlyArray ?

sheetalkamat · 2017-12-07T17:19:33Z

src/harness/fourslash.ts

+            const { changes, commands } = this.languageService.getCombinedCodeFix({ type: "file", fileName: this.activeFile.fileName }, fixId, this.formatCodeSettings);
+            assert.deepEqual(commands, options.commands);
+            this.applyChanges(changes);
+            this.verifyCurrentFileContent(newFileContent);


will this always be currentFile. Api call returns TextChangeRange which has file name per change.. So wouldnt it need to verify all those files?

Added an assertion that all changes are for the current file and a TODO for if we need to test changes affecting multiple files.

amcasey · 2017-12-07T18:22:29Z

@andy-ms Do we have a way to signal that some code fixes can't be repeated? Or, at least, can we add that later without affecting back-compat? Otherwise, LGTM.

ghost · 2017-12-07T18:26:57Z

I think we can do that by just not supplying a fixId.

amcasey · 2017-12-07T18:33:57Z

Can you please add a comment to that effect on the declaration? I'm worried that future implementers (i.e. me) might forget that that's an option and try to add a flag.

Support a "getCombinedCodeFix" service

5c70641

ghost force-pushed the codeFixAll branch from a0c79b5 to 5c70641 Compare November 29, 2017 20:51

ghost requested review from sheetalkamat, amcasey, armanio123 and aozgaa November 29, 2017 21:40

mhegazy reviewed Nov 29, 2017

View reviewed changes

mhegazy reviewed Nov 30, 2017

View reviewed changes

amcasey reviewed Nov 30, 2017

View reviewed changes

Rename things

be19542

mhegazy reviewed Nov 30, 2017

View reviewed changes

Andy Hanson added 2 commits November 30, 2017 15:47

Code review

8a1865f

Merge branch 'master' into codeFixAll

b89b7fe

amcasey reviewed Dec 1, 2017

View reviewed changes

Rename things

a8b3afc

Update API baselines

a38216c

CodeActionAll -> CombinedCodeActions

c621e3f

Andy Hanson added 4 commits December 5, 2017 10:12

Merge branch 'master' into codeFixAll

d0af16a

Take a scope parameter instead of fileName for flexibility

2db8649

Renames and bugfixes

f2031be

Merge branch 'master' into codeFixAll

79c0150

ghost mentioned this pull request Dec 7, 2017

Make getCombinedCodeFix public #20538

Closed

Make API changes internal

8a61761

ghost force-pushed the codeFixAll branch from fce0913 to 8a61761 Compare December 7, 2017 15:23

Merge branch 'master' into codeFixAll

41c5710

sheetalkamat reviewed Dec 7, 2017

View reviewed changes

Code review

87d30c0

ghost force-pushed the codeFixAll branch from 4df09b0 to 87d30c0 Compare December 7, 2017 18:26

amcasey approved these changes Dec 7, 2017

View reviewed changes

Update comment

5f7ba73

ghost merged commit 19ea189 into master Dec 7, 2017

ghost deleted the codeFixAll branch December 7, 2017 20:14

robyoder mentioned this pull request Dec 13, 2017

Fix all in file for import fix #20315

Closed

ananthakumaran mentioned this pull request Jan 28, 2018

add support for fix all in file & project-info ananthakumaran/tide#234

Merged

This was referenced Apr 11, 2018

Remove TODO: GH#18795 comments #23337

Merged

Fix insertNodeAtClassStart for empty class with comment #23342

Merged

Don't copy trivia when implementing an interface #23343

Merged

microsoft locked and limited conversation to collaborators Jun 14, 2018

This pull request was closed.

Support a "getCombinedCodeFix" service #20338

Support a "getCombinedCodeFix" service #20338

Conversation

ghost commented Nov 29, 2017 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcasey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcasey commented Dec 1, 2017

Choose a reason for hiding this comment

amcasey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Dec 1, 2017

amcasey commented Dec 1, 2017

amcasey commented Dec 1, 2017

mhegazy commented Dec 2, 2017 • edited Loading

amcasey commented Dec 2, 2017

amcasey commented Dec 6, 2017

amcasey commented Dec 6, 2017 • edited Loading

ghost commented Dec 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcasey commented Dec 7, 2017

ghost commented Dec 7, 2017

amcasey commented Dec 7, 2017

ghost commented Nov 29, 2017 •

edited by ghost

Loading

mhegazy commented Dec 2, 2017 •

edited

Loading

amcasey commented Dec 6, 2017 •

edited

Loading