Fix hover JSDoc for mapped type properties#3663
Fix hover JSDoc for mapped type properties#3663Andarist wants to merge 3 commits intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes hover/QuickInfo documentation for mapped type properties by sourcing JSDoc from the originating (“root”) symbols, aligning behavior with TypeScript’s expectations and updating baselines/tests accordingly.
Changes:
- Add a root-symbol documentation lookup path for hover text in the language service.
- Update an existing QuickInfo baseline to include the expected JSDoc in hover output.
- Add a new fourslash test covering mapped type property JSDoc and un-skip previously failing QuickInfo tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| testdata/baselines/reference/fourslash/quickInfo/quickInfoOnUnionPropertiesWithIdenticalJSDocComments01.baseline | Updates expected hover/QuickInfo output to include JSDoc text. |
| internal/ls/hover.go | Adds documentationFromRootSymbols and uses it to populate documentation for mapped/derived symbols. |
| internal/fourslash/tests/quickInfoMappedTypeJSDoc_test.go | Adds coverage ensuring mapped type properties show original JSDoc in QuickInfo. |
| internal/fourslash/_scripts/failingTests.txt | Removes entries for tests that now pass with the updated hover behavior. |
| for _, rootSymbol := range c.GetRootSymbols(symbol) { | ||
| if rootSymbol == nil { | ||
| continue | ||
| } | ||
| declaration := rootSymbol.ValueDeclaration | ||
| if declaration == nil { | ||
| declaration = core.FirstOrNil(rootSymbol.Declarations) | ||
| } | ||
| if declaration == nil { | ||
| continue | ||
| } | ||
| if documentation := l.getDocumentationFromDeclaration(c, rootSymbol, declaration, node, contentFormat, false /*commentOnly*/); documentation != "" { | ||
| return documentation | ||
| } | ||
| } |
There was a problem hiding this comment.
GetRootSymbols(symbol) can return multiple candidates (e.g., union/intersection properties). Returning the first non-empty documentation can yield incorrect or non-deterministic hover text when root symbols have differing JSDoc. Consider only returning documentation when it is unique/consistent across all root symbols (e.g., collect non-empty docs and return it only if they’re all identical), otherwise fall back to the existing alias/documentation path.
There was a problem hiding this comment.
Hm, actually in Strada all declarations (with some deduplication) contribute to the documentation: https://github.com/microsoft/TypeScript/blob/f350b52331494b68c90ab02e2b6d0828d2a22a74/src/services/jsDoc.ts#L187-L221
…ymbol # Conflicts: # testdata/baselines/reference/fourslash/quickInfo/quickInfoOnUnionPropertiesWithIdenticalJSDocComments01.baseline
fixes #3659