Add infrastructure for refactors by zhengbli · Pull Request #14624 · microsoft/TypeScript

zhengbli · 2017-03-13T07:38:27Z

This PR adds the infrastructure for refactors.

The PR adds one new kind of code fixes as well as refactors:

"diagnostic-generating code fixes": a special kind of code fixes that would generate non-error diagnostics. After normal syntactic check and semantic check, the language service will do an extra pass of the AST to get the non-error diagnostics generated by registered "diagnostic-generating code fixes". Then when the user interacts with the new diagnostics in code, a query for corresponding code actions will be triggered, just like with other code fixes.
"refactors": to get a refactor it is a two-step procedure. First, the editor will ask about all "applicable refactor info" at a given location / range, the returned "applicable refactor info" would only contain basic information like refactor names, it wouldn't compute the code actions yet. Then after the user chose a particular refactor, then the editor will send another request to compute the corresponding code actions.

aozgaa · 2017-03-14T20:04:49Z

src/harness/unittests/tsserverProjectSystem.ts

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


"messages" should be "message"

… refactor_only

aozgaa · 2017-03-17T22:12:39Z

src/server/client.ts

        }

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


Does this crash if range === undefined ?

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

aozgaa · 2017-03-17T22:34:07Z

src/server/protocol.ts

-      * Instances of this interface specify errorcodes on a specific location in a sourcefile.
-      */
-    export interface CodeFixRequestArgs extends FileRequestArgs {
+    export interface TextRangeRequestArgs extends FileRequestArgs {


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?

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

aozgaa · 2017-03-17T22:38:06Z

src/server/session.ts

+            return { startPosition, endPosition };
+
+            function getStartPosition() {
+                return args.startPosition !== undefined ? args.startPosition : scriptInfo.lineOffsetToPosition(args.startLine, args.startOffset);


Do you want to save the result in args.startPosition (resp. args.endPosition)?

aozgaa · 2017-03-17T23:06:42Z

src/services/refactorProvider.ts

+/* @internal */
+namespace ts {
+    interface BaseRefactor {
+        /** Description of the refactor to display in the UI of the editor */


should this have a property canBeSuggested: boolean; ?

aozgaa · 2017-03-17T23:07:53Z

src/services/refactorProvider.ts

+    export type Refactor = SuggestableRefactor | NonSuggestableRefactor;
+
+    export interface LightRefactorContext {
+        nonBoundSourceFile: SourceFile;


Can you add a comment explaining what bound and nonBound means?

aozgaa · 2017-03-17T23:14:17Z

src/services/refactorProvider.ts

+            return results;
+        }
+
+        export function getSuggestedRefactorDiagnosticsForNode(node: Node, context: RefactorContext) {


Would it be right to name this getSuggestable...? That would be more consistent with the naming of the interfaces.

aozgaa · 2017-03-17T23:18:31Z

src/services/services.ts

+            }
+        }
+
+        function getCodeActionsForRefactorAtPosition(


Do we have a convention for when we split function parameters across multiple lines?

I don't think so, though normally i do that when I have to use the horizontal scroll bar on my laptop if putting all in one line.

… refactor_only

mhegazy · 2017-03-21T17:12:14Z

src/server/protocol.ts

+        refactorCode: number;
+    }
+
+    export interface GetCodeActionsForRefactorResponse extends Response {


do not use an array. make it an map.

mhegazy · 2017-03-28T21:33:44Z

src/server/protocol.ts

+        position?: number;
+    }
+
+    export namespace Refactor {


we do not use namespaces anywhere else.

mhegazy · 2017-03-28T21:35:55Z

src/server/protocol.ts

+        }
+
+        export interface ApplicableRefactorInfo {
+            refactorKind: number;


i would call this name and make it string.

mhegazy · 2017-03-28T21:36:37Z

src/server/protocol.ts

+        }
+
+        export interface GetApplicableRefactorsResponse extends Response {
+            body?: ApplicableRefactorInfo[];


i would make this an object instead of an array.

mhegazy · 2017-03-28T21:54:35Z

src/server/protocol.ts

+
+        export interface GetRefactorCodeActionsRequestArgs extends FileLocationOrSpanWithPositionRequestArgs {
+            /* The kind of the applicable refactor */
+            refactorKinds?: number[];


probally just one refactoring to ask for code actions for

mhegazy · 2017-03-28T21:54:53Z

src/server/protocol.ts

+            /* The kind of the applicable refactor */
+            refactorKinds?: number[];
+            /* The diagnostic code of a refactor diagnostic */
+            diagnosticCodes?: number[];


do not think we need this, it should be covered by getCodeActions

… refactor_only

RyanCavanaugh · 2017-04-18T18:05:26Z

src/server/session.ts

+                const { startPosition, endPosition } = this.getStartAndEndPosition(args, scriptInfo);
+                textRange = { pos: startPosition, end: endPosition };
+            }
+            return { position, textRange };


position may be undefined, is this intended? Initialize at line 1450 if so

RyanCavanaugh · 2017-04-18T18:07:10Z

src/server/session.ts

-                return args.endPosition !== undefined ? args.endPosition : scriptInfo.lineOffsetToPosition(args.endLine, args.endOffset);
-            }
+        private getStartAndEndPosition(args: protocol.FileRangeRequestArgs, scriptInfo: ScriptInfo) {
+            const startPosition = args.startPosition !== undefined


This is really confusing, can we just use a normal if here

RyanCavanaugh · 2017-04-18T18:11:25Z

src/services/codeFixProvider.ts

+        export function getCodeFixDiagnosticsForNode(context: CodeFixDiagnoseContext, node: Node): Diagnostic[] | undefined {
+            let result: Diagnostic[];
+            for (const codeFix of diagnosticGeneratingCodeFixes) {
+                const newDiag = codeFix.createCodeFixDiagnosticIfApplicable(node, context);


This member is declared as optional, but isn't checked for undefined in this method

in line 41 it already checked the existence of createCodeFixDiagnosticIfApplicable

RyanCavanaugh · 2017-04-18T18:11:55Z

src/services/refactorProvider.ts

+        isApplicableForPositionOrRange(context: LightRefactorContext, positionOrRange: number | TextRange): boolean;
+    }
+
+    export interface LightRefactorContext {


Comment defining what "Light" means

RyanCavanaugh · 2017-04-18T18:16:22Z

src/services/refactorProvider.ts

+            return results;
+        }
+
+        export function getRefactorCodeActions(


Why does this return an empty array vs getApplicableRefactors returns undefined if the array would have been empty?

RyanCavanaugh · 2017-04-18T18:20:38Z

Should we add a trivial refactor with this just so we can have some testcases?

zhengbli · 2017-04-18T18:34:26Z

Sure, I'm adding one now will update soon

zhengbli · 2017-04-19T05:07:51Z

I added two things:

a diagnostic-generating code fix: converting functions that has prototype assignment to ES6 classes
a sample refactor: add Async suffix to async function declarations

RyanCavanaugh · 2017-04-19T17:24:01Z

@mhegazy any other comments?

mhegazy · 2017-05-19T21:53:12Z

closing in favor of #15569

Add infrastructure for refactors

e38b98a

msftclas added the cla-not-required label Mar 13, 2017

aozgaa reviewed Mar 14, 2017

View reviewed changes

zhengbli added 2 commits March 15, 2017 16:23

Merge branch 'master' of https://github.com/Microsoft/TypeScript into…

6cbc258

… refactor_only

Grammar

3011d16

aozgaa reviewed Mar 17, 2017

View reviewed changes

zhengbli added 2 commits March 20, 2017 16:14

cr feedbacks

fa5a483

Redesign APIs

b357841

zhengbli force-pushed the refactor_only branch from 132051b to b357841 Compare March 26, 2017 09:00

zhengbli added 2 commits March 27, 2017 00:31

Merge branch 'master' of https://github.com/Microsoft/TypeScript into…

e92d016

… refactor_only

fix linting issues

675cf03

mhegazy suggested changes Mar 28, 2017

View reviewed changes

zhengbli added 5 commits March 28, 2017 16:25

replace the 'suggestable' refactors with code fixes

f108fd7

Remove namespace in protocol

b86d40a

Merge branch 'master' of https://github.com/Microsoft/TypeScript into…

909b436

… refactor_only

Change the args shape to be more compatible

0965223

Merge branch 'master' of https://github.com/Microsoft/TypeScript into…

ba6e22c

… refactor_only

RyanCavanaugh reviewed Apr 18, 2017

View reviewed changes

zhengbli added 2 commits April 18, 2017 14:38

address CR feedback

683a0a0

add codefix to convert function to es6 class

dfa2506

zhengbli force-pushed the refactor_only branch from a6f34a9 to dfa2506 Compare April 19, 2017 01:33

zhengbli added 2 commits April 18, 2017 22:24

improve tests and force synchronizeHosts before codeFix diag

0aeff72

Add a simple refactor as a sample

2e8f6c6

RyanCavanaugh self-assigned this Apr 24, 2017

This was referenced May 1, 2017

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

Closed

Refactoring support #15569

Merged

mhegazy closed this May 19, 2017

microsoft locked and limited conversation to collaborators Jun 19, 2018

Conversation

zhengbli commented Mar 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aozgaa Mar 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhengbli Mar 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RyanCavanaugh Apr 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RyanCavanaugh commented Apr 18, 2017

Uh oh!

zhengbli commented Apr 18, 2017

Uh oh!

zhengbli commented Apr 19, 2017

Uh oh!

RyanCavanaugh commented Apr 19, 2017

Uh oh!

mhegazy commented May 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

zhengbli commented Mar 13, 2017 •

edited

Loading

aozgaa Mar 17, 2017 •

edited

Loading

zhengbli Mar 20, 2017 •

edited

Loading

RyanCavanaugh Apr 18, 2017 •

edited

Loading