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

Add infrastructure for refactors #14624

Closed
wants to merge 16 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
71 changes: 71 additions & 0 deletions src/harness/fourslash.ts
Original file line number Diff line number Diff line change
Expand Up @@ -485,6 +485,10 @@ namespace FourSlash {
return diagnostics;
}

private getRefactorDiagnostics(fileName: string, range?: ts.TextRange): ts.RefactorDiagnostic[] {
return this.languageService.getRefactorDiagnostics(fileName, range);
}

private getAllDiagnostics(): ts.Diagnostic[] {
const diagnostics: ts.Diagnostic[] = [];

Expand Down Expand Up @@ -2193,6 +2197,22 @@ namespace FourSlash {
return actions;
}

private getRefactorActions(fileName: string, range?: ts.TextRange, formattingOptions?: ts.FormatCodeSettings): ts.CodeAction[] {
const diagnostics = this.getRefactorDiagnostics(fileName, range);
const actions: ts.CodeAction[] = [];
formattingOptions = formattingOptions || this.formatCodeSettings;

for (const diagnostic of diagnostics) {
const diagnosticRange: ts.TextRange = {
pos: diagnostic.start,
end: diagnostic.end
};
const newActions = this.languageService.getCodeActionsForRefactorAtPosition(fileName, diagnosticRange, diagnostic.code, formattingOptions);
actions.push.apply(actions, newActions);
}
return actions;
}

private applyCodeAction(fileName: string, actions: ts.CodeAction[], index?: number): void {
if (index === undefined) {
if (!(actions && actions.length === 1)) {
Expand Down Expand Up @@ -2551,6 +2571,49 @@ namespace FourSlash {
}
}

public verifyRefactorAvailable(negative: boolean) {
// The ranges are used only when the refactors require a range as input information. For example the "extractMethod" refactor
// onlye one range is allowed per test
const ranges = this.getRanges();
if (ranges.length > 1) {
throw new Error("only one refactor range is allowed per test");
}

const range = ranges[0] ? { pos: ranges[0].start, end: ranges[0].end } : undefined;
const refactorDiagnostics = this.getRefactorDiagnostics(this.activeFile.fileName, range);
if (negative && refactorDiagnostics.length > 0) {
this.raiseError(`verifyRefactorAvailable failed - expected no refactors but found some.`);
}
if (!negative && refactorDiagnostics.length === 0) {
this.raiseError(`verifyRefactorAvailable failed: expected refactor diagnostics but none found.`);
}
}

public verifyFileAfterApplyingRefactors(expectedContent: string, formattingOptions?: ts.FormatCodeSettings) {
const ranges = this.getRanges();
if (ranges.length > 1) {
throw new Error("only one refactor range is allowed per test");
}

const range = ranges[0] ? { pos: ranges[0].start, end: ranges[0].end } : undefined;
const actions = this.getRefactorActions(this.activeFile.fileName, range, formattingOptions);

// Each refactor diagnostic will return one code action, but multiple refactor diagnostics can point to the same
// code action. For example in the case of "convert function to es6 class":
//
// function foo() { }
// ^^^
// foo.prototype.getName = function () { return "name"; };
// ^^^
// These two diagnostics result in the same code action, so we only apply the first one.
this.applyCodeAction(this.activeFile.fileName, actions, /*index*/ 0);
const actualContent = this.getFileContent(this.activeFile.fileName);

if (this.normalizeNewlines(actualContent) !== this.normalizeNewlines(expectedContent)) {
this.raiseError(`verifyFileAfterApplyingRefactors failed: expected:\n${expectedContent}\nactual:\n${actualContent}`);
}
}

// Get the text of the entire line the caret is currently at
private getCurrentLineContent() {
const text = this.getFileContent(this.activeFile.fileName);
Expand Down Expand Up @@ -3342,6 +3405,10 @@ namespace FourSlashInterface {
public codeFixAvailable() {
this.state.verifyCodeFixAvailable(this.negative);
}

public refactorAvailable() {
this.state.verifyRefactorAvailable(this.negative);
}
}

export class Verify extends VerifyNegatable {
Expand Down Expand Up @@ -3548,6 +3615,10 @@ namespace FourSlashInterface {
this.state.verifyRangeAfterCodeFix(expectedText, includeWhiteSpace, errorCode, index);
}

public fileAfterApplyingRefactors(expectedContent: string, formattingOptions: ts.FormatCodeSettings): void {
this.state.verifyFileAfterApplyingRefactors(expectedContent, formattingOptions);
}

public importFixAtPosition(expectedTextArray: string[], errorCode?: number): void {
this.state.verifyImportFixAtPosition(expectedTextArray, errorCode);
}
Expand Down
6 changes: 6 additions & 0 deletions src/harness/harnessLanguageService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -489,6 +489,12 @@ namespace Harness.LanguageService {
getCodeFixesAtPosition(): ts.CodeAction[] {
throw new Error("Not supported on the shim.");
}
getRefactorDiagnostics(): ts.RefactorDiagnostic[] {
throw new Error("Not supported on the shim.");
}
getCodeActionsForRefactorAtPosition(): ts.CodeAction[] {
throw new Error("Not supported on the shim.");
}
getEmitOutput(fileName: string): ts.EmitOutput {
return unwrapJSONCallResult(this.shim.getEmitOutput(fileName));
}
Expand Down
16 changes: 14 additions & 2 deletions src/harness/unittests/tsserverProjectSystem.ts
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,7 @@ namespace ts.projectSystem {
this.map[timeoutId] = cb.bind(undefined, ...args);
return timeoutId;
}

unregister(id: any) {
if (typeof id === "number") {
delete this.map[id];
Expand All @@ -328,10 +329,13 @@ namespace ts.projectSystem {
}

invoke() {
// Note: invoking a callback may result in new callbacks been queued,
// so do not clear the entire callback list regardless. Only remove the
// ones we have invoked.
for (const key in this.map) {
this.map[key]();
delete this.map[key];
}
this.map = [];
}
}

Expand Down Expand Up @@ -3446,10 +3450,18 @@ namespace ts.projectSystem {
assert.equal(e1.event, "syntaxDiag");
host.clearOutput();

// the semanticDiag message
host.runQueuedImmediateCallbacks();
assert.equal(host.getOutput().length, 2, "expect 2 messages");
assert.equal(host.getOutput().length, 1, "expect 1 messages");
Copy link
Contributor

Choose a reason for hiding this comment

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

"messages" should be "message"

const e2 = <protocol.Event>getMessage(0);
assert.equal(e2.event, "semanticDiag");
host.clearOutput();

// the refactor diagnostics check
host.runQueuedImmediateCallbacks();
assert.equal(host.getOutput().length, 2, "expect 2 messages");
const e3 = <protocol.Event>getMessage(0);
assert.equal(e3.event, "refactorDiag");
verifyRequestCompleted(getErrId, 1);

requestToCancel = -1;
Expand Down
44 changes: 44 additions & 0 deletions src/server/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -692,6 +692,50 @@ namespace ts.server {
return response.body.map(entry => this.convertCodeActions(entry, fileName));
}

getRefactorDiagnostics(fileName: string, range?: TextRange): RefactorDiagnostic[] {
const startLineOffset = this.positionToOneBasedLineOffset(fileName, range.pos);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this crash if range === undefined ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here the range is not supposed to be nullable, as we only use it to test the session API GetRefactorsForRange. Will update

const endLineOffset = this.positionToOneBasedLineOffset(fileName, range.end);

const args: protocol.GetRefactorsForRangeRequestArgs = {
file: fileName,
startLine: startLineOffset.line,
startOffset: startLineOffset.offset,
endLine: endLineOffset.line,
endOffset: endLineOffset.offset,
};

const request = this.processRequest<protocol.GetRefactorsForRangeRequest>(CommandNames.GetRefactorsForRange, args);
const response = this.processResponse<protocol.GetRefactorsForRangeResponse>(request);

return response.body.map(entry => {
return <RefactorDiagnostic>{
code: entry.code,
end: this.lineOffsetToPosition(fileName, entry.end),
start: this.lineOffsetToPosition(fileName, entry.start),
text: entry.text
};
});
}

getCodeActionsForRefactorAtPosition(fileName: string, range: TextRange, refactorCode: number): CodeAction[] {
const startLineOffset = this.positionToOneBasedLineOffset(fileName, range.pos);
const endLineOffset = this.positionToOneBasedLineOffset(fileName, range.end);

const args: protocol.GetCodeActionsForRefactorRequestArgs = {
file: fileName,
startLine: startLineOffset.line,
startOffset: startLineOffset.offset,
endLine: endLineOffset.line,
endOffset: endLineOffset.offset,
refactorCode
};

const request = this.processRequest<protocol.GetCodeActionsForRefactorRequest>(CommandNames.GetCodeActionsForRefactor, args);
const response = this.processResponse<protocol.GetCodeActionsForRefactorResponse>(request);

return response.body.map(entry => this.convertCodeActions(entry, fileName));
}

convertCodeActions(entry: protocol.CodeAction, fileName: string): CodeAction {
return {
description: entry.description,
Expand Down
62 changes: 58 additions & 4 deletions src/server/protocol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,9 @@ namespace ts.server.protocol {
/* @internal */
export type GetCodeFixesFull = "getCodeFixes-full";
export type GetSupportedCodeFixes = "getSupportedCodeFixes";

export type GetRefactorsForRange = "getRefactorsForRange";
export type GetCodeActionsForRefactor = "getCodeActionsForRefactor";
}

/**
Expand Down Expand Up @@ -394,6 +397,55 @@ namespace ts.server.protocol {
position?: number;
}

/**
* An diagnostic information suggesting refactors at applicable positions without
* clients asking.
*/
export interface RefactorDiagnostic {
text: string;
code: number;
start: Location;
end: Location;
}

export interface RefactorDiagnosticEventBody {
file: string;
diagnostics: RefactorDiagnostic[];
}

/**
* Returns a list of applicable refactors at a given position. This request does not actually
* compute the refactors; instead it goes through faster checks to determine what is possible.
*/
export interface GetRefactorsForRangeRequest extends Request {
command: CommandTypes.GetRefactorsForRange;
arguments: GetRefactorsForRangeRequestArgs;
}

export interface GetRefactorsForRangeRequestArgs extends TextRangeRequestArgs {
}

export interface GetRefactorsForRangeResponse extends Response {
body?: RefactorDiagnostic[];
}

/**
* Computes the code actions for a given refactor. This is normally called after the user commited one
* of the applicable refactors provided by the "GetApplicableRefactors" API.
*/
export interface GetCodeActionsForRefactorRequest extends Request {
command: CommandTypes.GetCodeActionsForRefactor;
arguments: GetCodeActionsForRefactorRequestArgs;
}

export interface GetCodeActionsForRefactorRequestArgs extends GetRefactorsForRangeRequestArgs {
refactorCode: number;
}

export interface GetCodeActionsForRefactorResponse extends Response {
body?: CodeAction[];
Copy link
Contributor

Choose a reason for hiding this comment

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

do not use an array. make it an map.

}

/**
* Request for the available codefixes at a specific position.
*/
Expand All @@ -402,10 +454,7 @@ namespace ts.server.protocol {
arguments: CodeFixRequestArgs;
}

/**
* Instances of this interface specify errorcodes on a specific location in a sourcefile.
*/
export interface CodeFixRequestArgs extends FileRequestArgs {
export interface TextRangeRequestArgs 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.

Can you name the first two members line and offset to more closely coincide with FileLocationRequestArgs andLocation? Or use a Location for the start and end?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe FileRangeRequestArgs would fit better. But using location for start and end would be an unnecessary breaking change for editors already consuming this type.

/**
* The line number for the request (1-based).
*/
Expand Down Expand Up @@ -437,7 +486,12 @@ namespace ts.server.protocol {
*/
/* @internal */
endPosition?: number;
}

/**
* Instances of this interface specify errorcodes on a specific location in a sourcefile.
*/
export interface CodeFixRequestArgs extends TextRangeRequestArgs {
/**
* Errorcodes we want to get the fixes for.
*/
Expand Down
Loading