Support a "getCombinedCodeFix" service · Pull Request #20338 · microsoft/TypeScript · GitHub

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?

Please add a GetCombinedCodeFixResponse definition

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?

why not optional instead?

why not optional instead.

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

I think a better name is actionIds or actionCodes.

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.

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.

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

"getCombinedCodeFix-full"?

CombinedCodeActions?

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.

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

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?

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

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

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.

gid is out of date

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.

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? Mohamed Hegazy (@mhegazy)?

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

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?

Mohamed Hegazy (@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.

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')

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?

Andrew Casey (@amcasey) Mohamed Hegazy (@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.

Can the return type be ReadonlyArray ?

👍

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.

Andy (Andrewkraft) (@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.

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

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 Andrew Casey (amcasey), Arthur Ozga (aozgaa), Armando Aguirre (armanio123) and Sheetal Nandi (sheetalkamat) November 29, 2017 21:40

Mohamed Hegazy (mhegazy) reviewed Nov 29, 2017

View reviewed changes

Mohamed Hegazy (mhegazy) reviewed Nov 30, 2017

View reviewed changes

Andrew Casey (amcasey) reviewed Nov 30, 2017

View reviewed changes

Rename things

be19542

Mohamed Hegazy (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

Andrew Casey (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

Sheetal Nandi (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

Andrew Casey (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

Rob Yoder (robyoder) mentioned this pull request Dec 13, 2017

Fix all in file for import fix #20315

Closed

Anantha Kumaran (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 (microsoft) locked and limited conversation to collaborators Jun 14, 2018

This pull request was closed.

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

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

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

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

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

                       errorCodes?: number[];
                   }
+                  export interface GetCombinedCodeFixRequestArgs extends FileRequestArgs {

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

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

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

                   }
+                  /** Add a value to a set, and return true if it wasn't already present. */
+                  export function addToSeenIds(seen: true[], key: number): boolean {

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

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

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

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

+                          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);

Support a "getCombinedCodeFix" service · Pull Request #20338 · microsoft/TypeScript · GitHub

Uh oh!

Support a "getCombinedCodeFix" service - #20338

Support a "getCombinedCodeFix" service#2033815 commits merged intomastermicrosoft/TypeScript:masterfrom codeFixAllmicrosoft/TypeScript:codeFixAllCopy head branch name to clipboard

Conversation

Deleted user (ghost) commented Nov 29, 2017 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Andrew Casey (amcasey) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Andrew Casey (amcasey) commented Dec 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Andrew Casey (amcasey) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Deleted user (ghost) commented Dec 1, 2017

Uh oh!

Andrew Casey (amcasey) commented Dec 1, 2017

Uh oh!

Andrew Casey (amcasey) commented Dec 1, 2017

Uh oh!

Mohamed Hegazy (mhegazy) commented Dec 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Andrew Casey (amcasey) commented Dec 2, 2017

Uh oh!

Andrew Casey (amcasey) commented Dec 6, 2017

Uh oh!

Andrew Casey (amcasey) commented Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Deleted user (ghost) commented Dec 7, 2017

Uh oh!

Support a "getCombinedCodeFix" service#20338
15 commits merged into
masterfrom
codeFixAll

Deleted user (ghost) commented Nov 29, 2017 •
edited by ghost
Loading

Mohamed Hegazy (mhegazy) commented Dec 2, 2017 •
edited
Loading

Andrew Casey (amcasey) commented Dec 6, 2017 •
edited
Loading