Skip to content
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

Merged
merged 22 commits into from May 7, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/compiler/diagnosticMessages.json
Expand Up @@ -4962,6 +4962,19 @@
"category": "Message",
"code": 95076
},
"Extract type": {
"category": "Message",
"code": 95077
},
"Extract to type alias": {
"category": "Message",
"code": 95078
},
"Extract to typedef": {
"category": "Message",
"code": 95079
},

"No value exists in scope for the shorthand property '{0}'. Either declare one or provide an initializer." :{
"category": "Error",
"code": 18004
Expand Down
147 changes: 147 additions & 0 deletions src/services/refactors/extractType.ts
@@ -0,0 +1,147 @@
/* @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, isStatementAndHasJSDoc) : findAncestor(selection, isStatement));
const typeParameters = collectTypeParameters(checker, selection, firstStatement, file);
if (!typeParameters) return undefined;

return { isJS, selection, firstStatement, typeParameters };
}

function isStatementAndHasJSDoc(n: Node): n is (Statement & HasJSDoc) {
return isStatement(n) && hasJSDocNodes(n);
}

function rangeContainsSkipTrivia(r1: TextRange, node: Node, file: SourceFile): boolean {
return rangeContainsStartEnd(r1, skipTrivia(file.text, node.pos), node.end);
}

function collectTypeParameters(checker: TypeChecker, selection: TypeNode, statement: Statement, file: SourceFile): TypeParameterDeclaration[] | undefined {
const result: TypeParameterDeclaration[] = [];
return visitor(selection) ? undefined : result;

function visitor(node: Node): true | undefined {
if (isTypeReferenceNode(node)) {
if (isIdentifier(node.typeName)) {
Copy link
Member

@gabritto gabritto Mar 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is it not necessary to handle the case where node.typeName is not an identifier but is a qualified name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could TypeParameter be a QualifiedName?
In checkAndCollectionTypeArguments we only iter the TypeParameter whitch defined in the statement

Copy link
Member

@gabritto gabritto Mar 29, 2019

Choose a reason for hiding this comment

The 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.
update: came up with an example of a qualified name for the type query check. i think there could be unusual cases where this check can fail.

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));
Copy link
Member

Choose a reason for hiding this comment

The 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 hasError but also (2) not push any results into result. What happens if you get an empty result list?

Test cases 31-33 show that this works, but I don't understand why.

Copy link
Contributor Author

@Kingwl Kingwl May 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That caused:

  1. find the selections by largest and topmost type node
  2. Infer type node contains only a TypeParameterDeclaration

in the following case:

type A = /*a*/Promise/*b*/

The selection is the TypeReference which have the same range as the Identifier``Promise.

In cases 31 and 32 were set the hasError flag.
And the case33:

type Item<T> = T extends (infer /*a*/P/*b*/)[] ? P : never

The selection is the TypeParameter(maybe, or an Identifier), that is a declaration rather a TypeNode. and we are already escaped before collectTypeParameters.

if (!conditionalTypeNode || !rangeContainsSkipTrivia(selection, conditionalTypeNode, file)) {
return true;
}
}
else if ((isTypePredicateNode(node) || isThisTypeNode(node))) {
const functionLikeNode = findAncestor(node.parent, isFunctionLike);
if (functionLikeNode && functionLikeNode.type && rangeContainsSkipTrivia(functionLikeNode.type, node, file) && !rangeContainsSkipTrivia(selection, functionLikeNode, file)) {
return true;
}
}
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)) {
return true;
}
}
else {
if (isThisIdentifier(node.exprName.left) && !rangeContainsSkipTrivia(selection, node.parent, file)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should skip this elsewhere too:

class C {
    m<T>(): /*a*/T | this | number/*b*/ {
        return {} as any
    }
}

currently produces type NewType<T> = T | this | number

return 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))));
}
}
1 change: 1 addition & 0 deletions src/services/tsconfig.json
Expand Up @@ -80,6 +80,7 @@
"refactors/convertExport.ts",
"refactors/convertImport.ts",
"refactors/extractSymbol.ts",
"refactors/extractType.ts",
"refactors/generateGetAccessorAndSetAccessor.ts",
"refactors/moveToNewFile.ts",
"refactors/addOrRemoveBracesToArrowFunction.ts",
Expand Down
16 changes: 16 additions & 0 deletions tests/cases/fourslash/refactorExtractType1.ts
@@ -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 = { };`,
});
17 changes: 17 additions & 0 deletions tests/cases/fourslash/refactorExtractType10.ts
@@ -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
}`,
});
17 changes: 17 additions & 0 deletions tests/cases/fourslash/refactorExtractType11.ts
@@ -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
}`,
});
21 changes: 21 additions & 0 deletions tests/cases/fourslash/refactorExtractType12.ts
@@ -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
}`,
});
21 changes: 21 additions & 0 deletions tests/cases/fourslash/refactorExtractType13.ts
@@ -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
}`,
});
13 changes: 13 additions & 0 deletions tests/cases/fourslash/refactorExtractType14.ts
@@ -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`,
});
13 changes: 13 additions & 0 deletions tests/cases/fourslash/refactorExtractType15.ts
@@ -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`,
});
13 changes: 13 additions & 0 deletions tests/cases/fourslash/refactorExtractType16.ts
@@ -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 } = { };`,
});
13 changes: 13 additions & 0 deletions tests/cases/fourslash/refactorExtractType17.ts
@@ -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>`,
});
14 changes: 14 additions & 0 deletions tests/cases/fourslash/refactorExtractType18.ts
@@ -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`,
});

14 changes: 14 additions & 0 deletions tests/cases/fourslash/refactorExtractType19.ts
@@ -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`,
});

13 changes: 13 additions & 0 deletions tests/cases/fourslash/refactorExtractType2.ts
@@ -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 = '';`,
});
13 changes: 13 additions & 0 deletions tests/cases/fourslash/refactorExtractType20.ts
@@ -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`,
});