Skip to content

Commit

Permalink
"Detect opposite" feature (#42)
Browse files Browse the repository at this point in the history
* Detect opposite feature

* self-review fixes

* self-review fixes
  • Loading branch information
Antonboom authored Oct 6, 2024
1 parent 22435d2 commit 289392e
Show file tree
Hide file tree
Showing 13 changed files with 346 additions and 115 deletions.
141 changes: 88 additions & 53 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,34 @@ if err != nil {
// Use user.
```
### Opposite situation
Sometimes people consider the opposite situation (returning a non-nil error and a valid value at the same time) to be
an anti-pattern too, since it can lead to hard-to-find bugs.
For example, this **kubernetes** code
```go
func (fh *fakeHistory) UpdateControllerRevision(revision *apps.ControllerRevision, newRevision int64) (*apps.ControllerRevision, error) {
clone := revision.DeepCopy()
clone.Revision = newRevision
return clone, fh.indexer.Update(clone)
}
```
can be rewritten as follows
```go
func (fh *fakeHistory) UpdateControllerRevision(revision *apps.ControllerRevision, newRevision int64) (*apps.ControllerRevision, error) {
clone := revision.DeepCopy()
clone.Revision = newRevision
if err := fh.indexer.Update(clone); err != nil {
return nil, fmt.Errorf("update index: %w", err)
}
return clone, nil
}
```
### What if I think it's bullshit?
I understand that each case needs to be analyzed separately,
Expand All @@ -67,24 +95,31 @@ In any case, you can just not enable the linter.
### CLI
```shell
# See help for full list of types.
$ nilnil --checked-types ptr,func ./...
$ nilnil --checked-types chan,func,iface,map,ptr,uintptr,unsafeptr ./...
$ nilnil --detect-opposite ./...
$ nilnil --detect-opposite --checked-types ptr ./..
```
### golangci-lint
https://golangci-lint.run/usage/linters/#nilnil
```yaml
nilnil:
checked-types:
- ptr
- func
- iface
- map
- chan
- uintptr
- unsafeptr
linters-settings:
nilnil:
# In addition, detect opposite situation (simultaneous return of non-nil error and valid value).
# Default: false
detect-opposite: true
# List of return types to check.
# Default: ["chan", "func", "iface", "map", "ptr", "uintptr", "unsafeptr"]
checked-types:
- chan
- func
- iface
- map
- ptr
- uintptr
- unsafeptr
```
## Examples
Expand Down Expand Up @@ -250,48 +285,48 @@ func (r *RateLimiter) Allow() bool {
```shell
$ cd $GOROOT/src
$ nilnil ./... 2>&1 | grep -v "_test.go"
/usr/local/go/src/internal/bisect/bisect.go:196:3: return both the `nil` error and invalid value: use a sentinel error instead
/usr/local/go/src/net/fd_unix.go:71:3: return both the `nil` error and invalid value: use a sentinel error instead
/usr/local/go/src/net/fd_unix.go:79:4: return both the `nil` error and invalid value: use a sentinel error instead
/usr/local/go/src/net/fd_unix.go:156:4: return both the `nil` error and invalid value: use a sentinel error instead
/usr/local/go/src/net/iprawsock_posix.go:36:3: return both the `nil` error and invalid value: use a sentinel error instead
/usr/local/go/src/net/tcpsock_posix.go:38:3: return both the `nil` error and invalid value: use a sentinel error instead
/usr/local/go/src/net/udpsock_posix.go:37:3: return both the `nil` error and invalid value: use a sentinel error instead
/usr/local/go/src/net/unixsock_posix.go:92:3: return both the `nil` error and invalid value: use a sentinel error instead
/usr/local/go/src/crypto/tls/key_agreement.go:46:2: return both the `nil` error and invalid value: use a sentinel error instead
/usr/local/go/src/crypto/tls/ticket.go:355:3: return both the `nil` error and invalid value: use a sentinel error instead
/usr/local/go/src/crypto/tls/ticket.go:359:3: return both the `nil` error and invalid value: use a sentinel error instead
/usr/local/go/src/database/sql/driver/types.go:157:3: return both the `nil` error and invalid value: use a sentinel error instead
/usr/local/go/src/database/sql/driver/types.go:232:3: return both the `nil` error and invalid value: use a sentinel error instead
/usr/local/go/src/database/sql/driver/types.go:263:4: return both the `nil` error and invalid value: use a sentinel error instead
/usr/local/go/src/database/sql/convert.go:548:3: return both the `nil` error and invalid value: use a sentinel error instead
/usr/local/go/src/database/sql/sql.go:205:3: return both the `nil` error and invalid value: use a sentinel error instead
/usr/local/go/src/database/sql/sql.go:231:3: return both the `nil` error and invalid value: use a sentinel error instead
/usr/local/go/src/database/sql/sql.go:257:3: return both the `nil` error and invalid value: use a sentinel error instead
/usr/local/go/src/database/sql/sql.go:284:3: return both the `nil` error and invalid value: use a sentinel error instead
/usr/local/go/src/database/sql/sql.go:311:3: return both the `nil` error and invalid value: use a sentinel error instead
/usr/local/go/src/database/sql/sql.go:337:3: return both the `nil` error and invalid value: use a sentinel error instead
/usr/local/go/src/database/sql/sql.go:363:3: return both the `nil` error and invalid value: use a sentinel error instead
/usr/local/go/src/database/sql/sql.go:389:3: return both the `nil` error and invalid value: use a sentinel error instead
/usr/local/go/src/database/sql/sql.go:422:3: return both the `nil` error and invalid value: use a sentinel error instead
/usr/local/go/src/debug/dwarf/entry.go:884:3: return both the `nil` error and invalid value: use a sentinel error instead
/usr/local/go/src/debug/dwarf/line.go:146:3: return both the `nil` error and invalid value: use a sentinel error instead
/usr/local/go/src/debug/dwarf/line.go:153:3: return both the `nil` error and invalid value: use a sentinel error instead
/usr/local/go/src/debug/dwarf/typeunit.go:138:3: return both the `nil` error and invalid value: use a sentinel error instead
/usr/local/go/src/debug/pe/file.go:470:3: return both the `nil` error and invalid value: use a sentinel error instead
/usr/local/go/src/net/http/h2_bundle.go:9530:3: return both the `nil` error and invalid value: use a sentinel error instead
/usr/local/go/src/net/http/transfer.go:765:3: return both the `nil` error and invalid value: use a sentinel error instead
/usr/local/go/src/net/http/transfer.go:775:3: return both the `nil` error and invalid value: use a sentinel error instead
/usr/local/go/src/net/http/transfer.go:798:3: return both the `nil` error and invalid value: use a sentinel error instead
/usr/local/go/src/go/build/build.go:1442:3: return both the `nil` error and invalid value: use a sentinel error instead
/usr/local/go/src/go/build/build.go:1453:3: return both the `nil` error and invalid value: use a sentinel error instead
/usr/local/go/src/go/build/build.go:1457:3: return both the `nil` error and invalid value: use a sentinel error instead
/usr/local/go/src/go/build/build.go:1491:3: return both the `nil` error and invalid value: use a sentinel error instead
/usr/local/go/src/go/internal/gccgoimporter/ar.go:125:3: return both the `nil` error and invalid value: use a sentinel error instead
/usr/local/go/src/image/jpeg/reader.go:622:5: return both the `nil` error and invalid value: use a sentinel error instead
/usr/local/go/src/image/png/reader.go:434:4: return both the `nil` error and invalid value: use a sentinel error instead
/usr/local/go/src/internal/profile/legacy_profile.go:1089:4: return both the `nil` error and invalid value: use a sentinel error instead
/usr/local/go/src/net/internal/socktest/switch.go:142:3: return both the `nil` error and invalid value: use a sentinel error instead
/usr/local/go/src/internal/bisect/bisect.go:196:3: return both a `nil` error and an invalid value: use a sentinel error instead
/usr/local/go/src/net/fd_unix.go:71:3: return both a `nil` error and an invalid value: use a sentinel error instead
/usr/local/go/src/net/fd_unix.go:79:4: return both a `nil` error and an invalid value: use a sentinel error instead
/usr/local/go/src/net/fd_unix.go:156:4: return both a `nil` error and an invalid value: use a sentinel error instead
/usr/local/go/src/net/iprawsock_posix.go:36:3: return both a `nil` error and an invalid value: use a sentinel error instead
/usr/local/go/src/net/tcpsock_posix.go:38:3: return both a `nil` error and an invalid value: use a sentinel error instead
/usr/local/go/src/net/udpsock_posix.go:37:3: return both a `nil` error and an invalid value: use a sentinel error instead
/usr/local/go/src/net/unixsock_posix.go:92:3: return both a `nil` error and an invalid value: use a sentinel error instead
/usr/local/go/src/crypto/tls/key_agreement.go:46:2: return both a `nil` error and an invalid value: use a sentinel error instead
/usr/local/go/src/crypto/tls/ticket.go:355:3: return both a `nil` error and an invalid value: use a sentinel error instead
/usr/local/go/src/crypto/tls/ticket.go:359:3: return both a `nil` error and an invalid value: use a sentinel error instead
/usr/local/go/src/database/sql/driver/types.go:157:3: return both a `nil` error and an invalid value: use a sentinel error instead
/usr/local/go/src/database/sql/driver/types.go:232:3: return both a `nil` error and an invalid value: use a sentinel error instead
/usr/local/go/src/database/sql/driver/types.go:263:4: return both a `nil` error and an invalid value: use a sentinel error instead
/usr/local/go/src/database/sql/convert.go:548:3: return both a `nil` error and an invalid value: use a sentinel error instead
/usr/local/go/src/database/sql/sql.go:205:3: return both a `nil` error and an invalid value: use a sentinel error instead
/usr/local/go/src/database/sql/sql.go:231:3: return both a `nil` error and an invalid value: use a sentinel error instead
/usr/local/go/src/database/sql/sql.go:257:3: return both a `nil` error and an invalid value: use a sentinel error instead
/usr/local/go/src/database/sql/sql.go:284:3: return both a `nil` error and an invalid value: use a sentinel error instead
/usr/local/go/src/database/sql/sql.go:311:3: return both a `nil` error and an invalid value: use a sentinel error instead
/usr/local/go/src/database/sql/sql.go:337:3: return both a `nil` error and an invalid value: use a sentinel error instead
/usr/local/go/src/database/sql/sql.go:363:3: return both a `nil` error and an invalid value: use a sentinel error instead
/usr/local/go/src/database/sql/sql.go:389:3: return both a `nil` error and an invalid value: use a sentinel error instead
/usr/local/go/src/database/sql/sql.go:422:3: return both a `nil` error and an invalid value: use a sentinel error instead
/usr/local/go/src/debug/dwarf/entry.go:884:3: return both a `nil` error and an invalid value: use a sentinel error instead
/usr/local/go/src/debug/dwarf/line.go:146:3: return both a `nil` error and an invalid value: use a sentinel error instead
/usr/local/go/src/debug/dwarf/line.go:153:3: return both a `nil` error and an invalid value: use a sentinel error instead
/usr/local/go/src/debug/dwarf/typeunit.go:138:3: return both a `nil` error and an invalid value: use a sentinel error instead
/usr/local/go/src/debug/pe/file.go:470:3: return both a `nil` error and an invalid value: use a sentinel error instead
/usr/local/go/src/net/http/h2_bundle.go:9530:3: return both a `nil` error and an invalid value: use a sentinel error instead
/usr/local/go/src/net/http/transfer.go:765:3: return both a `nil` error and an invalid value: use a sentinel error instead
/usr/local/go/src/net/http/transfer.go:775:3: return both a `nil` error and an invalid value: use a sentinel error instead
/usr/local/go/src/net/http/transfer.go:798:3: return both a `nil` error and an invalid value: use a sentinel error instead
/usr/local/go/src/go/build/build.go:1442:3: return both a `nil` error and an invalid value: use a sentinel error instead
/usr/local/go/src/go/build/build.go:1453:3: return both a `nil` error and an invalid value: use a sentinel error instead
/usr/local/go/src/go/build/build.go:1457:3: return both a `nil` error and an invalid value: use a sentinel error instead
/usr/local/go/src/go/build/build.go:1491:3: return both a `nil` error and an invalid value: use a sentinel error instead
/usr/local/go/src/go/internal/gccgoimporter/ar.go:125:3: return both a `nil` error and an invalid value: use a sentinel error instead
/usr/local/go/src/image/jpeg/reader.go:622:5: return both a `nil` error and an invalid value: use a sentinel error instead
/usr/local/go/src/image/png/reader.go:434:4: return both a `nil` error and an invalid value: use a sentinel error instead
/usr/local/go/src/internal/profile/legacy_profile.go:1089:4: return both a `nil` error and an invalid value: use a sentinel error instead
/usr/local/go/src/net/internal/socktest/switch.go:142:3: return both a `nil` error and an invalid value: use a sentinel error instead
```
</details>
6 changes: 3 additions & 3 deletions Taskfile.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ tasks:
- task: tidy
- task: fmt
- task: lint
- task: tests
- task: test
- task: install

tools:install:
Expand All @@ -34,12 +34,12 @@ tasks:
- echo "Lint..."
- golangci-lint run --fix ./...

tests:
test:
cmds:
- echo "Tests..."
- go test ./...

tests:coverage:
test:coverage:
cmds:
- echo "Tests with coverage..."
- go test -coverprofile=coverage.out ./...
Expand Down
27 changes: 16 additions & 11 deletions pkg/analyzer/analyzer.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ const (
name = "nilnil"
doc = "Checks that there is no simultaneous return of `nil` error and an invalid value."

reportMsg = "return both the `nil` error and invalid value: use a sentinel error instead"
nilNilReportMsg = "return both a `nil` error and an invalid value: use a sentinel error instead"
notNilNotNilReportMsg = "return both a non-nil error and a valid value: use separate returns instead"
)

// New returns new nilnil analyzer.
Expand All @@ -29,17 +30,21 @@ func New() *analysis.Analyzer {
Requires: []*analysis.Analyzer{inspect.Analyzer},
}
a.Flags.Var(&n.checkedTypes, "checked-types", "comma separated list of return types to check")
a.Flags.BoolVar(&n.detectOpposite, "detect-opposite", false,
"in addition, detect opposite situation (simultaneous return of non-nil error and valid value)")

return a
}

type nilNil struct {
checkedTypes checkedTypes
checkedTypes checkedTypes
detectOpposite bool
}

func newNilNil() *nilNil {
return &nilNil{
checkedTypes: newDefaultCheckedTypes(),
checkedTypes: newDefaultCheckedTypes(),
detectOpposite: false,
}
}

Expand Down Expand Up @@ -93,16 +98,16 @@ func (n *nilNil) run(pass *analysis.Pass) (interface{}, error) {

retVal, retErr := v.Results[0], v.Results[1]

var needWarn bool
switch zv {
case zeroValueNil:
needWarn = isNil(pass, retVal) && isNil(pass, retErr)
case zeroValueZero:
needWarn = isZero(retVal) && isNil(pass, retErr)
if ((zv == zeroValueNil) && isNil(pass, retVal) && isNil(pass, retErr)) ||
((zv == zeroValueZero) && isZero(retVal) && isNil(pass, retErr)) {
pass.Reportf(v.Pos(), nilNilReportMsg)
return false
}

if needWarn {
pass.Reportf(v.Pos(), reportMsg)
if n.detectOpposite && (((zv == zeroValueNil) && !isNil(pass, retVal) && !isNil(pass, retErr)) ||
((zv == zeroValueZero) && !isZero(retVal) && !isNil(pass, retErr))) {
pass.Reportf(v.Pos(), notNilNotNilReportMsg)
return false
}
}

Expand Down
25 changes: 24 additions & 1 deletion pkg/analyzer/analyzer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ func TestNilNil(t *testing.T) {
analysistest.Run(t, analysistest.TestData(), analyzer.New(), pkgs...)
}

func TestNilNil_Flags(t *testing.T) {
func TestNilNil_Flags_CheckedTypes(t *testing.T) {
t.Parallel()

anlzr := analyzer.New()
Expand All @@ -27,3 +27,26 @@ func TestNilNil_Flags(t *testing.T) {
}
analysistest.Run(t, analysistest.TestData(), anlzr, "pointers-only")
}

func TestNilNil_Flags_DetectOpposite(t *testing.T) {
t.Parallel()

anlzr := analyzer.New()
if err := anlzr.Flags.Set("detect-opposite", "true"); err != nil {
t.Fatal(err)
}
analysistest.Run(t, analysistest.TestData(), anlzr, "opposite")
}

func TestNilNil_Flags_DetectOppositeAndCheckedTypes(t *testing.T) {
t.Parallel()

anlzr := analyzer.New()
if err := anlzr.Flags.Set("detect-opposite", "true"); err != nil {
t.Fatal(err)
}
if err := anlzr.Flags.Set("checked-types", "chan,map"); err != nil {
t.Fatal(err)
}
analysistest.Run(t, analysistest.TestData(), anlzr, "opposite-chan-map-only")
}
4 changes: 2 additions & 2 deletions pkg/analyzer/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,11 @@ import (

func newDefaultCheckedTypes() checkedTypes {
return checkedTypes{
ptrType: {},
chanType: {},
funcType: {},
ifaceType: {},
mapType: {},
chanType: {},
ptrType: {},
uintptrType: {},
unsafeptrType: {},
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/analyzer/testdata/src/examples/issue29.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ type data struct {

func nilnil(a, b int) (*data, error) {
if a == 0 && b == 0 {
return nil, nil // want "return both the `nil` error and invalid value: use a sentinel error instead"
return nil, nil // want "return both a `nil` error and an invalid value: use a sentinel error instead"
}
return &data{a: a, b: b}, nil
}
Expand Down
62 changes: 62 additions & 0 deletions pkg/analyzer/testdata/src/examples/negative_opposite.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
package examples

import (
"bytes"
"errors"
"fmt"
"io"
"net"
"unsafe"
)

func primitivePtrTypeOpposite() (*int, error) {
if false {
return nil, io.EOF
}
return new(int), errors.New("validation failed")
}

func structPtrTypeOpposite() (*User, error) {
if false {
return nil, io.EOF
}
return new(User), fmt.Errorf("invalid %v", 42)
}

func unsafePtrOpposite() (unsafe.Pointer, error) {
if false {
return nil, io.EOF
}
var i int
return unsafe.Pointer(&i), io.EOF
}

func uintPtrOpposite() (uintptr, error) {
if false {
return 0, io.EOF
}
return 0xc82000c290, wrap(io.EOF)
}

func channelTypeOpposite() (ChannelType, error) {
if false {
return nil, io.EOF
}
return make(ChannelType), fmt.Errorf("wrapped: %w", io.EOF)
}

func funcTypeOpposite() (FuncType, error) {
if false {
return nil, io.EOF
}
return func(i int) int {
return 0
}, errors.New("no func type, please")
}

func ifaceTypeOpposite() (io.Reader, error) {
if false {
return nil, io.EOF
}
return new(bytes.Buffer), new(net.AddrError)
}
Loading

0 comments on commit 289392e

Please sign in to comment.