Detect loop vars unnecessarily passed as parameters to goroutines?

[telemachus]

(First, thanks for the tool.) This may be working as intended, but copyloopvar does not seem to detect loop vars that are unnecessarily passed as parameters to goroutines.

func main() {
    done := make(chan bool)

    values := []string{"a", "b", "c"}
    for _, v := range values {
        go func(v string) {
            fmt.Println(v)
            done <- true
        }(v)
    }

    for range values {
        <-done
    }
}

If I run copyloopvar on this code (using golangci or using go vet as you suggest on the README), I get no warnings even though it is no longer necessary to pass v as a parameter to the goroutine.

If this is working as intended, would you consider extending copyloopvar to detect such unnecessary parameters? (Or a PR to detect that?) Or do you already have reason to think that would be too complicated or out of scope for the tool?

[ldemailly]

I wouldn’t call it unnecessary but rather clearer, avoiding capture and using explicit parameters is better so shouldn’t be lint flagged imo

[karamaru-alpha]

@telemachus Thank you for raising such a great issue! As you pointed out, it’s no longer necessary to explicitly pass the loop variable as an argument to the goroutine function to address the "shared memory issue". However, I believe it's an overreach for the linter to flag this pattern.

As @ldemailly mentioned, sometimes this approach is intentionally used to make the function signature clearer or to differentiate between the loop variable and its copy passed as the goroutine function argument. In these cases, the warning would be unnecessary.

[telemachus]

@karamaru-alpha Thanks for the quick response. I see the point that @ldemailly and you make about clarity, and I completely get it. I also see why you might not want to flag this pattern by default.

However, as you showed with your link, my example comes from the official Go blog announcing the new loop var behavior (https://go.dev/blog/loopvar-preview). That suggests to me that they believe that people will (perhaps even should?) skip the repetition. With that in mind, would you consider adding an optional warning about this pattern behind a flag? I understand if you would rather not. Thanks for considering it.

[telemachus]

avoiding capture and using explicit parameters is better

@ldemailly not picking a fight, but I'd like to understand this all as well as possible. Are there reasons other than clarity that you think it is better to explicitly pass the values as parameters? Thanks.

[ldemailly]

another reason is have the function not capturing

eg think about this way: if body was a function:

go run foo(x)

then you wouldn’t have a choice to pass it

to me capture should only be if needed/no other convenient way

also I think it should be cheaper but maybe the compiler makes it same/no cost

ps: I should have added this is just my opinion.

[telemachus]

Thanks for the reply. I normally think of capturing as a (positive) feature of a language (the ability to create proper closures) not something to avoid. Still, thanks for giving me more to think about.

[ldemailly]

if you need to or there is value in creating closure it's indeed a great feature/capability, I think though it's more useful for bigger state than a loop variable; yet... sure you can too (as you found in the blog post, though I think that was to explain the issue not something to "do" necessarily)