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

cmd/vet: behavioral difference on unkeyed field detection #25453

Closed
karalabe opened this issue May 18, 2018 · 6 comments

Comments

Projects
None yet
3 participants
@karalabe
Copy link
Contributor

commented May 18, 2018

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

go version go1.10.1 linux/amd64

Does this issue reproduce with the latest release?

I guess.

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

GOARCH="amd64"
GOBIN=""
GOCACHE="/home/karalabe/.cache/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/work"
GORACE=""
GOROOT="/opt/google/go"
GOTMPDIR=""
GOTOOLDIR="/opt/google/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
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-build393891535=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Create two packages:

$ cat pkg1/pkg1.go

package pkg1

import "fmt"

type Struct struct {
	Field int
}

func f() {
	fmt.Println(Struct{15})
}
cat pkg2/pkg2.go

package pkg2

import (
	"fmt"
	"path/to/pkg1"
)

func f() {
	fmt.Println(pkg1.Struct{15})
}

What did you expect to see?

When running go vet against these two package, I'd expect either both to fail with composite literal uses unkeyed fields or none of them.

What did you see instead?

$ go vet ./pkg1
$ go vet ./pkg2
# path/to/pkg2
pkg2/pkg2.go:10: path/to/pkg1.Struct composite literal uses unkeyed fields

Seems packages are allowed to use unkeyed fields locally, but not remotely. This seems like an odd thing to allow tbh.

@karalabe

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2018

Ok, another very funky thing:

On our CI run, go vet on Windows amd64 raised this same error https://ci.appveyor.com/project/ethereum/go-ethereum/build/master.6352/job/7kmh05h4gn807ruf#L286, but on Windows 386, go vet passed without raising any errors https://ci.appveyor.com/project/ethereum/go-ethereum/build/master.6352/job/fyjulsi3jjwjy3wg#L264

@mvdan

This comment has been minimized.

Copy link
Member

commented May 18, 2018

Seems packages are allowed to use unkeyed fields locally, but not remotely. This seems like an odd thing to allow tbh.

That is correct. The reasoning is that if it's a local type, the people maintaining the code are also the people maintaining the declared types. For example, unkeyed composite literals are very helpful when writing long tables, such as those used in test cases.

However, that is not documented very well:

  -composites
        check that composite literals used field-keyed elements

I'll send a patch to clarify it.

As for Windows - could you double check that the amd64 vs 386 weirdness still happens with the small reproducer you pasted above?

@mvdan mvdan changed the title go vet: behavioral difference on unkeyed field detection cmd/vet: behavioral difference on unkeyed field detection May 18, 2018

@gopherbot

This comment has been minimized.

Copy link

commented May 18, 2018

Change https://golang.org/cl/113755 mentions this issue: cmd/vet: -composites only checks foreign types

@karalabe

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2018

@mvdan I'll try to set up a new repo with just these 2 repro files and let appveyor do its stuff.

@karalabe

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2018

No, my small repro https://github.com/karalabe/govetrepro seems to work fine on all Windows architectures https://ci.appveyor.com/project/karalabe/govetrepro . Not sure what went wrong on the Ethereum CI. Anyway, I think it's safe to close this issue and I'll reopen if we ever experience it again.

@mvdan

This comment has been minimized.

Copy link
Member

commented May 18, 2018

It could well be a bug in vet, but without having a Windows machine myself to test, it's hard to debug that weirdness. Yes, I agree that we should close this for now, until we have more information.

@mvdan mvdan closed this May 18, 2018

gopherbot pushed a commit that referenced this issue May 18, 2018

cmd/vet: -composites only checks imported types
The check has worked this way for a long time, but it has never been
well documented.

For #25453.

Change-Id: If603e53348ba51f73b2f449b943c6f97f64aa3eb
Reviewed-on: https://go-review.googlesource.com/113755
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rob Pike <r@golang.org>

@golang golang locked and limited conversation to collaborators May 18, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.