refactor: migrate lsp types #98
Conversation
WalkthroughMigrates the internal LSP integration from internal lsp_types to go.lsp.dev/protocol across core code and tests. Updates function signatures, type usages (Position, Range, TextEdit, DocumentURI, diagnostics, capabilities), and related helpers. Adjusts pointer vs value handling for ranges. No functional control-flow changes indicated. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Server as LSP Server
participant Protocol as go.lsp.dev/protocol types
Note over Server,Protocol: Types migrated from internal lsp_types → protocol
Client->>Server: initialize (InitializeParams)
Server-->>Client: InitializeResult (protocol.ServerCapabilities)
Client->>Server: textDocument/didOpen (DidOpenTextDocumentParams)
Server->>Server: updateDocument(uri: protocol.DocumentURI, text)
Server-->>Client: publishDiagnostics (PublishDiagnosticsParams)
Client->>Server: textDocument/hover (HoverParams)
Server-->>Client: Hover (protocol.Hover)
Client->>Server: textDocument/definition (DefinitionParams)
Server-->>Client: Location (protocol.Location)
Client->>Server: textDocument/documentSymbol (DocumentSymbolParams)
Server-->>Client: [DocumentSymbol]
Client->>Server: textDocument/codeAction (CodeActionParams)
Server-->>Client: [CodeAction] with WorkspaceEdit (protocol.TextEdit[])
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #98 +/- ##
==========================================
- Coverage 70.89% 70.86% -0.03%
==========================================
Files 42 42
Lines 4821 4823 +2
==========================================
Hits 3418 3418
- Misses 1247 1249 +2
Partials 156 156 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
internal/lsp/documents_store.go (1)
21-32: Prefer pointer receivers for Get/Set to avoid copying and clarify mutabilityUsing value receivers copies the struct on each call. Map and mutex pointer still reference shared state, but pointer receivers are idiomatic for mutable stores and avoid accidental copies.
Apply within this file:
-func (s documentStore[Doc]) Get(uri protocol.DocumentURI) (Doc, bool) { +func (s *documentStore[Doc]) Get(uri protocol.DocumentURI) (Doc, bool) { @@ -func (s documentStore[Doc]) Set(uri protocol.DocumentURI, doc Doc) { +func (s *documentStore[Doc]) Set(uri protocol.DocumentURI, doc Doc) {Optionally, store the mutex by value:
- mu *sync.RWMutex + mu sync.RWMutexAnd init:
- mu: &sync.RWMutex{}, + mu: sync.RWMutex{},Calls like
state.documents.Set(...)still work as the field is addressable.internal/lsp/lsp_test.go (1)
31-37: Make HoverProvider assertion robust to HoverOptionsField can be bool or HoverOptions. Current bool assertion will break if options are returned later.
Example:
- b, ok := init.Capabilities.HoverProvider.(bool) - require.True(t, ok, "cast init.Capabilities.HoverProvider to bool") - require.True(t, b) + switch v := init.Capabilities.HoverProvider.(type) { + case bool: + require.True(t, v) + case protocol.HoverOptions: + // ok + default: + t.Fatalf("unexpected HoverProvider type: %T", v) + }internal/lsp/codeactions_test.go (2)
178-190: Add simple bounds checks to avoid panics in edge casesDefensive guards prevent slice bounds panic if tests evolve.
func positionToOffset(lines []string, position protocol.Position) int { // TODO: check indexes are 0-based - offset := 0 - for _, line := range lines[0:position.Line] { + if int(position.Line) > len(lines) { + position.Line = protocol.UInteger(len(lines)) + } + offset := 0 + for _, line := range lines[0:position.Line] { // +1 for the newline which was trimmed in lines offset += len(line) + 1 } - offset += int(position.Character) + lineIdx := int(position.Line) + if lineIdx > 0 { + lineIdx-- + } + if lineIdx < len(lines) && int(position.Character) > len(lines[lineIdx]) { + position.Character = protocol.UInteger(len(lines[lineIdx])) + } + offset += int(position.Character) return offset }
192-202: Note: LSP positions are UTF‑16; this helper uses byte offsetsFine for ASCII test fixtures, but non‑ASCII could drift. Add a comment or restrict fixtures to ASCII to avoid confusion.
internal/lsp/handlers.go (2)
33-36: Pre‑allocate diagnostics slice capacityAvoids growth when pushing known count.
- var diagnostics = make([]protocol.Diagnostic, 0) + diagnostics := make([]protocol.Diagnostic, 0, len(checkResult.Diagnostics))
68-74: Use protocol.Markdown constant instead of raw stringPrevents typos and aligns with API constants.
- Contents: protocol.MarkupContent{ - Value: msg, - Kind: "markdown", - }, + Contents: protocol.MarkupContent{ + Value: msg, + Kind: protocol.Markdown, + },Also applies to: 108-114
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (6)
internal/lsp/code_actions.go(2 hunks)internal/lsp/codeactions_test.go(10 hunks)internal/lsp/documents_store.go(1 hunks)internal/lsp/handlers.go(8 hunks)internal/lsp/handlers_test.go(3 hunks)internal/lsp/lsp_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
internal/lsp/codeactions_test.go (3)
internal/analysis/diagnostic_kind.go (1)
DiagnosticKind(39-42)internal/parser/ast.go (1)
Program(325-329)internal/parser/range.go (2)
Position(8-11)Range(13-16)
internal/lsp/code_actions.go (3)
internal/analysis/diagnostic_kind.go (2)
UnboundVariable(91-94)UnboundVariable(101-103)internal/parser/ast.go (1)
Program(325-329)internal/parser/range.go (1)
Range(13-16)
internal/lsp/handlers_test.go (3)
internal/parser/range.go (1)
Position(8-11)internal/jsonrpc2/messages.go (1)
ResponseError(166-170)internal/lsp/handlers.go (1)
ParserToLspPosition(205-210)
internal/lsp/handlers.go (7)
internal/jsonrpc2/jsonrpc2.go (5)
Conn(23-32)NewNotificationHandler(101-131)SyncHandling(41-41)NewRequestHandler(48-96)AsyncHandling(42-42)internal/analysis/check.go (3)
CheckSource(296-304)CheckResult(125-140)Diagnostic(119-123)internal/parser/range.go (2)
Range(13-16)Position(8-11)internal/analysis/document_symbols.go (1)
DocumentSymbol(17-23)internal/analysis/diagnostic_kind.go (3)
UnboundVariable(91-94)UnboundVariable(101-103)Severity(12-12)internal/lsp/code_actions.go (1)
CreateVar(11-43)internal/parser/ast.go (1)
Program(325-329)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Tests
- GitHub Check: Dirty
🔇 Additional comments (4)
internal/lsp/code_actions.go (1)
11-27: LGTM on the migration to protocol typesSignatures and Range handling look correct; zero Range for insert when no vars block is appropriate.
Also applies to: 35-41
internal/lsp/handlers_test.go (1)
113-130: LGTM on protocol migration in test clientRequests/params built with protocol types correctly; URI casting and positions conversion are consistent.
Also applies to: 132-154
internal/lsp/handlers.go (2)
228-243: InitializeResult looks good; small nitCapabilities look correct. Consider returning HoverOptions later; tests are currently asserting bool. See lsp_test.go note.
151-154: Verify internal enum casts align with LSP specification
handlers.go (lines 151–154, 222–223): raw casts to protocol.SymbolKind and protocol.DiagnosticSeverity assume identical numeric values; manually confirm the external protocol enums or introduce explicit mapping to prevent off-by-one mismatches.
2 participants
Previous implementation of the lsp protocol types was a copy-past of an outdated version of the gopls typings. We can't just copy-paste the new gopls typings because they use a codegen tool that now strucutres the modules differently, and in any case, it's quite a manual solution.
Hopefully, migrating to a lib will get us faster integration for future lsp features (at the cost of more dependencies)
lib used: https://pkg.go.dev/go.lsp.dev/protocol