Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions internal/ast/ast.go
Original file line number Diff line number Diff line change
Expand Up @@ -897,6 +897,7 @@ func (n *Node) IsTypeOnly() bool {
return false
}

// If updating this function, also update `hasComment`.
func (n *Node) CommentList() *NodeList {
switch n.Kind {
case KindJSDoc:
Expand Down Expand Up @@ -1033,6 +1034,8 @@ func (n *Node) ElementList() *NodeList {
return n.AsNamedImports().Elements
case KindNamedExports:
return n.AsNamedExports().Elements
case KindObjectBindingPattern, KindArrayBindingPattern:
return n.AsBindingPattern().Elements
}

panic("Unhandled case in Node.ElementList: " + n.Kind.String())
Expand Down Expand Up @@ -1060,6 +1063,32 @@ func (n *Node) QuestionDotToken() *Node {
panic("Unhandled case in Node.QuestionDotToken: " + n.Kind.String())
}

func (n *Node) TypeExpression() *Node {
switch n.Kind {
case KindJSDocPropertyTag, KindJSDocParameterTag:
return n.AsJSDocParameterOrPropertyTag().TypeExpression
case KindJSDocReturnTag:
return n.AsJSDocReturnTag().TypeExpression
case KindJSDocTypeTag:
return n.AsJSDocTypeTag().TypeExpression
case KindJSDocTypedefTag:
return n.AsJSDocTypedefTag().TypeExpression
case KindJSDocSatisfiesTag:
return n.AsJSDocSatisfiesTag().TypeExpression
}
panic("Unhandled case in Node.TypeExpression: " + n.Kind.String())
}

func (n *Node) ClassName() *Node {
switch n.Kind {
case KindJSDocAugmentsTag:
return n.AsJSDocAugmentsTag().ClassName
case KindJSDocImplementsTag:
return n.AsJSDocImplementsTag().ClassName
}
panic("Unhandled case in Node.ClassName: " + n.Kind.String())
}

// Determines if `n` contains `descendant` by walking up the `Parent` pointers from `descendant`. This method panics if
// `descendant` or one of its ancestors is not parented except when that node is a `SourceFile`.
func (n *Node) Contains(descendant *Node) bool {
Expand Down Expand Up @@ -2049,6 +2078,8 @@ type (
NamedExportsNode = Node
UnionType = Node
LiteralType = Node
JSDocNode = Node
BindingPatternNode = Node
)

type (
Expand Down Expand Up @@ -9535,6 +9566,10 @@ func (node *JSDocTemplateTag) Clone(f NodeFactoryCoercible) *Node {
return cloneNode(f.AsNodeFactory().NewJSDocTemplateTag(node.TagName, node.Constraint, node.TypeParameters, node.Comment), node.AsNode(), f.AsNodeFactory().hooks)
}

func IsJSDocTemplateTag(n *Node) bool {
return n.Kind == KindJSDocTemplateTag
}

// JSDocParameterOrPropertyTag
type JSDocParameterOrPropertyTag struct {
JSDocTagBase
Expand Down Expand Up @@ -9600,6 +9635,10 @@ func (node *JSDocParameterOrPropertyTag) Clone(f NodeFactoryCoercible) *Node {

func (node *JSDocParameterOrPropertyTag) Name() *EntityName { return node.name }

func IsJSDocParameterTag(node *Node) bool {
return node.Kind == KindJSDocParameterTag
}

// JSDocReturnTag
type JSDocReturnTag struct {
JSDocTagBase
Expand Down Expand Up @@ -9893,6 +9932,10 @@ func (node *JSDocImplementsTag) Clone(f NodeFactoryCoercible) *Node {
return cloneNode(f.AsNodeFactory().NewJSDocImplementsTag(node.TagName, node.ClassName, node.Comment), node.AsNode(), f.AsNodeFactory().hooks)
}

func IsJSDocImplementsTag(node *Node) bool {
return node.Kind == KindJSDocImplementsTag
}

// JSDocAugmentsTag
type JSDocAugmentsTag struct {
JSDocTagBase
Expand Down
40 changes: 36 additions & 4 deletions internal/ast/utilities.go
Original file line number Diff line number Diff line change
Expand Up @@ -2104,6 +2104,9 @@
}

// True if node is of a JSDoc kind that may contain comment text.
// NOTE: while this is a faithful port of Strada's implementation,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should Comments just check this and return nil or something?

Copy link
Member Author

@gabritto gabritto Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not the way it used to work in Strada. You can access .comments on the node even if .comments is a string, but you wouldn't visit them in Strada's forEachChild.

Is there a chance we need the old non-special case way?

I think we might need a version that visits all things. e.g. in Corsa,ForEachChild is used in the binder and so probably needs to visit single comments in JSDoc. We may be able to get away with adding the special behavior to Corsa's VisitEachChild, but I'm not sure.

@sandersn might have a better idea.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stupid suggestion because I just started thinking about this, but: JSDocTypeLiteral and JSDocTypeSignature are the bodies of a typedef and a callback respectively. The reason they don't have comments is that their parent has the comment instead. Why do they return true for this function? Is it because they contain children that could themselves contain comments?

This function is only called in two places, neither of which appear to deal with jsdoc comment text, but finding nodes from a position. My best guess is that the function needs to be renamed so that people don't think Comment() is safe to call on nodes it returns true from.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I got totally confused in my original answer, I thought Jake's comment was about a different function. But I agree we should rename it, I just don't know to what. In Strada this function even has a comment saying "True if node is of a kind that may contain comment text", so maybe this function was wrong in Strada too? I'm not sure. Maybe we should rename to DoNotUseVeryBadIsJSDocCommentContainingNode 😅

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed it and also moved it to the astnav package, the only place where it's used. Couldn't think of a good name because I don't really know what the function is supposed to do.

// it is unsafe to call `.Comments()` on a node that returns true from this function,
// becuase JSDoc type literal and JSDoc signature nodes do not have comments.

Check failure on line 2109 in internal/ast/utilities.go

View workflow job for this annotation

GitHub Actions / lint (ubuntu-latest)

`becuase` is a misspelling of `because` (misspell)
func IsJSDocCommentContainingNode(node *Node) bool {
return node.Kind == KindJSDoc ||
node.Kind == KindJSDocText ||
Expand Down Expand Up @@ -3017,13 +3020,27 @@
return node.Kind == KindTypeKeyword
}

// If node is a single comment JSDoc, we do not visit the comment node list.
func IsJSDocSingleCommentNodeList(parent *Node, nodeList *NodeList) bool {
return IsJSDocSingleCommentNode(parent) && nodeList == parent.AsJSDoc().Comment
// See `IsJSDocSingleCommentNode`.
func IsJSDocSingleCommentNodeList(nodeList *NodeList) bool {
if nodeList == nil || len(nodeList.Nodes) == 0 {
return false
}
parent := nodeList.Nodes[0].Parent
return IsJSDocSingleCommentNode(parent) && nodeList == parent.CommentList()
}

// See `IsJSDocSingleCommentNode`.
func IsJSDocSingleCommentNodeComment(node *Node) bool {
if node == nil || node.Parent == nil {
return false
}
return IsJSDocSingleCommentNode(node.Parent) && node == node.Parent.CommentList().Nodes[0]
}

// In Strada, if a JSDoc node has a single comment, that comment is represented as a string property
// as a simplification, and therefore that comment is not visited by `forEachChild`.
func IsJSDocSingleCommentNode(node *Node) bool {
return node.Kind == KindJSDoc && node.AsJSDoc().Comment != nil && len(node.AsJSDoc().Comment.Nodes) == 1
return hasComment(node.Kind) && node.CommentList() != nil && len(node.CommentList().Nodes) == 1
}

func IsValidTypeOnlyAliasUseSite(useSite *Node) bool {
Expand Down Expand Up @@ -3635,3 +3652,18 @@
}
})
}

// Returns true if the node kind has a comment property.
func hasComment(kind Kind) bool {
switch kind {
case KindJSDoc, KindJSDocTag, KindJSDocAugmentsTag, KindJSDocImplementsTag,
KindJSDocDeprecatedTag, KindJSDocPublicTag, KindJSDocPrivateTag, KindJSDocProtectedTag,
KindJSDocReadonlyTag, KindJSDocOverrideTag, KindJSDocCallbackTag, KindJSDocOverloadTag,
KindJSDocParameterTag, KindJSDocPropertyTag, KindJSDocReturnTag, KindJSDocThisTag,
KindJSDocTypeTag, KindJSDocTemplateTag, KindJSDocTypedefTag, KindJSDocSeeTag,
KindJSDocSatisfiesTag, KindJSDocImportTag:
return true
default:
return false
}
}
55 changes: 35 additions & 20 deletions internal/astnav/tokens.go
Original file line number Diff line number Diff line change
Expand Up @@ -125,10 +125,8 @@ func getTokenAtPosition(
return nodeList
}

nodeVisitor := getNodeVisitor(visitNode, visitNodeList)

for {
VisitEachChildAndJSDoc(current, sourceFile, nodeVisitor)
VisitEachChildAndJSDoc(current, sourceFile, visitNode, visitNodeList)
// If prevSubtree was set on the last iteration, it ends at the target position.
// Check if the rightmost token of prevSubtree should be returned based on the
// `includePrecedingTokenAtEndPosition` callback.
Expand Down Expand Up @@ -217,7 +215,13 @@ func findRightmostNode(node *ast.Node) *ast.Node {
}
}

func VisitEachChildAndJSDoc(node *ast.Node, sourceFile *ast.SourceFile, visitor *ast.NodeVisitor) {
func VisitEachChildAndJSDoc(
node *ast.Node,
sourceFile *ast.SourceFile,
visitNode func(*ast.Node, *ast.NodeVisitor) *ast.Node,
visitNodes func(*ast.NodeList, *ast.NodeVisitor) *ast.NodeList,
) {
visitor := getNodeVisitor(visitNode, visitNodes)
if node.Flags&ast.NodeFlagsHasJSDoc != 0 {
for _, jsdoc := range node.JSDoc(sourceFile) {
if visitor.Hooks.VisitNode != nil {
Expand Down Expand Up @@ -275,9 +279,6 @@ func FindPrecedingTokenEx(sourceFile *ast.SourceFile, position int, startNode *a
}
if nodeList != nil && len(nodeList.Nodes) > 0 {
nodes := nodeList.Nodes
if ast.IsJSDocSingleCommentNodeList(n, nodeList) {
return nodeList
}
index, match := core.BinarySearchUniqueFunc(nodes, func(middle int, _ *ast.Node) int {
// synthetic jsdoc nodes should have jsdocNode.End() <= n.Pos()
if nodes[middle].Flags&ast.NodeFlagsReparsed != 0 {
Expand Down Expand Up @@ -308,8 +309,7 @@ func FindPrecedingTokenEx(sourceFile *ast.SourceFile, position int, startNode *a
}
return nodeList
}
nodeVisitor := getNodeVisitor(visitNode, visitNodes)
VisitEachChildAndJSDoc(n, sourceFile, nodeVisitor)
VisitEachChildAndJSDoc(n, sourceFile, visitNode, visitNodes)

if foundChild != nil {
// Note that the span of a node's tokens is [getStartOfNode(node, ...), node.end).
Expand Down Expand Up @@ -420,9 +420,6 @@ func findRightmostValidToken(endPos int, sourceFile *ast.SourceFile, containingN
}
visitNodes := func(nodeList *ast.NodeList, _ *ast.NodeVisitor) *ast.NodeList {
if nodeList != nil && len(nodeList.Nodes) > 0 {
if ast.IsJSDocSingleCommentNodeList(n, nodeList) {
return nodeList
}
hasChildren = true
index, _ := core.BinarySearchUniqueFunc(nodeList.Nodes, func(middle int, node *ast.Node) int {
if node.End() > endPos {
Expand Down Expand Up @@ -450,8 +447,7 @@ func findRightmostValidToken(endPos int, sourceFile *ast.SourceFile, containingN
}
return nodeList
}
nodeVisitor := getNodeVisitor(visitNode, visitNodes)
VisitEachChildAndJSDoc(n, sourceFile, nodeVisitor)
VisitEachChildAndJSDoc(n, sourceFile, visitNode, visitNodes)

// Three cases:
// 1. The answer is a token of `rightmostValidNode`.
Expand Down Expand Up @@ -563,8 +559,7 @@ func FindNextToken(previousToken *ast.Node, parent *ast.Node, file *ast.SourceFi
}
return nodeList
}
nodeVisitor := getNodeVisitor(visitNode, visitNodes)
VisitEachChildAndJSDoc(n, file, nodeVisitor)
VisitEachChildAndJSDoc(n, file, visitNode, visitNodes)
// Cases:
// 1. no answer exists
// 2. answer is an unvisited token
Expand Down Expand Up @@ -597,13 +592,33 @@ func getNodeVisitor(
visitNode func(*ast.Node, *ast.NodeVisitor) *ast.Node,
visitNodes func(*ast.NodeList, *ast.NodeVisitor) *ast.NodeList,
) *ast.NodeVisitor {
var wrappedVisitNode func(*ast.Node, *ast.NodeVisitor) *ast.Node
var wrappedVisitNodes func(*ast.NodeList, *ast.NodeVisitor) *ast.NodeList
if visitNode != nil {
wrappedVisitNode = func(n *ast.Node, v *ast.NodeVisitor) *ast.Node {
if ast.IsJSDocSingleCommentNodeComment(n) {
return n
}
return visitNode(n, v)
}
}

if visitNodes != nil {
wrappedVisitNodes = func(n *ast.NodeList, v *ast.NodeVisitor) *ast.NodeList {
if ast.IsJSDocSingleCommentNodeList(n) {
return n
}
return visitNodes(n, v)
}
}

return ast.NewNodeVisitor(core.Identity, nil, ast.NodeVisitorHooks{
VisitNode: visitNode,
VisitToken: visitNode,
VisitNodes: visitNodes,
VisitNode: wrappedVisitNode,
VisitToken: wrappedVisitNode,
VisitNodes: wrappedVisitNodes,
VisitModifiers: func(modifiers *ast.ModifierList, visitor *ast.NodeVisitor) *ast.ModifierList {
if modifiers != nil {
visitNodes(&modifiers.NodeList, visitor)
wrappedVisitNodes(&modifiers.NodeList, visitor)
}
return modifiers
},
Expand Down
5 changes: 5 additions & 0 deletions internal/checker/printer.go
Original file line number Diff line number Diff line change
Expand Up @@ -372,3 +372,8 @@ func (c *Checker) formatUnionTypes(types []*Type) []*Type {
}
return result
}

func (c *Checker) TypeToTypeNode(t *Type, enclosingDeclaration *ast.Node, flags nodebuilder.Flags) *ast.TypeNode {
nodeBuilder := c.getNodeBuilder()
return nodeBuilder.TypeToTypeNode(t, enclosingDeclaration, flags, nodebuilder.InternalFlagsNone, nil)
}
6 changes: 3 additions & 3 deletions internal/format/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ func formatNodeLines(ctx context.Context, sourceFile *ast.SourceFile, node *ast.
return nil
}
tokenStart := scanner.GetTokenPosOfNode(node, sourceFile, false)
lineStart := getLineStartPositionForPosition(tokenStart, sourceFile)
lineStart := GetLineStartPositionForPosition(tokenStart, sourceFile)
span := core.NewTextRange(lineStart, node.End())
return FormatSpan(ctx, span, sourceFile, requestKind)
}
Expand All @@ -90,7 +90,7 @@ func FormatDocument(ctx context.Context, sourceFile *ast.SourceFile) []core.Text
}

func FormatSelection(ctx context.Context, sourceFile *ast.SourceFile, start int, end int) []core.TextChange {
return FormatSpan(ctx, core.NewTextRange(getLineStartPositionForPosition(start, sourceFile), end), sourceFile, FormatRequestKindFormatSelection)
return FormatSpan(ctx, core.NewTextRange(GetLineStartPositionForPosition(start, sourceFile), end), sourceFile, FormatRequestKindFormatSelection)
}

func FormatOnOpeningCurly(ctx context.Context, sourceFile *ast.SourceFile, position int) []core.TextChange {
Expand All @@ -112,7 +112,7 @@ func FormatOnOpeningCurly(ctx context.Context, sourceFile *ast.SourceFile, posit
* ```
* and we wouldn't want to move the closing brace.
*/
textRange := core.NewTextRange(getLineStartPositionForPosition(scanner.GetTokenPosOfNode(outermostNode, sourceFile, false), sourceFile), position)
textRange := core.NewTextRange(GetLineStartPositionForPosition(scanner.GetTokenPosOfNode(outermostNode, sourceFile, false), sourceFile), position)
return FormatSpan(ctx, textRange, sourceFile, FormatRequestKindFormatOnOpeningCurlyBrace)
}

Expand Down
4 changes: 2 additions & 2 deletions internal/format/span.go
Original file line number Diff line number Diff line change
Expand Up @@ -467,7 +467,7 @@ func (w *formatSpanWorker) processChildNodes(
// }: {};
indentationOnListStartToken = w.indentationOnLastIndentedLine
} else {
startLinePosition := getLineStartPositionForPosition(tokenInfo.token.Loc.Pos(), w.sourceFile)
startLinePosition := GetLineStartPositionForPosition(tokenInfo.token.Loc.Pos(), w.sourceFile)
indentationOnListStartToken = findFirstNonWhitespaceColumn(startLinePosition, tokenInfo.token.Loc.Pos(), w.sourceFile, w.formattingContext.Options)
}

Expand Down Expand Up @@ -577,7 +577,7 @@ func (w *formatSpanWorker) tryComputeIndentationForListItem(startPos int, endPos
}
} else {
startLine, _ := scanner.GetLineAndCharacterOfPosition(w.sourceFile, startPos)
startLinePosition := getLineStartPositionForPosition(startPos, w.sourceFile)
startLinePosition := GetLineStartPositionForPosition(startPos, w.sourceFile)
column := findFirstNonWhitespaceColumn(startLinePosition, startPos, w.sourceFile, w.formattingContext.Options)
if startLine != parentStartLine || startPos == column {
// Use the base indent size if it is greater than
Expand Down
2 changes: 1 addition & 1 deletion internal/format/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ func getCloseTokenForOpenToken(kind ast.Kind) ast.Kind {
return ast.KindUnknown
}

func getLineStartPositionForPosition(position int, sourceFile *ast.SourceFile) int {
func GetLineStartPositionForPosition(position int, sourceFile *ast.SourceFile) int {
lineStarts := scanner.GetLineStarts(sourceFile)
line, _ := scanner.GetLineAndCharacterOfPosition(sourceFile, position)
return int(lineStarts[line])
Expand Down
18 changes: 0 additions & 18 deletions internal/fourslash/_scripts/failingTests.txt
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,8 @@ TestCompletionImportModuleSpecifierEndingTsxReact
TestCompletionImportModuleSpecifierEndingUnsupportedExtension
TestCompletionInFunctionLikeBody_includesPrimitiveTypes
TestCompletionInJsDoc
TestCompletionInJsDocQualifiedNames
TestCompletionInNamedImportLocation
TestCompletionInUncheckedJSFile
TestCompletionJSDocNamePath
TestCompletionListBuilderLocations_VariableDeclarations
TestCompletionListForDerivedType1
TestCompletionListForTransitivelyExportedMembers04
Expand Down Expand Up @@ -131,10 +129,6 @@ TestCompletionsJSDocImportTagAttributesEmptyModuleSpecifier1
TestCompletionsJSDocImportTagAttributesErrorModuleSpecifier1
TestCompletionsJSDocImportTagEmptyModuleSpecifier1
TestCompletionsJSDocNoCrash1
TestCompletionsJSDocNoCrash2
TestCompletionsJSDocNoCrash3
TestCompletionsJsdocParamTypeBeforeName
TestCompletionsJsdocTag
TestCompletionsJsdocTypeTagCast
TestCompletionsJsxAttribute2
TestCompletionsJsxAttributeInitializer2
Expand Down Expand Up @@ -247,7 +241,6 @@ TestJsDocFunctionSignatures12
TestJsDocFunctionSignatures13
TestJsDocFunctionSignatures7
TestJsDocFunctionSignatures8
TestJsDocFunctionTypeCompletionsNoCrash
TestJsDocGenerics2
TestJsDocInheritDoc
TestJsDocPropertyDescription1
Expand All @@ -266,26 +259,15 @@ TestJsDocTagsWithHyphen
TestJsQuickInfoGenerallyAcceptableSize
TestJsRequireQuickInfo
TestJsdocCallbackTag
TestJsdocExtendsTagCompletion
TestJsdocImplementsTagCompletion
TestJsdocImportTagCompletion1
TestJsdocLink2
TestJsdocLink3
TestJsdocLink6
TestJsdocLink_findAllReferences1
TestJsdocOverloadTagCompletion
TestJsdocParameterNameCompletion
TestJsdocPropTagCompletion
TestJsdocPropertyTagCompletion
TestJsdocSatisfiesTagCompletion1
TestJsdocSatisfiesTagCompletion2
TestJsdocTemplatePrototypeCompletions
TestJsdocTemplateTagCompletion
TestJsdocThrowsTagCompletion
TestJsdocTypedefTag
TestJsdocTypedefTag2
TestJsdocTypedefTagNamespace
TestJsdocTypedefTagTypeExpressionCompletion
TestJsxFindAllReferencesOnRuntimeImportWithPaths1
TestLetQuickInfoAndCompletionList
TestLocalFunction
Expand Down
Loading
Loading