Deep clone JSDoc #1412

sandersn · 2025-07-17T13:17:14Z

This PR switches the approach of JSDoc reparsing from copying pointers plus adding alternate parents to deep-cloning nodes and keeping a map of original nodes to clones. However, this PR does not include the map yet; it's not needed in the checker and the jsdoc support in the language service doesn't work correctly either way (as measured by our tests at least).

Notably, I couldn't figure out how to set parent pointers during the deep clone itself, due to the visitor structure. I think the cost of two passes is likely to be small given the number of jsdoc nodes that will need to be cloned. But suggestions are welcome for how to incorporate parent-setting into the deep clone.

The code is slightly more verbose than it could be, with individual DeepCloneNode calls for all children. I could have instead cloned the entire node at the end of the process, but this is more efficient for hosted tags that would be cloning all children of, say, a function, which would end up cloning the entire body: a possibly-large subtree. I also think that the explicit child clones convey the intent more obviously.

- No crashes - this types aren't working - type checking of `p` in `module.exports.p` isn't working - symbol baselining of `module.exports.p` no longer includes `p` either That last is probably fine.

Some things are stricter now, almost all because module.exports.p with module.exports= no longer binds a symbol. Also, type printing of aliases of module.exports changed to print the thing itself instead of an `import('.')` Need to look at fourslash tests and code now.

As far as I can tell, FAR didn't work with jsdoc before and doesn't work now at about the same level of brokenness.

Copilot

Pull Request Overview

This PR switches JSDoc handling from copying pointers plus alternate parents to deep-cloning nodes and maintaining proper parent pointers. The change improves type accuracy and eliminates complex parent pointer management in JSDoc nodes, though some functionality improvements are deferred to future work.

Switches from pointer copying to node deep-cloning for JSDoc processing
Improves type accuracy for property assignments and class expressions
Removes complex parent pointer handling logic for reparsed nodes

Reviewed Changes

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

File	Description
test baselines	Updated type and symbol baselines reflecting improved JSDoc type resolution
internal/testutil/tsbaseline/type_symbol_baseline.go	Modified node traversal logic to handle reparsed nodes in type contexts
internal/testrunner/compiler_runner.go	Removed parent pointer validation exceptions for JSDoc export scenarios

Copilot · 2025-07-17T13:17:54Z

internal/testutil/tsbaseline/type_symbol_baseline.go

