Refactoring support #15569

RyanCavanaugh · 2017-05-03T22:46:57Z

Picks up where #14624 left off.

RyanCavanaugh · 2017-05-08T22:21:25Z

Anyone want to take a look? The VS side is about to merge

mhegazy · 2017-05-09T20:59:42Z

src/compiler/types.ts

@@ -3368,6 +3368,7 @@ namespace ts {
        Warning,
        Error,
        Message,
+        CodeFix


i would call this Suggestion.

mhegazy · 2017-05-09T21:01:00Z

src/server/protocol.ts

+    export type GetApplicableRefactorsRequestArgs = FileLocationOrRangeRequestArgs;
+
+    export interface ApplicableRefactorInfo {
+        refactorName: string;


just name

mhegazy · 2017-05-09T21:04:01Z

src/server/protocol.ts

@@ -95,6 +95,10 @@ namespace ts.server.protocol {
        /* @internal */
        export type GetCodeFixesFull = "getCodeFixes-full";
        export type GetSupportedCodeFixes = "getSupportedCodeFixes";
+        export type GetCodeFixDiagnostics = "getCodeFixDiagnostics";


getSuggestionDiagnostics

mhegazy · 2017-05-09T21:39:45Z

src/services/codefixes/convertFunctionToEs6Class.ts

+    });
+
+    function createCodeFixDiagnosticIfApplicable(node: Node, context: CodeFixDiagnoseContext): Diagnostic | undefined {
+        if (!isSourceFileJavaScript(context.boundSourceFile)) {


this is not going to scale as we add code fixes. we can not call every code fix on every node on the tree, we need a more efficient way.
We could make codeFix register a SyntaxKind it is interested in, this will save us the dispatch on every node at least.
Alternatively, we can have the code fix return a Boolean whether to keep walking or not, this way we avoid walking expressions that we do not care about.

Codefix feature removed

mhegazy · 2017-05-09T21:40:38Z

src/services/codeFixProvider.ts

        }

        export function getSupportedErrorCodes() {
            return Object.keys(codeFixes);
        }

+        export function getCodeFixDiagnosticsForNode(context: CodeFixDiagnoseContext, node: Node): Diagnostic[] | undefined {
+            let result: Diagnostic[];


i think we need to optimize this function and getCodeFixDiagnostics.

mhegazy · 2017-05-09T21:42:59Z

src/services/codefixes/convertFunctionToEs6Class.ts

+
+        const checker = context.program.getTypeChecker();
+        const symbol = checker.getSymbolAtLocation(node);
+        if (isClassLikeSymbol(symbol)) {


i would say this should be limited to the declaration, not on every reference.

I would also say you do not need the checker here.

mhegazy · 2017-05-09T21:44:18Z

src/services/codeFixProvider.ts

@@ -3,6 +3,14 @@ namespace ts {
    export interface CodeFix {
        errorCodes: number[];
        getCodeActions(context: CodeFixContext): CodeAction[] | undefined;
+        createCodeFixDiagnosticIfApplicable?(node: Node, context: CodeFixDiagnoseContext): Diagnostic | undefined;


Ideally we want this to be syntactic in nature, passing in the program allows for getting the checker. this can be expensive if the codeFix author do not know what they are doing, consider not passing in the program, but just a sourceFile and options.

We also want to use the same mechanism for refactorings. a refactoring should have isApplicable, and getCodeActions. I would recommend switching this into isApplicable, and an optional diagnostic message.

mhegazy · 2017-05-09T21:50:07Z

src/services/refactorProvider.ts

+        getCodeActions(context: RefactorContext, positionOrRange: number | TextRange): CodeAction[];
+
+        /** A fast syntactic check to see if the refactor is applicable at given position. */
+        isApplicableForPositionOrRange(context: LightRefactorContext, positionOrRange: number | TextRange): boolean;


consider unifying this with the codeFix isApplicable. ideally we can have one implementation that serves as both a refactor and a codefix + suggestion provider.

mhegazy · 2017-05-09T21:52:07Z

src/services/refactors/addAsyncSuffix.ts

+
+    const asyncSuffixRefactor: Refactor = {
+        name: "Add Async suffix",
+        description: "Add an 'Async' suffix to async function declarations",


should be a diagnostics message for localizability.

mhegazy · 2017-05-09T21:55:00Z

src/services/refactors/addAsyncSuffix.ts

+        const token = getTokenAtPosition(nonBoundSourceFile, tokenPos);
+
+        let node = token;
+        while (node) {


It is kind of random that on any expression i can refactor the containing function to include async. i would say this has to be on function/method declaration at least.

mhegazy · 2017-05-15T20:22:46Z

src/services/refactorProvider.ts

+        getCodeActions(context: RefactorContext, positionOrRange: number | TextRange): CodeAction[];
+
+        /** A fast syntactic check to see if the refactor is applicable at given position. */
+        isApplicableForPositionOrRange(context: QueryRefactorContext, positionOrRange: number | TextRange): boolean;


just isApplicable, the positionOrRange is implied from the arguments.

mhegazy · 2017-05-15T20:26:39Z

src/services/refactors/addAsyncSuffix.ts

+/* @internal */
+namespace ts.refactor {
+    const asyncSuffixRefactor: Refactor = {
+        name: "Add Async suffix",


This should be in diagnostics

Removed this refactor

mhegazy · 2017-05-15T20:27:01Z

src/services/refactors/addAsyncSuffix.ts

+namespace ts.refactor {
+    const asyncSuffixRefactor: Refactor = {
+        name: "Add Async suffix",
+        description: "Add an 'Async' suffix to async function declarations",


should be in diagnostics to ensure localizatability.

mhegazy · 2017-05-15T20:30:50Z

src/services/refactors/addAsyncSuffix.ts

+        }];
+    }
+
+    function isApplicableForPositionOrRange(context: RefactorContext, positionOrRange: number | TextRange): boolean {


kinda silly comment, but can we have (startPosition, endPoisition) instead of positionOrRange? every refactor author has to write the same check typeof positionOrRange === "number"?...

Actually why not just put these on the RefactorContext? we put the errorCode on the codeFix context.

Makes sense

mhegazy · 2017-05-15T20:33:58Z

src/services/refactors/addAsyncSuffix.ts

+
+        let node = token;
+        while (node) {
+            if (node.kind === SyntaxKind.FunctionDeclaration) {


i would use isNameOfFunctionDeclaration or something similar.

i would check if token is one of identifier, function keyword, public, private, protected, or static , and their parent is a FunctionDeclatation or MethodDeclaration.

File removed

mhegazy · 2017-05-15T20:57:40Z

src/services/refactors/convertFunctionToEs6Class.ts

+            }
+
+            // all static members are stored in the "exports" array of symbol
+            if (symbol.exports) {


Is this intended for a .ts or a .js file?

I think it is a .js file, since the binding for the special properties do not work in .ts files any way.

If my understanding is correct, then for static members we can not just add declarations for them, this will result in invalid .js class.

this also applies to the property declarations and assignments..

If we want this to work in a .ts file, then we need to do something with the inference to make it work first.

I would also recommend adding a check at the top of isApplicable to return if !inJavaScriptFile(node)

also for member initialization, I wounder if we can even do these correctellly...

var c = function() { this.list.push(1); } c.prototype.list = [];

is not the same as:

class c { constructor() { this.list.push(1); } list = [];

let a side that it is not valid JS strictly speaking.

Fixed these up. Non-func-expr things remain as assignments following the class body

mhegazy · 2017-05-15T20:59:10Z

src/services/services.ts

+            positionOrRange: number | TextRange,
+            refactorName: string): CodeAction[] | undefined {
+
+            const context: RefactorContext = {


consider in-lining the const in the call and using the contextual type instead.

mhegazy · 2017-05-15T21:00:10Z

src/services/refactorProvider.ts

+            positionOrRange: number | TextRange): ApplicableRefactorInfo[] | undefined {
+
+            let results: ApplicableRefactorInfo[];
+            refactors.forEach(refactor => {


I would check the cancellation token here.

mhegazy · 2017-05-15T21:00:41Z

src/services/refactorProvider.ts

+        program: Program;
+    }
+
+    export interface RefactorContext extends QueryRefactorContext {


what is the value of the split here?

mhegazy · 2017-05-15T21:00:58Z

src/services/refactorProvider.ts

+
+    export interface RefactorContext extends QueryRefactorContext {
+        newLineCharacter: string;
+        rulesProvider: formatting.RulesProvider;


what about cancellation token?

consider exposing CodeFixContext minus errorCode.

mhegazy · 2017-05-19T21:47:02Z

src/server/protocol.ts

+
+        export type GetApplicableRefactors = "getApplicableRefactors";
+        export type GetRefactorCodeActions = "getRefactorCodeActions";
+        export type GetRefactorCodeActionsFull = "getRefactorCodeActions-full";


This should be marked as /* @internal */

mhegazy · 2017-05-19T21:47:45Z

src/server/protocol.ts

+
+    export type RefactorCodeActions = {
+        actions: protocol.CodeAction[];
+        renameLocation?: number


A comment here would be good.

mhegazy · 2017-05-19T21:48:45Z

src/services/refactorProvider.ts

+        program: Program;
+        newLineCharacter: string;
+        rulesProvider?: formatting.RulesProvider;
+        cancellationToken?: CancellationToken;


why is this optional?

mhegazy · 2017-05-19T21:50:40Z

src/services/refactorProvider.ts

+        endPosition?: number;
+        program: Program;
+        newLineCharacter: string;
+        rulesProvider?: formatting.RulesProvider;


why is this optional?

Fixes microsoft#25739, from microsoft/TypeScript#15569 Prototype of refactoring support for ts 2.4

* Prototype TS/JS Refactoring Provider Fixes #25739, from microsoft/TypeScript#15569 Prototype of refactoring support for ts 2.4 * Adding error reporting * Updating for new API * show quick pick for non-inlinable refactrings

introduced by microsoft/TypeScript#15569

msftclas added the cla-not-required label May 3, 2017

RyanCavanaugh requested review from billti and mhegazy May 8, 2017 22:21

mhegazy suggested changes May 9, 2017

View reviewed changes

mhegazy suggested changes May 15, 2017

View reviewed changes

angelozerr mentioned this pull request May 16, 2017

Refactoring Support angelozerr/typescript.java#178

Closed

mjbvz mentioned this pull request May 19, 2017

Support refactoring commands provided by TS Server microsoft/vscode#25739

Closed

RyanCavanaugh force-pushed the new_refactor branch from 326ba1c to 17b1eb2 Compare May 19, 2017 17:19

Refactoring support (squash)

f6fbe61

RyanCavanaugh force-pushed the new_refactor branch from 17b1eb2 to f6fbe61 Compare May 19, 2017 17:46

RyanCavanaugh merged commit f489f5a into microsoft:master May 19, 2017

RyanCavanaugh deleted the new_refactor branch May 19, 2017 18:23

mhegazy reviewed May 19, 2017

View reviewed changes

mhegazy mentioned this pull request May 19, 2017

Add infrastructure for refactors #14624

Closed

renovate-bot mentioned this pull request May 22, 2017

Update dependency typescript to version 2.3.3 singapore/lint-condo#297

Merged

mhegazy mentioned this pull request May 22, 2017

Pvb/refactorapi #11913

Closed

mjbvz mentioned this pull request May 23, 2017

Prototype TS/JS Refactoring Provider microsoft/vscode#27166

Merged

mhegazy mentioned this pull request May 24, 2017

Pvb/refactor/jsdoc #11951

Closed

bryphe mentioned this pull request Jun 4, 2017

Refactoring Support onivim/oni#474

Open

mjbvz added a commit to mjbvz/vscode that referenced this pull request Jun 16, 2017

Prototype TS/JS Refactoring Provider

dfadffe

Fixes microsoft#25739, from microsoft/TypeScript#15569 Prototype of refactoring support for ts 2.4

ananthakumaran added a commit to ananthakumaran/tide that referenced this pull request Jul 8, 2017

add support for refactor command

6534f73

introduced by microsoft/TypeScript#15569

ananthakumaran mentioned this pull request Jul 8, 2017

add support for refactor command ananthakumaran/tide#172

Merged

ananthakumaran added a commit to ananthakumaran/tide that referenced this pull request Jul 30, 2017

add support for refactor command

f921a42

introduced by microsoft/TypeScript#15569

microsoft locked and limited conversation to collaborators Jun 14, 2018

Refactoring support #15569

Refactoring support #15569

Conversation

RyanCavanaugh commented May 3, 2017

RyanCavanaugh commented May 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhegazy May 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhegazy May 19, 2017 •

edited

Loading