-
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 9 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,141 @@ | ||
/* @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) : | ||
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 TypeAliasInfo { isJS: false; selection: TypeNode; firstStatement: Statement; typeParameters: ReadonlyArray<string>; } | ||
interface TypedefInfo { isJS: true; selection: TypeNode; firstStatement: Statement & HasJSDoc; } | ||
type Info = TypeAliasInfo | TypedefInfo; | ||
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; | ||
|
||
if (isJS) { | ||
// typeparam tag is not supported yet | ||
return { | ||
isJS, | ||
selection, | ||
firstStatement: Debug.assertDefined((findAncestor(selection, isStatementButNotBlockAndHasJSDoc))), | ||
}; | ||
} | ||
else { | ||
const firstStatement = Debug.assertDefined((findAncestor(selection, isStatementButNotBlock))); | ||
const typeParameters = checkAndCollectionTypeArguments(context.program.getTypeChecker(), 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 && hasJSDocNodes(n); | ||
Kingwl marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
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): string[] | undefined { | ||
Kingwl marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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: string[] = []; | ||
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 = first(symbol.declarations); | ||
if (rangeContainsSkipTrivia(statement, declaration, file) && !rangeContainsSkipTrivia(selection, declaration, file)) { | ||
result.push(node.typeName.text); | ||
} | ||
} | ||
} | ||
} | ||
else if (isInferTypeNode(node)) { | ||
const conditionalTypeNode = findAncestor(node, isConditionalTypeNode); | ||
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 this might be wrong. we are assuming that the infer type node is only allowed in the extends clause of a conditional type. so we want to check if type Crazy<T> = T extends [infer P, (infer R extends string ? string : never)] ? P & R : string; we can apply the refactoring and extract type NewType = (infer R extends string ? string : never); // error: 'infer' declarations are only permitted in the 'extends' clause of a conditional type
type Crazy<T> = T extends [infer P, NewType] ? P & R : string; which gives an error. 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. sorry, mis operation😅 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. Find type node which is the first condition that contains selected infer type node in extends node |
||
if (!conditionalTypeNode || !rangeContainsSkipTrivia(selection, conditionalTypeNode, file)) { | ||
hasError = true; | ||
return; | ||
} | ||
} | ||
else if (isTypePredicateNode(node) && !rangeContainsSkipTrivia(selection, node.parent, file)) { | ||
hasError = true; | ||
return; | ||
} | ||
else if (isTypeQueryNode(node) && isIdentifier(node.exprName)) { | ||
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. same as above, 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. usually, I think that 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. And is not a TypeParameter and do not have TypeArguments 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. like before, i think common cases are covered by the identifier check, but just came up with the following code: interface I { a: string, f: (this: O, b: number) => typeof this.a }; something bad happens if we extract 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. Banned |
||
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)) { | ||
gabritto marked this conversation as resolved.
Show resolved
Hide resolved
|
||
hasError = true; | ||
return; | ||
} | ||
} | ||
forEachChild(node, visitor); | ||
} | ||
} | ||
|
||
function doTypeAliasChange(changes: textChanges.ChangeTracker, file: SourceFile, name: string, firstStatement: Statement, selection: TypeNode, typeParameters: ReadonlyArray<string>) { | ||
const newTypeNode = createTypeAliasDeclaration( | ||
/* decorators */ undefined, | ||
/* monifiers */ undefined, | ||
Kingwl marked this conversation as resolved.
Show resolved
Hide resolved
|
||
name, | ||
typeParameters.map(id => createTypeParameterDeclaration(id)), | ||
selection | ||
); | ||
changes.insertNodeBefore(file, firstStatement, newTypeNode, /* blankLineBetween */ true); | ||
changes.replaceNode(file, selection, createTypeReferenceNode(name, typeParameters.map(id => createTypeReferenceNode(id, /* typeArguments */ undefined)))); | ||
} | ||
|
||
function doTypedefChange(changes: textChanges.ChangeTracker, file: SourceFile, name: string, firstStatement: HasJSDoc, selection: TypeNode) { | ||
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); | ||
|
||
changes.insertNodeBefore(file, firstStatement, createJSDocComment(/* comment */ undefined, createNodeArray([node])), /* blankLineBetween */ true); | ||
changes.replaceNode(file, selection, createTypeReferenceNode(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`, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
/// <reference path='fourslash.ts' /> | ||
|
||
//// type A<T, U> = () => <T>(v: T) => (v: /*a*/T/*b*/) => <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: T) => (v: NewType<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.