LS rewrite #41
LS rewrite #41
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request updates multiple modules across the codebase. It refines the symbol extraction process by sorting symbols, removes deprecated language server dependencies, and reworks the JSON-RPC implementation. New message stream types, JSON-RPC connection handling, detailed error handling, and corresponding tests have been introduced. LSP handler methods now use standardized types from the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant JSONRPC2_Conn as "JSON-RPC Conn"
participant State
Client->>JSONRPC2_Conn: Send JSON-RPC Request ("initialize", etc.)
JSONRPC2_Conn->>State: Dispatch request to handler (e.g., updateDocument, hover)
State-->>JSONRPC2_Conn: Process request and return response
JSONRPC2_Conn-->>Client: Send JSON-RPC Response (e.g., InitializeResult)
sequenceDiagram
participant Producer
participant ChanObjStream
participant Consumer
Producer->>ChanObjStream: WriteMessage(message)
ChanObjStream->>Consumer: ReadMessage(message)
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #41 +/- ##
==========================================
+ Coverage 63.48% 66.36% +2.88%
==========================================
Files 31 33 +2
Lines 7101 7297 +196
==========================================
+ Hits 4508 4843 +335
+ Misses 2384 2230 -154
- Partials 209 224 +15 ☔ View full report in Codecov by Sentry. |
ascandone
force-pushed
the
refactor/improve-lsp-testing
branch
from
c955db9 to
0c87fce
Compare
ascandone
force-pushed
the
refactor/improve-lsp-testing
branch
from
29c65b6 to
1ae831a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (26)
internal/utils/utils.go (1)
37-44: Consider adding doc comments and improved test coverage forUnmarshal[T].
This function effectively leverages Go generics to unmarshal any type, which is great. However, adding usage examples and thorough unit tests would help ensure correctness across different data structures and edge cases.internal/analysis/document_symbols.go (1)
38-44: Return zero on equality for stable sorting.
In the current comparison function, you always return 1 whena.Range.Start.GtEq(b.Range.Start). Consider returning 0 if they are equal to preserve stable sorting or ensure correctness when positions match.internal/jsonrpc2/messages_test.go (1)
1-128: Add negative/boundary scenario tests.
While these tests provide good coverage of typical usage, consider adding negative or malformed scenarios to ensure robust error handling. For example, test with invalid JSON, missing fields likejsonrpc, or unexpected types formethodorparams.internal/lsp/lsp_test.go (2)
13-36: Test could benefit from proper resource cleanupThe test successfully verifies basic LSP initialization flow, but it doesn't properly clean up resources. The input and output channels are never closed, which could lead to resource leaks in more complex test scenarios.
Consider adding cleanup code with
t.Cleanup()to ensure channels are properly closed after the test:func TestServerReadWrite(t *testing.T) { in := make(chan jsonrpc2.Message) out := make(chan jsonrpc2.Message) + t.Cleanup(func() { + close(in) + // Note: out is closed by the connection + }) lsp.NewConn(jsonrpc2.NewChanObjStream(in, out))
31-36: Improve test coverage of server capabilitiesThe test only verifies the
HoverProvidercapability, but a complete initialization result would contain many more capabilities that should be tested.Consider extending the test to verify more capabilities:
require.True(t, init.Capabilities.HoverProvider) + // Verify other expected capabilities + require.NotNil(t, init.Capabilities.TextDocumentSync) + require.True(t, init.Capabilities.CompletionProvider != nil) + require.True(t, init.Capabilities.DiagnosticProvider != nil)internal/jsonrpc2/chan_object_stream.go (2)
3-7: Fix typo in comment and improve documentationThere's a typo in the comment and the struct documentation could be more comprehensive.
-// This is mostly intented as a testing utility +// ChanObjStream is primarily intended as a testing utility. +// It implements MessageStream using Go channels for message passing. type ChanObjStream struct { in <-chan Message out chan<- Message }
28-31: Add check for nil message in WriteMessageThe
WriteMessagemethod doesn't validate the input message, potentially allowing nil messages to be sent.Consider adding a nil check for the message:
func (c *ChanObjStream) WriteMessage(obj Message) error { + if obj == nil { + return fmt.Errorf("cannot write nil message") + } c.out <- obj return nil }internal/lsp/object_stream_test.go (3)
15-35: Add test for error conditionsThe test verifies the happy path but doesn't test error conditions. Consider adding tests for invalid messages.
Consider adding a test case for handling oversized messages:
func TestObjectStreamWrite(t *testing.T) { // Existing test... require.Equal(t, []byte(expectedMsg), bs) + + // Test large message handling + t.Run("oversized_message", func(t *testing.T) { + out := NewRwCloser() + stream := lsp.NewLsObjectStream(NewRwCloser(), out) + + // Create a message with large payload + largeParams := make([]byte, 10*1024*1024) // 10MB + for i := range largeParams { + largeParams[i] = 'x' + } + + msg := jsonrpc2.Request{ + Method: "largeRequest", + Params: largeParams, + } + + // Verify the stream can handle large messages + err := stream.WriteMessage(msg) + require.Nil(t, err) + + // Verify output contains correct length header + header := make([]byte, 30) // Large enough for the header + n, err := out.Read(header) + require.Nil(t, err) + require.Contains(t, string(header[:n]), fmt.Sprintf("Content-Length: %d", len(largeParams)+len(`{"method":"largeRequest","params":}`))) + }) }
56-73: Fix typo in test function nameThe function name has a typographical error.
-func TestObjectStreamSimmetric(t *testing.T) { +func TestObjectStreamSymmetric(t *testing.T) {
75-85: Enhance MockReadWriteCloser with tracking functionalityThe current mock implementation doesn't track operations, making it difficult to assert on specific behaviors like closing.
Enhance the mock with tracking capabilities:
type MockReadWriteCloser struct { io.ReadWriter + closed bool } func NewRwCloser() MockReadWriteCloser { - return MockReadWriteCloser{ReadWriter: &bytes.Buffer{}} + return MockReadWriteCloser{ + ReadWriter: &bytes.Buffer{}, + closed: false, + } } func (c *MockReadWriteCloser) Close() error { + c.closed = true return nil } +// WasClosed returns whether Close() was called +func (c *MockReadWriteCloser) WasClosed() bool { + return c.closed +}Note: You'll need to change the receiver to a pointer type and update the test code to use the pointer as well.
internal/lsp/documents_store.go (1)
21-21: Consider using pointer receivers for concurrency clarity.The methods
GetandSetare declared on a value receiver ((s documentStore[Doc])). Although the mutex is a pointer and should remain consistent, using pointer receivers (i.e.,func (s *documentStore[Doc]) Get(...)) is often clearer and prevents accidental copies of the struct.-func (s documentStore[Doc]) Get(uri lsp_types.DocumentURI) (Doc, bool) { +func (s *documentStore[Doc]) Get(uri lsp_types.DocumentURI) (Doc, bool) { s.mu.RLock() defer s.mu.RUnlock() doc, ok := s.documents[uri] return doc, ok } -func (s documentStore[Doc]) Set(uri lsp_types.DocumentURI, doc Doc) { +func (s *documentStore[Doc]) Set(uri lsp_types.DocumentURI, doc Doc) { s.mu.Lock() s.documents[uri] = doc s.mu.Unlock() }Also applies to: 28-28
internal/lsp/object_stream.go (2)
25-32: Return a pointer for the new stream object to avoid copying.Currently,
NewLsObjectStreamreturns anLsObjectStreamby value, but you use pointer receivers in its methods. Returning a pointer here is more conventional for concurrency types holding mutexes and I/O resources.-func NewLsObjectStream(in io.ReadCloser, out io.WriteCloser) LsObjectStream { +func NewLsObjectStream(in io.ReadCloser, out io.WriteCloser) *LsObjectStream { reader := bufio.NewReader(in) - return LsObjectStream{ + return &LsObjectStream{ reader: reader, in: in, out: out, } }
84-85: Fix reversed arguments in error message.The format string in line 85 flips the readBytes and total length. Consider swapping the placeholders or reversing the argument order.
- return nil, fmt.Errorf("missing bytes to read. Read: %d, total: %d", len, readBytes) + return nil, fmt.Errorf("missing bytes to read. Read: %d, total: %d", readBytes, len)internal/jsonrpc2/jsonrpc2_test.go (1)
57-57: Correct spelling of the test name.For clarity and consistency, rename from
TestErrIvalidParamtoTestErrInvalidParam.-func TestErrIvalidParam(t *testing.T) { +func TestErrInvalidParam(t *testing.T) {internal/jsonrpc2/jsonrpc2.go (6)
1-11: Improve file-level concurrency and error handling documentation.
This file introduces the JSON-RPC core connection logic. It would be helpful to include a brief file-level comment clarifying concurrency expectations and error-handling guarantees (e.g., whetherMessageStreamis safe for concurrent use, how errors surface, etc.).
19-20: Consider naming clarity for handler type aliases.
While concise, the namesrequestHandlerandnotificationHandlermight be improved for readability and maintainability (e.g.,jsonRPCRequestHandler).
37-55: Validate error handling inNewRequestHandler.
Any error returned by thehandleris wrapped in aResponseError, but the internal unmarshal step always returnsErrInvalidParamson decode failure. Consider logging or wrapping decode errors with more details if debugging is needed.
57-70: Graceful handling of JSON unmarshal errors in notification handlers.
Currently, there is no error response to the client if JSON unmarshal fails for notifications. This is common for notifications, but consider at least logging or tracking these failures.
72-108: Potential goroutine explosion inNewConn.
Each message read spawns a new goroutine if a request handler callsgo handler(...). Under high load, this can lead to many goroutines. Consider a worker pool or queue if performance becomes an issue.
146-159: Consider confirming notification send successes or failures.
While JSON-RPC notifications do not expect a response, it may be helpful to optionally log or handle errors froms.stream.WriteMessage(...).internal/lsp/handlers_test.go (4)
15-26: Potential negative path testing for diagnostics.
Currently,TestDiagnosticschecks happy paths. Consider adding tests with malformed input or incomplete scripts to validate error handling.
28-45: Thorough coverage for hover on variable.
This test exemplifies typical usage. Adding additional coverage for unresolvable variables or naming collisions might be beneficial.
73-86: TestGetSymbols verifies sorting and retrieval.
The snapshot approach is fine. Consider verifying empty or large inputs.
88-105: TestGotoDef ensures variable definitions are resolved.
Test is well-defined. Could add out-of-range or unresolved references test scenarios.internal/lsp/handlers.go (2)
184-203:initializeResultis suitable for essential LSP capabilities.
Storing version and capabilities in a global variable is functional. If you plan on supporting multiple concurrency or dynamic config, consider a more dynamic approach.
204-207:RunServerdoes not handle errors fromWait().
IfWait()returns an error, it silently exits. Consider logging or returning the error to the caller.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
.github/workflows/checks.ymlis excluded by!**/*.ymlgo.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.suminternal/jsonrpc2/__snapshots__/messages_test.snapis excluded by!**/*.snap,!**/*.snapinternal/lsp/__snapshots__/handlers_test.snapis excluded by!**/*.snap,!**/*.snap
📒 Files selected for processing (18)
internal/analysis/document_symbols.go(3 hunks)internal/cmd/lsp.go(1 hunks)internal/jsonrpc2/chan_object_stream.go(1 hunks)internal/jsonrpc2/jsonrpc2.go(1 hunks)internal/jsonrpc2/jsonrpc2_test.go(1 hunks)internal/jsonrpc2/messages.go(1 hunks)internal/jsonrpc2/messages_test.go(1 hunks)internal/lsp/documents_store.go(1 hunks)internal/lsp/handlers.go(7 hunks)internal/lsp/handlers_test.go(1 hunks)internal/lsp/language_server/message_buffer.go(0 hunks)internal/lsp/language_server/message_buffer_test.go(0 hunks)internal/lsp/language_server/server.go(0 hunks)internal/lsp/lsp_test.go(1 hunks)internal/lsp/lsp_types/bindings.go(1 hunks)internal/lsp/object_stream.go(1 hunks)internal/lsp/object_stream_test.go(1 hunks)internal/utils/utils.go(2 hunks)
💤 Files with no reviewable changes (3)
- internal/lsp/language_server/message_buffer_test.go
- internal/lsp/language_server/server.go
- internal/lsp/language_server/message_buffer.go
✅ Files skipped from review due to trivial changes (1)
- internal/lsp/lsp_types/bindings.go
🧰 Additional context used
🪛 GitHub Actions: Go
internal/jsonrpc2/jsonrpc2_test.go
[error] 64-64: Test timed out after 500ms in TestErrIvalidParam.
🔇 Additional comments (28)
internal/jsonrpc2/messages.go (2)
8-16: JSON-RPC error definitions appear consistent.
These error constants align well with the official JSON-RPC specification codes and messages.
112-125: Consider handling numeric IDs beyond 64-bit range.
The code coerces numeric IDs to float64 or int64, which may cause overflow or precision loss for very large integers. Evaluate whether you should handle big.Int or produce an error on extremely large IDs.internal/lsp/documents_store.go (2)
3-7: Adoption oflsp_typesis consistent with LSP standardization.Using
lsp_types.DocumentURIaligns well with official LSP data structures and improves consistency across the codebase.
11-11: Maps now keyed bylsp_types.DocumentURI: good improvement.Switching from a custom
DocumentURItolsp_types.DocumentURIsimplifies integration with other LSP logic. Themake(map[lsp_types.DocumentURI]Doc)usage is correct and ready for concurrency-safe read/write operations guarded bymu.Also applies to: 17-17
internal/jsonrpc2/jsonrpc2.go (6)
13-17: Ensure clarity inMessageStreamconcurrency.
It is unclear whether implementations ofMessageStream(e.g., for real sockets vs. test channels) must be goroutine-safe. Consider documenting or enforcing concurrency requirements within theMessageStreaminterface to prevent race conditions onWriteMessage/ReadMessage.
161-167: Close pending request channels before shutting down.
Close()cleanly closes channels inpendingRequests. This is good practice. No concerns here.
169-206: Ensure concurrency safety aroundgo s.stream.WriteMessage(...).
Requests that spawn a goroutine to write a response (lines 187 & 190) rely onMessageStreamto be concurrency-safe. Double-check that allMessageStreamimplementations handle concurrent writes properly.
208-222: Clean separation for response handling.
ThehandleResponsemethod uses a channel-based approach withpendingRequeststo deliver responses. This ensures correct pairing of request and response. Looks solid.
224-237: Potential panic indefaultcase ofhandleMessage.
The fallback callsutils.NonExhaustiveMatchPanic(...). Ensure that unhandled message types are truly unexpected and won't occur in deployment.
239-242: Blocking approach inWait()is straightforward.
Wait()returns after an error is received or the connection is closed. This looks appropriate for typical JSON-RPC usage.internal/lsp/handlers_test.go (9)
1-13: Overall test coverage is good.
These test imports (snaps, require) and basic scaffolding indicate a comprehensive approach for LSP feature coverage. No immediate issues.
47-59: Hover function origin coverage.
Completes coverage of built-in function hovers. Looks good.
61-71: Hover function statement coverage.
Same pattern as the origin test. Straightforward and consistent.
107-111:TestClientstruct is straightforward.
No issues here.
113-127: OpenFile properly simulatesdidOpen.
Adds to the concurrency by grabbing a single-diagnostic result from thediagnosticschannel. Fine for typical usage. Ensure the channel can handle unexpected concurrency if multiple notifications arrive in quick succession.
129-136: Hover request coverage.
Implementation is standard for JSON-RPC requests. Looks good.
138-142: GetSymbols request coverage.
Straightforward request usage aligns with the LSP spec.
144-151: GotoDefinition request coverage.
Matches typical usage.
153-174:newTestClientconcurrency flow.
This sets up twochanObjStreampairs—one for the server side and one for the client side. If high concurrency or parallel tests are planned, ensure channels do not block. Currently acceptable for single-thread tests.internal/lsp/handlers.go (9)
5-12: Check import usage and duplicates.
Multiple new imports forjsonrpc2andlsp_typesappear correct. Everything is used, so no immediate improvements needed.
23-40: Document concurrency inupdateDocument.
updateDocumentcallsanalysis.CheckSource, which might be computationally intensive for large documents. If used concurrently, ensure thedocumentStoreis thread-safe or locked.
42-115: Hover logic uses standard LSP approach.
Implementation is clear. The code gracefully handles null or unresolved references in theCheckResult. This is well-structured and easy to maintain.
117-133: GotoDefinition flow is consistent.
The function returns an LSPLocationreferencing the same URI. Looks good. Consider adding cross-file references in the future, if needed.
135-153: Symbol retrieval with conversion approach.
Works well. The localGetSymbols()is nicely converted into LSP-friendly symbols.
155-167: Position conversions remain a common source of off-by-one errors.
fromLspPositionandParserToLspPositionappear correct for typical zero-based vs. line-based indexing. Double-check for any off-by-one pitfalls in the rest of the code.
169-178:toLspRangehandles start/end positions.
Implementation is straightforward.
176-182:toLspDiagnosticseverity and message mapping.
The severity logic referencesKind.Severity(). Ensure the mapping of diagnostic kinds to LSP severities is accurate for all kind values.
209-239:NewConnregisters all LSP handlers in one place.
This is a clean approach for a centralized LSP server entry point. Be mindful of concurrency handling in each handler for large-scale usage.
internal/cmd/lsp.go
Outdated
| InitialState: lsp.InitialState, | ||
| Handler: lsp.Handle, | ||
| }) | ||
| lsp.RunServer() |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider adding error handling for RunServer
The previous implementation may have had error handling built into the parameter structure. With the simplified call, there's no visible error handling.
Check if lsp.RunServer() handles errors internally or should return an error that needs to be handled here:
🏁 Script executed:
#!/bin/bash
# Check implementation of RunServer to see if it returns an error
rg -A 5 "func RunServer\(\)" --glob "internal/lsp/*.go"Length of output: 381
Action: Add proper error handling for the RunServer call
It appears that lsp.RunServer() now returns an error (as confirmed in internal/lsp/handlers.go), but in internal/cmd/lsp.go (line 15) the error is not being handled. Please update the call to check for a non-nil error. For example:
if err := lsp.RunServer(); err != nil {
log.Fatal(err)
}This update will ensure that any server initialization errors are caught and handled appropriately.
| func (c *ChanObjStream) ReadMessage() (Message, error) { | ||
| msg := <-c.in | ||
| return msg, nil | ||
| } |
There was a problem hiding this comment.
Add channel closure handling in ReadMessage
The ReadMessage implementation doesn't check if the channel is closed, which could lead to a zero value being returned without an error.
Modify the ReadMessage method to properly handle closed channels:
func (c *ChanObjStream) ReadMessage() (Message, error) {
- msg := <-c.in
- return msg, nil
+ msg, ok := <-c.in
+ if !ok {
+ return nil, io.EOF
+ }
+ return msg, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (c *ChanObjStream) ReadMessage() (Message, error) { | |
| msg := <-c.in | |
| return msg, nil | |
| } | |
| func (c *ChanObjStream) ReadMessage() (Message, error) { | |
| msg, ok := <-c.in | |
| if !ok { | |
| return nil, io.EOF | |
| } | |
| return msg, nil | |
| } |
internal/lsp/object_stream.go
Outdated
| func (s *LsObjectStream) WriteMessage(obj jsonrpc2.Message) error { | ||
| bytes, err := json.Marshal(obj) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| encoded := fmt.Sprintf("Content-Length: %d\r\n\r\n%s", len(bytes), bytes) | ||
|
|
||
| s.mu.Lock() | ||
| s.out.Write([]byte(encoded)) | ||
| s.mu.Unlock() | ||
|
|
||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Handle write errors.
WriteMessage does not check for write failures from s.out.Write. Ignoring the error can hide underlying I/O issues.
func (s *LsObjectStream) WriteMessage(obj jsonrpc2.Message) error {
bytes, err := json.Marshal(obj)
if err != nil {
return err
}
encoded := fmt.Sprintf("Content-Length: %d\r\n\r\n%s", len(bytes), bytes)
s.mu.Lock()
- s.out.Write([]byte(encoded))
+ _, writeErr := s.out.Write([]byte(encoded))
s.mu.Unlock()
+ if writeErr != nil {
+ return writeErr
+ }
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (s *LsObjectStream) WriteMessage(obj jsonrpc2.Message) error { | |
| bytes, err := json.Marshal(obj) | |
| if err != nil { | |
| return err | |
| } | |
| encoded := fmt.Sprintf("Content-Length: %d\r\n\r\n%s", len(bytes), bytes) | |
| s.mu.Lock() | |
| s.out.Write([]byte(encoded)) | |
| s.mu.Unlock() | |
| return nil | |
| } | |
| func (s *LsObjectStream) WriteMessage(obj jsonrpc2.Message) error { | |
| bytes, err := json.Marshal(obj) | |
| if err != nil { | |
| return err | |
| } | |
| encoded := fmt.Sprintf("Content-Length: %d\r\n\r\n%s", len(bytes), bytes) | |
| s.mu.Lock() | |
| _, writeErr := s.out.Write([]byte(encoded)) | |
| s.mu.Unlock() | |
| if writeErr != nil { | |
| return writeErr | |
| } | |
| return nil | |
| } |
internal/jsonrpc2/jsonrpc2_test.go
Outdated
| _, err := client.SendRequest("capitalize", 42) | ||
| require.Equal(t, &jsonrpc2.ErrInvalidParams, err) | ||
| } |
There was a problem hiding this comment.
Use require.ErrorIs instead of pointer equality.
Comparing errors via pointer equality can fail if the error is wrapped or reallocated. Prefer require.ErrorIs to check for ErrInvalidParams. This might also fix the timeouts if the code is never returning that exact pointer.
- require.Equal(t, &jsonrpc2.ErrInvalidParams, err)
+ require.ErrorIs(t, err, jsonrpc2.ErrInvalidParams)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _, err := client.SendRequest("capitalize", 42) | |
| require.Equal(t, &jsonrpc2.ErrInvalidParams, err) | |
| } | |
| _, err := client.SendRequest("capitalize", 42) | |
| require.ErrorIs(t, err, jsonrpc2.ErrInvalidParams) | |
| } |
🧰 Tools
🪛 GitHub Actions: Go
[error] 64-64: Test timed out after 500ms in TestErrIvalidParam.
internal/jsonrpc2/jsonrpc2.go
Outdated
| // Send a json rpc request and wait for the response. Thread safe. | ||
| // | ||
| // Will panick whenever the params object fails json.Marshal-ing | ||
| func (s *Conn) SendRequest(method string, params any) (json.RawMessage, *ResponseError) { | ||
| bytes, err := json.Marshal(params) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
|
|
||
| freshId := NewIntId(atomic.AddInt64(&s.currentId, 1)) | ||
|
|
||
| s.stream.WriteMessage(Request{ | ||
| ID: freshId, | ||
| Method: method, | ||
| Params: bytes, | ||
| }) | ||
|
|
||
| ch := make(chan Response) | ||
|
|
||
| s.pendingRequestMu.Lock() | ||
| s.pendingRequests[freshId] = ch | ||
| s.pendingRequestMu.Unlock() | ||
|
|
||
| response := <-ch | ||
|
|
||
| s.pendingRequestMu.Lock() | ||
| delete(s.pendingRequests, freshId) | ||
| s.pendingRequestMu.Unlock() | ||
|
|
||
| if response.Error != nil { | ||
| return nil, response.Error | ||
| } | ||
|
|
||
| return response.Result, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid indefinite blocking in SendRequest.
SendRequest blocks until a response is received via <-ch. If the remote endpoint never responds, the caller remains blocked. Consider adding a timeout mechanism or context to prevent indefinite blocking and allow graceful recovery.
internal/jsonrpc2/jsonrpc2.go
Outdated
| type Conn struct { | ||
| listenErr chan error | ||
| currentId int64 | ||
| opened bool | ||
| stream MessageStream | ||
| requestsHandlers map[string]requestHandler | ||
| notificationHandlers map[string]notificationHandler | ||
| pendingRequestMu sync.RWMutex | ||
| pendingRequests map[ID](chan Response) | ||
| } | ||
|
|
||
| type Handler struct { | ||
| register func(*Conn) | ||
| } |
There was a problem hiding this comment.
Address data race on the opened field.
The opened field is read in one goroutine (line 92) and written to in Close() (line 166) without synchronization, creating a data race. Use a mutex or atomic operations to ensure thread-safe updates.
type Conn struct {
listenErr chan error
currentId int64
- opened bool
+ opened int32
stream MessageStream
...
}
func (s *Conn) Close() {
s.stream.Close()
...
- s.opened = false
+ atomic.StoreInt32(&s.opened, 0)
}
// usage in the receive loop:
for atomic.LoadInt32(&conn.opened) == 1 {
msg, err := conn.stream.ReadMessage()
...
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type Conn struct { | |
| listenErr chan error | |
| currentId int64 | |
| opened bool | |
| stream MessageStream | |
| requestsHandlers map[string]requestHandler | |
| notificationHandlers map[string]notificationHandler | |
| pendingRequestMu sync.RWMutex | |
| pendingRequests map[ID](chan Response) | |
| } | |
| type Handler struct { | |
| register func(*Conn) | |
| } | |
| import ( | |
| "sync" | |
| "sync/atomic" | |
| ) | |
| type Conn struct { | |
| listenErr chan error | |
| currentId int64 | |
| opened int32 | |
| stream MessageStream | |
| requestsHandlers map[string]requestHandler | |
| notificationHandlers map[string]notificationHandler | |
| pendingRequestMu sync.RWMutex | |
| pendingRequests map[ID](chan Response) | |
| } | |
| // Close safely updates the opened flag using atomic operations. | |
| func (s *Conn) Close() { | |
| s.stream.Close() | |
| // Other cleanup code... | |
| atomic.StoreInt32(&s.opened, 0) | |
| } | |
| // Example usage in the receive loop using atomic load. | |
| func (conn *Conn) receiveLoop() { | |
| for atomic.LoadInt32(&conn.opened) == 1 { | |
| msg, err := conn.stream.ReadMessage() | |
| if err != nil { | |
| // Handle error... | |
| break | |
| } | |
| // Process the message... | |
| } | |
| } | |
| type Handler struct { | |
| register func(*Conn) | |
| } |
ascandone
force-pushed
the
refactor/improve-lsp-testing
branch
from
e628e8c to
6a40ac3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
internal/lsp/object_stream.go (1)
84-85: Fix the mismatched placeholders in error message
The error message at line 85 reverses the parameter order, which can lead to confusion:- return nil, fmt.Errorf("missing bytes to read. Read: %d, total: %d", len, readBytes) + return nil, fmt.Errorf("missing bytes to read. Read: %d, total: %d", readBytes, len)internal/jsonrpc2/jsonrpc2.go (1)
161-167: Consider broadcasting normal closure
WhenClose()is called normally (no error), the receive loop exits only after encountering an error from reading the closed stream. A small improvement is to send a graceful “close” signal soWait()does not always rely on an error to break out.internal/lsp/handlers.go (3)
155-182: Consider standardizing function naming conventions.The conversion functions between parser positions/ranges and LSP positions/ranges have been correctly updated to use the new types, but the naming convention is inconsistent:
fromLspPositionuses "from" prefixParserToLspPositionuses "To" infix and has capitalized first lettertoLspRangeandtoLspDiagnosticuse "to" prefixThis inconsistency could make the code harder to navigate.
-func fromLspPosition(p lsp_types.Position) parser.Position { +func LspToParserPosition(p lsp_types.Position) parser.Position { return parser.Position{ Line: int(p.Line), Character: int(p.Character), } } -func ParserToLspPosition(p parser.Position) lsp_types.Position { +func ParserToLspPosition(p parser.Position) lsp_types.Position { return lsp_types.Position{ Line: uint32(p.Line), Character: uint32(p.Character), } }
184-202: Clear declaration of server capabilities.The
initializeResultvariable provides a well-structured declaration of server capabilities. This is important for LSP clients to understand what features are supported by the server.One minor issue is the comment on line 194 about the structure being "ugly." Instead of just commenting on it, consider creating a helper function to build the server info or using a cleaner struct initialization approach.
- // This is ugly. Is there a shortcut? - ServerInfo: struct { - Name string "json:\"name\"" - Version string "json:\"version,omitempty\"" - }{ - Name: "numscript-ls", - Version: "0.0.1", - }, + ServerInfo: lsp_types.ServerInfo{ + Name: "numscript-ls", + Version: "0.0.1", + },
214-237: Implement consistent error handling for LSP requests.While the handlers are well-structured, there's no consistent error handling pattern. If any of the handlers encounter errors (like invalid parameters or internal errors), they should return proper JSON-RPC error responses rather than potentially returning nil.
Consider implementing a pattern like this for error-prone handlers:
jsonrpc2.NewRequestHandler("textDocument/hover", func(p lsp_types.HoverParams, conn *jsonrpc2.Conn) any { - return state.handleHover(p) + result := state.handleHover(p) + if result == nil { + return &jsonrpc2.ResponseError{ + Code: -32803, // LSP-specific error code + Message: "No hover information available", + } + } + return result }),internal/jsonrpc2/messages.go (2)
112-125: Consider improved error handling in ID unmarshaling.The
unmarshalIDfunction correctly handles various ID types, but the error message for invalid types could be more informative.- return nil, fmt.Errorf("invalid id type: %s", raw) + return nil, fmt.Errorf("invalid id type: %T (value: %v)", raw, raw)
127-154: Robust message unmarshaling with differentiation between requests and responses.The
UnmarshalMessagefunction effectively:
- Deserializes the JSON data into the combined struct
- Converts the ID correctly
- Differentiates between requests and responses based on the presence of the "method" field
However, it doesn't validate the "jsonrpc" field to ensure it's "2.0" as required by the spec.
if err != nil { return nil, err } + + if combined.VersionTag != versionTag { + return nil, fmt.Errorf("invalid JSON-RPC version: %s, expected %s", combined.VersionTag, versionTag) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
.github/workflows/checks.ymlis excluded by!**/*.ymlgo.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.suminternal/jsonrpc2/__snapshots__/messages_test.snapis excluded by!**/*.snap,!**/*.snapinternal/lsp/__snapshots__/handlers_test.snapis excluded by!**/*.snap,!**/*.snap
📒 Files selected for processing (18)
internal/analysis/document_symbols.go(3 hunks)internal/cmd/lsp.go(1 hunks)internal/jsonrpc2/chan_object_stream.go(1 hunks)internal/jsonrpc2/jsonrpc2.go(1 hunks)internal/jsonrpc2/jsonrpc2_test.go(1 hunks)internal/jsonrpc2/messages.go(1 hunks)internal/jsonrpc2/messages_test.go(1 hunks)internal/lsp/documents_store.go(1 hunks)internal/lsp/handlers.go(7 hunks)internal/lsp/handlers_test.go(1 hunks)internal/lsp/language_server/message_buffer.go(0 hunks)internal/lsp/language_server/message_buffer_test.go(0 hunks)internal/lsp/language_server/server.go(0 hunks)internal/lsp/lsp_test.go(1 hunks)internal/lsp/lsp_types/bindings.go(1 hunks)internal/lsp/object_stream.go(1 hunks)internal/lsp/object_stream_test.go(1 hunks)internal/utils/utils.go(2 hunks)
💤 Files with no reviewable changes (3)
- internal/lsp/language_server/message_buffer_test.go
- internal/lsp/language_server/message_buffer.go
- internal/lsp/language_server/server.go
🚧 Files skipped from review as they are similar to previous changes (10)
- internal/lsp/lsp_types/bindings.go
- internal/utils/utils.go
- internal/analysis/document_symbols.go
- internal/lsp/lsp_test.go
- internal/cmd/lsp.go
- internal/jsonrpc2/messages_test.go
- internal/jsonrpc2/jsonrpc2_test.go
- internal/lsp/object_stream_test.go
- internal/lsp/documents_store.go
- internal/jsonrpc2/chan_object_stream.go
🔇 Additional comments (16)
internal/lsp/object_stream.go (1)
49-62: Handle write errors
This code does not check for errors returned bys.out.Write. Ignoring write failures can mask potential I/O issues that would otherwise surface.func (s *LsObjectStream) WriteMessage(obj jsonrpc2.Message) error { bytes, err := json.Marshal(obj) if err != nil { return err } encoded := fmt.Sprintf("Content-Length: %d\r\n\r\n%s", len(bytes), bytes) s.mu.Lock() - s.out.Write([]byte(encoded)) + n, writeErr := s.out.Write([]byte(encoded)) s.mu.Unlock() + if writeErr != nil { + return fmt.Errorf("failed to write %d out of %d bytes: %w", n, len(bytes), writeErr) + } return nil }internal/jsonrpc2/jsonrpc2.go (2)
25-25: Use atomic or mutex for theopenedfield
Access toopenedoccurs concurrently (e.g., line 92 in the goroutine vs. line 166 inClose). A plain boolean is not thread-safe. Use atomic operations or a mutex to avoid data races.type Conn struct { listenErr chan error currentId int64 - opened bool + opened int32 stream MessageStream // ... } func (s *Conn) Close() { s.stream.Close() // ... - s.opened = false + atomic.StoreInt32(&s.opened, 0) } - for conn.opened { + for atomic.LoadInt32(&conn.opened) == 1 { // ... }
110-144: Prevent indefinite blocking inSendRequest
If the remote endpoint never responds, the current approach causes the caller to block on<-chindefinitely. Introduce a timeout or a context-based cancellation mechanism to avoid deadlocks.internal/lsp/handlers_test.go (1)
113-127: Blocking on diagnostics
This test architecture consumes diagnostics from a channel. Should the server fail to send diagnostics, the test will block indefinitely. Ensure that these tests are robust when diagnostics may be delayed or missing.internal/lsp/handlers.go (6)
5-9: Properly updated imports for the language server refactoring.The imports have been updated to include the new dependencies needed for the refactored Language Server Protocol implementation. The addition of the os package supports the new RunServer function, while the new internal packages (jsonrpc2 and lsp_types) reflect the architectural changes mentioned in the PR objectives.
23-40: Well-designed dependency injection for document updates.The method signature has been improved to accept a JSON-RPC connection, which follows good dependency injection practices. This allows the method to directly send notifications rather than relying on a callback function stored in the state.
The transition to using standardized LSP types from the lsp_types package enhances compatibility and maintainability.
42-115: Handler methods effectively refactored to use standard LSP types.All handler methods have been systematically updated to use the standardized types from the lsp_types package. The underlying logic remains consistent, which demonstrates a well-executed refactoring approach. This change aligns with the PR objective of making a clearer separation between LSP-specific, JSON-RPC-specific, and data encoding components.
204-207: Simple and effective server startup function.The
RunServerfunction provides a clean way to start the LSP server using standard I/O streams. This follows the typical pattern for language servers that communicate via stdin/stdout.
209-239: Well-structured handler registration for JSON-RPC.The
NewConnfunction effectively sets up all the request and notification handlers in one place, creating a clean mapping between LSP methods and their handler functions. This approach:
- Makes it easy to see all supported LSP methods at a glance
- Follows a consistent pattern for handler registration
- Properly connects each method to the appropriate function in the State struct
- Includes proper shutdown handling
This implementation is significantly more maintainable than the previous approach, as mentioned in the PR objectives.
219-224: Robust handling of text document changes.The implementation correctly handles both document open and change notifications, and uses the latest content change in case of multiple changes. This ensures that the document store always has the most up-to-date version of the document.
internal/jsonrpc2/messages.go (6)
8-23: Well-defined error constants following the JSON-RPC specification.The error constants are properly defined according to the JSON-RPC 2.0 specification with appropriate error codes and messages. The
NewErrorconstructor function provides a clean way to create error objects.The link to the specification in the comments helps with future maintainability.
25-46: Clean interface design for ID types.The ID interface and its implementations (intId and stringId) are well-designed:
- The interface is minimal and focused
- Both string and integer IDs are supported as per the spec
- Constructor functions make it easy to create new IDs
- Type assertions ensure correct implementation
This approach allows flexible ID handling while maintaining type safety.
47-82: Clear message type definitions with good documentation.The
Messageinterface and its implementations (RequestandResponse) are clearly defined with helpful documentation comments explaining the purpose of each field. TheIsNotificationmethod is a useful utility for checking if a request is a notification (no response expected).
83-110: Efficient JSON serialization implementation.The JSON marshaling methods correctly implement the JSON-RPC 2.0 format, handling the mutually exclusive Result/Error fields properly. The code efficiently reuses the
messageCombinedstruct to avoid duplication.
156-170: Well-structured support types with proper JSON tags.The
messageCombinedandResponseErrorstructs are properly defined with appropriate JSON tags that follow the JSON-RPC 2.0 specification. The omitempty tags are correctly used for optional fields.
1-171: Comprehensive implementation of JSON-RPC 2.0 message handling.This implementation provides a clean, focused JSON-RPC 2.0 library that follows the specification closely. The separation of concerns between message types, serialization, and error handling is well-done. The code is well-documented with links to the specification, which enhances maintainability.
This aligns perfectly with the PR objective of implementing a more flexible JSON-RPC implementation as a replacement for the sourcegraph/jsonrpc2 dependency.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal/jsonrpc2/jsonrpc2.go (2)
25-25:⚠️ Potential issueEnsure thread-safe access to 'opened' to avoid data races.
Reading and writing the
openedfield concurrently (e.g., at line 105 in the receive loop and line 179 inClose()) without synchronization can introduce a data race. Consider using atomic operations or a mutex to ensure safe reads/writes of this boolean.Also applies to: 105-105, 179-179
146-146:⚠️ Potential issuePrevent indefinite blocking in
SendRequest.The call
response := <-chat line 146 will block forever if the remote side never replies. Consider adding a timeout or context cancellation to avoid indefinite blocking and facilitate graceful error handling.
🧹 Nitpick comments (1)
internal/jsonrpc2/jsonrpc2.go (1)
108-110: Add test coverage for untested lines.Static analysis indicates these lines are not covered by existing tests (e.g., error-handling paths and connection closure). Ensuring robust unit tests for these scenarios helps validate resilience and maintainability.
Would you like me to propose new test cases covering these code paths or open an issue to track adding these tests?
Also applies to: 114-116, 129-129, 165-165, 174-179, 186-187, 211-212, 227-232, 237-238
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 108-110: internal/jsonrpc2/jsonrpc2.go#L108-L110
Added lines #L108 - L110 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/jsonrpc2/jsonrpc2.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
internal/jsonrpc2/jsonrpc2.go
[warning] 108-110: internal/jsonrpc2/jsonrpc2.go#L108-L110
Added lines #L108 - L110 were not covered by tests
[warning] 114-116: internal/jsonrpc2/jsonrpc2.go#L114-L116
Added lines #L114 - L116 were not covered by tests
[warning] 129-129: internal/jsonrpc2/jsonrpc2.go#L129
Added line #L129 was not covered by tests
[warning] 165-165: internal/jsonrpc2/jsonrpc2.go#L165
Added line #L165 was not covered by tests
[warning] 174-179: internal/jsonrpc2/jsonrpc2.go#L174-L179
Added lines #L174 - L179 were not covered by tests
[warning] 186-187: internal/jsonrpc2/jsonrpc2.go#L186-L187
Added lines #L186 - L187 were not covered by tests
[warning] 211-212: internal/jsonrpc2/jsonrpc2.go#L211-L212
Added lines #L211 - L212 were not covered by tests
[warning] 227-232: internal/jsonrpc2/jsonrpc2.go#L227-L232
Added lines #L227 - L232 were not covered by tests
[warning] 237-238: internal/jsonrpc2/jsonrpc2.go#L237-L238
Added lines #L237 - L238 were not covered by tests
internal/jsonrpc2/jsonrpc2.go
Outdated
| func (s *Conn) SendRequest(method string, params any) (json.RawMessage, *ResponseError) { | ||
| bytes, err := json.Marshal(params) | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid panicking on marshalling errors in production code.
Calling panic(err) at lines 129 and 165 in a server setting may bring down the entire application. Returning a structured JSON-RPC error response or handling the error gracefully (e.g., logging) is a safer approach.
Also applies to: 165-165
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 129-129: internal/jsonrpc2/jsonrpc2.go#L129
Added line #L129 was not covered by tests
ascandone
force-pushed
the
refactor/improve-lsp-testing
branch
from
7753915 to
f692fee
Compare
ascandone
force-pushed
the
refactor/improve-lsp-testing
branch
from
f692fee to
0d7e4f3
Compare
NOTE FOR REVIEWERS: this PR is not designed to be read commit by commit.
You can follow modules dependency graph instead:
This PR doesn't add any new functionality for the user but repays a bit of the tech debt of the previous lsp implementation. The main issue is that previous implementation had 0 test coverage, except for the data decoding/encoding part
I kept a from-scratch implementation of json-rpc2 because 1. other alternatives are still marked as unstable (examples: sourcegraph/jsonrpc2, go/exp/jsonrpc2) 2. the other implementation look more low-level and complex
so it though made sense not to depend on those for a 200 LOC implementation
This PR doesn't handle some jsonrpc2 features which we aren't used from language servers, like messages batching, or ability to define synchronous message handlers (every message handlers is async, e.g. responses don't necessarily follow order of requests), as well as specs-compliant error handling of every corner case, as we'll rely for the client's to honor the protocol