Refactor jsdoc types to typescript #18747

sandersn · 2017-09-25T16:06:51Z

When the caret is on a Typescript declaration that has no type, but does have a JSDoc annotation with a type, this refactor will add the Typescript equivalent of the JSDoc type. If the caret is on a parameter or a function, then it will add types to all the parameters of the function, the return type, and the type parameters, if any of these are available in JSDoc and are not already present on the function.

Notes:

This doesn't delete the JSDoc comment or delete parts of it.
As a bonus, when noImplicitAny: true, this shows up as a code fix in VS Code whenever there is a no-implicit-any error. With noImplicityAny: false, this code must be invoked via the refactoring command.
The emitter needs to understand JSDoc types in case we emit some, but I ended up converting JSDoc types to Typescript types as part of the refactoring.

When the caret is on a Typescript declaration that has no type, but does have a JSDoc annotation with a type, this refactor will add the Typescript equivalent of the JSDoc type. Notes: 1. This doesn't delete the JSDoc comment or delete parts of it. In fact, due to bugs in trivia handling, it sometimes duplicates the comment. These bugs are tracked in #18626. 2. As a bonus, when `noImplicitAny: true`, this shows up as a code fix in VS Code whenever there is a no-implicit-any error. With `noImplicityAny: false`, this code must be invoked via the refactoring command.

DanielRosenwasser · 2017-09-25T21:00:15Z

I haven't looked at the implementation yet, but I just tried this out in a JSDoc codebase. Some feedback about the current functionality.

Boolean, Undefined and the like need to convert properly to boolean, undefined, etc.
Object needs to convert to any or give the option between object and any.
Array needs to get the appropriate type arguments.
We should reconsider whether we understand Mixed.
I don't think Convert to TypeScript type is the appropriate text.
1. You're annotating the variable, you're not converting it.
2. Please make it explicit that you're inferring from JSDoc.
3. For return types, you should make it explicit that you're adding the return type to a function/method. For example
```
class FooBlah {
   /**
   * @return {string}
   */
   foo() {
    this.blah;
  }
}
```
  There's an error span on blah, and your refactoring actually gives me a misleading suggestion to "Convert to TypeScript type".

Other than that I notice we currently don't account for

JSDoc type assertions
JSDoc typedefs
JSDoc augments clauses

sandersn · 2017-09-25T22:42:42Z

@DanielRosenwasser
Here are some replies, in reverse order:

This refactoring only tries to hit the common cases. I can add the other cases later. In particular, JSDoc type assertions are Closure syntax and isn't common outside Closure and mixed TS/JS codebases.

How about "Annotate [return] type from JSDoc"? "Annotate type based on JSDoc"? A smaller repro of your example is:

/** @return {number} */
function f() {
  /*1*/return 12;
}

I didn't think about the fact that other nodes besides FunctionDeclaration could trigger the addition of a return type to the function declaration. You're right, the refactor should at least indicate that a return type will be added.

As for Boolean, Undefined, Object, etc, I'm worried that we'll overshoot the intelligence of this refactor into annoying territory. I'm not even sure we should be converting * and T=, except that those are always errors in Typescript, and I feel bad about creating nodes that are guaranteed to be red-underlined immediately.

I'd like some way to figure out the right amount of translation to do here, but I don't have any data or intuitions right now.

DanielRosenwasser · 2017-09-26T08:18:08Z

I'm worried that we'll overshoot the intelligence of this refactor into annoying territory.

I very much doubt this - I don't think I've ever once seen String used correctly outside of lib.d.ts. You also end up not being able to call anything that expects a string instead.

those are always errors in Typescript

So is Undefined and Null unless you can statically resolve it to something else (probably not worth it)

The emitter now understands JSDoc types but emits them in the original format.

weswigham · 2017-09-26T19:47:32Z

src/services/refactors/annotateWithTypeFromJSDoc.ts

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


Consider rest for a rest parameter instead of argN?

