if-return incorrectly treated as golint rule #537
Assignees
Labels
Comments
|
Hi @rliebz, thanks for filling the issue. I expect to work on it in the coming days. |
Merged
rliebz
added a commit
to rliebz/revive
that referenced
this issue
Jun 23, 2023
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.
rliebz
added a commit
to rliebz/revive
that referenced
this issue
Jun 23, 2023
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.
rliebz
added a commit
to rliebz/revive
that referenced
this issue
Jun 23, 2023
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. See-also: golang/lint#363
mgechev
pushed a commit
that referenced
this issue
Jun 26, 2023
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: golang/lint#363
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Describe the bug
The README describes
if-returnas agolintrule, and therefore includes it in the set of defaults here and ingolangci-lint. This is no longer true as of 2018.Because of this discrepancy, there is an increased barrier to adoption of
reviveovergolint.To Reproduce
Steps to reproduce the behavior:
go install github.com/mgechev/revive@latestconfig.toml:And got the following results:
With
golint:$ golint example.go # no outputExpected behavior
I would expect the output from
reviveto be the same asgolint, since this is what the README claims.Logs
N/A
Desktop (please complete the following information):
go version go1.16.5 darwin/amd64Additional context
The same problem exists with
golangci-lint:The text was updated successfully, but these errors were encountered: