-
Notifications
You must be signed in to change notification settings - Fork 759
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 our terms of service and privacy 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?
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>
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 to getDocumentationFromDeclaration 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 in f6b3006. Updated the test to include renamed destructuring: const {foo, bar, baz: biz} = fubar; where biz is tested to ensure it shows the JSDoc comment from baz.
| return "", "" | ||
| } | ||
| return quickInfo, l.getDocumentationFromDeclaration(c, declaration, contentFormat) | ||
| return quickInfo, 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 like getDocumentationComment 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 in f6b3006. Made the implementation more general by iterating through all symbol.Declarations (instead of just checking symbol.ValueDeclaration) to find binding elements. This matches the pattern in TypeScript where getDocumentationComment processes all declarations from a symbol.
…est with renamed destructuring Co-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) | ||
| defer done() | ||
| 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 before baz:
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 in 8a13eab. Added marker before baz: to test that hovering over the property name shows (property) FooBar.baz: string with the JSDoc comment, while hovering over biz shows const biz: string with the same JSDoc comment.
Co-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
internal/ls/hover.go
Outdated
| if propertySymbol != 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 the ValueDeclaration approach if we're doing anything like this.
Also, look for, or add, a test for quick info at 1 and 2.
interface Foo {
/** This is bar from the interface */
bar: string;
/** This is baz from the interface */
baz: number;
}
declare var foo: 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 in 73c9497. Switched back to the ValueDeclaration 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 test TestDestructuredWithOwnJSDoc 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 both bar and baz correctly fall back to showing the interface property's JSDoc ("This is bar from the interface" and "This is baz from the interface").
…JSDoc Co-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
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 agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.