Type nodes for JSDoc #1013

sandersn · 2025-06-02T16:46:45Z

In Javascript, you can give a type and modifiers (for classes) to expandos, exports and object literal properties:

/** @type {number} */
module.exports = 12
const o = {
  /** @type {number} */
  p: 12
}
/** @type {number} */
f.p = 12
class C {
  constructor() {
    /** 
     * @private
     * @type {number}
     */
    this.p = 12
  }
}

Alternate PR name: Types where they shouldn't be.

This PR adds Type nodes to nodes that can only get types from jsdoc. Then it checks those types, replacing the previous hack of reparsing a type assertion. In order to make this work on commonjs exports it starts calling withJSDoc on synthetic commonjs nodes. It also (sorry), refactors reparser.go into separate functions, and stops cloning type nodes since we're moving to a no-clone reparsing. I'll remove the other 3 clones later, as part of the PR that corrects synthetic-parent walks for @template.

This is the first infrastructure/fix PR of the ones I'll spend most of June on. I hope they'll get more coherent but this one sprawls a bit.

Nodes with new Type nodes

PropertyAssignment
ShorthandPropertyAssignment
[JS]ExportAssignment
CommonJSExport
BinaryExpression, which also adds Modifiers to support private on this.p assignments.
These additions are going to cost memory, which we agreed on at the design meeting.

Returns do not add type annotations because I think this is a low-value feature, and TS type checking has no equivalent whatsoever.

Notes

To fix contextual typing on commonjs exports, which of course walks up the parent chain, I should also have to introduce a Host property on BinaryExpression, because CommonJSExport is reparsed from BinaryExpression. This was a bridge too far for me, so I cheated: both CommonJSExport and BinaryExpression have the same type annotation, so I let the @type double-attach and have special-case code only in getContextualTypeForBinaryOperand to recognise the BinaryExpression syntax for module.exports.

ShorthandPropertyAssignment type checking was inconsistent in the two places it existed. When I refactored it to a single function, I left the (unannotated) behaviour the same, but it means that the inDestructuringPattern argument seems backwards. We should revisit this because I think the consistent behaviour may be right.

I started removing Clones in reparsing, starting with type nodes. This required me to add a synthetic-parent lookup in 2 more places (the existing one is in nameresolver.go). I expect there are a few more, so I created a utility. In order to nudge us toward making Parent a method, I gave it the name getEffectiveTypeParent. But I'm not sure how hard/how important a Parent method is.

The reparser refactor obscures a few changes (sorry!), but not many. The cases in reparseHosted for PropertyAssignment, etc just change from makeNewTypeAssertion to makeNewType.

Copilot

Pull Request Overview

This PR adds explicit TypeNode fields to various AST nodes that previously only got types via JSDoc hacks, refactors the reparser to remove cloning of type assertions, and updates factory and transformer calls to pass the new typeNode argument.

Introduce TypeNode on PropertyAssignment, ShorthandPropertyAssignment, ExportAssignment, CommonJSExport, and BinaryExpression
Refactor reparser.go to share parsing logic for JSDoc tags and remove clone-based reparsing
Update all factory, transformer, binding, and checker code to thread through the new typeNode parameter

Reviewed Changes

