Extract Method #17625

RyanCavanaugh · 2017-08-04T23:11:38Z

Continuation of #16960

amcasey · 2017-08-07T18:27:41Z

Extracting [0] from

var q = [0]
q[0]++

yields

var q = newFunction()q[0]++

function newFunction() {
    return [0];
}

It looks like a semicolon is required between () and q.

amcasey · 2017-08-07T18:37:01Z

Does extracting the LHS of an assignment ever make sense?

amcasey · 2017-08-07T19:55:57Z

Channelling Vladimir Reshetnikov: extracting []

function F() {
    return[]
}

yields

function F() {
    returnnewFunction()
    function newFunction() {
        return [];
    }
}

amcasey · 2017-08-07T19:58:16Z

Should it be impossible to extract one or more empty statements?

amcasey · 2017-08-09T20:25:15Z

.vscode/tasks.json

@@ -18,6 +18,13 @@
            "problemMatcher": [
                "$tsc"
            ]
+        },
+        {


What does this do?

Allows debugging single tests from VS Code by pressing F5 in the testcase's editor window

That sounds great. How does it do that?

amcasey · 2017-08-09T20:26:38Z

src/compiler/checker.ts

-                location = getParseTreeNode(location);
-                return resolveName(location, escapeLeadingUnderscores(name), meaning, /*nameNotFoundMessage*/ undefined, escapeLeadingUnderscores(name));
+            resolveName(name, location, meaning) {
+                return resolveName(location, name as __String, meaning, /*nameNotFoundMessage*/ undefined, /*nameArg*/ undefined);


Does name as __String perform the same escaping as escapeLeadingUnderscores or was that unnecessary?

It does not. It is probably needed, as without it, submitting the string __proto__ will fail to find the correct symbol (since we will attempt to find an internal symbol named __proto__, rather than looking up ___proto__, which is where we keep the real user symbol for that name.). The caveat to escaping the input is that it becomes impossible to look up internal symbols like __call - but external users shouldn't be doing that anyway, right? Anyways, if you don't call escapeLeadingUnderscores, the resolveName signature should use __String instead of string (rendering the cast unneeded), to punt the escape required/not required issue to the API consumer, rather than doing something unsafe here.

amcasey · 2017-08-09T20:27:29Z

src/compiler/utilities.ts

@@ -693,7 +693,7 @@ namespace ts {
                // At this point, node is either a qualified name or an identifier
                Debug.assert(node.kind === SyntaxKind.Identifier || node.kind === SyntaxKind.QualifiedName || node.kind === SyntaxKind.PropertyAccessExpression,
                    "'node' was expected to be a qualified name, identifier or property access in 'isPartOfTypeNode'.");
-                // falls through
+            // falls through


Intentional?

Below too.

In reply to: 132297135 [](ancestors = 132297135)

amcasey · 2017-08-09T20:34:39Z

src/compiler/utilities.ts

+        return isStatementKindButNotDeclarationKind(kind)
+            || isDeclarationStatementKind(kind)
+            || node.kind === SyntaxKind.Block;
+    }


What about just having the only caller check node.kind === SyntaxKind.Block || isStatement(node)? This function is kind of hard to explain as an independent entity.

amcasey · 2017-08-09T20:38:25Z

src/harness/fourslash.ts

-                assert.isTrue(firstEditEnd <= edits[i + 1].span.start);
-            }
+            // Copy this so we don't ruin someone else's copy
+            edits = JSON.parse(JSON.stringify(edits));


JSON.parse(JSON.stringify(edits)); [](start = 20, length = 34)

This seems like a particularly expensive way to accomplish the copy. #WontFix

Never mind - didn't see that this was test code.

In reply to: 132299991 [](ancestors = 132299991)

amcasey · 2017-08-09T20:39:56Z

src/harness/fourslash.ts

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


Could we avoid this work by sorting the edits in descending order of start position? (That would also make it easy to restore the no-overlap assertion.)

Given that this is test code, I don't particularly care about the extra work, but having the assert seems worthwhile.

In reply to: 132300325 [](ancestors = 132300325)

amcasey · 2017-08-09T20:43:13Z

src/harness/fourslash.ts

+            const isAvailable = refactors.length > 0;
+
+            if (negative && isAvailable) {
+                this.raiseError(`verifyApplicableRefactorAvailableForRange failed - expected no refactor but found some.`);


some [](start = 115, length = 4)

Would it be helpful to list the refactors we found?

amcasey · 2017-08-09T21:24:09Z

src/services/refactors/extractMethod.ts

+     * Each returned ExtractResultForScope corresponds to a possible target scope and is either a set of changes
+     * or an error explaining why we can't extract into that scope.
+     */
+    export function extractRange(targetRange: TargetRange, context: RefactorContext, requestedChangesIndex: number = undefined): ReadonlyArray<ExtractResultForScope> | undefined {


extractRange [](start = 20, length = 12)

Personally, if would find a name like "getExtractResults" or "getPossibleExtractions" clearer. In particular, it's very hard to tell at the call site what this method is expected to return.

amcasey · 2017-08-09T21:25:37Z

src/services/refactors/extractMethod.ts

+                continue;
+            }
+
+            // Don't issue refactorings with duplicated names


Don't issue refactorings with duplicated names [](start = 15, length = 46)

Is the order of extractions guaranteed? Which duplicate is expected to "win"?

(leave comment indicating that the inner scope always 'wins' due to the list being order "inner scopes first")

amcasey · 2017-08-09T21:26:27Z

src/services/refactors/extractMethod.ts

+
+            // Don't issue refactorings with duplicated names
+            const description = formatStringFromArgs(Diagnostics.Extract_function_into_0.message, [extr.scopeDescription]);
+            if (!usedNames.get(description)) {


get [](start = 27, length = 3)

has might be more explicit.

amcasey · 2017-08-09T21:27:42Z

src/services/refactors/extractMethod.ts

+                    name: `scope_${i}`
+                });
+            }
+            i++;


