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

go/types: UsesCgo exposes mangled names #39072

Open
heschi opened this issue May 14, 2020 · 17 comments
Open

go/types: UsesCgo exposes mangled names #39072

heschi opened this issue May 14, 2020 · 17 comments
Assignees
Labels
Milestone

Comments

@heschi
Copy link
Contributor

@heschi heschi commented May 14, 2020

When UsesCgo is enabled, cgo symbols appear in the package as their mangled names, e.g. _CFunc_GoString. This breaks autocomplete in gopls for C symbols. Also, it exposes things that are implementation details, like the _cgo_ and __cgofn symbols.

We can try to fix this in gopls, but probably it'd be better in go/types. I'm not sure offhand how it'll work, given that each package needs its own version of "C". Maybe the package path could be rewritten to something specific?

@mdempsky
Copy link
Member

@mdempsky mdempsky commented May 14, 2020

In CL 33677, I added support for resolving C.foo as _Cprefix_foo, but otherwise left the cgo-generated symbols in place in package scope. This is adequate for mere type-checking and analysis, but evidently suboptimal for tools like gopls, which want an accurate description of identifiers declared by the package (and imported packages, like the "C" pseudo-package).

@mdempsky
Copy link
Member

@mdempsky mdempsky commented May 14, 2020

So two limitations that will need to be addressed through further cmd/cgo support, and probably can't be resolved this late in the release cycle:

  1. cmd/cgo only emits Go wrapper code for C identifiers that are actually used. E.g., if we have a package that includes <string.h> and uses C.strlen, but not C.strcmp, then go/types isn't going to know the latter exists. So a completion attempt for C.str<tab> won't be able to list it.

    Getting a complete description of all C identifiers that are accessible is possible, but would be tricky and might be prohibitively slow in practice. E.g., instead of analyzing the Go code to see what identifiers are used, cgo could analyze the C preprocessor output from the preamble and just analyze every possible C identifier.

  2. Conversely, cmd/cgo emits a single Go wrapper file for the entire package, whereas each file is only allowed to access C symbols that are declared in its import "C" preamble. I.e., even if one file includes <string.h> and uses C.strlen, that doesn't necessarily mean it's valid to use in every other file that imports C. But gopls will probably suggest it anyway.

@mdempsky
Copy link
Member

@mdempsky mdempsky commented May 14, 2020

@heschik provided https://golang.org/cl/234057 as a test case.

@gopherbot
Copy link

@gopherbot gopherbot commented May 19, 2020

Change https://golang.org/cl/234525 mentions this issue: go/types: type-check _cgo_gotypes.go as its own Package

@mdempsky
Copy link
Member

@mdempsky mdempsky commented May 19, 2020

@heschik I've uploaded CL 234525. It passes your test case. I'd appreciate if you could test it out in the context of gopls.

My expectation is that it should mean the _Cfoo_bar identifiers are no longer exposed within the cgo-using package, and that auto-completion for names from package C should work (aside from the limitations pointed out above).

I'll give the caveat that I think this should be fine, but it's somewhat more invasive than I feel really comfortable with at this point in the release cycle. I still think it's worth testing out now though, because if this is a desirable direction to go, I think we should tweak the UsesCgo API for this release (before it's cemented in place under Go 1 compat).

@gopherbot
Copy link

@gopherbot gopherbot commented May 19, 2020

Change https://golang.org/cl/234526 mentions this issue: go/types: replace Config.UsesCgo with Checker.CgoFiles

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 10, 2020

Change https://golang.org/cl/237417 mentions this issue: go/types: rename UsesCgo to go115UsesCgo

gopherbot pushed a commit that referenced this issue Jun 10, 2020
This API and functionality was added late in the Go 1.15 release
cycle, and use within gopls has revealed some shortcomings. It's
possible (but not decided) that we'll want a different API long-term,
so for now this CL renames UsesCgo to a non-exported name to avoid
long-term commitment under the Go 1 compat guarantee.

Updates #16623.
Updates #39072.

Change-Id: I04bc0c161a84adebe43e926df5df406bc794c3db
Reviewed-on: https://go-review.googlesource.com/c/go/+/237417
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
@gopherbot
Copy link

