Skip to content

Pvb/codeaction/api #10185

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
merged 29 commits into from
Oct 12, 2016
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
94ea825
Api Changes and simple superfixes
Aug 5, 2016
466d26f
Fourslash support
Aug 5, 2016
6f4fb06
SuperFix fourslash tests
Aug 5, 2016
e0b73c4
Clean up
Aug 5, 2016
124e05f
PR Feedback
Aug 5, 2016
730f31f
Merge branch 'master' into pvb/codeaction/api
Aug 26, 2016
9a26da1
Merge branch 'pvb/codeaction/api' of https://github.com/Microsoft/Typ…
Aug 26, 2016
f931e60
Fix build break caused by merge from master
Aug 26, 2016
66e1c92
Merge branch 'master' into pvb/codeaction/api
Aug 31, 2016
49b65c7
PR feedback
Sep 9, 2016
3df79d5
Merge branch 'master' into pvb/codeaction/api
Sep 16, 2016
682f812
PR feedback
Sep 16, 2016
6e8eb1d
Merge branch 'pvb/codeaction/api' of https://github.com/Microsoft/Typ…
Oct 4, 2016
20dea29
Merge branch 'master' into pvb/codeaction/api
Oct 4, 2016
4f404ad
Implement codefixes in tsserver
Oct 4, 2016
1cc9732
PR feedback
Oct 5, 2016
cf4e300
Merge branch 'master' into pvb/codeaction/api
Oct 5, 2016
ebcfce4
Error span moved from constructor to this keyword.
Oct 5, 2016
163e758
Handle case where this access is inside the supercall
Oct 6, 2016
75e1b80
Use just the errorcode, without the TS prefix
Oct 6, 2016
1e9b653
Fix linting issues
Oct 6, 2016
c29e9b7
PR feedback
Oct 7, 2016
77610f6
rename of the response body
Oct 7, 2016
a83a2b0
Codefixes in client for testing the server.
Oct 7, 2016
a88ceb5
Merge branch 'master' into pvb/codeaction/api
Oct 7, 2016
f310310
Deal with buildbreak
Oct 7, 2016
7c96c28
Fix test failure
Oct 7, 2016
dc516c0
Remove duplicate interface
Oct 12, 2016
9b98d00
Merge branch 'master' into pvb/codeaction/api
Oct 12, 2016
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
9 changes: 9 additions & 0 deletions src/compiler/core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,15 @@ namespace ts {
return -1;
}

export function firstOrUndefined<T>(array: T[], predicate: (x: T) => boolean): T {
Copy link
Contributor

Choose a reason for hiding this comment

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

forEach

Copy link
Contributor

Choose a reason for hiding this comment

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

forEach is slightly different. Also this is usually called find.

for (let i = 0, len = array.length; i < len; i++) {
if (predicate(array[i])) {
return array[i];
}
}
return undefined;
}