weswigham · 2017-09-26T19:49:29Z

src/compiler/emitter.ts

@@ -574,6 +576,20 @@ namespace ts {
                    return emitMappedType(<MappedTypeNode>node);
                case SyntaxKind.LiteralType:
                    return emitLiteralType(<LiteralTypeNode>node);
+                case SyntaxKind.JSDocAllType:
+                    write("*");
+                    break;


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.

Nice catch, thanks.

weswigham · 2017-09-26T20:12:16Z

src/services/refactors/annotateWithTypeFromJSDoc.ts

+        return createParameter(node.decorators, node.modifiers, node.dotDotDotToken, name, node.questionToken, node.type, node.initializer);
+    }
+
+    function visitJSDocTypeReference(node: TypeReferenceNode) {


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

@weswigham why parentheses?

@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.

@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.

sandersn · 2017-09-26T20:58:56Z

@DanielRosenwasser After talking again to you and @mhegazy, I added a transform that explicitly makes the String → string as well as other transforms.

DanielRosenwasser · 2017-09-28T04:37:09Z

If this works, I think the thing we need to provide is the ability to do it per a signature, if not on the entire file. Too small a level of granularity and users will find this too cumbersome to bother with.

sandersn · 2017-09-28T16:23:42Z

@DanielRosenwasser I agree, but apply-entire-file is definitely an architectural change for refactorings, and apply-entire-signature might be. Either way I'd like to ship this as-is and expand it later.

And give a better name for rest params

sandersn · 2017-10-11T20:44:08Z

I swapped out the annotate-return-type refactor entirely for annotate-entire-signature. It's actually 3 lines shorter and would be even shorter if not for limitations in the change tracker.

mhegazy · 2017-10-12T17:33:47Z

src/services/refactors/annotateWithTypeFromJSDoc.ts

+        };
+    }
+
+    function isTypedNode(node: Node): node is DeclarationWithType {


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

Thanks, that name is much better.

mhegazy · 2017-10-12T17:39:07Z

src/services/refactors/annotateWithTypeFromJSDoc.ts

+            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);


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

mhegazy · 2017-10-12T17:40:06Z

src/services/refactors/annotateWithTypeFromJSDoc.ts

+            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);


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

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)?

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.)

@amcasey

Incorporate suppressLeadingAndTrailingTrivia just added by @amcasey.

sandersn · 2017-10-12T21:30:40Z

I incorporated suppressLeadingAndTrailingTrivia, which was just added by @amcasey, into the PR. This fixes the duplicated JSDoc comments that happened sometimes.

mhegazy · 2017-10-12T23:59:28Z

src/services/refactors/annotateWithTypeFromJSDoc.ts

+            case SyntaxKind.JSDocUnknownType:
+                return createTypeReferenceNode("any", emptyArray);
+            case SyntaxKind.JSDocOptionalType:
+                return visitJSDocOptionalType(node as JSDocOptionalType);


consider calling them all transform*

DanielRosenwasser

Looks good! Would appreciate if you could integrate some of the nits though. :)

DanielRosenwasser · 2017-10-13T06:39:24Z

tests/cases/fourslash/annotateWithTypeFromJSDoc5.ts

+////    /*1*/p = null
+////}
+
+// NOTE: The duplicated comment is unintentional but needs a serious fix in trivia handling


What duplicated comment?

Nice catch. 🐱

DanielRosenwasser · 2017-10-13T06:45:20Z

src/services/refactors/annotateWithTypeFromJSDoc.ts

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


Consider a never assertion

Nice. I didn't know we had Debug.assertNever.

DanielRosenwasser · 2017-10-13T06:47:27Z

src/services/refactors/annotateWithTypeFromJSDoc.ts

