-
Notifications
You must be signed in to change notification settings - Fork 322
Fetch full documentation in code completion #2207
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?
Fetch full documentation in code completion #2207
Conversation
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.
Very cool stuff. Excited to see this. I left a couple comments inline.
Sources/SwiftSourceKitPlugin/ASTCompletion/CompletionSession.swift
Outdated
Show resolved
Hide resolved
Tests/SwiftSourceKitPluginTests/SwiftSourceKitPluginTests.swift
Outdated
Show resolved
Hide resolved
…n sourcekitd" This reverts commit db44320.
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.
Thanks, looks good to me, the only open question is whether we want to return both the brief and the full documentation from the plugin.
Also: Could you format your changes using swift-format as described in https://github.com/swiftlang/sourcekit-lsp/blob/main/CONTRIBUTING.md#formatting?
I have added one more functionality change. I found that existing LSP implementations (mainly Clang and TypeScript) don't show the declaration itself as part of the full documentation in completion (contrary to the hover request!) and I think the reason for this is that the completion item already describes the declaration and typically the popup showing the documentation is small so it makes sense to show the documentation comment right away. @ahoppen @hamishknight Can you please recheck? 🙏🏼 |
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.
Looks great to me. Thank you 🚀
@swift-ci Please test |
CI failed because Swift CI unconditionally enables all tests that are guarded by |
@ahoppen Yes, I think that makes sense. I wonder where in the console output did you find this though 😅 Edit: I found it by downloading the plain text output and using |
No, I always download the log files as well. |
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.
Woo! This is awesome, thanks @a7medev!
As of now, SourceKit-LSP doesn't cache completion item documentation either, should we introduce a new full documentation cache (e.g. using LRUCache)?
It's probably worth merging this, seeing how it performs, and only adding the cache if we need it. If we do then it'll be a little more than just caching them as they're retrieved, since if that's too slow then getting them in the first place is as well 😅 (in which case we might need to pre-fetch a bunch).
c0b4ca2
to
0bcd943
Compare
We've found that the XML to markdown implementation has some problems. Consider this example: /// Adds two integers together
/// A really interesting function, no?
///
/// Usage:
/// ```swift
/// add(x: 14, y: 22)
/// ```
///
/// - Parameters:
/// - x: First arg
/// - y: Second arg
/// - Returns: Sum
func add(x: Int, y: Int) -> Int {
return x + y
} The full documentation for add is shown like this in VS Code: this has some issues:
If we just output the raw comment instead, it looks like this. Which looks a lot nicer. The downside of course is losing the structured format that the XML-based comment has so we should ultimately switch back to using the XML to markdown approach after improving it. I agreed with @hamishknight on sticking to the raw comment for now to follow what @hamishknight @ahoppen @bnbarham Can you please re-review the PR after these changes? 🙏🏼 |
Depends on swiftlang/swift#82464
When resolving documentation for code completion items, we fetch full documentation through the newly added
swiftide_completion_item_get_doc_full_copy
SourceKitD function, if not found we fallback to brief documentation as before usingswiftide_completion_item_get_doc_brief
.Note
Unlike brief documentation, SourceKitD doesn't cache full documentation for completion results to avoid bloating memory with a lot of large strings.
As of now, SourceKit-LSP doesn't cache completion item documentation either, should we introduce a new full documentation cache (e.g. using
LRUCache
)?