Add callback tag, fix jsdoc type parameter resolution #1187

sandersn · 2025-06-13T21:35:27Z

This PR adds support for the @callback tag. More importantly, it fixes type parameter resolution and number of other bugs in @typedef and @template. It does some of this by simplifying the rules for how @template attaches its type parameter to its host.

Specifically, all @template tags would previously apply to all @overloads, @typedef, function declarations, etc, that they shared a jsdoc with. They didn't even have to come before their usage:

/**
 * @template T
 * @typedef {T} Alias
 * @template U
 * @overload
 * @param {[T,U]} tu
 * @overload
 * @param {number} n
 * @return {string}
 * @param {any} tun
 * @return {string}
 */
function f(tun) {}

// is equivalent to TS
type Alias<T,U> = T
function f<T,U>(tu: [T,U])
function f<T,U>(n: number): string
function f<T,U>(tun: any): string {}

Not only is this nonsensical, it's really hard to implement, and I don't think people write their types in one single jsdoc like this in the first place. Corsa, as implemented in this PR, requires @template to precede its usage and for it to apply in only one place:

type Alias<T> = T
function f<U>(tu: [T,U])
function f(n: number): string
function f(tun: any): string

This will break any intentional sharing, but those will have to be rewritten. Sharing reparsed nodes to more than one other place in the tree would require Host to be a slice, plus some way to choose the correct host when resolving names.

Speaking of Host, JSDocTemplateTag now has a host, and the grossly-named GetEffectiveTypeParent now has two cases. It's also used consistently in the 5 places it makes sense. I do think we're nearing the total number of places it'll be needed, but there are 512 uses of Node.Parent in the checker alone, so I could be wrong.

Despite the improvements in typing, there is little code churn outside reparser.go. Inside, I refactored callback/typedef/overload quite a bit to share code, so there are almost 200 lines changed. Best reviewed ignoring whitespace, as always.

Direct from Copilot agent Claude, needs lots of cleanup/expansion.

Copilot

Pull Request Overview

This PR enhances JSDoc support by adding @callback tag handling, tightening how @template tags attach to their specific hosts, and simplifying type parameter resolution across @typedef/@overload/callback declarations.

Added support for parsing and emitting @callback tags as type aliases.
Refactored gatherTypeParameters to only apply templates preceding their host tag.
Introduced a Host field on JSDocTemplateTag and updated parent traversal (GetEffectiveTypeParent).

Reviewed Changes

Copilot reviewed 105 out of 105 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
internal/printer/printer.go	Added nil-check in `emitReturnType` to guard against missing nodes.
internal/parser/reparser.go	Major reparser overhaul for `@typedef`, `@callback`, and `@overload` with host assignment.
internal/checker/checker.go	Updated parent traversal to use `GetEffectiveTypeParent`.
internal/binder/nameresolver.go	Integrated effective parent logic in name-resolution loop.
internal/ast/utilities.go	Extended `GetEffectiveTypeParent` to handle `@template` hosts.
internal/ast/ast.go	Added `Host` to `JSDocTemplateTag` and updated `JSDocSignature`.
internal/api/encoder/encoder.go	Adjusted encoding mask to reflect renamed `TypeParameters` field.

Comments suppressed due to low confidence (1)

internal/parser/reparser.go:82

[nitpick] The indentation of typeAlias.Loc, typeAlias.Flags, and the append call is inconsistent with the surrounding block; aligning these lines will improve readability.

typeAlias.Loc = tag.Loc

Copilot · 2025-06-13T21:37:03Z

internal/parser/reparser.go

 		typeAlias.Loc = tag.Loc
 		typeAlias.Flags = p.contextFlags | ast.NodeFlagsReparsed
 		p.reparseList = append(p.reparseList, typeAlias)
 	case ast.KindJSDocImportTag:
 		importTag := tag.AsJSDocImportTag()
-		importClause := importTag.ImportClause.Clone(&p.factory)
+		importClause := importTag.ImportClause


Cloning of importTag.ImportClause was removed, so setting Flags will mutate the shared AST node. Please clone the clause (e.g., importTag.ImportClause.Clone(&p.factory)) before updating its flags to avoid side-effects.

Suggested change

importClause := importTag.ImportClause

importClause := importTag.ImportClause.Clone(&p.factory)

Copilot · 2025-06-13T21:37:04Z

internal/binder/nameresolver.go

+		lastLocation = location
+		location = ast.GetEffectiveTypeParent(location.Parent)


Assigning lastLocation before normalizing location means lastLocation may reference raw JSDoc nodes instead of their hosts, which could break self-reference detection. Consider updating lastLocation after computing the effective parent.

jakebailey

This seems like it regresses generic typedefs?

...submoduleAccepted/compiler/contravariantOnlyInferenceFromAnnotatedFunctionJs.errors.txt.diff