+
+        const node = getTokenAtPosition(context.file, context.startPosition, /*includeJsDocComment*/ false);
+        const decl = findAncestor(node, isDeclarationWithType);
+        if (decl && !decl.type) {


Can you use early bailouts to reduce nesting here?

DanielRosenwasser · 2017-10-13T06:48:15Z

src/services/refactors/annotateWithTypeFromJSDoc.ts

+        const node = getTokenAtPosition(context.file, context.startPosition, /*includeJsDocComment*/ false);
+        const decl = findAncestor(node, isDeclarationWithType);
+        if (decl && !decl.type) {
+            const type = getJSDocType(decl);


jsDocType

DanielRosenwasser · 2017-10-13T06:49:31Z

src/services/refactors/annotateWithTypeFromJSDoc.ts

+        if (decl && !decl.type) {
+            const type = getJSDocType(decl);
+            const isFunctionWithJSDoc = isFunctionLikeDeclaration(decl) && (getJSDocReturnType(decl) || decl.parameters.some(p => !!getJSDocType(p)));
+            const annotate = (isFunctionWithJSDoc || type && decl.kind === SyntaxKind.Parameter) ? annotateFunctionFromJSDoc :


refactoringType

DanielRosenwasser · 2017-10-13T06:50:25Z

src/services/refactors/annotateWithTypeFromJSDoc.ts

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


return Debug.fail(...)?

cyrilletuzi · 2017-10-18T08:52:54Z

Trying this with typescript@next, and with this simple code :

/**
 * @param {boolean} option 
 */
function test(option) {}

option is still indicated as any, and then raising an error with noImplicitAny. Is this normal, or did I misunderstood this feature ?

sandersn · 2017-10-18T16:50:28Z

@cyrilletuzi It sounds like you are thinking of using JSDoc as type annotation. This works in JS files only, so if you copy your example there, you should see option get the right type.

This PR give you a refactoring to move the JSDoc down into type annotation position in TS files. But JSDoc still isn't checked — it's just copied to type annotation position, which is then checked. If the refactoring isn't showing up in VS Code, you may need to add this to your config:

    "typescript.tsdk": "/home/YOURNAME/ts/built/local",
    "typescript.tsdk_version": "2.5.0",

Checking JSDoc types inside TS files is tricky to do well: if there are duplicates, the binder shouldn't use the JSDoc, but this means the binder has to detect duplicates before deciding which annotations to bind.
Checking JSDoc types inside TS is currently not part of the upgrade workflow we envision -- we think people will either (1) import their JS as-is and use it alongside newly-written TS files or (2) Convert existing JS to TS, which means converting JSDoc to type annotations.

sandersn added 2 commits September 25, 2017 08:56

Test:refactor JSDoc types to Typescript types

8996d11

msftclas added the cla-not-required label Sep 25, 2017

sandersn requested review from RyanCavanaugh, mhegazy and weswigham September 25, 2017 16:06

msftgits removed the cla-not-required label Sep 26, 2017

sandersn added 3 commits September 26, 2017 08:58

Change refactoring name and description

13b37a4

Move filenames to match refactoring rename

96b8093

Transform jsdoc types in the refactor, not emitter

fc933d7

The emitter now understands JSDoc types but emits them in the original format.

weswigham reviewed Sep 26, 2017

View reviewed changes

Merge branch 'master' into refactor-jsdoc-types-to-typescript

724a813

sandersn added 5 commits September 28, 2017 11:40

Correctly transform jsdoc parameter types

d797b4a

And give a better name for rest params

Merge branch 'master' into refactor-jsdoc-types-to-typescript

6831e65

Convert all JSDoc parameters and return types of functions

4930cad

Add and update jsdoc annotation refactoring tests

1a1c1f9

Merge branch 'master' into refactor-jsdoc-types-to-typescript

d7424b0

Merge branch 'master' into refactor-jsdoc-types-to-typescript

260d37e

mhegazy reviewed Oct 12, 2017

View reviewed changes

sandersn added 2 commits October 12, 2017 11:40

Convert @template tag to type parameters in refactor

123347d

Test refactor of JSDoc @template tag

b440d75

sandersn mentioned this pull request Oct 12, 2017

Introduce and consume suppressLeadingAndTrailingTrivia #19135

Merged

sandersn added 2 commits October 12, 2017 14:18

Merge branch 'master' into refactor-jsdoc-types-to-typescript

c2c18a8

Fix duplicated JSDoc comments

f35764d

Incorporate suppressLeadingAndTrailingTrivia just added by @amcasey.

mhegazy reviewed Oct 12, 2017

View reviewed changes

mhegazy approved these changes Oct 13, 2017

View reviewed changes

DanielRosenwasser approved these changes Oct 13, 2017

View reviewed changes

sandersn added 3 commits October 13, 2017 09:38

JSDoc->type refactor:Renames+improve never handling

c83daa6

return more Debug.fails instead of undefined.

84e3507

Fix spacing lint

4cf06bb

sandersn merged commit 22769d9 into master Oct 13, 2017

sandersn deleted the refactor-jsdoc-types-to-typescript branch October 13, 2017 17:21

cyrilletuzi mentioned this pull request Oct 18, 2017

Feature: migration tool to convert a full project from JSDoc to TypeScript strict #19285

Closed

ghost mentioned this pull request Mar 5, 2018

Convert annotateWithTypeFromJSDoc refactor to a codefix #22336

Merged

microsoft locked and limited conversation to collaborators Jun 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor jsdoc types to typescript #18747

Refactor jsdoc types to typescript #18747

sandersn commented Sep 25, 2017 •

edited

Loading

DanielRosenwasser commented Sep 25, 2017 •

edited by sandersn

Loading

sandersn commented Sep 25, 2017

DanielRosenwasser commented Sep 26, 2017 •

edited

Loading

weswigham Sep 26, 2017

weswigham Sep 26, 2017

sandersn Sep 28, 2017

weswigham Sep 26, 2017

DanielRosenwasser Sep 28, 2017 •

edited

Loading

sandersn Sep 28, 2017

weswigham Sep 28, 2017

sandersn commented Sep 26, 2017

DanielRosenwasser commented Sep 28, 2017

sandersn commented Sep 28, 2017

sandersn commented Oct 11, 2017

mhegazy Oct 12, 2017

sandersn Oct 12, 2017

mhegazy Oct 12, 2017

mhegazy Oct 12, 2017

weswigham Oct 12, 2017

sandersn Oct 12, 2017

sandersn commented Oct 12, 2017

mhegazy Oct 12, 2017 •

edited

Loading

DanielRosenwasser left a comment

DanielRosenwasser Oct 13, 2017

sandersn Oct 13, 2017

DanielRosenwasser Oct 13, 2017

sandersn Oct 13, 2017

DanielRosenwasser Oct 13, 2017

DanielRosenwasser Oct 13, 2017

DanielRosenwasser Oct 13, 2017

DanielRosenwasser Oct 13, 2017

cyrilletuzi commented Oct 18, 2017

sandersn commented Oct 18, 2017

Refactor jsdoc types to typescript #18747

Refactor jsdoc types to typescript #18747

Conversation

sandersn commented Sep 25, 2017 • edited Loading

DanielRosenwasser commented Sep 25, 2017 • edited by sandersn Loading

sandersn commented Sep 25, 2017

DanielRosenwasser commented Sep 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielRosenwasser Sep 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn commented Sep 26, 2017

DanielRosenwasser commented Sep 28, 2017

sandersn commented Sep 28, 2017

sandersn commented Oct 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn commented Oct 12, 2017

mhegazy Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

DanielRosenwasser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyrilletuzi commented Oct 18, 2017

sandersn commented Oct 18, 2017

sandersn commented Sep 25, 2017 •

edited

Loading

DanielRosenwasser commented Sep 25, 2017 •

edited by sandersn

Loading

DanielRosenwasser commented Sep 26, 2017 •

edited

Loading

DanielRosenwasser Sep 28, 2017 •

edited

Loading

mhegazy Oct 12, 2017 •

edited

Loading