Extract method #16960

RyanCavanaugh · 2017-07-06T00:13:25Z

Implements the "extract function" refactor

Given a user selection, this produces a set of "extractions" the user can select from. For example, with an expression in a class, we can refactor into a peer method, a function in an enclosing namespace, or a function in the same file (these are referred to as "scopes" in the code):

Not all selections will produce the same set of extractions. For example, if we refer to a property of this in the selection body, then we won't be able to extract to the enclosing namespace or file:

Still writing some tests so expect a few commits with bugfixes to come in, but it's pushing 2,000 lines of implementation already so jump in

Most code courtesy @vladima so I may not be able to perfectly answer all questions

…or exprs

ghost · 2017-07-06T15:02:39Z

Tests should be fixed by ivogabe/gulp-typescript#522

RyanCavanaugh · 2017-07-10T16:21:21Z

🔔

weswigham

Not all selections will produce the same set of extractions. For example, if we refer to a property of this in the selection body, then we won't be able to extract to the enclosing namespace or file

You could add an option to extract to namespace where the this object becomes a normal function parameter, rather than this, provided no private or protected members are in use in the extracted code. I've done this by hand frequently in the past (extracting internal code into an external helper) - it may be worth adding. Actually, even private member access can be transformed to extracted-function-parameters provided they don't have unexposed types. Maybe an enhancement for the future.

weswigham · 2017-07-14T17:45:19Z

src/compiler/checker.ts

