Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
aletheia7 opened this issue May 17, 2016 · 17 comments
Closed

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

aletheia7 opened this issue May 17, 2016 · 17 comments

Comments

@aletheia7
Copy link

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. (GoDef returns "vim-go: guru no object for identifier" fatih/vim-go#851 (comment)). 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
Copy link
Author

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

@ianlancetaylor ianlancetaylor changed the title GoDef returns "vim-go: guru no object for identifier" x/tools/cmd/guru: GoDef returns "vim-go: guru no object for identifier" May 17, 2016
@ianlancetaylor ianlancetaylor added this to the Unreleased milestone May 17, 2016
@adonovan
Copy link
Member

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

@aletheia7
Copy link
Author

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 golang/tools@c86fe59 (master)

@jasonkeene
Copy link
Contributor

@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 alandonovan changed the title x/tools/cmd/guru: GoDef returns "vim-go: guru no object for identifier" x/tools/cmd/guru: describe: objects defined in cgo files are not found Oct 5, 2016
@alandonovan
Copy link
Contributor

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
Copy link
Author

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

@aletheia7
Copy link
Author

@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
Copy link
Contributor

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
Copy link

FrankReh commented Mar 5, 2017

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
Copy link
Author

Thank you!

@FrankReh
Copy link

FrankReh commented Mar 6, 2017

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
Copy link
Contributor

Exactly. Care to send me your CL?

@FrankReh
Copy link

FrankReh commented Mar 6, 2017 via email

@gopherbot
Copy link

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

@FrankReh
Copy link

FrankReh commented Mar 7, 2017

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.

FrankReh added a commit to FrankReh/tools that referenced this issue Mar 13, 2018
The logic in guru for collecting files for the definition and describe
modes had expressly avoided go files that included the import "C"
statement (known as cgo files) by setting CgoEnabled to false.

Cgo files normally get handed off to the cgo command, requiring a temp
directory and separate processes, which is much more time consuming than
having the go files parsed directly by the loader.

With this change, the cgo files are collected but then they are moved to
the go files list so the expensive cgo processing is avoided and the cgo
files are parsed along with the normal go files.

Guru already set a loader AllowErrors flag to get as much of the ASTs
returned as possible.  Failures in cgo file parsing due to cgo not
having been run is not expected to make the guru results worse.  The
only extra time should be due to extra files now being parsed, generally
making the guru search more complete.

Adds a testdata/cgo/cgo.go test case.

Fixes golang/go#13971
Fixes golang/go#15710
@unship
Copy link

unship commented Jul 12, 2019

will this fix soon?

@aletheia7
Copy link
Author

According to #24661 (comment), guru is deprecated for gopls.

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

@golang golang locked and limited conversation to collaborators Aug 22, 2020
zchee pushed a commit to zchee/golang-tools that referenced this issue Aug 14, 2021
The logic in guru for collecting files for the definition and describe
modes had expressly avoided go files that included the import "C"
statement (known as cgo files) by setting CgoEnabled to false.

Cgo files normally get handed off to the cgo command, requiring a temp
directory and separate processes, which is much more time consuming than
having the go files parsed directly by the loader.

With this change, the cgo files are collected but then they are moved to
the go files list so the expensive cgo processing is avoided and the cgo
files are parsed along with the normal go files.

Guru already set a loader AllowErrors flag to get as much of the ASTs
returned as possible.  Failures in cgo file parsing due to cgo not
having been run is not expected to make the guru results worse.  The
only extra time should be due to extra files now being parsed, generally
making the guru search more complete.

Adds a testdata/cgo/cgo.go test case.  Increases the go test time for
guru about 2%.  Because the testdata/lib package is used, the other
referrers golden files are modified.  (The cgo test case could use its
own version of the library, thereby keeping other referrers unaffected.)

Fixes golang/go#13971
Fixes golang/go#15710

Change-Id: I2962bdf2cda48e12a42ca187db97ef2cd760370c
zchee pushed a commit to zchee/golang-tools that referenced this issue Jun 23, 2023
The logic in guru for collecting files for the definition and describe
modes had expressly avoided go files that included the import "C"
statement (known as cgo files) by setting CgoEnabled to false.

Cgo files normally get handed off to the cgo command, requiring a temp
directory and separate processes, which is much more time consuming than
having the go files parsed directly by the loader.

With this change, the cgo files are collected but then they are moved to
the go files list so the expensive cgo processing is avoided and the cgo
files are parsed along with the normal go files.

Guru already set a loader AllowErrors flag to get as much of the ASTs
returned as possible.  Failures in cgo file parsing due to cgo not
having been run is not expected to make the guru results worse.  The
only extra time should be due to extra files now being parsed, generally
making the guru search more complete.

Adds a testdata/cgo/cgo.go test case.  Increases the go test time for
guru about 2%.  Because the testdata/lib package is used, the other
referrers golden files are modified.  (The cgo test case could use its
own version of the library, thereby keeping other referrers unaffected.)

Fixes golang/go#13971
Fixes golang/go#15710

Change-Id: I2962bdf2cda48e12a42ca187db97ef2cd760370c
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

8 participants