inferFromUsage codefix now emits JSDoc in JS files #27610

sandersn · 2018-10-08T16:13:12Z

inferFromUsage now emits JSDoc. It emits @type by default, but @param for parameters and @return for get accessors. It does not emit @return type annotations because the codefix itself does not. (It probably should, but I'll address that in a followup PR.)

For example:

function f(x) {
}
f(1)

Gives the annotation

/** 
 * @param {number} x 
 */

Currently, only @param blocks are multi-line.

Note that I generate strings for JSDoc. I don't know enough about printing trivia to know whether it's worthwhile to generate a structure and then print it.

Probably everything else, but not as well.

It's still bad, but at least it's not wrong.

Also, remove redundant is(Set|Get)Accessor functions.

By surrounding the parameter name with brackets. It is super, super ugly right now.

1. Format multiple params nicely in a single-multiline block. 2. Generate only params that haven't already been documented. Existing documentation is not touched.

More to come, though. annotate is only called in annotateVariableDeclaration where we don't know whether we're in JS or not.

ghost · 2018-10-08T18:12:30Z

src/services/codefixes/inferFromUsage.ts

@@ -50,7 +47,7 @@ namespace ts.codefix {
    function getDiagnostic(errorCode: number, token: Node): DiagnosticMessage {
        switch (errorCode) {
            case Diagnostics.Parameter_0_implicitly_has_an_1_type.code:
-                return isSetAccessor(getContainingFunction(token)!) ? Diagnostics.Infer_type_of_0_from_usage : Diagnostics.Infer_parameter_types_from_usage; // TODO: GH#18217
+                return isSetAccessorDeclaration(getContainingFunction(token)!) ? Diagnostics.Infer_type_of_0_from_usage : Diagnostics.Infer_parameter_types_from_usage; // TODO: GH#18217


Why do we have both of these -- isSetAccessor and isSetAccessorDeclaration?

No reason, but removing one would change the public API, so I decided not do it in this PR.

src/services/codefixes/inferFromUsage.ts

src/services/textChanges.ts

tests/cases/fourslash/codeFixInferFromUsageMember2JS.ts

ghost · 2018-10-08T18:31:12Z

tests/cases/fourslash/codeFixInferFromUsagePropertyAccessJS.ts

+
+// @allowJs: true
+// @checkJs: true
+// @noImplicitAny: true


I think this shouldn't be necessary because we should make it a suggestion diagnostic anyway, if we don't already.

Either noImplicitAny isn't triggered as a suggestion (perhaps just for JS?), or the test is only seeing codefixes from errors. Any idea which? I don't know how to tell what gets triggered as a suggestion, do you?

verify.getSuggestionDiagnostics should allow you to test that.

There is one suggestion, but it's "'numeric' is declared but its value is never read.". I don't think noImplicitAny is leading to a infer-from-usage suggestion. It probably should someday, but I'd like to improve the precision and recall of it first.

Filed #27624

ghost · 2018-10-08T18:31:35Z

tests/cases/fourslash/codeFixInferFromUsageRestParam2JS.ts

+
+verify.fileAfterCodeFix(
+`/** @param {number} a */
+/**


Could you file this as a bug? We should combine comments, not add a new one.

I'll file it after merging the PR since the bug depends on being able to emit JSDoc in the first place.

I personally think the current approach is a good compromise and that we'll rarely have to generate partial JSDoc. The bigger lack is untyped JSDoc:

/** @param x Just a description, no type */ function f(x) { return x * x }

ghost · 2018-10-08T18:33:47Z

tests/cases/fourslash/codeFixInferFromUsageSetterWithInaccessibleTypeJS.ts

+    newFileContent:
+`export class C {
+    /**
+     * @param {Promise<typeof import("/a")>} val


I think this should be "./a" when we synthesize the import. Doesn't look so bad in this example but in a real example this might import from /home/myname/projects/typescript/someproject/src/components/a.

Wow, nice catch. I asked Wesley and he said that getTypeNodeIfAccessible needs to provide more members to the symbol tracker host (from Program and LanguageServiceHost). It sounds fairly simple, but it's not related to this change, so I'll put it in a separate PR.

ghost · 2018-10-08T18:40:28Z

Fixes #20113

ghost · 2018-10-09T00:42:11Z

src/services/textChanges.ts

@@ -365,6 +394,27 @@ namespace ts.textChanges {
            this.insertNodeAt(sourceFile, endNode.end, type, { prefix: ": " });
        }

+        public tryInsertJSDocType(sourceFile: SourceFile, node: Node, type: TypeNode): void {
+            const printed = createPrinter().printNode(EmitHint.Unspecified, type, sourceFile);


Think this could also use getNonformattedText.

src/services/textChanges.ts

amcasey · 2018-10-09T17:54:29Z

src/services/textChanges.ts

@@ -339,6 +345,12 @@ namespace ts.textChanges {
            this.insertText(sourceFile, token.getStart(sourceFile), text);
        }

+        public insertCommentThenNewline(sourceFile: SourceFile, character: number, position: number, commentText: string): void {
+            const token = getTouchingToken(sourceFile, position);
+            const text = "/**" + commentText + "*/" + this.newLineCharacter + repeatString(" ", character);


" " [](start = 91, length = 3)

Will the formatting done after the fix is applied convert this to tabs if that's what the user prefers?

What we do for other fixes is to copy exactly the indentation string that they were using -- if their preferred indentation is tab, space, space, tab, space, space, we should copy that. I'll make a new PR unifying our comment code since #27565 has its own separate jsdoc-adding helpers.

There are cases where we need to make to new indentation, though, as in the single-line test case that needs to annotate class C {m(x) {return x;}}

amcasey · 2018-10-09T17:57:49Z

src/services/textChanges.ts

+                    commentText += this.printJSDocParameter(indent, printed, declaration.name, isOptional);
+                }
+            }
+            commentText += repeatString(" ", indent + 1);


commentText += repeatString(" ", indent + 1); [](start = 12, length = 45)

This seems like it belongs in insertCommentThenNewline (which would then either need to check for newlines in the text or accept a flag indicating whether or not the */ should be indented).

amcasey · 2018-10-09T17:59:44Z

src/services/textChanges.ts

+            if (isOptionalParameter) {
+                printName = `[${printName}]`;
+            }
+            return repeatString(" ", indent) + ` * @param {${printed}} ${printName}\n`;


repeatString(" ", indent) [](start = 19, length = 25)

Having these scattered through the code seems fragile. What about passing an array of lines to insertCommentThenNewline and letting it handle breaks and indentation?

amcasey · 2018-10-09T18:01:11Z

src/services/textChanges.ts

@@ -806,7 +856,7 @@ namespace ts.textChanges {
        }

        /** Note: output node may be mutated input node. */
-        function getNonformattedText(node: Node, sourceFile: SourceFile | undefined, newLineCharacter: string): { text: string, node: Node } {
+        export function getNonformattedText(node: Node, sourceFile: SourceFile | undefined, newLineCharacter: string): { text: string, node: Node } {


export [](start = 8, length = 6)

Why is this exported?

amcasey · 2018-10-09T18:05:08Z

src/services/textChanges.ts

@@ -365,6 +394,27 @@ namespace ts.textChanges {
            this.insertNodeAt(sourceFile, endNode.end, type, { prefix: ": " });
        }

+        public tryInsertJSDocType(sourceFile: SourceFile, node: Node, type: TypeNode): void {


tryInsertJSDocType [](start = 15, length = 18)

I feel like I'm missing something obvious, but why "try"?

amcasey · 2018-10-09T18:06:11Z

src/services/codefixes/inferFromUsage.ts

+        changes.tryInsertJSDocParameters(sourceFile, result);
+    }
+
+   function getTypeNodeIfAccessible(type: Type, enclosingScope: Node, program: Program, host: LanguageServiceHost): TypeNode | undefined {


[](start = 0, length = 3)

This looks unintentional.

amcasey · 2018-10-09T18:09:16Z

src/services/codefixes/inferFromUsage.ts

@@ -274,8 +312,10 @@ namespace ts.codefix {
            return callContexts && declaration.parameters.map((parameter, parameterIndex) => {
                const types: Type[] = [];
                const isRest = isRestParameter(parameter);
+                let isOptional = false;


isOptional [](start = 20, length = 10)

I don't understand this flag. I'll drop by.

amcasey · 2018-10-09T18:27:56Z

src/services/codefixes/inferFromUsage.ts

                for (const callContext of callContexts) {
                    if (callContext.argumentTypes.length <= parameterIndex) {
+                        isOptional = isInJSFile(declaration);


isInJSFile [](start = 37, length = 10)

I assume isInJSFile is very cheap but I would habitually have either pulled it out of the loop or checked isOptional ||.

sandersn added 19 commits September 28, 2018 15:22

Now adding @type to variable declarations, at least

1bc0cd1

Probably everything else, but not as well.

Improve @param output and add test

e34bf54

It's still bad, but at least it's not wrong.

Add some js/inferFromUsage tests and fixes

0f4a800

Also, remove redundant is(Set|Get)Accessor functions.

Fix @typedef refactor

be3577c

Emit JSDoc optional parameters

bf5529b

By surrounding the parameter name with brackets. It is super, super ugly right now.

Get rest of existing tests working

4ee4d69

Merge branch 'master' into js/infer-from-usage

9ae4a99

Merge branch 'master' into js/infer-from-usage

09ae612

Merge branch 'master' into js/infer-from-usage

e99220f

Correct location of comments

ae062b2

Handle @param blocks

69dec3f

1. Format multiple params nicely in a single-multiline block. 2. Generate only params that haven't already been documented. Existing documentation is not touched.

Re-add isGet/SetAccessor -- it is part of the API

b469d9f

Move isSet/GetAccessor back to the original location

56bcf3f

Merge branch 'master' into js/infer-from-usage

84b08c0

Oh no I missed a newline and a space

a3aae7b

Switch to an object type

d22961a

A lot of cleanup

3cc99ea

More to come, though. annotate is only called in annotateVariableDeclaration where we don't know whether we're in JS or not.

Move and delegate to annotateJSDocParameters

31db1ce

Merge branch 'master' into js/infer-from-usage

6b0be8b

sandersn requested review from a user and amcasey October 8, 2018 16:13

ghost reviewed Oct 8, 2018

View reviewed changes

Address PR comments

edcb30d

This was referenced Oct 8, 2018

infer-from-usage codefix should be a suggestion #27624

Closed

Infer from usage better import types #27626

Merged

Lint: newline problems!!!!

d129e32

ghost reviewed Oct 9, 2018

View reviewed changes

sandersn mentioned this pull request Oct 9, 2018

Cannot use generics in JSDoc object signature #27215

Closed

Switch another call to getNonformattedText

e3cf787

sandersn added 2 commits October 9, 2018 10:08

Merge branch 'master' into js/infer-from-usage

bc32d3c

Update baseline missed after merge

5066880

ghost approved these changes Oct 9, 2018

View reviewed changes

sandersn merged commit 88d3c6f into master Oct 9, 2018

sandersn deleted the js/infer-from-usage branch October 9, 2018 17:38

amcasey reviewed Oct 9, 2018

View reviewed changes

src/services/textChanges.ts Show resolved Hide resolved

amcasey reviewed Oct 9, 2018

View reviewed changes

sandersn mentioned this pull request Oct 18, 2018

Infer from usage codefix should work in JS #20113

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

inferFromUsage codefix now emits JSDoc in JS files #27610

inferFromUsage codefix now emits JSDoc in JS files #27610

sandersn commented Oct 8, 2018

ghost Oct 8, 2018 •

edited by ghost

Loading

sandersn Oct 8, 2018

ghost Oct 8, 2018

sandersn Oct 8, 2018

ghost Oct 8, 2018

sandersn Oct 8, 2018

sandersn Oct 8, 2018

ghost Oct 8, 2018

sandersn Oct 8, 2018

ghost Oct 8, 2018

sandersn Oct 8, 2018

ghost commented Oct 8, 2018

ghost Oct 9, 2018

amcasey Oct 9, 2018

ghost Oct 9, 2018

sandersn Oct 9, 2018

amcasey Oct 9, 2018

amcasey Oct 9, 2018

amcasey Oct 9, 2018

amcasey Oct 9, 2018

amcasey Oct 9, 2018

amcasey Oct 9, 2018

amcasey Oct 9, 2018

inferFromUsage codefix now emits JSDoc in JS files #27610

inferFromUsage codefix now emits JSDoc in JS files #27610

Conversation

sandersn commented Oct 8, 2018

ghost Oct 8, 2018 • edited by ghost 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

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

ghost commented Oct 8, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Oct 8, 2018 •

edited by ghost

Loading