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!

[prologic]

Also ran into this recently, in my case:

func (t *thing) runloop() {
   for {
      select {
      case obj, ok := <-t.queue:
         if !ok {
            break
         }
         do_something_with_obj(obj)
      }
   }

Don't think the warning here is valid either because I'm pretty sure you can't do a:

for obj, ok := range t.queue { ...}

I think that's syntactically incorrect?

[cameronelliott]

Maybe if you keep the 'for', but remove the 'select' and turn it into an 'if'. You wouldn't need the range operator.

[prologic]

Maybe if you keep the 'for', but remove the 'select' and turn it into an 'if'. You wouldn't need the range operator.

Good point, I think I had this structure with the intent of adding another case 😉

[BatmanAoD]

@dominikh Was the documentation updated without updating the actual tool output? I got this lint, googled "golang s1000 for range select" (because the warning came from my IDE and I didn't know it originated in staticcheck), and found this thread rather than the documentation you linked. I'm using 2023.1.3 (v0.4.3), which seems quite recent.

The documentation is helpful, but the tool still suggests should use for range instead of for { select {} }, which, as noted in several of the comments above, would imply turning an infinite loop into a non-infinite loop, probably erroneously.

[dominikh]

@BatmanAoD

The documentation is helpful, but the tool still suggests should use for range instead of for { select {} }, which, as noted in several of the comments above, would imply turning an infinite loop into a non-infinite loop, probably erroneously.

The two examples (ignoring side-effects changing what channel is being received from) only differ in behavior if the channel gets closed. And if it does get closed, the for { select {} } loop will spin, which should always be a bug.

The documentation hasn't been changed, it is merely incomplete. It only covers one of the two distinct diagnostics that S1000 can emit. S1000 recommends that

select {
case x := <-ch: ...
}

be turned into a simple <-ch

and it recommends that

for {
    select {
    case x := <-ch: ...
    }
}

be turned into for x := range ch instead, correctly under the assumption that ch will not be closed (else the original code was buggy to begin with), and erroneously under the assumption that the meaning of ch won't change between iterations in the original code. This issue tracks fixing the erroneous assumption.