@gopherbot gopherbot commented Jun 10, 2020

Change https://golang.org/cl/237423 mentions this issue: go/packages: move TypecheckCgo to packagesinternal

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 10, 2020

Change https://golang.org/cl/237422 mentions this issue: internal/typesinternal: remove fallback check for UsesCgo

gopherbot pushed a commit to golang/tools that referenced this issue Jun 10, 2020
Updates golang/go#16623.
Updates golang/go#39072.

Change-Id: I6612cdac14faa5ad724ceb805e1d6c823b4046ff
Reviewed-on: https://go-review.googlesource.com/c/tools/+/237423
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
@heschi heschi added this to the Go1.16 milestone Jul 23, 2020
@mdempsky
Copy link
Member

@mdempsky mdempsky commented Oct 8, 2020

@heschik FYI, last night I rebased golang.org/cl/234525 and cleaned it up somewhat. It changes the "C" package so that it looks more like a normal Go package. It still fixes your test case (CL 234057).

There's still one issue I'm aware of: if you have a global C variable or macro, the Go type will likely be wrong. In particular, for something like:

// int x;
// #define xpp (x++)
import "C"

cgo makes C.x and C.xpp appear as C.int-typed values, and expressions accessing them will be type checked appropriately; but the objects declared by the pseudo C package will appear to have types *C.int and func() C.int, respectively. I have thoughts on how to fix these, but I might punt on them if this doesn't visibly affect gopls.

I'd be interested to know if you can spot any other issues.

@heschi
Copy link
Contributor Author

@heschi heschi commented Oct 21, 2020

Apologies for the rather extreme delay.

It looks good and is a definite improvement. The one thing I noticed is that while the mangled names are no longer visible at the top level, they've moved to the C package. It would be nice to hide them from there too if possible.

The incorrect types will confuse autocomplete somewhat. If the user has a function that takes a C.int, and they trigger autocompletion on that parameter, it will propose *C.x instead of C.x, which will then fail to compile. I don't think it's the end of the world.

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Oct 21, 2020

It looks good and is a definite improvement.

Thanks for the test report.

The one thing I noticed is that while the mangled names are no longer visible at the top level, they've moved to the C package. It would be nice to hide them from there too if possible.

Interesting. You mean like the _Cfoo_bar identifiers are still visible somehow to gopls users?

I thought this should hide the mangled names as non-exported (https://go-review.googlesource.com/c/go/+/234525/5/src/go/types/object.go):

func (obj *object) Exported() bool {
	if obj.pkg != nil && obj.pkg.cgo {
		return !strings.HasPrefix(obj.name, "_C")
	}
	return token.IsExported(obj.name)
}

I can probably just suppress declaring the mangled identifiers though.

@heschi
Copy link
Contributor Author

@heschi heschi commented Oct 22, 2020

Yeah. I see _GoString_, __cgo_x, __cgofn__cgo_..., etc. as autocomplete suggestions on the C package.

@heschi
Copy link
Contributor Author

@heschi heschi commented Nov 13, 2020

@mdempsky is it too late to get this merged?

@odeke-em
Copy link
Member

@odeke-em odeke-em commented Feb 6, 2021

Howdy, and thank you Heschik and Matthew!

@mdempsky is it too late to get this merged?

Yes, it currently is. However I shall punt this to Go1.17 and make it early-in-cycle.

@odeke-em odeke-em removed this from the Go1.16 milestone Feb 6, 2021
@odeke-em odeke-em added this to the Go1.17 milestone Feb 6, 2021
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Apr 27, 2021

Should we try to merge this for 1.17, or push again? Thanks.

@mdempsky mdempsky removed this from the Go1.17 milestone Apr 27, 2021
@mdempsky mdempsky added this to the Go1.18 milestone Apr 27, 2021
@einthusan
Copy link

@einthusan einthusan commented Oct 5, 2021

Bump. What is the status of this? Did this get merged into 1.17 .. ? I am using go 1.17.1 and this problem is still there. Any suggestions to workaround would be appreciated. Thanks in advance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants