Skip to content

Support a "getCombinedCodeFix" service #20338

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.

Sign up for GitHub

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

Merged
15 commits merged into from
Dec 7, 2017
Merged

Support a "getCombinedCodeFix" service #20338

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
5c70641
Support a "getCombinedCodeFix" service
Nov 13, 2017
be19542
Rename things
Nov 30, 2017
8a1865f
Code review
Nov 30, 2017
b89b7fe
Merge branch 'master' into codeFixAll
Nov 30, 2017
a8b3afc
Rename things
Dec 1, 2017
a38216c
Update API baselines
Dec 1, 2017
c621e3f
CodeActionAll -> CombinedCodeActions
Dec 1, 2017
d0af16a
Merge branch 'master' into codeFixAll
Dec 5, 2017
2db8649
Take a `scope` parameter instead of `fileName` for flexibility
Dec 5, 2017
f2031be
Renames and bugfixes
Dec 5, 2017
79c0150
Merge branch 'master' into codeFixAll
Dec 6, 2017
8a61761
Make API changes internal
Dec 7, 2017
41c5710
Merge branch 'master' into codeFixAll
Dec 7, 2017
87d30c0
Code review
Dec 7, 2017
5f7ba73
Update comment
Dec 7, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions src/compiler/core.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,13 +215,27 @@ namespace ts {

export function zipWith<T, U, V>(arrayA: ReadonlyArray<T>, arrayB: ReadonlyArray<U>, callback: (a: T, b: U, index: number) => V): V[] {
const result: V[] = [];
Debug.assert(arrayA.length === arrayB.length);
Debug.assertEqual(arrayA.length, arrayB.length);
for (let i = 0; i < arrayA.length; i++) {
result.push(callback(arrayA[i], arrayB[i], i));
}
return result;
}

export function zipToIterator<T, U>(arrayA: ReadonlyArray<T>, arrayB: ReadonlyArray<U>): Iterator<[T, U]> {
Debug.assertEqual(arrayA.length, arrayB.length);
let i = 0;
return {
next() {
if (i === arrayA.length) {
return { value: undefined as never, done: true };
}
i++;
return { value: [arrayA[i - 1], arrayB[i - 1]], done: false };
}
};
}

export function zipToMap<T>(keys: ReadonlyArray<string>, values: ReadonlyArray<T>): Map<T> {
Debug.assert(keys.length === values.length);
const map = createMap<T>();
Expand Down Expand Up @@ -1345,7 +1359,6 @@ namespace ts {
this.set(key, values = [value]);
}
return values;

}
function multiMapRemove<T>(this: MultiMap<T>, key: string, value: T) {
const values = this.get(key);
Expand Down
12 changes: 6 additions & 6 deletions src/compiler/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -949,22 +949,22 @@ namespace ts {

export interface ConstructorDeclaration extends FunctionLikeDeclarationBase, ClassElement, JSDocContainer {
kind: SyntaxKind.Constructor;
parent?: ClassDeclaration | ClassExpression;
parent?: ClassLikeDeclaration;
body?: FunctionBody;
/* @internal */ returnFlowNode?: FlowNode;
}

/** For when we encounter a semicolon in a class declaration. ES6 allows these as class elements. */
export interface SemicolonClassElement extends ClassElement {
kind: SyntaxKind.SemicolonClassElement;
parent?: ClassDeclaration | ClassExpression;
parent?: ClassLikeDeclaration;
}

// See the comment on MethodDeclaration for the intuition behind GetAccessorDeclaration being a
// ClassElement and an ObjectLiteralElement.
export interface GetAccessorDeclaration extends FunctionLikeDeclarationBase, ClassElement, ObjectLiteralElement, JSDocContainer {
kind: SyntaxKind.GetAccessor;
parent?: ClassDeclaration | ClassExpression | ObjectLiteralExpression;
parent?: ClassLikeDeclaration | ObjectLiteralExpression;
name: PropertyName;
body: FunctionBody;
}
Expand All @@ -973,7 +973,7 @@ namespace ts {
// ClassElement and an ObjectLiteralElement.
export interface SetAccessorDeclaration extends FunctionLikeDeclarationBase, ClassElement, ObjectLiteralElement, JSDocContainer {
kind: SyntaxKind.SetAccessor;
parent?: ClassDeclaration | ClassExpression | ObjectLiteralExpression;
parent?: ClassLikeDeclaration | ObjectLiteralExpression;
name: PropertyName;
body: FunctionBody;
}
Expand All @@ -982,7 +982,7 @@ namespace ts {

export interface IndexSignatureDeclaration extends SignatureDeclarationBase, ClassElement, TypeElement {
kind: SyntaxKind.IndexSignature;
parent?: ClassDeclaration | ClassExpression | InterfaceDeclaration | TypeLiteralNode;
parent?: ClassLikeDeclaration | InterfaceDeclaration | TypeLiteralNode;
}

export interface TypeNode extends Node {
Expand Down Expand Up @@ -1986,7 +1986,7 @@ namespace ts {

export interface HeritageClause extends Node {
kind: SyntaxKind.HeritageClause;
parent?: InterfaceDeclaration | ClassDeclaration | ClassExpression;
parent?: InterfaceDeclaration | ClassLikeDeclaration;
token: SyntaxKind.ExtendsKeyword | SyntaxKind.ImplementsKeyword;
types: NodeArray<ExpressionWithTypeArguments>;
}
Expand Down
44 changes: 33 additions & 11 deletions src/harness/fourslash.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2371,7 +2371,7 @@ Actual: ${stringify(fullActual)}`);
*/
public getAndApplyCodeActions(errorCode?: number, index?: number) {
const fileName = this.activeFile.fileName;
this.applyCodeActions(this.getCodeFixActions(fileName, errorCode), index);
this.applyCodeActions(this.getCodeFixes(fileName, errorCode), index);
}

public applyCodeActionFromCompletion(markerName: string, options: FourSlashInterface.VerifyCompletionActionOptions) {
Expand Down Expand Up @@ -2424,6 +2424,16 @@ Actual: ${stringify(fullActual)}`);
this.verifyRangeIs(expectedText, includeWhiteSpace);
}

public verifyCodeFixAll(options: FourSlashInterface.VerifyCodeFixAllOptions): void {
const { groupId, newFileContent } = options;
const groupIds = ts.mapDefined(this.getCodeFixes(this.activeFile.fileName), a => a.groupId);
ts.Debug.assert(ts.contains(groupIds, groupId), "No available code fix has that group id.", () => `Expected '${groupId}'. Available group ids: ${groupIds}`);
const { changes, commands } = this.languageService.getCombinedCodeFix(this.activeFile.fileName, groupId, 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.

}

/**
* Applies fixes for the errors in fileName and compares the results to
* expectedContents after all fixes have been applied.
Expand All @@ -2436,7 +2446,7 @@ Actual: ${stringify(fullActual)}`);
public verifyFileAfterCodeFix(expectedContents: string, fileName?: string) {
fileName = fileName ? fileName : this.activeFile.fileName;

this.applyCodeActions(this.getCodeFixActions(fileName));
this.applyCodeActions(this.getCodeFixes(fileName));

const actualContents: string = this.getFileContent(fileName);
if (this.removeWhitespace(actualContents) !== this.removeWhitespace(expectedContents)) {
Expand All @@ -2446,7 +2456,7 @@ Actual: ${stringify(fullActual)}`);

public verifyCodeFix(options: FourSlashInterface.VerifyCodeFixOptions) {
const fileName = this.activeFile.fileName;
const actions = this.getCodeFixActions(fileName, options.errorCode);
const actions = this.getCodeFixes(fileName, options.errorCode);
let index = options.index;
if (index === undefined) {
if (!(actions && actions.length === 1)) {
Expand All @@ -2472,8 +2482,8 @@ Actual: ${stringify(fullActual)}`);
}

private verifyNewContent(options: FourSlashInterface.NewContentOptions) {
if (options.newFileContent) {
assert(!options.newRangeContent);
if (options.newFileContent !== undefined) {
assert(options.newRangeContent === undefined);
this.verifyCurrentFileContent(options.newFileContent);
}
else {
Expand All @@ -2485,7 +2495,7 @@ Actual: ${stringify(fullActual)}`);
* Rerieves a codefix satisfying the parameters, or undefined if no such codefix is found.
* @param fileName Path to file where error should be retrieved from.
*/
private getCodeFixActions(fileName: string, errorCode?: number): ts.CodeAction[] {
private getCodeFixes(fileName: string, errorCode?: number): ts.CodeFix[] {
const diagnosticsForCodeFix = this.getDiagnostics(fileName).map(diagnostic => ({
start: diagnostic.start,
length: diagnostic.length,
Expand All @@ -2501,7 +2511,7 @@ Actual: ${stringify(fullActual)}`);
});
}

private applyCodeActions(actions: ts.CodeAction[], index?: number): void {
private applyCodeActions(actions: ReadonlyArray<ts.CodeAction>, index?: number): void {
if (index === undefined) {
if (!(actions && actions.length === 1)) {
this.raiseError(`Should find exactly one codefix, but ${actions ? actions.length : "none"} found. ${actions ? actions.map(a => `${Harness.IO.newLine()} "${a.description}"`) : ""}`);
Expand All @@ -2514,8 +2524,10 @@ Actual: ${stringify(fullActual)}`);
}
}

const changes = actions[index].changes;
this.applyChanges(actions[index].changes);
}

private applyChanges(changes: ReadonlyArray<ts.FileTextChanges>): void {
for (const change of changes) {
this.applyEdits(change.fileName, change.textChanges, /*isFormattingEdit*/ false);
}
Expand All @@ -2527,7 +2539,7 @@ Actual: ${stringify(fullActual)}`);
this.raiseError("At least one range should be specified in the testfile.");
}

const codeFixes = this.getCodeFixActions(this.activeFile.fileName, errorCode);
const codeFixes = this.getCodeFixes(this.activeFile.fileName, errorCode);

if (codeFixes.length === 0) {
if (expectedTextArray.length !== 0) {
Expand Down Expand Up @@ -2866,7 +2878,7 @@ Actual: ${stringify(fullActual)}`);
}

public verifyCodeFixAvailable(negative: boolean, info: FourSlashInterface.VerifyCodeFixAvailableOptions[] | undefined) {
const codeFixes = this.getCodeFixActions(this.activeFile.fileName);
const codeFixes = this.getCodeFixes(this.activeFile.fileName);

if (negative) {
if (codeFixes.length) {
Expand Down Expand Up @@ -3033,7 +3045,7 @@ Actual: ${stringify(fullActual)}`);
}

public printAvailableCodeFixes() {
const codeFixes = this.getCodeFixActions(this.activeFile.fileName);
const codeFixes = this.getCodeFixes(this.activeFile.fileName);
Harness.IO.log(stringify(codeFixes));
}

Expand Down Expand Up @@ -4143,6 +4155,10 @@ namespace FourSlashInterface {
this.state.verifyRangeAfterCodeFix(expectedText, includeWhiteSpace, errorCode, index);
}

public codeFixAll(options: VerifyCodeFixAllOptions): void {
this.state.verifyCodeFixAll(options);
}

public fileAfterApplyingRefactorAtMarker(markerName: string, expectedContent: string, refactorNameToApply: string, actionName: string, formattingOptions?: ts.FormatCodeSettings): void {
this.state.verifyFileAfterApplyingRefactorAtMarker(markerName, expectedContent, refactorNameToApply, actionName, formattingOptions);
}
Expand Down Expand Up @@ -4578,6 +4594,12 @@ namespace FourSlashInterface {
commands?: ts.CodeActionCommand[];
}

export interface VerifyCodeFixAllOptions {
groupId: string;
newFileContent: string;
commands: ReadonlyArray<{}>;
}

export interface VerifyRefactorOptions {
name: string;
actionName: string;
Expand Down
3 changes: 2 additions & 1 deletion src/harness/harnessLanguageService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -503,9 +503,10 @@ namespace Harness.LanguageService {
getSpanOfEnclosingComment(fileName: string, position: number, onlyMultiLine: boolean): ts.TextSpan {
return unwrapJSONCallResult(this.shim.getSpanOfEnclosingComment(fileName, position, onlyMultiLine));
}
getCodeFixesAtPosition(): ts.CodeAction[] {
getCodeFixesAtPosition(): never {
throw new Error("Not supported on the shim.");
}
getCombinedCodeFix = ts.notImplemented;
applyCodeActionCommand = ts.notImplemented;
getCodeFixDiagnostics(): ts.Diagnostic[] {
throw new Error("Not supported on the shim.");
Expand Down
21 changes: 10 additions & 11 deletions src/server/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ namespace ts.server {
const response = this.processResponse<protocol.CompletionDetailsResponse>(request);
Debug.assert(response.body.length === 1, "Unexpected length of completion details response body.");

const convertedCodeActions = map(response.body[0].codeActions, codeAction => this.convertCodeActions(codeAction, fileName));
const convertedCodeActions = map(response.body[0].codeActions, ({ description, changes }) => ({ description, changes: this.convertChanges(changes, fileName) }));
return { ...response.body[0], codeActions: convertedCodeActions };
}

Expand Down Expand Up @@ -552,15 +552,17 @@ namespace ts.server {
return notImplemented();
}

getCodeFixesAtPosition(file: string, start: number, end: number, errorCodes: number[]): CodeAction[] {
getCodeFixesAtPosition(file: string, start: number, end: number, errorCodes: number[]): CodeFix[] {
const args: protocol.CodeFixRequestArgs = { ...this.createFileRangeRequestArgs(file, start, end), errorCodes };

const request = this.processRequest<protocol.CodeFixRequest>(CommandNames.GetCodeFixes, args);
const response = this.processResponse<protocol.CodeFixResponse>(request);

return response.body.map(entry => this.convertCodeActions(entry, file));
return response.body.map(({ description, changes, groupId }) => ({ description, changes: this.convertChanges(changes, file), groupId }));
}

getCombinedCodeFix = notImplemented;

applyCodeActionCommand = notImplemented;

private createFileLocationOrRangeRequestArgs(positionOrRange: number | TextRange, fileName: string): protocol.FileLocationOrRangeRequestArgs {
Expand Down Expand Up @@ -637,14 +639,11 @@ namespace ts.server {
});
}

convertCodeActions(entry: protocol.CodeAction, fileName: string): CodeAction {
return {
description: entry.description,
changes: entry.changes.map(change => ({
fileName: change.fileName,
textChanges: change.textChanges.map(textChange => this.convertTextChangeToCodeEdit(textChange, fileName))
}))
};
private convertChanges(changes: protocol.FileCodeEdits[], fileName: string): FileTextChanges[] {
return changes.map(change => ({
fileName: change.fileName,
textChanges: change.textChanges.map(textChange => this.convertTextChangeToCodeEdit(textChange, fileName))
}));
}

convertTextChangeToCodeEdit(change: protocol.CodeEdit, fileName: string): ts.TextChange {
Expand Down
26 changes: 24 additions & 2 deletions src/server/protocol.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,9 +99,12 @@ namespace ts.server.protocol {
BreakpointStatement = "breakpointStatement",
CompilerOptionsForInferredProjects = "compilerOptionsForInferredProjects",
GetCodeFixes = "getCodeFixes",
ApplyCodeActionCommand = "applyCodeActionCommand",
/* @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"?

ApplyCodeActionCommand = "applyCodeActionCommand",
GetSupportedCodeFixes = "getSupportedCodeFixes",

GetApplicableRefactors = "getApplicableRefactors",
Expand Down Expand Up @@ -533,6 +536,11 @@ namespace ts.server.protocol {
arguments: CodeFixRequestArgs;
}

export interface GetCombinedCodeFixRequest extends Request {
command: CommandTypes.GetCombinedCodeFix;
arguments: GetCombinedCodeFixRequestArgs;
}

export interface ApplyCodeActionCommandRequest extends Request {
command: CommandTypes.ApplyCodeActionCommand;
arguments: ApplyCodeActionCommandRequestArgs;
Expand Down Expand Up @@ -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

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?

}

export interface ApplyCodeActionCommandRequestArgs {
/** May also be an array of commands. */
command: {};
Expand Down Expand Up @@ -1568,7 +1580,7 @@ namespace ts.server.protocol {

export interface CodeFixResponse extends Response {
/** The code actions that are available */
body?: CodeAction[];
body?: CodeFix[];
}

export interface CodeAction {
Expand All @@ -1580,6 +1592,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?

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.

}

/**
* Format and format on key response message.
*/
Expand Down
Loading