@@ -327,8 +327,9 @@ func forEachASTNode(node *ast.Node) []*ast.Node {
 	for len(work) > 0 {
 		elem := work[len(work)-1]
 		work = work[:len(work)-1]
-		if elem.Flags&ast.NodeFlagsReparsed == 0 || elem.Kind == ast.KindAsExpression || elem.Kind == ast.KindSatisfiesExpression {
-			if elem.Flags&ast.NodeFlagsReparsed == 0 {
+		if elem.Flags&ast.NodeFlagsReparsed == 0 || elem.Kind == ast.KindAsExpression || elem.Kind == ast.KindSatisfiesExpression || 


The condition on line 330-332 is complex and spans multiple lines with unclear logic. Consider extracting this into a helper function with a descriptive name like shouldIncludeNodeInBaseline to improve readability and maintainability.

I guess, but it's also in test code, and is an ad-hoc condition that will only ever appear here. The mess makes that obvious; extracting to a function would make readers think that we might want to use it elsewhere.

I'm on the fence. Any humans want to weigh in?

Copilot · 2025-07-17T13:17:55Z

internal/testutil/tsbaseline/type_symbol_baseline.go

+		if elem.Flags&ast.NodeFlagsReparsed == 0 || elem.Kind == ast.KindAsExpression || elem.Kind == ast.KindSatisfiesExpression || 
+		((elem.Parent.Kind == ast.KindSatisfiesExpression || elem.Parent.Kind == ast.KindAsExpression) && elem == elem.Parent.Expression()) {
+			if elem.Flags&ast.NodeFlagsReparsed == 0 || elem.Parent.Kind == ast.KindAsExpression || elem.Parent.Kind == ast.KindSatisfiesExpression {
 				result = append(result, elem)
 			}


This nested condition duplicates logic from the outer condition and creates confusing control flow. The logic for handling reparsed nodes should be consolidated and clearly documented.

Suggested change

if elem.Flags&ast.NodeFlagsReparsed == 0 || elem.Kind == ast.KindAsExpression || elem.Kind == ast.KindSatisfiesExpression ||

((elem.Parent.Kind == ast.KindSatisfiesExpression || elem.Parent.Kind == ast.KindAsExpression) && elem == elem.Parent.Expression()) {

if elem.Flags&ast.NodeFlagsReparsed == 0 || elem.Parent.Kind == ast.KindAsExpression || elem.Parent.Kind == ast.KindSatisfiesExpression {

result = append(result, elem)

}

if (elem.Flags&ast.NodeFlagsReparsed == 0 || elem.Kind == ast.KindAsExpression || elem.Kind == ast.KindSatisfiesExpression ||

((elem.Parent.Kind == ast.KindSatisfiesExpression || elem.Parent.Kind == ast.KindAsExpression) && elem == elem.Parent.Expression())) &&

(elem.Flags&ast.NodeFlagsReparsed == 0 || elem.Parent.Kind == ast.KindAsExpression || elem.Parent.Kind == ast.KindSatisfiesExpression) {

result = append(result, elem)

sandersn · 2025-07-17T13:26:58Z

internal/testutil/tsbaseline/type_symbol_baseline.go

@@ -327,8 +327,9 @@ func forEachASTNode(node *ast.Node) []*ast.Node {
 	for len(work) > 0 {
 		elem := work[len(work)-1]
 		work = work[:len(work)-1]
-		if elem.Flags&ast.NodeFlagsReparsed == 0 || elem.Kind == ast.KindAsExpression || elem.Kind == ast.KindSatisfiesExpression {
-			if elem.Flags&ast.NodeFlagsReparsed == 0 {
+		if elem.Flags&ast.NodeFlagsReparsed == 0 || elem.Kind == ast.KindAsExpression || elem.Kind == ast.KindSatisfiesExpression || 


I guess, but it's also in test code, and is an ad-hoc condition that will only ever appear here. The mess makes that obvious; extracting to a function would make readers think that we might want to use it elsewhere.

I'm on the fence. Any humans want to weigh in?

sandersn · 2025-07-17T13:32:08Z

internal/parser/reparser.go

@@ -5,7 +5,9 @@ import (
 	"github.com/microsoft/typescript-go/internal/core"
 )

-func (p *Parser) finishReparsedNode(node *ast.Node) {
+func (p *Parser) finishReparsedNode(node *ast.Node, locationNode *ast.Node) {
+	node.Flags = p.contextFlags | ast.NodeFlagsReparsed


This change is responsible for a lot of the diff, but that's because it dedupes a lot of code.

sandersn · 2025-07-17T13:47:07Z

internal/parser/reparser.go

-func (p *Parser) gatherTypeParameters(j *ast.Node, tagWithTypeParameters *ast.Node, host *ast.Node) *ast.NodeList {
-	typeParameters := p.nodeSlicePool.NewSlice(0)
+func (p *Parser) gatherTypeParameters(j *ast.Node, tagWithTypeParameters *ast.Node) *ast.NodeList {
+	var typeParameters []*ast.Node


I don't know why I didn't lazy-initialise this before, because the common case is to have no parameters, which previously called NewSlice(0) each time anyway.

sandersn · 2025-07-17T13:56:58Z

internal/parser/reparser.go

-						p.finishReparsedNode(declaration)
+					if declaration.AsVariableDeclaration().Type == nil && tag.AsJSDocTypeTag().TypeExpression != nil {
+						declaration.AsVariableDeclaration().Type = p.factory.DeepCloneReparse(tag.AsJSDocTypeTag().TypeExpression.Type())
+						p.overrideParentInImmediateChildren(declaration)


Since finishReparsedNode now sets location and flags, this just (re)sets parent pointers of the existing node's children. I think this makes more sense since the variable declaration is not a reparsed node, just its type annotation.

sandersn · 2025-07-17T13:58:18Z

internal/parser/reparser.go

 func getClassLikeData(parent *ast.Node) *ast.ClassLikeBase {
 	var class *ast.ClassLikeBase
-	if parent.Kind == ast.KindClassDeclaration {
+	switch parent.Kind {


gopls or some tool wouldn't be quiet until I accepted this change. I guess it's better.

sandersn · 2025-07-17T15:00:59Z

...ference/submoduleAccepted/conformance/jsDeclarationsExportAssignedClassExpression.types.diff

-+>exports : typeof import(".")
-+>class Thing {    /**     * @param {number} p     */    constructor(p) {        this.t = 12 + p;    }} : typeof import(".")
-+>Thing : typeof import(".")
+>module.exports = class Thing {    /**     * @param {number} p     */    constructor(p) {        this.t = 12 + p;    }} : typeof Thing


I'm not sure why this type printing changed, but it seems like an improvement.

Thing is in scope at the return position now because the .Parent chain isn't inside the jsdoc comment anymore, so it's resolving at the actual export now. I think.

Or we're mistakenly binding Thing into the local scope. One of those.

Sounds like the first then, because the only weirdness with binding module.exports= compared to export= is that the former always binds with the source file as a container, even if it's inside some other container. Which doesn't apply here.

I checked this test by changing it to use export=, and that result is the same as the new result, so this is an improvement inasmuch as it makes JS more like TS.

sandersn · 2025-07-17T15:02:05Z

...ubmoduleAccepted/conformance/jsDeclarationsExportAssignedClassExpressionAnonymous.types.diff

-+>exports : typeof import(".")
-+>class {    /**     * @param {number} p     */    constructor(p) {        this.t = 12 + p;    }} : typeof import(".")
+>module.exports = class {    /**     * @param {number} p     */    constructor(p) {        this.t = 12 + p;    }} : typeof exports
+>module.exports : typeof (Anonymous class)


same, except arguably import(".") is better than (Anonymous class) since it's unique.

sandersn · 2025-07-17T15:05:09Z

...ference/submoduleAccepted/conformance/jsdocTypeReferenceToImportOfClassExpression.types.diff

@@ -24,18 +24,18 @@
    */
   constructor(compiler) {
 ->compiler : import("MC")
-+>compiler : import("./MC")
+>compiler : MC


this is similar, except here MC is the name of the type alias and import("./MC") is its value.

sandersn · 2025-07-17T15:10:07Z

...reference/submoduleAccepted/conformance/moduleExportWithExportPropertyAssignment3.types.diff

@@ -75,7 +75,7 @@
 +>module.exports : { justExport: number; bothBefore: number; bothAfter: number; }
 +>module : { "export=": { justExport: number; bothBefore: number; bothAfter: number; }; }
 +>exports : { justExport: number; bothBefore: number; bothAfter: number; }
-+>bothBefore : "string"
+>bothBefore : number


Corsa dropped the combination of module.exports.both=1 with module.exports={ both: "string" } -- now that the module.exports= always wins, it always provides the type. Before, the property would sometimes win (when it comes first?).

sandersn · 2025-07-17T15:17:07Z

...ines/reference/submoduleAccepted/conformance/moduleExportsElementAccessAssignment.types.diff

@@ -80,8 +75,7 @@
 +>module["exports"] : typeof import("./mod1")
 +>module : { "\"mod1\"": typeof import("./mod1"); }
 >"exports" : "exports"
->"d" : "d"
-+>"d" : {}
+ >"d" : "d"


I don't understand this change, but it's the way Strada works. Maybe it's checking the literal itself instead of the property with the literal name.

internal/parser/reparser.go

weswigham · 2025-07-17T17:47:55Z

...ference/submoduleAccepted/conformance/jsDeclarationsExportAssignedClassExpression.types.diff

-+>exports : typeof import(".")
-+>class Thing {    /**     * @param {number} p     */    constructor(p) {        this.t = 12 + p;    }} : typeof import(".")
-+>Thing : typeof import(".")
+>module.exports = class Thing {    /**     * @param {number} p     */    constructor(p) {        this.t = 12 + p;    }} : typeof Thing


Thing is in scope at the return position now because the .Parent chain isn't inside the jsdoc comment anymore, so it's resolving at the actual export now. I think.

Or we're mistakenly binding Thing into the local scope. One of those.

sandersn added 6 commits July 15, 2025 08:56

Nearly done, maybe.

715f9ff

- No crashes - this types aren't working - type checking of `p` in `module.exports.p` isn't working - symbol baselining of `module.exports.p` no longer includes `p` either That last is probably fine.

Accept fourslash test diffs

dc27fd8

As far as I can tell, FAR didn't work with jsdoc before and doesn't work now at about the same level of brokenness.

simplify binder diff

7e02714

Make reparser code more efficient and readable

fe63e18

remove parent pointer test exemption

c2bae73

sandersn requested review from Copilot, weswigham, iisaduan and andrewbranch and removed request for Copilot July 17, 2025 13:17

Copilot AI reviewed Jul 17, 2025

View reviewed changes

sandersn added 2 commits July 17, 2025 06:27

hereby format

2ee069d

readability improvements from the PR

0fa72bb

sandersn commented Jul 17, 2025

View reviewed changes

sandersn added 2 commits July 17, 2025 09:12

Merge branch 'main' into deepclone-jsdoc

f0aed28

add unit tests showing that hover of jsdoc nodes does not work yet

478b7e2

weswigham approved these changes Jul 17, 2025

View reviewed changes

purpose-named wrapper for overrideParentInImmediateChildren

139841e

jakebailey approved these changes Jul 18, 2025

View reviewed changes

sandersn added this pull request to the merge queue Jul 18, 2025

Merged via the queue into microsoft:main with commit 96c0688 Jul 18, 2025
22 checks passed

sandersn deleted the deepclone-jsdoc branch July 18, 2025 15:55

Deep clone JSDoc #1412

Deep clone JSDoc #1412

Uh oh!

Conversation

sandersn commented Jul 17, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 17, 2025

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!