Fix panic in getTokenAtPosition for JSDoc type assertions

copilot-notes.md

@@ -0,0 +1,179 @@
+# Investigation: JSDoc Type Assertion Panic in getTokenAtPosition
+
+## Problem Statement
+
+The `getTokenAtPosition` function panics with the message:
+```
+panic: did not expect KindParenthesizedExpression to have KindIdentifier in its trivia
+```
+
+When processing code like:
+```javascript
+function foo(x) {
+    const s = /**@type {string}*/(x)  // Go-to-definition on x causes panic
+}
+```
+
+## Initial (Incorrect) Fix
+
+My initial fix simply removed the panic and returned `current` when an identifier was found during trivia scanning. However, @andrewbranch correctly pointed out that:
+- Identifiers are NEVER allowed in the trivia of nodes
+- JSDoc nodes should be fully-formed subtrees with their identifiers accounted for in the AST
+- The panic is there for a reason and should not be removed
+
+## Investigation Plan
+
+1. Understand how JSDoc nodes are structured in the AST
+2. Compare working JSDoc cases (that don't panic) with the failing JSDoc type assertion case
+3. Identify why the identifier is appearing in trivia for type assertions but not other JSDoc constructs
+4. Find the root cause and proper fix
+
+## Step 1: Understanding the AST Structure
+
+From the test output, here's what I observed:
+
+### Case 1: JSDoc Type Assertion (The Failing Case)
+```
+KindParenthesizedExpression [31..54) Flags=0x210000
+  JSDoc: KindJSDoc [31..51)
+  KindAsExpression [52..53)
+    KindIdentifier [52..53)
+    KindStringKeyword [42..48)
+```
+
+Key observations:
+- The `ParenthesizedExpression` has the `HasJSDoc` flag (0x200000 in Flags=0x210000)
+- The JSDoc is at [31..51), which includes the `/**@type {string}*/` part
+- The JSDoc is attached as metadata to the `ParenthesizedExpression`
+- The `AsExpression` starts at position 52 (the identifier `x`)
+- The `StringKeyword` at [42..48) is from inside the JSDoc comment
+
+### Case 2: Regular JSDoc Comment (Works Fine)
+```
+KindFunctionDeclaration [0..62) Flags=0x210000
+  JSDoc: KindJSDoc [0..28)
+  KindIdentifier [37..41)
+  KindParameter [42..43)
+    KindIdentifier [42..43)
+    KindStringKeyword [15..21)
+```
+
+Key observations:
+- The JSDoc is properly attached to the `FunctionDeclaration`
+- The `StringKeyword` at [15..21) is also from the JSDoc
+- The parameter identifier at [42..43) is a separate node
+
+## Step 2: Identifying the Problem
+
+**ROOT CAUSE FOUND!**
+
+When `getTokenAtPosition` is called with position 52 (the identifier `x`):
+
+1. It starts at `ParenthesizedExpression` [31..54)
+2. Calls `VisitEachChildAndJSDoc`, which:
+   - First visits JSDoc children if `HasJSDoc` flag is set (the JSDoc is at [31..51), doesn't contain position 52)
+   - Then visits regular children via `VisitEachChild` (the `AsExpression` at [52..53))
+3. The `AsExpression` IS visited, and `testNode` would return 0 (match)
+4. **BUT** - the `visitNode` function has this check:
+   ```go
+   if node != nil && node.Flags&ast.NodeFlagsReparsed == 0 && next == nil {
+   ```
+5. The `AsExpression` has Flags=0x10008, which includes `NodeFlagsReparsed` (0x8)
+6. Because the Reparsed flag is set, `visitNode` **skips the AsExpression entirely**!
+7. Since no child is found, the function falls back to using the scanner
+8. The scanner scans from `left` (which is after the JSDoc) and encounters the identifier `x`
+9. The code panics because it doesn't expect an identifier in trivia
+
+## Step 3: Understanding "Reparsed" Nodes
+
+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`).
+
+The problem is that `getTokenAtPosition` explicitly skips reparsed nodes during traversal:
+```go
+if node != nil && node.Flags&ast.NodeFlagsReparsed == 0 && next == nil {
+```
+
+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.
+
+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.
+
+## Step 4: Comparing with Working JSDoc Cases
+
+Let me check how other JSDoc cases work:
+
+**Case 2: Regular @param tag**
+```
+KindFunctionDeclaration [0..62)
+  JSDoc: KindJSDoc [0..28)
+  KindParameter [42..43)
+    KindIdentifier [42..43)
+    KindStringKeyword [15..21)  <-- Reparsed node, but it's inside the Parameter
+```
+
+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.
+
+**Case 1: JSDoc type assertion (failing)**
+```
+KindParenthesizedExpression [31..54)
+  JSDoc: KindJSDoc [31..51)
+  KindAsExpression [52..53)  <-- Reparsed node, ONLY child
+    KindIdentifier [52..53)
+```
+
+In this case, the `AsExpression` IS the only child (besides the JSDoc). When it's skipped, there's NO other path to the identifier.
+
+## Step 5: The Solution (Revised)
+
+After feedback from @andrewbranch, the solution was refined to be more targeted. Instead of a general fallback mechanism for all reparsed nodes, the fix specifically handles `AsExpression` and `SatisfiesExpression`.
+
+**Key Insight:** `AsExpression` and `SatisfiesExpression` are special cases among reparsed nodes. While most reparsed nodes are synthetic and exist outside the "real" tree with no real position in the file, these two node kinds can have the `Reparsed` flag when they come from JSDoc type assertions, but their `.Expression` child should still be visited.
+
+**The Fix:** Modify `visitNode` to special-case `AsExpression` and `SatisfiesExpression`:
+
+1. Check if a node is `AsExpression` or `SatisfiesExpression` when deciding whether to skip reparsed nodes
+2. When we navigate into one of these special nodes, set an `allowReparsed` flag that allows visiting all their reparsed children
+3. This allows recursive navigation through the reparsed tree structure to reach the actual identifier
+
+The logic:
+```go
+// Skip reparsed nodes unless:
+// 1. The node itself is AsExpression or SatisfiesExpression, OR
+// 2. We're already inside an AsExpression or SatisfiesExpression (allowReparsed=true)
+isSpecialReparsed := node.Flags&ast.NodeFlagsReparsed != 0 &&
+    (node.Kind == ast.KindAsExpression || node.Kind == ast.KindSatisfiesExpression)
+
+if node.Flags&ast.NodeFlagsReparsed == 0 || isSpecialReparsed || allowReparsed {
+    // Process the node
+}
+```
+
+When we navigate into an `AsExpression` or `SatisfiesExpression`, we set `allowReparsed = true` for the next iteration, which allows their reparsed children (like the identifier) to be visited.
+
+## Implementation
+
+The changes made to `internal/astnav/tokens.go`:
+
+1. Added `allowReparsed` flag to track when we're inside an AsExpression or SatisfiesExpression
+2. Modified `visitNode` to:
+   - Allow AsExpression and SatisfiesExpression nodes even if they're reparsed
+   - Allow any reparsed node if `allowReparsed` is true (we're inside a special node)
+3. Set `allowReparsed = true` when navigating into AsExpression or SatisfiesExpression
+
+This targeted approach:
+- Only affects the specific node types that need special handling
+- Maintains the strict reparsed node filtering for all other cases
+- Keeps the panic intact - identifiers should never appear in actual trivia
+- Maintains backward compatibility with all existing code
+
+## Testing
+
+All existing tests pass, including:
+- The new test cases for JSDoc type assertions
+- All baseline tests for `GetTokenAtPosition` and `GetTouchingPropertyName`
+- All other astnav tests
+
+The fix correctly handles:
+- JSDoc type assertions like `/**@type {string}*/(x)`
+- JSDoc satisfies expressions
+- Regular JSDoc comments (unchanged behavior)
+- All other token position lookups (unchanged behavior)

internal/astnav/tokens.go

@@ -65,6 +65,10 @@ func getTokenAtPosition(
 	// `left` tracks the lower boundary of the node/token that could be returned,
 	// and is eventually the scanner's start position, if the scanner is used.
 	left := 0
+	// `allowReparsed` is set when we're navigating inside an AsExpression or
+	// SatisfiesExpression, which allows visiting their reparsed children to reach
+	// the actual identifier from JSDoc type assertions.
+	allowReparsed := false
 
 	testNode := func(node *ast.Node) int {
 		if node.Kind != ast.KindEndOfFile && node.End() == position && includePrecedingTokenAtEndPosition != nil {
@@ -74,7 +78,8 @@ func getTokenAtPosition(
 		if node.End() < position || node.Kind != ast.KindEndOfFile && node.End() == position {
 			return -1
 		}
-		if getPosition(node, sourceFile, allowPositionInLeadingTrivia) > position {
+		nodePos := getPosition(node, sourceFile, allowPositionInLeadingTrivia)
+		if nodePos > position {
 			return 1
 		}
 		return 0
@@ -86,18 +91,29 @@ func getTokenAtPosition(
 	visitNode := func(node *ast.Node, _ *ast.NodeVisitor) *ast.Node {
 		// We can't abort visiting children, so once a match is found, we set `next`
 		// and do nothing on subsequent visits.
-		if node != nil && node.Flags&ast.NodeFlagsReparsed == 0 && next == nil {
-			switch testNode(node) {
-			case -1:
-				if !ast.IsJSDocKind(node.Kind) {
-					// We can't move the left boundary into or beyond JSDoc,
-					// because we may end up returning the token after this JSDoc,
-					// constructing it with the scanner, and we need to include
-					// all its leading trivia in its position.
-					left = node.End()
+		if node != nil && next == nil {
+			// Skip reparsed nodes unless:
+			// 1. The node itself is AsExpression or SatisfiesExpression, OR
+			// 2. We're already inside an AsExpression or SatisfiesExpression (allowReparsed=true)
+			// These are special cases where reparsed nodes from JSDoc type assertions
+			// should still be navigable to reach identifiers.
+			isSpecialReparsed := node.Flags&ast.NodeFlagsReparsed != 0 &&
+				(node.Kind == ast.KindAsExpression || node.Kind == ast.KindSatisfiesExpression)
+
+			if node.Flags&ast.NodeFlagsReparsed == 0 || isSpecialReparsed || allowReparsed {
+				result := testNode(node)
+				switch result {
+				case -1:
+					if !ast.IsJSDocKind(node.Kind) {
+						// We can't move the left boundary into or beyond JSDoc,
+						// because we may end up returning the token after this JSDoc,
+						// constructing it with the scanner, and we need to include
+						// all its leading trivia in its position.
+						left = node.End()
+					}
+				case 0:
+					next = node
 				}
-			case 0:
-				next = node
 			}
 		}
 		return node
@@ -194,6 +210,11 @@ func getTokenAtPosition(
 		current = next
 		left = current.Pos()
 		next = nil
+		// When navigating into AsExpression or SatisfiesExpression, allow visiting
+		// their reparsed children to reach identifiers from JSDoc type assertions.
+		if current.Kind == ast.KindAsExpression || current.Kind == ast.KindSatisfiesExpression {
+			allowReparsed = true
+		}
 	}
 }
 

internal/astnav/tokens_test.go

@@ -43,6 +43,52 @@ func TestGetTokenAtPosition(t *testing.T) {
 		)
 	})
 
+	t.Run("JSDoc type assertion", func(t *testing.T) {
+		t.Parallel()
+		fileText := `function foo(x) {
+    const s = /**@type {string}*/(x)
+}`
+		file := parser.ParseSourceFile(ast.SourceFileParseOptions{
+			FileName: "/test.js",
+			Path:     "/test.js",
+		}, fileText, core.ScriptKindJS)
+
+		// Position of 'x' inside the parenthesized expression (position 52)
+		position := 52
+
+		// This should not panic - it previously panicked with:
+		// "did not expect KindParenthesizedExpression to have KindIdentifier in its trivia"
+		token := astnav.GetTouchingPropertyName(file, position)
+		if token == nil {
+			t.Fatal("Expected to get a token, got nil")
+		}
+
+		// The function may return either the identifier itself or the containing
+		// parenthesized expression, depending on how the AST is structured
+		if token.Kind != ast.KindIdentifier && token.Kind != ast.KindParenthesizedExpression {
+			t.Errorf("Expected identifier or parenthesized expression, got %s", token.Kind)
+		}
+	})
+
+	t.Run("JSDoc type assertion with comment", func(t *testing.T) {
+		t.Parallel()
+		// Exact code from the issue report
+		fileText := `function foo(x) {
+    const s = /**@type {string}*/(x)  // Go-to-definition on x causes panic
+}`
+		file := parser.ParseSourceFile(ast.SourceFileParseOptions{
+			FileName: "/test.js",
+			Path:     "/test.js",
+		}, fileText, core.ScriptKindJS)
+
+		// Find position of 'x' in the type assertion
+		xPos := 52 // Position of 'x' in (x)
+
+		// This should not panic
+		token := astnav.GetTouchingPropertyName(file, xPos)
+		assert.Assert(t, token != nil, "Expected to get a token")
+	})
+
 	t.Run("pointer equality", func(t *testing.T) {
 		t.Parallel()
 		fileText := `