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 20 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 @@ -4951,6 +4951,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
154 changes: 154 additions & 0 deletions src/services/refactors/extractType.ts
@@ -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);
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes,

() => {
  {
   // here
  }
}

And i'm cannot remember sure why i do that🤷🏻‍♂️
Is this unnecessary?

Copy link
Member

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.

}

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 {
Copy link
Member

Choose a reason for hiding this comment

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

probably should be named collectTypeParameters ? I don't think you're really checking them, and it seems like they are intended to be type parameters in addition to arguments, so matching the type name TypeParameterDeclaration feels a little better.

let hasError = false;
Copy link
Member

Choose a reason for hiding this comment

The 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 hasError is true, so stopping the tree walk early should be more efficient.

const result: TypeParameterDeclaration[] = [];
visitor(selection);
return hasError ? undefined : result;

function visitor(node: Node) {
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)) {
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)) {
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

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))));
}
}
1 change: 1 addition & 0 deletions src/services/tsconfig.json
Expand Up @@ -81,6 +81,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`,
});