x/tools/go/analysis/passes/fieldalignment/cmd/fieldalignment: ignore generated files

[ainar-g]

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

$ go version
go version go1.20.4 linux/amd64
$ go version -m $( which fieldalignment )
…/bin/fieldalignment: go1.20.4
        path    golang.org/x/tools/go/analysis/passes/fieldalignment/cmd/fieldalignment
        mod     golang.org/x/tools      v0.8.1-0.20230421161920-b9619ee54b47    h1:fQlOhMJ24apqitZX8S4hbCbHU1Z9AvyWkN3BYI55Le4=
        dep     golang.org/x/mod        v0.10.0 h1:lFO9qtOdlre5W1jxS3r/4szv2/6iXxScdzjoBMXNhYk=
        dep     golang.org/x/sys        v0.7.0  h1:3jlCCIQZPdOYu1h8BkNvLz8Kgwtae2cagcG/VamtZRU=
        build   -buildmode=exe
        build   -compiler=gc
        build   CGO_ENABLED=1
        build   CGO_CFLAGS=
        build   CGO_CPPFLAGS=
        build   CGO_CXXFLAGS=
        build   CGO_LDFLAGS=
        build   GOARCH=amd64
        build   GOOS=linux
        build   GOAMD64=v1

Does this issue reproduce with the latest release?

Yes.

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

go env Output

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/ainar/.cache/go-build"
GOENV="/home/ainar/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/ainar/go/pkg/mod"
GONOPROXY="REMOVED"
GONOSUMDB="REMOVED"
GOOS="linux"
GOPATH="/home/ainar/go"
GOPRIVATE="REMOVED"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/ainar/go/go1.20"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/ainar/go/go1.20/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.20.4"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="REMOVED"
GOWORK="REMOVED"
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 -fno-caret-diagnostics -Qunused-arguments -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build3548065178=/tmp/go-build -gno-record-gcc-switches"

What did you do?

fieldalignment /path/to/pkg/

(The package containing generated code in generated.pb.go.)

What did you expect to see?

Nothing, exit code 0.

What did you see instead?

/path/to/pkg/generated.pb.go:25:16: struct of size 104 could be 96
/path/to/pkg/generated.pb.go:96:14: struct with 168 pointer bytes could be 144
[…]

Exit code 3.

[mknyszek]

CC @timothy-king @matloob @zpavlinovic via https://dev.golang.org/owners

[timothy-king]

My intuition is that a lot of users would have the opposite desire from cmd/fieldalignment, i.e. 'I definitely expect to reports from fieldalignment in generated files'. They would want to be warned on alignment issues on files they may never have looked at. So I think reporting in generated files is probably the right default here, but we can consider mechanisms for optionally suppressing some reports.

I am slightly unsure about the technical feasibility. IIUC whether a file is generated is not a part of the x/tools/go/packages output. Is there an existing Go wide convention that is encoded in the files to let us know whether a file is considered "generated"?

We could get this from the user, but at the point that is roughly equivalent to just having a flag for which files to exclude from reports. FWIW it is relatively straightforward to postprocess the -json output to exclude files. So it is unclear if we x/tools/go/analysis really is the thing to be updated.

[ainar-g]

A lot of people use code generators they have little control over, protoc-gen-go being the most obvious example. So yes, having an option to either ignore all generated files or a per-file ignore option would be nice to have.

Is there an existing Go wide convention that is encoded in the files to let us know whether a file is considered "generated"?

I'm not sure if I understand the question correctly, but the convention is documented in go help generate:

To convey to humans and machine tools that code is generated,
generated source should have a line that matches the following
regular expression (in Go syntax):

        ^// Code generated .* DO NOT EDIT\.$
This line must appear before the first non-comment, non-blank
text in the file.

This is to be added in Go 1.21 with go/ast.IsGenerated, and there is an analyzer available in Staticcheck's repo: https://github.com/dominikh/go-tools/blob/bc668a17ee05a79bb9fdc320018de7503f0d9646/analysis/facts/generated/generated.go.

[timothy-king]

This is to be added in Go 1.21 with go/ast.IsGenerated,

TIL! Great to know there is a standard. (This function would need to be copied for x/tools to use this until after the release of 1.23. Nothing here does not look copiable though.) Given that there is a standard, a bool flag for the analyzer to disable reports for generated files seems fairly reasonable.

I think there is a question of whether such a flag should be on a checker or if it should be generalized to the driver level, e.g. unitchecker, singlechecker or gopls. @adonovan @findleyr thoughts?

[adonovan]

TIL! Great to know there is a standard. (This function would need to be copied for x/tools

In fact, this function evolved from the version in x/tools (see gopls/internal/lsp/source/util.go) though the go/ast version is much better (in large part due to @dmitshur's careful review).

whether such a flag should be on a checker or if it should be generalized to the driver level

Applying edits is certainly a driver responsibility. Our drivers should not apply edits to generated files, or, for that matter, to read-only files such as those in the module cache, nor apply edits twice due to file-system level aliasing such as symbolic and hard links.

[adonovan]

I think there are several issues here.

1. Fixing generated files. It never makes sense for a fix to be applied to a generated file. Instead, the generator should be manually changed, or the diagnostic and its fix should be ignored. Many of our analyzers (e.g. all the modernizers) explicitly skip generated files. We should probably change the behavior of -fix flag to always skip generated files. I've split this questioh off as x/tools/go/analysis: don't apply SuggestedFixes to generated files #75948.
2. Diagnostics in generated files. Some diagnostics make sense in generated files (e.g. reports of serious problems), though the solution is a change to the generator. Other diagnostics (e.g. stylistic issues such as redundant arithmetic) should be suppressed. This is x/tools/go/analysis: a Diagnostic should indicate when it is a lint-style warning that should be ignored in generated code #41436.
3. fieldalignment diagnostics are neither bugs, nor style issues, and the fixes it suggests to rearrange fields are not universally safe to apply. (For this reason, it was removed from the gopls analyzer suite and replaced by a feature in Hover; see x/tools/gopls: hover: show fraction of wasted space in structs #66582.)

Since fieldalignment is not appropriate for the cmd/vet suite, the cmd/fix suite, nor the gopls suite, I don't think we should spend further effort on it specifically. I will therefore close this issue.

[ainar-g]

@adonovan, thanks for the analysis. Would some kind of proposal about a generic mechanism for ignoring files (e.g. by name regexp) make more sense? As in, a single mechanism for nilness, fieldalignment, shadow, etc.

[adonovan]

Would some kind of proposal about a generic mechanism for ignoring files (e.g. by name regexp) make more sense? As in, a single mechanism for nilness, fieldalignment, shadow, etc.

There's already a proposal. See:

• x/tools/gopls: provide a way to suppress analysis diagnostics for certain files #50764

[ainar-g]

I see, thank you.