go-langserver icon indicating copy to clipboard operation
go-langserver copied to clipboard

How to autocomplete unimported packages?

Open JeanMertz opened this issue 6 years ago • 11 comments

I'd like something like strconv. to give me auto completion, but that only works when the strconv package is already imported.

Any way to make go-langserver work with unimported packages?

I found some references here:

https://github.com/sourcegraph/go-langserver/blob/6d17b634f244650517ca2ec9a82068d02a581b09/langserver/internal/gocode/config.go#L14-L31

But I couldn't find any other references/issues/documentation, so just wanted to make sure this is still correct (and that I need to configure gocode manually, to make this work).

JeanMertz avatar May 07 '18 07:05 JeanMertz

Ah, in fact, I already have this configuration file, but it doesn't seem to be working?

{
  "propose-builtins": true,
  "lib-path": "",
  "custom-pkg-prefix": "",
  "custom-vendor-dir": "",
  "autobuild": true,
  "force-debug-output": "",
  "package-lookup-mode": "go",
  "close-timeout": 1800,
  "unimported-packages": true,
  "partials": true,
  "ignore-case": false,
  "class-filtering": true
}

I'm using go-langserver together with https://github.com/tomv564/LSP, but as far as I know, the client doesn't need to do anything special to support this, does it?

JeanMertz avatar May 07 '18 07:05 JeanMertz

I just tried this in my terminal, and gocode is definitely returning values as expected:

echo -e "package main\n\nfunc main() {\n  strconv.At\n}\n" | gocode -f json autocomplete . 40 | jq .[1]
[
  {
    "class": "func",
    "name": "Atoi",
    "type": "func(s string) (int, error)",
    "package": "strconv"
  }
]

JeanMertz avatar May 07 '18 07:05 JeanMertz

When enabling debug logging in the client, I see the following response returned by the server:

{'isIncomplete': False, 'items': []}

JeanMertz avatar May 07 '18 07:05 JeanMertz

After digging some more, I guess the reason is that the code that I found regarding gocode is vendored code, and it looks like go-langserver doesn't use the file-based gocode configuration.

Looking at the InitDaemon code, it seems there are some hard-coded preferences, that can't be changed:

https://github.com/sourcegraph/go-langserver/blob/6d17b634f244650517ca2ec9a82068d02a581b09/langserver/internal/gocode/export.go#L9-L14

Would you be willing to accept a PR to enable auto-completing unimported package by default? Or, if I'm off the mark and there is a way to enable this, let me know!

JeanMertz avatar May 07 '18 09:05 JeanMertz

I validated that this diff works as expected:

diff --git a/langserver/internal/gocode/export.go b/langserver/internal/gocode/export.go
index ac533ff..00a0b70 100644
--- a/langserver/internal/gocode/export.go
+++ b/langserver/internal/gocode/export.go
@@ -9,6 +9,7 @@ var bctx go_build_context
 func InitDaemon(bc *build.Context) {
        bctx = pack_build_context(bc)
        g_config.ProposeBuiltins = true
+       g_config.UnimportedPackages = true
        g_daemon = new(daemon)
        g_daemon.drop_cache()
 }

obviously this would be a big change to make, so I'm unsure if this is something you want in a PR, or if more of this should be configurable through flags...

JeanMertz avatar May 07 '18 09:05 JeanMertz

cc @Contextualist Any reason why we shouldn't enable unimportedpackages?

keegancsmith avatar May 08 '18 09:05 keegancsmith

I just noticed that this breaks if you trigger autocomplete on a package that doesn't exist.

Using this file:

package main

func main() {

}

If I write fmt. in the main function, things work as expected, but if I instead write fm., then it crashes.

Here's the log:

https://gist.github.com/JeanMertz/eb2db980229d5caef496c9cdbd7a69b5

And here's the offending request/response:

--> request #5: textDocument/completion: {"textDocument":{"uri":"file:///Users/jean/code/go/src/github.com/blendle/importtest/main.go"},"position":{"line":3,"character":4}}
<-- result #5: textDocument/completion: {"isIncomplete":false,"items":[{"label":"PANIC","detail":"PANIC","insertText":"PANIC","insertTextFormat":1,"textEdit":{"range":{"start":{"line":3,"character":4},"end":{"line":3,"character":4}},"newText":"PANIC"}}]}

JeanMertz avatar May 08 '18 10:05 JeanMertz

Seems to originate from here:

https://github.com/sourcegraph/go-langserver/blob/7f84cdc942f706cf3e36bdbd5c5bf6e1f8cb044b/langserver/internal/gocode/decl.go#L35-L36

JeanMertz avatar May 08 '18 10:05 JeanMertz

FYI We do plan on switching our vendored in gocode to a new upstream implementation. So that sort of issues may disappear / belong in the upstream repo. See the discussion in https://github.com/sourcegraph/go-langserver/issues/268

keegancsmith avatar May 08 '18 10:05 keegancsmith

In the end, I copy/pasted the latest master source of gocode into go-langserver (and removed some unrelated changes/code), it now works as expected.

Full diff here:

https://gist.github.com/JeanMertz/aeb524db3b6eabfa46d9298ac228f1a5

I suspect this is the relevant change that fixed the above bug:

@@ -361,15 +362,29 @@ func (c *auto_complete_context) apropos(file []byte, filename string, cursor int
        // And we're ready to Go. ;)
 
        if !ok {
                var d *decl
                if ident, ok := cc.expr.(*ast.Ident); ok && g_config.UnimportedPackages {
                        p := resolveKnownPackageIdent(ident.Name, c.current.name, c.current.context)
-                       c.pcache[p.name] = p
-                       d = p.main
+                       if p != nil {
+                               c.pcache[p.name] = p
+                               d = p.main
+                       }
                }
                if d == nil {
                        return nil, 0

JeanMertz avatar May 08 '18 10:05 JeanMertz

@JeanMertz 👍 Wow, such a trek of digging! You're right. Sorry that I didn't include the config capability of gocode for simplicity sake. Also, nice finding on the behavior of nonexistent package. However, I am a little bit hesitated on whether to enable the option. See my second concern below.

@keegancsmith For why I drop that option is largely opinionated. I was planning to bring that option in together with the "auto import on formatting" (as being brought up recently by #272) as a integrated workflow. Anyway, there is really no harm to just enable it. My second concern is that mdempsky/gocode we are switching to (see #268) seems lack of support for both gocode config file and unimportedpackages. I haven't thought of how to implement that on our side.

Contextualist avatar May 08 '18 12:05 Contextualist