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/vet: do not report variable capture for loop variables with the new lifetime rules #63888

Closed
timothy-king opened this issue Nov 1, 2023 · 6 comments
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) NeedsFix The path to resolution is known, but the work has not been done. release-blocker Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@timothy-king
Copy link
Contributor

This issue tracks support in x/tools/go/analysis/passes/loopclosure for the new loop variable lifetime rules of https://go.dev/issue/60078, available now behind GOEXPERIMENT=loopvar and likely on for language versions >= 1.22.

Marking as a release blocker as this will make cmd/vet would report a large number of false positives due to the new language feature.

@timothy-king timothy-king added release-blocker Analysis Issues related to static analysis (vet, x/tools/go/analysis) labels Nov 1, 2023
@timothy-king timothy-king added this to the Go1.22 milestone Nov 1, 2023
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Nov 1, 2023
@timothy-king
Copy link
Contributor Author

Example of a false positive https://go.dev/play/p/WsTuyNr6YnK

package main

import (
	"fmt"
	"sync"
)

func main() {
	const N = 4
	var wg sync.WaitGroup
	for i := 0; i < N; i++ {
		wg.Add(1)
		go func() { defer wg.Done(); fmt.Println(i) }()
	}
	wg.Wait()
}

Playground output for Go 1.21:

./prog.go:13:44: loop variable i captured by func literal

Go vet failed.

4
4
4
4

Program exited.

Playground output for Go dev branch:

# [play]
./prog.go:13:44: loop variable i captured by func literal

Go vet failed.

3
2
0
1

Program exited.

The dev currently branch implements the https://go.dev/issue/60078 loop lifetimes. So go vet should not continue to report on this example after 1.22.

@timothy-king
Copy link
Contributor Author

Note this will need to support packages with a mixture of lifetimes in different files or files without //go: version declared in the files (like the playground example). See #62605 for a potential solution via go/types.

@gopherbot
Copy link

Change https://go.dev/cl/539016 mentions this issue: go/analysis/passes/loopclosure: disable checker after go1.22.

@timothy-king timothy-king changed the title x/tools/go/analysis/passes/loopclosure: do not report variable capture for loop variables with the new lifetime rules cmd/vet: do not report variable capture for loop variables with the new lifetime rules Nov 2, 2023
@cherrymui cherrymui added the NeedsFix The path to resolution is known, but the work has not been done. label Nov 2, 2023
gopherbot pushed a commit to golang/tools that referenced this issue Nov 15, 2023
Uses the (*types.Info).FileVersion to disable the loopclosure checker
when in an *ast.File that uses GoVersion >= 1.22.

Updates golang/go#62605
Updates golang/go#63888

Change-Id: I2ebe974bc2ee2323eafb0f02d455ab76b3b9268d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/539016
Run-TryBot: Tim King <taking@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@timothy-king
Copy link
Contributor Author

Fixed by https://go.dev/cl/c/go/+/543975

@alexaandru
Copy link

I still see this issue with 1.22.0:

$ go version    
go version go1.22.0 linux/amd64

$ grep go go.mod              
go 1.22.0

$ cat main.go
package main

func main() {
	for i := range 10 {
		go func() { println(i) }()
	}
}

$ go vet main.go
./main.go:5:23: loop variable i captured by func literal

@timothy-king
Copy link
Contributor Author

@alexaandru Thank you for the report. I opened #65612. Looks like a plumbing issue when running vet on files listed on the command line. You can either invoke vet differently, suppress the finding (add _ = i at the end of the loop) or follow along on the new bug for a fix. If you have any more questions please ask them on #65612.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) NeedsFix The path to resolution is known, but the work has not been done. release-blocker Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

4 participants