Incorrectly identifies channel draining as "empty code block"

[jpopadak]

Describe the bug
revive incorrectly identities channel draining as an empty code block and suggests to remove it.

To Reproduce
Steps to reproduce the behavior:

1. I updated revive go get -u github.com/mgechev/revive
2. I run it with the following flags & configuration file:

cd $PROJECT_DIR
revive -config revive.toml .

ignoreGeneratedHeader = false
severity = "error"
confidence = 0.8
errorCode = 1
warningCode = 0

[rule.add-constant]
    arguments = [{maxLitCount = "3",allowStrs ="\"\"",allowInts="0,1,2"}]
[rule.argument-limit]
    arguments = [6]
[rule.atomic]
[rule.blank-imports]
[rule.bool-literal-in-expr]
[rule.call-to-gc]
[rule.constant-logical-expr]
[rule.context-as-argument]
[rule.context-keys-type]
[rule.cyclomatic]
    arguments = [10]
[rule.deep-exit]
[rule.dot-imports]
[rule.duplicated-imports]
[rule.empty-block]
[rule.error-naming]
[rule.error-return]
[rule.error-strings]
[rule.errorf]
[rule.exported]
[rule.flag-parameter]
[rule.function-result-limit]
    arguments = [4]
[rule.get-return]
[rule.if-return]
[rule.import-shadowing]
[rule.increment-decrement]
[rule.indent-error-flow]
[rule.line-length-limit]
    arguments = [120]
[rule.package-comments]
[rule.range]
[rule.range-val-in-closure]
[rule.receiver-naming]
[rule.redefines-builtin-id]
[rule.superfluous-else]
[rule.time-naming]
[rule.unexported-return]
[rule.unhandled-error]
[rule.unreachable-code]
[rule.unused-parameter]
[rule.unused-receiver]
[rule.var-declaration]
[rule.var-naming]
[rule.waitgroup-by-value]

A really bad example, but in our scenario, we have many routines reading from processChan where we send another message to have them stop processing. We reuse the channel, but we want to ensure the channel was empty from previous processing.

package main

import "sync"

const (
	chanSize = 10
	maxNums = 100
	modNum = 5
)

func main() {
	wg := sync.WaitGroup{}
	processChan := make(chan int, chanSize)

	wg.Add(1)
	go func() {
		defer wg.Done()
		for i := 0; i < maxNums; i++ {
			processChan <- i
		}
	}()

	wg.Add(1)
	go func() {
		defer wg.Done()

		for i := 0; i < maxNums; i++ {
			if !isValidNum(i) {
				break
			}
		}
	}()

	for range processChan {
	}
}

func isValidNum(i int) bool {
	return i%modNum == 0
}

3. Revive returns an error

myflie.go:34:24: this block is empty, you can remove it

Expected behavior
No error to occur as this is channel draining.

Logs
If applicable, add screenshots to help explain your problem.

Desktop (please complete the following information):

• OS: macOS 10.14.6
• Version of Go: v1.14.1

Additional context
We have two workarounds:

1. We can ignore the few lines across our project where we are iterating over a channel, however, it would be nicer if revive knew that this is channel draining.
2. We can delete and recreate the channel, but since the channel is passed around on startup / Dependency Injection, we cannot just "create a new one and reinitialize all services" easily.

[mgechev]

Looks like we should type check to verify that we're dealing with a channel. This would slow down the rule though, but seems like the right way to go.

[chavacava]

For the sake of rule simplicity, I think these corner cases should be handled by disabling the linter in the concerned lines.
Anyway, I've pushed PR #415 with a fix based in type information.

[mgechev]

@jpopadak, because of some regressions (both stability and performance), we reverted the fix. It's still possible to recognize this pattern without type checking. This is something we'd investigate in the future.

[powerman]

Would be nice to have this fixed. I've a project with dozen of such false positives and have to disable the rule. :(

[chavacava]

branch https://github.com/mgechev/revive/tree/fix-386 implements a fix consisting in trying to identify channel draining loops by their syntax (a range without value nor key) and avoids raising a failure for such cases

branch https://github.com/mgechev/revive/tree/alleviates-386 also identifies channel draining loops by their syntax but in place of not raising a failure it raises one with a lower (0.5) confidence.

Neither implementation uses type information

[feldgendler]

The fix for this bug was reverted soon after being introduced. Are the reasons in #416 still valid? That was in 2020, and some things might have changed since (for instance, Go modules are now the norm).

I can see that many other revive checks use type information. Can't this one?

[chavacava]

Hi @feldgendler, thanks for commenting in this thread.
Rules requiring type information introduce some performance degradation thus we try to keep rules type free
At the time we decided to not evolve the rule because in the trade-off analysis precision vs performance degradation+complexity we considered the number of false positives not high enough to justify the use of type information to avoid them (and adding a disabling annotation in the rare cases was not too much effort)
It is something we can re-open to discussion. I'll try to rebase the branches I've pushed with the fix proposals long time ago.

[feldgendler]

Hi! Thanks for coming back to me @chavacava!

Do I understand correctly that if any revive rule that needs type information is enabled, that triggers type analysis, and once it's triggered, any such rules have negligible marginal cost?

Also, is it true that all revive rules that currently require type information are disabled by default?

[chavacava]

Yes, and yes

[feldgendler]

Would it be possible to add a fine-tuning option to the empty-block rule (whether to use type information) so that one could choose between accuracy and performance?

[chavacava]

I've rebased the branch #415 and now the rule does not warn on empty blocks on channel draining ranges.
@feldgendler you can try the fix and let me know if it is sufficiently precise for you.

[feldgendler]

@chavacava Yes, thank you! My code doesn't drigger empty-block with this fix.