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

Refactor jsdoc types to typescript #18747

Merged
merged 19 commits into from
Oct 13, 2017
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -3703,5 +3703,13 @@
"Extract to {0}": {
"category": "Message",
"code": 95004
},
"Annotate with type from JSDoc": {
"category": "Message",
"code": 95005
},
"Annotate with return type from JSDoc": {
"category": "Message",
"code": 95006
}
}
55 changes: 53 additions & 2 deletions src/compiler/emitter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -546,6 +546,8 @@ namespace ts {
return emitTypeReference(<TypeReferenceNode>node);
case SyntaxKind.FunctionType:
return emitFunctionType(<FunctionTypeNode>node);
case SyntaxKind.JSDocFunctionType:
return emitJSDocFunctionType(node as JSDocFunctionType);
case SyntaxKind.ConstructorType:
return emitConstructorType(<ConstructorTypeNode>node);
case SyntaxKind.TypeQuery:
Expand Down Expand Up @@ -574,6 +576,20 @@ namespace ts {
return emitMappedType(<MappedTypeNode>node);
case SyntaxKind.LiteralType:
return emitLiteralType(<LiteralTypeNode>node);
case SyntaxKind.JSDocAllType:
write("*");
break;
Copy link
Member

Choose a reason for hiding this comment

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

These should be return and not break, otherwise someone might add them to the isToken check at the bottom in the future and they could get printed twice.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch, thanks.

case SyntaxKind.JSDocUnknownType:
write("?");
break;
case SyntaxKind.JSDocNullableType:
return emitJSDocNullableType(node as JSDocNullableType);
case SyntaxKind.JSDocNonNullableType:
return emitJSDocNonNullableType(node as JSDocNonNullableType);
case SyntaxKind.JSDocOptionalType:
return emitJSDocOptionalType(node as JSDocOptionalType);
case SyntaxKind.JSDocVariadicType:
return emitJSDocVariadicType(node as JSDocVariadicType);

// Binding patterns
case SyntaxKind.ObjectBindingPattern:
Expand Down Expand Up @@ -914,9 +930,16 @@ namespace ts {
emitDecorators(node, node.decorators);
emitModifiers(node, node.modifiers);
emitIfPresent(node.dotDotDotToken);
emit(node.name);
if (node.name) {
emit(node.name);
}
emitIfPresent(node.questionToken);
emitWithPrefix(": ", node.type);
if (node.parent && node.parent.kind === SyntaxKind.JSDocFunctionType && !node.name) {
emit(node.type);
}
else {
emitWithPrefix(": ", node.type);
}
emitExpressionWithPrefix(" = ", node.initializer);
}

Expand Down Expand Up @@ -1035,6 +1058,29 @@ namespace ts {
emit(node.type);
}

function emitJSDocFunctionType(node: JSDocFunctionType) {
write("function");
emitParameters(node, node.parameters);
write(":");
emit(node.type);
}


function emitJSDocNullableType(node: JSDocNullableType) {
write("?");
emit(node.type);
}

function emitJSDocNonNullableType(node: JSDocNonNullableType) {
write("!");
emit(node.type);
}

function emitJSDocOptionalType(node: JSDocOptionalType) {
emit(node.type);
write("=");
}

function emitConstructorType(node: ConstructorTypeNode) {
write("new ");
emitTypeParameters(node, node.typeParameters);
Expand All @@ -1060,6 +1106,11 @@ namespace ts {
write("[]");
}

function emitJSDocVariadicType(node: JSDocVariadicType) {
write("...");
emit(node.type);
}

function emitTupleType(node: TupleTypeNode) {
write("[");
emitList(node, node.elementTypes, ListFormat.TupleTypeElements);
Expand Down
9 changes: 8 additions & 1 deletion src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5034,7 +5034,14 @@ namespace ts {
|| kind === SyntaxKind.UndefinedKeyword
|| kind === SyntaxKind.NullKeyword
|| kind === SyntaxKind.NeverKeyword
|| kind === SyntaxKind.ExpressionWithTypeArguments;
|| kind === SyntaxKind.ExpressionWithTypeArguments
|| kind === SyntaxKind.JSDocAllType
|| kind === SyntaxKind.JSDocUnknownType
|| kind === SyntaxKind.JSDocNullableType
|| kind === SyntaxKind.JSDocNonNullableType
|| kind === SyntaxKind.JSDocOptionalType
|| kind === SyntaxKind.JSDocFunctionType
|| kind === SyntaxKind.JSDocVariadicType;
}

/**
Expand Down
224 changes: 224 additions & 0 deletions src/services/refactors/annotateWithTypeFromJSDoc.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,224 @@
/* @internal */
namespace ts.refactor.annotateWithTypeFromJSDoc {
const actionName = "annotate";

const annotateTypeFromJSDoc: Refactor = {
name: "Annotate with type from JSDoc",
description: Diagnostics.Annotate_with_type_from_JSDoc.message,
getEditsForAction,
getAvailableActions
};
const annotateReturnTypeFromJSDoc: Refactor = {
name: "Annotate with return type from JSDoc",
description: Diagnostics.Annotate_with_return_type_from_JSDoc.message,
getEditsForAction,
getAvailableActions
};

type DeclarationWithType =
| FunctionLikeDeclaration
| VariableDeclaration
| ParameterDeclaration
| PropertySignature
| PropertyDeclaration;

registerRefactor(annotateTypeFromJSDoc);
registerRefactor(annotateReturnTypeFromJSDoc);

function getAvailableActions(context: RefactorContext): ApplicableRefactorInfo[] | undefined {
if (isInJavaScriptFile(context.file)) {
return undefined;
}

const node = getTokenAtPosition(context.file, context.startPosition, /*includeJsDocComment*/ false);
const decl = findAncestor(node, isTypedNode);
if (decl && !decl.type) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you use early bailouts to reduce nesting here?

const annotate = getJSDocType(decl) ? annotateTypeFromJSDoc :
getJSDocReturnType(decl) ? annotateReturnTypeFromJSDoc :
undefined;
if (annotate) {
return [{
name: annotate.name,
description: annotate.description,
actions: [
{
description: annotate.description,
name: actionName
}
]
}];
}
}
}

function getEditsForAction(context: RefactorContext, action: string): RefactorEditInfo | undefined {
// Somehow wrong action got invoked?
if (actionName !== action) {
Debug.fail(`actionName !== action: ${actionName} !== ${action}`);
return undefined;
Copy link
Member

Choose a reason for hiding this comment

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

return Debug.fail(...)?

}

const start = context.startPosition;
const sourceFile = context.file;
const token = getTokenAtPosition(sourceFile, start, /*includeJsDocComment*/ false);
const decl = findAncestor(token, isTypedNode);
const jsdocType = getJSDocReturnType(decl) || getJSDocType(decl);
if (!decl || !jsdocType || decl.type) {
Debug.fail(`!decl || !jsdocType || decl.type: !${decl} || !${jsdocType} || ${decl.type}`);
return undefined;
}

const changeTracker = textChanges.ChangeTracker.fromContext(context);
if (isParameterOfSimpleArrowFunction(decl)) {
// `x => x` becomes `(x: number) => x`, but in order to make the changeTracker generate the parentheses,
// we have to replace the entire function; it doesn't check that the node it's replacing might require
// other syntax changes
const arrow = decl.parent as ArrowFunction;
const param = decl as ParameterDeclaration;
const replacementParam = createParameter(param.decorators, param.modifiers, param.dotDotDotToken, param.name, param.questionToken, transformJSDocType(jsdocType) as TypeNode, param.initializer);
const replacement = createArrowFunction(arrow.modifiers, arrow.typeParameters, [replacementParam], arrow.type, arrow.equalsGreaterThanToken, arrow.body);
changeTracker.replaceRange(sourceFile, { pos: arrow.getStart(), end: arrow.end }, replacement);
}
else {
changeTracker.replaceRange(sourceFile, { pos: decl.getStart(), end: decl.end }, replaceType(decl, transformJSDocType(jsdocType) as TypeNode));
}
return {
edits: changeTracker.getChanges(),
renameFilename: undefined,
renameLocation: undefined
};
}

function isTypedNode(node: Node): node is DeclarationWithType {
Copy link
Contributor

Choose a reason for hiding this comment

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

TypeNode has a rather well defined meaning in other parts of the system. so i would make it isDeclarationWithType instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, that name is much better.

return isFunctionLikeDeclaration(node) ||
node.kind === SyntaxKind.VariableDeclaration ||
node.kind === SyntaxKind.Parameter ||
node.kind === SyntaxKind.PropertySignature ||
node.kind === SyntaxKind.PropertyDeclaration;
}

function replaceType(decl: DeclarationWithType, jsdocType: TypeNode) {
switch (decl.kind) {
case SyntaxKind.VariableDeclaration:
return createVariableDeclaration(decl.name, jsdocType, decl.initializer);
case SyntaxKind.Parameter:
return createParameter(decl.decorators, decl.modifiers, decl.dotDotDotToken, decl.name, decl.questionToken, jsdocType, decl.initializer);
case SyntaxKind.PropertySignature:
return createPropertySignature(decl.modifiers, decl.name, decl.questionToken, jsdocType, decl.initializer);
case SyntaxKind.PropertyDeclaration:
return createProperty(decl.decorators, decl.modifiers, decl.name, decl.questionToken, jsdocType, decl.initializer);
case SyntaxKind.FunctionDeclaration:
return createFunctionDeclaration(decl.decorators, decl.modifiers, decl.asteriskToken, decl.name, decl.typeParameters, decl.parameters, jsdocType, decl.body);
case SyntaxKind.FunctionExpression:
return createFunctionExpression(decl.modifiers, decl.asteriskToken, decl.name, decl.typeParameters, decl.parameters, jsdocType, decl.body);
case SyntaxKind.ArrowFunction:
return createArrowFunction(decl.modifiers, decl.typeParameters, decl.parameters, jsdocType, decl.equalsGreaterThanToken, decl.body);
case SyntaxKind.MethodDeclaration:
return createMethod(decl.decorators, decl.modifiers, decl.asteriskToken, decl.name, decl.questionToken, decl.typeParameters, decl.parameters, jsdocType, decl.body);
case SyntaxKind.GetAccessor:
return createGetAccessor(decl.decorators, decl.modifiers, decl.name, decl.parameters, jsdocType, decl.body);
default:
Debug.fail(`Unexpected SyntaxKind: ${decl.kind}`);
return undefined;
}
}

function isParameterOfSimpleArrowFunction(decl: DeclarationWithType) {
return decl.kind === SyntaxKind.Parameter && decl.parent.kind === SyntaxKind.ArrowFunction && isSimpleArrowFunction(decl.parent);
}

function isSimpleArrowFunction(parentNode: FunctionTypeNode | ArrowFunction | JSDocFunctionType) {
const parameter = singleOrUndefined(parentNode.parameters);
return parameter
&& parameter.pos === parentNode.pos // may not have parsed tokens between parent and parameter
&& !(isArrowFunction(parentNode) && parentNode.type) // arrow function may not have return type annotation
&& !some(parentNode.decorators) // parent may not have decorators
&& !some(parentNode.modifiers) // parent may not have modifiers
&& !some(parentNode.typeParameters) // parent may not have type parameters
&& !some(parameter.decorators) // parameter may not have decorators
&& !some(parameter.modifiers) // parameter may not have modifiers
&& !parameter.dotDotDotToken // parameter may not be rest
&& !parameter.questionToken // parameter may not be optional
&& !parameter.type // parameter may not have a type annotation
&& !parameter.initializer // parameter may not have an initializer
&& isIdentifier(parameter.name); // parameter name must be identifier
}

function transformJSDocType(node: Node): Node | undefined {
if (node === undefined) {
return undefined;
}
switch (node.kind) {
case SyntaxKind.JSDocAllType:
case SyntaxKind.JSDocUnknownType:
return createTypeReferenceNode("any", emptyArray);
case SyntaxKind.JSDocOptionalType:
return visitJSDocOptionalType(node as JSDocOptionalType);
Copy link
Contributor

@mhegazy mhegazy Oct 12, 2017

Choose a reason for hiding this comment

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

consider calling them all transform*

case SyntaxKind.JSDocNonNullableType:
return transformJSDocType((node as JSDocNonNullableType).type);
case SyntaxKind.JSDocNullableType:
return visitJSDocNullableType(node as JSDocNullableType);
case SyntaxKind.JSDocVariadicType:
return visitJSDocVariadicType(node as JSDocVariadicType);
case SyntaxKind.JSDocFunctionType:
return visitJSDocFunctionType(node as JSDocFunctionType);
case SyntaxKind.Parameter:
return visitJSDocParameter(node as ParameterDeclaration);
case SyntaxKind.TypeReference:
return visitJSDocTypeReference(node as TypeReferenceNode);
default:
return visitEachChild(node, transformJSDocType, /*context*/ undefined) as TypeNode;
}
}

function visitJSDocOptionalType(node: JSDocOptionalType) {
return createUnionTypeNode([visitNode(node.type, transformJSDocType), createTypeReferenceNode("undefined", emptyArray)]);
}

function visitJSDocNullableType(node: JSDocNullableType) {
return createUnionTypeNode([visitNode(node.type, transformJSDocType), createTypeReferenceNode("null", emptyArray)]);
}

function visitJSDocVariadicType(node: JSDocVariadicType) {
return createArrayTypeNode(visitNode(node.type, transformJSDocType));
}

function visitJSDocFunctionType(node: JSDocFunctionType) {
const parameters = node.parameters && node.parameters.map(transformJSDocType);
return createFunctionTypeNode(emptyArray, parameters as ParameterDeclaration[], node.type);
}

function visitJSDocParameter(node: ParameterDeclaration) {
const name = node.name || "arg" + node.parent.parameters.indexOf(node);
Copy link
Member

Choose a reason for hiding this comment

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

Consider rest for a rest parameter instead of argN?

return createParameter(node.decorators, node.modifiers, node.dotDotDotToken, name, node.questionToken, node.type, node.initializer);
}

function visitJSDocTypeReference(node: TypeReferenceNode) {
Copy link
Member

Choose a reason for hiding this comment

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

Would it be worth converting Array.<T> to (T)[], rather than Array<T>?

Copy link
Member

@DanielRosenwasser DanielRosenwasser Sep 28, 2017

Choose a reason for hiding this comment

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

@weswigham why parentheses?

Copy link
Member Author

Choose a reason for hiding this comment

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

@weswigham Nope. I don't want to introduce arbitrary style fixups, just ones that we believe are nearly always wrong. And Array<T> is a style choice that is favoured by lots (a majority?) of the JS community.

Copy link
Member

Choose a reason for hiding this comment

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

@DanielRosenwasser I just included those in case T was string | number, to serve as a reminder parens may be needed. But if @sandersn wants to preserve the existing style, that's fine.

let name = node.typeName;
let args = node.typeArguments;
if (isIdentifier(node.typeName)) {
let text = node.typeName.text;
switch (node.typeName.text) {
case "String":
case "Boolean":
case "Object":
case "Number":
text = text.toLowerCase();
break;
case "array":
case "date":
case "promise":
text = text[0].toUpperCase() + text.slice(1);
break;
}
name = createIdentifier(text);
if ((text === "Array" || text === "Promise") && !node.typeArguments) {
args = createNodeArray([createTypeReferenceNode("any", emptyArray)]);
}
else {
args = visitNodes(node.typeArguments, transformJSDocType);
}
}
return createTypeReferenceNode(name, args);
}
}
1 change: 1 addition & 0 deletions src/services/refactors/refactors.ts
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
/// <reference path="annotateWithTypeFromJSDoc.ts" />
/// <reference path="convertFunctionToEs6Class.ts" />
/// <reference path="extractMethod.ts" />
10 changes: 10 additions & 0 deletions tests/cases/fourslash/annotateWithTypeFromJSDoc1.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/// <reference path='fourslash.ts' />

// @Filename: test123.ts
/////** @type {number} */
////var /*1*/x;

verify.applicableRefactorAvailableAtMarker('1');
verify.fileAfterApplyingRefactorAtMarker('1',
`/** @type {number} */
var x: number;`, 'Annotate with type from JSDoc', 'annotate');
15 changes: 15 additions & 0 deletions tests/cases/fourslash/annotateWithTypeFromJSDoc10.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/// <reference path='fourslash.ts' />

/////**
//// * @param {?} x
//// * @returns {number}
//// */
////var f = /*1*/(/*2*/x) => x

verify.applicableRefactorAvailableAtMarker('1');
verify.fileAfterApplyingRefactorAtMarker('1',
`/**
* @param {?} x
* @returns {number}
*/
var f = (x): number => x`, 'Annotate with return type from JSDoc', 'annotate');
15 changes: 15 additions & 0 deletions tests/cases/fourslash/annotateWithTypeFromJSDoc11.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/// <reference path='fourslash.ts' />

/////**
//// * @param {?} x
//// * @returns {number}
//// */
////var f = /*1*/(/*2*/x) => x

verify.applicableRefactorAvailableAtMarker('2');
verify.fileAfterApplyingRefactorAtMarker('2',
`/**
* @param {?} x
* @returns {number}
*/
var f = (x: any) => x`, 'Annotate with type from JSDoc', 'annotate');
Loading