Initial implementation of Extract Constant #18783

amcasey · 2017-09-27T00:46:57Z

[First, I propose standardizing on the terminology "extract function" and "extract constant", which, I argue, are more accurate than "extract method" and "extract local". This has no impact on the user-facing strings.]

This change generalizes the Extract Function refactoring (formerly, "Extract Method") to Extract Symbol and has it return both function and constant extractions. This required adding consideration for additional scopes into which it's sensible to insert constants but not functions. For Extract Function, the new scopes are immediately marked with errors.

This change also switches the tests to use the "public" API so that the two families of extractions can be distinguished. The baselines have been updated accordingly.

Outstanding work:

Rename the extractMethod baselines for consistency. I didn't want the diff to be even worse.
Improve the insertion locations of locals in function scopes. Right now, they are only inserted at the root level, which is frequently invalidated by the lack of access to variables from nested scopes.
We build a bad syntax tree when substitutions are applied more than once.

Major changes: 1) Instead of skipping undesirable scopes, include them and mark them with errors. Constants can be extracted into more scopes. 2) Update the tests to call through the "public" API. This caused some baseline changes. 3) Rename refactoring to "Extract Symbol" for generality. 4) Return a second ApplicableRefactorInfo for constants. Distinguish the two by splitting the action name.

amcasey · 2017-09-27T00:48:15Z

src/harness/unittests/extractMethods.ts

@@ -1,823 +0,0 @@
-/// <reference path="..\harness.ts" />


This wasn't detected as a rename because large chunks of it were pulled out into extractRanges.ts and extractSymbolTestHelpers.ts.

amcasey · 2017-09-27T00:49:25Z

src/harness/unittests/extractConstants.ts

