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
Comments
Debian: testing/stretch. Fully apt-get'd. |
Possible duplicate of Issue #13971. (That relates to 'describe' operations, but I think the underlying cause is the same.) |
Good. Here is the output from guru describe:
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) |
@aletheia7 I ran into the same issue with the same type. That |
To avoid the cgo files ending up in the wrong bucket, the loader must not call |
Thank you. Guru works great for packages under vendor that do not import "C." |
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. |
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. |
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. 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? |
Thank you! |
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. |
Exactly. Care to send me your CL? |
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 ***@***.***> 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.
|
CL https://golang.org/cl/37856 mentions this issue. |
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. |
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
will this fix soon? |
According to #24661 (comment), guru is deprecated for gopls. vim-go version 1.20+ is supporting (maybe starting to support) gopls. |
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
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
golang guru/GoDef cannot find non-golang package documentation that is in the
GOPATH
.Please answer these questions before submitting your issue. Thanks!
What version of Go are you using (
go version
)?go version go1.6.1 linux/386
What operating system and processor architecture are you using (
go env
)?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.
What did you expect to see?
Please see the issue above.
Please see the issue above.
The text was updated successfully, but these errors were encountered: