From 1b6e991924e2ad226129b7a95d22414ec415513d Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Thu, 12 Oct 2017 17:55:33 -0700 Subject: [PATCH 1/2] Demote some extraction ranges to produce better results Attempt to extract return expressions, rather than return statements, and initializers, rather than variable declarations. --- src/harness/unittests/extractFunctions.ts | 18 ++++++---- src/harness/unittests/extractRanges.ts | 25 +++++++++++++ src/services/refactors/extractSymbol.ts | 35 +++++++++++++++++-- .../extractFunction/extractFunction29.ts | 4 +-- .../extractFunction/extractFunction32.ts | 8 ++--- ...nction_VariableDeclaration_Const_NoType.js | 4 ++- ...nction_VariableDeclaration_Const_NoType.ts | 4 ++- ...Function_VariableDeclaration_Const_Type.ts | 4 ++- ...ction_VariableDeclaration_ConsumedTwice.ts | 4 ++- ...Function_VariableDeclaration_Let_NoType.js | 4 ++- ...Function_VariableDeclaration_Let_NoType.ts | 4 ++- ...ctFunction_VariableDeclaration_Let_Type.ts | 4 ++- ...extractFunction_VariableDeclaration_Var.js | 4 ++- ...extractFunction_VariableDeclaration_Var.ts | 4 ++- tests/cases/fourslash/extract-method14.ts | 8 ++--- tests/cases/fourslash/extract-method22.ts | 2 +- 16 files changed, 107 insertions(+), 29 deletions(-) diff --git a/src/harness/unittests/extractFunctions.ts b/src/harness/unittests/extractFunctions.ts index 32c7139587d..56b3c35a292 100644 --- a/src/harness/unittests/extractFunctions.ts +++ b/src/harness/unittests/extractFunctions.ts @@ -362,27 +362,32 @@ function parsePrimaryExpression(): any { }`); testExtractFunction("extractFunction_VariableDeclaration_Var", ` -[#|var x = 1;|] +[#|var x = 1; +"hello"|] x; `); testExtractFunction("extractFunction_VariableDeclaration_Let_Type", ` -[#|let x: number = 1;|] +[#|let x: number = 1; +"hello";|] x; `); testExtractFunction("extractFunction_VariableDeclaration_Let_NoType", ` -[#|let x = 1;|] +[#|let x = 1; +"hello";|] x; `); testExtractFunction("extractFunction_VariableDeclaration_Const_Type", ` -[#|const x: number = 1;|] +[#|const x: number = 1; +"hello";|] x; `); testExtractFunction("extractFunction_VariableDeclaration_Const_NoType", ` -[#|const x = 1;|] +[#|const x = 1; +"hello";|] x; `); @@ -404,7 +409,8 @@ x; y; z; `); testExtractFunction("extractFunction_VariableDeclaration_ConsumedTwice", ` -[#|const x: number = 1;|] +[#|const x: number = 1; +"hello";|] x; x; `); diff --git a/src/harness/unittests/extractRanges.ts b/src/harness/unittests/extractRanges.ts index a467bb23e0f..e8e9a918f0d 100644 --- a/src/harness/unittests/extractRanges.ts +++ b/src/harness/unittests/extractRanges.ts @@ -162,6 +162,19 @@ namespace ts { }|]|] } `); + + // Variable statements + testExtractRange(`[#|let x = [$|1|];|]`); + testExtractRange(`[#|let x = [$|1|], y;|]`); + testExtractRange(`[#|[$|let x = 1, y = 1;|]|]`); + + // Variable declarations + testExtractRange(`let [#|x = [$|1|]|];`); + testExtractRange(`let [#|x = [$|1|]|], y = 2;`); + testExtractRange(`let x = 1, [#|y = [$|2|]|];`); + + // Return statements + testExtractRange(`[#|return [$|1|];|]`); }); testExtractRangeFailed("extractRangeFailed1", @@ -340,6 +353,18 @@ switch (x) { refactor.extractSymbol.Messages.CannotExtractRangeContainingConditionalBreakOrContinueStatements.message ]); + testExtractRangeFailed("extractRangeFailed12", + `let [#|x|];`, + [ + refactor.extractSymbol.Messages.StatementOrExpressionExpected.message + ]); + + testExtractRangeFailed("extractRangeFailed13", + `[#|return;|]`, + [ + refactor.extractSymbol.Messages.CannotExtractRange.message + ]); + testExtractRangeFailed("extract-method-not-for-token-expression-statement", `[#|a|]`, [refactor.extractSymbol.Messages.CannotExtractIdentifier.message]); }); } \ No newline at end of file diff --git a/src/services/refactors/extractSymbol.ts b/src/services/refactors/extractSymbol.ts index 84f6ffb0815..c76cdc074aa 100644 --- a/src/services/refactors/extractSymbol.ts +++ b/src/services/refactors/extractSymbol.ts @@ -245,11 +245,42 @@ namespace ts.refactor.extractSymbol { } // We have a single node (start) - const errors = checkRootNode(start) || checkNode(start); + let node = start; + + if (isReturnStatement(start)) { + if (!start.expression) { + // Makes no sense to extract an expression-less return statement. + return { errors: [createFileDiagnostic(sourceFile, span.start, length, Messages.CannotExtractRange)] }; + } + + node = start.expression; + } + else if (isVariableStatement(start)) { + let numInitializers = 0; + let lastInitializer: Expression | undefined = undefined; + for (const declaration of start.declarationList.declarations) { + if (declaration.initializer) { + numInitializers++; + lastInitializer = declaration.initializer; + } + } + + if (numInitializers === 1) { + node = lastInitializer; + } + // No special handling if there are multiple initializers. + } + else if (isVariableDeclaration(node)) { + if (node.initializer) { + node = node.initializer; + } + } + + const errors = checkRootNode(node) || checkNode(node); if (errors) { return { errors }; } - return { targetRange: { range: getStatementOrExpressionRange(start), facts: rangeFacts, declarations } }; + return { targetRange: { range: getStatementOrExpressionRange(node), facts: rangeFacts, declarations } }; function checkRootNode(node: Node): Diagnostic[] | undefined { if (isIdentifier(isExpressionStatement(node) ? node.expression : node)) { diff --git a/tests/baselines/reference/extractFunction/extractFunction29.ts b/tests/baselines/reference/extractFunction/extractFunction29.ts index 492db8e6df9..339d218c714 100644 --- a/tests/baselines/reference/extractFunction/extractFunction29.ts +++ b/tests/baselines/reference/extractFunction/extractFunction29.ts @@ -26,7 +26,7 @@ interface UnaryExpression { function parseUnaryExpression(operator: string): UnaryExpression { return /*RENAME*/newFunction(); - function newFunction() { + function newFunction(): UnaryExpression { return { kind: "Unary", operator, @@ -49,7 +49,7 @@ function parseUnaryExpression(operator: string): UnaryExpression { return /*RENAME*/newFunction(operator); } -function newFunction(operator: string) { +function newFunction(operator: string): UnaryExpression { return { kind: "Unary", operator, diff --git a/tests/baselines/reference/extractFunction/extractFunction32.ts b/tests/baselines/reference/extractFunction/extractFunction32.ts index 720d0b227b0..acd5ac50804 100644 --- a/tests/baselines/reference/extractFunction/extractFunction32.ts +++ b/tests/baselines/reference/extractFunction/extractFunction32.ts @@ -17,11 +17,11 @@ namespace N { export const value = 1; () => { - /*RENAME*/newFunction(); + var c = /*RENAME*/newFunction() } function newFunction() { - var c = class { + return class { M() { return value; } @@ -34,12 +34,12 @@ namespace N { export const value = 1; () => { - /*RENAME*/newFunction(); + var c = /*RENAME*/newFunction() } } function newFunction() { - var c = class { + return class { M() { return N.value; } diff --git a/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Const_NoType.js b/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Const_NoType.js index fff1488ef41..f4b8a452a6e 100644 --- a/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Const_NoType.js +++ b/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Const_NoType.js @@ -1,6 +1,7 @@ // ==ORIGINAL== -/*[#|*/const x = 1;/*|]*/ +/*[#|*/const x = 1; +"hello";/*|]*/ x; // ==SCOPE::Extract to function in global scope== @@ -10,5 +11,6 @@ x; function newFunction() { const x = 1; + "hello"; return x; } diff --git a/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Const_NoType.ts b/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Const_NoType.ts index fff1488ef41..f4b8a452a6e 100644 --- a/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Const_NoType.ts +++ b/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Const_NoType.ts @@ -1,6 +1,7 @@ // ==ORIGINAL== -/*[#|*/const x = 1;/*|]*/ +/*[#|*/const x = 1; +"hello";/*|]*/ x; // ==SCOPE::Extract to function in global scope== @@ -10,5 +11,6 @@ x; function newFunction() { const x = 1; + "hello"; return x; } diff --git a/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Const_Type.ts b/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Const_Type.ts index 94576953758..5b619e2e4f5 100644 --- a/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Const_Type.ts +++ b/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Const_Type.ts @@ -1,6 +1,7 @@ // ==ORIGINAL== -/*[#|*/const x: number = 1;/*|]*/ +/*[#|*/const x: number = 1; +"hello";/*|]*/ x; // ==SCOPE::Extract to function in global scope== @@ -10,5 +11,6 @@ x; function newFunction() { const x: number = 1; + "hello"; return x; } diff --git a/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_ConsumedTwice.ts b/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_ConsumedTwice.ts index ac57711614c..943fee55fa0 100644 --- a/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_ConsumedTwice.ts +++ b/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_ConsumedTwice.ts @@ -1,6 +1,7 @@ // ==ORIGINAL== -/*[#|*/const x: number = 1;/*|]*/ +/*[#|*/const x: number = 1; +"hello";/*|]*/ x; x; // ==SCOPE::Extract to function in global scope== @@ -10,5 +11,6 @@ x; x; function newFunction() { const x: number = 1; + "hello"; return x; } diff --git a/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Let_NoType.js b/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Let_NoType.js index 4f407ce5703..39002be4a73 100644 --- a/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Let_NoType.js +++ b/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Let_NoType.js @@ -1,6 +1,7 @@ // ==ORIGINAL== -/*[#|*/let x = 1;/*|]*/ +/*[#|*/let x = 1; +"hello";/*|]*/ x; // ==SCOPE::Extract to function in global scope== @@ -10,5 +11,6 @@ x; function newFunction() { let x = 1; + "hello"; return x; } diff --git a/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Let_NoType.ts b/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Let_NoType.ts index 4f407ce5703..39002be4a73 100644 --- a/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Let_NoType.ts +++ b/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Let_NoType.ts @@ -1,6 +1,7 @@ // ==ORIGINAL== -/*[#|*/let x = 1;/*|]*/ +/*[#|*/let x = 1; +"hello";/*|]*/ x; // ==SCOPE::Extract to function in global scope== @@ -10,5 +11,6 @@ x; function newFunction() { let x = 1; + "hello"; return x; } diff --git a/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Let_Type.ts b/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Let_Type.ts index 048b77094ea..75d8643941f 100644 --- a/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Let_Type.ts +++ b/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Let_Type.ts @@ -1,6 +1,7 @@ // ==ORIGINAL== -/*[#|*/let x: number = 1;/*|]*/ +/*[#|*/let x: number = 1; +"hello";/*|]*/ x; // ==SCOPE::Extract to function in global scope== @@ -10,5 +11,6 @@ x; function newFunction() { let x: number = 1; + "hello"; return x; } diff --git a/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Var.js b/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Var.js index 5a784c366c1..e4ee8ac186c 100644 --- a/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Var.js +++ b/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Var.js @@ -1,6 +1,7 @@ // ==ORIGINAL== -/*[#|*/var x = 1;/*|]*/ +/*[#|*/var x = 1; +"hello"/*|]*/ x; // ==SCOPE::Extract to function in global scope== @@ -10,5 +11,6 @@ x; function newFunction() { var x = 1; + "hello"; return x; } diff --git a/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Var.ts b/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Var.ts index 5a784c366c1..e4ee8ac186c 100644 --- a/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Var.ts +++ b/tests/baselines/reference/extractFunction/extractFunction_VariableDeclaration_Var.ts @@ -1,6 +1,7 @@ // ==ORIGINAL== -/*[#|*/var x = 1;/*|]*/ +/*[#|*/var x = 1; +"hello"/*|]*/ x; // ==SCOPE::Extract to function in global scope== @@ -10,5 +11,6 @@ x; function newFunction() { var x = 1; + "hello"; return x; } diff --git a/tests/cases/fourslash/extract-method14.ts b/tests/cases/fourslash/extract-method14.ts index 11ea1f1b3fd..bdb9727f3d1 100644 --- a/tests/cases/fourslash/extract-method14.ts +++ b/tests/cases/fourslash/extract-method14.ts @@ -7,7 +7,7 @@ // @Filename: foo.js //// function foo() { //// var i = 10; -//// /*a*/return i++;/*b*/ +//// /*a*/return i + 1;/*b*/ //// } goTo.select('a', 'b'); @@ -18,13 +18,11 @@ edit.applyRefactor({ newContent: `function foo() { var i = 10; - let __return; - ({ __return, i } = /*RENAME*/newFunction(i)); - return __return; + return /*RENAME*/newFunction(i); } function newFunction(i) { - return { __return: i++, i }; + return i + 1; } ` }); diff --git a/tests/cases/fourslash/extract-method22.ts b/tests/cases/fourslash/extract-method22.ts index 85f88f3952c..4539e5cd470 100644 --- a/tests/cases/fourslash/extract-method22.ts +++ b/tests/cases/fourslash/extract-method22.ts @@ -3,7 +3,7 @@ // You may not extract variable declarations with the export modifier //// namespace NS { -//// /*start*/export var x = 10;/*end*/ +//// /*start*/export var x = 10, y = 15;/*end*/ //// } goTo.select('start', 'end') From e4d5d1c16d88c68934b61a02abba9668b6d1adf8 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Tue, 24 Oct 2017 11:33:30 -0700 Subject: [PATCH 2/2] Extract refineNode into its own function --- src/services/refactors/extractSymbol.ts | 69 ++++++++++++++----------- 1 file changed, 39 insertions(+), 30 deletions(-) diff --git a/src/services/refactors/extractSymbol.ts b/src/services/refactors/extractSymbol.ts index c76cdc074aa..b18e12d2b96 100644 --- a/src/services/refactors/extractSymbol.ts +++ b/src/services/refactors/extractSymbol.ts @@ -244,37 +244,13 @@ namespace ts.refactor.extractSymbol { return { targetRange: { range: statements, facts: rangeFacts, declarations } }; } + if (isReturnStatement(start) && !start.expression) { + // Makes no sense to extract an expression-less return statement. + return { errors: [createFileDiagnostic(sourceFile, span.start, length, Messages.CannotExtractRange)] }; + } + // We have a single node (start) - let node = start; - - if (isReturnStatement(start)) { - if (!start.expression) { - // Makes no sense to extract an expression-less return statement. - return { errors: [createFileDiagnostic(sourceFile, span.start, length, Messages.CannotExtractRange)] }; - } - - node = start.expression; - } - else if (isVariableStatement(start)) { - let numInitializers = 0; - let lastInitializer: Expression | undefined = undefined; - for (const declaration of start.declarationList.declarations) { - if (declaration.initializer) { - numInitializers++; - lastInitializer = declaration.initializer; - } - } - - if (numInitializers === 1) { - node = lastInitializer; - } - // No special handling if there are multiple initializers. - } - else if (isVariableDeclaration(node)) { - if (node.initializer) { - node = node.initializer; - } - } + const node = refineNode(start); const errors = checkRootNode(node) || checkNode(node); if (errors) { @@ -282,6 +258,39 @@ namespace ts.refactor.extractSymbol { } return { targetRange: { range: getStatementOrExpressionRange(node), facts: rangeFacts, declarations } }; + /** + * Attempt to refine the extraction node (generally, by shrinking it) to produce better results. + * @param node The unrefined extraction node. + */ + function refineNode(node: Node) { + if (isReturnStatement(node)) { + if (node.expression) { + return node.expression; + } + } + else if (isVariableStatement(node)) { + let numInitializers = 0; + let lastInitializer: Expression | undefined = undefined; + for (const declaration of node.declarationList.declarations) { + if (declaration.initializer) { + numInitializers++; + lastInitializer = declaration.initializer; + } + } + if (numInitializers === 1) { + return lastInitializer; + } + // No special handling if there are multiple initializers. + } + else if (isVariableDeclaration(node)) { + if (node.initializer) { + return node.initializer; + } + } + + return node; + } + function checkRootNode(node: Node): Diagnostic[] | undefined { if (isIdentifier(isExpressionStatement(node) ? node.expression : node)) { return [createDiagnosticForNode(node, Messages.CannotExtractIdentifier)];