+/// <reference path="extractTestHelpers.ts" />
+
+namespace ts {
+    describe("extractConstants", () => {


extractConstants [](start = 14, length = 16)

This test coverage isn't great because I didn't want to revise everything as soon as I updated the insertion locations.

RyanCavanaugh · 2017-09-27T00:50:02Z

src/harness/unittests/extractRanges.ts

+}
+        `,
+        [
+            "Cannot extract range containing conditional break or continue statements."


Can we symbolically refer to these strings?

Possibly. This was all copied from the old extractMethods.ts.

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

amcasey · 2017-09-27T00:50:39Z

src/services/refactors/extractSymbol.ts

-    const extractMethod: Refactor = {
-        name: "Extract Method",
-        description: Diagnostics.Extract_function.message,
+namespace ts.refactor.extractSymbol {


extractSymbol [](start = 22, length = 13)

@RyanCavanaugh, the interesting changes are here.

amcasey · 2017-09-27T00:51:14Z

tests/baselines/reference/extractMethod/extractMethod10.ts

 namespace A {
    export interface I { x: number };
    class C {
        a() {
            let z = 1;
-            return this./*RENAME*/newFunction();
+            return this./*RENAME*/newMethod();


newMethod [](start = 34, length = 9)

Slightly non-trivial baseline update

amcasey · 2017-09-27T00:52:51Z

tests/cases/fourslash/extract-method1.ts

-    refactorName: "Extract Method",
-    actionName: "scope_0",
+    refactorName: "Extract Symbol",
+    actionName: "function_scope_1",


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

0 is now the method itself. We won't insert a function there, but we will insert a local.

RyanCavanaugh · 2017-09-27T01:14:19Z

src/services/refactors/extractSymbol.ts

        const visibleDeclarationsInExtractedRange: Symbol[] = [];

+        const expressionDiagnostics =
+            isReadonlyArray(targetRange.range) && !(targetRange.range.length === 1 && isExpressionStatement(targetRange.range[0]))


Please use "extract local" on this expression

On which expression? targetRange.range?

Everything to the left of the ?

That breaks control flow type checking.

RyanCavanaugh · 2017-09-27T01:17:51Z

src/services/refactors/extractSymbol.ts

+                isFunctionLikeDeclaration(scope) && scope.kind !== SyntaxKind.FunctionDeclaration
+                    ? [createDiagnosticForNode(scope, Messages.CannotExtractToOtherFunctionLike)]
+                    : []);
+            constantErrorsPerScope.push(expressionDiagnostics);


The re-use of the same array object going into multiple positions in constantErrorsPerScope makes me nervous since this is currently mutable - let's change both to ReadonlyArray<Diagnostic>[]

Reading further down the function, I'm now confused

RyanCavanaugh · 2017-09-27T01:28:34Z

src/services/refactors/extractSymbol.ts

+                break;
+            }
+        }
+


I would assert prevChild !== undefined here since it's not obvious by construction that this is impossible

Sure, but it follows from the fact that extraction position is in a child that begins before the extraction position (i.e. there will be at least one iteration and that iteration won't kick out based on the position).

RyanCavanaugh · 2017-09-27T01:31:10Z

src/harness/unittests/extractFunctions.ts

+        // Confirm that the constraint is preserved.
+        testExtractMethod("extractMethod15",
+            `function F<T>(t1: T) {
+    function G<U extends T[]>(t2: U) {


Was it a test correctness thing to change F to G here?

We de-dup actions by description and I wanted to see all of them.

RyanCavanaugh · 2017-09-27T01:57:23Z

Testcase we were looking at

/*!
* MIT APACHE
*/
"use strict";
"use module";


namespace Y { }

namespace X {
    namespace Y {
        export const j = 10;
        namespace Z {
            export const y = j * j;
        }
    }
}

as well as

/// <reference ...

and

x = y; // Extract the whole thing

amcasey · 2017-09-27T18:15:51Z

I can repro the assert in a unit test. This looks relevant: https://github.com/Microsoft/TypeScript/pull/18484/files#diff-20a9fa4d3e9d09f60c275a3dda245867

amcasey · 2017-09-28T01:06:44Z

@RyanCavanaugh The problem we were seeing (in our case, an assert about token spans) will occur any time a substitution is applied more than once. Since the same issue affects Extract Function, I'm going to handle it in a separate PR.

amcasey · 2017-09-28T01:11:43Z

@RyanCavanaugh I believe all outstanding issues can be addressed in subsequent PRs.

amcasey · 2017-09-28T17:09:39Z

Many fixes to follow. 😄

amcasey added 5 commits September 26, 2017 17:29

Rename extractMethod.ts to extractSymbol.ts

eb1fb5c

Add simple tests for Extract Constant

2601bbc

Rename extractMethods.ts to extractFunctions.ts for consistency

52ab05e

Split range tests and helpers out of extractFunctions.ts

697bce7

amcasey requested a review from RyanCavanaugh September 27, 2017 00:46

amcasey commented Sep 27, 2017

View reviewed changes

RyanCavanaugh reviewed Sep 27, 2017

View reviewed changes

amcasey commented Sep 27, 2017

View reviewed changes

amcasey mentioned this pull request Sep 27, 2017

Support JS baselines in Extract Method unit tests #18627

Closed

RyanCavanaugh reviewed Sep 27, 2017

View reviewed changes

amcasey added 3 commits September 27, 2017 10:30

Forbid extraction of constants to class scopes in JS

cb6037b

Use resources, rather than string literals, in test baselines

13e60bc

Add additional TODO about insertion positions

e6bfce1

TODOs for repeated substitution

386e765

RyanCavanaugh approved these changes Sep 28, 2017

View reviewed changes

amcasey merged commit 5f30106 into microsoft:master Sep 28, 2017

amcasey deleted the ExtractConstant branch September 28, 2017 17:09

microsoft locked and limited conversation to collaborators Jun 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial implementation of Extract Constant #18783

Initial implementation of Extract Constant #18783

amcasey commented Sep 27, 2017 •

edited

Loading

amcasey Sep 27, 2017

amcasey Sep 27, 2017

RyanCavanaugh Sep 27, 2017

amcasey Sep 27, 2017

amcasey Sep 27, 2017

amcasey Sep 27, 2017

amcasey Sep 27, 2017

RyanCavanaugh Sep 27, 2017

amcasey Sep 27, 2017

RyanCavanaugh Sep 27, 2017

amcasey Sep 28, 2017

RyanCavanaugh Sep 27, 2017 •

edited

Loading

RyanCavanaugh Sep 27, 2017

RyanCavanaugh Sep 27, 2017

RyanCavanaugh Sep 27, 2017

amcasey Sep 27, 2017

RyanCavanaugh Sep 27, 2017

amcasey Sep 27, 2017

RyanCavanaugh commented Sep 27, 2017

amcasey commented Sep 27, 2017

amcasey commented Sep 28, 2017

amcasey commented Sep 28, 2017

amcasey commented Sep 28, 2017

Initial implementation of Extract Constant #18783

Initial implementation of Extract Constant #18783

Conversation

amcasey commented Sep 27, 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

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

RyanCavanaugh Sep 27, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RyanCavanaugh commented Sep 27, 2017

amcasey commented Sep 27, 2017

amcasey commented Sep 28, 2017

amcasey commented Sep 28, 2017

amcasey commented Sep 28, 2017

amcasey commented Sep 27, 2017 •

edited

Loading

RyanCavanaugh Sep 27, 2017 •

edited

Loading