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

Comments

Projects
None yet
5 participants
@ianlancetaylor
Copy link
Contributor

commented Aug 11, 2017

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

This comment has been minimized.

Copy link
Member

commented Aug 12, 2017

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2017

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

@dominikh

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2017

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

@spenczar

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2017

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Nov 22, 2017

Duplicate of #16520 I believe.

@rsc rsc closed this 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.
You can’t perform that action at this time.