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 support for authoring cgo packages #35721

Open
heschik opened this issue Nov 20, 2019 · 17 comments
Labels

Comments

@heschik
Copy link
Contributor

@heschik heschik commented Nov 20, 2019

Forked from #31561. This issue is not for users of cgo packages; that issue is #35720.

Files that import "C" are not really valid Go. They depend heavily on extra information that is produced by the go tool when it builds the package. They can be parsed, but not type checked, though there is some rudimentary support for working around the issues by setting the go/types.Config.FakeImportC flag.

Many people have suggested that gopls use the package's GoFiles rather than its CompiledGoFiles. The GoFiles are the original (invalid) files. Using those may give a slightly better experience for people editing the package, but it will completely break anything that depends on the type information, since the package will no longer type check. We would have to evaluate the effects very carefully before making that change in the current architecture.

We could consider type checking cgo packages twice, once with the real code and once with the user's code. That would be a significant architectural change.

Alternatively, we could run the cgo tool as the .go files change to produce the real code. This is a much more robust approach, but the cost of running cgo processing may be too high, especially on large packages.

No matter what, this is a large project that will need a lot of testing and thought. It's unlikely we'll make huge improvements here very soon.

@OneOfOne

This comment has been minimized.

Copy link
Contributor

@OneOfOne OneOfOne commented Nov 20, 2019

A suggestion (not sure how viable it is) would be to use clangd if it's available and proxy the C code to it.

@heschik

This comment has been minimized.

Copy link
Contributor Author

@heschik heschik commented Nov 20, 2019

That's probably overkill, but does point to an approach I didn't mention: tightly integrate cgo processing into gopls so that we can mitigate the performance problems of running the actual cgo tool. As you imply, the expensive part of cgo is learning about the C code. If we could separate that part from the code generation part, we could cache the C information and run just code generation when .go files changed. That might be the ideal solution, but would require changes to the cgo tool's implementation.

@mdempsky

This comment has been minimized.

Copy link
Member

@mdempsky mdempsky commented Nov 20, 2019

Possibly related to #16623?

(It's been a while since I've worked on any go/types-based tools though. I seem to recall something changed where it became easier to process cgo-using packages, if still not perfect, and working on #16623 dropped in priority for me... maybe column position information?)

@heschik

This comment has been minimized.

Copy link
Contributor Author

@heschik heschik commented Nov 20, 2019

Definitely related. If the net result of that change was that GoFiles was [x.go, y.go] and CompiledGoFiles was [x.go,y.go,$GOCACHE/.../_cgo_gotypes.go] per #16623 (comment) then I think we would be in much better shape.

@mdempsky

This comment has been minimized.

Copy link
Member

@mdempsky mdempsky commented Nov 20, 2019

@heschik To confirm, by "much better shape" do you mean you think there would still be further work to do to address this issue? (Seems like this would address #35720 too, actually.)

Looking at CLs 33677 and 33678 again, I think they're probably in reasonable shape to polish up for the next release cycle. I think the main issues are:

  1. Is go/types.Config.UsesCgo the right API for enabling that setting within go/types?

  2. CL 33678 needs to set go/types.Config.UsesCgo instead of FakeImportC. (CL 33677 PS1 originally just extended the semantics of FakeImportC; PS4 instead added UsesCgo, but it looks like I never pushed a matching revision of CL 33678.)

  3. It probably also needs to be ported to go/packages too? How does gopls actually invoke go/types?

@heschik

This comment has been minimized.

Copy link
Contributor Author

@heschik heschik commented Nov 21, 2019

Further work: Yes -- we'd still have to regenerate the generated file sometimes. But that's a much more manageable concern.

#35720: I think we want a fix for that before 1.15 is released. Otherwise I'd say yeah.

If you have a set of patches that work against master, even if the API is rough, I'm happy to work to get go/packages working on top of it if that's necessary. But as long as go list -json returns the right stuff in CompiledGoFiles there shouldn't be anything much to do for either go/packages or gopls -- both just follow go list's lead.

@mdempsky

This comment has been minimized.

Copy link
Member

@mdempsky mdempsky commented Nov 21, 2019

@heschik FYI, I just rebased CL 33677 and fixed merge conflicts. I verified that tests still pass, but haven't done anything more extensive than that.

I don't see CompiledGoFiles when I run go list -json net though. Is cmd/go supposed to be running cmd/cgo?

@mdempsky

This comment has been minimized.

Copy link
Member

@mdempsky mdempsky commented Nov 21, 2019

Nevermind. Read the docs and realized it needs to be invoked as go list -json -compiled net.

@mdempsky

This comment has been minimized.

Copy link
Member

@mdempsky mdempsky commented Nov 21, 2019

I think cmd/go will need a new flag to indicate that you want the unprocessed versions of CgoFiles rather than the processed versions. Old tools might not be able to handle the processed versions, and we can't just break those. I'm not sure what the best way to control this is.

However, for the short term, here's what I think you can do within go/packages:

  1. CompiledGoFiles consists of (in this order): GoFiles, cgo(CgoFiles), SFiles, CFiles, CXXFiles.

  2. The cgo(CgoFiles) list consists of 1 entry per CgoFiles, plus 2 more fixed entres. _cgo_types.go is the first element (and is what provides the C pseudo-package declarations); the last one is _cgo_dynimport.go (which just provides compiler directives that influence linker behavior, and isn't relevant to go/types).

  3. You want to pass to go/types: GoFiles, CgoFiles, and the first file from cgo(CgoFiles). I.e., rather than taking CompiledGoFiles and filtering out the non-go files, take GoFiles; and if CgoFiles is non-empty, take CgoFiles and CompiledGoFiles[len(GoFiles)] too.

  4. You also need to set go/types.Config.UsesCgo = true.

@mdempsky

This comment has been minimized.

Copy link
Member

@mdempsky mdempsky commented Nov 21, 2019

https://go-review.googlesource.com/c/tools/+/208264 is an ugly hack to do the above. It passes go test, but I haven't tried using it with gopls.

@mdempsky

This comment has been minimized.

Copy link
Member

@mdempsky mdempsky commented Nov 21, 2019

Updated CL 33677 for parity with CL 43970.

With CL 33677 and CL 208264 in place, I can run:

$ cd $(go env GOROOT)/misc/cgo/test
$ GOPACKAGESDRIVER=off gopackages -json -mode=types .

and confirm that it's processing the original .go sources, and it's type-checking okay.

Edit: GOPACKAGESDRIVER=off is necessary because I simply hacked the golist driver implementation. The proper solution here will probably require extending the go/packages driver interface.

@mdempsky

This comment has been minimized.

Copy link
Member

@mdempsky mdempsky commented Nov 22, 2019

@heschik I think CLs 33677 and 208264 should be in good shape if you want to play with them in the context of gopls.

@heschik

This comment has been minimized.

Copy link
Contributor Author

@heschik heschik commented Nov 22, 2019

I'm just about done with #35720, and then I'm excited to give the patches a try. Should be Monday.

@heschik

This comment has been minimized.

Copy link
Contributor Author

@heschik heschik commented Nov 25, 2019

So, this is a bit of a good news/bad news situation.

The good news is that @mdempsky's patches work beautifully, and features like autocomplete start working in cgo files with no changes to gopls whatsoever. As I said before, there's still some work to re-trigger cgo compilation when appropriate, and the import "C" line currently shows an error, but these should be pretty tractable problems.

The bad news is that these are changes to the compiler and standard library. As such, they won't be ready until Go 1.15, scheduled for about 8 months from now. Hopefully we can land them early in the cycle so that people can try them out. If there's demand, I can write up some instructions on how to set up an environment with local patches even now.

@mdempsky

This comment has been minimized.

Copy link
Member

@mdempsky mdempsky commented Nov 25, 2019

The bad news is that these are changes to the compiler and standard library.

It's only a standard library change, so it should be possible to backport the changes via vendoring if gopls users want to use it before 1.15.

@heschik

This comment has been minimized.

Copy link
Contributor Author

@heschik heschik commented Nov 25, 2019

I don't think that's a good option in module mode, unfortunately.

@mdempsky

This comment has been minimized.

Copy link
Member

@mdempsky mdempsky commented Nov 25, 2019

@heschik Fair enough. I'm still very much a modules noob, so I defer to your expertise on how best to address the issue. I mostly wanted to clarify that compiler changes aren't involved here.

I'd be curious what happens for "jump to definition" on a C.foo symbol. I'm guessing it jumps to the _cgo_gotypes.go file?

We could maybe get cgo to emit //line directives to make it point into the C preamble and/or header files if that would be more useful.

@stamblerre stamblerre modified the milestones: Unreleased, gopls unplanned Dec 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.