i++; [](start = 12, length = 4)

We increment whether or not we add an entry to the list. Is that intentional? If not, the length of actions is probably sufficient.

Looks like it's important to leave gaps in the sequence. Consider adding a comment to that effect.

In reply to: 132311523 [](ancestors = 132311523)

amcasey · 2017-08-09T21:29:53Z

src/services/refactors/extractMethod.ts

+        if (extractions === undefined) {
+            // Scope is no longer valid from when the user issued the refactor
+            // return undefined;
+            return undefined;


This should be very rare, right? Cases like the file refreshing in the background?

amcasey · 2017-08-09T21:31:28Z

src/services/refactors/extractMethod.ts

+
+        const parsedIndexMatch = /scope_(\d+)/.exec(actionName);
+        if (!parsedIndexMatch) {
+            throw new Error("Expected to match the regexp");


throw new Error("Expected to match the regexp"); [](start = 12, length = 48)

Can this occur other than through programmer error?

amcasey · 2017-08-09T21:32:52Z

src/services/refactors/extractMethod.ts

+        const rangeToExtract = getRangeToExtract(context.file, { start: context.startPosition, length });
+        const targetRange: TargetRange = rangeToExtract.targetRange;
+
+        const parsedIndexMatch = /scope_(\d+)/.exec(actionName);


/scope_(\d+)/ [](start = 33, length = 13)

Is this a substring match? If so, would ^ and $ help?

amcasey · 2017-08-09T21:36:44Z

src/services/refactors/extractMethod.ts

+    } | {
+            readonly targetRange: TargetRange;
+            readonly errors?: never;
+        };


amcasey · 2017-08-09T21:44:42Z

src/services/refactors/extractMethod.ts

+            return { targetRange: { range: statements, facts: rangeFacts, declarations } };
+        }
+        else {
+            // We have a single expression (start)


expression [](start = 32, length = 10)

Why does it have to be an expression? Couldn't it be (e.g.) a statement?

We check isStatement below...

In reply to: 132315243 [](ancestors = 132315243)

"single node"

amcasey · 2017-08-09T21:45:14Z

src/services/refactors/extractMethod.ts

+                return { errors };
+            }
+
+            // If our selection is the expression in an ExrpessionStatement, expand


ExrpessionStatement [](start = 56, length = 19)

Typo

amcasey · 2017-08-09T21:47:49Z

src/services/refactors/extractMethod.ts

+                ? [start]
+                : start.parent && start.parent.kind === SyntaxKind.ExpressionStatement
+                    ? [start.parent as Statement]
+                    : <Expression>start;


start [](start = 22, length = 17)

Is this cast guaranteed to succeed? There are no other types that start could have?

Match assertion styles

amcasey · 2017-08-09T21:48:40Z

src/services/refactors/extractMethod.ts

+                return [createDiagnosticForNode(nodeToCheck, Messages.CannotExtractAmbientBlock)];
+            }
+
+            // If we're in a class, see if we're in a static region (static property initializer, static method, class constructor parameter default) or not


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

"whether"

