-
Notifications
You must be signed in to change notification settings - Fork 12.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add refactor of extract type #30562
add refactor of extract type #30562
Changes from 20 commits
0b66cfc
8a6f1ed
3948938
379de4b
0f414b4
9e405fc
ead7b05
5749f7f
0e37661
0c1866c
65ecee5
26ff926
566da68
73e8297
5c28b60
a861ec4
788d0d2
ee09157
af994fc
2a8a7e4
1dd1c68
bff56b7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,154 @@ | ||
/* @internal */ | ||
namespace ts.refactor { | ||
const refactorName = "Extract type"; | ||
const extractToTypeAlias = "Extract to type alias"; | ||
const extractToTypeDef = "Extract to typedef"; | ||
registerRefactor(refactorName, { | ||
getAvailableActions(context): ReadonlyArray<ApplicableRefactorInfo> { | ||
const info = getRangeToExtract(context); | ||
if (!info) return emptyArray; | ||
|
||
return [{ | ||
name: refactorName, | ||
description: getLocaleSpecificMessage(Diagnostics.Extract_type), | ||
actions: [info.isJS ? { | ||
name: extractToTypeDef, description: getLocaleSpecificMessage(Diagnostics.Extract_to_typedef) | ||
} : { | ||
name: extractToTypeAlias, description: getLocaleSpecificMessage(Diagnostics.Extract_to_type_alias) | ||
}] | ||
}]; | ||
}, | ||
getEditsForAction(context, actionName): RefactorEditInfo { | ||
Debug.assert(actionName === extractToTypeAlias || actionName === extractToTypeDef); | ||
const { file } = context; | ||
const info = Debug.assertDefined(getRangeToExtract(context)); | ||
Debug.assert(actionName === extractToTypeAlias && !info.isJS || actionName === extractToTypeDef && info.isJS); | ||
|
||
const name = getUniqueName("NewType", file); | ||
const edits = textChanges.ChangeTracker.with(context, changes => info.isJS ? | ||
doTypedefChange(changes, file, name, info.firstStatement, info.selection, info.typeParameters) : | ||
doTypeAliasChange(changes, file, name, info.firstStatement, info.selection, info.typeParameters)); | ||
|
||
const renameFilename = file.fileName; | ||
const renameLocation = getRenameLocation(edits, renameFilename, name, /*preferLastLocation*/ false); | ||
return { edits, renameFilename, renameLocation }; | ||
} | ||
}); | ||
|
||
interface Info { isJS: boolean; selection: TypeNode; firstStatement: Statement; typeParameters: ReadonlyArray<TypeParameterDeclaration>; } | ||
|
||
function getRangeToExtract(context: RefactorContext): Info | undefined { | ||
const { file, startPosition } = context; | ||
const isJS = isSourceFileJS(file); | ||
const current = getTokenAtPosition(file, startPosition); | ||
const range = createTextRangeFromSpan(getRefactorContextSpan(context)); | ||
|
||
const selection = findAncestor(current, (node => node.parent && rangeContainsSkipTrivia(range, node, file) && !rangeContainsSkipTrivia(range, node.parent, file))); | ||
if (!selection || !isTypeNode(selection)) return undefined; | ||
|
||
const checker = context.program.getTypeChecker(); | ||
const firstStatement = Debug.assertDefined(isJS ? findAncestor(selection, isStatementButNotBlockAndHasJSDoc) : findAncestor(selection, isStatementButNotBlock)); | ||
const typeParameters = checkAndCollectionTypeArguments(checker, selection, firstStatement, file); | ||
if (!typeParameters) return undefined; | ||
|
||
return { isJS, selection, firstStatement, typeParameters }; | ||
} | ||
|
||
function isStatementButNotBlock(n: Node): n is Statement { | ||
return n && isStatement(n) && !isBlock(n); | ||
} | ||
|
||
function isStatementButNotBlockAndHasJSDoc(n: Node): n is (Statement & HasJSDoc) { | ||
return isStatementButNotBlock(n) && hasJSDocNodes(n); | ||
} | ||
|
||
function rangeContainsSkipTrivia(r1: TextRange, node: Node, file: SourceFile): boolean { | ||
return rangeContainsStartEnd(r1, skipTrivia(file.text, node.pos), node.end); | ||
} | ||
|
||
function checkAndCollectionTypeArguments(checker: TypeChecker, selection: TypeNode, statement: Statement, file: SourceFile): TypeParameterDeclaration[] | undefined { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. probably should be named |
||
let hasError = false; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. seems more elegant to turn this into a return value. It looks like the expected behaviour is to return undefined whenever |
||
const result: TypeParameterDeclaration[] = []; | ||
visitor(selection); | ||
return hasError ? undefined : result; | ||
|
||
function visitor(node: Node) { | ||
if (isTypeReferenceNode(node)) { | ||
if (isIdentifier(node.typeName)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is it not necessary to handle the case where There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could TypeParameter be a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think in the common cases where you have to collect the type, the type name is an identifier and not a qualified name. but i'm really not sure, so that's why i asked. |
||
const symbol = checker.resolveName(node.typeName.text, node.typeName, SymbolFlags.TypeParameter, /* excludeGlobals */ true); | ||
if (symbol) { | ||
const declaration = cast(first(symbol.declarations), isTypeParameterDeclaration); | ||
if (rangeContainsSkipTrivia(statement, declaration, file) && !rangeContainsSkipTrivia(selection, declaration, file)) { | ||
result.push(declaration); | ||
} | ||
} | ||
} | ||
} | ||
else if (isInferTypeNode(node)) { | ||
const conditionalTypeNode = findAncestor(node, n => isConditionalTypeNode(n) && rangeContainsSkipTrivia(n.extendsType, node, file)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if the selection contains only a syntactically-correct infer type, then I think this code should (1) not set Test cases 31-33 show that this works, but I don't understand why. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That caused:
in the following case: type A = /*a*/Promise/*b*/ The selection is the In cases 31 and 32 were set the type Item<T> = T extends (infer /*a*/P/*b*/)[] ? P : never The selection is the |
||
if (!conditionalTypeNode || !rangeContainsSkipTrivia(selection, conditionalTypeNode, file)) { | ||
hasError = true; | ||
return; | ||
} | ||
} | ||
else if ((isTypePredicateNode(node) || isThisTypeNode(node)) && !rangeContainsSkipTrivia(selection, node.parent, file)) { | ||
hasError = true; | ||
return; | ||
} | ||
else if (isTypeQueryNode(node)) { | ||
if (isIdentifier(node.exprName)) { | ||
const symbol = checker.resolveName(node.exprName.text, node.exprName, SymbolFlags.Value, /* excludeGlobals */ false); | ||
if (symbol && rangeContainsSkipTrivia(statement, symbol.valueDeclaration, file) && !rangeContainsSkipTrivia(selection, symbol.valueDeclaration, file)) { | ||
hasError = true; | ||
return; | ||
} | ||
} | ||
else { | ||
if (isThisIdentifier(node.exprName.left) && !rangeContainsSkipTrivia(selection, node.parent, file)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you should skip class C {
m<T>(): /*a*/T | this | number/*b*/ {
return {} as any
}
} currently produces |
||
hasError = true; | ||
return; | ||
} | ||
} | ||
} | ||
forEachChild(node, visitor); | ||
} | ||
} | ||
|
||
function doTypeAliasChange(changes: textChanges.ChangeTracker, file: SourceFile, name: string, firstStatement: Statement, selection: TypeNode, typeParameters: ReadonlyArray<TypeParameterDeclaration>) { | ||
const newTypeNode = createTypeAliasDeclaration( | ||
/* decorators */ undefined, | ||
/* modifiers */ undefined, | ||
name, | ||
typeParameters.map(id => updateTypeParameterDeclaration(id, id.name, id.constraint, /* defaultType */ undefined)), | ||
selection | ||
); | ||
changes.insertNodeBefore(file, firstStatement, newTypeNode, /* blankLineBetween */ true); | ||
changes.replaceNode(file, selection, createTypeReferenceNode(name, typeParameters.map(id => createTypeReferenceNode(id.name, /* typeArguments */ undefined)))); | ||
} | ||
|
||
function doTypedefChange(changes: textChanges.ChangeTracker, file: SourceFile, name: string, firstStatement: Statement, selection: TypeNode, typeParameters: ReadonlyArray<TypeParameterDeclaration>) { | ||
const node = <JSDocTypedefTag>createNode(SyntaxKind.JSDocTypedefTag); | ||
node.tagName = createIdentifier("typedef"); // TODO: jsdoc factory https://github.com/Microsoft/TypeScript/pull/29539 | ||
node.fullName = createIdentifier(name); | ||
node.name = node.fullName; | ||
node.typeExpression = createJSDocTypeExpression(selection); | ||
|
||
const templates: JSDocTemplateTag[] = []; | ||
forEach(typeParameters, typeParameter => { | ||
const constraint = getEffectiveConstraintOfTypeParameter(typeParameter); | ||
|
||
const template = <JSDocTemplateTag>createNode(SyntaxKind.JSDocTemplateTag); | ||
template.tagName = createIdentifier("template"); | ||
template.constraint = constraint && cast(constraint, isJSDocTypeExpression); | ||
|
||
const parameter = <TypeParameterDeclaration>createNode(SyntaxKind.TypeParameter); | ||
parameter.name = typeParameter.name; | ||
template.typeParameters = createNodeArray([parameter]); | ||
|
||
templates.push(template); | ||
}); | ||
|
||
changes.insertNodeBefore(file, firstStatement, createJSDocComment(/* comment */ undefined, createNodeArray(concatenate<JSDocTag>(templates, [node]))), /* blankLineBetween */ true); | ||
changes.replaceNode(file, selection, createTypeReferenceNode(name, typeParameters.map(id => createTypeReferenceNode(id.name, /* typeArguments */ undefined)))); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
/// <reference path='fourslash.ts' /> | ||
|
||
//// var x: /*a*/{ a?: number, b?: string }/*b*/ = { }; | ||
|
||
goTo.select("a", "b"); | ||
edit.applyRefactor({ | ||
refactorName: "Extract type", | ||
actionName: "Extract to type alias", | ||
actionDescription: "Extract to type alias", | ||
newContent: `type /*RENAME*/NewType = { | ||
a?: number; | ||
b?: string; | ||
}; | ||
|
||
var x: NewType = { };`, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
/// <reference path='fourslash.ts' /> | ||
|
||
//// function foo(a: number, b?: number, ...c: number[]): /*a*/boolean/*b*/ { | ||
//// return false as boolean | ||
//// } | ||
|
||
goTo.select("a", "b"); | ||
edit.applyRefactor({ | ||
refactorName: "Extract type", | ||
actionName: "Extract to type alias", | ||
actionDescription: "Extract to type alias", | ||
newContent: `type /*RENAME*/NewType = boolean; | ||
|
||
function foo(a: number, b?: number, ...c: number[]): NewType { | ||
return false as boolean | ||
}`, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
/// <reference path='fourslash.ts' /> | ||
|
||
//// function foo(a: number, b?: number, ...c: number[]): boolean { | ||
//// return false as /*a*/boolean/*b*/ | ||
//// } | ||
|
||
goTo.select("a", "b"); | ||
edit.applyRefactor({ | ||
refactorName: "Extract type", | ||
actionName: "Extract to type alias", | ||
actionDescription: "Extract to type alias", | ||
newContent: `function foo(a: number, b?: number, ...c: number[]): boolean { | ||
type /*RENAME*/NewType = boolean; | ||
|
||
return false as NewType | ||
}`, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
/// <reference path='fourslash.ts' /> | ||
|
||
//// interface A<T = /*a*/string/*b*/> { | ||
//// a: boolean | ||
//// b: number | ||
//// c: T | ||
//// } | ||
|
||
goTo.select("a", "b"); | ||
edit.applyRefactor({ | ||
refactorName: "Extract type", | ||
actionName: "Extract to type alias", | ||
actionDescription: "Extract to type alias", | ||
newContent: `type /*RENAME*/NewType = string; | ||
|
||
interface A<T = NewType> { | ||
a: boolean | ||
b: number | ||
c: T | ||
}`, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
/// <reference path='fourslash.ts' /> | ||
|
||
//// interface A<T = string> { | ||
//// a: /*a*/boolean/*b*/ | ||
//// b: number | ||
//// c: T | ||
//// } | ||
|
||
goTo.select("a", "b"); | ||
edit.applyRefactor({ | ||
refactorName: "Extract type", | ||
actionName: "Extract to type alias", | ||
actionDescription: "Extract to type alias", | ||
newContent: `type /*RENAME*/NewType = boolean; | ||
|
||
interface A<T = string> { | ||
a: NewType | ||
b: number | ||
c: T | ||
}`, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
/// <reference path='fourslash.ts' /> | ||
|
||
//// type A<T = /*a*/boolean/*b*/> = string | number | T | ||
|
||
goTo.select("a", "b"); | ||
edit.applyRefactor({ | ||
refactorName: "Extract type", | ||
actionName: "Extract to type alias", | ||
actionDescription: "Extract to type alias", | ||
newContent: `type /*RENAME*/NewType = boolean; | ||
|
||
type A<T = NewType> = string | number | T`, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
/// <reference path='fourslash.ts' /> | ||
|
||
//// type A<T = boolean> = /*a*/string/*b*/ | number | T | ||
|
||
goTo.select("a", "b"); | ||
edit.applyRefactor({ | ||
refactorName: "Extract type", | ||
actionName: "Extract to type alias", | ||
actionDescription: "Extract to type alias", | ||
newContent: `type /*RENAME*/NewType = string; | ||
|
||
type A<T = boolean> = NewType | number | T`, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
/// <reference path='fourslash.ts' /> | ||
|
||
//// var x: { a?: /*a*/number/*b*/, b?: string } = { }; | ||
|
||
goTo.select("a", "b"); | ||
edit.applyRefactor({ | ||
refactorName: "Extract type", | ||
actionName: "Extract to type alias", | ||
actionDescription: "Extract to type alias", | ||
newContent: `type /*RENAME*/NewType = number; | ||
|
||
var x: { a?: NewType, b?: string } = { };`, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
/// <reference path='fourslash.ts' /> | ||
|
||
//// type A<T = boolean> = string | number | /*a*/T/*b*/ | ||
|
||
goTo.select("a", "b"); | ||
edit.applyRefactor({ | ||
refactorName: "Extract type", | ||
actionName: "Extract to type alias", | ||
actionDescription: "Extract to type alias", | ||
newContent: `type /*RENAME*/NewType<T> = T; | ||
|
||
type A<T = boolean> = string | number | NewType<T>`, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
/// <reference path='fourslash.ts' /> | ||
|
||
//// type A<B, C, D = B> = /*a*/Partial<C | string>/*b*/ & D | C | ||
|
||
goTo.select("a", "b"); | ||
edit.applyRefactor({ | ||
refactorName: "Extract type", | ||
actionName: "Extract to type alias", | ||
actionDescription: "Extract to type alias", | ||
newContent: `type /*RENAME*/NewType<C> = Partial<C | string>; | ||
|
||
type A<B, C, D = B> = NewType<C> & D | C`, | ||
}); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
/// <reference path='fourslash.ts' /> | ||
|
||
//// type A<B, C, D = B> = /*a*/Partial<C | string | D>/*b*/ & D | C | ||
|
||
goTo.select("a", "b"); | ||
edit.applyRefactor({ | ||
refactorName: "Extract type", | ||
actionName: "Extract to type alias", | ||
actionDescription: "Extract to type alias", | ||
newContent: `type /*RENAME*/NewType<C, D> = Partial<C | string | D>; | ||
|
||
type A<B, C, D = B> = NewType<C, D> & D | C`, | ||
}); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
/// <reference path='fourslash.ts' /> | ||
|
||
//// var x: /*a*/string/*b*/ = ''; | ||
|
||
goTo.select("a", "b"); | ||
edit.applyRefactor({ | ||
refactorName: "Extract type", | ||
actionName: "Extract to type alias", | ||
actionDescription: "Extract to type alias", | ||
newContent: `type /*RENAME*/NewType = string; | ||
|
||
var x: NewType = '';`, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
/// <reference path='fourslash.ts' /> | ||
|
||
//// type A<T, U> = () => <T>(v: /*a*/T/*b*/) => (v: T) => <T>(v: T) => U | ||
|
||
goTo.select("a", "b"); | ||
edit.applyRefactor({ | ||
refactorName: "Extract type", | ||
actionName: "Extract to type alias", | ||
actionDescription: "Extract to type alias", | ||
newContent: `type /*RENAME*/NewType<T> = T; | ||
|
||
type A<T, U> = () => <T>(v: NewType<T>) => (v: T) => <T>(v: T) => U`, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in what cases will you encounter a block before a statement in findAncestor? That is, a block inside a statement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes,
And i'm cannot remember sure why i do that🤷🏻♂️
Is this unnecessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird, I would have that that
//here
would have its own statement, but I guess not. In that case, it seems necessary.