Why does vet's 'rangeloop' check only check the last statement of a loop?

[spe...@justin.tv]

I've noticed that cmd/vet's rangeloop check, which warns you about binding to range loop variables in function literals which are launched with 'go' or 'defer', is more limited than I thought:

https://github.com/golang/go/blob/d5ad7793d610bddfb3e7e09b8dafa0b0837f0cb2/src/cmd/vet/rangeloop.go#L6-L9

This file contains the code to check range loop variables bound inside function literals that are deferred or launched in new goroutines. We only check instances where the defer or go statement is the last statement in the loop body, as otherwise we would need whole program analysis.

I don't understand why whole program analysis is necessary here. Why can't cmd/vet check each function literal declared within the loop's braces?

[Steven Blenkinsop]

If you have a loop like:

for i := range slice {

    f(&i)

}

There's no way to know whether f launches a goroutine that captures the loop variable without also evaluating the body of f and potentially every function directly or indirectly called by f. If i  escapes into a global variable, you also have to evaluate the entire program to see if a goroutine is ever launched which could access i by way of that variable. This is what they mean by whole program analysis.

However, I feel like only catching a go or defer statement which is the last statement in the loop body is very far from a best effort. I also wonder if the analysis could take advantage of the existing escape analysis in the compiler to flag likely misuses of loop variables. There are certainly valid use cases for loop variables escaping (such as when this coincides with returning from the function or otherwise breaking the loop), so I'm not sure how easy it would be to discriminate, even if it's feasible to begin with.

--
You received this message because you are subscribed to the Google Groups "golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[spe...@justin.tv]

Yes, it makes sense that it would be impossible to really check at that level. What surprised me was that this does not trigger vet:

for i := range slice {

    go f(i)

    _ = 1

}

Yet this does trigger vet:

for i := range slice {

    _ = 1

    go f(i)

}

Is there something special about the last statement that makes it easier to check?

For what it's worth, this exact bug caused real user-impacting issues at my company. The team responsible for the code was very surprised that vet did not alert them to the issue before it made it into production.

[Ian Lance Taylor]

On Thu, Aug 10, 2017 at 1:43 PM, <spe...@justin.tv> wrote:
>
> Yes, it makes sense that it would be impossible to really check at that
> level. What surprised me was that this does not trigger vet:
>
> for i := range slice {
> go f(i)
> _ = 1
> }
>
> Yet this does trigger vet:
>
> for i := range slice {
> _ = 1
> go f(i)
> }
>
> Is there something special about the last statement that makes it easier to
> check?
>
> For what it's worth, this exact bug caused real user-impacting issues at my
> company. The team responsible for the code was very surprised that vet did
> not alert them to the issue before it made it into production.

I agree that this appears to be a bug. I filed https://golang.org/issue/21412.

Ian

[Ian Lance Taylor]

If you read the issue, you will see that it is clearly more subtle
than I initially thought. If you have examples that vet could
reliably catch that it does not catch today, that would be helpful.
What did the code look like that had a uncaught case that surprised
people? Thanks.

Ian

[Spencer Nelson]

Yes, very tricky stuff. I've replied on the issue - it seems best to keep discussion in one place.

Thank you very much for opening the issue!