From 2a4ab08655f78863b3123b6e46ab7de9dc843ba3 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Tue, 3 Oct 2017 11:18:40 -0700 Subject: [PATCH 1/2] Refine extraction of expression statements 1) Replace range, rather than node, to leave trivia intact. 2) Only replace node in the innermost scope - otherwise insert as usual and delete the original statement. --- src/harness/unittests/extractConstants.ts | 7 ++++++ src/services/refactors/extractSymbol.ts | 17 ++++++++++---- ...nstant_ExpressionStatementInNestedScope.js | 23 +++++++++++++++++++ ...nstant_ExpressionStatementInNestedScope.ts | 23 +++++++++++++++++++ 4 files changed, 65 insertions(+), 5 deletions(-) create mode 100644 tests/baselines/reference/extractConstant/extractConstant_ExpressionStatementInNestedScope.js create mode 100644 tests/baselines/reference/extractConstant/extractConstant_ExpressionStatementInNestedScope.ts diff --git a/src/harness/unittests/extractConstants.ts b/src/harness/unittests/extractConstants.ts index 28109a6fa4d..b58016cd56f 100644 --- a/src/harness/unittests/extractConstants.ts +++ b/src/harness/unittests/extractConstants.ts @@ -33,6 +33,13 @@ namespace ts { testExtractConstant("extractConstant_ExpressionStatementExpression", `[#|"hello"|];`); + testExtractConstant("extractConstant_ExpressionStatementInNestedScope", ` +let i = 0; +function F() { + [#|i++|]; +} + `); + testExtractConstant("extractConstant_BlockScopes_NoDependencies", `for (let i = 0; i < 10; i++) { for (let j = 0; j < 10; j++) { diff --git a/src/services/refactors/extractSymbol.ts b/src/services/refactors/extractSymbol.ts index 694162bc98a..a7646f6234e 100644 --- a/src/services/refactors/extractSymbol.ts +++ b/src/services/refactors/extractSymbol.ts @@ -911,12 +911,13 @@ namespace ts.refactor.extractSymbol { const localReference = createIdentifier(localNameText); changeTracker.replaceRange(context.file, { pos: node.getStart(), end: node.end }, localReference); } - else if (node.parent.kind === SyntaxKind.ExpressionStatement) { - // If the parent is an expression statement, replace the statement with the declaration. + else if (node.parent.kind === SyntaxKind.ExpressionStatement && scope === findAncestor(node, isScope)) { + // If the parent is an expression statement and the target scope is the immediately enclosing one, + // replace the statement with the declaration. const newVariableStatement = createVariableStatement( /*modifiers*/ undefined, createVariableDeclarationList([newVariableDeclaration], NodeFlags.Const)); - changeTracker.replaceNodeWithNodes(context.file, node.parent, [newVariableStatement], { nodeSeparator: context.newLineCharacter }); + changeTracker.replaceRange(context.file, { pos: node.parent.getStart(), end: node.parent.end }, newVariableStatement); } else { const newVariableStatement = createVariableStatement( @@ -940,8 +941,14 @@ namespace ts.refactor.extractSymbol { } // Consume - const localReference = createIdentifier(localNameText); - changeTracker.replaceRange(context.file, { pos: node.getStart(), end: node.end }, localReference); + if (node.parent.kind === SyntaxKind.ExpressionStatement) { + // If the parent is an expression statement, delete it. + changeTracker.deleteRange(context.file, { pos: node.parent.getStart(), end: node.parent.end }); + } + else { + const localReference = createIdentifier(localNameText); + changeTracker.replaceRange(context.file, { pos: node.getStart(), end: node.end }, localReference); + } } } diff --git a/tests/baselines/reference/extractConstant/extractConstant_ExpressionStatementInNestedScope.js b/tests/baselines/reference/extractConstant/extractConstant_ExpressionStatementInNestedScope.js new file mode 100644 index 00000000000..f76af9e9390 --- /dev/null +++ b/tests/baselines/reference/extractConstant/extractConstant_ExpressionStatementInNestedScope.js @@ -0,0 +1,23 @@ +// ==ORIGINAL== + +let i = 0; +function F() { + i++; +} + +// ==SCOPE::Extract to constant in enclosing scope== + +let i = 0; +function F() { + const /*RENAME*/newLocal = i++; +} + +// ==SCOPE::Extract to constant in global scope== + +let i = 0; +const /*RENAME*/newLocal = i++; + +function F() { + +} + \ No newline at end of file diff --git a/tests/baselines/reference/extractConstant/extractConstant_ExpressionStatementInNestedScope.ts b/tests/baselines/reference/extractConstant/extractConstant_ExpressionStatementInNestedScope.ts new file mode 100644 index 00000000000..f76af9e9390 --- /dev/null +++ b/tests/baselines/reference/extractConstant/extractConstant_ExpressionStatementInNestedScope.ts @@ -0,0 +1,23 @@ +// ==ORIGINAL== + +let i = 0; +function F() { + i++; +} + +// ==SCOPE::Extract to constant in enclosing scope== + +let i = 0; +function F() { + const /*RENAME*/newLocal = i++; +} + +// ==SCOPE::Extract to constant in global scope== + +let i = 0; +const /*RENAME*/newLocal = i++; + +function F() { + +} + \ No newline at end of file From 7b1147fbcefbd67fb0a790e6ac54a7e93c94d8f8 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Tue, 3 Oct 2017 12:23:32 -0700 Subject: [PATCH 2/2] Stop inadvertently exempting expression statements from check --- src/harness/unittests/extractConstants.ts | 7 +++++++ src/services/refactors/extractSymbol.ts | 15 +++++++-------- ...ctConstant_ExpressionStatementConsumesLocal.js | 14 ++++++++++++++ ...ctConstant_ExpressionStatementConsumesLocal.ts | 14 ++++++++++++++ 4 files changed, 42 insertions(+), 8 deletions(-) create mode 100644 tests/baselines/reference/extractConstant/extractConstant_ExpressionStatementConsumesLocal.js create mode 100644 tests/baselines/reference/extractConstant/extractConstant_ExpressionStatementConsumesLocal.ts diff --git a/src/harness/unittests/extractConstants.ts b/src/harness/unittests/extractConstants.ts index b58016cd56f..63a150856f5 100644 --- a/src/harness/unittests/extractConstants.ts +++ b/src/harness/unittests/extractConstants.ts @@ -40,6 +40,13 @@ function F() { } `); + testExtractConstant("extractConstant_ExpressionStatementConsumesLocal", ` +function F() { + let i = 0; + [#|i++|]; +} + `); + testExtractConstant("extractConstant_BlockScopes_NoDependencies", `for (let i = 0; i < 10; i++) { for (let j = 0; j < 10; j++) { diff --git a/src/services/refactors/extractSymbol.ts b/src/services/refactors/extractSymbol.ts index a7646f6234e..9e2ca3da4f7 100644 --- a/src/services/refactors/extractSymbol.ts +++ b/src/services/refactors/extractSymbol.ts @@ -1351,14 +1351,13 @@ namespace ts.refactor.extractSymbol { } for (let i = 0; i < scopes.length; i++) { - if (!isReadonlyArray(targetRange.range)) { - const scopeUsages = usagesPerScope[i]; - // Special case: in the innermost scope, all usages are available. - // (The computed value reflects the value at the top-level of the scope, but the - // local will actually be declared at the same level as the extracted expression). - if (i > 0 && (scopeUsages.usages.size > 0 || scopeUsages.typeParameterUsages.size > 0)) { - constantErrorsPerScope[i].push(createDiagnosticForNode(targetRange.range, Messages.CannotAccessVariablesFromNestedScopes)); - } + const scopeUsages = usagesPerScope[i]; + // Special case: in the innermost scope, all usages are available. + // (The computed value reflects the value at the top-level of the scope, but the + // local will actually be declared at the same level as the extracted expression). + if (i > 0 && (scopeUsages.usages.size > 0 || scopeUsages.typeParameterUsages.size > 0)) { + const errorNode = isReadonlyArray(targetRange.range) ? targetRange.range[0] : targetRange.range; + constantErrorsPerScope[i].push(createDiagnosticForNode(errorNode, Messages.CannotAccessVariablesFromNestedScopes)); } let hasWrite = false; diff --git a/tests/baselines/reference/extractConstant/extractConstant_ExpressionStatementConsumesLocal.js b/tests/baselines/reference/extractConstant/extractConstant_ExpressionStatementConsumesLocal.js new file mode 100644 index 00000000000..b57da0d8e8c --- /dev/null +++ b/tests/baselines/reference/extractConstant/extractConstant_ExpressionStatementConsumesLocal.js @@ -0,0 +1,14 @@ +// ==ORIGINAL== + +function F() { + let i = 0; + i++; +} + +// ==SCOPE::Extract to constant in enclosing scope== + +function F() { + let i = 0; + const /*RENAME*/newLocal = i++; +} + \ No newline at end of file diff --git a/tests/baselines/reference/extractConstant/extractConstant_ExpressionStatementConsumesLocal.ts b/tests/baselines/reference/extractConstant/extractConstant_ExpressionStatementConsumesLocal.ts new file mode 100644 index 00000000000..b57da0d8e8c --- /dev/null +++ b/tests/baselines/reference/extractConstant/extractConstant_ExpressionStatementConsumesLocal.ts @@ -0,0 +1,14 @@ +// ==ORIGINAL== + +function F() { + let i = 0; + i++; +} + +// ==SCOPE::Extract to constant in enclosing scope== + +function F() { + let i = 0; + const /*RENAME*/newLocal = i++; +} + \ No newline at end of file