Copilot reviewed 207 out of 207 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
testdata/baselines/reference/submodule/compiler/argumentsReferenceInConstructor3_Js.types	Baseline updated to reflect new type inference on `super.arguments`
internal/ast/ast.go	Added new `Type()` cases and panic message enhancements
internal/ast/utilities.go	Introduced `GetEffectiveTypeParent` helper
internal/parser/jsdoc.go	Changed `withJSDoc` to return parsed tags array
internal/parser/reparser.go	Refactored `reparseCommonJS`, added `reparseTags`, `gatherTypeParameters`, and `reparseHosted`
internal/parser/parser.go	Updated calls to `withJSDoc`, `reparseCommonJS`, and many factory invocations
internal/transformers/*	Passed `nil /typeNode/` to all `New` and `Update` calls
internal/checker/checker.go	Added type-node–based checks for exports and property assignments
internal/binder/binder.go	Extended binding to include `expr.Type` in destructor flows
internal/printer/factory.go	Updated helper methods to pass `nil /typeNode/` on binary ops

Comments suppressed due to low confidence (1)

testdata/baselines/reference/submodule/compiler/argumentsReferenceInConstructor3_Js.types:48

The baseline now shows any instead of the expected object type for super.arguments.foo. Verify the new double-attach logic isn't overriding the correct contextual type.

>this.bar = super.arguments.foo : any

Copilot · 2025-06-02T16:49:18Z

internal/parser/jsdoc.go

@@ -60,7 +60,9 @@ func (p *Parser) withJSDoc(node *ast.Node, hasJSDoc bool) {
 			p.reparseTags(node, jsdoc)
 		}
 		p.jsdocCache[node] = jsdoc
+		return jsdoc
 	}


The cache branch always returns nil, dropping previously parsed JSDoc tags. You should return the cached jsdoc slice (e.g. return p.jsdocCache[node]) instead of nil when node is already in jsdocCache.

Suggested change

}

}

if cached, ok := p.jsdocCache[node]; ok {

return cached

}

No. jsdocCache is write-only, write-once here by the nature of the parser. The current bode's jsdoc will never be cached.

Copilot · 2025-06-02T16:49:19Z

internal/parser/reparser.go

 		panic("JSDoc type expression already has a host: " + typeExpression.AsJSDocTypeExpression().Host.Kind.String())
 	}
-	t := typeExpression.Type().Clone(&p.factory)
+	t := typeExpression.Type()


Removing the clone here means the same TypeNode instance may be reused across multiple hosts, leading to shared mutations. Consider cloning (Clone(&p.factory)) or otherwise ensuring each host gets its own TypeNode.

Suggested change

t := typeExpression.Type()

t := typeExpression.Type().Clone(&p.factory)

That's what the panic is for! The JSExportAssignment/CommonJSExport exemption avoids a Host pointer on BinaryExpression, at the cost of special handling later.

Copilot · 2025-06-02T16:49:19Z

internal/ast/ast.go

@@ -366,7 +366,7 @@ func (n *Node) Expression() *Node {
 	case KindJsxSpreadAttribute:
 		return n.AsJsxSpreadAttribute().Expression
 	}
-	panic("Unhandled case in Node.Expression")
+	panic("Unhandled case in Node.Expression" + n.Kind.String())


[nitpick] The panic message concatenates the kind string without a separator. For clarity, add a space or colon (e.g. "Unhandled case in Node.Expression: " + n.Kind.String()).

Suggested change

panic("Unhandled case in Node.Expression" + n.Kind.String())

panic("Unhandled case in Node.Expression: " + n.Kind.String())

Yeah, good idea.

sandersn · 2025-06-02T17:00:19Z

...aselines/reference/submoduleAccepted/compiler/argumentsReferenceInConstructor3_Js.types.diff

@@ -30,13 +29,12 @@
 		this.bar = super.arguments.foo;
 ->this.bar = super.arguments.foo : error
 ->this.bar : any
-+>this.bar = super.arguments.foo : object
+>this.bar = super.arguments.foo : any


any is better than object because it shows that the sythetic cast is gone. There's an error, so error would be even better though.

sandersn · 2025-06-02T17:02:32Z

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

-
+ a.js(8,18): error TS2353: Object literal may only specify known properties, and 'c' does not exist in type 'Foo'.
+
+
 -==== checkJsdocTypeTagOnExportAssignment1.js (0 errors) ====


all that's left is a diff in the test framework I think

#1014 will fix this.

sandersn · 2025-06-02T17:06:18Z

...selines/reference/submoduleAccepted/compiler/checkJsdocTypeTagOnExportAssignment3.types.diff


 a;
 ->a : import("a").Foo
-+>a : import("./a").Foo
+>a : { c: number; }


same type printing diff as for all reparsed imports. Needs later investigation.

sandersn · 2025-06-02T17:08:01Z

...reference/submoduleAccepted/compiler/jsdocTypeNongenericInstantiationAttempt.errors.txt.diff

@@ -4,120 +4,39 @@
 index.js(2,19): error TS2315: Type 'Boolean' is not generic.
 -index2.js(2,19): error TS2315: Type 'Void' is not generic.
 -index3.js(2,19): error TS2315: Type 'Undefined' is not generic.
-+index.js(2,27): error TS2304: Cannot find name 'T'.
 +index2.js(2,19): error TS2304: Cannot find name 'Void'.


this name resolution improvement is a result of not cloning the type anymore

sandersn · 2025-06-02T17:21:59Z

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

+     import * as debug from './mod'
+
+     debug.formatters.j
+                     ~


this is an improvement in imports, it's just that multi-file expandos are not supported in Corsa. I'll send a later PR adding to the list of accepted diffs.

sandersn · 2025-06-02T17:25:31Z

...eAccepted/conformance/instantiateTemplateTagTypeParameterOnVariableStatement.errors.txt.diff

+    /** @type {string} */
+    var text3 = seq(text1)(text2);
+        ~~~~~
+!!! error TS2322: Type 'T' is not assignable to type 'string'.


@template still needs a fix to type instantation -- I believe it's inspecting parents at some point and failing.

sandersn · 2025-06-02T17:27:00Z

...baselines/reference/submoduleAccepted/conformance/jsDeclarationsFunctionsCjs.errors.txt.diff

 +    module.exports.a = function a() {}
 +    
 +    module.exports.b = function b() {}
 +    module.exports.b.cat = "cat";
+                     ~~~


same typing improvement, but nested expandos aren't supported yet

sandersn · 2025-06-02T17:33:23Z

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

 module.exports = 0;
- >module.exports = 0 : string
+->module.exports = 0 : string
+>module.exports = 0 : 0


I think this is an improvement, in which the binary expression source of the export ignores the annotation, but usages of it have the annotated type.

jakebailey · 2025-06-02T22:41:47Z

testdata/baselines/reference/submodule/conformance/typeFromContextualThisType.symbols.diff

