x/tools/cmd/golsp: panic when running `gopls query definition internal/lsp/cmd/definition.go:#1277`

[mbana]

What version of Go are you using (go version)?

$ go version
go version go1.11.5 linux/amd64

Does this issue reproduce with the latest release?

I also tried with go version go1.12rc1 darwin/amd64 and the issue seems to occur there as well.

What operating system and processor architecture are you using (go env)?

go env Output

$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/root/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build711491476=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Start Go Docker image:

$ docker run --rm -it golang:1.11-stretch

Inside Docker:

root@41d0e457e4e5:/go# ( \
    go get -u golang.org/x/tools/... && \
    cd ${GOPATH}/src/golang.org/x/tools && \
    # check usage
    golsp query definition || true && \
    echo '----' && \
    # check file exists
    find . -ipath './internal/lsp/cmd/definition.go' &&
    echo '----' && \
    gopls query definition 'internal/lsp/cmd/definition.go:#1277' \
)
...
definition: definition expects 1 argument
show declaration of selected identifier

Usage: definition [flags] <position>

Example: show the definition of the identifier at syntax at offset 1277 in this file (flag.FlagSet):

  $ gopls definition internal/lsp/cmd/definition.go:#1277

	gopls definition flags are:
----
./internal/lsp/cmd/definition.go
----
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x758334]

goroutine 1 [running]:
go/token.(*File).Pos(...)
	/usr/local/go/src/go/token/position.go:251
golang.org/x/tools/internal/lsp/cmd.(*definition).Run(0xc0000fa0d0, 0x8b72a0, 0xc000016090, 0xc000010130, 0x1, 0x1, 0x0, 0x0)
	/go/src/golang.org/x/tools/internal/lsp/cmd/definition.go:69 +0x254
golang.org/x/tools/internal/tool.Main.func2(0x0, 0x8b76e0, 0xc0000fa0d0, 0x8b72a0, 0xc000016090, 0xc0000dc900, 0x0, 0x0)
	/go/src/golang.org/x/tools/internal/tool/tool.go:130 +0xb6
golang.org/x/tools/internal/tool.Main(0x8b72a0, 0xc000016090, 0x8b76e0, 0xc0000fa0d0, 0xc000010130, 0x1, 0x1)
	/go/src/golang.org/x/tools/internal/tool/tool.go:131 +0x250
golang.org/x/tools/internal/lsp/cmd.(*query).Run(0xc0000f6b40, 0x8b72a0, 0xc000016090, 0xc000010130, 0x2, 0x2, 0x0, 0x0)
	/go/src/golang.org/x/tools/internal/lsp/cmd/query.go:59 +0x26c
golang.org/x/tools/internal/tool.Main.func2(0x0, 0x8b7720, 0xc0000f6b40, 0x8b72a0, 0xc000016090, 0xc0000dc8a0, 0x0, 0x0)
	/go/src/golang.org/x/tools/internal/tool/tool.go:130 +0xb6
golang.org/x/tools/internal/tool.Main(0x8b72a0, 0xc000016090, 0x8b7720, 0xc0000f6b40, 0xc000010120, 0x2, 0x2)
	/go/src/golang.org/x/tools/internal/tool/tool.go:131 +0x250
golang.org/x/tools/internal/lsp/cmd.(*Application).Run(0xc000126200, 0x8b72a0, 0xc000016090, 0xc000010120, 0x3, 0x2, 0x0, 0x0)
	/go/src/golang.org/x/tools/internal/lsp/cmd/cmd.go:90 +0x2f3
golang.org/x/tools/internal/tool.Main.func2(0xc000126200, 0x8b7660, 0xc000126200, 0x8b72a0, 0xc000016090, 0xc0000dc840, 0x0, 0x0)
	/go/src/golang.org/x/tools/internal/tool/tool.go:130 +0xb6
golang.org/x/tools/internal/tool.Main(0x8b72a0, 0xc000016090, 0x8b7660, 0xc000126200, 0xc000010110, 0x3, 0x3)
	/go/src/golang.org/x/tools/internal/tool/tool.go:131 +0x250
main.main()
	/go/src/golang.org/x/tools/cmd/gopls/main.go:20 +0xb0
root@41d0e457e4e5:/go# 

What did you expect to see?

The definition of the identifier.

What did you see instead?

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x758334]

goroutine 1 [running]:
go/token.(*File).Pos(...)
	/usr/local/go/src/go/token/position.go:251
