GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Initial implementation of Extract Constant by amcasey · Pull Request #18783 · microsoft/TypeScript

Conversation

Andrew Casey (amcasey) commented Sep 27, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

[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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Andrew Casey (amcasey) added 5 commits

September 26, 2017 17:29


          Generalize extract method to handle constants as well

3eea1a9

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.


          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

Andrew Casey (amcasey) requested a review from Ryan Cavanaugh (RyanCavanaugh)

September 27, 2017 00:46

Andrew Casey (amcasey) commented

Sep 27, 2017

View reviewed changes

Comment thread

src/harness/unittests/extractMethods.ts

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

Andrew Casey (amcasey) Sep 27, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

amcasey reacted with thumbs up emoji

All reactions

👍 1 reaction

Andrew Casey (amcasey) commented

Sep 27, 2017

View reviewed changes

Comment thread

src/harness/unittests/extractConstants.ts

		/// <reference path="extractTestHelpers.ts" />

		namespace ts {
		describe("extractConstants", () => {

Andrew Casey (amcasey) Sep 27, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

amcasey reacted with thumbs up emoji

All reactions

👍 1 reaction

Ryan Cavanaugh (RyanCavanaugh) reviewed

Sep 27, 2017

View reviewed changes

Comment thread

src/harness/unittests/extractRanges.ts Outdated

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

Ryan Cavanaugh (RyanCavanaugh) Sep 27, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Can we symbolically refer to these strings?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Andrew Casey (amcasey) Sep 27, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

amcasey reacted with thumbs up emoji

All reactions

👍 1 reaction

Andrew Casey (amcasey) commented

Sep 27, 2017

View reviewed changes

Comment thread

src/services/refactors/extractSymbol.ts

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

Andrew Casey (amcasey) Sep 27, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Ryan Cavanaugh (@RyanCavanaugh), the interesting changes are here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

amcasey reacted with thumbs up emoji

All reactions

👍 1 reaction

Andrew Casey (amcasey) commented

Sep 27, 2017

View reviewed changes

Comment thread

tests/baselines/reference/extractMethod/extractMethod10.ts

		a() {
		let z = 1;
		return this./RENAME/newFunction();
		return this./RENAME/newMethod();

Andrew Casey (amcasey) Sep 27, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Slightly non-trivial baseline update

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

amcasey reacted with thumbs up emoji

All reactions

👍 1 reaction

Andrew Casey (amcasey) commented

Sep 27, 2017

View reviewed changes

Comment thread

tests/cases/fourslash/extract-method1.ts

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

Andrew Casey (amcasey) Sep 27, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

amcasey reacted with thumbs up emoji

All reactions

👍 1 reaction

Andrew Casey (amcasey) mentioned this pull request

Sep 27, 2017

Support JS baselines in Extract Method unit tests #18627

Closed

Ryan Cavanaugh (RyanCavanaugh) reviewed

Sep 27, 2017

View reviewed changes

Comment thread

src/services/refactors/extractSymbol.ts

		const visibleDeclarationsInExtractedRange: Symbol[] = [];

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

Ryan Cavanaugh (RyanCavanaugh) Sep 27, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Please use "extract local" on this expression

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Andrew Casey (amcasey) Sep 27, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

On which expression? targetRange.range?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Ryan Cavanaugh (RyanCavanaugh) Sep 27, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Everything to the left of the ?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Andrew Casey (amcasey) Sep 28, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

That breaks control flow type checking.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Ryan Cavanaugh (RyanCavanaugh) reviewed

Sep 27, 2017

View reviewed changes

Comment thread

src/services/refactors/extractSymbol.ts Outdated

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

Ryan Cavanaugh (RyanCavanaugh) Sep 27, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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>[]

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

amcasey reacted with thumbs up emoji

All reactions

👍 1 reaction

Ryan Cavanaugh (RyanCavanaugh) Sep 27, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Reading further down the function, I'm now confused

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Ryan Cavanaugh (RyanCavanaugh) Sep 27, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

.slice()

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Ryan Cavanaugh (RyanCavanaugh) reviewed

Sep 27, 2017

View reviewed changes

Comment thread

src/services/refactors/extractSymbol.ts

		break;
		}
		}

Ryan Cavanaugh (RyanCavanaugh) Sep 27, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Andrew Casey (amcasey) Sep 27, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

amcasey reacted with thumbs up emoji

All reactions

👍 1 reaction

Ryan Cavanaugh (RyanCavanaugh) reviewed

Sep 27, 2017

View reviewed changes

Comment thread

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) {

Ryan Cavanaugh (RyanCavanaugh) Sep 27, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Andrew Casey (amcasey) Sep 27, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

amcasey reacted with thumbs up emoji

All reactions

👍 1 reaction

Ryan Cavanaugh (RyanCavanaugh) commented Sep 27, 2017

Copy link

Copy Markdown

Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Andrew Casey (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

Andrew Casey (amcasey) commented Sep 27, 2017

Copy link

Copy Markdown

Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Andrew Casey (amcasey) commented Sep 28, 2017

Copy link

Copy Markdown

Member Author

Ryan Cavanaugh (@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          TODOs for repeated substitution

386e765

Andrew Casey (amcasey) commented Sep 28, 2017

Copy link

Copy Markdown

Member Author

Ryan Cavanaugh (@RyanCavanaugh) I believe all outstanding issues can be addressed in subsequent PRs.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Ryan Cavanaugh (RyanCavanaugh) approved these changes

Sep 28, 2017

View reviewed changes

Andrew Casey (amcasey) commented Sep 28, 2017

Copy link

Copy Markdown

Member Author

Many fixes to follow. 😄

RyanCavanaugh reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Andrew Casey (amcasey) merged commit 5f30106 into microsoft:master

Sep 28, 2017

Andrew Casey (amcasey) deleted the ExtractConstant branch

September 28, 2017 17:09

Microsoft (microsoft) locked and limited conversation to collaborators

Jun 14, 2018

Reviewers

Ryan Cavanaugh (RyanCavanaugh)

RyanCavanaugh approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

Andrew Casey (amcasey) commented Sep 27, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ryan Cavanaugh (RyanCavanaugh) Sep 27, 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!

Ryan Cavanaugh (RyanCavanaugh) commented Sep 27, 2017

Uh oh!

Andrew Casey (amcasey) commented Sep 27, 2017

Uh oh!

Andrew Casey (amcasey) commented Sep 28, 2017

Uh oh!

Andrew Casey (amcasey) commented Sep 28, 2017

Uh oh!

Andrew Casey (amcasey) commented Sep 28, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Andrew Casey (amcasey) commented Sep 27, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Ryan Cavanaugh (RyanCavanaugh) Sep 27, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.