Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/cover: missing whole switch body in coverage #32200

Closed
rsc opened this issue May 23, 2019 · 3 comments
Closed

cmd/cover: missing whole switch body in coverage #32200

rsc opened this issue May 23, 2019 · 3 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@rsc
Copy link
Contributor

rsc commented May 23, 2019

cd go/src/regexp
go test -run=TestMatch -coverprofile=x.prof
go tool cover -coverhtml=x.prof

Navigate to backtrack.go and scroll down.
In the tryBacktrack function, the switch inst.Op line is itself green but the entire switch body is gray, meaning untracked. It seems as if the coverage tool did not find and instrument the body at all. This may be a systemic problem so we should try to understand it for Go 1.13, and fix it if the fix is straightforward and low risk.

/cc @robpike in case you are interested

Screen Shot 2019-05-23 at 7 25 20 AM

@rsc rsc added this to the Go1.13 milestone May 23, 2019
@rsc rsc added the NeedsFix The path to resolution is known, but the work has not been done. label May 23, 2019
@robpike
Copy link
Contributor

robpike commented May 23, 2019

@rsc It doesn't do this in go1.4's version, which manipulated the AST. I suspect the bug was added during the conversion to source code manipulation.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/179581 mentions this issue: cmd/cover: minimal repro for issue #32200

@adg
Copy link
Contributor

adg commented May 31, 2019

As a diversion, I took a quick look at this, and managed to reproduce this in cmd/cover's tests.

func testSwitch() {
        for i := 0; i < 5; func() { i++; check(LINE, 5) }() {
                goto label2
        label1:
                goto label1
        label2:
                switch i {
                case 0:
                        check(LINE, 1)
                case 1:
                        check(LINE, 1)
                case 2:
                        check(LINE, 1)
                default:
                        check(LINE, 2)
                }
        }
}

Adding this peculiar set of lines before the switch—a goto, a label, another goto, and another label—apparently causes ast.Walk to not even visit the switch's block. Will investigate further.

edit: I originally had an unnecessary if in there, now removed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants