From a5e184118088342db9db79c49f6cb95e53f63b72 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Thu, 5 Oct 2017 15:37:47 -0700 Subject: [PATCH 1/7] Handle undefined in getSynthesizedClone --- src/compiler/factory.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/compiler/factory.ts b/src/compiler/factory.ts index 968144c0fc7..fe183c5e806 100644 --- a/src/compiler/factory.ts +++ b/src/compiler/factory.ts @@ -47,10 +47,15 @@ namespace ts { * Creates a shallow, memberwise clone of a node with no source map location. */ /* @internal */ - export function getSynthesizedClone(node: T | undefined): T { + export function getSynthesizedClone(node: T | undefined): T | undefined { // We don't use "clone" from core.ts here, as we need to preserve the prototype chain of // the original node. We also need to exclude specific properties and only include own- // properties (to skip members already defined on the shared prototype). + + if (node === undefined) { + return undefined; + } + const clone = createSynthesizedNode(node.kind); clone.flags |= node.flags; setOriginalNode(clone, node); From 380b8df13f3ff9deaf36c6a7aa3ae0c7fd4ce960 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Thu, 5 Oct 2017 15:38:02 -0700 Subject: [PATCH 2/7] Introduce getSynthesizedDeepClone --- src/compiler/factory.ts | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/compiler/factory.ts b/src/compiler/factory.ts index fe183c5e806..84c28abe1a5 100644 --- a/src/compiler/factory.ts +++ b/src/compiler/factory.ts @@ -71,6 +71,15 @@ namespace ts { return clone; } + /** + * Creates a deep, memberwise clone of a node with no source map location. + */ + export function getSynthesizedDeepClone(node: T | undefined): T | undefined { + return node + ? getSynthesizedClone(visitEachChild(node, child => getSynthesizedDeepClone(child), nullTransformationContext)) + : undefined; + } + // Literals export function createLiteral(value: string): StringLiteral; From ad148dbc8800da4bbd2863a68f6f6617ee776a7a Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Thu, 5 Oct 2017 15:46:14 -0700 Subject: [PATCH 3/7] Use deep cloning, rather than thunking for repeated substitution Replaces b244cd4fb47 --- src/services/refactors/extractSymbol.ts | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/services/refactors/extractSymbol.ts b/src/services/refactors/extractSymbol.ts index 0e2f5ebfe3c..0176841e346 100644 --- a/src/services/refactors/extractSymbol.ts +++ b/src/services/refactors/extractSymbol.ts @@ -1064,7 +1064,7 @@ namespace ts.refactor.extractSymbol { } } - function transformFunctionBody(body: Node, writes: ReadonlyArray, substitutions: ReadonlyMap<() => Node>, hasReturn: boolean): { body: Block, returnValueProperty: string } { + function transformFunctionBody(body: Node, writes: ReadonlyArray, substitutions: ReadonlyMap, 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 }; @@ -1112,21 +1112,21 @@ namespace ts.refactor.extractSymbol { const oldIgnoreReturns = ignoreReturns; ignoreReturns = ignoreReturns || isFunctionLikeDeclaration(node) || isClassLike(node); const substitution = substitutions.get(getNodeId(node).toString()); - const result = substitution ? substitution() : visitEachChild(node, visitor, nullTransformationContext); + const result = substitution ? getSynthesizedDeepClone(substitution) : visitEachChild(node, visitor, nullTransformationContext); ignoreReturns = oldIgnoreReturns; return result; } } } - function transformConstantInitializer(initializer: Expression, substitutions: ReadonlyMap<() => Node>): Expression { + function transformConstantInitializer(initializer: Expression, substitutions: ReadonlyMap): Expression { return substitutions.size ? visitor(initializer) as Expression : initializer; function visitor(node: Node): VisitResult { const substitution = substitutions.get(getNodeId(node).toString()); - return substitution ? substitution() : visitEachChild(node, visitor, nullTransformationContext); + return substitution ? getSynthesizedDeepClone(substitution) : visitEachChild(node, visitor, nullTransformationContext); } } @@ -1255,7 +1255,7 @@ namespace ts.refactor.extractSymbol { interface ScopeUsages { readonly usages: Map; readonly typeParameterUsages: Map; // Key is type ID - readonly substitutions: Map<() => Node>; + readonly substitutions: Map; } interface ReadsAndWrites { @@ -1274,7 +1274,7 @@ namespace ts.refactor.extractSymbol { const allTypeParameterUsages = createMap(); // Key is type ID const usagesPerScope: ScopeUsages[] = []; - const substitutionsPerScope: Map<() => Node>[] = []; + const substitutionsPerScope: Map[] = []; const functionErrorsPerScope: Diagnostic[][] = []; const constantErrorsPerScope: Diagnostic[][] = []; const visibleDeclarationsInExtractedRange: Symbol[] = []; @@ -1298,8 +1298,8 @@ namespace ts.refactor.extractSymbol { // initialize results for (const scope of scopes) { - usagesPerScope.push({ usages: createMap(), typeParameterUsages: createMap(), substitutions: createMap<() => Expression>() }); - substitutionsPerScope.push(createMap<() => Expression>()); + usagesPerScope.push({ usages: createMap(), typeParameterUsages: createMap(), substitutions: createMap() }); + substitutionsPerScope.push(createMap()); functionErrorsPerScope.push( isFunctionLikeDeclaration(scope) && scope.kind !== SyntaxKind.FunctionDeclaration @@ -1598,20 +1598,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); + ? createQualifiedName(prefix, createIdentifier(symbol.name)) + : createPropertyAccess(prefix, symbol.name); } } From 5c9f8c56d95eee6bc897c8dfb64c398bf0e273fd Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Fri, 6 Oct 2017 10:20:12 -0700 Subject: [PATCH 4/7] Mark getSynthesizedDeepClone @internal --- src/compiler/factory.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/compiler/factory.ts b/src/compiler/factory.ts index 84c28abe1a5..7d703ffe062 100644 --- a/src/compiler/factory.ts +++ b/src/compiler/factory.ts @@ -74,6 +74,7 @@ namespace ts { /** * Creates a deep, memberwise clone of a node with no source map location. */ + /* @internal */ export function getSynthesizedDeepClone(node: T | undefined): T | undefined { return node ? getSynthesizedClone(visitEachChild(node, child => getSynthesizedDeepClone(child), nullTransformationContext)) From 9ece0cc956215ca1d82bf3f531a98ca44cdf5d66 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Tue, 10 Oct 2017 13:01:06 -0700 Subject: [PATCH 5/7] Move getSynthesizedDeepClone to services/utilities.ts --- src/compiler/factory.ts | 10 ---------- src/services/utilities.ts | 12 ++++++++++++ 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/src/compiler/factory.ts b/src/compiler/factory.ts index 7d703ffe062..fe183c5e806 100644 --- a/src/compiler/factory.ts +++ b/src/compiler/factory.ts @@ -71,16 +71,6 @@ namespace ts { return clone; } - /** - * Creates a deep, memberwise clone of a node with no source map location. - */ - /* @internal */ - export function getSynthesizedDeepClone(node: T | undefined): T | undefined { - return node - ? getSynthesizedClone(visitEachChild(node, child => getSynthesizedDeepClone(child), nullTransformationContext)) - : undefined; - } - // Literals export function createLiteral(value: string): StringLiteral; diff --git a/src/services/utilities.ts b/src/services/utilities.ts index 6166ceea28c..c22b981b4ff 100644 --- a/src/services/utilities.ts +++ b/src/services/utilities.ts @@ -1334,4 +1334,16 @@ namespace ts { } return position; } + + /** + * Creates a deep, memberwise clone of a node with no source map location. + * + * WARNING: This is an expensive operation and is only intended to be used in refactorings + * and code fixes (because those are triggered by explicit user actions). + */ + export function getSynthesizedDeepClone(node: T | undefined): T | undefined { + return node + ? getSynthesizedClone(visitEachChild(node, child => getSynthesizedDeepClone(child), nullTransformationContext)) + : undefined; + } } From 18afd8a50d9d3430062b126c7404501c190376af Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Tue, 10 Oct 2017 13:08:57 -0700 Subject: [PATCH 6/7] Optimize getSynthesizedDeepClone --- src/services/utilities.ts | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/src/services/utilities.ts b/src/services/utilities.ts index c22b981b4ff..5affb8a8887 100644 --- a/src/services/utilities.ts +++ b/src/services/utilities.ts @@ -1342,8 +1342,24 @@ namespace ts { * and code fixes (because those are triggered by explicit user actions). */ export function getSynthesizedDeepClone(node: T | undefined): T | undefined { - return node - ? getSynthesizedClone(visitEachChild(node, child => getSynthesizedDeepClone(child), nullTransformationContext)) - : undefined; + if (node === undefined) { + return undefined; + } + + const visited = visitEachChild(node, getSynthesizedDeepClone, nullTransformationContext); + if (visited === node) { + // This only happens for leaf nodes - internal nodes always see their children change. + return getSynthesizedClone(node); + } + + // PERF: As an optimization, rather than calling getSynthesizedClone, we'll update + // the new node created by visitEachChild with the extra changes getSynthesizedClone + // would have made. + + visited.pos = -1; + visited.end = -1; + visited.parent = undefined; + + return visited; } } From eb4f067ecbdd3b43b9df8ea9c4826b766f38f8b7 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Wed, 11 Oct 2017 13:53:52 -0700 Subject: [PATCH 7/7] Don't clobber the position of cloned nodes --- src/services/utilities.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/services/utilities.ts b/src/services/utilities.ts index 5affb8a8887..a8dea4ddd0e 100644 --- a/src/services/utilities.ts +++ b/src/services/utilities.ts @@ -1349,15 +1349,16 @@ namespace ts { const visited = visitEachChild(node, getSynthesizedDeepClone, nullTransformationContext); if (visited === node) { // This only happens for leaf nodes - internal nodes always see their children change. - return getSynthesizedClone(node); + const clone = getSynthesizedClone(node); + clone.pos = node.pos; + clone.end = node.end; + return clone; } // PERF: As an optimization, rather than calling getSynthesizedClone, we'll update // the new node created by visitEachChild with the extra changes getSynthesizedClone // would have made. - visited.pos = -1; - visited.end = -1; visited.parent = undefined; return visited;