Skip to content

Drop if-return from default ruleset#843

Merged
mgechev merged 1 commit intomgechev:masterfrom
rliebz:undefault-if-return
Jun 26, 2023
Merged

Drop if-return from default ruleset#843
mgechev merged 1 commit intomgechev:masterfrom
rliebz:undefault-if-return

Conversation

@rliebz
Copy link
Copy Markdown
Contributor

@rliebz rliebz commented 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 #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:

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:

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

@rliebz
Drop if-return from default ruleset
b27780b 
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
mgechev approved these changes Jun 26, 2023
View reviewed changes
Copy link
Copy Markdown
Owner

@mgechev mgechev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

@mgechev mgechev merged commit 2b4286e into mgechev:master Jun 26, 2023
@rliebz rliebz deleted the undefault-if-return branch June 26, 2023 16:47
@pgimalac pgimalac mentioned this pull request Oct 20, 2023
Merged
10 tasks
raghavendra-talur added a commit to raghavendra-talur/ramen that referenced this pull request Jan 9, 2025
@raghavendra-talur
golangci-lint: remove if-return from the linters
dcbb587 
if-return has been removed from the default list of revive for a valid
reason. See the argument in mgechev/revive#843.

Comparing two snippets

1.
```
if err := v.volSyncHandler.DeleteRS(name, namespace); err != nil {
	return err
}

if err := v.volSyncHandler.DeleteRD(name, namespace); err != nil {
	return err
}

if err := v.volSyncHandler.DeleteSnapshots(namespace); err != nil {
	return err
}

return nil
```

2.
```
if err := v.volSyncHandler.DeleteRS(name, namespace); err != nil {
	return err
}

if err := v.volSyncHandler.DeleteRD(name, namespace); err != nil {
	return err
}

return v.volSyncHandler.DeleteSnapshots(namespace)
```

1 is a lot easier to read compared to 2 but if-return throws an error
for it.

Signed-off-by: Raghavendra Talur <raghavendra.talur@gmail.com>
raghavendra-talur added a commit to raghavendra-talur/ramen that referenced this pull request Jan 9, 2025
@raghavendra-talur
golangci-lint: remove if-return from the linters
fc2b4f1 
if-return has been removed from the default list of revive for a valid
reason. See the argument in mgechev/revive#843.

Comparing two snippets

1.
```
if err := v.volSyncHandler.DeleteRS(name, namespace); err != nil {
	return err
}

if err := v.volSyncHandler.DeleteRD(name, namespace); err != nil {
	return err
}

if err := v.volSyncHandler.DeleteSnapshots(namespace); err != nil {
	return err
}

return nil
```

2.
```
if err := v.volSyncHandler.DeleteRS(name, namespace); err != nil {
	return err
}

if err := v.volSyncHandler.DeleteRD(name, namespace); err != nil {
	return err
}

return v.volSyncHandler.DeleteSnapshots(namespace)
```

1 is a lot easier to read compared to 2 but if-return throws an error
for it.

Signed-off-by: Raghavendra Talur <raghavendra.talur@gmail.com>
raghavendra-talur added a commit to raghavendra-talur/ramen that referenced this pull request Jan 10, 2025
@raghavendra-talur
golangci-lint: remove if-return from the linters
99b6747 
if-return has been removed from the default list of revive for a valid
reason. See the argument in mgechev/revive#843.

Comparing two snippets

1.
```
if err := v.volSyncHandler.DeleteRS(name, namespace); err != nil {
	return err
}

if err := v.volSyncHandler.DeleteRD(name, namespace); err != nil {
	return err
}

if err := v.volSyncHandler.DeleteSnapshots(namespace); err != nil {
	return err
}

return nil
```

2.
```
if err := v.volSyncHandler.DeleteRS(name, namespace); err != nil {
	return err
}

if err := v.volSyncHandler.DeleteRD(name, namespace); err != nil {
	return err
}

return v.volSyncHandler.DeleteSnapshots(namespace)
```

1 is a lot easier to read compared to 2 but if-return throws an error
for it.

Signed-off-by: Raghavendra Talur <raghavendra.talur@gmail.com>
raghavendra-talur added a commit to RamenDR/ramen that referenced this pull request Jan 13, 2025
@raghavendra-talur
golangci-lint: remove if-return from the linters
5a49bea 
if-return has been removed from the default list of revive for a valid
reason. See the argument in mgechev/revive#843.

Comparing two snippets

1.
```
if err := v.volSyncHandler.DeleteRS(name, namespace); err != nil {
	return err
}

if err := v.volSyncHandler.DeleteRD(name, namespace); err != nil {
	return err
}

if err := v.volSyncHandler.DeleteSnapshots(namespace); err != nil {
	return err
}

return nil
```

2.
```
if err := v.volSyncHandler.DeleteRS(name, namespace); err != nil {
	return err
}

if err := v.volSyncHandler.DeleteRD(name, namespace); err != nil {
	return err
}

return v.volSyncHandler.DeleteSnapshots(namespace)
```

1 is a lot easier to read compared to 2 but if-return throws an error
for it.

Signed-off-by: Raghavendra Talur <raghavendra.talur@gmail.com>
raghavendra-talur added a commit to raghavendra-talur/ramen that referenced this pull request Jan 13, 2025
@raghavendra-talur
golangci-lint: remove if-return from the linters
bd34794 
if-return has been removed from the default list of revive for a valid
reason. See the argument in mgechev/revive#843.

Comparing two snippets

1.
```
if err := v.volSyncHandler.DeleteRS(name, namespace); err != nil {
	return err
}

if err := v.volSyncHandler.DeleteRD(name, namespace); err != nil {
	return err
}

if err := v.volSyncHandler.DeleteSnapshots(namespace); err != nil {
	return err
}

return nil
```

2.
```
if err := v.volSyncHandler.DeleteRS(name, namespace); err != nil {
	return err
}

if err := v.volSyncHandler.DeleteRD(name, namespace); err != nil {
	return err
}

return v.volSyncHandler.DeleteSnapshots(namespace)
```

1 is a lot easier to read compared to 2 but if-return throws an error
for it.

Signed-off-by: Raghavendra Talur <raghavendra.talur@gmail.com>
(cherry picked from commit 5a49bea)
asn1809 pushed a commit to asn1809/ramen that referenced this pull request Jan 22, 2025
@raghavendra-talur @asn1809
golangci-lint: remove if-return from the linters
c285a16 
if-return has been removed from the default list of revive for a valid
reason. See the argument in mgechev/revive#843.

Comparing two snippets

1.
```
if err := v.volSyncHandler.DeleteRS(name, namespace); err != nil {
	return err
}

if err := v.volSyncHandler.DeleteRD(name, namespace); err != nil {
	return err
}

if err := v.volSyncHandler.DeleteSnapshots(namespace); err != nil {
	return err
}

return nil
```

2.
```
if err := v.volSyncHandler.DeleteRS(name, namespace); err != nil {
	return err
}

if err := v.volSyncHandler.DeleteRD(name, namespace); err != nil {
	return err
}

return v.volSyncHandler.DeleteSnapshots(namespace)
```

1 is a lot easier to read compared to 2 but if-return throws an error
for it.

Signed-off-by: Raghavendra Talur <raghavendra.talur@gmail.com>
@albertony albertony mentioned this pull request Aug 19, 2025
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mgechev mgechev mgechev approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@rliebz @mgechev