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: incorrect type reported for comma-err C functions (manifests itself in IDEs) #47777

Closed
AndSDev opened this issue Aug 18, 2021 · 17 comments
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@AndSDev
Copy link

AndSDev commented Aug 18, 2021

Incorrect type for error in C-binding in IDE.
Go build / run works fine, but in IDE (VS Code) type of err is bool instead of error.

What version of Go are you using (go version)?

$ go version
go version go1.17 linux/amd64
$ gopls version
golang.org/x/tools/gopls v0.7.1
    golang.org/x/tools/gopls@v0.7.1 h1:Mh3Z8Xcoq3Zy7ksSlwDV/nzQSbjFf06A+L+F8YHq55U

Does this issue reproduce with the latest release?

Yes. Only the latest release is affected (1.17).

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/dev/.cache/go-build"
GOENV="/home/dev/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/dev/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/dev/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/dev/.local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/dev/.local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.17"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/dev/go/src/sandbox/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build3354119137=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Open the following in VS Code (main.go):

package main

// int fortytwo() { return 42; }
import "C"
import "fmt"

func main() {
	r, err := C.fortytwo()
	if err != nil {
		panic(err)
	}
	fmt.Println(r)
	// Output: 42
}

What did you expect to see?

Type of err is error.

What did you see instead?

Type of err is bool.
IDE shows error: cannot convert nil (untyped nil value) to bool.

cgo_gopls_diff

@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Aug 18, 2021
@gopherbot gopherbot added this to the Unreleased milestone Aug 18, 2021
@AndSDev AndSDev changed the title x/tools/gopls: incorrect type for error in C-binding in IDE x/tools/gopls: incorrect type for error in C-binding (CGO) in IDE Aug 18, 2021
@AndSDev AndSDev changed the title x/tools/gopls: incorrect type for error in C-binding (CGO) in IDE x/tools/gopls: incorrect type for error in C-binding (CGO) in IDE (go 1.17) Aug 18, 2021
@stamblerre
Copy link
Contributor

@AndSDev: Does clicking the "regenerate cgo definitions" button fix the problem?

@stamblerre stamblerre added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Aug 18, 2021
@AndSDev
Copy link
Author

AndSDev commented Aug 18, 2021

@AndSDev: Does clicking the "regenerate cgo definitions" button fix the problem?

"regenerate cgo definitions" does not help.

After switching the go version, VS Code asks to update the tools (in the notification) - after the update there is a problem.
Doing "Go: Install/Update tools" manually does not solve the problem.
"Go: Install/Update tools" allows to select packages to update.
If I manually rebuild only the gopls (after switching the go version and without rebuilding everything from the notification), there will be problems.
If I manually rebuild everything without gopls (after switching the go version and without rebuilding everything from the notification) - no problem.

@stamblerre
Copy link
Contributor

Can you please share your gopls logs when you see the issue? Information on how to capture them can be found here.

@zosmac
Copy link

zosmac commented Aug 18, 2021

VSCode source and problems view

@zosmac
Copy link

zosmac commented Aug 18, 2021

VSCode gopls output.txt

@zosmac
Copy link

zosmac commented Aug 18, 2021

I too am seeing this problem (on macOS/Darwin) with gopls reporting a type mismatch for C function calls, expecting a bool return type rather than errno/error.

Go 1.17,
VSCode 1.59.0
gopls 0.7.1

@zosmac
Copy link

zosmac commented Aug 18, 2021

Interestingly, each function that is complaining about the mismatched error type has the C interface routine defined twice, one with two return values, and one with only one return value.

@zosmac
Copy link

zosmac commented Aug 18, 2021

If I select "Go to Definition" for C.proc_pidfdinfo called in the example above, VSCode takes me to this:

//go:cgo_unsafe_args
func _Cfunc_proc_pidfdinfo(p0 _Ctype_int, p1 _Ctype_int, p2 _Ctype_int, p3 unsafe.Pointer, p4 _Ctype_int) (r1 _Ctype_int) {
_cgo_runtime_cgocall(_cgo_577eabb07159_Cfunc_proc_pidfdinfo, uintptr(unsafe.Pointer(&p0)))
if _Cgo_always_false {
_Cgo_use(p0)
_Cgo_use(p1)
_Cgo_use(p2)
_Cgo_use(p3)
_Cgo_use(p4)
}
return
}

@zosmac
Copy link

zosmac commented Aug 18, 2021

However, if I search in the file for proc_pidfdinfo, I also find this:

//go:cgo_import_static _cgo_577eabb07159_C2func_proc_pidfdinfo
//go:linkname __cgofn__cgo_577eabb07159_C2func_proc_pidfdinfo _cgo_577eabb07159_C2func_proc_pidfdinfo
var __cgofn__cgo_577eabb07159_C2func_proc_pidfdinfo byte
var _cgo_577eabb07159_C2func_proc_pidfdinfo = unsafe.Pointer(&__cgofn__cgo_577eabb07159_C2func_proc_pidfdinfo)

//go:cgo_unsafe_args
func _C2func_proc_pidfdinfo(p0 _Ctype_int, p1 _Ctype_int, p2 _Ctype_int, p3 unsafe.Pointer, p4 _Ctype_int) (r1 _Ctype_int, r2 error) {
errno := _cgo_runtime_cgocall(_cgo_577eabb07159_C2func_proc_pidfdinfo, uintptr(unsafe.Pointer(&p0)))
if errno != 0 { r2 = syscall.Errno(errno) }
if _Cgo_always_false {
_Cgo_use(p0)
_Cgo_use(p1)
_Cgo_use(p2)
_Cgo_use(p3)
_Cgo_use(p4)
}
return
}

@AndSDev
Copy link
Author

AndSDev commented Aug 19, 2021

I reproduce this issue on a new user (all caches are empty, clear install of go 1.17).
gopls trace with verbose flag (for empty user):
trace_verb.log

@findleyr
Copy link
Contributor

This is a regression in go/types in Go 1.17, introduced by me.

Bisected to https://golang.org/cl/282193, where at call.go:305 I checked for commaerr operands, but overlooked that the operand mode had already been set to value on line 303.

This needs to be fixed and backported to 1.17. @mdempsky might know more about the potential impact of this bug beyond gopls.

CC @griesemer

I have spotty internet right now, so might be slow to respond.

@mdempsky
Copy link
Member

I think commaerr should only affect gopls. The UsesCgo flag isn't generally available to other go/types users yet.

@griesemer
Copy link
Contributor

I can reproduce this and have a fix. Will send out CL a bit later today.

@griesemer griesemer self-assigned this Aug 19, 2021
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/343809 mentions this issue: go/types: don't override x.mode before using it

@griesemer griesemer modified the milestones: gopls/unplanned, Go1.17.1 Aug 20, 2021
@griesemer griesemer removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Aug 20, 2021
@griesemer griesemer changed the title x/tools/gopls: incorrect type for error in C-binding (CGO) in IDE (go 1.17) go/types: incorrect type reported for comma-err C functions (manifests itself in IDEs) Aug 20, 2021
@griesemer
Copy link
Contributor

@gopherbot please backport to 1.17. This is a regression.

@gopherbot
Copy link
Contributor

Backport issue(s) opened: #47854 (for 1.17).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@dmitshur dmitshur added the NeedsFix The path to resolution is known, but the work has not been done. label Sep 1, 2021
@dmitshur
Copy link
Contributor

dmitshur commented Sep 1, 2021

The fix that closed this issue landed into master branch, which is where Go 1.18 development is happening, so moving this issue to the Go1.18 milestone. (The 1.17 backport issue #47854 is in 1.17.1 milestone.)

@dmitshur dmitshur modified the milestones: Go1.17.1, Go1.18 Sep 1, 2021
@golang golang locked and limited conversation to collaborators Jun 23, 2023
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. NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

8 participants