From 30802cda977f1a3f0ba2a0b7d6ff5280ac3ebddc Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Fri, 11 Aug 2017 16:14:48 -0700 Subject: [PATCH] Handle loose type parameters in Extract Method Known limitations: 1. If a type parameter on an inner symbol shadows a type parameter on an outer symbol, the generated code will be incorrect. We should either rename one or more type parameters or forbid the extraction. 2. Type arguments are always passed explicitly, even if they would be inferred correctly. --- src/compiler/types.ts | 15 +++ src/compiler/utilities.ts | 32 ++++++ src/services/refactors/extractMethod.ts | 135 ++++++++++++++++++++++-- 3 files changed, 175 insertions(+), 7 deletions(-) diff --git a/src/compiler/types.ts b/src/compiler/types.ts index 87ac4afbab1..64444693acf 100644 --- a/src/compiler/types.ts +++ b/src/compiler/types.ts @@ -651,6 +651,20 @@ namespace ts { } export interface SignatureDeclaration extends NamedDeclaration { + kind: SyntaxKind.CallSignature + | SyntaxKind.ConstructSignature + | SyntaxKind.MethodSignature + | SyntaxKind.IndexSignature + | SyntaxKind.FunctionType + | SyntaxKind.ConstructorType + | SyntaxKind.JSDocFunctionType + | SyntaxKind.FunctionDeclaration + | SyntaxKind.MethodDeclaration + | SyntaxKind.Constructor + | SyntaxKind.GetAccessor + | SyntaxKind.SetAccessor + | SyntaxKind.FunctionExpression + | SyntaxKind.ArrowFunction; name?: PropertyName; typeParameters?: NodeArray; parameters: NodeArray; @@ -1816,6 +1830,7 @@ namespace ts { export type DeclarationWithTypeParameters = SignatureDeclaration | ClassLikeDeclaration | InterfaceDeclaration | TypeAliasDeclaration | JSDocTemplateTag; export interface ClassLikeDeclaration extends NamedDeclaration { + kind: SyntaxKind.ClassDeclaration | SyntaxKind.ClassExpression; name?: Identifier; typeParameters?: NodeArray; heritageClauses?: NodeArray; diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts index 197e91ac383..2a07d2b6560 100644 --- a/src/compiler/utilities.ts +++ b/src/compiler/utilities.ts @@ -477,6 +477,38 @@ namespace ts { return false; } + /* @internal */ + export function isDeclarationWithTypeParameters(node: Node): node is DeclarationWithTypeParameters; + export function isDeclarationWithTypeParameters(node: DeclarationWithTypeParameters): node is DeclarationWithTypeParameters { + switch (node.kind) { + case SyntaxKind.CallSignature: + case SyntaxKind.ConstructSignature: + case SyntaxKind.MethodSignature: + case SyntaxKind.IndexSignature: + case SyntaxKind.FunctionType: + case SyntaxKind.ConstructorType: + case SyntaxKind.JSDocFunctionType: + case SyntaxKind.ClassDeclaration: + case SyntaxKind.ClassExpression: + case SyntaxKind.InterfaceDeclaration: + case SyntaxKind.TypeAliasDeclaration: + case SyntaxKind.JSDocTemplateTag: + case SyntaxKind.FunctionDeclaration: + case SyntaxKind.MethodDeclaration: + case SyntaxKind.Constructor: + case SyntaxKind.GetAccessor: + case SyntaxKind.SetAccessor: + case SyntaxKind.FunctionExpression: + case SyntaxKind.ArrowFunction: + return true; + default: + staticAssertNever(node); + return false; + } + } + + function staticAssertNever(_: never): void {} + // Gets the nearest enclosing block scope container that has the provided node // as a descendant, that is not the provided node. export function getEnclosingBlockScopeContainer(node: Node): Node { diff --git a/src/services/refactors/extractMethod.ts b/src/services/refactors/extractMethod.ts index 4fa22d24b8d..ff376026e52 100644 --- a/src/services/refactors/extractMethod.ts +++ b/src/services/refactors/extractMethod.ts @@ -610,7 +610,7 @@ namespace ts.refactor.extractMethod { export function extractFunctionInScope( node: Statement | Expression | Block, scope: Scope, - { usages: usagesInScope, substitutions }: ScopeUsages, + { usages: usagesInScope, typeParameterUsages, substitutions }: ScopeUsages, range: TargetRange, context: RefactorContext): ExtractResultForScope { @@ -652,7 +652,18 @@ namespace ts.refactor.extractMethod { callArguments.push(createIdentifier(name)); }); - // Provide explicit return types for contexutally-typed functions + const typeParametersAndDeclarations = arrayFrom(typeParameterUsages.values()).map(type => ({ type, declaration: getFirstDeclaration(type) })); + const sortedTypeParametersAndDeclarations = typeParametersAndDeclarations.sort(compareTypesByDeclarationOrder); + + const typeParameters: ReadonlyArray = sortedTypeParametersAndDeclarations.map(t => t.declaration as TypeParameterDeclaration); + + // Strictly speaking, we should check whether each name actually binds to the appropriate type + // parameter. In cases of shadowing, they may not. + const callTypeArguments: ReadonlyArray | undefined = typeParameters.length > 0 + ? typeParameters.map(decl => createTypeReferenceNode(decl.name, /*typeArguments*/ undefined)) + : undefined; + + // Provide explicit return types for contextually-typed functions // to avoid problems when there are literal types present if (isExpression(node) && !isJS) { const contextualType = checker.getContextualType(node); @@ -677,7 +688,7 @@ namespace ts.refactor.extractMethod { range.facts & RangeFacts.IsGenerator ? createToken(SyntaxKind.AsteriskToken) : undefined, functionName, /*questionToken*/ undefined, - /*typeParameters*/[], + typeParameters, parameters, returnType, body @@ -689,7 +700,7 @@ namespace ts.refactor.extractMethod { range.facts & RangeFacts.IsAsyncFunction ? [createToken(SyntaxKind.AsyncKeyword)] : undefined, range.facts & RangeFacts.IsGenerator ? createToken(SyntaxKind.AsteriskToken) : undefined, functionName, - /*typeParameters*/[], + typeParameters, parameters, returnType, body @@ -704,7 +715,7 @@ namespace ts.refactor.extractMethod { // replace range with function call let call: Expression = createCall( isClassLike(scope) ? createPropertyAccess(range.facts & RangeFacts.InStaticRegion ? createIdentifier(scope.name.getText()) : createThis(), functionReference) : functionReference, - /*typeArguments*/ undefined, + callTypeArguments, // Note that no attempt is made to take advantage of type argument inference callArguments); if (range.facts & RangeFacts.IsGenerator) { call = createYield(createToken(SyntaxKind.AsteriskToken), call); @@ -774,6 +785,51 @@ namespace ts.refactor.extractMethod { changes: changeTracker.getChanges() }; + function getFirstDeclaration(type: Type): Declaration | undefined { + let firstDeclaration = undefined; + + const symbol = type.symbol; + if (symbol && symbol.declarations) { + for (const declaration of symbol.declarations) { + if (firstDeclaration === undefined || declaration.pos < firstDeclaration.pos) { + firstDeclaration = declaration; + } + } + } + + return firstDeclaration; + } + + function compareTypesByDeclarationOrder( + {type: type1, declaration: declaration1}: {type: Type, declaration?: Declaration}, + {type: type2, declaration: declaration2}: {type: Type, declaration?: Declaration}) { + + if (declaration1) { + if (declaration2) { + const positionDiff = declaration1.pos - declaration2.pos; + if (positionDiff !== 0) { + return positionDiff; + } + } + else { + return 1; // Sort undeclared type parameters to the front. + } + } + else if (declaration2) { + return -1; // Sort undeclared type parameters to the front. + } + + const name1 = type1.symbol ? type1.symbol.getName() : ""; + const name2 = type2.symbol ? type2.symbol.getName() : ""; + const nameDiff = compareStrings(name1, name2); + if (nameDiff !== 0) { + return nameDiff; + } + + // IDs are guaranteed to be unique, so this ensures a total ordering. + return type1.id - type2.id; + } + function getPropertyAssignmentsForWrites(writes: UsageEntry[]) { return writes.map(w => createShorthandPropertyAssignment(w.symbol.name)); } @@ -867,6 +923,7 @@ namespace ts.refactor.extractMethod { export interface ScopeUsages { usages: Map; + typeParameterUsages: Map; // Key is type ID substitutions: Map; } @@ -877,6 +934,7 @@ namespace ts.refactor.extractMethod { sourceFile: SourceFile, checker: TypeChecker) { + const allTypeParameterUsages = createMap(); // Key is type ID const usagesPerScope: ScopeUsages[] = []; const substitutionsPerScope: Map[] = []; const errorsPerScope: Diagnostic[][] = []; @@ -884,7 +942,7 @@ namespace ts.refactor.extractMethod { // initialize results for (const _ of scopes) { - usagesPerScope.push({ usages: createMap(), substitutions: createMap() }); + usagesPerScope.push({ usages: createMap(), typeParameterUsages: createMap(), substitutions: createMap() }); substitutionsPerScope.push(createMap()); errorsPerScope.push([]); } @@ -892,8 +950,42 @@ namespace ts.refactor.extractMethod { const target = isReadonlyArray(targetRange.range) ? createBlock(targetRange.range) : targetRange.range; const containingLexicalScopeOfExtraction = isBlockScope(scopes[0], scopes[0].parent) ? scopes[0] : getEnclosingBlockScopeContainer(scopes[0]); + const unmodifiedNode = isReadonlyArray(targetRange.range) ? targetRange.range[0] : targetRange.range; + const inGenericContext = isInGenericContext(unmodifiedNode); + collectUsages(target); + if (allTypeParameterUsages.size > 0) { + const seenTypeParameterUsages = createMap(); // Key is type ID + + let i = 0; + for (let curr: Node = unmodifiedNode; curr !== undefined && i < scopes.length; curr = curr.parent) { + if (curr === scopes[i]) { + // Copy current contents of seenTypeParameterUsages into scope. + seenTypeParameterUsages.forEach((typeParameter, id) => { + usagesPerScope[i].typeParameterUsages.set(id, typeParameter); + }); + + i++; + } + + // Note that we add the current node's type parameters *after* updating the corresponding scope. + if (isDeclarationWithTypeParameters(curr) && curr.typeParameters) { + for (const typeParameterDecl of curr.typeParameters) { + const typeParameter = checker.getTypeAtLocation(typeParameterDecl) as TypeParameter; + if (allTypeParameterUsages.has(typeParameter.id.toString())) { + seenTypeParameterUsages.set(typeParameter.id.toString(), typeParameter); + } + } + } + } + + // If we didn't get through all the scopes, then there were some that weren't in our + // parent chain (impossible at time of writing). A conservative solution would be to + // copy allTypeParameterUsages into all remaining scopes. + Debug.assert(i === scopes.length); + } + for (let i = 0; i < scopes.length; i++) { let hasWrite = false; let readonlyClassPropertyWrite: Declaration | undefined = undefined; @@ -912,7 +1004,7 @@ namespace ts.refactor.extractMethod { errorsPerScope[i].push(createDiagnosticForNode(targetRange.range, Messages.CannotCombineWritesAndReturns)); } else if (readonlyClassPropertyWrite && i > 0) { - errorsPerScope[i].push(createDiagnosticForNode(readonlyClassPropertyWrite, Messages.CannotCombineWritesAndReturns)); + errorsPerScope[i].push(createDiagnosticForNode(readonlyClassPropertyWrite, Messages.CannotExtractReadonlyPropertyInitializerOutsideConstructor)); } } @@ -924,7 +1016,36 @@ namespace ts.refactor.extractMethod { return { target, usagesPerScope, errorsPerScope }; + function hasTypeParameters(node: Node) { + return isDeclarationWithTypeParameters(node) && + node.typeParameters !== undefined && + node.typeParameters.length > 0; + } + + function isInGenericContext(node: Node) { + for (; node; node = node.parent) { + if (hasTypeParameters(node)) { + return true; + } + } + + return false; + } + function collectUsages(node: Node, valueUsage = Usage.Read) { + if (inGenericContext) { + const type = checker.getTypeAtLocation(node); + + const symbolWalker = checker.getSymbolWalker(); + const {visitedTypes} = symbolWalker.walkType(type); + + for (const visitedType of visitedTypes) { + if (visitedType.flags & TypeFlags.TypeParameter) { + allTypeParameterUsages.set(visitedType.id.toString(), visitedType as TypeParameter); + } + } + } + if (isDeclaration(node) && node.symbol) { visibleDeclarationsInExtractedRange.push(node.symbol); }