-
Notifications
You must be signed in to change notification settings - Fork 737
Fully resolve LSP client caps to non-pointers, pass by context #2095
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
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.
Pull Request Overview
This PR refactors LSP client capabilities handling by generating a fully "resolved" version of ClientCapabilities with all pointer fields converted to values, eliminating the need for repetitive nil checking throughout the codebase. The capabilities are now passed via context to LSP handlers instead of being passed as individual function parameters.
Key Changes:
- Generated
ResolvedClientCapabilitiestype and conversion function that dereferences all pointers with zero-value fallbacks - Implemented context-based capability access via
WithClientCapabilitiesandGetClientCapabilities - Removed numerous helper functions that performed nil-checking on capability fields
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
internal/lsp/lsproto/_generate/generate.mts |
Added generator logic to create resolved capability types and conversion functions with proper dependency ordering |
internal/lsp/lsproto/lsp_generated.go |
Generated resolved types for all capability structures with derefOr helper for safe pointer dereferencing |
internal/lsp/lsproto/lsp.go |
Added context utilities for storing/retrieving capabilities and PreferredMarkupKind helper function |
internal/lsp/server.go |
Resolved capabilities on initialization, added context passing in request handler, removed obsolete helper functions, includes workaround for diagnostic capability bug |
internal/ls/definition.go |
Updated to retrieve capability from context instead of function parameters |
internal/ls/diagnostics.go |
Simplified diagnostic conversion by accessing resolved capabilities from context |
internal/ls/completions.go |
Removed clientOptions parameters throughout, using context-based capability access instead |
internal/ls/hover.go |
Retrieves content format preference from context capabilities |
internal/ls/signaturehelp.go |
Removed capability parameters, using context-based access for documentation format |
internal/ls/symbols.go |
Accesses hierarchical symbol support directly from context capabilities |
internal/ls/string_completions.go |
Removed capability parameters, passing context instead |
internal/ls/findallreferences.go |
Simplified implementation lookup by accessing link support from context |
internal/project/untitled_test.go |
Updated test call to match new function signature without capability parameter |
| s.initializeParams.Capabilities.Workspace == nil || | ||
| s.initializeParams.Capabilities.Workspace.Diagnostics == nil || | ||
| !ptrIsTrue(s.initializeParams.Capabilities.Workspace.Diagnostics.RefreshSupport) { | ||
| if !s.clientCapabilities.Workspace.Diagnostics.RefreshSupport { |
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.
No ctx here, since this is not an LSP method, but rather a callback given to Session.
|
@sheetalkamat I'm sorry if this causes you to have to solve conflicts on #1991 again |
This PR modifies the LSP generator to also generate a version of
ClientCapabilitieswhich is fully "resolved", that is, free of pointers and safe to access all the way down to the deepest field, along with a function that does this. Oninitialized, we resolve the caps, fix up the weird diagnostic cap bug (for now), then save that result for later.This allows us to stop checking nil 1000 times over, eliminating a load of random helpers, if statements, etc, and preventing us from making the same mistake in the future.
This is however a lossy process; we won't know if something was
nilversus"", or something. But I do not believe his matters for anything in the caps, so I think the ergonomics is worth it.In addition, I've replaced the one-off passing of options with a context value; this is passed to LSP handler methods, replacing our very random use of capabilities (or, derived-from-capability values).