staticcheck: detect access to modified closure

[judwhite]

Example bad code:

for _, pr := range prs {
	go func(prNumber int) {
		addPRLabel(gh, prNumber, pr.Base.Ref) // oops
		wg.Done()
	}(pr.Number)
}

[dominikh]

Not a priority because go vet already catches this.

[johanbrandhorst]

I think this issue should be extended to any use of loop variables inside closures, where the closure isn't executed before the next iteration. That would be quite complicated but apparently possible.

See https://golang.org/doc/faq#closures_and_goroutines and https://github.com/golang/go/wiki/CommonMistakes#using-goroutines-on-loop-iterator-variables for discussion on this problem from official sources, though specifically referring to the use of goroutines, which is probably the most common.

As anecdotal evidence I can also say that using Ginkgo, which relies on closures, this issue crops up quite a bit, unrelated to goroutines. A linter that could detect any incorrect loop variables in closures would be very valuable.

[bradleyfalzon]

@johanbrandhorst the issue that wiki page points out, as mentioned by @dominikh, is picked up in go vet:

package main

import "fmt"

func main() {
        values := []int{1, 2, 3}
        for val := range values {
                go func() {
                        fmt.Println(val) // main.go:9: range variable val captured by func literal
                }()
        }
}

Or were you referring to something different?

[johanbrandhorst]

This isn't unique to goroutines, and vet doesn't cover the following problematic code:

package main

import "fmt"

func main() {
	values := []int{1, 2, 3}
	var funcs []func()
	for _, val := range values {
		funcs = append(funcs, func() {
			fmt.Println(val)
		})
	}
	for _, f :=  range funcs {
		f()
	}
}

See https://play.golang.org/p/QQgUiKv4xE

Output:

3
3
3