x/tools/cmd/guru: describe: objects defined in cgo files are not found

[aletheia7]

golang guru/GoDef cannot find non-golang package documentation that is in the GOPATH.

Please answer these questions before submitting your issue. Thanks!

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

   go version go1.6.1 linux/386

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

   GOARCH="386"
   GOBIN=""
   GOEXE=""
   GOHOSTARCH="386"
   GOHOSTOS="linux"
   GOOS="linux"
   GOPATH="/home/erik/go:/opt/erik/dev/g/go:/opt/erik/dev/p/go"
   GORACE=""
   GOROOT="/usr/lib/go"
   GOTOOLDIR="/usr/lib/go/pkg/tool/linux_386"
   GO15VENDOREXPERIMENT="1"
   CC="gcc"
   GOGCCFLAGS="-fPIC -m32 -pthread -fmessage-length=0"
   CXX="g++"
   CGO_ENABLED="1"

3. What did you do? I filed an issue with vim-go. (https://github.com/fatih/vim-go/issues/851#issuecomment-219248856). The vim-go author believes the problem is with golang guru. Please see the issue for the repeatable problem.
4. What did you expect to see?

Please see the issue above.

1. What did you see instead?

Please see the issue above.

[aletheia7]

Debian: testing/stretch. Fully apt-get'd.

[adonovan]

Possible duplicate of Issue #13971. (That relates to 'describe' operations, but I think the underlying cause is the same.)

[aletheia7]

Good. Here is the output from guru describe:

# Searching for log.Println()
guru describe v.go:#86
<path omitted>/go/src/vgotest/v.go:10.6-10.12: reference to func log.Println(v ...interface{})
/usr/lib/go/src/log/log.go:294:6: defined here

# Searching for zmq4.NewSocket()
guru describe v.go:#139
<path omitted>/go/src/vgotest/v.go:12.12-12.25: selector of type invalid type

The zmq4.NewSocket() file cannot be found under ~/go/src/github.com ... or go/src/vgotest/vendor/github.com ...

Debian stretch is using tools/guru https://github.com/golang/tools/tree/f42ec616d3061dd0a453e8f174d62b38eddab928

I confirmed that the problem occurs with the latest tools/guru https://github.com/golang/tools/commit/c86fe5956d4575f29850535871a97abbd403a145 (master)

[jasonkeene]

@aletheia7 I ran into the same issue with the same type. That zmq4.Socket type has a lot of methods. Would be nice if guru could describe this.

[alandonovan]

zmq4.NewSocket is defined in a file that imports "C". The bug is in the way that, for speed, guru disables cgo preprocessing for many type-base queries including describe and definition. Guru sets build.Context.CgoEnabled=false, which causes the cgo files to appear not under build.Package.CgoFiles but under Package.IgnoredGoFiles. The go/loader package doesn't parse or type-check these files, so it is as if they don't exist. Ideally the loader would do the best it can to type-check these files without cgo preprocessing, but it can't simply parse the IgnoredCgoFiles because there are many reasons a file may be ignored, such as having the wrong GOOS or GOARCH.

To avoid the cgo files ending up in the wrong bucket, the loader must not call build.Context.Import with Context.CgoEnabled=0. It must, in effect, temporarily save, set, and restore this flag around the Import call, and then, if the restored flag was clear, tack any CgoFiles that were found on to the regular GoFiles list so that they are parsed without cgo preprocessing.

[aletheia7]

Thank you. Guru works great for packages under vendor that do not import "C."

[aletheia7]

@alandonovan

Are there any plans to enhance guru to "Ideally the loader would do the best it can type type-check these files ...?"

As it stands, when I create a Go package to be used as a cgo wrapper, I have to split the Go Exported functions into a separate file from the cgo in order for great tools like vim-go to use guru to complete the function signature. For a sizeable C API, this gets tedious.

I hope this functionality is added to guru. Please consider this request.

[alandonovan]

We do our best to fix bugs in the x/tools repository as time and priority permits. I don't have a specific plan to fix this bug soon, but if you'd like to attempt the fix outlined above I'd be happy to review your change.

[FrankReh]

guru, guru definition specifically for me, is gold. I got used to following definitions from vim so quickly and easily that when I was editing other languages and didn't have it, or tried to follow a definition into a cgo file, I felt lost. Having to resort to grep in another window and then manually enter the file's path into vim suddenly seemed so archaic, even difficult. Running into this issue and the description about how to proceed was a windfall.

I have a guru working now with cgo files. Both cgo files in imported packages and in the current package - slightly different paths in guru. I had to make four minor changes, two in the guru/definition.go file and two in the loader/loader.go package. Is making a modification to the loader package an acceptable solution, in theory?

guru/definition calls the loader/(Config).Load() and this would ultimately get (Config).parsePackageFiles() called. In order to get parsePackageFiles to treat the Cgo files as Go files as suggested above, I added a flag to the Config struct.

guru/definition.go calls the above mentioned Load() so the CgoEnabled flag had to be set there so the cgo files would not be ignored and the new Config flag had to be set so the loader wouldn't actually call cgo on the files, just parser.ParseFile().

guru/definition.go also calls build.Context.Import() so the CgoEnabled flag needed to be set there too.

Are there other guru modes, like 'describe', I should look at to see if a similar modification would work? Not using the other guru modes much, I wouldn't be sure.

Is there another reason guru goes to lengths to set CgoEnabled to false? It does so on two different code paths. If not, I could try allowing the cgo files all around guru, just disabling their actual cgo processing by the loader package.

And are there some forms of cgo file that might cause the parser.ParseFile() some difficulty that I should test with?

[aletheia7]

Thank you!

[FrankReh]

Okay. Am understanding the Oct 5 comment even better now. The changes are mostly to the loader. In it, the conf.Build.CgoEnabled can be set to true just when FindPackage() is called. So the cgo files are not ignored. And then in parsePackageFiles() the CgoEnabled flag can be checked and if unset while there are cgo files, treat them as go files, so the cgo extra processing can be skipped. So no change to the loader.Config structure, and fewer changes to the guru code. The guru code may only be affected where it was explicitly calling the build.Context.Import() itself.

[alandonovan]

Exactly. Care to send me your CL?

[FrankReh]

Soon. Am seeing whether I can keep the changes local to guru by providing a custom FindPackage to the loader.Conf.

Thanks, -Frank

On Mar 6, 2017, at 10:24 AM, alandonovan notifications@github.com wrote:

Exactly. Care to send me your CL?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[gopherbot]

CL https://golang.org/cl/37856 mentions this issue.

[FrankReh]

I can add some tests once I get the nod that I'm going in the right direction with these changes. There are also other guru modes that might benefit from a similar treatment.

[unship]

will this fix soon?

[aletheia7]

According to https://github.com/golang/go/issues/24661#issuecomment-492478653, guru is deprecated for gopls.

vim-go version 1.20+ is supporting (maybe starting to support) gopls.