amcasey · 2017-08-09T21:49:56Z

src/services/refactors/extractMethod.ts

+                    }
+                    current = current.parent;
+                }
+            }


Consider pulling this out into its own method.

amcasey · 2017-08-09T21:51:26Z

src/services/refactors/extractMethod.ts

+                if (isDeclaration(node)) {
+                    const declaringNode = (node.kind === SyntaxKind.VariableDeclaration) ? node.parent.parent : node;
+                    if (hasModifier(declaringNode, ModifierFlags.Export)) {
+                        (errors || (errors = []).push(createDiagnosticForNode(node, Messages.CannotExtractExportedEntity)));


) [](start = 47, length = 1)

Suppose to be ))?

amcasey · 2017-08-09T21:55:21Z

src/services/refactors/extractMethod.ts

+    }
+
+    function isValidExtractionTarget(node: Node) {
+        // Note that we don't use isFunctionLike because we don't want to put the extracted closure *inside* a method


we don't want to put the extracted closure inside a method [](start = 57, length = 60)

I'm sure I'm missing something obvious, but why not?

this stranding

amcasey · 2017-08-09T22:06:32Z

src/services/refactors/extractMethod.ts

+            // A function parameter's initializer is actually in the outer scope, not the function declaration
+            if (current && current.parent && current.parent.kind === SyntaxKind.Parameter) {
+                // Skip all the way to the outer scope of the function that declared this parameter
+                current = current.parent.parent.parent;


current.parent.parent.parent [](start = 26, length = 28)

This makes me a bit nervous. Consider asserting that the result is of an appropriate kind.

Or use findAncestor - I believe it should assert the kind, once found.

amcasey · 2017-08-09T22:24:53Z

src/services/refactors/extractMethod.ts

+        }
+        else {
+            const file = scope.getSourceFile();
+            functionNameText = getUniqueName(n => !file.identifiers.has(n as string));


as string [](start = 74, length = 9)

I didn't think direct conversion between __String and string was safe. Is this a special case?

This is fine just because getUniqueName always returns a string which begins with newFunction, which would never need to be escaped.... that's a nonlocal assumption to make, though, and would need to be documented in a comment, IMO. But all use sites for getUniqueName require a string, so should really just operate on strings instead of escaped strings.

amcasey · 2017-08-09T22:25:53Z

src/services/refactors/extractMethod.ts

+    }
+
+    function getUniqueName(isNameOkay: (name: __String) => boolean) {
+        let functionNameText = "newFunction" as __String;


__String [](start = 48, length = 8)

It looks like the only consumers expect string.

amcasey · 2017-08-09T22:32:47Z

src/services/refactors/extractMethod.ts

+        }
+    }
+
+    function isModuleBlock(n: Node): n is ModuleBlock {


I would have guessed there was a family of such type guards in a shared location.

amcasey · 2017-08-09T22:34:02Z

src/services/refactors/extractMethod.ts

+     * such as `import x from 'y'` -- the 'y' is a StringLiteral but is *not* an expression
+     * in the sense of something that you could extract on
+     */
+    function isLegalExpressionExtraction(node: Node): boolean {


isLegalExpressionExtraction [](start = 13, length = 27)

"isExtractableExpression"?

amcasey · 2017-08-09T22:34:16Z

src/services/refactors/extractMethod.ts

+     * such as `import x from 'y'` -- the 'y' is a StringLiteral but is *not* an expression
+     * in the sense of something that you could extract on
+     */
+    function isLegalExpressionExtraction(node: Node): boolean {


boolean [](start = 54, length = 7)

node is Expression?

amcasey · 2017-08-09T22:35:02Z

src/services/refactors/extractMethod.ts

+        switch (node.kind) {
+            case SyntaxKind.StringLiteral:
+                return node.parent.kind !== SyntaxKind.ImportDeclaration &&
+                    node.parent.kind !== SyntaxKind.ImportSpecifier;


The parent should probably also not be an export.

amcasey · 2017-08-09T22:37:11Z

src/services/refactors/extractMethod.ts

+    }
+
+    function spanContainsNode(span: TextSpan, node: Node, file: SourceFile): boolean {
+        return textSpanContainsPosition(span, node.getStart(file)) &&


textSpanContainsPosition(span, node.getStart(file)) [](start = 15, length = 52)

Why not just compare the start positions? (Both for symmetry and to avoid the unnecessary comparison between the node start and the span end.)

amcasey · 2017-08-09T22:38:09Z

src/services/refactors/extractMethod.ts

+    }
+
+    function getParentNodeInSpan(node: Node, file: SourceFile, span: TextSpan): Node {
+        while (node) {


node [](start = 15, length = 4)

If we don't expect the argument to be undefined, the loop exit condition is probably redundant.

amcasey · 2017-08-09T22:41:55Z

src/services/refactors/extractMethod.ts

+            }
+            if (range.facts & RangeFacts.InStaticRegion) {
+                modifiers.push(createToken(SyntaxKind.StaticKeyword));
+            }


Would it be more conventional to put static before async?

Conventional -> correct 😅

amcasey · 2017-08-09T22:42:45Z

src/services/refactors/extractMethod.ts

+                modifiers.push(createToken(SyntaxKind.StaticKeyword));
+            }
+            newFunction = createMethod(
+                /*decorators*/ undefined,


/decorators/ undefined [](start = 16, length = 24)

We might eventually want to propagate decorators (e.g. @readonly on a parameter.

amcasey · 2017-08-09T22:44:06Z

src/services/refactors/extractMethod.ts

+                range.facts & RangeFacts.IsGenerator ? createToken(SyntaxKind.AsteriskToken) : undefined,
+                functionName,
+                /*questionToken*/ undefined,
+                /*typeParameters*/[],


/typeParameters/[] [](start = 16, length = 20)

Don't we need to have type parameters if they're consumed by the extracted code and not in scope in the new method?

amcasey · 2017-08-09T22:47:57Z

src/services/refactors/extractMethod.ts

+                // has both writes and return, need to create variable declaration to hold return value;
+                newNodes.push(createVariableStatement(
+                    /*modifiers*/ undefined,
+                    [createVariableDeclaration(returnValueProperty, createKeywordTypeNode(SyntaxKind.AnyKeyword))]


createKeywordTypeNode(SyntaxKind.AnyKeyword) [](start = 68, length = 44)

Why not use returnType?

amcasey · 2017-08-09T22:50:25Z

src/services/refactors/extractMethod.ts

+        }
+
+        function generateReturnValueProperty() {
+            return "__return";


"__return" [](start = 19, length = 10)

Does this have to be made unique?

amcasey · 2017-08-09T22:53:05Z

src/services/refactors/extractMethod.ts

+            if (range.facts & RangeFacts.HasReturn) {
+                newNodes.push(createReturn(call));
+            }
+            else if (isArray(range.range)) {


isArray(range.range) [](start = 21, length = 20)

Consider extracting a local for this. It has the opposite sense, but "isExpression" might be the most readable.

amcasey · 2017-08-09T22:55:51Z

src/services/refactors/extractMethod.ts

+                const rewrittenStatements = visitNodes(statements, visitor).slice();
+                if (writes && !(range.facts & RangeFacts.HasReturn) && isStatement(body)) {
+                    // add return at the end to propagate writes back in case if control flow falls out of the function body
+                    // it is ok to know that range has at least one return since it we only allow unconditional returns


This sentence has some extra words in it.

amcasey · 2017-08-09T22:56:53Z

src/services/refactors/extractMethod.ts

+        return n.kind === SyntaxKind.ModuleBlock;
+    }
+
+    function isReadonlyArray(v: any): v is ReadonlyArray<any> {


isReadonlyArray [](start = 13, length = 15)

I suspect this could replace some of the isArray calls above.

amcasey · 2017-08-09T23:01:40Z

src/services/refactors/extractMethod.ts

+            }
+
+            if (isAssignmentExpression(node)) {
+                const savedValueUsage = valueUsage;


valueUsage [](start = 40, length = 10)

Personally, I would just make valueUsage a parameter of collectUsages. That would eliminate both the side effects and the saving/restoration.

amcasey

I'm out of steam for today but I have no fundamental objections to checking this in as-is.

amcasey · 2017-08-10T00:00:52Z

Did we end up doing anything about chained assignments after our discussion on the unsquashed PR?

weswigham · 2017-08-10T00:13:29Z

src/compiler/checker.ts

-                location = getParseTreeNode(location);
-                return resolveName(location, escapeLeadingUnderscores(name), meaning, /*nameNotFoundMessage*/ undefined, escapeLeadingUnderscores(name));
+            resolveName(name, location, meaning) {
+                return resolveName(location, name as __String, meaning, /*nameNotFoundMessage*/ undefined, /*nameArg*/ undefined);


It does not. It is probably needed, as without it, submitting the string __proto__ will fail to find the correct symbol (since we will attempt to find an internal symbol named __proto__, rather than looking up ___proto__, which is where we keep the real user symbol for that name.). The caveat to escaping the input is that it becomes impossible to look up internal symbols like __call - but external users shouldn't be doing that anyway, right? Anyways, if you don't call escapeLeadingUnderscores, the resolveName signature should use __String instead of string (rendering the cast unneeded), to punt the escape required/not required issue to the API consumer, rather than doing something unsafe here.

weswigham · 2017-08-10T00:17:55Z

src/compiler/types.ts

@@ -2606,9 +2606,8 @@ namespace ts {
         * Does not include properties of primitive types.
         */
        /* @internal */ getAllPossiblePropertiesOfType(type: Type): Symbol[];
-
+        /* @internal */ resolveName(name: string, location: Node, meaning: SymbolFlags): Symbol | undefined;


See comment on the implementation.

weswigham · 2017-08-10T00:23:32Z

src/compiler/utilities.ts

+            || node.kind === SyntaxKind.Block;
+    }
+
+    function isBlockStatement(node: Node): node is Block {


This name seems misleading - it checks if the node is a block, and that block isn't in a try or a catch (but not finally?), and that it's not a function block? I think the simpler check for use with isStatement is node.kind === SyntaxKind.Block && isBlockLike(node.parent). Since a block is only a block statement when it occurs..... directly inside a block. I don't think we have an isBlockLike written yet, but we do have the BlockLike union type already - which if I'm not mistaken, covers a few more cases than here.

I missed the finally case; your suggested implementation sounds much better.

The name is correct (or at least the best one I can come up with?). A block statement is a block that occurs in a grammatical context where a statement is allowed.

weswigham · 2017-08-10T01:40:40Z

src/services/refactors/extractMethod.ts

+            // A function parameter's initializer is actually in the outer scope, not the function declaration
+            if (current && current.parent && current.parent.kind === SyntaxKind.Parameter) {
+                // Skip all the way to the outer scope of the function that declared this parameter
+                current = current.parent.parent.parent;


Or use findAncestor - I believe it should assert the kind, once found.

weswigham · 2017-08-10T01:44:12Z

src/services/refactors/extractMethod.ts

+        }
+        else {
+            const file = scope.getSourceFile();
+            functionNameText = getUniqueName(n => !file.identifiers.has(n as string));


This is fine just because getUniqueName always returns a string which begins with newFunction, which would never need to be escaped.... that's a nonlocal assumption to make, though, and would need to be documented in a comment, IMO. But all use sites for getUniqueName require a string, so should really just operate on strings instead of escaped strings.

Extract Method (squash)

c7f665f

msftclas added the cla-not-required label Aug 4, 2017

RyanCavanaugh mentioned this pull request Aug 4, 2017

Extract method #16960

Closed

amcasey reviewed Aug 9, 2017

View reviewed changes

src/services/refactors/extractMethod.ts

} | {

readonly targetRange: TargetRange;

readonly errors?: never;

};

Copy link

Member

amcasey Aug 9, 2017

Choose a reason for hiding this comment

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

Cute. :)

amcasey reviewed Aug 9, 2017

View reviewed changes

src/services/refactors/extractMethod.ts Outdated

}

current = current.parent;

}

}

Copy link

Member

amcasey Aug 9, 2017

Choose a reason for hiding this comment

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

Consider pulling this out into its own method.

amcasey reviewed Aug 9, 2017

View reviewed changes

amcasey approved these changes Aug 9, 2017

View reviewed changes

weswigham reviewed Aug 10, 2017

View reviewed changes

RyanCavanaugh added 8 commits August 10, 2017 13:07

Various fixes

12403d9

Various fixes

1ad411e

Style fixups

f957429

Style fixes

a04633c

Use the function stack!

db37cea

Use isReadonlyArray

24de14a

Merge branch 'master' into extract-method-2

c27ee81

Merge

8a92315

RyanCavanaugh merged commit c110ecb into microsoft:master Aug 11, 2017

microsoft locked and limited conversation to collaborators Jun 14, 2018

Extract Method #17625

Extract Method #17625

Uh oh!

Conversation

RyanCavanaugh commented Aug 4, 2017

Uh oh!

amcasey commented Aug 7, 2017

Uh oh!

amcasey commented Aug 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amcasey commented Aug 7, 2017

Uh oh!

amcasey commented Aug 7, 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!

amcasey Aug 9, 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!

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

amcasey commented Aug 7, 2017 •

edited

Loading

amcasey Aug 9, 2017 •

edited

Loading

weswigham Aug 10, 2017 •

edited

Loading