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 users of cgo packages #35720

Closed
heschik opened this issue Nov 20, 2019 · 8 comments
Closed

x/tools/gopls: improve support for users of cgo packages #35720

heschik opened this issue Nov 20, 2019 · 8 comments
Labels
Milestone

Comments

@heschik
Copy link
Contributor

@heschik heschik commented Nov 20, 2019

Forked from #31561. This issue is not for packages that directly import "C"; that issue is #35721.

Various features, notably Go to Definition and friends, fail when the declaring package uses cgo. That's because the Go code for cgo files is actually in a generated file in the build cache, not the .go file the user might expect. The generated file has //line directives that point to the original code, but gopls is currently not honoring those directives. Fixing that should improve the experience for people who depend on a cgo library.

If there are other features that break when depending on a cgo package, please note them here so that we can make sure they're fixed and add tests.

@heschik

This comment has been minimized.

Copy link
Contributor Author

@heschik heschik commented Nov 20, 2019

#34345 says that completions don't work, I'll test those too.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 20, 2019

Change https://golang.org/cl/208101 mentions this issue: internal/lsp: rename Files to CompiledGoFiles

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 20, 2019

Change https://golang.org/cl/208100 mentions this issue: internal: avoid use of (*token.File).Name

gopherbot pushed a commit to golang/tools that referenced this issue Nov 20, 2019
When line directives are in use, we want the logical file name, not the
one we found the bytes in. This matters most for cgo, where the file we
parsed is not the one the user wants to see.

Updates golang/go#35720.

Change-Id: I495328071d8865e6895cb731467f1601f11e93db
Reviewed-on: https://go-review.googlesource.com/c/tools/+/208100
Run-TryBot: Heschi Kreinick <heschi@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
gopherbot pushed a commit to golang/tools that referenced this issue Nov 20, 2019
As we improve support for cgo we'll need to reference GoFiles, not just
CompiledGoFiles. "Files" is right out.

I think I got everything that needs renaming but please let me know if
not.

Updates golang/go#35720.

Change-Id: I97a6ebf5b395535de0d5f4f8b3f84b46ca34643f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/208101
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 22, 2019

Change https://golang.org/cl/208501 mentions this issue: internal/span: support line directives

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 22, 2019

Change https://golang.org/cl/208502 mentions this issue: internal/lsp: track and parse non-compiled go files

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 22, 2019

Change https://golang.org/cl/208503 mentions this issue: internal/lsp: add tests for cgo package users

gopherbot pushed a commit to golang/tools that referenced this issue Nov 25, 2019
When //line directives are in play, the ast.File's Offset function will
return offsets in the generated file. We want offsets in the authored
file, so we need to pass a Converter for the authored file, in addition
to the ast.File for the generated file. For the same reason, we have to
start (Range).Span() by translating into positions in the authored file,
then calculate offsets from that.

A lot of call sites outside of the LSP don't pass the Converter, but
they probably don't matter much. I think everything inside does because
it ends up using mappedRange.

Updates golang/go#35720.

Change-Id: I7be09b3a50720b078e862d48cfdb02208f8187ae
Reviewed-on: https://go-review.googlesource.com/c/tools/+/208501
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
gopherbot pushed a commit to golang/tools that referenced this issue Nov 25, 2019
When packages.Load'ing cgo packages, the authored files show up in
GoFiles, and the generated files show up in CompiledGoFiles. We need the
AST and type information for the latter, since they're the only thing we
can type check. But we also need the contents (and column mapper) for
the authored file so that we can navigate into it.

Store GoFiles in package metadata and checked Packages. Parse the extra
files, just for their mappers. Refactor the View functions a little bit,
since there's only one place that actually needs to find the mapper for
a file.

Updates golang/go#35720.

Change-Id: I9f96872a9a592bf0e11da27ebd8976c6db8752c9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/208502
Run-TryBot: Heschi Kreinick <heschi@google.com>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@frou

This comment has been minimized.

Copy link
Contributor

@frou frou commented Jan 18, 2020

Thank you for improving this. Does golang.org/x/tools/gopls@latest not include it yet?

@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Jan 21, 2020

@frou: It does not yet, but I will update #33030 when a new version (and this fix) is released.

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
4 participants
You can’t perform that action at this time.