The `if-return` rule was originally a golint rule which was removed
from their ruleset for being out of scope. Similarly, it was dropped
from revive intentionally as a result of #537. More recently, it was
reintroduced into the default ruleset as a result of #799 due to a
discrepancy in documentation without a discussion of whether this rule
in particular belonged as a part of that default rule set.
While it is no longer a goal of this project to align 100% with the
golint defaults, I believe that this rule gives bad advice often enough
that it should not be enabled by default.
For example, consider the following code:
```go
if err := func1(); err != nil {
return err
}
if err := func2(); err != nil {
return err
}
if err := func3(); err != nil {
return err
}
return nil
```
The `if-return` rule considers this a violation of style, and instead
suggests the following:
```go
if err := func1(); err != nil {
return err
}
if err := func2(); err != nil {
return err
}
return func3()
```
While this is more terse, it has a few shortcomings compared to the
original. In particular, it means extending the size of the diff if
changing the order of checks, adding logic after the call that currently
happens to be last, or choosing to wrap the error. And in that last
case, it can make it less obvious that there is an unwrapped error being
propagated up the call stack.
This in practice has a very similar effect to disabling trailing commas;
while it is not necessarily wrong as a style choice, I don't believe it
warrants a position as part of the default ruleset here.
See-also: https://github.com/golang/lint/issues/363
* refactor: extract shared code for linting if-else chains
The rules "early-return", "indent-error-flow" and
"superfluous-else" have a similar structure. This
moves the common logic for classifying if-else chains
to a common package.
A few side benefits:
- "early-return" now handles os.Exit/log.Panicf/etc
- "superfluous-else" now handles (builtin) panic
- "superfluous-else" and "indent-error-flow" now handle if/else
chains with 2+ "if" branches
* internal/ifelse: style fixes, renames, spelling
This fixes a false positive reported by revive on the following:
select {}
This is a way to block the program indefinitely.
It is used in cases like:
- Running a long-running server in a background thread
and blocking `main` from exiting until the application dies.
This is something you might do if your process has
multiple servers/listeners in the same process.
```go
go grpcListenAndServe()
go httpListenAndServe()
go logFlusher()
select {}
```
- In programs compiled to WASM to prevent the application from exiting,
so that the Javascript components may interact with it.
```go
func main() {
js.Global().Set("foo", foo)
select {}
}
```
Without this, one may see an error like,
"Error: Go program has already exited"
As a workaround, these programs can block forever
by receiving from a throwaway channel (`<-make(chan struct{})`),
but `select {}` is still a completely valid way of doing this,
so supporting it makes sense.
The issue was previously reported in #698 but was closed
because the author was satisfied with a `//nolint` comment.
Now that this rule is part of the default rule set (#799)
the case for addressing the false positive is stronger.
Resolves#804
