GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — inferFromUsage codefix now emits JSDoc in JS files by sandersn · Pull Request #27610 · microsoft/TypeScript

Conversation

Nathan Shively-Sanders (sandersn) commented Oct 8, 2018

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

SlurpTheo reacted with thumbs up emoji

All reactions

👍 1 reaction

Nathan Shively-Sanders (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

Nathan Shively-Sanders (sandersn) requested review from a user and Andrew Casey (amcasey)

October 8, 2018 16:13

Deleted user (ghost) reviewed

Oct 8, 2018

View reviewed changes

Comment thread

src/services/codefixes/inferFromUsage.ts

		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

Deleted user (ghost) Oct 8, 2018 •

edited by ghost

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Nathan Shively-Sanders (sandersn) Oct 8, 2018

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/services/codefixes/inferFromUsage.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

src/services/codefixes/inferFromUsage.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

src/services/codefixes/inferFromUsage.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

src/services/codefixes/inferFromUsage.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

src/services/textChanges.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

tests/cases/fourslash/codeFixInferFromUsageMember2JS.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

tests/cases/fourslash/codeFixInferFromUsagePropertyAccessJS.ts


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

Deleted user (ghost) Oct 8, 2018

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Nathan Shively-Sanders (sandersn) Oct 8, 2018

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Deleted user (ghost) Oct 8, 2018

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

verify.getSuggestionDiagnostics should allow you to test that.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Nathan Shively-Sanders (sandersn) Oct 8, 2018

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Nathan Shively-Sanders (sandersn) Oct 8, 2018

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Filed #27624

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

tests/cases/fourslash/codeFixInferFromUsageRestParam2JS.ts


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

Deleted user (ghost) Oct 8, 2018

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Nathan Shively-Sanders (sandersn) Oct 8, 2018

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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
}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

tests/cases/fourslash/codeFixInferFromUsageSetterWithInaccessibleTypeJS.ts Outdated

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

Deleted user (ghost) Oct 8, 2018

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Nathan Shively-Sanders (sandersn) Oct 8, 2018

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Deleted user (ghost) commented Oct 8, 2018

Copy link

Copy Markdown

Fixes #20113

sandersn reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          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

Deleted user (ghost) reviewed

Oct 9, 2018

View reviewed changes

Comment thread

src/services/textChanges.ts Outdated

		}

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

Deleted user (ghost) Oct 9, 2018

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Think this could also use getNonformattedText.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Nathan Shively-Sanders (sandersn) mentioned this pull request

Oct 9, 2018

Cannot use generics in JSDoc object signature #27215

Closed


          Switch another call to getNonformattedText

e3cf787

Nathan Shively-Sanders (sandersn) added 2 commits

October 9, 2018 10:08


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

bc32d3c


          Update baseline missed after merge

Deleted user (ghost) approved these changes

Oct 9, 2018

View reviewed changes

Nathan Shively-Sanders (sandersn) merged commit 88d3c6f into master

Oct 9, 2018

Nathan Shively-Sanders (sandersn) deleted the js/infer-from-usage branch

October 9, 2018 17:38

Andrew Casey (amcasey) reviewed

Oct 9, 2018

View reviewed changes

Comment thread

src/services/textChanges.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

Andrew Casey (amcasey) reviewed

Oct 9, 2018

View reviewed changes

Comment thread

src/services/textChanges.ts


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

Andrew Casey (amcasey) Oct 9, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Deleted user (ghost) Oct 9, 2018

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Nathan Shively-Sanders (sandersn) Oct 9, 2018

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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;}}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Andrew Casey (amcasey) reviewed

Oct 9, 2018

View reviewed changes

Comment thread

src/services/textChanges.ts

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

Andrew Casey (amcasey) Oct 9, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Andrew Casey (amcasey) reviewed

Oct 9, 2018

View reviewed changes

Comment thread

src/services/textChanges.ts

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

Andrew Casey (amcasey) Oct 9, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Andrew Casey (amcasey) reviewed

Oct 9, 2018

View reviewed changes

Comment thread

src/services/textChanges.ts


		/** 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 } {

Andrew Casey (amcasey) Oct 9, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Why is this exported?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Andrew Casey (amcasey) reviewed

Oct 9, 2018

View reviewed changes

Comment thread

src/services/textChanges.ts

		this.insertNodeAt(sourceFile, endNode.end, type, { prefix: ": " });
		}

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

Andrew Casey (amcasey) Oct 9, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Andrew Casey (amcasey) reviewed

Oct 9, 2018

View reviewed changes

Comment thread

src/services/codefixes/inferFromUsage.ts

		changes.tryInsertJSDocParameters(sourceFile, result);
		}

		function getTypeNodeIfAccessible(type: Type, enclosingScope: Node, program: Program, host: LanguageServiceHost): TypeNode \| undefined {

Andrew Casey (amcasey) Oct 9, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

[](start = 0, length = 3)

This looks unintentional.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Andrew Casey (amcasey) reviewed

Oct 9, 2018

View reviewed changes

Comment thread

src/services/codefixes/inferFromUsage.ts

		return callContexts && declaration.parameters.map((parameter, parameterIndex) => {
		const types: Type[] = [];
		const isRest = isRestParameter(parameter);
		let isOptional = false;

Andrew Casey (amcasey) Oct 9, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Andrew Casey (amcasey) reviewed

Oct 9, 2018

View reviewed changes

Comment thread

src/services/codefixes/inferFromUsage.ts

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

Andrew Casey (amcasey) Oct 9, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Nathan Shively-Sanders (sandersn) mentioned this pull request

Oct 18, 2018

Infer from usage codefix should work in JS #20113

Closed

Microsoft (microsoft) locked as resolved and limited conversation to collaborators

Oct 21, 2025

TypeScript Bot (typescript-bot) assigned Nathan Shively-Sanders (sandersn)

Oct 21, 2025

Reviewers

2 more reviewers

Andrew Casey (amcasey)

amcasey left review comments

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

Nathan Shively-Sanders (sandersn)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

Nathan Shively-Sanders (sandersn) commented Oct 8, 2018

Uh oh!

Deleted user (ghost) Oct 8, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Deleted user (ghost) commented Oct 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Deleted user (ghost) Oct 8, 2018 •

edited by ghost

Loading

Uh oh!

There was an error while loading. Please reload this page.