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/gopls: improve cgo support #32898

Closed
stamblerre opened this issue Jul 2, 2019 · 10 comments
Closed

x/tools/gopls: improve cgo support #32898

stamblerre opened this issue Jul 2, 2019 · 10 comments
Assignees
Milestone

Comments

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Jul 2, 2019

Original issue: #31561.
Currently, gopls does not crash with cgo, but language features are still missing for any symbols imported from "C".

@myitcv

This comment has been minimized.

Copy link
Member

@myitcv myitcv commented Jul 30, 2019

Just to confirm, any file that imports "C" is affected by this, correct?

package p

/*
#include <stdio.h>
#include <stdlib.h>

void myprint(char* s) {
	printf("%s\n", s);
}
*/
import "C"

import "fmt"

import "unsafe"

func Example() {
	fmt.Println()
	cs := C.CString("Hello from stdio\n")
	C.myprint(cs)
	C.free(unsafe.Pointer(cs))
}

For example, an attempt to go-to-def on fmt.Println results in:

GetAST: unable to check package for file:///home/myitcv/gostuff/src/github.com/myitcv/playground/p/p.go: loadParseTypecheck: no metadata found for /home/myitcv/gostuff/src/github.com/myitcv/playground/p/p.go

At least based on some rough tests, I can't get anything to work when a file imports "C".

@myitcv

This comment has been minimized.

Copy link
Member

@myitcv myitcv commented Jul 30, 2019

Actually, this appears to be limited to files that import "C". I've updated my previous comment to reflect this.

@myitcv

This comment has been minimized.

Copy link
Member

@myitcv myitcv commented Jul 30, 2019

Here is a test that demonstrates the issue within govim:

https://github.com/myitcv/govim/blob/cmd_govim_cgo_example/cmd/govim/testdata/cgo_support.txt

This attempts to go-to-def from SameFileN in each of the two files. In the file that does import "C" all is good; in the one that does we get:

GetAST: unable to check package for file://$WORK/p_importc.go: loadParseTypecheck: no metadata found for $WORK/p_importc.go
@stamblerre stamblerre modified the milestones: Unreleased, gopls v1.0 Aug 26, 2019
@gopherbot gopherbot added the Tools label Sep 12, 2019
@ctxkenb

This comment has been minimized.

Copy link

@ctxkenb ctxkenb commented Sep 14, 2019

I think I've found the cause of this. go list -json returns cached files in the list of CompileGoFiles, in this form (on Mac):

	"CompiledGoFiles": [
		"a.go",
		"b.go",
		"/Users/me/Library/Caches/go-build/d0/d0806a53076964ea7f690a3852e2ada78bc4bf692c37febfacbb6f22bb0c74e5-d",
		"/Users/me/Library/Caches/go-build/f0/f02a2cefdc3e9521cf5705b4c5d454f0b606b37fc6f3186eb6f05235e53979a3-d",
	]

These cache files appear to be the cgo source. Because the name doesn't match the file name of the original source, gopls is unable to attribute the metadata.

I'm not sure what the 'right' solution is, but i found a quick hack that enumerates GoFiles and CgoFiles instead of using CompiledGoFiles: (golang.org/x/tools)

