Full JSDoc support in LSP #1702
Conversation
ahejlsberg
requested review from
DanielRosenwasser,
Copilot,
jakebailey and
sandersn
and removed request for
Copilot
|
|
||
| // If the given node is a declaration name node in a JSDoc comment that is subject to reparsing, return the declaration name node | ||
| // for the corresponding reparsed construct. Otherwise, just return the node. | ||
| func getReparsedNodeForNode(node *ast.Node) *ast.Node { |
There was a problem hiding this comment.
Is this a replacement for the bidirectional mapping plan we had originally conceived?
There was a problem hiding this comment.
Yeah, this is reasonably performant and not used in anything time critical. I haven't really seen a need for anything else.
| } | ||
| // no other meta properties are valid syntax, thus no others should have symbols | ||
| return nil | ||
| } else if ast.IsJSDocParameterTag(parent) && parent.Name() == node { |
There was a problem hiding this comment.
Is it odd to have this one special case here in the checker? I don't think we have any cases like this elsewhere after the reparser change.
There was a problem hiding this comment.
We do have a few. We used to have logic in the binder that would bind declaration names in JSDoc nodes, but we don't do that anymore (which is good). The logic also can't be in getReparsedNodeForNode because that function is supposed to preserve the location of the node (it basically just returns the cloned copy of an identifier created by re-parse). So, I think it's in the right place.
There was a problem hiding this comment.
Pull Request Overview
This PR implements comprehensive JSDoc support for the TypeScript language server, enabling hover, find-all-references, go-to-definition, and rename functionality to work properly with JSDoc annotations in both TypeScript and JavaScript files.
Key changes include:
- Updated parser to correctly position JSDoc elements for improved navigation
- Enhanced language service utilities to handle JSDoc name references
- Added support for reparsed JSDoc nodes in various language features
- Modified checker to better resolve JSDoc member name references
Reviewed Changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
testdata/baselines/reference/fourslash/hover/* |
Test baseline updates showing hover information now working for JSDoc typedef, callback, and type references |
testdata/baselines/reference/fourslash/goToDef/* |
Test baseline updates showing go-to-definition now working for JSDoc @see tags and @link references |
testdata/baselines/reference/fourslash/findAllRef/* |
Test baseline updates showing find-all-references now working across JSDoc annotations |
internal/parser/reparser.go |
Fixed JSDoc signature positioning and property name cloning in type literals |
internal/parser/jsdoc.go |
Corrected positioning for JSDoc type literals and callback signatures |
internal/ls/utilities.go |
Enhanced meaning resolution to support JSDoc name reference contexts |
internal/ls/symbols.go |
Added filtering to exclude reparsed nodes from document symbols |
internal/checker/checker.go |
Improved JSDoc member name resolution logic |
internal/astnav/tokens.go |
Added support for finding reparsed nodes for JSDoc declarations |
internal/ast/utilities.go |
Added JSDoc name reference context detection and utility functions |
internal/ast/ast.go |
Added helper functions and type predicates for JSDoc nodes |
| if symbol := c.resolveEntityName(name, meaning, true /*ignoreErrors*/, true /*dontResolveAlias*/, location); symbol != nil { | ||
| return symbol | ||
| } |
There was a problem hiding this comment.
[nitpick] The early return pattern introduced here removes the original variable assignment and changes the control flow. While functionally correct, this creates inconsistency with the original nested if-else structure that was handling the symbol resolution. Consider maintaining the original structure for better code readability and consistency.
Copilot uses AI. Check for mistakes.
| if tag.Kind == ast.KindJSDocOverloadTag { | ||
| loc = tag.AsJSDocOverloadTag().TagName |
There was a problem hiding this comment.
The variable assignment has changed from tag to jsSignature, but the conditional check on line 195-197 still references tag. This creates a potential inconsistency where loc might not match the intended logic in the conditional block.
| if tag.Kind == ast.KindJSDocOverloadTag { | |
| loc = tag.AsJSDocOverloadTag().TagName | |
| if jsSignature.Kind == ast.KindJSDocOverloadTag { | |
| loc = jsSignature.AsJSDocOverloadTag().TagName |
Copilot uses AI. Check for mistakes.
2 participants
This PR implements full support for JSDoc in the language service such that hover, find-all-references, go-to-definition, and rename all work properly with JSDoc annotated code (be that in .ts or .js files).