Skip to content

Commit

Permalink
Drop if-return from default ruleset
Browse files Browse the repository at this point in the history
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 mgechev#537. More recently, it was
reintroduced into the default ruleset as a result of mgechev#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.
  • Loading branch information
rliebz committed Jun 23, 2023
1 parent e5d5d09 commit 1b75c62
Show file tree
Hide file tree
Showing 4 changed files with 4 additions and 6 deletions.
1 change: 0 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,6 @@ warningCode = 0
[rule.error-strings]
[rule.error-naming]
[rule.exported]
[rule.if-return]
[rule.increment-decrement]
[rule.var-naming]
[rule.var-declaration]
Expand Down
2 changes: 1 addition & 1 deletion config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ var defaultRules = []lint.Rule{
&rule.TimeNamingRule{},
&rule.ContextKeysType{},
&rule.ContextAsArgumentRule{},
&rule.IfReturnRule{},
&rule.EmptyBlockRule{},
&rule.SuperfluousElseRule{},
&rule.UnusedParamRule{},
Expand Down Expand Up @@ -87,6 +86,7 @@ var allRules = append([]lint.Rule{
&rule.UseAnyRule{},
&rule.DataRaceRule{},
&rule.CommentSpacingsRule{},
&rule.IfReturnRule{},
}, defaultRules...)

var allFormatters = []lint.Formatter{
Expand Down
1 change: 0 additions & 1 deletion defaults.toml
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ warningCode = 0
[rule.error-strings]
[rule.error-naming]
[rule.exported]
[rule.if-return]
[rule.increment-decrement]
[rule.var-naming]
[rule.var-declaration]
Expand Down
6 changes: 3 additions & 3 deletions revivelib/core_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"github.com/mgechev/revive/config"
"github.com/mgechev/revive/lint"
"github.com/mgechev/revive/revivelib"
"github.com/mgechev/revive/rule"
)

func TestReviveLint(t *testing.T) {
Expand Down Expand Up @@ -45,7 +46,6 @@ func TestReviveFormat(t *testing.T) {

// ACT
failures, exitCode, err := revive.Format("stylish", failuresChan)

// ASSERT
if err != nil {
t.Fatal(err)
Expand All @@ -70,8 +70,7 @@ func TestReviveFormat(t *testing.T) {
}
}

type mockRule struct {
}
type mockRule struct{}

func (r *mockRule) Name() string {
return "mock-rule"
Expand All @@ -93,6 +92,7 @@ func getMockRevive(t *testing.T) *revivelib.Revive {
conf,
true,
2048,
revivelib.NewExtraRule(&rule.IfReturnRule{}, lint.RuleConfig{}),
revivelib.NewExtraRule(&mockRule{}, lint.RuleConfig{}),
)
if err != nil {
Expand Down

0 comments on commit 1b75c62

Please sign in to comment.