x/tools/analysis/passes/tests: report examples with misplaced Output comments

[ameowlia]

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

go version go1.17 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/adowns/Library/Caches/go-build"
GOENV="/Users/adowns/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/adowns/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/adowns/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.17"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/adowns/workspace/text/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/4c/1b3s36l94blb6xg1_4p_13800000gn/T/go-build2465748359=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I ran this test in a test file:

func ExampleThisTestShouldNotPass() {
	fmt.Println("meow")

	// Output:
	// bark

	// TODO: foo bar
}

What did you expect to see?

The test fail

What did you see instead?

The test passed.

I saw this in a real example with the ExampleMatcher here, which I reported in #48361.

[smasher164]

Also reproduces on the playground: https://play.golang.org/p/0wPM6durES5.

[ALTree]

IMO this is working as intended, the testing package documentation says:

Example functions may include a concluding line comment that begins with "Output:"

In your example the Output line comment is not "concluding" (it's not the last thing in the function, and not the last comment block) so the Example is not processed as a "check-output" one.

[smasher164]

@ALTree In that case, the broken examples can probably just be addressed by moving the non-Output comments before the Output comment.

[ameowlia]

IMO this is working as intended

@ALTree: I agree, but it worries me that this allowed a broken example to be around for 4 years.

Interestingly, it only fails this way when there is another comment after the output. See here where it still fails when it is more code after the output: https://play.golang.org/p/RRN7vl8KayA

[smasher164]

@ameowlia I agree that this is behavior that is subtle and hard to catch. What if the testing tool's heuristic was changed to only process CommentGroups that begin with Output:? It would be able to ignore that TODO comment, and the output comment could be placed anywhere in the function, without affecting the test result.

[ameowlia]

What if the testing tool's heuristic was changed to only process CommentGroups that begin with Output:?

@smasher164 - I looked into this today (code here). I would like to try to contribute this if others think it is an acceptable solution.

The main argument against this fix that I can see is "what happens if an example has multiple output commentblocks"? In that case I would suggest returning the first one and ignoring later ones. Multiple outputs seems like flagrant disregard for the spec. Adding a comment at the end (the issue I reported here) seems like an accident that would be nice to protect against.

Alternatively we could add a go/vet warning to make sure that output commentblocks are always the last commentblock. This seems a little heavy handed though.

[zpavlinovic]

Regarding the vet approach, this would at least need to satisfy the requirement of high frequency. It is not clear at the moment how frequent this issue is.

Another approach is to perhaps make the documentation more clear.

[smasher164]

It is not clear at the moment how frequent this issue is.

Yeah, the bug in x/text/language's example might be a rare exception. Running an *analysis.Analyzer on a corpus of open source code might give a better idea of how frequently this pops up. Without that information, it seems heavy handed both to change how comments are processed and to add a vet check.

Updating the documentation seems like a good option. Right now, testing says

Example functions may include a concluding line comment that begins with "Output:" and is compared with the standard output of the function when the tests are run. (The comparison ignores leading and trailing space.)

[zpavlinovic]

@matloob

[ameowlia]

I wrote a parser, ran it against this repo, and found one example: #48494. I plan on running it against golang.org/x/... repos today.

❓ @smasher164 - Any advice on how to run against a "corpus of open source code"?

[gopherbot]

Change https://golang.org/cl/350995 mentions this issue: cmd/compile: fix ExampleInfo output

[smasher164]

❓ @smasher164 - Any advice on how to run against a "corpus of open source code"?

I typically run tools like that over my GOPATH, assuming I have a decent number of packages installed. For instance, this is what I did for the string(int) conversion vet check: #32479 (comment), #32479 (comment).

I guess nowadays with go modules, I'd run that over the vendor directory in some project that imports a large number of dependencies? I couldn't find an official way to list all major versions of modules in the cache, but this might work:

$ find $(go env GOMODCACHE) -type f -name go.mod |
while read fname; do echo "${fname#$(go env GOMODCACHE)/}" |
cut -f1 -d"@"; done |
sort -u

[jayconrod]

I think a vet check is the right place to fix this. See checkExample in golang.org/x/tools/go/analysis/passes/tests. It already checks that examples have the right name and signature. It could check that the // Output: comment is in the right place, too.

[zpavlinovic]

I think a vet check is the right place to fix this. See checkExample in golang.org/x/tools/go/analysis/passes/tests. It already checks that examples have the right name and signature. It could check that the // Output: comment is in the right place, too.

Good point. I guess we don't need to worry about the frequency of this pattern because we are just extending an existing checker with a simple logic?

[jayconrod]

Good point. I guess we don't need to worry about the frequency of this pattern because we are just extending an existing checker with a simple logic?

Right, exactly. Just reporting something that was already buggy.

[ameowlia]

I ended up running the parser against the top 1000 open source golang repos based on stars and I found 5 additional instances of the bug(after removing duplicates from vendoring).

Project Name	Links
golang/go	buggy test, github issue
golang/text	buggy test, github issue
miekg/dns	buggy test, github issue
m3db/m3	buggy test, github issue
cuelang/cue -> cue-lang/cue	buggy test, github issue
zalando/skipper	buggy test, github issue
smartcontractkit/chainlink	buggy test, github issue

@jayconrod - I'll start working on extending the checkExample vet check. Thank you for the point in the right direction.

[jayconrod]

@ameowlia Nice finds. I bet there are a lot more out there, too. Thanks for working on this.

[gopherbot]

Change https://golang.org/cl/351553 mentions this issue: go/analysis/passes/tests: check example output