Skip to content

Commit b59ddfc

Browse files
Copilotandrewbranch
Copilot
and
andrewbranch
committed
Fix JSDoc type assertion panic properly by handling reparsed nodes
The issue was that JSDoc type assertions like /**@type {string}*/(x) create a ParenthesizedExpression with a reparsed AsExpression child. The visitNode function was skipping all reparsed nodes unconditionally, causing the scanner to be used as a fallback and finding the identifier in what it thought was trivia. The fix tracks reparsed node matches as a fallback and uses them only when no non-reparsed match is found. This allows navigation through JSDoc type assertions while maintaining the invariant that identifiers never appear in actual trivia. This approach matches how visitNodeList handles reparsed nodes and maintains backward compatibility with all existing code. Co-authored-by: andrewbranch <[email protected]>
1 parent cbe994f commit b59ddfc

File tree

2 files changed

+216
-17
lines changed

2 files changed

+216
-17
lines changed

‎copilot-notes.md‎

Lines changed: 178 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,178 @@
1+
# Investigation: JSDoc Type Assertion Panic in getTokenAtPosition
2+
3+
## Problem Statement
4+
5+
The `getTokenAtPosition` function panics with the message:
6+
```
7+
panic: did not expect KindParenthesizedExpression to have KindIdentifier in its trivia
8+
```
9+
10+
When processing code like:
11+
```javascript
12+
function foo(x) {
13+
const s = /**@type {string}*/(x) // Go-to-definition on x causes panic
14+
}
15+
```
16+
17+
## Initial (Incorrect) Fix
18+
19+
My initial fix simply removed the panic and returned `current` when an identifier was found during trivia scanning. However, @andrewbranch correctly pointed out that:
20+
- Identifiers are NEVER allowed in the trivia of nodes
21+
- JSDoc nodes should be fully-formed subtrees with their identifiers accounted for in the AST
22+
- The panic is there for a reason and should not be removed
23+
24+
## Investigation Plan
25+
26+
1. Understand how JSDoc nodes are structured in the AST
27+
2. Compare working JSDoc cases (that don't panic) with the failing JSDoc type assertion case
28+
3. Identify why the identifier is appearing in trivia for type assertions but not other JSDoc constructs
29+
4. Find the root cause and proper fix
30+
31+
## Step 1: Understanding the AST Structure
32+
33+
From the test output, here's what I observed:
34+
35+
### Case 1: JSDoc Type Assertion (The Failing Case)
36+
```
37+
KindParenthesizedExpression [31..54) Flags=0x210000
38+
JSDoc: KindJSDoc [31..51)
39+
KindAsExpression [52..53)
40+
KindIdentifier [52..53)
41+
KindStringKeyword [42..48)
42+
```
43+
44+
Key observations:
45+
- The `ParenthesizedExpression` has the `HasJSDoc` flag (0x200000 in Flags=0x210000)
46+
- The JSDoc is at [31..51), which includes the `/**@type {string}*/` part
47+
- The JSDoc is attached as metadata to the `ParenthesizedExpression`
48+
- The `AsExpression` starts at position 52 (the identifier `x`)
49+
- The `StringKeyword` at [42..48) is from inside the JSDoc comment
50+
51+
### Case 2: Regular JSDoc Comment (Works Fine)
52+
```
53+
KindFunctionDeclaration [0..62) Flags=0x210000
54+
JSDoc: KindJSDoc [0..28)
55+
KindIdentifier [37..41)
56+
KindParameter [42..43)
57+
KindIdentifier [42..43)
58+
KindStringKeyword [15..21)
59+
```
60+
61+
Key observations:
62+
- The JSDoc is properly attached to the `FunctionDeclaration`
63+
- The `StringKeyword` at [15..21) is also from the JSDoc
64+
- The parameter identifier at [42..43) is a separate node
65+
66+
## Step 2: Identifying the Problem
67+
68+
**ROOT CAUSE FOUND!**
69+
70+
When `getTokenAtPosition` is called with position 52 (the identifier `x`):
71+
72+
1. It starts at `ParenthesizedExpression` [31..54)
73+
2. Calls `VisitEachChildAndJSDoc`, which:
74+
- First visits JSDoc children if `HasJSDoc` flag is set (the JSDoc is at [31..51), doesn't contain position 52)
75+
- Then visits regular children via `VisitEachChild` (the `AsExpression` at [52..53))
76+
3. The `AsExpression` IS visited, and `testNode` would return 0 (match)
77+
4. **BUT** - the `visitNode` function has this check:
78+
```go
79+
if node != nil && node.Flags&ast.NodeFlagsReparsed == 0 && next == nil {
80+
```
81+
5. The `AsExpression` has Flags=0x10008, which includes `NodeFlagsReparsed` (0x8)
82+
6. Because the Reparsed flag is set, `visitNode` **skips the AsExpression entirely**!
83+
7. Since no child is found, the function falls back to using the scanner
84+
8. The scanner scans from `left` (which is after the JSDoc) and encounters the identifier `x`
85+
9. The code panics because it doesn't expect an identifier in trivia
86+
87+
## Step 3: Understanding "Reparsed" Nodes
88+
89+
The `NodeFlagsReparsed` flag is used to mark nodes that are created from reparsing JSDoc comments. When a JSDoc type annotation like `/**@type {string}*/` is encountered, it gets "reparsed" into proper AST nodes (in this case, an `AsExpression`).
90+
91+
The problem is that `getTokenAtPosition` explicitly skips reparsed nodes during traversal:
92+
```go
93+
if node != nil && node.Flags&ast.NodeFlagsReparsed == 0 && next == nil {
94+
```
95+
96+
This is intentional - reparsed nodes represent synthetic AST nodes created from JSDoc, and their positions overlap with the JSDoc comment text. The code is designed to skip them to avoid confusion.
97+
98+
However, in the case of JSDoc type assertions, the `AsExpression` is the ONLY child of the `ParenthesizedExpression` (besides the JSDoc itself). When it's skipped, there's no other child to navigate to, so the scanner kicks in and finds the identifier.
99+
100+
## Step 4: Comparing with Working JSDoc Cases
101+
102+
Let me check how other JSDoc cases work:
103+
104+
**Case 2: Regular @param tag**
105+
```
106+
KindFunctionDeclaration [0..62)
107+
JSDoc: KindJSDoc [0..28)
108+
KindParameter [42..43)
109+
KindIdentifier [42..43)
110+
KindStringKeyword [15..21) <-- Reparsed node, but it's inside the Parameter
111+
```
112+
113+
In this case, the `StringKeyword` (the type from JSDoc) is a child of the `Parameter` node. When we navigate to the Parameter node, we have a real, non-reparsed identifier at [42..43) that we can find. The reparsed `StringKeyword` is a sibling, not the only path to the identifier.
114+
115+
**Case 1: JSDoc type assertion (failing)**
116+
```
117+
KindParenthesizedExpression [31..54)
118+
JSDoc: KindJSDoc [31..51)
119+
KindAsExpression [52..53) <-- Reparsed node, ONLY child
120+
KindIdentifier [52..53)
121+
```
122+
123+
In this case, the `AsExpression` IS the only child (besides the JSDoc). When it's skipped, there's NO other path to the identifier.
124+
125+
## Step 5: The Solution
126+
127+
The issue is that when navigating into a JSDoc type assertion, we need to be able to navigate through reparsed nodes to reach the actual identifiers within them. The current code correctly skips reparsed nodes to avoid returning positions within JSDoc comments, but it doesn't handle the case where the reparsed node is the ONLY way to reach a real token.
128+
129+
Looking at `visitNodeList`, there's already special handling for reparsed nodes:
130+
```go
131+
if match && nodes[index].Flags&ast.NodeFlagsReparsed != 0 {
132+
// filter and search again
133+
nodes = core.Filter(nodes, func(node *ast.Node) bool {
134+
return node.Flags&ast.NodeFlagsReparsed == 0
135+
})
136+
// ... search again without reparsed nodes
137+
}
138+
```
139+
140+
The problem is that `visitNode` (used for single children) doesn't have this fallback logic. It just skips reparsed nodes unconditionally.
141+
142+
**The Fix:** Modify `visitNode` to match the behavior of `visitNodeList`. When a reparsed node is encountered that would match the position, we should still process it if there are no non-reparsed alternatives. This allows navigation through JSDoc type assertions while maintaining the invariant that identifiers never appear in trivia.
143+
144+
Specifically, the fix is to:
145+
1. When `visitNode` encounters a reparsed node that matches, don't immediately skip it - instead, save it in a `reparsedNext` variable
146+
2. After visiting all children, if no non-reparsed match was found (`next == nil`), use the reparsed match
147+
3. Reset `reparsedNext` at the end of each iteration
148+
149+
This way:
150+
- Normal cases continue to prefer non-reparsed nodes (e.g., when a parameter has both a JSDoc type and a real identifier)
151+
- JSDoc type assertion cases can navigate through the reparsed AsExpression to reach the identifier
152+
- The invariant is maintained: we never return an identifier found in trivia
153+
154+
## Implementation
155+
156+
The changes made to `internal/astnav/tokens.go`:
157+
158+
1. Added `reparsedNext` variable to track reparsed node matches as a fallback
159+
2. Modified `visitNode` to:
160+
- Still test reparsed nodes
161+
- Save matching reparsed nodes in `reparsedNext` instead of skipping them entirely
162+
- Prefer non-reparsed nodes by only setting `next` for non-reparsed matches
163+
3. Added logic after `VisitEachChildAndJSDoc` to use `reparsedNext` if `next` is nil
164+
4. Reset both `next` and `reparsedNext` at the end of each loop iteration
165+
166+
This maintains backward compatibility while fixing the JSDoc type assertion panic. The panic condition is kept intact - identifiers should never appear in trivia.
167+
168+
## Testing
169+
170+
All existing tests pass, including:
171+
- The new test cases for JSDoc type assertions
172+
- All baseline tests for `GetTokenAtPosition` and `GetTouchingPropertyName`
173+
- All other astnav tests
174+
175+
The fix correctly handles:
176+
- JSDoc type assertions like `/**@type {string}*/(x)`
177+
- Regular JSDoc comments (unchanged behavior)
178+
- All other token position lookups (unchanged behavior)

‎internal/astnav/tokens.go‎

Lines changed: 38 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,10 @@ func getTokenAtPosition(
6060
// only if `includePrecedingTokenAtEndPosition` is provided. Once set, the next
6161
// iteration of the loop will test the rightmost token of `prevSubtree` to see
6262
// if it should be returned.
63-
var next, prevSubtree *ast.Node
63+
// `reparsedNext` tracks a reparsed node that matches, used as a fallback if no
64+
// non-reparsed node is found. This handles JSDoc type assertions where the
65+
// reparsed AsExpression is the only path to the identifier.
66+
var next, prevSubtree, reparsedNext *ast.Node
6467
current := sourceFile.AsNode()
6568
// `left` tracks the lower boundary of the node/token that could be returned,
6669
// and is eventually the scanner's start position, if the scanner is used.
@@ -87,19 +90,29 @@ func getTokenAtPosition(
8790
visitNode := func(node *ast.Node, _ *ast.NodeVisitor) *ast.Node {
8891
// We can't abort visiting children, so once a match is found, we set `next`
8992
// and do nothing on subsequent visits.
90-
if node != nil && node.Flags&ast.NodeFlagsReparsed == 0 && next == nil {
93+
if node != nil && next == nil {
9194
result := testNode(node)
92-
switch result {
93-
case -1:
94-
if !ast.IsJSDocKind(node.Kind) {
95-
// We can't move the left boundary into or beyond JSDoc,
96-
// because we may end up returning the token after this JSDoc,
97-
// constructing it with the scanner, and we need to include
98-
// all its leading trivia in its position.
99-
left = node.End()
95+
if node.Flags&ast.NodeFlagsReparsed != 0 {
96+
// This is a reparsed node (from JSDoc). We prefer non-reparsed nodes,
97+
// but track this as a fallback in case there are no non-reparsed matches.
98+
// This handles JSDoc type assertions like /**@type {string}*/(x) where
99+
// the AsExpression child is reparsed but is the only path to the identifier.
100+
if result == 0 && reparsedNext == nil {
101+
reparsedNext = node
102+
}
103+
} else {
104+
switch result {
105+
case -1:
106+
if !ast.IsJSDocKind(node.Kind) {
107+
// We can't move the left boundary into or beyond JSDoc,
108+
// because we may end up returning the token after this JSDoc,
109+
// constructing it with the scanner, and we need to include
110+
// all its leading trivia in its position.
111+
left = node.End()
112+
}
113+
case 0:
114+
next = node
100115
}
101-
case 0:
102-
next = node
103116
}
104117
}
105118
return node
@@ -160,6 +173,14 @@ func getTokenAtPosition(
160173
prevSubtree = nil
161174
}
162175

176+
// If no non-reparsed node was found but we have a reparsed match, use it.
177+
// This handles JSDoc type assertions where the only path to an identifier
178+
// is through a reparsed AsExpression node.
179+
if next == nil && reparsedNext != nil {
180+
next = reparsedNext
181+
reparsedNext = nil
182+
}
183+
163184
// No node was found that contains the target position, so we've gone as deep as
164185
// we can in the AST. We've either found a token, or we need to run the scanner
165186
// to construct one that isn't stored in the AST.
@@ -175,11 +196,10 @@ func getTokenAtPosition(
175196
tokenEnd := scanner.TokenEnd()
176197
if tokenStart <= position && (position < tokenEnd) {
177198
if token == ast.KindIdentifier || !ast.IsTokenKind(token) {
178-
// If we encounter an identifier or complex node while scanning, it means
179-
// the token is part of the current node's structure (even if not properly
180-
// visited as a child). This can happen with JSDoc type assertions and
181-
// other complex expressions. Return the current node as it contains the token.
182-
return current
199+
if ast.IsJSDocKind(current.Kind) {
200+
return current
201+
}
202+
panic(fmt.Sprintf("did not expect %s to have %s in its trivia", current.Kind.String(), token.String()))
183203
}
184204
return sourceFile.GetOrCreateToken(token, tokenFullStart, tokenEnd, current)
185205
}
@@ -197,6 +217,7 @@ func getTokenAtPosition(
197217
current = next
198218
left = current.Pos()
199219
next = nil
220+
reparsedNext = nil
200221
}
201222
}
202223

0 commit comments

Comments
 (0)