x/tools/gopls: field alignment warning has issues on custom types

Karitham commented 3 years ago

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

❯ go version
go version go1.16.5 linux/amd64

❯ gopls version
golang.org/x/tools/gopls v0.7.0
    golang.org/x/tools/gopls@v0.7.0 h1:JQBHW81Gsyim6iDjUwGoPeSpXrSqwen3isPJLfDfaYU=

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

go env Output

❯ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/karitham/.cache/go-build"
GOENV="/home/karitham/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/karitham/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/karitham/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.16.5"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
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-build3891545200=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Have a struct with a custom type, for example

// session/permission.go
type Permission int64

// ./session.go
type User struct {
    DeletedAt  sql.NullTime       `json:"deleted_at"`
    Email      string             `json:"email"`
    Name       string             `json:"name"`
    Password   string             `json:"password"`
    Permission session.Permission `json:"permission"`
    ID         int32              `json:"id"`
}

What did you see?

struct of size 72 could be 64

What did you expect to see instead?

No warning, since that same exact struct passes with https://pkg.go.dev/golang.org/x/tools/go/analysis/passes/fieldalignment.

The warning also disappears if I replace my custom type with

type User struct {
    DeletedAt  sql.NullTime `json:"deleted_at"`
    Email      string       `json:"email"`
    Name       string       `json:"name"`
    Password   string       `json:"password"`
    Permission int64        `json:"permission"`
    ID         int32        `json:"id"`
}

From my understanding, this issue only applied to types outside the standard library, since I get the issue on all the structs that contains custom types, either mines or from imported packages, such as https://github.com/shopspring/decimal's decimal.Decimal

mknyszek commented 3 years ago

CC @stamblerre via https://dev.golang.org/owners

Taking a quick stab at this: it looks like the analysis is assuming you're on a 32-bit platform, which is somewhat surprising to me. On a 64-bit platform that struct is 96 bytes (see https://play.golang.org/p/gbSmYJ9-2XW).

What's even weirder is if on linux/amd64 I run the above program as GOARCH=386 I get 60, which isn't either of the numbers the message is giving you.

mknyszek commented 3 years ago

BTW if I add back in the Permission type instead of a raw int64, I get the same numbers.

findleyr commented 3 years ago

This is almost certainly due to AST trimming of packages outside of the workspace: time.Time is effectively a struct{} in the definition of sql.NullTime. This analyzer unfortunately doesn't make any sense in the presence of non-workspace types.

We need a general solution for this, or should consider walking back the AST memory optimizations.

golang / go