Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support a "getCombinedCodeFix" service #20338

Merged
15 commits merged into from
Dec 7, 2017
Merged

Support a "getCombinedCodeFix" service #20338

15 commits merged into from
Dec 7, 2017

Conversation

ghost
Copy link

@ghost ghost commented Nov 29, 2017

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?

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

export interface GetCombinedCodeFixRequestArgs extends FileRequestArgs {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a GetCombinedCodeFixResponse definition

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

export interface GetCombinedCodeFixRequestArgs extends FileRequestArgs {
groupId: {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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

export interface CodeActionAll {
changes: FileCodeEdits[];
commands: {}[] | undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not optional instead?


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not optional instead.

getCodeActions(context: CodeFixContext): CodeAction[] | undefined;
getCodeActions(context: CodeFixContext): CodeFix[] | undefined;
groupIds: string[];
fixAllInGroup(context: CodeFixAllContext): CodeActionAll;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

though this si not a public API.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe getAllCodeActions or getAllCodeActionsInFile

errorCodes: number[];
getCodeActions(context: CodeFixContext): CodeAction[] | undefined;
getCodeActions(context: CodeFixContext): CodeFix[] | undefined;
groupIds: string[];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a better name is actionIds or actionCodes.

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

export interface CodeFixAllContext extends CodeFixContextBase {
groupId: {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@amcasey amcasey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/* @internal */
GetCodeFixesFull = "getCodeFixes-full",
GetCombinedCodeFix = "getCombinedCodeFix",
/* @internal */
GetCombinedCodeFixFull = "getCombinedCodeFix",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"getCombinedCodeFix-full"?

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

export interface CodeActionAll {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CombinedCodeActions?

getCodeActions(context: CodeFixContext): CodeAction[] | undefined;
getCodeActions(context: CodeFixContext): CodeFix[] | undefined;
actionIds: string[];
getAllCodeActions(context: CodeFixAllContext): CodeActionAll;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should make this one optional

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or make CodeFixRegisraction a union type

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@amcasey
Copy link
Member

amcasey commented Dec 1, 2017

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?


export function registerCodeFix(codeFix: CodeFix) {
forEach(codeFix.errorCodes, error => {
export function registerCodeFix(codeFix: CodeFixRegistration) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@amcasey amcasey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

});
}
if (codeFix.actionIds) {
for (const gid of codeFix.actionIds) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gid is out of date

@ghost
Copy link
Author

ghost commented Dec 1, 2017

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
Copy link
Member

amcasey commented Dec 1, 2017

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
Copy link
Member

amcasey commented Dec 1, 2017

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

@mhegazy
Copy link
Contributor

mhegazy commented Dec 2, 2017

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
Copy link
Member

amcasey commented Dec 2, 2017

@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
Copy link
Member

amcasey commented Dec 6, 2017

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
Copy link
Member

amcasey commented Dec 6, 2017

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 ghost mentioned this pull request Dec 7, 2017
@ghost
Copy link
Author

ghost commented Dec 7, 2017

@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.

@@ -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[] {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the return type be ReadonlyArray ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

amcasey commented Dec 7, 2017

@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
Copy link
Author

ghost commented Dec 7, 2017

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

@amcasey
Copy link
Member

amcasey commented Dec 7, 2017

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.

This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix all in file for import fix Proposal: 'Fix All In File' for CodeFixes
3 participants