Skip typedef in services getJSDocTags

[sandersn]

1. Skip @typedef and @callback tags when generating quickinfo for a declaration. They are unattached tags that happen to be attached to a particular node because of the way the compiler works.
2. Skip comment text from jsdocs that contain only @typedef and @callback tags. Again, these are attached because of an implementation detail.

For example:

/**
 * This is skipped in f's quickinfo
 * @typedef {number} N - and this tag is skipped completely
 */
function f() { }
/**
 * This is NOT skipped because of the param tag
 * @typedef {string} S - but this still is
 * @param {S} x
 */ 
function g(x) { }

The idea is that the comment text of a jsdoc that contains other tags probably describes both tags.

This is big-ish change in a poorly tested area of services (though with fourslash baselines, it's getting better!). I don't think this should go into 4.3.

Fixes #43534

[sandersn]

@mjbvz You might have an opinion about this as well.

[sandersn]

From editor meeting:

1. People thought it was a good idea overall.
2. Might make the heuristic stricter -- require a newline to skip a whole jsdoc, and maybe include the typedef in a jsdoc that has other tags. Need to look at existing usage, especially for the former.
3. Need to correctly handle @property and @param inside @typedef and @callback respectively. (The parse tree might make this work already, but it needs a test.)

[andrewbranch]

 /**
  * This is NOT skipped because of the param tag
  * @typedef {string} S - but this still is
  * @param {S} x
  */ 
 function g(x) { }

It seems fairly useful to preserve this typedef in the quick info for g, since otherwise it may be hard to know what S is. If this “local alias” pattern is the majority of typedefs mixed into comments with other tags, I would say it’s fine to preserve them, and try to instead filter out whole doc comments that seem to be purely typedef-like.

The problem is, it may be hard to reason about what doc comments are “purely typedef-like” from their tags:

/**
 * @deprecated
 * @see Some newer type that you should use instead
 * @author Andrew Branch
 * @typedef {Object} Foo
 * @property {string} email
 * @property {number} whatever
 */

function foo() {}

Two alternative strategies for deciding what comments to remove come to mind:

1. For a given doc comment attached to a node, if that doc comment includes @type, @param, or @returns, include everything in that doc comment in the quick info, because clearly those tags are supposed to apply to some declaration/expression, not just stand alone as a typedef declaration. Any embedded typedefs are probably contextually relevant to that node, or else the author would have written them somewhere else. If the doc comment doesn’t contain any of those declaration/expression-affecting tags, but does contain a @typedef, treat the entire doc comment as a standalone declaration and don’t show any of it in the quick info for the following node.
2. For a doc comment that contains a @typedef and some other stuff, use a blank line between the end of the comment and the following node to indicate that none of that doc comment should be included in the node’s quick info.

[jessetrinity]

andrewbranch's point 2. makes the most sense to me.

[thorn0]

I'd rather vote for the strategy 1 and avoid significant blank lines.

[sandersn]

I looked at my usual corpora [1] to see which strategy is best. I started by considering three options:

1. Exclude the whole comment for comments containing a @typedef/@callback and no @param or @return tags. (Strategy 1 proposed by @andrewbranch)
2. Exclude the whole comment for comments containing only @typedef, @callback and perhaps a small set of associated tags.
3. Exclude the whole comment for comments containing a (OR only) @typedef/@callback if a newline separated the comment from the next element in the source. (Strategy 2 proposed by @andrewbranch)

I found all comments containing @typedef and dumped the list of tags from each comment.

.ts files

.ts files had almost 300 @typedef, about 250 single @typedefs. The remaining typedefs were in 3 projects:

• openfin and google-gax used @summary, @desc and @example to explain the typedef further.
• protobufjs used a @callback-like variant of @typedef that Typescript doesn't parse:

/**
 * @typedef {function} Typename
 * @param {number} a
 * @param {string} b
 * @return {string}
 * @this {?object} Please don't pass `this` it is the worst.
 */

Also, notably, in protobufjs, the @typedefs are often immediately followed with a Typescript declaration for the same thing. I think the .d.ts files were generated from Closure @typedefs.

.js files

.js files had more than 8000 @typedef, 7500 single @typedefs. The remaining typedefs fell into 4 categories:

• embellishments for documentation or structure like @summary, @description, @category, @public, @module, @global, @static, @interface, @example, @throws. There was a lot of variety here.
• 'local' typedefs that were immediately used with a @param and @return tag (the typedefs are not actually local to the declaration)
• @callback-variants like the protobuf example above, which also used @param and @return tags. (and @this, once or twice)

6 projects use the unsupported callback-like typedefs compared to 3 projects that use local typedefs.

Additionally, I observed plenty of unassociated @typedef comments that were not separated by a newline from the next element, though the median next element was another comment.

Analysis

The best rule for excluding typedefs in quickinfo is (1). Except for local typedefs, all typedef-containing comments that I saw were exclusively about that typedef. And, although both local typedefs and callback-like typedefs are rare, it's better to show a few callback-like typedefs than to hide local typedefs that were intentionally defined in-place. (2) won't work well because there it's too hard to capture the set of associated tags -- we would end up showing any unforeseen variants needlessly. And the data show that (3) is both unnecessary and ineffective.

In the future, if we decide to support callback-like typedefs, then (1) would stop incorrectly showing the callback-like typedefs. The code to implement this wouldn't be large, although I think there isn't much demand.

[1] A full install of Definitely Typed, using @definitelytyped/dtslint-runner to npm install every package inside types/, then find . -iname '*.ts'. The same for the user tests in the TS repo.

[andrewbranch]

I like it 🐙

[andrewbranch]

Worth an explanatory comment? This looks puzzling without the context of the conversation we had in this PR.