jakebailey · 2025-06-13T21:54:48Z

testdata/baselines/reference/submoduleAccepted/compiler/jsFileMethodOverloads2.types.diff

@@ -17,7 +17,7 @@
    */
   transform(fn) {
 ->transform : { <U>(fn: (y: T) => U): U; <U>(): T; }
-+>transform : { <U>(fn: (y: T) => U): U; <U_1>(): T; }
+>transform : { <U>(fn: (y: T) => U): U; (): T; }


This is spooky; now there's a func without type params.

intentional change that's an improvement in this case (T comes from the class not the function). see my comment.

I guess it's that sharing thing again. I feel like it can't be good to break it...

...es/reference/submoduleAccepted/compiler/unmetTypeConstraintInJSDocImportCall.errors.txt.diff

sandersn

explanation of some test diffs

sandersn · 2025-06-13T21:42:42Z

...submoduleAccepted/compiler/contravariantOnlyInferenceFromAnnotatedFunctionJs.errors.txt.diff

@@ -27,6 +28,8 @@
 +     * @template A
 +     * @template {Record<string, unknown>} B
 +     * @param {Funcs<A, B>} fns
+               ~~~~~~~~~~~


template tags have to come before their usage now. If this restriction is lifted, then this test passes. But I really don't want to because it makes so little sense and complicates the code for choosing between possible hosts so much.

It's better than nothing to have it work, but I do wonder how breaky this would be.

I will try to support this specific case (only one possible host in the comment, template after typedef) if somebody complains about it. But I really don't think anybody expects this to work.

sandersn · 2025-06-13T21:43:43Z

...a/baselines/reference/submoduleAccepted/compiler/expandoFunctionContextualTypesJs.types.diff

@@ -6,27 +6,22 @@
 const MyComponent = () => /* @type {any} */(null);
 ->MyComponent : StatelessComponent<MyComponentProps>
 ->() => /* @type {any} */(null) : { (): any; defaultProps: Partial<MyComponentProps>; }
-+>MyComponent : { (): any; defaultProps?: P; }
-+>() => /* @type {any} */(null) : { (): any; defaultProps: { color: string; }; }
+>MyComponent : { (): any; defaultProps?: Partial<{ color: "blue" | "red"; }>; }


type printing still de-aliases typedef when it shouldn't. It's on my list of things to investigate.

sandersn · 2025-06-13T21:44:54Z

testdata/baselines/reference/submoduleAccepted/compiler/jsFileFunctionOverloads2.types.diff

+  */
+ function flatMap(array, iterable = identity) {
+->flatMap : { <T, U>(array: T[], iterable: (x: T) => U[]): U[]; <T, U>(array: T[][]): T[]; }
+>flatMap : { <T, U>(array: T[], iterable: (x: T) => U[]): U[]; (array: T[][]): T[]; }


no type parameter sharing means that the second overload would need to add a @template T in order to be correct.

This seems like it's going to end up being even more breaky...

@overload is usually written with separated jsdoc comments to avoid the ambiguity that arises when looking for the end of a JSDocSignature, so most code will already be written in a way that requires the non-shared template tags (template tags are not shared between jsdoc comments)

sandersn · 2025-06-13T21:45:48Z

testdata/baselines/reference/submoduleAccepted/compiler/jsFileFunctionOverloads2.types.diff

+>flatMap : { <T, U>(array: T[], iterable: (x: T) => U[]): U[]; (array: T[][]): T[]; }
+ >array : unknown[]
+ >iterable : (x: unknown) => unknown
+->identity : <T_1>(x: T_1) => T_1


pretty sure T_1 was because the implementation signature had its own T in scope, but that's no longer shared.

sandersn · 2025-06-13T21:46:12Z

...es/reference/submoduleAccepted/compiler/unmetTypeConstraintInJSDocImportCall.errors.txt.diff

-      * @typedef {import('./file1').Foo<T>} Bar
+-    /**
+-     * @template T
+-     * @typedef {import('./file1').Foo<T>} Bar


still not checking tags on EOF

sandersn · 2025-06-13T21:55:13Z

...ta/baselines/reference/submoduleAccepted/conformance/callbackTagVariadicType.errors.txt.diff

 +    export const x = () => 1
 +    var res = x('a', 'b')
+                     ~~~


...T no longer makes an argument with its type into a rest type

sandersn · 2025-06-13T21:56:47Z

...nes/reference/submoduleAccepted/conformance/jsdocTemplateConstructorFunction.errors.txt.diff

+-    }
+-    var z = new Zet(1)
+-    z.t = 2
+-    z.u = false


still no constructor functions

sandersn · 2025-06-13T21:57:55Z

testdata/baselines/reference/submoduleAccepted/conformance/jsdocTemplateTag6.errors.txt.diff

+    /**
+     * @template const T
+                 ~~~~~
+!!! error TS1277: 'const' modifier can only appear on a type parameter of a function, method or class


oh nooo, I completely forgot about this feature. Not sure what is broken here exactly.

sandersn · 2025-06-13T22:00:35Z

testdata/baselines/reference/submoduleAccepted/conformance/templateInsideCallback.types.diff

- >flatMap : { <U>(): any; (): any; }
+ function flatMap(array, iterable = identity) {
+->flatMap : { <U>(): any; (): any; }
+>flatMap : { (): any; (): any; }


no more sharing (unintended in this case, so it's an improvement)

sandersn · 2025-06-13T22:01:04Z

testdata/baselines/reference/submoduleAccepted/conformance/typeTagNoErasure.types.diff

 >dibbity : T1
 >dibbity : T1

 test(1) // ok, T=1
 >test(1) : 1
 ->test : Test<number>
-+>test : <T1 extends T>(data: T1) => T1
+>test : <T1 extends number>(data: T1) => T1


type printing still following aliases incorrectly

jakebailey

Going to approve since it seems like it's net improving things, but the behavior changes do seem unfortunate and likely to bite us later

jakebailey · 2025-06-13T22:12:16Z

...submoduleAccepted/compiler/contravariantOnlyInferenceFromAnnotatedFunctionJs.errors.txt.diff

@@ -27,6 +28,8 @@
 +     * @template A
 +     * @template {Record<string, unknown>} B
 +     * @param {Funcs<A, B>} fns
+               ~~~~~~~~~~~


It's better than nothing to have it work, but I do wonder how breaky this would be.

jakebailey · 2025-06-13T22:12:47Z

testdata/baselines/reference/submoduleAccepted/compiler/jsFileFunctionOverloads2.types.diff

+  */
+ function flatMap(array, iterable = identity) {
+->flatMap : { <T, U>(array: T[], iterable: (x: T) => U[]): U[]; <T, U>(array: T[][]): T[]; }
+>flatMap : { <T, U>(array: T[], iterable: (x: T) => U[]): U[]; (array: T[][]): T[]; }


This seems like it's going to end up being even more breaky...

jakebailey · 2025-06-13T22:13:07Z

testdata/baselines/reference/submoduleAccepted/compiler/jsFileMethodOverloads2.types.diff

@@ -17,7 +17,7 @@
    */
   transform(fn) {
 ->transform : { <U>(fn: (y: T) => U): U; <U>(): T; }
-+>transform : { <U>(fn: (y: T) => U): U; <U_1>(): T; }
+>transform : { <U>(fn: (y: T) => U): U; (): T; }


I guess it's that sharing thing again. I feel like it can't be good to break it...

jakebailey · 2025-06-13T22:13:40Z

...lines/reference/submoduleAccepted/conformance/asyncFunctionDeclaration16_es5.errors.txt.diff

     const f2 = async str => {
+          ~~


complex and very hard to support

this will not stop me from asking for the feature back 🫠

sandersn added 6 commits June 11, 2025 06:54

First draft

8ac3f8a

Direct from Copilot agent Claude, needs lots of cleanup/expansion.

Mostly done except refactoring and nested signatures

15ec790

Unify typedef/callback reparsing, fix type parameter lookup

99ecb76

refactor/cleanup/regularise data structures

c79ec31

More uses of GetEffectiveTypeParent

8002b0c

Remove last Clone in reparser.go

097fe59

Copilot AI review requested due to automatic review settings June 13, 2025 21:35

Copilot AI reviewed Jun 13, 2025

View reviewed changes

jakebailey reviewed Jun 13, 2025

View reviewed changes

sandersn commented Jun 13, 2025

View reviewed changes

jakebailey approved these changes Jun 13, 2025

View reviewed changes

Merge branch 'main' into add-callback-tag

f6179a8

jakebailey approved these changes Jun 13, 2025

View reviewed changes

Merge branch 'main' into add-callback-tag

bdcd892

sandersn enabled auto-merge June 13, 2025 22:40

sandersn added this pull request to the merge queue Jun 13, 2025

Merged via the queue into microsoft:main with commit 2668cdf Jun 13, 2025
22 checks passed

sandersn deleted the add-callback-tag branch June 13, 2025 23:08

Andarist mentioned this pull request Jun 21, 2025

Bring back SourceFile.EndOfFileToken #1257

Draft

	importClause := importTag.ImportClause
	importClause := importTag.ImportClause.Clone(&p.factory)

		lastLocation = location
		location = ast.GetEffectiveTypeParent(location.Parent)

Add callback tag, fix jsdoc type parameter resolution #1187

Add callback tag, fix jsdoc type parameter resolution #1187

Uh oh!

Conversation

sandersn commented Jun 13, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

jakebailey left a comment

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!

Uh oh!

sandersn left a comment

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!

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!

jakebailey left a comment

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!

Uh oh!

Uh oh!