JSDoc comments separated by white-space should not be combined

[rjgotten]

TypeScript Version: (Whatever is current with VSCode 1.35.1)

Search Terms: JSDoc combining comments

Code:

/**
 * @typedef {Object} Foo
 */

/**
 * @typedef {Object} Bar
 */

/**
 * Description goes here
 */
export default class Baz {
}

Expected behavior:
Documentation for Baz does not include information from JSDoc islets that are not connected to the class definition.

Actual behavior:
Documentation for Baz includes information from earlier JSDoc islets that are not connected to the class definition.

Related Issues:
#15106
#19537
#41811 (vscode)
(And possibly a lot of others)

Possible workarounds:

1. Place comments that are not attached to code artifacts, such as @typedef, at the bottom of the file.
2. Place those comments into separate files and use {import("")} typings.
3. Insert dummy code lines inbetween those comments and the code such as void 0; to force the TypeScript compiler to treat them in isolation.

---

I understand that others have closed this type of issue before as a won't fix, for instance because:

@mhegazy
this is how the comment system is built. changing that would not be simple,

or:

@mjbvz
This is a limitation of how our JS Doc support is designed. JSDoc comments are merged into the next declaration

I am re-filing this issue again, but with reason to doubt the above.

The decision to treat this "as designed" makes no sense. Clearly it is broken - as in delivering a broken user experience for JSDoc users. And one which runs counter to practically any other sane JSDoc or JSDoc-like parser on market. Microsoft should pave the cowpath here and follow existing convention, not drop back into old misbehavior and towing their own line.

I doubt that - as prior quoted - this "would not be simple" in the sense of requiring large amounts of code to be adapted. Or that there is a hard "limitation" somehow at play in the parser.

The core problem here seems that when parsing JSDoc comments attached to nodes, the entire range of leading comment blocks is processed, using - eventually - the scanner.ts file's getLeadingCommentRanges function, which calls into the reduceEachLeadingCommentRange function.

I.e.

TypeScript/src/compiler/parser.ts

Lines 866 to 871 in 917cd6c

	function addJSDocComment<T extends HasJSDoc>(node: T): T {
	Debug.assert(!node.jsDoc); // Should only be called once per node
	const jsDoc = mapDefined(getJSDocCommentRanges(node, sourceFile.text), comment => JSDocParser.parseJSDocComment(node, comment.pos, comment.end - comment.pos));
	if (jsDoc.length) node.jsDoc = jsDoc;
	return node;
	}

TypeScript/src/compiler/utilities.ts

Lines 997 to 1010 in 1cbace6

	export function getJSDocCommentRanges(node: Node, text: string) {
	const commentRanges = (node.kind === SyntaxKind.Parameter ||
	node.kind === SyntaxKind.TypeParameter ||
	node.kind === SyntaxKind.FunctionExpression ||
	node.kind === SyntaxKind.ArrowFunction ||
	node.kind === SyntaxKind.ParenthesizedExpression) ?
	concatenate(getTrailingCommentRanges(text, node.pos), getLeadingCommentRanges(text, node.pos)) :
	getLeadingCommentRanges(text, node.pos);
	// True if the comment starts with '/**' but not if it is '/**/'
	return filter(commentRanges, comment =>
	text.charCodeAt(comment.pos + 1) === CharacterCodes.asterisk &&
	text.charCodeAt(comment.pos + 2) === CharacterCodes.asterisk &&
	text.charCodeAt(comment.pos + 3) !== CharacterCodes.slash);
	}

TypeScript/src/compiler/scanner.ts

Lines 794 to 796 in c3a9429

	export function getLeadingCommentRanges(text: string, pos: number): CommentRange[] | undefined {
	return reduceEachLeadingCommentRange(text, pos, appendCommentRange, /*state*/ undefined, /*initial*/ undefined);
	}

The reduceEachLeadingCommentRange combines all preceding comments, also those separated from the code by lines of white-space. A simple fix for this JSDoc parsing bug would be to not do this and to instead ignore comments that are separated from the declaring node by one or more lines of whitespace.

This requires either:

1. a separate implementation which traverses bottom-to-top through the comment blocks and halts at the first line of white-space; or
2. a switch/flag on the existing function to trigger aforementioned alternate behavior.

Even a more simple and less performant implementation which simply discards disconnected comments after they've already been parsed would atleast be functional. (And have no worse performance than current.)

Either way, it seems to not quite be rocket-science. So why this problem has been marked as "won't fix" / "as designed" before is beyond me.

If there's a particularly hard component here to fix that I am missing, then please also consider this bug report a request to illustrate where that would be, as this was not disclosed when any of the previous issues were filed and - wrongfully, imho - closed.

Thank you.

[RyanCavanaugh]

The argumentative presentation of this isn't helpful, FWIW

[RyanCavanaugh]

@sandersn this seems entirely reasonable - can you see what the impact would be?

[RyanCavanaugh]

You could say the argumentative presentation isn't helpful; but on the other hand it got someone to look and hopefully consider, rather than just closing this issue again.

In spite of it, not because of it.

[sandersn]

The biggest question is backward compatibility: if lots of people depend on Typescript's existing behaviour, we can't really change it. I surveyed almost 9000 files with almost 11 million nodes (1.5 million of which were declarations). I used four projects from our user test suite and their dependencies in node_modules, which meant that most of the files were .js files.

Here's the breakdown:
11,000,000 nodes
1,500,000 declarations
75,000 single jsdocs
833 double jsdocs
3, 4 and 5 jsdocs in a row: 85, 20, 2.

I then dumped the tag kinds of the jsdocs that were not immediately in front of their node. There was a lot of @license, @author, etc, but I was mostly interested in @param and @return, which indicate that the jsdoc is intended to be attached to a declaration.

@param in 2-in-a-row: 26 (of 833)
@return in 2-in-a-row: 6 (of 833)
@param in 3-in-a-row: 4 (of 85)

Note that we are using @param and @return as a proxy for attachment to a subsequent declaration. It's still possible (but not likely) that a jsdoc could have only text in it and still be intended to be attached.

So, to summarise: ignoring all but the last jsdoc would break 5 out of 10,000 uses of jsdoc. Seems like a good idea.

My biggest technical question is where to implement the dropping so that now-standalone jsdocs still get bound so that @typedef continues to work. It might be better to drop them right before producing combined comments in the language service, for example. I think that our test suite has enough coverage to point out failures here.

[sandersn]

Update: I looked at the anomalies and they are of 3 types, none of which we care about.

1. Deleted functions leaving behind orphaned jsdoc. (and a variant, typescript-emitted code that shuffles definitions away from their comments incorrectly.)
2. @typedef syntax is so complicated, you wouldn't believe how many ways there are to get it wrong.
3. Multiple jsdocs on a single factory function that creates other functions. There is no way any machine is going to get this one right.

[rjgotten]

@sandersn

That was crazy fast. 🥇

Thank you so much for making this problem - hopefully (fingers crossed it can ride the rails ofcourse...) - a thing of the past soon.

---

@RyanCavanaugh

In the interest of not wanting to burden this issue with bystanders continuing to play morality police, I've amended the issue description to take the edge off of the argumentative form, and I've removed atleast my end of the side-tracked discussion regarding its original tone.

Hopefully people will give it a rest now.