♻️ Refactor and optimize code across the LSP, HIR, and formatter modules - #825
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors and optimizes code across LSP, HIR, and formatter modules to improve performance and reduce unnecessary allocations. The key changes focus on:
- Reducing lock contention and cloning in the LSP server by acquiring locks once and passing references
- Optimizing the HIR's unused function detection algorithm from O(n²) to O(n) using a hash map
- Improving formatter output handling to consistently add trailing newlines to multi-line formatted code
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/mq-lsp/src/server.rs | Optimized lock handling to acquire guards once per operation and refactored diagnostics logic to use O(1) lookups via a hash map |
| crates/mq-lsp/src/references.rs | Changed to accept source_map by reference instead of cloning, and uses read lock instead of write lock |
| crates/mq-lsp/src/goto_definition.rs | Changed to use read lock instead of write lock for HIR access |
| crates/mq-lsp/src/document_symbol.rs | Updated function signature to accept source_map by reference |
| crates/mq-lsp/src/completions.rs | Updated function signature to accept source_map by reference |
| crates/mq-hir/src/resolve.rs | Optimized symbol resolution to extract only necessary fields instead of cloning entire Symbol structs |
| crates/mq-hir/src/hir.rs | Refactored unused_functions to use O(n) hash map lookup and optimized builtin initialization to avoid borrow checker issues |
| crates/mq-hir/src/builtin.rs | Wrapped builtin collections in Arc to enable cheap cloning and added Clone derive to Builtin struct |
| crates/mq-formatter/src/formatter.rs | Simplified trailing whitespace removal and ensured consistent trailing newlines in multi-line formatted output |
| crates/mq-cli/tests/integration_tests.rs | Updated expected output to include trailing newline |
| let uri_string = uri.to_string(); | ||
| self.source_map.write().unwrap().insert(uri_string.clone(), source_id); | ||
| self.cst_nodes_map.insert(uri_string.clone(), nodes); | ||
| self.error_map.insert(uri_string, errors.error_ranges(&text)); |
There was a problem hiding this comment.
Calling uri.to_string() three times and cloning uri_string twice creates unnecessary allocations. The original code called uri.to_string() inline at each usage point, which is equally inefficient. Consider using Arc<str> or Rc<str> for the URI string if it needs to be shared across multiple collections, or restructure the code to avoid the repeated clones.
| let lines = self.output.lines(); | ||
| let line_count = &lines.count(); | ||
|
|
||
| if line_count == &1 { |
There was a problem hiding this comment.
The iterator lines is consumed by calling count() on line 58, making it unusable for the subsequent for loop on line 66. The code needs to call self.output.lines() again or collect the lines into a Vec first.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
crates/mq-hir/src/hir.rs:1
- Duplicate test case name 'def_' at line 1220. This is already used at line 1206. Each test case should have a unique name to avoid confusion and potential test identification issues. Consider renaming to something more descriptive like 'def_with_pipes' or 'def_multiline_pipes'.
use std::{path::PathBuf, vec};
| #[rstest] | ||
| #[case("", "")] | ||
| #[case( | ||
| #[case::empty("", "")] |
There was a problem hiding this comment.
The empty string test case now has inconsistent behavior with other test cases. All non-empty multiline test cases now expect a trailing newline in their output (as seen in lines 1203-1204, 1210-1211, etc.), but the empty string case returns an empty string without a newline. This inconsistency should be verified: either empty input should also produce a newline for consistency, or the early return at line 57-58 should be documented as intentional special handling.
No description provided.