-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Pvb/codeaction/api #10185
Changes from 3 commits
94ea825
466d26f
6f4fb06
e0b73c4
124e05f
730f31f
9a26da1
f931e60
66e1c92
49b65c7
3df79d5
682f812
6e8eb1d
20dea29
4f404ad
1cc9732
cf4e300
ebcfce4
163e758
75e1b80
1e9b653
c29e9b7
77610f6
a83a2b0
a88ceb5
f310310
7c96c28
dc516c0
9b98d00
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2547,6 +2547,7 @@ namespace ts { | |
Warning, | ||
Error, | ||
Message, | ||
CodeFix, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAIK this seems no different than |
||
} | ||
|
||
export enum ModuleResolutionKind { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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}'`); | ||
} | ||
} | ||
|
||
|
@@ -638,7 +638,6 @@ namespace FourSlash { | |
} | ||
} | ||
|
||
|
||
public verifyCompletionListAllowsNewIdentifier(negative: boolean) { | ||
const completions = this.getCompletionListAtCaret(); | ||
|
||
|
@@ -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"); | ||
} | ||
} | ||
|
@@ -1545,6 +1544,10 @@ namespace FourSlash { | |
} | ||
} | ||
|
||
private removeWhitespace(text: string): string { | ||
return text.replace(/\s/g, ""); | ||
} | ||
|
||
public goToBOF() { | ||
this.goToPosition(0); | ||
} | ||
|
@@ -1862,6 +1865,44 @@ namespace FourSlash { | |
} | ||
} | ||
|
||
public verifyCodeFixAtPosition(expectedText: string, errorCode?: number) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. verifyCodeActionAtPosition There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can There was a problem hiding this comment. Choose a reason for hiding this commentThe 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}`]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should not need the |
||
|
||
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); | ||
|
@@ -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); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 = { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
/* @internal */ | ||
namespace ts { | ||
export interface CodeFix { | ||
name: string; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I'm not mistaken, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's very useful for debugging... There was a problem hiding this comment. Choose a reason for hiding this commentThe 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[]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
const codeFixes: Map<CodeFix[]> = {}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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}'."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
///<reference path='..\services.ts' /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we stoped using |
||
///<reference path='codeFixProvider.ts' /> | ||
///<reference path='superFixes.ts' /> | ||
///<reference path='unusedIdentifierFixes.ts' /> | ||
///<reference path='changeExtendsToImplementsFix.ts' /> | ||
///<reference path='interfaceFixes.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"], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} | ||
} | ||
}); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ | |
/// <reference path='jsTyping.ts' /> | ||
/// <reference path='formatting\formatting.ts' /> | ||
/// <reference path='formatting\smartIndenter.ts' /> | ||
/// <reference path='codefixes\references.ts' /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 */ | ||
|
@@ -1237,6 +1238,8 @@ namespace ts { | |
|
||
isValidBraceCompletionAtPosition(fileName: string, position: number, openingBrace: number): boolean; | ||
|
||
getCodeFixesAtPosition(fileName: string, start: number, end: number, errorCodes: string[]): CodeAction[]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A JSDoc comment would be nice. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should not this be There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
@@ -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. */ | ||
|
@@ -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; | ||
|
@@ -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; | ||
|
@@ -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 }, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
checker: checker, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
@@ -8302,6 +8346,7 @@ namespace ts { | |
getFormattingEditsAfterKeystroke, | ||
getDocCommentTemplateAtPosition, | ||
isValidBraceCompletionAtPosition, | ||
getCodeFixesAtPosition, | ||
getEmitOutput, | ||
getNonBoundSourceFile, | ||
getProgram | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forEach
There was a problem hiding this comment.
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 calledfind
.