From 3b4cbebee094e01dac4f8036df0f663dac6e3e17 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Fri, 29 Sep 2017 18:13:38 -0700 Subject: [PATCH] Handle repeated substitution of the same symbol during extraction Old: Insert the same Node for every occurrence. The position was repeatedly mutated during printing. New: Thunk the Node so that a new one is constructed for every occurrence. Fixes #18857 --- src/harness/unittests/extractConstants.ts | 11 ++++----- src/harness/unittests/extractFunctions.ts | 11 ++++----- src/services/refactors/extractSymbol.ts | 24 ++++++++++--------- .../extractConstant_RepeatedSubstitution.ts | 19 +++++++++++++++ .../extractFunction_RepeatedSubstitution.ts | 22 +++++++++++++++++ 5 files changed, 64 insertions(+), 23 deletions(-) create mode 100644 tests/baselines/reference/extractConstant/extractConstant_RepeatedSubstitution.ts create mode 100644 tests/baselines/reference/extractFunction/extractFunction_RepeatedSubstitution.ts diff --git a/src/harness/unittests/extractConstants.ts b/src/harness/unittests/extractConstants.ts index 179affd86c..28109a6fa4 100644 --- a/src/harness/unittests/extractConstants.ts +++ b/src/harness/unittests/extractConstants.ts @@ -84,12 +84,11 @@ namespace ts { let x = [#|t + 1|]; }`); -// TODO (18857): handle repeated substitution -// testExtractConstant("extractConstant_RepeatedSubstitution", -// `namespace X { -// export const j = 10; -// export const y = [#|j * j|]; -// }`); + testExtractConstant("extractConstant_RepeatedSubstitution", + `namespace X { + export const j = 10; + export const y = [#|j * j|]; +}`); testExtractConstant("extractConstant_VariableList_const", `const a = 1, b = [#|a + 1|];`); diff --git a/src/harness/unittests/extractFunctions.ts b/src/harness/unittests/extractFunctions.ts index 291324e08f..750e590ca4 100644 --- a/src/harness/unittests/extractFunctions.ts +++ b/src/harness/unittests/extractFunctions.ts @@ -355,12 +355,11 @@ function parsePrimaryExpression(): any { [#|function G() { }|] }`); -// TODO (18857): handle repeated substitution -// testExtractFunction("extractFunction_RepeatedSubstitution", -// `namespace X { -// export const j = 10; -// export const y = [#|j * j|]; -// }`); + testExtractFunction("extractFunction_RepeatedSubstitution", + `namespace X { + export const j = 10; + export const y = [#|j * j|]; +}`); }); function testExtractFunction(caption: string, text: string) { diff --git a/src/services/refactors/extractSymbol.ts b/src/services/refactors/extractSymbol.ts index df9da034da..5ca6efbd57 100644 --- a/src/services/refactors/extractSymbol.ts +++ b/src/services/refactors/extractSymbol.ts @@ -1048,7 +1048,7 @@ namespace ts.refactor.extractSymbol { } } - function transformFunctionBody(body: Node, writes: ReadonlyArray, substitutions: ReadonlyMap, hasReturn: boolean): { body: Block, returnValueProperty: string } { + function transformFunctionBody(body: Node, writes: ReadonlyArray, substitutions: ReadonlyMap<() => Node>, hasReturn: boolean): { body: Block, returnValueProperty: string } { if (isBlock(body) && !writes && substitutions.size === 0) { // already block, no writes to propagate back, no substitutions - can use node as is return { body: createBlock(body.statements, /*multLine*/ true), returnValueProperty: undefined }; @@ -1096,21 +1096,21 @@ namespace ts.refactor.extractSymbol { const oldIgnoreReturns = ignoreReturns; ignoreReturns = ignoreReturns || isFunctionLikeDeclaration(node) || isClassLike(node); const substitution = substitutions.get(getNodeId(node).toString()); - const result = substitution || visitEachChild(node, visitor, nullTransformationContext); + const result = substitution ? substitution() : visitEachChild(node, visitor, nullTransformationContext); ignoreReturns = oldIgnoreReturns; return result; } } } - function transformConstantInitializer(initializer: Expression, substitutions: ReadonlyMap): Expression { + function transformConstantInitializer(initializer: Expression, substitutions: ReadonlyMap<() => Node>): Expression { return substitutions.size ? visitor(initializer) as Expression : initializer; function visitor(node: Node): VisitResult { const substitution = substitutions.get(getNodeId(node).toString()); - return substitution || visitEachChild(node, visitor, nullTransformationContext); + return substitution ? substitution() : visitEachChild(node, visitor, nullTransformationContext); } } @@ -1239,7 +1239,7 @@ namespace ts.refactor.extractSymbol { interface ScopeUsages { readonly usages: Map; readonly typeParameterUsages: Map; // Key is type ID - readonly substitutions: Map; + readonly substitutions: Map<() => Node>; } interface ReadsAndWrites { @@ -1258,7 +1258,7 @@ namespace ts.refactor.extractSymbol { const allTypeParameterUsages = createMap(); // Key is type ID const usagesPerScope: ScopeUsages[] = []; - const substitutionsPerScope: Map[] = []; + const substitutionsPerScope: Map<() => Node>[] = []; const functionErrorsPerScope: Diagnostic[][] = []; const constantErrorsPerScope: Diagnostic[][] = []; const visibleDeclarationsInExtractedRange: Symbol[] = []; @@ -1270,8 +1270,8 @@ namespace ts.refactor.extractSymbol { // initialize results for (const scope of scopes) { - usagesPerScope.push({ usages: createMap(), typeParameterUsages: createMap(), substitutions: createMap() }); - substitutionsPerScope.push(createMap()); + usagesPerScope.push({ usages: createMap(), typeParameterUsages: createMap(), substitutions: createMap<() => Expression>() }); + substitutionsPerScope.push(createMap<() => Expression>()); functionErrorsPerScope.push( isFunctionLikeDeclaration(scope) && scope.kind !== SyntaxKind.FunctionDeclaration @@ -1567,18 +1567,20 @@ namespace ts.refactor.extractSymbol { } } - function tryReplaceWithQualifiedNameOrPropertyAccess(symbol: Symbol, scopeDecl: Node, isTypeNode: boolean): PropertyAccessExpression | EntityName { + function tryReplaceWithQualifiedNameOrPropertyAccess(symbol: Symbol, scopeDecl: Node, isTypeNode: boolean): () => (PropertyAccessExpression | EntityName) { if (!symbol) { return undefined; } if (symbol.getDeclarations().some(d => d.parent === scopeDecl)) { - return createIdentifier(symbol.name); + return () => createIdentifier(symbol.name); } const prefix = tryReplaceWithQualifiedNameOrPropertyAccess(symbol.parent, scopeDecl, isTypeNode); if (prefix === undefined) { return undefined; } - return isTypeNode ? createQualifiedName(prefix, createIdentifier(symbol.name)) : createPropertyAccess(prefix, symbol.name); + return isTypeNode + ? () => createQualifiedName(prefix(), createIdentifier(symbol.name)) + : () => createPropertyAccess(prefix(), symbol.name); } } diff --git a/tests/baselines/reference/extractConstant/extractConstant_RepeatedSubstitution.ts b/tests/baselines/reference/extractConstant/extractConstant_RepeatedSubstitution.ts new file mode 100644 index 0000000000..da92e8be93 --- /dev/null +++ b/tests/baselines/reference/extractConstant/extractConstant_RepeatedSubstitution.ts @@ -0,0 +1,19 @@ +// ==ORIGINAL== +namespace X { + export const j = 10; + export const y = j * j; +} +// ==SCOPE::Extract to constant in enclosing scope== +namespace X { + export const j = 10; + const newLocal = j * j; + + export const y = /*RENAME*/newLocal; +} +// ==SCOPE::Extract to constant in global scope== +const newLocal = X.j * X.j; + +namespace X { + export const j = 10; + export const y = /*RENAME*/newLocal; +} \ No newline at end of file diff --git a/tests/baselines/reference/extractFunction/extractFunction_RepeatedSubstitution.ts b/tests/baselines/reference/extractFunction/extractFunction_RepeatedSubstitution.ts new file mode 100644 index 0000000000..080f694360 --- /dev/null +++ b/tests/baselines/reference/extractFunction/extractFunction_RepeatedSubstitution.ts @@ -0,0 +1,22 @@ +// ==ORIGINAL== +namespace X { + export const j = 10; + export const y = j * j; +} +// ==SCOPE::Extract to function in namespace 'X'== +namespace X { + export const j = 10; + export const y = /*RENAME*/newFunction(); + + function newFunction() { + return j * j; + } +} +// ==SCOPE::Extract to function in global scope== +namespace X { + export const j = 10; + export const y = /*RENAME*/newFunction(); +} +function newFunction() { + return X.j * X.j; +}