x/tools/go/analysis/analysistest: RunWithSuggestedFixes should check against golden file even if the Go file has no suggested fixes

[nishanths]

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

$ go version
go version go1.16.5 darwin/amd64

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="/Users/nishanthshanmugham/Library/Caches/go-build"
GOENV="/Users/nishanthshanmugham/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/nishanthshanmugham/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/nishanthshanmugham/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.16.5"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/nishanthshanmugham/src/exhaustive/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/xr/2qd98lwx3l709x7jt26376qr0000gn/T/go-build2147913176=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

• Used analysistest.RunWithSuggestedFixes().
• In my testdata, had a Go file that did not have a corresponding golden file. (The issue also applies if the Go file has a golden file with non-matching output.)
• The test passed though no corresponding golden file exists for the Go file.

Minimal example repo, along with explanation in README: https://github.com/nishanths/analysistest-golden-issue

What did you expect to see?

I would have expected the test to fail, considering a golden file isn't present.

What did you see instead?

The test passes without error.

[nishanths]

In particular, the issue occurs because the testdata Go file in question has no suggested fixes. So RunWithSuggestedFixes skips the file during its assertions.

Ideally files that don't have suggested fixes are still compared against a golden file: some test authors may write tests to ensure that no fixes are suggested for a given file, and that the file remains unchanged after applying (the zero) suggested fixes.

[cherrymui]

cc @matloob @timothy-king @guodongli-google @zpavlinovic

[timothy-king]

Documentation:

// RunWithSuggestedFixes behaves like Run, but additionally verifies suggested fixes.
// It uses golden files placed alongside the source code under analysis:
// suggested fixes for code in example.go will be compared against example.go.golden.

I think there is a reasonable interpretation here that the .golden file is only needed for verifying fixes, and if there are no fixes, [there] is no need to look at a .golden file. This lets folks use this as the "super" operator if they want utilities that can test analyzers both with and without fixes.

I think an alternative reading of this as suggesting that it will always compare against a golden file is also kinda reasonable. This would mean tightening the contact of this function.

If we tighten the contract of RunWithSuggestedFixes(), we may want another function to act as the generalization of these two modes. If we don't, we might want to clarify the documentation of this function for what happens if there are no suggested edits/no golden file.

This change would break existing users that use RunWithSuggestedFixes() without specifying a golden file. For example, https://github.com/dominikh/go-tools/blob/master/simple/testdata/src/CheckSortHelpers/LintSortHelpers.go

some test authors may write tests to ensure that no fixes are suggested for a given file, and that the file remains unchanged after applying (the zero) suggested fixes.

I believe you can already write such tests by not specifying a golden file or giving a no-op golden file. But I believe the only way to distinguish between 0 edits and all edits combined are a no-op is by not giving a golden file. This is more implicit, but if there are any edits I believe an error is raised here. It is not clear we need to be able to distinguish the 0 edits and the net no-op cases.

[nishanths]

Thank you for looking into this, @timothy-king. I tend to agree with all the points you made in your comment.

I think there is a reasonable interpretation here that the .golden file is only needed for verifying fixes, and if there are no fixes is no need to look at a .golden file. This lets folks use this as the "super" operator if they want utilities that can test analyzers both with and without fixes.

I hadn't read the doc comment on RunWithSuggestedFixes closely earlier. I have now, and I also think the interpretation that you mention is a reasonable one. I also hadn't thought of the use of RunWithSuggestedFixes as the "super" operator, which makes a lot of sense too.

But I believe the only way to distinguish between 0 edits and all edits combined are a no-op is by not giving a golden file. This is more implicit, but if there are any edits I believe an error is raised here. It is not clear we need to be able to distinguish the 0 edits and the net no-op cases.

This is very well put. And I don't think we need to able to distinguish between these two cases as well.

So overall, I tend to agree that no changes necessarily need to be made to address this issue. Feel free to close the issue (or move it to a similar appropriate state).

[timothy-king]

So overall, I tend to agree that no changes necessarily need to be made to address this issue. Feel free to close the issue (or move it to a similar appropriate state).

Closing for now.

If someone wants to improve the documentation of RunWithSuggestedFixes to clarify what happens when there is no .golden file, you can send a CL, assign me as the reviewer, and I will happily take a look.