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

Go vet misses closure using variable from outer loop #32876

Closed
dmowcomber opened this issue Jul 1, 2019 · 1 comment
Closed

Go vet misses closure using variable from outer loop #32876

dmowcomber opened this issue Jul 1, 2019 · 1 comment

Comments

@dmowcomber
Copy link

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

$ go version
go version go1.12.5 darwin/amd64

Does this issue reproduce with the latest release?

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

go env Output
$ go env |egrep 'GOOS|GOARCH'
GOARCH="amd64"
GOOS="darwin"

What did you do?

Normally go vet return an error if you have a loop with a goroutine function call with closure:
./prog.go:17:23: loop variable i captured by func literal
https://play.golang.org/p/PAg99GZMa7__8

But if you have nested loops with a closure that captures a variable from the outerloop and you have a goroutine inside an inner loop go vet will not return an error.
https://play.golang.org/p/zDsGfz0iESI

What did you expect to see?

./prog.go:16:22: loop variable i captured by func literal
Go vet exited.

10 10 10 10 10 10 10 10 10 10 

go vet returned an error: loop variable i captured by func literal
https://play.golang.org/p/PAg99GZMa7__8

What did you see instead?

10 10 10 10 10 10 10 10 10 10 

same results but no go vet error
https://play.golang.org/p/zDsGfz0iESI

@bcmills
Copy link
Contributor

bcmills commented Jul 1, 2019

Duplicate of #16520

@bcmills bcmills marked this as a duplicate of #16520 Jul 1, 2019
@bcmills bcmills closed this as completed Jul 1, 2019
@golang golang locked and limited conversation to collaborators Jun 30, 2020
gopherbot pushed a commit to golang/tools that referenced this issue Nov 21, 2022
…statements like if, switch, and for

In golang/go#16520, there was a suggestion to extend the
current loopclosure check to check more statements.

The current loopclosure flags patterns like:

	for k, v := range seq {
		go/defer func() {
			... k, v ...
		}()
	}

For this CL, the motivating example from golang/go#16520 is:

	var wg sync.WaitGroup
	for i := 0; i < 10; i++ {
		for j := 0; j < 1; j++ {
			wg.Add(1)
			go func() {
				fmt.Printf("%d ", i)
				wg.Done()
			}()
		}
	}
	wg.Wait()

The current loopclosure check does not flag this because of the
inner for loop, and the checker looks only at the last statement
in the outer loop body.

The suggestion is we redefine "last" recursively. For example,
if the last statement is an if, then we examine the last statements
in both of its branches. Or if the last statement is a nested loop,
then we examine the last statement of that loop's body, and so on.

A few years ago, Alan Donovan sent a sketch in CL 184537.

This CL attempts to complete Alan's sketch, as well as integrates
with the ensuing changes from golang/go#55972 to check
errgroup.Group.Go, which with this CL can now be recursively "last".

Updates golang/go#16520
Updates golang/go#55972
Fixes golang/go#30649
Fixes golang/go#32876

Change-Id: If66c6707025c20f32a2a781f6d11c4901f15742a
GitHub-Last-Rev: 04980e0
GitHub-Pull-Request: #415
Reviewed-on: https://go-review.googlesource.com/c/tools/+/452155
Reviewed-by: Tim King <taking@google.com>
Run-TryBot: Tim King <taking@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants