Go-tools: S1000: don't suggest `for range` when case statement has side effects

[rayjlinden]

S1000: should use for range instead of for { select {} }

I get this lint error on this code:

for {
    select {
    case <-time.After(scavenger.Config.TimeSinceTransferExpired):
        scavenger.ScavengeInProgressTransfers()
    }
}

How does range replace a for(ever) loop? This seems like a false positive to me. Or the description needs to be better at least? How is gosimple suggesting this should be fixed?

[dominikh]

Theoretically this is a false positive (because the case statement contains a function call), yes. Practically, this only affects the output message of the check. If it weren't for the for loop around it, we'd suggest the following instead: should use a simple channel send/receive instead of select with a single case

I.e., the concrete alternative would be

for {
  <-time.After(scavenger.Config.TimeSinceTransferExpired)
  scavenger.ScavengeInProgressTransfers()
}

I will repurpose this issue to document the fact we need to improve what message S1000 emits.

[cameronelliott]

I just ran into this, the cool thing is I learned something from the warning,
but I totally agree the language could be more clear.

I really like this idea from @dominikh: should use a simple channel send/receive instead of select with a single case

[ruudk]

I was bitten by this too. I was told that my for select time.after loop was wrong and I accidentally refactored it to for range time.After(time.Duration(refreshInterval) * time.Second) { only to find out months later that this only loops once 🤦 My bad of course, I should have tested it first, but maybe the error can be improved 🙏

[bo-er]

I just ran into this, the cool thing is I learned something from the warning,
but I totally agree the language could be more clear.

I really like this idea from @dominikh: should use a simple channel send/receive instead of select with a single case

Thank you camerone! I love you! Just saved so much time!