export function indexOfAnyCharCode(text: string, charCodes: number[], start?: number): number {
for (let i = start || 0, len = text.length; i < len; i++) {
if (contains(charCodes, text.charCodeAt(i))) {
Expand Down
28 changes: 28 additions & 0 deletions src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -3031,5 +3031,33 @@
"Unknown typing option '{0}'.": {
"category": "Error",
"code": 17010
},
"Add missing 'super()' call.": {
"category": "CodeFix",
"code": 90001
},
"Make 'super()' call the first statement in the constructor.": {
"category": "CodeFix",
"code": 90002
},
"Change 'extends' to 'implements'": {
"category": "CodeFix",
"code": 90003
},
"Remove unused identifiers": {
"category": "CodeFix",
"code": 90004
},
"Implement interface on reference": {
"category": "CodeFix",
"code": 90005
},
"Implement interface on class": {
"category": "CodeFix",
"code": 90006
},
"Implement inherited abstract class": {
"category": "CodeFix",
"code": 90007
}
}
1 change: 1 addition & 0 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2547,6 +2547,7 @@ namespace ts {
Warning,
Error,
Message,
CodeFix,
Copy link
Member

Choose a reason for hiding this comment

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

AFAIK this seems no different than Message. We should probably just reuse Message.

}

export enum ModuleResolutionKind {
Expand Down
51 changes: 48 additions & 3 deletions src/harness/fourslash.ts
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,7 @@ namespace FourSlash {

if (exists !== negative) {
this.printErrorLog(negative, this.getAllDiagnostics());
throw new Error("Failure between markers: " + startMarkerName + ", " + endMarkerName);
throw new Error(`Failure between markers: '${startMarkerName}', '${endMarkerName}'`);
}
}

Expand Down Expand Up @@ -638,7 +638,6 @@ namespace FourSlash {
}
}


public verifyCompletionListAllowsNewIdentifier(negative: boolean) {
const completions = this.getCompletionListAtCaret();

Expand Down Expand Up @@ -1479,7 +1478,7 @@ namespace FourSlash {
if (isFormattingEdit) {
const newContent = this.getFileContent(fileName);

if (newContent.replace(/\s/g, "") !== oldContent.replace(/\s/g, "")) {
if (this.removeWhitespace(newContent) !== this.removeWhitespace(oldContent)) {
this.raiseError("Formatting operation destroyed non-whitespace content");
}
}
Expand Down Expand Up @@ -1545,6 +1544,10 @@ namespace FourSlash {
}
}

private removeWhitespace(text: string): string {
return text.replace(/\s/g, "");
}

public goToBOF() {
this.goToPosition(0);
}
Expand Down Expand Up @@ -1862,6 +1865,44 @@ namespace FourSlash {
}
}

public verifyCodeFixAtPosition(expectedText: string, errorCode?: number) {
Copy link
Contributor

Choose a reason for hiding this comment

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

verifyCodeActionAtPosition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CodeFix.. CodeAction includes both fixes and refactorings, this only verifies fixes


const ranges = this.getRanges();
if (ranges.length == 0) {
this.raiseError("At least one range should be specified in the testfile.");
}

const fileName = this.activeFile.fileName;
const diagnostics = this.getDiagnostics(fileName);

if (diagnostics.length === 0) {
this.raiseError("Errors expected.");
}

if (diagnostics.length > 1 && !errorCode) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can errorCode be 0 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure I'll check for undefined

this.raiseError("When there's more than one error, you must specify the errror to fix.");
}

const diagnostic = !errorCode ? diagnostics[0] : ts.firstOrUndefined(diagnostics, d => d.code == errorCode);

const actual = this.languageService.getCodeFixesAtPosition(fileName, diagnostic.start, diagnostic.length, [`TS${diagnostic.code}`]);
Copy link
Contributor

Choose a reason for hiding this comment

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

should not need the TS prefix here as well.


if (!actual || actual.length == 0) {
this.raiseError("No codefixes returned.");
}

if (actual.length > 1) {
this.raiseError("More than 1 codefix returned.");
}

this.applyEdits(actual[0].changes[0].fileName, actual[0].changes[0].textChanges, /*isFormattingEdit*/ false);
const actualText = this.rangeText(ranges[0]);

if (this.removeWhitespace(actualText) !== this.removeWhitespace(expectedText)) {
this.raiseError(`Actual text doesn't match expected text. Actual: '${actualText}' Expected: '${expectedText}'`);
}
}

public verifyDocCommentTemplate(expected?: ts.TextInsertion) {
const name = "verifyDocCommentTemplate";
const actual = this.languageService.getDocCommentTemplateAtPosition(this.activeFile.fileName, this.currentCaretPosition);
Expand Down Expand Up @@ -3066,6 +3107,10 @@ namespace FourSlashInterface {
this.DocCommentTemplate(/*expectedText*/ undefined, /*expectedOffset*/ undefined, /*empty*/ true);
}

public codeFixAtPosition(expectedText: string, errorCode?: number): void {
this.state.verifyCodeFixAtPosition(expectedText, errorCode);
}

public navigationBar(json: any) {
this.state.verifyNavigationBar(json);
}
Expand Down
3 changes: 3 additions & 0 deletions src/harness/harnessLanguageService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -453,6 +453,9 @@ namespace Harness.LanguageService {
isValidBraceCompletionAtPosition(fileName: string, position: number, openingBrace: number): boolean {
return unwrapJSONCallResult(this.shim.isValidBraceCompletionAtPosition(fileName, position, openingBrace));
}
getCodeFixesAtPosition(fileName: string, start: number, end: number, errorCodes: string[]): ts.CodeAction[] {
return unwrapJSONCallResult(this.shim.getCodeFixesAtPosition(fileName, start, end, JSON.stringify(errorCodes)));
}
getEmitOutput(fileName: string): ts.EmitOutput {
return unwrapJSONCallResult(this.shim.getEmitOutput(fileName));
}
Expand Down
4 changes: 4 additions & 0 deletions src/server/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -592,6 +592,10 @@ namespace ts.server {
throw new Error("Not Implemented Yet.");
}

getCodeFixesAtPosition(fileName: string, start: number, end: number, errorCodes: string[]): ts.CodeAction[] {
throw new Error("Not Implemented Yet.");
Copy link
Contributor

Choose a reason for hiding this comment

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

why? add implementation here, and add a test in tests\cases\fourslash\server

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}

getBraceMatchingAtPosition(fileName: string, position: number): TextSpan[] {
const lineOffset = this.positionToOneBasedLineOffset(fileName, position);
const args: protocol.FileLocationRequestArgs = {
Expand Down
53 changes: 53 additions & 0 deletions src/services/codefixes/codeFixProvider.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
/* @internal */
namespace ts {
export interface CodeFix {
name: string;
Copy link
Member

Choose a reason for hiding this comment

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

If I'm not mistaken, name is unused. If so, then there's no real reason for it, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's very useful for debugging...

Copy link
Member

Choose a reason for hiding this comment

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

Aren't the error codes handled and the descriptions returned by the function descriptive enough for debugging? 😃

errorCodes: string[];
getCodeActions(context: CodeFixContext): CodeAction[];
Copy link
Contributor

Choose a reason for hiding this comment

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

getCodeActions(context: CodeFixContext): CodeAction[] | undefined to allow strictNullChecks to work for users implementing it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}

export interface CodeFixContext {
errorCode: string;
sourceFile: SourceFile;
span: TextSpan;
checker: TypeChecker;
newLineCharacter: string;
}

export namespace codeFix {
Copy link
Member

Choose a reason for hiding this comment

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

Style nit: It feels odd to have a lower-camelcase namespace. I know we already have jsTypings, but that feels odd, too. Given that we favor lowercase styled namespaces codefix seems more appropriate.

const codeFixes: Map<CodeFix[]> = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

does it mean that registration of the fix is global and fix will be visible\available in all loaded projects. I think when we discussed it last time we wanted to make them more fine-grained.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forcing users to have to specify which fixes they want for each project seems counter intuitive. They can always ignore, some/all of them.... this is the behavior C#/VB has.

If we make this extensible at some point, we can revisit this.


export function registerCodeFix(action: CodeFix) {
forEach(action.errorCodes, error => {
let fixes = codeFixes[error];
if (!fixes) {
fixes = [];
codeFixes[error] = fixes;
}
fixes.push(action);
});
}

export class CodeFixProvider {
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you need this class? all the state is in a const. so just loose the class, and export a getCodeFixes functions

public static getSupportedErrorCodes() {
return getKeys(codeFixes);
}

public getFixes(context: CodeFixContext): CodeAction[] {
const fixes = codeFixes[context.errorCode];
let allActions: CodeAction[] = [];

Debug.assert(fixes && fixes.length > 0, "No fixes found for error: '${errorCode}'.");
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't it too strict?


forEach(fixes, f => {
const actions = f.getCodeActions(context);
if (actions && actions.length > 0) {
allActions = allActions.concat(actions);
}
});

return allActions;
}
}
}
}
6 changes: 6 additions & 0 deletions src/services/codefixes/references.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
///<reference path='..\services.ts' />
Copy link
Contributor

Choose a reason for hiding this comment

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

we stoped using references for a while. they should be picked up by the tsconfig.json in this folder any ways. so please remove this.

///<reference path='codeFixProvider.ts' />
///<reference path='superFixes.ts' />
///<reference path='unusedIdentifierFixes.ts' />
///<reference path='changeExtendsToImplementsFix.ts' />
///<reference path='interfaceFixes.ts' />
65 changes: 65 additions & 0 deletions src/services/codefixes/superFixes.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
/* @internal */
namespace ts.codeFix {
function getOpenBraceEnd(constructor: ConstructorDeclaration, sourceFile: SourceFile) {
// First token is the open curly, this is where we want to put the 'super' call.
return constructor.body.getFirstToken(sourceFile).getEnd();
}

registerCodeFix({
name: "AddMissingSuperCallFix",
errorCodes: ["TS2377"],
Copy link
Contributor

@vladima vladima Aug 5, 2016

Choose a reason for hiding this comment

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

can we check that this error code is not taken or diagnostics with this error code exists?

getCodeActions: (context: CodeFixContext) => {
const sourceFile = context.sourceFile;
const token = getTokenAtPosition(sourceFile, context.span.start);
Debug.assert(token.kind === SyntaxKind.ConstructorKeyword, "Failed to find the constructor.");

const newPosition = getOpenBraceEnd(<ConstructorDeclaration>token.parent, sourceFile);
return [{
description: getLocaleSpecificMessage(Diagnostics.Add_missing_super_call),
changes: [{ fileName: sourceFile.fileName, textChanges: [{ newText: "super();", span: { start: newPosition, length: 0 } }] }]
}];
}
});

registerCodeFix({
name: "MakeSuperCallTheFirstStatementInTheConstructor",
errorCodes: ["TS17009"],
getCodeActions: (context: CodeFixContext) => {
const sourceFile = context.sourceFile;

const token = getTokenAtPosition(sourceFile, context.span.start);
const constructor = getContainingFunction(token);
Debug.assert(constructor.kind === SyntaxKind.Constructor, "Failed to find the constructor.");

const superCall = findSuperCall((<ConstructorDeclaration>constructor).body);
Debug.assert(!!superCall, "Failed to find super call.");

const newPosition = getOpenBraceEnd(<ConstructorDeclaration>constructor, sourceFile);
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 add a test for the case where this is an argument to super

super(this.x);

const changes = [{
fileName: sourceFile.fileName, textChanges: [{
newText: superCall.getText(sourceFile),
span: { start: newPosition, length: 0 }
},
{
newText: "",
span: { start: superCall.getStart(sourceFile), length: superCall.getWidth(sourceFile) }
}]
}];

return [{
description: getLocaleSpecificMessage(Diagnostics.Make_super_call_the_first_statement_in_the_constructor),
changes
}];

function findSuperCall(n: Node): Node {
if (n.kind === SyntaxKind.ExpressionStatement && isSuperCallExpression((<ExpressionStatement>n).expression)) {
return n;
}
if (isFunctionLike(n)) {
return undefined;
}
return forEachChild(n, findSuperCall);
}
}
});
}
49 changes: 47 additions & 2 deletions src/services/services.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
/// <reference path='jsTyping.ts' />
/// <reference path='formatting\formatting.ts' />
/// <reference path='formatting\smartIndenter.ts' />
/// <reference path='codefixes\references.ts' />
Copy link
Contributor

Choose a reason for hiding this comment

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

just include "codefixes\codefixes". and add the files to the tsconfig.json


namespace ts {
/** The version of the language service API */
Expand Down Expand Up @@ -1237,6 +1238,8 @@ namespace ts {

isValidBraceCompletionAtPosition(fileName: string, position: number, openingBrace: number): boolean;

getCodeFixesAtPosition(fileName: string, start: number, end: number, errorCodes: string[]): CodeAction[];
Copy link
Contributor

Choose a reason for hiding this comment

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

A JSDoc comment would be nice.

Copy link
Contributor

Choose a reason for hiding this comment

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

should not this be getCodeActionsAtPosition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No this matches the naming in the Roslyn codebase


getEmitOutput(fileName: string): EmitOutput;

getProgram(): Program;
Expand Down Expand Up @@ -1283,6 +1286,18 @@ namespace ts {
newText: string;
}

export interface FileTextChanges {
fileName: string;
textChanges: TextChange[];
}

export interface CodeAction {
/** Description of the code action to display in the UI of the editor */
description: string;
/** Text changes to apply to each file as part of the code action */
changes: FileTextChanges[];
}

export interface TextInsertion {
newText: string;
/** The position in newText the caret should point to after the insertion. */
Expand Down Expand Up @@ -1886,9 +1901,13 @@ namespace ts {
};
}

// Cache host information about script should be refreshed
export function getSupportedCodeFixes() {
return codeFix.CodeFixProvider.getSupportedErrorCodes();
}

// Cache host information about script Should be refreshed
// at each language service public entry point, since we don't know when
// set of scripts handled by the host changes.
// the set of scripts handled by the host changes.
class HostCache {
private fileNameToEntry: FileMap<HostFileInformation>;
private _compilationSettings: CompilerOptions;
Expand Down Expand Up @@ -3022,6 +3041,7 @@ namespace ts {
documentRegistry: DocumentRegistry = createDocumentRegistry(host.useCaseSensitiveFileNames && host.useCaseSensitiveFileNames(), host.getCurrentDirectory())): LanguageService {

const syntaxTreeCache: SyntaxTreeCache = new SyntaxTreeCache(host);
const codeFixProvider: codeFix.CodeFixProvider = new codeFix.CodeFixProvider();
let ruleProvider: formatting.RulesProvider;
let program: Program;
let lastProjectVersion: string;
Expand Down Expand Up @@ -7832,6 +7852,30 @@ namespace ts {
return [];
}

function getCodeFixesAtPosition(fileName: string, start: number, end: number, errorCodes: string[]): CodeAction[] {
synchronizeHostData();
const sourceFile = getValidSourceFile(fileName);
const checker = program.getTypeChecker();
let allFixes: CodeAction[] = [];

forEach(errorCodes, error => {
const context = {
errorCode: error,
sourceFile: sourceFile,
span: { start, length: end - start },
Copy link
Contributor

Choose a reason for hiding this comment

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

define the span outside the loop and reuse it. no reason to create a new one every time. also use createTextSpanFromBounds instead.

checker: checker,
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 program and not checker. since you can get the checker from the program but not the opposite.

newLineCharacter: getNewLineOrDefaultFromHost(host)
};

const fixes = codeFixProvider.getFixes(context);
Copy link
Contributor

Choose a reason for hiding this comment

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

use ts.concatenate instead:

allFixes = concatenate(allFixes , codeFixProvider.getFixes(context));

alternatively, you can rewrite this foreach as a reduce call.

if (fixes) {
allFixes = allFixes.concat(fixes);
}
});

return allFixes;
}

/**
* Checks if position points to a valid position to add JSDoc comments, and if so,
* returns the appropriate template. Otherwise returns an empty string.
Expand Down Expand Up @@ -8302,6 +8346,7 @@ namespace ts {
getFormattingEditsAfterKeystroke,
getDocCommentTemplateAtPosition,
isValidBraceCompletionAtPosition,
getCodeFixesAtPosition,
getEmitOutput,
getNonBoundSourceFile,
getProgram
Expand Down
Loading