Skip to content
New issue

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

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

Already on GitHub? # 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) FrozenDueToAge 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
Contributor

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.

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) FrozenDueToAge 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