-            resolveNameAtLocation(location: Node, name: string, meaning: SymbolFlags): Symbol | undefined {
-                location = getParseTreeNode(location);
-                return resolveName(location, name, meaning, /*nameNotFoundMessage*/ undefined, name);
+            resolveName(name, location, meaning) {


Is this the same as resolveNameAtLocation, but with a changed argument order and no looking up of parse tree nodes (I can only assume so it works on synthesized nodes with parent pointers setup)? Can we not just remove the looking up of parse tree nodes in resolveNameAtLocation if that's the case? Oh, nameArg is also undefined. Can that not just be added as an optional parameter to this existing function?

@rbuckton would know authoritatively, but as I understand it, we check isParseTreeNode on public API requests because the checker does not work well with synthesized nodes, since we don't perform binding on synthesized nodes as part of transforms (for performance reasons?) and we may inspect the literal text as part of some checker operations. That is, the checker API is only supposed to work with trees parsed and bound from an actual source file.

@aozgaa is correct. If the checker receives a node that did not originate from a parse tree, we attempt to find the parse tree node from which it was created. Passing in a synthesized node to the checker without a parse-tree check will likely result in compiler crashes.

That said, resolveNameAtLocation (nee. resolveName) is marked /* @internal */ and isn't used in transforms. The guard is mostly present in case we decide to use it at some point down the road.

weswigham · 2017-07-14T17:50:26Z

src/services/formatting/formatting.ts

                if (isToken(child) && child.kind !== SyntaxKind.JsxText) {
                    // if child node is a token, it does not impact indentation, proceed it using parent indentation scope rules
                    const tokenInfo = formattingScanner.readTokenInfo(child);
-                    Debug.assert(tokenInfo.token.end === child.end, "Token end is child end");


Was this assertion incorrect? This PR didn't change any parsing related things, AFAIK?

Finally tracked this down - the problem is we were creating a synthetic type reference node with text like { x: string } when the type was anonymous, which was confusing the scanner because it expects a type reference node to point to an identifier or qualified name, so the formatting scanner and syntax tree disagreed about whether to expect a token. Fixed in extractMethod#485 and restored the assert

weswigham · 2017-07-14T17:58:10Z

src/services/refactors/extractMethod.ts

+    const nullTransformationContext: TransformationContext = {
+        enableEmitNotification: noop,
+        enableSubstitution: noop,
+        endLexicalEnvironment: () => undefined,


Isn't this just noop?

Sort of. noop returns void, but endLexicalEnvironment expects a Statement[] | undefined return value. Switched to noop and added a type assertion

DanielRosenwasser · 2017-07-14T19:16:37Z

src/services/refactors/extractMethod.ts

+                case SyntaxKind.FunctionDeclaration:
+                    return `function ${s.name.getText()}`;
+                case SyntaxKind.ArrowFunction:
+                    return "arrow function";


These won't be localized.

I think Roslyn decided not to localize syntax kind names, but their use case is slightly different.

Half these are actual keywords. Do we actually expect the loc team to change e.g. "method" to correct corresponding word in the target language?

rosieks · 2017-07-15T10:33:54Z

Does it work with JSX? Can I select some JSX markup and extract it to stateless component?

amcasey · 2017-07-17T17:34:35Z

src/compiler/types.ts

         */
        /* @internal */ getAllPossiblePropertiesOfType(type: Type): Symbol[];
-
+        /* @internal */ resolveName(name: string, location: Node, meaning: SymbolFlags): Symbol;


What happened to | undefined? Does it throw on failure?

The compiler doesn't have strictNullChecks on. Added | undefined to be explicit.

amcasey · 2017-07-17T17:38:20Z

src/services/textChanges.ts

        readonly useIndentationFromFile?: boolean;
-        readonly node?: Node;
-        readonly options?: ChangeNodeOptions;
+        readonly node?: Node | Node[];


I'm not sure I understand the type. It's either undefined, one Node, zero Nodes, a singleton list of Nodes, or multiple Nodes?

Changed this to be more explicit, but we actually do allocate a fair number of these and I think it's worth avoiding the array allocation in the (much more common) 1-element case.

amcasey · 2017-07-17T18:28:37Z

src/services/textChanges.ts

        }

+        public static fromRefactorContext(context: RefactorContext) {
+            return new ChangeTracker(context.newLineCharacter === "\n" ? NewLineKind.LineFeed : NewLineKind.CarriageReturnLineFeed, context.rulesProvider);


This appears to be the third occurrence. Consider extracting context.newLineKind.

👍 but only found two, is the third in another file?

Yep, separate file.

amcasey · 2017-07-17T18:38:55Z

src/services/textChanges.ts


+        public replaceNodes(sourceFile: SourceFile, oldNodes: Node | Node[], newNodes: Node | Node[], options: ChangeNodeOptions & ChangeNodesOptions = {}) {
+            const startPosition = getAdjustedStartPosition(sourceFile, isArray(oldNodes) ? oldNodes[0] : oldNodes, options, Position.Start);
+            const endPosition = getAdjustedEndPosition(sourceFile, isArray(oldNodes) ? lastOrUndefined(oldNodes) : oldNodes, options);


From context, I assume that lastOrUndefined never returns undefined?

It's like the .NET version (actual signature is export function lastOrUndefined<T>(array: T[]): T | undefined {)

lastOrUndefined(ar) returns undefined if either the ar is undefined or ar is empty. We also have a firstOrUndefined(ar) that does the same thing except it reads the first element.

Generally, lastOrUndefined is just a shortcut for ar[ar.length - 1].

To clarify, this particular call to lastOrUndefined never returns undefined? If that's not the case, is getAdjustedEndPosition going to throw?

amcasey · 2017-07-17T18:40:26Z

src/services/textChanges.ts

+            }
+            // strip initial indentation (spaces or tabs) if text will be inserted in the middle of the line
+            // however keep indentation if it is was forced
+            text = posStartsLine || change.options.indentation !== undefined ? text : text.replace(/^\s+/, "");


This seems like an expensive way to TrimStart. There isn't a helper?

Also, I'm routinely confused by JS's short-circuiting operators, but won't text be a number if posStartsLine is defined?

We don't have a leading trim function yet; since we'd theoretically have to scan for all the Unicode whitespace I'm not sure it can be done more cheaply than that RegEx.

Added parens for clarity (the || expression is the first operand to ?)

It looks like there is a string.prototype.trim. Is there a reason to keep the whitespace at the end of the string?

amcasey · 2017-07-17T18:42:49Z

src/services/textChanges.ts

+        }
+
+        private getFormattedTextOfNode(node: Node, sourceFile: SourceFile, pos: number, useIndentationFromFile: boolean, options: ChangeNodeOptions) {
+            const nonFormattedText = getNonformattedText(node, sourceFile, this.newLine);


"nonFormatted" but "Nonformatted"?

amcasey · 2017-07-17T18:43:47Z

src/services/textChanges.ts

-                    ? change.options.indentation
-                    : change.useIndentationFromFile
-                        ? formatting.SmartIndenter.getIndentation(change.range.pos, sourceFile, formatOptions, posStartsLine || (change.options.prefix === this.newLineCharacter))
+                options.indentation !== undefined


The first two lines are just options.indentation ||, aren't they?

options.identation could be 0, which is falsy

amcasey · 2017-07-17T19:59:37Z

src/services/textChanges.ts

    }

+    export interface ChangeNodesOptions {
+        nodesSeparator?: string;


Is this also applicable to replaceNodeRange?

More accurate but impossibly-verbose names:
ChangeNodesOptions -> OptionsWhenInsertingMoreThanOneNewNode
ChangeNodeOptions -> OptionsWhenInsertingOneNode

I believe you've cleaned this up offline.

amcasey · 2017-08-03T22:37:43Z

It seems to be impossible to extract a single identifier into a method. Is that intentional? e.g. I might want to wrap access to a variable in a function so that I can bracket it with EH or logging.

minestarks · 2017-08-03T23:03:24Z

tests/cases/fourslash/extract-method14.ts

+    return __return;
+}
+function newFunction(i) {
+    return { i, __return: i++ };


You assign i before i++ happens. Meaning at the call site, i will not actually have the incremented value. In this case, that's fine because you return immediately, but here's a case that breaks:

function foo() { var i = 10; function inner() { return i++; // extract this line into file scope, the console output changes } inner(); console.log(i); } foo();

I agree that __return should be computed first.

amcasey · 2017-08-03T23:20:16Z

src/harness/fourslash.ts

+                    if (edits[j].span.start >= edits[i].span.start) {
+                        edits[j].span.start += editDelta;
+                    }
+                }


Could you eliminate the need to do this by walking the list backwards?

amcasey · 2017-08-03T23:24:00Z

src/harness/fourslash.ts

+            if (negative && isAvailable) {
+                this.raiseError(`verifyApplicableRefactorAvailableForRange failed - expected no refactor but found some.`);
+            }
+            if (!negative && !isAvailable) {


if [](start = 12, length = 2)

else if

amcasey · 2017-08-03T23:25:15Z

src/harness/fourslash.ts

            }
        }

+        public verifyRefactorAvailable(negative: boolean, name?: string, subName?: string) {


negative [](start = 39, length = 8)

We seem to have an existing pattern, but this is a particularly pointed example of using a predicate with a negative name.

This is to unify the verify.something and verify.not.something functions

amcasey · 2017-08-03T23:26:15Z

src/harness/fourslash.ts

        }

+        public applyRefactor(refactorName: string, actionName: string) {
+            const range = { pos: this.currentCaretPosition, end: this.selectionEnd === -1 ? this.currentCaretPosition : this.selectionEnd };


{ pos: this.currentCaretPosition, end: this.selectionEnd === -1 ? this.currentCaretPosition : this.selectionEnd } [](start = 26, length = 113)

Duplicated from above?

amcasey · 2017-08-03T23:37:10Z

src/services/textChanges.ts

+            return new ChangeTracker(getNewlineKind(context), context.rulesProvider);
+        }
+
+        public static fromRefactorContext(context: RefactorContext) {


fromRefactorContext [](start = 22, length = 19)

Can this be merged with fromCodeFixContext?

amcasey · 2017-08-03T23:38:31Z

src/services/textChanges.ts

+            return this;
+        }
+
+        private replaceWithMutiple(sourceFile: SourceFile, startPosition: number, endPosition: number, newNodes: ReadonlyArray<Node>, options: ChangeMultipleNodesOptions): this {


replaceWithMutiple [](start = 16, length = 18)

Typo

amcasey · 2017-08-03T23:40:35Z

src/services/textChanges.ts

+                const parts = change.nodes.map(n => this.getFormattedTextOfNode(n, sourceFile, pos, options));
+                text = parts.join(change.options.nodeSeparator);
+            }
+            else {


else { [](start = 12, length = 6)

Would it be worthwhile to assert the kind so that we get some kind of warning if a new one is added?

amcasey · 2017-08-03T23:41:26Z

src/services/textChanges.ts

+                text = this.getFormattedTextOfNode(change.node, sourceFile, pos, options);
+            }
+            // strip initial indentation (spaces or tabs) if text will be inserted in the middle of the line
+            // however keep indentation if it is was forced


Is this comment still accurate?

amcasey · 2017-08-03T23:42:05Z

src/services/textChanges.ts

-                        ? formatting.SmartIndenter.getIndentation(change.range.pos, sourceFile, formatOptions, posStartsLine || (change.options.prefix === this.newLineCharacter))
+                options.indentation !== undefined
+                    ? options.indentation
+                    : (options.useIndentationFromFile !== false)


(options.useIndentationFromFile !== false) [](start = 22, length = 42)

!options.useIndentationFromFile?

We want true and undefined to behave the same here, hence the odd conditional

amcasey · 2017-08-03T23:44:30Z

tests/baselines/reference/extractMethod1.js

+            ({ a } = newFunction(a));
+        }
+
+        function newFunction(a: any) {


any [](start = 32, length = 3)

number?

Something's going weird with the unit test harness setup - this gets : number when I do it in the editor

amcasey · 2017-08-03T23:46:45Z

tests/baselines/reference/extractMethod10.js

+    export interface I { x: number };
+    class C {
+        a() {
+            let z = 1;


let z = 1; [](start = 11, length = 11)

Did you intend to consume z or is this a way to confirm that it's ignored?

I didn't write it, but I'd go with the latter

minestarks · 2017-08-03T23:50:00Z

Type is referenced from context where it's not visible: minestarks@d1fa06e#diff-92a31e9489e4badbd019ff88953fe79f

amcasey · 2017-08-04T00:04:28Z

function M() {
    let a = [1, 2, 3]
    let x = 0
    a[x++]++
}

Extract a[x++]++ into the containing scope.

function M() {
    let a = [1, 2, 3]
    let x = 0
        ({ a, x } = newFunction(a, x));
}
function newFunction(a: number[], x: number) {
    a[x++]++;
    return { a, x };
}

There's a syntax error because of the missing semicolon between 0 and (.
AFAIK, there's no reason to assign to a.

amcasey · 2017-08-04T00:25:19Z

I suspect @minestarks or I may already have reported this, but if you extract a[x] into a global scope, x is not passed as a parameter and fails to bind.

function M() {
    let a = [1,2,3];
    let x = 0;
    a[x];
}

RyanCavanaugh · 2017-08-04T23:12:16Z

Fresh sheet of paper at #17625

RyanCavanaugh added 14 commits June 30, 2017 13:23

Rebased EM WIP

be27a1a

Change refactor api in fourslash

e798257

Add comments

bf176a6

Better baselines

0130ebb

Rename test we we can run it independent of unit tests

bbf5cd7

Change fourslash refactor API

9dd6bef

Remove invalid assert

f21b7ae

Fix undefined computation of length

674b1bf

Improved type baselines for EM .js

9367473

Add validation to fourslash test

1ea9972

Move text to diagnostic messages

1787fa8

Merge branch 'master' into extract-method

fb89c00

Properly widen literal param types + emit correct return statements f…

4aef5e7

…or exprs

Don't offer to extract lone identifiers

b5c5c40

msftclas added the cla-not-required label Jul 6, 2017

RyanCavanaugh added 2 commits July 5, 2017 17:35

Don't fail if span is zero-length

b1fa49f

Don't crash when span is empty

825237b

weswigham reviewed Jul 14, 2017

View reviewed changes

DanielRosenwasser reviewed Jul 14, 2017

View reviewed changes

amcasey reviewed Jul 17, 2017

View reviewed changes

Merge branch 'master' into extract-method

a97ab93

Add additional test

37ae957

minestarks reviewed Aug 3, 2017

View reviewed changes

amcasey reviewed Aug 3, 2017

View reviewed changes

RyanCavanaugh added 7 commits August 4, 2017 11:21

Fix some class scenarios (static methods, readonly inits in ctor)

2adb2b3

Prevent extraction of exported variables

ba37830

No extraction of ambient declarations

4dafb40

Put __return first

c1c7e83

Don't wrongly exclude accessor in indexed access exprs

a4c41b8

Share selection logic; fix typos

b5a88c1

Merge branch 'master' into extract-method

568d5f6

RyanCavanaugh mentioned this pull request Aug 4, 2017

Extract Method #17625

Merged

RyanCavanaugh closed this Aug 4, 2017

ananthakumaran mentioned this pull request Aug 15, 2017

add support for extract method ananthakumaran/tide#185

Merged

microsoft locked and limited conversation to collaborators Jun 14, 2018

Extract method #16960

Extract method #16960

Uh oh!

Conversation

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

Uh oh!

Uh oh!

ghost commented Jul 6, 2017

Uh oh!

RyanCavanaugh commented Jul 10, 2017

Uh oh!

weswigham left a comment

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!

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!

rosieks commented Jul 15, 2017

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!

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!

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 Jul 6, 2017 •

edited

Loading