golang.org/x/tools/internal/lsp/cmd.(*definition).Run(0xc0000fa0d0, 0x8b72a0, 0xc000016090, 0xc000010130, 0x1, 0x1, 0x0, 0x0)
	/go/src/golang.org/x/tools/internal/lsp/cmd/definition.go:69 +0x254
golang.org/x/tools/internal/tool.Main.func2(0x0, 0x8b76e0, 0xc0000fa0d0, 0x8b72a0, 0xc000016090, 0xc0000dc900, 0x0, 0x0)
	/go/src/golang.org/x/tools/internal/tool/tool.go:130 +0xb6
golang.org/x/tools/internal/tool.Main(0x8b72a0, 0xc000016090, 0x8b76e0, 0xc0000fa0d0, 0xc000010130, 0x1, 0x1)
	/go/src/golang.org/x/tools/internal/tool/tool.go:131 +0x250
golang.org/x/tools/internal/lsp/cmd.(*query).Run(0xc0000f6b40, 0x8b72a0, 0xc000016090, 0xc000010130, 0x2, 0x2, 0x0, 0x0)
	/go/src/golang.org/x/tools/internal/lsp/cmd/query.go:59 +0x26c
golang.org/x/tools/internal/tool.Main.func2(0x0, 0x8b7720, 0xc0000f6b40, 0x8b72a0, 0xc000016090, 0xc0000dc8a0, 0x0, 0x0)
	/go/src/golang.org/x/tools/internal/tool/tool.go:130 +0xb6
golang.org/x/tools/internal/tool.Main(0x8b72a0, 0xc000016090, 0x8b7720, 0xc0000f6b40, 0xc000010120, 0x2, 0x2)
	/go/src/golang.org/x/tools/internal/tool/tool.go:131 +0x250
golang.org/x/tools/internal/lsp/cmd.(*Application).Run(0xc000126200, 0x8b72a0, 0xc000016090, 0xc000010120, 0x3, 0x2, 0x0, 0x0)
	/go/src/golang.org/x/tools/internal/lsp/cmd/cmd.go:90 +0x2f3
golang.org/x/tools/internal/tool.Main.func2(0xc000126200, 0x8b7660, 0xc000126200, 0x8b72a0, 0xc000016090, 0xc0000dc840, 0x0, 0x0)
	/go/src/golang.org/x/tools/internal/tool/tool.go:130 +0xb6
golang.org/x/tools/internal/tool.Main(0x8b72a0, 0xc000016090, 0x8b7660, 0xc000126200, 0xc000010110, 0x3, 0x3)
	/go/src/golang.org/x/tools/internal/tool/tool.go:131 +0x250
main.main()
	/go/src/golang.org/x/tools/cmd/gopls/main.go:20 +0xb0

[wingyplus]

nil pointer dereference error happen because (*File).GetToken() found an error and return nil back to the caller.

Reproduce by adding logs into file.go in package cache:

diff --git a/internal/lsp/cache/file.go b/internal/lsp/cache/file.go
index d75c6916..3a90fecb 100644
--- a/internal/lsp/cache/file.go
+++ b/internal/lsp/cache/file.go
@@ -8,6 +8,7 @@ import (
 	"go/ast"
 	"go/token"
 	"io/ioutil"
+	"log"
 
 	"golang.org/x/tools/go/packages"
 	"golang.org/x/tools/internal/lsp/source"
@@ -41,6 +42,7 @@ func (f *File) GetToken() *token.File {
 	defer f.view.mu.Unlock()
 	if f.token == nil {
 		if err := f.view.parse(f.URI); err != nil {
+			log.Println("cache: cannot parse uri:", err)
 			return nil
 		}
 	}

Then build and run:

$ gopls query definition './internal/lsp/cmd/definition.go:#1277'
2019/02/18 11:07:14 cache: cannot parse uri: no packages found for /internal/lsp/cmd/definition.go
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x1366ac9]

goroutine 1 [running]:
go/token.(*File).Pos(...)
	/Users/thanabodee/tools/go/src/go/token/position.go:266
golang.org/x/tools/internal/lsp/cmd.(*definition).Run(0xc0001160c8, 0x14e38e0, 0xc0000b2000, 0xc0000980b0, 0x1, 0x1, 0x0, 0x0)
	/Users/thanabodee/src/golang.org/x/tools/internal/lsp/cmd/definition.go:69 +0x1f9
golang.org/x/tools/internal/tool.Main.func2(0x0, 0x14e53e0, 0xc0001160c8, 0x14e38e0, 0xc0000b2000, 0xc0000fe900, 0x0, 0x0)
	/Users/thanabodee/src/golang.org/x/tools/internal/tool/tool.go:130 +0xb7
golang.org/x/tools/internal/tool.Main(0x14e38e0, 0xc0000b2000, 0x14e53e0, 0xc0001160c8, 0xc0000980b0, 0x1, 0x1)
	/Users/thanabodee/src/golang.org/x/tools/internal/tool/tool.go:131 +0x243
golang.org/x/tools/internal/lsp/cmd.(*query).Run(0xc000112c20, 0x14e38e0, 0xc0000b2000, 0xc0000980b0, 0x2, 0x2, 0x0, 0x0)
	/Users/thanabodee/src/golang.org/x/tools/internal/lsp/cmd/query.go:59 +0x262
golang.org/x/tools/internal/tool.Main.func2(0x0, 0x14e5420, 0xc000112c20, 0x14e38e0, 0xc0000b2000, 0xc0000fe8a0, 0x0, 0x0)
	/Users/thanabodee/src/golang.org/x/tools/internal/tool/tool.go:130 +0xb7
golang.org/x/tools/internal/tool.Main(0x14e38e0, 0xc0000b2000, 0x14e5420, 0xc000112c20, 0xc0000980a0, 0x2, 0x2)
	/Users/thanabodee/src/golang.org/x/tools/internal/tool/tool.go:131 +0x243
golang.org/x/tools/internal/lsp/cmd.(*Application).Run(0xc000148200, 0x14e38e0, 0xc0000b2000, 0xc0000980a0, 0x3, 0x2, 0x0, 0x0)
	/Users/thanabodee/src/golang.org/x/tools/internal/lsp/cmd/cmd.go:90 +0x2e9
golang.org/x/tools/internal/tool.Main.func2(0xc000148200, 0x14e5360, 0xc000148200, 0x14e38e0, 0xc0000b2000, 0xc0000fe840, 0x0, 0x0)
	/Users/thanabodee/src/golang.org/x/tools/internal/tool/tool.go:130 +0xb7
golang.org/x/tools/internal/tool.Main(0x14e38e0, 0xc0000b2000, 0x14e5360, 0xc000148200, 0xc000098090, 0x3, 0x3)
	/Users/thanabodee/src/golang.org/x/tools/internal/tool/tool.go:131 +0x243
main.main()
	/Users/thanabodee/src/golang.org/x/tools/cmd/gopls/main.go:20 +0xb1

This will probably because of source.ToURI doesn't returns absolute path of the args[1] ( internal/lsp/cmd/definition.go). I reproduce by add more test cases into ToURI:

diff --git a/internal/lsp/source/uri_test.go b/internal/lsp/source/uri_test.go
index e3d1e7d8..3ca8a8ca 100644
--- a/internal/lsp/source/uri_test.go
+++ b/internal/lsp/source/uri_test.go
@@ -37,6 +37,14 @@ func TestURI(t *testing.T) {
 			uri:      URI(`file:///a/b/c/src/bob.go`),
 			filename: `/a/b/c/src/bob.go`,
 		},
+		{
+			uri:      URI(`file://./internal/lsp/cmd/definition.go`),
+			filename: `./internal/lsp/cmd/definition.go`,
+		},
+		{
+			uri:      URI(`file://internal/lsp/cmd/definition.go`),
+			filename: `internal/lsp/cmd/definition.go`,
+		},
 	} {
 		if string(tt.uri) != toURI(tt.filename).String() {
 			t.Errorf("ToURI: expected %s, got %s", tt.uri, ToURI(tt.filename))

The result is:

$ go test ./internal/lsp/source/
--- FAIL: TestURI (0.00s)
    uri_test.go:57: Filename: expected ./internal/lsp/cmd/definition.go, got /internal/lsp/cmd/definition.go
    uri_test.go:57: Filename: expected internal/lsp/cmd/definition.go, got /lsp/cmd/definition.go
FAIL
FAIL	golang.org/x/tools/internal/lsp/source	0.008s

[wingyplus]

Try to fixing by use filepath.Abs before parse to uri:

diff --git a/internal/lsp/source/uri.go b/internal/lsp/source/uri.go
index d630e704..a7ee4945 100644
--- a/internal/lsp/source/uri.go
+++ b/internal/lsp/source/uri.go
@@ -46,7 +46,8 @@ func filename(uri URI) (string, error) {
 // ToURI returns a protocol URI for the supplied path.
 // It will always have the file scheme.
 func ToURI(path string) URI {
-	u := toURI(path)
+	abspath, _ := filepath.Abs(path)
+	u := toURI(abspath)
 	u.Path = filepath.ToSlash(u.Path)
 	return URI(u.String())
 }

This would work but not sure is it real solution or not.

[odeke-em]

Thank you for this report @mbana and for the patch discussions @wingyplus!

I shall kindly page @stamblerre to take a look.

[mbana]

Thanks, anyone else think the interface should change from:

func (f *File) GetToken() *token.File

to

func (f *File) GetToken() (*token.File, error)

Actually to be a bit more pedantic, the interface below which is defined in internal/lsp/source/view.go and mainly implemented in internal/lsp/cache/file.go would look better like:

type File interface {
	GetAST() (*ast.File, error)
	GetFileSet() (*token.FileSet, error)
	GetPackage() (*packages.Package, error)
	GetToken() (*token.File, error)
	GetContent() ([]byte, error)
}

Example of errorr being masked as already pointed out before:

func (f *File) GetAST() *ast.File {
	f.view.mu.Lock()
	defer f.view.mu.Unlock()
	if f.ast == nil {
		if err := f.view.parse(f.URI); err != nil {
			return nil
		}
	}
	return f.ast
}

[mbana]

Thanks, anyone else think the interface should change from:

func (f *File) GetToken() *token.File

to

func (f *File) GetToken() (*token.File, error)

Actually to be a bit more pedantic, the interface below which is defined in internal/lsp/source/view.go and mainly implemented in internal/lsp/cache/file.go would look better like:

type File interface {
	GetAST() (*ast.File, error)
	GetFileSet() (*token.FileSet, error)
	GetPackage() (*packages.Package, error)
	GetToken() (*token.File, error)
	GetContent() ([]byte, error)
}

Example of errorr being masked as already pointed out before:

func (f *File) GetAST() *ast.File {
	f.view.mu.Lock()
	defer f.view.mu.Unlock()
	if f.ast == nil {
		if err := f.view.parse(f.URI); err != nil {
			return nil
		}
	}
	return f.ast
}

I've created a PR to this effect. Please see golang/tools#75.

There's more error handling that could be done from my reading of the code, but I'll do this in another PR.

[mbana]

nil pointer dereference error happen because (*File).GetToken() found an error and return nil back to the caller.

Reproduce by adding logs into file.go in package cache:

diff --git a/internal/lsp/cache/file.go b/internal/lsp/cache/file.go
index d75c6916..3a90fecb 100644
--- a/internal/lsp/cache/file.go
+++ b/internal/lsp/cache/file.go
@@ -8,6 +8,7 @@ import (
 	"go/ast"
 	"go/token"
 	"io/ioutil"
+	"log"
 
 	"golang.org/x/tools/go/packages"
 	"golang.org/x/tools/internal/lsp/source"
@@ -41,6 +42,7 @@ func (f *File) GetToken() *token.File {
 	defer f.view.mu.Unlock()
 	if f.token == nil {
 		if err := f.view.parse(f.URI); err != nil {
+			log.Println("cache: cannot parse uri:", err)
 			return nil
 		}
 	}

Then build and run:

$ gopls query definition './internal/lsp/cmd/definition.go:#1277'
2019/02/18 11:07:14 cache: cannot parse uri: no packages found for /internal/lsp/cmd/definition.go
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x1366ac9]

goroutine 1 [running]:
go/token.(*File).Pos(...)
	/Users/thanabodee/tools/go/src/go/token/position.go:266
golang.org/x/tools/internal/lsp/cmd.(*definition).Run(0xc0001160c8, 0x14e38e0, 0xc0000b2000, 0xc0000980b0, 0x1, 0x1, 0x0, 0x0)
	/Users/thanabodee/src/golang.org/x/tools/internal/lsp/cmd/definition.go:69 +0x1f9
golang.org/x/tools/internal/tool.Main.func2(0x0, 0x14e53e0, 0xc0001160c8, 0x14e38e0, 0xc0000b2000, 0xc0000fe900, 0x0, 0x0)
	/Users/thanabodee/src/golang.org/x/tools/internal/tool/tool.go:130 +0xb7
golang.org/x/tools/internal/tool.Main(0x14e38e0, 0xc0000b2000, 0x14e53e0, 0xc0001160c8, 0xc0000980b0, 0x1, 0x1)
	/Users/thanabodee/src/golang.org/x/tools/internal/tool/tool.go:131 +0x243
golang.org/x/tools/internal/lsp/cmd.(*query).Run(0xc000112c20, 0x14e38e0, 0xc0000b2000, 0xc0000980b0, 0x2, 0x2, 0x0, 0x0)
	/Users/thanabodee/src/golang.org/x/tools/internal/lsp/cmd/query.go:59 +0x262
golang.org/x/tools/internal/tool.Main.func2(0x0, 0x14e5420, 0xc000112c20, 0x14e38e0, 0xc0000b2000, 0xc0000fe8a0, 0x0, 0x0)
	/Users/thanabodee/src/golang.org/x/tools/internal/tool/tool.go:130 +0xb7
golang.org/x/tools/internal/tool.Main(0x14e38e0, 0xc0000b2000, 0x14e5420, 0xc000112c20, 0xc0000980a0, 0x2, 0x2)
	/Users/thanabodee/src/golang.org/x/tools/internal/tool/tool.go:131 +0x243
golang.org/x/tools/internal/lsp/cmd.(*Application).Run(0xc000148200, 0x14e38e0, 0xc0000b2000, 0xc0000980a0, 0x3, 0x2, 0x0, 0x0)
	/Users/thanabodee/src/golang.org/x/tools/internal/lsp/cmd/cmd.go:90 +0x2e9
golang.org/x/tools/internal/tool.Main.func2(0xc000148200, 0x14e5360, 0xc000148200, 0x14e38e0, 0xc0000b2000, 0xc0000fe840, 0x0, 0x0)
	/Users/thanabodee/src/golang.org/x/tools/internal/tool/tool.go:130 +0xb7
golang.org/x/tools/internal/tool.Main(0x14e38e0, 0xc0000b2000, 0x14e5360, 0xc000148200, 0xc000098090, 0x3, 0x3)
	/Users/thanabodee/src/golang.org/x/tools/internal/tool/tool.go:131 +0x243
main.main()
	/Users/thanabodee/src/golang.org/x/tools/cmd/gopls/main.go:20 +0xb1

This will probably because of source.ToURI doesn't returns absolute path of the args[1] ( internal/lsp/cmd/definition.go). I reproduce by add more test cases into ToURI:

diff --git a/internal/lsp/source/uri_test.go b/internal/lsp/source/uri_test.go
index e3d1e7d8..3ca8a8ca 100644
--- a/internal/lsp/source/uri_test.go
+++ b/internal/lsp/source/uri_test.go
@@ -37,6 +37,14 @@ func TestURI(t *testing.T) {
 			uri:      URI(`file:///a/b/c/src/bob.go`),
 			filename: `/a/b/c/src/bob.go`,
 		},
+		{
+			uri:      URI(`file://./internal/lsp/cmd/definition.go`),
+			filename: `./internal/lsp/cmd/definition.go`,
+		},
+		{
+			uri:      URI(`file://internal/lsp/cmd/definition.go`),
+			filename: `internal/lsp/cmd/definition.go`,
+		},
 	} {
 		if string(tt.uri) != toURI(tt.filename).String() {
 			t.Errorf("ToURI: expected %s, got %s", tt.uri, ToURI(tt.filename))

The result is:

$ go test ./internal/lsp/source/
--- FAIL: TestURI (0.00s)
    uri_test.go:57: Filename: expected ./internal/lsp/cmd/definition.go, got /internal/lsp/cmd/definition.go
    uri_test.go:57: Filename: expected internal/lsp/cmd/definition.go, got /lsp/cmd/definition.go
FAIL
FAIL	golang.org/x/tools/internal/lsp/source	0.008s

Thanks.

In this PR, golang/tools#75, I added error handling across the board. Now, like you tests I now get:

$ gopls query definition './internal/lsp/cmd/definition.go:#1277'
definition: no packages found for /internal/lsp/cmd/definition.go

[stamblerre]

I think the issue here is that when using gopls in the server mode, we expect file URIs in the format that is sent by VSCode. It looks like we should probably absolutize the paths for the other modes.

As @ianthehat mentioned on your CL, we intentionally removed the error handling in golang.org/cl/162399 because we never really did anything useful with these errors.

[gopherbot]

Change https://golang.org/cl/163160 mentions this issue: internal/lsp: absolutize paths when converting filenames to URIs