Fix the goxls language server cannot find xgo #373
Conversation
Summary of ChangesHello @tsingbx, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to fix an issue where the language server fails to find the xgo command by attempting to locate xgo first and falling back to gop. It also adds the .vscode directory to .gitignore. The .gitignore change is appropriate. However, the logic for falling back from xgo to gop is flawed because the condition to check if xgo was found is incorrect. This will prevent the fallback from ever executing. I've provided a critical comment with a suggested fix to correctly implement the fallback behavior.
gop/langserver/langserver.go
Outdated
| if len(cmd) == 0 { | ||
| cmd = lookupCmd("gop") | ||
| } |
There was a problem hiding this comment.
The condition len(cmd) == 0 to check if xgo was found is incorrect. The lookupCmd function returns the original command name string if the command is not found, so its length will not be zero. Consequently, the fallback to gop will never be triggered.
A better approach is to check if lookupCmd returned an absolute path, which it does on success via exec.LookPath. You can use filepath.IsAbs for this check.
| if len(cmd) == 0 { | |
| cmd = lookupCmd("gop") | |
| } | |
| if !filepath.IsAbs(cmd) { | |
| cmd = lookupCmd("gop") | |
| } |
![xgopilot[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
gop/langserver/langserver.go
Outdated
| onceInit.Do(func() { | ||
| cmd := lookupCmd("gop") | ||
| cmd, err := lookupCmd("xgo") | ||
| if err != nil { | ||
| cmd, _ = lookupCmd("gop") |
There was a problem hiding this comment.
Critical: Fallback logic is broken
The condition if len(cmd) == 0 will never be true because lookupCmd() returns the command name itself when not found (line 59: return cmd). This means:
- If "xgo" is not found,
lookupCmd("xgo")returns"xgo"(not"") - The fallback to "gop" never executes
- The language server will fail to start when only "gop" is available
Recommended fix:
cmd := lookupCmd("xgo")
// Verify the command actually exists
if _, err := exec.LookPath(cmd); err != nil {
cmd = lookupCmd("gop")
}This also fixes the performance issue of potentially calling lookupCmd() twice unnecessarily.
|
|
||
| func Get() langserver.Client { | ||
| onceInit.Do(func() { | ||
| cmd := lookupCmd("gop") | ||
| cmd, err := lookupCmd("xgo") | ||
| if err != nil { | ||
| cmd, _ = lookupCmd("gop") | ||
| } |
There was a problem hiding this comment.
Missing documentation for behavior change
This change switches from "gop" to "xgo" as the primary language server command, which is a significant behavior change. Please add a comment explaining:
- Why "xgo" is now preferred over "gop"
- What the fallback strategy is
- Impact on users with only "gop" installed
Example:
// Get returns the language server client, initializing it on first call.
// Prefers xgo command over gop for improved functionality.
// Falls back to gop for backward compatibility.
func Get() langserver.Client {
|
Review Summary The .gitignore change looks good. However, there's a critical logic bug in the langserver.go fallback mechanism that prevents it from working as intended. The fallback to "gop" will never execute because |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## goplus #373 +/- ##
=======================================
Coverage 46.45% 46.45%
=======================================
Files 12 12
Lines 1610 1610
=======================================
Hits 748 748
Misses 784 784
Partials 78 78 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
No description provided.