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 12 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
12 changes: 10 additions & 2 deletions src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -3758,12 +3758,20 @@
"category": "Message",
"code": 95008
},
"Infer type of '{0}' from usage.": {
"Annotate with type from JSDoc": {
"category": "Message",
"code": 95009
},
"Infer parameter types from usage.": {
"Annotate with types from JSDoc": {
"category": "Message",
"code": 95010
},
"Infer type of '{0}' from usage.": {
"category": "Message",
"code": 95011
},
"Infer parameter types from usage.": {
"category": "Message",
"code": 95012
}
}
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("*");
return;
case SyntaxKind.JSDocUnknownType:
write("?");
return;
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 @@ -5130,7 +5130,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: getEditsForAnnotation,
getAvailableActions
};
const annotateFunctionFromJSDoc: Refactor = {
name: "Annotate with types from JSDoc",
description: Diagnostics.Annotate_with_types_from_JSDoc.message,
getEditsForAction: getEditsForFunctionAnnotation,
getAvailableActions
};

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

registerRefactor(annotateTypeFromJSDoc);
registerRefactor(annotateFunctionFromJSDoc);

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 type = getJSDocType(decl);
Copy link
Member

Choose a reason for hiding this comment

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

jsDocType

const returnType = getJSDocReturnType(decl);
const annotate = (returnType || type && decl.kind === SyntaxKind.Parameter) ? annotateFunctionFromJSDoc :
type ? annotateTypeFromJSDoc :
undefined;
if (annotate) {
return [{
name: annotate.name,
description: annotate.description,
actions: [
{
description: annotate.description,
name: actionName
}
]
}];
}
}
}

function getEditsForAnnotation(context: RefactorContext, action: string): RefactorEditInfo | undefined {
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 sourceFile = context.file;
const token = getTokenAtPosition(sourceFile, context.startPosition, /*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);
changeTracker.replaceRange(sourceFile, { pos: decl.getStart(), end: decl.end }, addType(decl, transformJSDocType(jsdocType) as TypeNode));
return {
edits: changeTracker.getChanges(),
renameFilename: undefined,
renameLocation: undefined
};
}

function getEditsForFunctionAnnotation(context: RefactorContext, action: string): RefactorEditInfo | undefined {
if (actionName !== action) {
Debug.fail(`actionName !== action: ${actionName} !== ${action}`);
return undefined;
}

const sourceFile = context.file;
const token = getTokenAtPosition(sourceFile, context.startPosition, /*includeJsDocComment*/ false);
const decl = findAncestor(token, isFunctionLikeDeclaration);
const changeTracker = textChanges.ChangeTracker.fromContext(context);
changeTracker.replaceRange(sourceFile, { pos: decl.getStart(), end: decl.end }, addTypesToFunctionLike(decl));
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 addTypesToFunctionLike(decl: FunctionLikeDeclaration) {
const returnType = decl.type || transformJSDocType(getJSDocReturnType(decl)) as TypeNode;
const parameters = decl.parameters.map(
p => createParameter(p.decorators, p.modifiers, p.dotDotDotToken, p.name, p.questionToken, p.type || transformJSDocType(getJSDocType(p)) as TypeNode, p.initializer));
switch (decl.kind) {
case SyntaxKind.FunctionDeclaration:
return createFunctionDeclaration(decl.decorators, decl.modifiers, decl.asteriskToken, decl.name, decl.typeParameters, parameters, returnType, decl.body);
Copy link
Contributor

Choose a reason for hiding this comment

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

random thought, seems weird that we allow this refactoring on functions with type parameters..

Copy link
Member

Choose a reason for hiding this comment

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

Should probably use getEffectiveTypeParameterDeclarations to pull type parameters from @template tags, no (or a version of it which does not limit it looking at jsdoc to js files)?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's easy to add, I think.

(Also, adding it exposed a bug: functions required @return in order to trigger the refactoring at all, which is wrong.)

case SyntaxKind.Constructor:
return createConstructor(decl.decorators, decl.modifiers, parameters, decl.body);
case SyntaxKind.FunctionExpression:
return createFunctionExpression(decl.modifiers, decl.asteriskToken, (decl as FunctionExpression).name, decl.typeParameters, parameters, returnType, decl.body);
case SyntaxKind.ArrowFunction:
return createArrowFunction(decl.modifiers, decl.typeParameters, parameters, returnType, decl.equalsGreaterThanToken, decl.body);
case SyntaxKind.MethodDeclaration:
return createMethod(decl.decorators, decl.modifiers, decl.asteriskToken, decl.name, decl.questionToken, decl.typeParameters, parameters, returnType, decl.body);
case SyntaxKind.GetAccessor:
return createGetAccessor(decl.decorators, decl.modifiers, decl.name, parameters, returnType, decl.body);
Copy link
Contributor

Choose a reason for hiding this comment

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

a get accessor should have 0 args, so why not just use decl.parameters.

case SyntaxKind.SetAccessor:
return createSetAccessor(decl.decorators, decl.modifiers, decl.name, parameters, decl.body);
default:
return Debug.fail(`Unexpected SyntaxKind: ${(decl as any).kind}`);
Copy link
Member

Choose a reason for hiding this comment

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

Consider a never assertion

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. I didn't know we had Debug.assertNever.

}
}

function addType(decl: DeclarationWithType, jsdocType: TypeNode) {
switch (decl.kind) {
case SyntaxKind.VariableDeclaration:
return createVariableDeclaration(decl.name, 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);
default:
Debug.fail(`Unexpected SyntaxKind: ${decl.kind}`);
return undefined;
}
}

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 index = node.parent.parameters.indexOf(node);
const isRest = node.type.kind === SyntaxKind.JSDocVariadicType && index === node.parent.parameters.length - 1;
const name = node.name || (isRest ? "rest" : "arg" + index);
const dotdotdot = isRest ? createToken(SyntaxKind.DotDotDotToken) : node.dotDotDotToken;
return createParameter(node.decorators, node.modifiers, dotdotdot, name, node.questionToken, visitNode(node.type, transformJSDocType), 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);
}
}
2 changes: 1 addition & 1 deletion src/services/refactors/convertFunctionToEs6Class.ts
Original file line number Diff line number Diff line change
Expand Up @@ -269,4 +269,4 @@ namespace ts.refactor.convertFunctionToES6Class {
return filter(source.modifiers, modifier => modifier.kind === kind);
}
}
}
}
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="extractSymbol.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: any): number => x`, 'Annotate with types from JSDoc', 'annotate');
Loading