diff --git a/go/packages/packages.go b/go/packages/packages.go
index 4ca2b117..75b91f9d 100644
--- a/go/packages/packages.go
+++ b/go/packages/packages.go
@@ -249,6 +249,8 @@ type Package struct {
        // GoFiles lists the absolute file paths of the package's Go source files.
        GoFiles []string
 
+       CgoFiles []string
+
        // CompiledGoFiles lists the absolute file paths of the package's source
        // files that were presented to the compiler.
        // This may differ from GoFiles if files are processed before compilation.
@@ -333,6 +335,7 @@ type flatPackage struct {
        PkgPath         string            `json:",omitempty"`
        Errors          []Error           `json:",omitempty"`
        GoFiles         []string          `json:",omitempty"`
+       CgoFiles        []string          `json:",omitempty"`
        CompiledGoFiles []string          `json:",omitempty"`
        OtherFiles      []string          `json:",omitempty"`
        ExportFile      string            `json:",omitempty"`
@@ -381,6 +384,7 @@ func (p *Package) UnmarshalJSON(b []byte) error {
                PkgPath:         flat.PkgPath,
                Errors:          flat.Errors,
                GoFiles:         flat.GoFiles,
+               CgoFiles:        flat.CgoFiles,
                CompiledGoFiles: flat.CompiledGoFiles,
                OtherFiles:      flat.OtherFiles,
                ExportFile:      flat.ExportFile,
diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go
index 6b1320bc..ba7abd6e 100644
--- a/internal/lsp/cache/load.go
+++ b/internal/lsp/cache/load.go
@@ -241,7 +241,7 @@ func (v *view) link(ctx context.Context, g *importGraph) error {
                typesSizes: g.pkg.TypesSizes,
                errors:     g.pkg.Errors,
        }
-       for _, filename := range g.pkg.CompiledGoFiles {
+       for _, filename := range append(g.pkg.GoFiles, g.pkg.CgoFiles...) {
                m.files = append(m.files, span.FileURI(filename))
 
                // Call the unlocked version of getFile since we are holding the view's mutex.
@muirdm

This comment has been minimized.

Copy link

@muirdm muirdm commented Sep 19, 2019

The generated files that correspond to real files have a //line ... directive mentioning the real file right? Maybe we can parse those to map them back to the real file, then proxy requests/responses between the real file and generated file, adjusting the file offsets as necessary?

@ctxkenb

This comment has been minimized.

Copy link

@ctxkenb ctxkenb commented Oct 11, 2019

load.go got changed in master, here's an updated diff for the hacky fix...

diff --git a/go/packages/packages.go b/go/packages/packages.go
index b29c9136..f025c2f5 100644
--- a/go/packages/packages.go
+++ b/go/packages/packages.go
@@ -248,6 +248,8 @@ type Package struct {
        // GoFiles lists the absolute file paths of the package's Go source files.
        GoFiles []string
 
+       CgoFiles []string
+
        // CompiledGoFiles lists the absolute file paths of the package's source
        // files that were presented to the compiler.
        // This may differ from GoFiles if files are processed before compilation.
@@ -332,6 +334,7 @@ type flatPackage struct {
        PkgPath         string            `json:",omitempty"`
        Errors          []Error           `json:",omitempty"`
        GoFiles         []string          `json:",omitempty"`
+       CgoFiles        []string          `json:",omitempty"`
        CompiledGoFiles []string          `json:",omitempty"`
        OtherFiles      []string          `json:",omitempty"`
        ExportFile      string            `json:",omitempty"`
@@ -380,6 +383,7 @@ func (p *Package) UnmarshalJSON(b []byte) error {
                PkgPath:         flat.PkgPath,
                Errors:          flat.Errors,
                GoFiles:         flat.GoFiles,
+               CgoFiles:        flat.CgoFiles,
                CompiledGoFiles: flat.CompiledGoFiles,
                OtherFiles:      flat.OtherFiles,
                ExportFile:      flat.ExportFile,
diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go
index cdbe9703..fe42c4b6 100644
--- a/internal/lsp/cache/load.go
+++ b/internal/lsp/cache/load.go
@@ -152,7 +152,7 @@ func (s *snapshot) updateMetadata(ctx context.Context, uri span.URI, pkgs []*pac
 
        var results []*metadata
        for _, pkg := range pkgs {
-               log.Print(ctx, "go/packages.Load", tag.Of("package", pkg.PkgPath), tag.Of("files", pkg.CompiledGoFiles))
+               log.Print(ctx, "go/packages.Load", tag.Of("package", pkg.PkgPath), tag.Of("files", append(pkg.GoFiles, pkg.CgoFiles...)))
 
                // Set the metadata for this package.
                if err := s.updateImports(ctx, packagePath(pkg.PkgPath), pkg, cfg); err != nil {
@@ -183,7 +183,7 @@ func (s *snapshot) updateImports(ctx context.Context, pkgPath packagePath, pkg *
                errors:     pkg.Errors,
                config:     cfg,
        }
-       for _, filename := range pkg.CompiledGoFiles {
+       for _, filename := range append(pkg.GoFiles, pkg.CgoFiles...) {
                uri := span.FileURI(filename)
                m.files = append(m.files, uri)
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 20, 2019

Change https://golang.org/cl/202238 mentions this issue: internal/lsp: use Go/cgo source files instead of generated files

@ctxkenb

This comment has been minimized.

Copy link

@ctxkenb ctxkenb commented Oct 26, 2019

@stamblerre I've gone back and done a more detailed analysis for the change in https://golang.org/cl/202238. I've come to the same conclusion that the 'ids' map in snapshot should be populated from pkg.GoFiles.

It might be that i should raise a separate issue for the change, as it doesn't really add 'cgo support' - what it does do is fix the breakage of gopls on Go symbols in packages where there is a cgo file.
From my own use in VS Code, the change makes completion, find references, lint all spring into life for Go symbols in cgo files. What it doesn't do is expose C symbols or fix the issue where import "C" is flagged as an error ("could not import package C").

Analysis is here: https://docs.google.com/document/d/1r5iVQdcAhMhGKDi0rc-sgBF4ZAsrZuZvRs9Zz2Y_6vQ/edit?usp=sharing

There are two different types of generation as far as I could model it. For go generate and protobuf, the generated Go files are present on disk, and so in pkg.GoFiles. Alternately, where pkg.CompiledGoFiles seems to be populated with generated files, it seems to be limited to files generated by go build that the user wouldn't normally expect to interact with (i.e. source generated by cgo)

I wasn't able to locate anywhere in the gopls code where the clients of the snapshot.ids map are currently expecting it to be populated with the CompiledGoFiles list.

I'm hoping you'll reconsider the change, and guide me if it would be better to raise a separate issue for the improvement the change makes.

@frou

This comment has been minimized.

Copy link
Contributor

@frou frou commented Nov 13, 2019

It's a shame that gopls gets jammed by cgo being involved, because this hampers the value of the wonderfully clean OpenGL wrapper packages provided by the go-gl organisation.

@heschik

This comment has been minimized.

Copy link
Contributor

@heschik heschik commented Nov 20, 2019

Closing this in favor of #35720 and #35721. Please direct any followups to the appropriate one of those.

@heschik heschik closed this Nov 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.