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.

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
17 changes: 15 additions & 2 deletions src/compiler/core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -228,13 +228,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 @@ -1385,7 +1399,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
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
40 changes: 31 additions & 9 deletions src/harness/fourslash.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2376,7 +2376,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 @@ -2429,6 +2429,16 @@ Actual: ${stringify(fullActual)}`);
this.verifyRangeIs(expectedText, includeWhiteSpace);
}

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

}

/**
* Applies fixes for the errors in fileName and compares the results to
* expectedContents after all fixes have been applied.
Expand All @@ -2441,7 +2451,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 @@ -2451,7 +2461,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 Down Expand Up @@ -2490,7 +2500,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.CodeFixAction[] {
const diagnosticsForCodeFix = this.getDiagnostics(fileName).map(diagnostic => ({
start: diagnostic.start,
length: diagnostic.length,
Expand All @@ -2506,7 +2516,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 @@ -2519,8 +2529,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 @@ -2532,7 +2544,7 @@ Actual: ${stringify(fullActual)}`);
this.raiseError("Exactly 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 @@ -2871,7 +2883,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 @@ -3038,7 +3050,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 @@ -4149,6 +4161,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 @@ -4584,6 +4600,12 @@ namespace FourSlashInterface {
commands?: ts.CodeActionCommand[];
}

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

export interface VerifyRefactorOptions {
name: string;
actionName: string;
Expand Down
4 changes: 2 additions & 2 deletions src/harness/harness.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@ namespace assert {
export function isFalse(expr: boolean, msg = "Expected value to be false."): void {
assert(!expr, msg);
}
export function equal<T>(a: T, b: T, msg = "Expected values to be equal."): void {
assert(a === b, msg);
export function equal<T>(a: T, b: T, msg?: string): void {
assert(a === b, msg || (() => `Expected to be equal:\nExpected:\n${JSON.stringify(a)}\nActual:\n${JSON.stringify(b)}`));
}
export function notEqual<T>(a: T, b: T, msg = "Expected values to not be equal."): void {
assert(a !== b, msg);
Expand Down
3 changes: 2 additions & 1 deletion src/harness/harnessLanguageService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -521,9 +521,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
22 changes: 11 additions & 11 deletions src/server/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,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 @@ -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 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));
// TODO: GH#20538 shouldn't need cast
return (response.body as protocol.CodeFixAction[]).map(({ description, changes, fixId }) => ({ description, changes: this.convertChanges(changes, file), fixId }));
}

getCombinedCodeFix = notImplemented;

applyCodeActionCommand = notImplemented;

private createFileLocationOrRangeRequestArgs(positionOrRange: number | TextRange, fileName: string): protocol.FileLocationOrRangeRequestArgs {
Expand Down Expand Up @@ -638,14 +641,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
50 changes: 48 additions & 2 deletions src/server/protocol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,9 +100,14 @@ namespace ts.server.protocol {
BreakpointStatement = "breakpointStatement",
CompilerOptionsForInferredProjects = "compilerOptionsForInferredProjects",
GetCodeFixes = "getCodeFixes",
ApplyCodeActionCommand = "applyCodeActionCommand",
/* @internal */
GetCodeFixesFull = "getCodeFixes-full",
// TODO: GH#20538
/* @internal */
GetCombinedCodeFix = "getCombinedCodeFix",
/* @internal */
GetCombinedCodeFixFull = "getCombinedCodeFix-full",
ApplyCodeActionCommand = "applyCodeActionCommand",
GetSupportedCodeFixes = "getSupportedCodeFixes",

GetApplicableRefactors = "getApplicableRefactors",
Expand Down Expand Up @@ -552,6 +557,19 @@ namespace ts.server.protocol {
arguments: CodeFixRequestArgs;
}

// TODO: GH#20538
/* @internal */
export interface GetCombinedCodeFixRequest extends Request {
command: CommandTypes.GetCombinedCodeFix;
arguments: GetCombinedCodeFixRequestArgs;
}

// TODO: GH#20538
/* @internal */
export interface GetCombinedCodeFixResponse extends Response {
body: CombinedCodeActions;
}

export interface ApplyCodeActionCommandRequest extends Request {
command: CommandTypes.ApplyCodeActionCommand;
arguments: ApplyCodeActionCommandRequestArgs;
Expand Down Expand Up @@ -604,6 +622,20 @@ namespace ts.server.protocol {
errorCodes?: number[];
}

// TODO: GH#20538
/* @internal */
export interface GetCombinedCodeFixRequestArgs {
scope: GetCombinedCodeFixScope;
fixId: {};
}

// TODO: GH#20538
/* @internal */
export interface GetCombinedCodeFixScope {
type: "file";
args: FileRequestArgs;
}

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

export interface CodeFixResponse extends Response {
/** The code actions that are available */
body?: CodeAction[];
body?: CodeAction[]; // TODO: GH#20538 CodeFixAction[]
}

export interface CodeAction {
Expand All @@ -1599,6 +1631,20 @@ namespace ts.server.protocol {
commands?: {}[];
}

// TODO: GH#20538
/* @internal */
export interface CombinedCodeActions {
changes: FileCodeEdits[];
commands?: {}[];
}

// TODO: GH#20538
/* @internal */
export interface CodeFixAction extends CodeAction {
/** If present, one may call 'getCombinedCodeFix' with this fixId. */
fixId?: {};
}

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