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.

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.

@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.

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

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

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?

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.

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?

Yes, and yes

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?

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.

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

For info, I believe I've hit this same issue with the following code:

https://github.com/twpayne/go-heap/blob/fb16bd2df012f1689aa9c0682b0c611078c10908/heap.go#L39-L40

revive incorrectly reports that the block can be removed:

heap.go:39:69: empty-block: this block is empty, you can remove it (revive)
                for value, ok := h.Pop(); ok && yield(value); value, ok = h.Pop() {
                }

This is with revive run by golangci-lint 1.61.0, i.e. revive v1.3.9.

Hi @twpayne thanks for reporting the issue.
Effectively, the rule fails to detect the side-effect of the loop predicate. Doing that kind of analysis in a complete way is beyond the capabilities of revive (and of any static analysis tool in the general case)
As an immediate "fix" I can just propose to deactivate the rule at that line.
I'm studying to modify the rule to just skip analyzing loops to avoid false positives.

Thank you very much for the analysis and explanation. I've indeed added a //nolint:revive comment to suppress the warning for that particular line.

I agree that any complete static analysis is infeasible. Maybe a couple of heuristics like "does any part of the for clause, condition, or range clause contain a function call or an assignment to a variable outside the loop" might help reduce the false positives of this rule. Just a thought, I don't know how easy this would be to implement or how well it work in practice.

Thank you very much for the analysis and explanation. I've indeed added a //nolint:revive comment to suppress the warning for that particular line.

I agree that any complete static analysis is infeasible. Maybe a couple of heuristics like "does any part of the for clause, condition, or range clause contain a function call or an assignment to a variable outside the loop" might help reduce the false positives of this rule. Just a thought, I don't know how easy this would be to implement or how well it work in practice.

Or ++/--, or a channel op.

@twpayne the rule already have a very primitive version of such an heuristic but as you mention (and @feldgendler comment confirms) the cases are "infinite".
IMO the true positives this rule is spotting are very very few vs the false positives, thus I think we should just give up and do not introduce complexity to catch a negligible number of true loops with empty blocks.

IMO the true positives this rule is spotting are very very few vs the false positives, thus I think we should just give up and do not introduce complexity to catch a negligible number of true loops with empty blocks.

Ack, makes sense. Is it worth closing this issue as "not planned" or "won't fix"?

If I had a say, I would err for the side of false negatives rather than false positives. In this context, it means that an empty loop body is not reported as “empty code block” — either never reported at all, or only reported if the loop statement itself is definitely side-effect-free (that is, if in doubt, don't report).