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: -rangeloops check only checks last statement in range loop body #21412

Closed
ianlancetaylor opened this issue Aug 11, 2017 · 7 comments
Closed
Milestone

Comments

@ianlancetaylor
Copy link
Contributor

Running go vet on this file:

package main

import (
	"fmt"
)

func P(i int) {
	fmt.Println(i)
}

func F(s []int) {
	for i := range s {
		go func() {
			F(i)
		}()
	}
}

func main() {
	F(nil)
}

prints

foo.go:14: range variable i captured by func literal

If I change F to this:

func F(s []int) {
	for i := range s {
		go func() {
			F(i)
		}()
		_ = 1
	}
}

then go vet prints nothing.

Looking at the vet source code, checkRangeLoop only looks at the last statement of a range loop to see whether a variable is capture. This behavior dates back to when the rangeloop check was first added in https://golang.org/cl/6494075. As far as I can tell, it ought to look at every statement in the loop.

@ianlancetaylor ianlancetaylor added this to the Go1.10 milestone Aug 11, 2017
@dominikh
Copy link
Member

Related #8732. Generally, vet can only reliably prove an issue for the last statement in the loop. Using captured loop variables in earlier statements isn't provably incorrect. The closure could be executed before the end of the loop iteration. For example:

for i := range s {
  fn := func() { println(i) }
  useCallback(fn) // useCallback returns only after fn has been called
}

Similarly, spawned goroutines can be waited on.

@ianlancetaylor
Copy link
Contributor Author

Good point, but we can do better for explicit go and defer statements.

@dominikh
Copy link
Member

dominikh commented Aug 14, 2017

Can you elaborate on which additional cases we could catch with regard to go? Keep in mind that goroutines that close over loop variables aren't categorically wrong. For example:

for i := range s {
	wg := &sync.WaitGroup{}
	wg.Add(2)
	go func() {
		work1(i)
		wg.Done()
	}()
	go func() {
		work2(i)
		wg.Done()
	}()
	wg.Wait()
}

@ianlancetaylor
Copy link
Contributor Author

I filed this for https://groups.google.com/d/msg/golang-nuts/M9Hq76csSaU/nVrpNOXyBwAJ which has examples of problems that we could catch.

@spenczar
Copy link
Contributor

Thanks for filing this, Ian.

Here's the real-world code that caused problems at my company. Once the root cause was traced to this bug, engineers responsible for this code were surprised (and slightly alarmed) that vet didn't warn them first.

I've redacted or altered a few of the object names, but the idea should be clear. The code handles a batch of requests, then gathers errors for later.

func (r *redactedStruct) handleBatch(ctx context.Context, batch *redactedpkg.Batch) error {
	var chans []chan error
	for _, request := range batch.Requests {
		c := make(chan error)
		go func(c chan error) {
			c <- r.handle(ctx, request)
		}(c)
		chans = append(chans, c)
	}

	var err error
	for _, c := range chans {
		err = combineErrors(err, <-c)
	}
	return err
}

@ianlancetaylor
Copy link
Contributor Author

Thanks for the example. So the question is whether vet can be smart enough to see that chans = append(chans, c) will not wait for a goroutine to complete.

@rsc rsc modified the milestones: Go1.10, Go1.11 Nov 22, 2017
@rsc
Copy link
Contributor

rsc commented Nov 22, 2017

Duplicate of #16520 I believe.

@rsc rsc closed this as completed Nov 22, 2017
@golang golang locked and limited conversation to collaborators Nov 22, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants