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

govet fieldalignment produces false positives for embedded structures #1825

Closed
4 tasks done
iamtakingiteasy opened this issue Mar 9, 2021 · 6 comments
Closed
4 tasks done
Labels
question Further information is requested

Comments

@iamtakingiteasy
Copy link

  • Yes, I'm using a binary release within 2 latest major releases. Only such installations are supported.
  • Yes, I've searched similar issues on GitHub and didn't find any.
  • Yes, I've included all information below (version, config, etc).
  • Yes, I've tried with the standalone linter if available. (https://golangci-lint.run/usage/linters/)
Description of the problem

govet's fieldalignment produces false positives for embedded structures. maligned has no such issue, running go vet also does not report same thing.

Version of golangci-lint
$ golangci-lint --version
golangci-lint has version 1.38.0 built from 507703b4 on 2021-03-03T13:53:01Z
Config file
$ cat .golangci.yml
run:
  concurrency: 4
  deadline: 5m
  issues-exit-code: 1
  tests: true

output:
  format: colored-line-number
  print-issued-lines: true
  print-linter-name: true

linters-settings:
  govet:
    check-shadowing: true
    enable:
      - fieldalignment

linters:
  enable:
    - govet
  disable-all: true
Go environment
$ go version && go env
go version go1.15.8 linux/amd64
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/user/.cache/go-build"
GOENV="/home/user/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/go/pkg/mod"
GOOS="linux"
GOPATH="/home/go"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
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-build360390239=/tmp/go-build -gno-record-gcc-switches"
Verbose output of running
$ golangci-lint cache clean
$ golangci-lint run -v
INFO [config_reader] Config search paths: [./ /tmp/z /tmp / /home/user] 
INFO [config_reader] Used config file .golangci.yml 
INFO [lintersdb] Active 1 linters: [govet]        
INFO [loader] Go packages loading at mode 575 (deps|compiled_files|exports_file|files|imports|name|types_sizes) took 86.458911ms 
INFO [runner/filename_unadjuster] Pre-built 0 adjustments in 81.861µs 
INFO [linters context/goanalysis] analyzers took 1.044009ms with top 10 stages: ctrlflow: 295.127µs, printf: 246.776µs, unmarshal: 125.055µs, fieldalignment: 61.93µs, inspect: 43.601µs, shadow: 25.146µs, framepointer: 22.24µs, buildtag: 19.501µs, unreachable: 17.748µs, unusedresult: 17.265µs 
INFO [runner] Processors filtering stat (out/in): exclude-rules: 1/1, uniq_by_line: 1/1, path_prefixer: 1/1, path_prettifier: 1/1, autogenerated_exclude: 1/1, max_per_file_from_linter: 1/1, severity-rules: 1/1, cgo: 1/1, filename_unadjuster: 1/1, skip_files: 1/1, skip_dirs: 1/1, identifier_marker: 1/1, diff: 1/1, exclude: 1/1, nolint: 1/1, max_same_issues: 1/1, max_from_linter: 1/1, source_code: 1/1, path_shortener: 1/1, sort_results: 1/1 
INFO [runner] processing took 164.092µs with stages: identifier_marker: 46.124µs, nolint: 36.919µs, exclude-rules: 31.245µs, autogenerated_exclude: 13.726µs, source_code: 11.264µs, path_prettifier: 10.725µs, skip_dirs: 5.291µs, uniq_by_line: 1.645µs, cgo: 1.392µs, max_same_issues: 1.387µs, path_shortener: 891ns, max_from_linter: 856ns, filename_unadjuster: 677ns, max_per_file_from_linter: 530ns, diff: 372ns, skip_files: 283ns, severity-rules: 282ns, exclude: 213ns, sort_results: 155ns, path_prefixer: 115ns 
INFO [runner] linters took 58.431317ms with stages: govet: 58.203915ms 
foo.go:12:10: fieldalignment: struct with 24 pointer bytes could be 16 (govet)
type exA struct {
         ^
INFO File cache stats: 1 entries of total size 262B 
INFO Memory: 3 samples, avg is 72.5MB, max is 72.6MB 
INFO Execution took 148.770601ms  
Code example or link to a public repository
package main

import (
	"fmt"
	"unsafe"
)

type some struct {
	F *int32
}

type exA struct {
	S string
	some
}

type exB struct {
	some
	S string
}

func main() {
	a := exA{}
	b := exB{}

	fmt.Println("a", unsafe.Sizeof(a)) // 24
	fmt.Println("b", unsafe.Sizeof(b)) // 24
}
@iamtakingiteasy iamtakingiteasy added the bug Something isn't working label Mar 9, 2021
@boring-cyborg
Copy link

boring-cyborg bot commented Mar 9, 2021

Hey, thank you for opening your first Issue ! 🙂 If you would like to contribute we have a guide for contributors.

@iamtakingiteasy
Copy link
Author

golang/go#44877

@ldez ldez added the dependencies Relates to an upstream dependency label Mar 9, 2021
@iamtakingiteasy
Copy link
Author

iamtakingiteasy commented Mar 10, 2021

As per golang/go#44877 (comment) it seems that "pointer bytes" reports from fieldalignment linter correspond to GC scanning the structure rather than structure size. Latter has messages in form of "struct of size X could be Y", and former are "struct with X pointer bytes could be Y", which was pretty confusing. As suggested in linked comment, maybe it is worth ignoring reports of pointer bytes by default in golangci-lint, maybe add an option to handle this.

@iamtakingiteasy
Copy link
Author

iamtakingiteasy commented Mar 10, 2021

The issue seems to be rooted in that maligned used in golangci-lint's go.mod is of pretty old version (as of 2018), while GC pointer scan diagnostic was added there 6 months ago. Naturally, this diagnostic was also introduced to fieldalignment too, but it is a noticable change of behavior when trying to use fieldalignment as drop-in replacement to old maligned version which had no such diagnostic in golangci-lint case.

@ldez ldez added question Further information is requested and removed bug Something isn't working dependencies Relates to an upstream dependency labels Mar 23, 2021
@ldez
Copy link
Member

ldez commented Mar 23, 2021

Thank you @iamtakingiteasy for the details behind the fieldalignment behavior.

We can close the issue now.

@ldez ldez closed this as completed Mar 23, 2021
@atc0005
Copy link

atc0005 commented Apr 13, 2021

@ldez: Thank you @iamtakingiteasy for the details behind the fieldalignment behavior.

We can close the issue now.

Apologies if this was spelled out, but if users enable the govet fieldalignment linter to replace the maligned linter (for struct size linter warnings), how can they avoid the reports regarding pointer bytes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants