golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
122.7k stars 17.5k forks source link

cmd/go: test -coverprofile should ask compiler to drop (incorrect) position information #67938

Open rsc opened 2 months ago

rsc commented 2 months ago
% cat x.go
package p

func f(int) string

func g() {
    b := f(1)
    collect := func(min, max, stop int) []int {
        return nil
    }
    b := f(2)
    _ = collect
}
% go test -coverprofile=c.out x.go
# command-line-arguments
./x.go:6:2: `b' declared and not used
./x.go:10:35: no new variables on left side of :=

There is no column 35 on the line b := f(2), making the error message very confusing. The problem is that cover has inserted text into the lines. When the compiler did not print column information, that was invisible. Now it's not.

To make it invisible again, the compiler should add a flag to drop position information, and then the go command should use that flag when compiling with test coverage.

gabyhelp commented 2 months ago

Similar Issues

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

rsc commented 2 months ago

The similar issues are perfect: #6329 is a related instance of this bug (line number instead of column number), and #56475 was an incorrectly-filed and incorrectly-fixed issue, the fix for which is the source of the current instance of this bug.

rsc commented 2 months ago

Note that the compiler does not need a flag to drop position information. Cmd/cover just needs to stop claiming accurate position information in the first place, rolling back the change in CL 446259 (with a good comment added explaining why the original code is correct).

/cc @thanm

rsc commented 2 months ago

It occurs to me that cmd/compile may now support /*line x.go:1:2*/ comments (it did not originally), and so a different fix would be to keep the CL 446259 change and arrange to emit a /*line*/ comment after each text insertion to correct the column information.

rsc commented 2 months ago

Ouch, here is a worse error:

% cat x.go
package p

func g() {
|

func h() {
}
% go test x.go
# command-line-arguments
./x.go:4:1: syntax error: unexpected |, expected }
./x.go:6:1: syntax error: unexpected func after top level declaration
FAIL    command-line-arguments [build failed]
FAIL
% go test -cover x.go
# command-line-arguments
# [/Users/rsc/go/pkg/tool/darwin_arm64/cover -pkgcfg $WORK/b001/pkgcfg.txt -mode set -var goCover_14e7eac9fa7b_ -outfilelist $WORK/b001/coveroutfiles.txt /private/tmp/x.go]
2024/06/12 07:36:17 cover: /private/tmp/x.go: /private/tmp/x.go:4:1: expected statement, found '|' (and 1 more errors)
FAIL    command-line-arguments [build failed]
FAIL
% 

Maybe go test -cover should compile the packages normally first and only ever invoke cmd/cover on well-formed programs?

Either that or cover needs to emit errors in the standard compiler form (without date time cover: file:) and the go command needs to expect cover to be able to fail (not echoing the command line above the failure).

rsc commented 2 months ago

I am running into these because I am running go test -coverprofile=c.out && uncover c.out as my testing loop. But I think VSCode Go users would run into these too, since VSCode Go uses coverage in tests by default. Of course, VSCode Go also warns about such mistakes in the IDE so maybe users tend not to invoke coverage on malformed programs. How civilized of them.