- Notifications
You must be signed in to change notification settings - Fork768
Fix missing JSDoc comments on destructured interface properties#2195
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pull request overview
This PR fixes an issue where destructured interface members were not showing their inherited JSDoc comments when hovering over them in the editor. The fix adds special handling for binding elements (variables created from destructuring) in the hover implementation to retrieve documentation from the corresponding property in the source type.
Key Changes
- Modified
getDocumentationFromDeclarationto detect binding elements and retrieve documentation from the interface property - Added helper function
findPropertyInTypeto handle union types when searching for properties - Updated function signature to pass symbol and location context
- Added comprehensive tests for both basic and renamed destructuring scenarios
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/ls/hover.go | Added binding element detection logic and helper function to find property documentation in source types, updated function signature |
| internal/ls/signaturehelp.go | Updated call togetDocumentationFromDeclaration with new signature |
| internal/fourslash/tests/destructuredInterfaceJSDoc_test.go | Added new tests for destructured interface JSDoc comments, including renamed properties |
| testdata/baselines/reference/fourslash/quickInfo/*.baseline | Updated 4 baseline tests to correctly show JSDoc comments for destructured properties |
jakebailey left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think this is not unreasonable; it is the same code, but things are shifted around in the Go version.
DanielRosenwasser left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@copilot address
| declare const fubar: FooBar; | ||
| const {/*1*/foo, /*2*/bar, /*3*/baz} = fubar; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Change the test case to use a renamed destructuring
const {/*1*/foo, /*2*/bar, /*3*/baz: /*4*/biz} = fubar;There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done inf6b3006. Updated the test to include renamed destructuring:const {foo, bar, baz: biz} = fubar; wherebiz is tested to ensure it shows the JSDoc comment frombaz.
| return"","" | ||
| } | ||
| returnquickInfo,l.getDocumentationFromDeclaration(c,declaration,contentFormat) | ||
| returnquickInfo,l.getDocumentationFromDeclaration(c,symbol,declaration,node,contentFormat) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not so familiar with this, so it could be right, but what I would say is that maybe this needs to be more general and look at all declarations nodes from a given symbol likegetDocumentationComment did in the original codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done inf6b3006. Made the implementation more general by iterating through allsymbol.Declarations (instead of just checkingsymbol.ValueDeclaration) to find binding elements. This matches the pattern in TypeScript wheregetDocumentationComment processes all declarations from a symbol.
…est with renamed destructuringCo-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
| const {/*1*/foo, /*2*/bar, baz: /*3*/biz} = fubar; | ||
| ` | ||
| f,done:=fourslash.NewFourslash(t,nil/*capabilities*/,content) | ||
| deferdone() | ||
| f.VerifyQuickInfoAt(t,"1","const foo: number","foo comment") | ||
| f.VerifyQuickInfoAt(t,"2","const bar: string","bar comment") | ||
| f.VerifyQuickInfoAt(t,"3","const biz: string","baz comment") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Add a marker beforebaz:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@copilot do it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done in8a13eab. Added marker beforebaz: to test that hovering over the property name shows(property) FooBar.baz: string with the JSDoc comment, while hovering overbiz showsconst biz: string with the same JSDoc comment.
Co-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
internal/ls/hover.go Outdated
| ifpropertySymbol!=nil&&propertySymbol.ValueDeclaration!=nil { | ||
| declaration=propertySymbol.ValueDeclaration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@copilot I was actually expecting all comments for a given symbol to be smushed together, but that's not what the old codebase does either.
So I think we actually should switch back to theValueDeclaration approach if we're doing anything like this.
Also, look for, or add, a test for quick info at1 and2.
interfaceFoo{/** This is bar from the interface */bar:string;/** This is baz from the interface */baz:number;}declarevarfoo:Foo;/** Comment on the variable statement. */const{/** Comment on bar destructuring. *//*1*/bar,/** Comment on baz destructuring. *//*2*/baz}=foo;
@copilot go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done in73c9497. Switched back to theValueDeclaration approach and restructured the logic to match TypeScript's behavior: first try to get JSDoc from the binding element itself, only fall back to the property's JSDoc if none is found.
Added testTestDestructuredWithOwnJSDoc that verifies the behavior when binding elements have JSDoc comments before them. The test shows that TypeScript doesn't currently attach JSDoc to individual binding elements in destructuring patterns, so bothbar andbaz correctly fall back to showing the interface property's JSDoc ("This is bar from the interface" and "This is baz from the interface").
…JSDocCo-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
Uh oh!
There was an error while loading.Please reload this page.
Fixed the issue where destructured interface members were not showing inherited JSDoc comments when hovering.
Summary
When hovering over destructured variables from interfaces (e.g.,
const {foo} = fubarwherefubarhas an interface type), the hover information was showing type information but omitting JSDoc comments from the source interface properties.Changes Made
getDocumentationFromDeclarationininternal/ls/hover.goto detect binding elements and retrieve documentation from the corresponding property in the source typefindPropertyInTypehelper function to handle union types properly (simplified usingIsUnion()andTypes()methods)internal/ls/signaturehelp.gowith new signatureTesting
TestDestructuredInterfaceJSDoc: Tests basic destructuring ({foo, bar}) and renamed destructuring ({baz: biz})TestDestructuredInterfaceJSDocWithRename: Tests renamed destructuring with JSDoc inheritanceTestDestructuredWithOwnJSDoc: Verifies behavior when binding elements have their own JSDoc (demonstrates current TypeScript behavior where binding element JSDoc is not attached)Implementation Details
The fix matches TypeScript's reference implementation in
symbolDisplay.ts(lines 787-799). The implementation:ValueDeclarationfor binding element detection (per code review feedback)Original prompt
✨ Let Copilot coding agentset things up for you — coding agent works faster and does higher quality work when set up for your repo.