@@ -6,7 +6,7 @@
 >this.b : Symbol(b, Decl(bug25926.js, 0, 23))
 ->this : Symbol(__type, Decl(bug25926.js, 0, 11))
 ->b : Symbol(b, Decl(bug25926.js, 2, 9))
-+>this : Symbol(o1, Decl(bug25926.js, 0, 11))
+>this : Symbol(�type, Decl(bug25926.js, 0, 11))


This is getting some sort of goofy symbol name, it seems.

this is the usual string for the intenral name of types. Does it not normally print?
Edit: It prints in Strada, whereit's __type not th'type.

jakebailey · 2025-06-03T04:24:40Z

In Javascript, you can give a type and modifiers (for classes) to expandos, exports and object literal properties:

And functions 😭

sandersn · 2025-06-03T13:10:21Z

I know, I know. But the function annotation code is so complicated. It was a bug farm for years in Strada.

sandersn · 2025-06-02T17:34:11Z

internal/ast/ast.go

@@ -366,7 +366,7 @@ func (n *Node) Expression() *Node {
 	case KindJsxSpreadAttribute:
 		return n.AsJsxSpreadAttribute().Expression
 	}
-	panic("Unhandled case in Node.Expression")
+	panic("Unhandled case in Node.Expression" + n.Kind.String())


Yeah, good idea.

sandersn · 2025-06-02T21:54:32Z

internal/binder/binder.go

@@ -1649,7 +1649,7 @@ func (b *Binder) bindChildren(node *ast.Node) {
 		b.inAssignmentPattern = saveInAssignmentPattern
 		b.bindEachChild(node)
 	case ast.KindJSExportAssignment, ast.KindCommonJSExport:
-		return // Reparsed nodes do not double-bind children, which are not reparsed
+		// Reparsed nodes do not double-bind children, which are not reparsed


I think this is an improvement--now setJSDocParents runs and inAssignmentPattern correctly follows dynamic scope

sandersn · 2025-06-02T21:57:38Z

internal/checker/checker.go

@@ -15352,11 +15374,15 @@ func (c *Checker) getTypeOfVariableOrParameterOrPropertyWorker(symbol *ast.Symbo
 	case ast.KindPropertyAssignment:
 		result = c.checkPropertyAssignment(declaration, CheckModeNormal)
 	case ast.KindShorthandPropertyAssignment:
-		result = c.checkExpressionForMutableLocation(declaration.Name(), CheckModeNormal)
+		result = c.checkShorthandPropertyAssignment(declaration, true /*inDestructuringPattern*/, CheckModeNormal)


this isn't necessarily in a destructuring pattern, but true=never use the object assigmebt initialiser. Sometimes this shows up outside a destructuring though! I've passed true here to keep the behaviour the same but we should check this out in more detail.

sandersn · 2025-06-02T22:16:13Z

internal/parser/jsdoc.go

@@ -60,7 +60,9 @@ func (p *Parser) withJSDoc(node *ast.Node, hasJSDoc bool) {
 			p.reparseTags(node, jsdoc)
 		}
 		p.jsdocCache[node] = jsdoc
+		return jsdoc
 	}


No. jsdocCache is write-only, write-once here by the nature of the parser. The current bode's jsdoc will never be cached.

sandersn · 2025-06-02T22:22:06Z

internal/parser/reparser.go

 		panic("JSDoc type expression already has a host: " + typeExpression.AsJSDocTypeExpression().Host.Kind.String())
 	}
-	t := typeExpression.Type().Clone(&p.factory)
+	t := typeExpression.Type()


That's what the panic is for! The JSExportAssignment/CommonJSExport exemption avoids a Host pointer on BinaryExpression, at the cost of special handling later.

sandersn added 2 commits June 2, 2025 07:58

initial commit

2ef6da2

Merge branch 'main' into type-nodes-for-jsdoc

35b83e8

Copilot AI review requested due to automatic review settings June 2, 2025 16:46

Copilot AI reviewed Jun 2, 2025

View reviewed changes

sandersn commented Jun 2, 2025

View reviewed changes

sandersn added 3 commits June 2, 2025 10:57

add ShorthandPropertyAssignment.Type correctly

2c286cc

more nil-param comments--thanks Copilot

11909f4

Merge branch 'main' into pr/sandersn/1013

aaf9260

jakebailey reviewed Jun 2, 2025

View reviewed changes

sandersn commented Jun 3, 2025

View reviewed changes

-	}
+	}
+	if cached, ok := p.jsdocCache[node]; ok {
+		return cached
+	}

	t := typeExpression.Type()
	t := typeExpression.Type().Clone(&p.factory)

	panic("Unhandled case in Node.Expression" + n.Kind.String())
	panic("Unhandled case in Node.Expression: " + n.Kind.String())

Type nodes for JSDoc #1013

Are you sure you want to change the base?

Type nodes for JSDoc #1013

Uh oh!

Conversation

sandersn commented Jun 2, 2025

Nodes with new Type nodes

Notes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jun 2, 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!

sandersn Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakebailey commented Jun 3, 2025

Uh oh!

sandersn commented Jun 3, 2025

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!

sandersn Jun 3, 2025 •

edited

Loading