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
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@stamblerre
Copy link
Contributor

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

@stamblerre stamblerre added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. gopls Issues related to the Go language server, gopls. labels Jul 2, 2019
@gopherbot gopherbot added this to the Unreleased milestone Jul 2, 2019
@myitcv
Copy link
Member

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

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

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 This label describes issues relating to any tools in the x/tools repository. label Sep 12, 2019
@ctxkenb
Copy link

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

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

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

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

@ctxkenb
Copy link

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

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.

@heschi
Copy link
Contributor

heschi commented Nov 20, 2019

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

@heschi heschi closed this as completed Nov 20, 2019
@stamblerre stamblerre modified the milestones: gopls/v1.0.0, gopls/v0.4.0 Jul 22, 2020
@golang golang locked and limited conversation to collaborators Jul 22, 2021
@rsc rsc unassigned heschi Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

7 participants