Improvements #60
Improvements #60
Conversation
… respect to the end bracket
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis update introduces version 0.3.3, refactors document and language management in the LSP server, enhances section header completion logic, and adds new development tooling for the website. It also expands language detection capabilities, updates completion item behaviors, and provides new tests for INI parsing edge cases. Changes
Sequence Diagram(s)Centralized Document & Language Management in LSPsequenceDiagram
participant Client
participant LSPServer
participant SharedStore
Client->>LSPServer: textDocument/didOpen (URI, content)
LSPServer->>SharedStore: AddDocument(URI, OpenedFile{Language})
LSPServer-->>Client: Ack
Client->>LSPServer: textDocument/didChange (URI, content)
LSPServer->>SharedStore: GetDocument(URI)
alt Language changed
LSPServer->>SharedStore: AddDocument(URI, OpenedFile{NewLanguage})
end
LSPServer-->>Client: Ack
Client->>LSPServer: textDocument/didClose (URI)
LSPServer->>SharedStore: RemoveDocument(URI)
LSPServer-->>Client: Ack
Client->>LSPServer: textDocument/completion (URI, position)
LSPServer->>SharedStore: GetDocument(URI)
LSPServer->>LSPServer: Use Language from OpenedFile
LSPServer-->>Client: Completion response
Section Header Completion with ContextsequenceDiagram
participant LSPClient
participant CompletionHandler
participant INIParser
LSPClient->>CompletionHandler: Request header completion (with section context)
CompletionHandler->>INIParser: Parse document, get section header (may be existing)
CompletionHandler->>CompletionHandler: getHeaderCompletion(sectionName, doc, existingHeader)
alt existingHeader is nil
CompletionHandler-->>LSPClient: Insert new header suggestion
else existingHeader present
CompletionHandler-->>LSPClient: Replace header with snippet suggestion
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
|
Here's the code health analysis summary for commits Analysis Summary
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
server/root-handler/lsp/text-document-did-change.go (1)
35-64: Potential issue with double language detection and variable shadowing.The logic performs language detection twice and has variable shadowing that could lead to confusion:
- Line 35 detects language using
utils.DetectLanguage- Lines 39-44 call
initFilewhich likely performs language detection again, with the result assigned to a newnewLanguagevariable that shadows the one from line 35- Line 59 uses the shadowed
newLanguagefrominitFile, not the original detectionThis could lead to inconsistent behavior if the two detection calls return different results.
Consider simplifying this flow:
- newLanguage, err := utils.DetectLanguage(content, "", params.TextDocument.URI) - - // User changed the language OR - if err == nil && newLanguage != *document.Language { - newLanguage, err := initFile( + detectedLanguage, err := utils.DetectLanguage(content, "", params.TextDocument.URI) + + // User changed the language OR + if err == nil && detectedLanguage != *document.Language { + confirmedLanguage, err := initFile( context, content, params.TextDocument.URI, "", ) if err != nil { if common.ServerOptions.NoUndetectableErrors { return nil } else { return err } } params := &protocol.DidOpenTextDocumentParams{ TextDocument: protocol.TextDocumentItem{ URI: params.TextDocument.URI, Text: content, Version: params.TextDocument.Version, - LanguageID: string(*newLanguage), + LanguageID: string(*confirmedLanguage), }, }
🧹 Nitpick comments (6)
server/main.go (1)
21-22: Remove redundant return statements.The
returnstatements afteros.Exit(0)are unreachable sinceos.Exit(0)terminates the program immediately.if os.Args[1] == "--version" || os.Args[1] == "version" { fmt.Println(common.Version) os.Exit(0) - return } else if os.Args[1] == "--help" || os.Args[1] == "help" { fmt.Println("Usage: config-lsp [--version | --help]") fmt.Println(" --version: Print the version of config-lsp") fmt.Println(" --help: Print this help message") fmt.Println("Version:", common.Version) os.Exit(0) - return }Also applies to: 29-30
server/handlers/wireguard/handlers/completions_header.go (1)
11-11: Consider removing unused constant.The
END_BRACKETconstant is defined but not used in this file. Consider removing it unless it's intended for future use.-const END_BRACKET = ']' -server/root-handler/lsp/text-document-did-change.go (3)
35-64: Enhanced language detection with dynamic re-initialization looks good.The logic properly detects language changes and triggers re-initialization. The error handling respects the server configuration, and the delegation to
TextDocumentDidOpenensures consistent document lifecycle management.Consider adding a nil check for
document.Languagebefore dereferencing on line 38, though it's likely safe given the document structure.
83-83: Improve panic message construction to handle potential nil pointer.The updated panic message is consistent with using
document.Language, but directly dereferencing and concatenating could cause issues if the pointer is nil.Consider using a safer approach:
-panic("root-handler/TextDocumentDidChange: unexpected language" + *document.Language) +panic(fmt.Sprintf("root-handler/TextDocumentDidChange: unexpected language: %v", document.Language))This would require adding
"fmt"to the imports.
83-83: Minor formatting improvement needed in panic message.The panic message should have proper spacing for readability.
- panic("root-handler/TextDocumentDidChange: unexpected language" + *document.Language) + panic("root-handler/TextDocumentDidChange: unexpected language: " + *document.Language)server/root-handler/shared/singleton.go (1)
21-25: Consider thread safety for concurrent access.The global
OpenedFilesmap andAddDocumentfunction provide a clean interface, but consider whether concurrent access protection is needed for your LSP server implementation.If concurrent access is possible, consider adding mutex protection:
+import "sync" + +var filesMutex sync.RWMutex var OpenedFiles = make(map[protocol.DocumentUri]OpenedFile) func AddDocument(uri protocol.DocumentUri, file OpenedFile) { + filesMutex.Lock() + defer filesMutex.Unlock() OpenedFiles[uri] = file }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
flake.nix(2 hunks)justfile(1 hunks)server/common/common.go(1 hunks)server/handlers/bitcoin_conf/handlers/completions.go(1 hunks)server/handlers/bitcoin_conf/handlers/completions_header.go(3 hunks)server/handlers/wireguard/handlers/completions.go(1 hunks)server/handlers/wireguard/handlers/completions_body.go(2 hunks)server/handlers/wireguard/handlers/completions_header.go(2 hunks)server/main.go(1 hunks)server/parsers/ini/ast.go(1 hunks)server/parsers/ini/parser.go(1 hunks)server/parsers/ini/parser_test.go(1 hunks)server/root-handler/handler.go(0 hunks)server/root-handler/lsp/text-document-code-action.go(2 hunks)server/root-handler/lsp/text-document-completion.go(2 hunks)server/root-handler/lsp/text-document-definition.go(2 hunks)server/root-handler/lsp/text-document-did-change.go(3 hunks)server/root-handler/lsp/text-document-did-close.go(1 hunks)server/root-handler/lsp/text-document-did-open.go(1 hunks)server/root-handler/lsp/text-document-hover.go(2 hunks)server/root-handler/lsp/text-document-prepare-rename.go(2 hunks)server/root-handler/lsp/text-document-range-formatting.go(2 hunks)server/root-handler/lsp/text-document-rename.go(2 hunks)server/root-handler/lsp/text-document-signature-help.go(2 hunks)server/root-handler/shared/constants.go(1 hunks)server/root-handler/shared/indexes.go(0 hunks)server/root-handler/shared/singleton.go(1 hunks)server/root-handler/utils/language-detection.go(2 hunks)vs-code-extension/package.json(1 hunks)
💤 Files with no reviewable changes (2)
- server/root-handler/handler.go
- server/root-handler/shared/indexes.go
🧰 Additional context used
🧬 Code Graph Analysis (18)
server/handlers/wireguard/handlers/completions.go (1)
server/handlers/wireguard/handlers/completions_header.go (1)
GetSectionHeaderCompletions(43-65)
server/root-handler/lsp/text-document-rename.go (3)
server/root-handler/shared/singleton.go (1)
GetDocument(31-36)server/common/options.go (1)
ServerOptions(44-44)server/root-handler/utils/language-detection.go (1)
LanguageUndetectableError(23-23)
server/root-handler/lsp/text-document-range-formatting.go (3)
server/root-handler/shared/singleton.go (1)
GetDocument(31-36)server/common/options.go (1)
ServerOptions(44-44)server/root-handler/utils/language-detection.go (1)
LanguageUndetectableError(23-23)
server/handlers/bitcoin_conf/handlers/completions.go (2)
server/handlers/bitcoin_conf/handlers/completions_header.go (1)
GetSectionHeaderCompletions(13-34)server/parsers/ini/ast.go (1)
Header(36-40)
server/root-handler/lsp/text-document-signature-help.go (1)
server/root-handler/shared/singleton.go (1)
GetDocument(31-36)
server/main.go (1)
server/common/common.go (1)
Version(5-5)
server/root-handler/lsp/text-document-did-open.go (1)
server/root-handler/shared/singleton.go (2)
AddDocument(23-25)OpenedFile(9-14)
server/root-handler/lsp/text-document-completion.go (1)
server/root-handler/shared/singleton.go (1)
GetDocument(31-36)
server/handlers/wireguard/handlers/completions_body.go (2)
server/handlers/wireguard/handlers/completions_header.go (1)
GetSectionHeaderCompletions(43-65)server/parsers/ini/ast.go (1)
Header(36-40)
server/root-handler/lsp/text-document-hover.go (1)
server/root-handler/shared/singleton.go (1)
GetDocument(31-36)
server/root-handler/lsp/text-document-prepare-rename.go (3)
server/root-handler/shared/singleton.go (1)
GetDocument(31-36)server/common/options.go (1)
ServerOptions(44-44)server/root-handler/utils/language-detection.go (1)
LanguageUndetectableError(23-23)
server/root-handler/utils/language-detection.go (1)
server/root-handler/shared/constants.go (2)
LanguageSSHDConfig(7-7)LanguageSSHConfig(6-6)
server/root-handler/lsp/text-document-code-action.go (2)
server/root-handler/shared/singleton.go (1)
GetDocument(31-36)server/root-handler/utils/code-actions.go (1)
FetchAddLanguageActions(10-44)
server/root-handler/lsp/text-document-definition.go (3)
server/root-handler/shared/singleton.go (1)
GetDocument(31-36)server/common/options.go (1)
ServerOptions(44-44)server/root-handler/utils/language-detection.go (1)
LanguageUndetectableError(23-23)
server/root-handler/lsp/text-document-did-close.go (1)
server/root-handler/shared/singleton.go (2)
GetDocument(31-36)RemoveDocument(27-29)
server/parsers/ini/parser_test.go (2)
server/parsers/ini/parser.go (1)
NewConfig(15-20)server/parsers/ini/ast.go (1)
Header(36-40)
server/handlers/bitcoin_conf/handlers/completions_header.go (1)
server/parsers/ini/ast.go (1)
Header(36-40)
server/root-handler/shared/singleton.go (2)
server/root-handler/shared/constants.go (1)
SupportedLanguage(3-3)server/common/location.go (1)
LocationRange(31-34)
🔇 Additional comments (52)
server/common/common.go (1)
5-5: LGTM! Version bump is consistent.The version update from "0.3.2" to "0.3.3" aligns with the version updates across other project files and maintains the CI/CD comment for automation.
vs-code-extension/package.json (1)
5-5: LGTM! Version consistency maintained.The version update to "0.3.3" is consistent with the project-wide version bump and follows proper semantic versioning.
justfile (1)
70-72: LGTM! New development recipe follows established patterns.The
dev-websiterecipe correctly uses the working-directory directive and follows the same structure as other recipes in the justfile. Theyarn run devcommand is appropriate for website development.flake.nix (2)
26-26: LGTM! Version update is consistent.The version bump to "0.3.3" maintains consistency across the project and includes the CI/CD comment for automation.
167-175: LGTM! Well-configured website development shell.The new
websitedevShell provides appropriate modern web development tooling:
nodejs_22for the latest Node.js runtimeyarnfor package managementoxlintandbiomefor linting and formattingjustfor task automationThis complements the new
dev-websiterecipe in the justfile nicely.server/main.go (1)
17-32: Enhanced CLI help functionality looks good.The addition of
--helpsupport improves the user experience. The help message is clear and informative, showing both available options and the current version.The logic correctly handles both
--versionand--helparguments while maintaining backward compatibility.server/parsers/ini/parser.go (1)
87-88: LGTM! Clean addition of raw value preservation.The assignment of the raw input line to the
RawValuefield appropriately preserves the original header text alongside the parsedName. This enhancement enables more precise text editing operations in completion handlers.server/handlers/bitcoin_conf/handlers/completions.go (1)
24-24: LGTM! Function call updated to match new signature.The addition of
section.Headeras the second argument correctly aligns with the updatedGetSectionHeaderCompletionsfunction signature, enabling context-aware completion behavior for existing section headers.server/parsers/ini/ast.go (1)
38-39: LGTM! Well-structured addition to preserve raw header text.The
RawValuefield addition to theHeaderstruct appropriately complements the existingNamefield by preserving the original header text. This enhancement supports more precise text editing operations in completion handlers.server/handlers/wireguard/handlers/completions.go (1)
25-25: LGTM! Correct use of nil for non-existent header context.The addition of
nilas the second argument correctly aligns with the updatedGetSectionHeaderCompletionsfunction signature. Passingnilis appropriate here since this code path executes whensection == nil, indicating no existing header to reference.server/root-handler/lsp/text-document-did-open.go (1)
79-81: LGTM: Clean refactor to centralized document management.The refactor properly replaces the old direct map assignment pattern with the new structured
OpenedFileapproach. The language pointer is correctly set and aligns with the updatedshared.AddDocumentfunction signature.server/handlers/wireguard/handlers/completions_body.go (1)
43-43: LGTM: Enhanced completion logic with existing header context.The addition of
section.Headeras a parameter toGetSectionHeaderCompletionsenables context-aware completions that can differentiate between inserting new headers and replacing existing ones. This is a sensible enhancement to the completion system.Also applies to: 67-67
server/root-handler/lsp/text-document-range-formatting.go (1)
19-19: LGTM: Consistent refactor to new document management API.The changes correctly implement the refactor from handler-based language retrieval to direct document retrieval:
- Proper function call update to
shared.GetDocument- Appropriate nil check adjustment
- Correct dereferencing of
document.Languagein switch statement- Updated panic message for consistency
All changes align with the new centralized document management approach.
Also applies to: 21-21, 29-29, 46-46
server/parsers/ini/parser_test.go (1)
328-330: LGTM: Good test coverage for edge cases.The test assertions properly verify that incomplete section headers are parsed correctly:
- Single section is created
- Header name is empty string
- RawValue contains the actual input (
[or[])- No properties are created
This enhances test coverage for the new RawValue field functionality.
Also applies to: 343-345
server/root-handler/lsp/text-document-definition.go (1)
16-16: LGTM: Consistent refactor matching other LSP handlers.The changes follow the same correct pattern as other LSP handler files in this refactor:
- Updated to use
shared.GetDocumentAPI- Proper nil check adjustment for document instead of language
- Correct dereferencing of
document.Languagein switch logic- Updated panic message for consistency
The refactor maintains the same control flow while using the new centralized document management system.
Also applies to: 18-18, 26-26, 43-43
server/root-handler/lsp/text-document-signature-help.go (2)
15-21: LGTM: Consistent refactoring to centralized document management.The change from
shared.Handler.GetLanguageForDocument()toshared.GetDocument()followed by accessingdocument.Languageis part of a well-structured refactoring effort. The nil check is properly updated to verify the document object rather than the language pointer.
38-38: LGTM: Panic message correctly updated.The panic message properly reflects the change from accessing
*languageto*document.Language.server/root-handler/lsp/text-document-hover.go (2)
17-23: LGTM: Consistent document retrieval refactoring.The changes maintain the same logical flow while adopting the new document-centric approach. The nil check and language access through
document.Languageare correctly implemented.
40-40: LGTM: Panic message updated correctly.The panic message properly uses
*document.Languageinstead of the previous*language.server/root-handler/lsp/text-document-completion.go (2)
18-24: LGTM: Refactoring follows established pattern.The change to retrieve the full document object and access language through
document.Languageis consistent with the broader architectural refactoring. The nil check and switch statement are correctly updated.
41-41: LGTM: Panic message updated appropriately.The panic message correctly uses
*document.Languageto maintain error reporting consistency.server/root-handler/lsp/text-document-rename.go (2)
15-25: LGTM: Consistent refactoring with appropriate error handling.The change follows the established refactoring pattern. The error handling when
document == nilis more sophisticated than other handlers, returning eithernilorLanguageUndetectableErrorbased on server options, which is appropriate for rename operations.
42-42: LGTM: Panic message updated correctly.The panic message properly reflects the change to use
*document.Language.server/root-handler/lsp/text-document-prepare-rename.go (2)
16-26: LGTM: Consistent refactoring with proper error handling.The changes follow the established refactoring pattern. Like the rename handler, this function includes sophisticated error handling when
document == nil, using server options to determine whether to returnnilorLanguageUndetectableError, which is appropriate for prepare-rename operations.
43-43: LGTM: Panic message updated correctly.The panic message appropriately uses
*document.Languageinstead of*language.server/root-handler/lsp/text-document-code-action.go (4)
18-18: LGTM: Document retrieval centralized through shared.GetDocument.The change from
shared.Handler.GetLanguageForDocumenttoshared.GetDocumentaligns with the broader refactor to centralize document state management.
20-24: LGTM: Simplified nil handling and return logic.The updated nil check for
documentand simplified return logic (removing the previous conditional based on server options) makes the code cleaner and more straightforward.
26-26: LGTM: Switch statement correctly uses document's language field.The switch on
*document.Languageis consistent with the new document-centric approach.
43-43: LGTM: Panic message updated to reflect variable change.The panic message correctly references
*document.Languageinstead of the previous variable.server/root-handler/lsp/text-document-did-close.go (4)
17-17: LGTM: Consistent document retrieval pattern.The change to
shared.GetDocumentmaintains consistency with the broader refactor across LSP handlers.
19-21: LGTM: Appropriate nil check for document object.The nil check correctly handles cases where the document is not found in the opened files map.
23-23: LGTM: Document cleanup consolidated into shared function.Using
shared.RemoveDocumentconsolidates the cleanup logic that previously involved multiple map deletions and handler calls, improving maintainability.
25-25: LGTM: Switch statement uses document's language field.Consistent with the refactor pattern, switching on
*document.Languageinstead of a separate language variable.server/root-handler/utils/language-detection.go (3)
30-30: LGTM: New language mappings for SSH configurations.Adding "sshd" and "ssh" as direct mappings to their respective language types improves language detection for users who might use these shorter forms.
Also applies to: 34-34
98-98: LGTM: More flexible regex pattern for language overrides.The updated regex pattern
#\s*\?\s*lsp\.language\s*=\s*(\w+)\s*now allows optional whitespace before the question mark, making the language override syntax more flexible for users.
106-124: LGTM: Simplified language detection without metadata storage.Removing the language overwrite metadata storage aligns with the broader refactor to centralize document state management. The function now focuses solely on detection, which is cleaner and more maintainable.
server/handlers/bitcoin_conf/handlers/completions_header.go (3)
15-15: LGTM: Enhanced function signature for context-aware completions.Adding the
existingHeaderparameter enables the function to provide more intelligent completions that can replace existing headers when appropriate.
32-34: LGTM: Refactored completion generation into helper function.The refactor into
getHeaderCompletionimproves maintainability and allows for more sophisticated completion logic based on context.
36-64: LGTM: Smart completion logic for header insertion vs replacement.The helper function correctly handles two scenarios:
- When
existingHeaderis nil: Simple text insertion for new headers- When
existingHeaderis non-nil: TextEdit replacement using the existing header's rangeThis provides a better user experience by replacing existing headers rather than inserting additional ones.
server/handlers/wireguard/handlers/completions_header.go (2)
13-41: LGTM: Consistent smart completion logic.The
getHeaderCompletionfunction follows the same beneficial pattern as the bitcoin_conf handler, providing context-aware completions that can either insert new headers or replace existing ones.
45-45: LGTM: Enhanced header completions with context awareness.The function signature update and passing
existingHeaderto both Interface and Peer completions enables more intelligent completion behavior consistent with the broader enhancement.Also applies to: 59-59, 62-62
server/root-handler/lsp/text-document-did-change.go (7)
13-13: LGTM!The utils import is necessary for the enhanced language detection functionality.
20-20: LGTM!Centralizing document retrieval through
shared.GetDocumentimproves consistency and provides access to the full document state.
23-33: LGTM!The document initialization logic handles the edge case where a change event arrives before the document was properly opened. The delegation to
TextDocumentDidOpenensures proper initialization.
66-66: LGTM!The switch statement now correctly uses the language from the centralized document object, maintaining consistency with the refactor.
13-13: LGTM: Proper integration with centralized document management.The addition of the utils import and the shift to using
shared.GetDocumentaligns well with the broader refactor to centralize document state management.Also applies to: 20-20
23-33: Good defensive programming for uninitialized documents.The logic properly handles the edge case where a document change event arrives before the document is initialized by delegating to
TextDocumentDidOpen. The parameter construction is correct.
66-66: LGTM: Consistent with centralized document management.Switching on
*document.Languageis the correct approach and aligns with the new document management pattern.server/root-handler/shared/singleton.go (4)
4-5: LGTM!The common package import is necessary for the
LocationRangetype used inUnavailableInfo.
9-14: LGTM!The
OpenedFilestruct design is well-structured with appropriate pointer types for optional fields and clear documentation.
16-19: LGTM!The
UnavailableInfostruct is well-designed with clear documentation and appropriate use of pointer types for optional fields.
4-4: Well-designed document state structures.The import addition is necessary for
common.LocationRange. The struct design is sound:
- Using pointers for optional fields (
Language,UnavailableInfo,OverwritePosition) allows proper nil-checkingOpenedFileappropriately combines language information with metadata- Clear documentation explains the purpose of
UnavailableInfoAlso applies to: 9-19
| if !(len(errors) == 0) { | ||
| t.Fatalf("Parse: Expected errors, but got none") | ||
| } |
There was a problem hiding this comment.
Fix inconsistent test error expectations.
The test comments and assertions are inconsistent. Both tests check for len(errors) == 0 but the failure messages state "Expected errors, but got none", which suggests the tests expect errors but are actually checking for no errors.
Based on the test context (parsing incomplete headers like [ and []), these should likely parse successfully without errors. Update the comments to match the assertions:
- if !(len(errors) == 0) {
- t.Fatalf("Parse: Expected errors, but got none")
- }
+ if !(len(errors) == 0) {
+ t.Fatalf("Parse: Expected no errors, but got %v", errors)
+ }Also applies to: 339-341
🤖 Prompt for AI Agents
In server/parsers/ini/parser_test.go around lines 324 to 326 and 339 to 341, the
test failure messages incorrectly state "Expected errors, but got none" while
the assertions check for no errors (len(errors) == 0). Update the failure
messages to correctly reflect that no errors are expected, such as changing the
message to "Parse: Unexpected errors encountered" to align with the assertion
and test context of parsing incomplete headers successfully.
| LanguageSSHDConfig SupportedLanguage = "sshd_config" | ||
| LanguageFstab SupportedLanguage = "fstab" | ||
| LanguageWireguard SupportedLanguage = "languagewireguard" | ||
| LanguageWireguard SupportedLanguage = "wireguard" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify all references to the old constant value are updated.
The simplification from "languagewireguard" to "wireguard" improves consistency. Ensure that all code referencing the old constant value has been updated to prevent runtime issues.
🏁 Script executed:
#!/bin/bash
# Description: Search for any remaining references to the old "languagewireguard" string
# Expected: No results, indicating all references have been updated
echo "Searching for old 'languagewireguard' references:"
rg -i "languagewireguard"
echo "Searching for new 'wireguard' language references:"
rg -A 2 -B 2 '"wireguard"'Length of output: 4309
Remove legacy “languagewireguard” alias in language detection
The SupportedLanguage constant has been simplified to "wireguard", but there’s still a literal "languagewireguard" key in your language‐detection map. If you no longer need the old alias, please remove it (or otherwise document why it must remain).
• server/root-handler/utils/language-detection.go:
– Remove or update the line
"languagewireguard": shared.LanguageWireguard,
🤖 Prompt for AI Agents
In server/root-handler/utils/language-detection.go around the line containing
`"languagewireguard": shared.LanguageWireguard,` remove this line entirely to
eliminate the legacy alias since the SupportedLanguage constant is now
simplified to "wireguard" and the old alias is no longer needed.
|
|
||
| func (h *RootHandler) GetLanguageForDocument(uri protocol.DocumentUri) *SupportedLanguage { | ||
| language, found := h.languageMap[uri] | ||
| var OpenedFiles = make(map[protocol.DocumentUri]OpenedFile) |
There was a problem hiding this comment.
Critical: Thread safety concern with global map.
The global OpenedFiles map will be accessed concurrently by multiple LSP request handlers, but there's no synchronization mechanism. This can lead to race conditions and data corruption.
Consider adding synchronization:
+import "sync"
+
+var (
+ OpenedFiles = make(map[protocol.DocumentUri]OpenedFile)
+ openedFilesMutex sync.RWMutex
+)
-var OpenedFiles = make(map[protocol.DocumentUri]OpenedFile)Then update the accessor functions to use proper locking.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In server/root-handler/shared/singleton.go at line 21, the global map
OpenedFiles is accessed concurrently without synchronization, risking race
conditions. To fix this, introduce a sync.RWMutex to protect access to
OpenedFiles. Update all functions that read or write to OpenedFiles to acquire
the mutex lock appropriately—use RLock for reads and Lock for writes—to ensure
thread-safe access.
| func AddDocument(uri protocol.DocumentUri, file OpenedFile) { | ||
| OpenedFiles[uri] = file | ||
| } | ||
|
|
||
| return &language | ||
| func RemoveDocument(uri protocol.DocumentUri) { | ||
| delete(OpenedFiles, uri) | ||
| } |
There was a problem hiding this comment.
Thread safety issue in document management functions.
These functions modify the global map without synchronization, which can cause race conditions in concurrent LSP operations.
With the mutex suggested earlier, update these functions:
func AddDocument(uri protocol.DocumentUri, file OpenedFile) {
+ openedFilesMutex.Lock()
+ defer openedFilesMutex.Unlock()
OpenedFiles[uri] = file
}
func RemoveDocument(uri protocol.DocumentUri) {
+ openedFilesMutex.Lock()
+ defer openedFilesMutex.Unlock()
delete(OpenedFiles, uri)
}🤖 Prompt for AI Agents
In server/root-handler/shared/singleton.go around lines 23 to 29, the
AddDocument and RemoveDocument functions modify the global OpenedFiles map
without any synchronization, leading to potential race conditions. To fix this,
use the previously suggested mutex to lock the map before modifying it and
unlock it afterward. Wrap the map assignment in AddDocument and the delete
operation in RemoveDocument with mutex.Lock() and mutex.Unlock() calls to ensure
thread safety during concurrent access.
| func RemoveDocument(uri protocol.DocumentUri) { | ||
| delete(OpenedFiles, uri) | ||
| } | ||
|
|
||
| func (h *RootHandler) RemoveDocument(uri protocol.DocumentUri) { | ||
| delete(h.languageMap, uri) | ||
| func GetDocument(uri protocol.DocumentUri) *OpenedFile { | ||
| if file, exists := OpenedFiles[uri]; exists { | ||
| return &file | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Fix potential issue with pointer to temporary copy in GetDocument.
The RemoveDocument function is correct, but GetDocument has a subtle issue: it returns &file where file is a copy of the map value, creating a pointer to a temporary.
This could lead to undefined behavior. Consider these solutions:
Option 1: Store pointers in the map
-var OpenedFiles = make(map[protocol.DocumentUri]OpenedFile)
+var OpenedFiles = make(map[protocol.DocumentUri]*OpenedFile)
func AddDocument(uri protocol.DocumentUri, file OpenedFile) {
- OpenedFiles[uri] = file
+ OpenedFiles[uri] = &file
}
func GetDocument(uri protocol.DocumentUri) *OpenedFile {
- if file, exists := OpenedFiles[uri]; exists {
- return &file
- }
- return nil
+ return OpenedFiles[uri] // returns nil if not found
}Option 2: Return a copy instead of pointer
-func GetDocument(uri protocol.DocumentUri) *OpenedFile {
+func GetDocument(uri protocol.DocumentUri) (OpenedFile, bool) {
if file, exists := OpenedFiles[uri]; exists {
- return &file
+ return file, true
}
- return nil
+ return OpenedFile{}, false
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In server/root-handler/shared/singleton.go around lines 27 to 36, the
GetDocument function returns a pointer to a local copy of the map value, which
is unsafe. To fix this, either change the OpenedFiles map to store pointers to
OpenedFile so you can return the pointer directly, or modify GetDocument to
return a copy of the OpenedFile value instead of a pointer. Adjust the map type
or return type accordingly to avoid returning pointers to temporary variables.
| func GetDocument(uri protocol.DocumentUri) *OpenedFile { | ||
| if file, exists := OpenedFiles[uri]; exists { | ||
| return &file | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Critical issues: Thread safety and unsafe pointer to map value.
This function has two serious issues:
- Thread safety: No synchronization for concurrent map access
- Unsafe pointer: Returning
&filecreates a pointer to a local copy of the map value, which becomes invalid if the map is modified
Fix both issues:
func GetDocument(uri protocol.DocumentUri) *OpenedFile {
+ openedFilesMutex.RLock()
+ defer openedFilesMutex.RUnlock()
if file, exists := OpenedFiles[uri]; exists {
- return &file
+ // Return a copy to avoid pointer-to-map-value issues
+ fileCopy := file
+ return &fileCopy
}
return nil
}Alternatively, consider storing pointers in the map:
var OpenedFiles = make(map[protocol.DocumentUri]*OpenedFile)🤖 Prompt for AI Agents
In server/root-handler/shared/singleton.go around lines 31 to 36, the
GetDocument function has thread safety issues due to unsynchronized map access
and returns a pointer to a local copy of the map value, which is unsafe. Fix
this by adding synchronization (e.g., using a mutex) around map access to ensure
thread safety, and modify the map to store pointers to OpenedFile instead of
values to safely return the pointer directly from the map without creating a
local copy.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Other