From 41676248e545428d2ef4125781616a281a102302 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Thu, 28 Sep 2017 14:54:52 -0700 Subject: [PATCH] Eliminate special case for extracting from a binary operator chain 1) It assumed left-associativity and was, therefore, wrong for (e.g.) exponentiation. 2) Arguably, if a user selects `a + |b + c|`, they want to extract `b + c`, not `a + b + c`. Not being able to do so is surprising (and we may eventually want to allow it), but so is having the rest of the least-common subtree extracted. Fixes #18268 --- src/harness/unittests/extractFunctions.ts | 10 --- src/harness/unittests/extractRanges.ts | 25 ++++--- src/services/refactors/extractSymbol.ts | 31 ++------- .../extractFunction/extractFunction8.ts | 65 ------------------- 4 files changed, 17 insertions(+), 114 deletions(-) delete mode 100644 tests/baselines/reference/extractFunction/extractFunction8.ts diff --git a/src/harness/unittests/extractFunctions.ts b/src/harness/unittests/extractFunctions.ts index 522ea7b1293..27c21ce3fb6 100644 --- a/src/harness/unittests/extractFunctions.ts +++ b/src/harness/unittests/extractFunctions.ts @@ -109,16 +109,6 @@ namespace ts { return C.foo();|] } } -}`); - testExtractFunction("extractFunction8", - `namespace A { - let x = 1; - namespace B { - function a() { - let a1 = 1; - return 1 + [#|a1 + x|] + 100; - } - } }`); testExtractFunction("extractFunction9", `namespace A { diff --git a/src/harness/unittests/extractRanges.ts b/src/harness/unittests/extractRanges.ts index fcffffeba9d..2ddcac482a9 100644 --- a/src/harness/unittests/extractRanges.ts +++ b/src/harness/unittests/extractRanges.ts @@ -152,18 +152,6 @@ namespace ts { } } `); - testExtractRange(` - function f() { - return [$|1 + [#|2 + 3|]|]; - } - } - `); - testExtractRange(` - function f() { - return [$|1 + 2 + [#|3 + 4|]|]; - } - } - `); }); testExtractRangeFailed("extractRangeFailed1", @@ -311,7 +299,18 @@ switch (x) { testExtractRangeFailed("extractRangeFailed9", `var x = ([#||]1 + 2);`, [ - "Cannot extract empty range." + refactor.extractSymbol.Messages.CannotExtractEmpty.message + ]); + + testExtractRangeFailed("extractRangeFailed10", + ` + function f() { + return 1 + [#|2 + 3|]; + } + } + `, + [ + refactor.extractSymbol.Messages.CannotExtractRange.message ]); testExtractRangeFailed("extract-method-not-for-token-expression-statement", `[#|a|]`, [refactor.extractSymbol.Messages.CannotExtractIdentifier.message]); diff --git a/src/services/refactors/extractSymbol.ts b/src/services/refactors/extractSymbol.ts index 375acb7e585..b88a013f52b 100644 --- a/src/services/refactors/extractSymbol.ts +++ b/src/services/refactors/extractSymbol.ts @@ -201,9 +201,9 @@ namespace ts.refactor.extractSymbol { // Walk up starting from the the start position until we find a non-SourceFile node that subsumes the selected span. // This may fail (e.g. you select two statements in the root of a source file) - let start = getParentNodeInSpan(getTokenAtPosition(sourceFile, span.start, /*includeJsDocComment*/ false), sourceFile, span); + const start = getParentNodeInSpan(getTokenAtPosition(sourceFile, span.start, /*includeJsDocComment*/ false), sourceFile, span); // Do the same for the ending position - let end = getParentNodeInSpan(findTokenOnLeftOfPosition(sourceFile, textSpanEnd(span)), sourceFile, span); + const end = getParentNodeInSpan(findTokenOnLeftOfPosition(sourceFile, textSpanEnd(span)), sourceFile, span); const declarations: Symbol[] = []; @@ -217,31 +217,10 @@ namespace ts.refactor.extractSymbol { } if (start.parent !== end.parent) { - // handle cases like 1 + [2 + 3] + 4 - // user selection is marked with []. - // in this case 2 + 3 does not belong to the same tree node - // instead the shape of the tree looks like this: - // + - // / \ - // + 4 - // / \ - // + 3 - // / \ - // 1 2 - // in this case there is no such one node that covers ends of selection and is located inside the selection - // to handle this we check if both start and end of the selection belong to some binary operation - // and start node is parented by the parent of the end node - // if this is the case - expand the selection to the entire parent of end node (in this case it will be [1 + 2 + 3] + 4) - const startParent = skipParentheses(start.parent); - const endParent = skipParentheses(end.parent); - if (isBinaryExpression(startParent) && isBinaryExpression(endParent) && isNodeDescendantOf(startParent, endParent)) { - start = end = endParent; - } - else { - // start and end nodes belong to different subtrees - return { errors: [createFileDiagnostic(sourceFile, span.start, length, Messages.CannotExtractRange)] }; - } + // start and end nodes belong to different subtrees + return { errors: [createFileDiagnostic(sourceFile, span.start, length, Messages.CannotExtractRange)] }; } + if (start !== end) { // start and end should be statements and parent should be either block or a source file if (!isBlockLike(start.parent)) { diff --git a/tests/baselines/reference/extractFunction/extractFunction8.ts b/tests/baselines/reference/extractFunction/extractFunction8.ts deleted file mode 100644 index adb8adbe56a..00000000000 --- a/tests/baselines/reference/extractFunction/extractFunction8.ts +++ /dev/null @@ -1,65 +0,0 @@ -// ==ORIGINAL== -namespace A { - let x = 1; - namespace B { - function a() { - let a1 = 1; - return 1 + a1 + x + 100; - } - } -} -// ==SCOPE::Extract to inner function in function 'a'== -namespace A { - let x = 1; - namespace B { - function a() { - let a1 = 1; - return /*RENAME*/newFunction() + 100; - - function newFunction() { - return 1 + a1 + x; - } - } - } -} -// ==SCOPE::Extract to function in namespace 'B'== -namespace A { - let x = 1; - namespace B { - function a() { - let a1 = 1; - return /*RENAME*/newFunction(a1) + 100; - } - - function newFunction(a1: number) { - return 1 + a1 + x; - } - } -} -// ==SCOPE::Extract to function in namespace 'A'== -namespace A { - let x = 1; - namespace B { - function a() { - let a1 = 1; - return /*RENAME*/newFunction(a1) + 100; - } - } - - function newFunction(a1: number) { - return 1 + a1 + x; - } -} -// ==SCOPE::Extract to function in global scope== -namespace A { - let x = 1; - namespace B { - function a() { - let a1 = 1; - return /*RENAME*/newFunction(a1, x) + 100; - } - } -} -function newFunction(a1: number, x: number) { - return 1 + a1 + x; -}