Revise logic for gathering JSDoc @template type parameters

[ahejlsberg]

Fixes the first issue described in #4037 and documents the differences from the second issue in CHANGES.md.

Fixes #4037.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[DanielRosenwasser]

It seems like the logic has changed from "get every @template type parameter above the tag of interest until the next overload" to "get every @template type parameter in the entire JSDoc block, unless I need to defer to someone else".

Is that accurate? I think that changes the behavior a bit in ways we probably need to discuss.

For example, if you have something like the following:

// @ts-check

/**
 * @param {unknown} x
 * @param {unknown} y
 * @return {boolean}
 * 
 * @template {string} T
 * @overload
 * @param {T} x
 * @param {T} y
 * @return {boolean}
 *
 * @template {number} U
 * @overload
 * @param {U} x
 * @param {U} y
 * @return {boolean}
 */
export function fff(x, y) {
  return x === y;
}

Then with this PR, that will boil down to something like this:

export function fff<T extends string, U extends number>(x: T, y: T): boolean;
export function fff<T extends string, U extends number>(x: U, y: U): boolean;
export function fff<T extends string, U extends number>(x: unknown, y: unknown): boolean {
  return x === y;
}

whereas before you'd have

export function fff<T extends string>(x: T, y: T): boolean;
export function fff<U extends number>(x: U, y: U): boolean;
export function fff(x: unknown, y: unknown): boolean {
  return x === y;
}

Note that if you write the above in a JS file, you'll get a suggestion diagnostic in the editor:

Subtly, this means type parameters from other overloads are now allowed to be referenced in any other signature (including portions of the implementation signature), whereas you could not reference them before.

// @ts-check

/**
 * @template {string} T
 * @overload
 * @param {T} x
 * @param {T} y
 * @return {boolean}
 *
 * @template {number} U
 * @overload
 * @param {T} x <- used to be an error to reference 'T', now is allowed.
 * @param {U} y
 * @return {boolean}
 * 
 * @param {T} x <- used to be an error to reference 'T', now is allowed.
 * @param {T} y <- used to be an error to reference 'T', now is allowed.
 * @return {boolean}
 */
export function fff(x, y) {
  return x === y;
}

fff

Another new behavior this PR introduces is that if the @template tags are repeated (e.g. two @typedef ... Ts, but with different constraints)...

// @ts-check

/**
 * @param {unknown} x
 * @param {unknown} y
 * @return {boolean}
 * 
 * @template {string} T
 * @overload
 * @param {T} x
 * @param {T} y
 * @return {boolean}
 *
 * @template {number} T
 * @overload
 * @param {T} x
 * @param {T} y
 * @return {boolean}
 */
export function ggg(x, y) {
  return x === y;
}

ggg

then you'll now get a duplicate declaration error, whereas you would not have before.

[DanielRosenwasser]

Suggested change

	if !typedefOrCallback && (ast.IsJSDocTypedefTag(tag) || ast.IsJSDocCallbackTag(tag)) {
	// In some instances, JSDoc may contain multiple kinds of tags
	// that permit type parameters through a `@template` tag.
	// For example, a JSDoc comment may contain a `@typedef`
	// and an `@overload`. In such cases, we assume that type parameters
	// solely belong to `@typedef` and `@callback` tags.
	if !typedefOrCallback && (ast.IsJSDocTypedefTag(tag) || ast.IsJSDocCallbackTag(tag)) {

[DanielRosenwasser]

Why didn't this happen before? Shouldn't we have failed to issue an error back when we were stopping early on the input node?

[ahejlsberg]

The old error handling was bizarre. It would stop parsing the @typedef children upon hitting an @template. The @template and the following @property tags would then apply to nothing.

[ahejlsberg]

It seems like the logic has changed from "get every @template type parameter above the tag of interest until the next overload" to "get every @template type parameter in the entire JSDoc block..."

Yes, that's how it works in Strada, and this was actually discussed in here in the original PR. I'm not sure why Corsa wasn't implemented that way to begin with.

In order to have separate type parameters, you need to write separate JSDoc comments, one after the other. For example:

/**
 * First overload
 * @template T
 * @overload
 * @param {T[]} items
 * @returns {T}
 */
/**
 * Second overload
 * @template {string | number} K
 * @overload
 * @param {K} items
 * @returns {void}
 */
/**
 * Implementation signature.
 * @param {any} items
 * @returns {any}
 */
function funky(obj) {
    // ...
}