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.

Sign up for GitHub

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

Jump to bottom

cmd/vet: panic in new parallel subtest check when t.Run has single argument #57908

Closed
thepudds opened this issue Jan 18, 2023 · 5 comments
Closed

cmd/vet: panic in new parallel subtest check when t.Run has single argument #57908

thepudds opened this issue Jan 18, 2023 · 5 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.20

Comments

@thepudds
Copy link
Contributor

thepudds commented Jan 18, 2023 •
edited

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

$ go version

go version devel go1.20-9088c69 Tue Jan 17 18:21:06 2023 +0000 linux/amd64

Does this issue reproduce with the latest release?

Yes, tip. Not in Go 1.19 given the parallel subtest check is new in Go 1.20.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env

What did you do?

Ran many public modules through vet as part of testing #57173 and the related https://go.dev/cl/455195.

What did you see?

The new Go 1.20 parallel subtest check in the loopclosure pass can panic in the rare case of t.Run with a single argument, such as this simplified example:

    fn := func() (string, func(t *testing.T)) { return "", nil }
    t.Run(fn())

A real-world example:

https://github.com/go-spatial/geom/blob/3cd2f5a9a082dd4f827c9f9b69ba5d736d2dcb12/planar/planar_test.go#L118

I saw this in a public module, then backed up to observe that this particular panic is in the 1.20 release candidate / tip.

git clone https://github.com/go-spatial/geom
cd geom/planar
go vet .

Error:

# github.com/go-spatial/geom/planar
panic: runtime error: index out of range [1] with length 1

Stack:

goroutine 54 [running]:                                                                              [64/269]
cmd/vendor/golang.org/x/tools/go/analysis/passes/loopclosure.parallelSubtest(0xc0001dd810, 0xc0001e3840)     
        /home/thepudds1460/sdk/gotip/src/cmd/vendor/golang.org/x/tools/go/analysis/passes/loopclosure/loopclo
sure.go:306 +0x55b                                                                                           
cmd/vendor/golang.org/x/tools/go/analysis/passes/loopclosure.run.func1({0x767eb0?, 0xc00006df80?})
        /home/thepudds1460/sdk/gotip/src/cmd/vendor/golang.org/x/tools/go/analysis/passes/loopclosure/loopclo
sure.go:164 +0x55d                                                                                           
cmd/vendor/golang.org/x/tools/go/ast/inspector.(*Inspector).Preorder(0xc00021a0c0, {0xc00038fcb8?, 0x2?, 0xc0
0004e400?}, 0xc00038fca8)                                                                                    
        /home/thepudds1460/sdk/gotip/src/cmd/vendor/golang.org/x/tools/go/ast/inspector/inspector.go:77 +0x8b
cmd/vendor/golang.org/x/tools/go/analysis/passes/loopclosure.run(0xc00069e0f0)                               
        /home/thepudds1460/sdk/gotip/src/cmd/vendor/golang.org/x/tools/go/analysis/passes/loopclosure/loopclo
sure.go:92 +0xa5
cmd/vendor/golang.org/x/tools/go/analysis/unitchecker.run.func5.1()
        /home/thepudds1460/sdk/gotip/src/cmd/vendor/golang.org/x/tools/go/analysis/unitchecker/unitchecker.go
:355 +0x904
sync.(*Once).doSlow(0x6b83c0?, 0xc00041b440?)
        /home/thepudds1460/sdk/gotip/src/sync/once.go:74 +0xc2
sync.(*Once).Do(...)
        /home/thepudds1460/sdk/gotip/src/sync/once.go:65
cmd/vendor/golang.org/x/tools/go/analysis/unitchecker.run.func5(0x8ef7e0)
        /home/thepudds1460/sdk/gotip/src/cmd/vendor/golang.org/x/tools/go/analysis/unitchecker/unitchecker.go
:306 +0x1a5
cmd/vendor/golang.org/x/tools/go/analysis/unitchecker.run.func6.1(0x0?)
        /home/thepudds1460/sdk/gotip/src/cmd/vendor/golang.org/x/tools/go/analysis/unitchecker/unitchecker.go
:367 +0x29
created by cmd/vendor/golang.org/x/tools/go/analysis/unitchecker.run.func6
        /home/thepudds1460/sdk/gotip/src/cmd/vendor/golang.org/x/tools/go/analysis/unitchecker/unitchecker.go
:366 +0x47
@thepudds
Copy link
Contributor Author

I sent https://go.dev/cl/462282, which attempts to fix this.

@thepudds thepudds mentioned this issue Jan 18, 2023
Closed
@gopherbot
Copy link

Change https://go.dev/cl/462282 mentions this issue: go/analysis/passes/loopclosure: avoid panic in new parallel subtest check when t.Run has single argument

@bcmills bcmills added this to the Go1.20 milestone Jan 18, 2023
@bcmills bcmills added the NeedsFix The path to resolution is known, but the work has not been done. label Jan 18, 2023
@gopherbot
Copy link

Change https://go.dev/cl/462596 mentions this issue: go/analysis/passes/loopclosure: avoid panic in new parallel subtest check when t.Run has single argument

@ianlancetaylor
Copy link
Contributor

@gopherbot Please open backport to 1.20.

Seems like this affects the 1.20 release.

@gopherbot gopherbot mentioned this issue Jan 18, 2023
Closed
@gopherbot
Copy link

Backport issue(s) opened: #57911 (for 1.20).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

gopherbot pushed a commit to golang/tools that referenced this issue Jan 18, 2023
@thepudds @rsc
[internal-branch.go1.20-vendor] go/analysis/passes/loopclosure: avoid…
070db29 
… panic in new parallel subtest check when t.Run has single argument

The new Go 1.20 parallel subtest check in the loopclosure pass
can panic in the rare case of t.Run with a single argument:

    fn := func() (string, func(t *testing.T)) { return "", nil }
    t.Run(fn())

A real-world example:

https://github.com/go-spatial/geom/blob/3cd2f5a9a082dd4f827c9f9b69ba5d736d2dcb12/planar/planar_test.go#L118

This has been present for multiple months without seeming to
be reported, so presumably this is a rare pattern.

Avoid that panic by checking t.Run has an expected number
of arguments.

Fixes golang/go#57908.
Fixes golang/go#57911.

Change-Id: I5cde60edf624e16bb9f534e435bcd55e63a15647
GitHub-Last-Rev: 24e65a4
GitHub-Pull-Request: #424
Reviewed-on: https://go-review.googlesource.com/c/tools/+/462282
Run-TryBot: thepudds <thepudds1460@gmail.com>
Reviewed-by: Russ Cox <rsc@golang.org>
Auto-Submit: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-on: https://go-review.googlesource.com/c/tools/+/462596
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Tim King <taking@google.com>
TryBot-Bypass: Russ Cox <rsc@golang.org>
@golang golang locked and limited conversation to collaborators Jan 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.20
4 participants
@ianlancetaylor @bcmills @gopherbot @thepudds