x/tools/go/analysis/analysistest: output and golden file differ, but RunWithSuggestedFixes does not report failure

[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/analysistestissue/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-build2078020356=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

See a minimal example at https://github.com/nishanths/analysistest-fmt-issue, along with an explanation in the README.

I originally discovered the issue doing the following:

• Wrote an analysis.Analyzer that (inadvertently) produces broken suggested fixes (produces invalid Go code).
• Ran a test using analysistest.RunWithSuggestedFixes.
• The output—after suggested fixes were applied—did not match the golden file. But the test reported PASS.

What did you expect to see?

I would have expected analysistest.RunWithSuggestedFixes to report a failure.

What did you see instead?

The test passed.

[nishanths]

Currently in two locations in RunWithSuggestedFixes() in analysistest.go, when format.Source() returns a non-nil error the code quietly continues on to the next file. So, though the output and the golden file don't match in this scenario, a failure isn't reported.

	formatted, err := format.Source([]byte(out))
	if err != nil {
		continue
	}

Calling t.Errorf when format.Source() fails should fix the issue. A formatting error means that the Analyzer is producing broken code, which should be reported as a failure. (And I can't think of a reason it shouldn't be reported as a test failure.) Edit: see below for better possible fixes.

I can submit a change if there's agreement on the fix.

[seankhliao]

cc @matloob

[timothy-king]

Skipping on err != nil goes back to when RunWithSuggestedFixes() was introduced: https://go-review.googlesource.com/c/tools/+/224959 . @matloob or @stamblerre reviewed this and may recall the history of why this behavior was chosen.

An alternative fix is to return an error whenever want != string(formatted). I don't have a compelling reason to prefer this vs returning the error. Maybe we are okay with producing a suggestion that is not valid Go code?

[nishanths]

Thank you for looking into this issue.

An alternative fix is to return an error whenever ...

To clarify, by "return an error", did you mean "call t.Errorf"?

Maybe we are okay with producing a suggestion that is not valid Go code?

I think it's worth keeping this option open for the future. In this case, I don't think we should fail the test if format.Source fails, as I had somewhat precipitously suggested in my earlier comment.

Instead we should call format.Source, and either ignore the error or t.Logf() the error. But definitely not continue upon error as the code does currently. If we do this, the subsequent want != string(formatted) check and its associated t.Errorf() would naturally report the mismatch between the program's output and the golden file, I think.

[timothy-king]

To clarify, by "return an error", did you mean "call t.Errorf"?

Not quite what I had in mind. But your suggestion makes more sense in context.

Here is roughly my understanding of this:

formatted, err := format.Source([]byte(out))
if err != nil {
	t.Errorf("failed to format source code: %s", err)
	continue
}

Instead we should call format.Source, and either ignore the error or t.Logf() the error. But definitely not continue upon error as the code does currently.

Here is roughly my understanding of this:

formatted, err := format.Source([]byte(out))
if err != nil {
	t.Logf("failed to format source code: %s", err)
	formatted = out
}

I think both make sense. I am really interested if we know why we chose to skip reporting an error historically.

[matloob]

Looking at the CL it seems like not reporting an error was an oversight. I can't think of a good reason to just ignore that case

[nishanths]

https://golang.org/cl/351552/ has fixed this issue by calling t.Errorf if formatting fails.