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: fieldalignment analyzer reports confusing "errors" about memory layout #67762

Open
new001code opened this issue Jun 1, 2024 · 6 comments
Assignees
Labels
gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@new001code
Copy link

gopls version

golang.org/x/tools/gopls v0.15.3

go env

GO111MODULE='on'
GOARCH='amd64'
GOBIN='/home/new001/Code/gopath/bin'
GOCACHE='/home/new001/.cache/go-build'
GOENV='/home/new001/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/new001/Code/gopath/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/new001/Code/gopath'
GOPRIVATE=''
GOPROXY='https://proxy.golang.com.cn,direct'
GOROOT='/usr/local/etc/golang/go1.22.3'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/etc/golang/go1.22.3/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.22.3'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build3190462353=/tmp/go-build -gno-record-gcc-switches'

What did you do?

I defined a structure.

1717215303598

type ApiResponse struct {
Success bool json:"success"
Code string json:"code"
Message string json:"message"
Data interface{} json:"data"
}

What did you see happen?

Gave me a hint: "struct with 56 pointer bytes could be 40 [default]"

What did you expect to see?

I adjusted to the field order of the structure expected by gopls, but the memory size of the structure should still be 56 instead of 40.

package main

import (
"fmt"
"testing"
"unsafe"
)

type ApiResponse struct {
Data interface{} json:"data"
Code string json:"code"
Message string json:"message"
Success bool json:"success"
}

func TestStruct(t *testing.T) {
fmt.Println(unsafe.Sizeof(ApiResponse{}))
fmt.Println(unsafe.Alignof(ApiResponse{}))

fmt.Println(unsafe.Sizeof(ApiResponse{}.Code))
fmt.Println(unsafe.Sizeof(ApiResponse{}.Data))
fmt.Println(unsafe.Sizeof(ApiResponse{}.Message))
fmt.Println(unsafe.Sizeof(ApiResponse{}.Success))

}

image

Editor and settings

No response

Logs

No response

@new001code new001code added gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository. labels Jun 1, 2024
@gopherbot gopherbot added this to the Unreleased milestone Jun 1, 2024
@MikeMitchellWebDev
Copy link
Contributor

I didn't get the same hint with your file

@new001code
Copy link
Author

new001code commented Jun 2, 2024 via email

@adonovan
Copy link
Member

adonovan commented Jun 3, 2024

The message you're seeing comes from gopls' fieldalignment analyzer. It is not enabled by default, which is why others are not seeing it; you must have explicitly enabled it in your gopls configuration.

The reason it is disabled by default is that its false positive rate is almost 100%: nearly every time it reports a diagnostic, it is not due to a mistake in your code. A suboptimal field ordering may be desirable for various reasons, first among them clarity. Only in the tiny handful of data structures that show up at the top of a profile is there any need to consider field ordering, and that's not within the power of an analyzer to deduce.

Also, the diagnostics produced by the analyzer are very confusing: there are two kinds, one which refers to the size of the struct, and the other which uses the term "pointer bytes". This term is intended to refer to the prefix of the object that contains pointers and must therefore be scanned by the garbage collector. Even though I was familiar with the concept, the phrase "pointer bytes" did not make me think of it. I suspect very few users understand the working of the GC in sufficient detail to understand this term, and not be confused by it as both you and I were in different ways.

The upcoming gopls v0.16 will have a new feature: hovering over the declaring identifier of a struct type, or one of its field declarations, will reveal the type size, alignment, and offset as part of the hover information. This seems to me to be a more useful way to surface the information as it is discreet and unobtrusive and can be sought on demand by the user acting on the information provided by a heap profile.

I propose that we delete the fieldalignment analyzer altogether.

@adonovan adonovan changed the title x/tools/gopls: issue Gopls gives errors about the memory occupancy in the structure x/tools/gopls: fieldalignment analyzer reports confusing "errors" about memory layout Jun 3, 2024
@new001code
Copy link
Author

new001code commented Jun 4, 2024 via email

@findleyr
Copy link
Contributor

findleyr commented Jun 4, 2024

Another option is to make the severity of this analyzer "hint", which decreases the visibility in many LSP clients.
I'll make that change now, independent of what we decide to do with this analyzer, because it is simply wrong for the severity to be warning.

The fieldalignment analyzer was added in https://go.dev/cl/270217, as a public package in x/tools, without a proposal or even associated issue. We were more lax about the proposal process in those days, but as an unfortunate result we cannot simply delete this analyzer. We'd have to carve out a module, tag, and delete.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/590375 mentions this issue: gopls/internal/settings: remove fieldalignment analyzer

@adonovan adonovan self-assigned this Jun 4, 2024
@findleyr findleyr modified the milestones: Unreleased, gopls/v0.17.0 Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

5 participants