Skip to content

Commit

Permalink
Support uintptr and unsafe.Pointer (#34)
Browse files Browse the repository at this point in the history
* Support uintptr and unsafe.Pointer

* README: link to golangci-lint usage

* More tests
  • Loading branch information
Antonboom authored May 10, 2024
1 parent 43a41a7 commit 285f2af
Show file tree
Hide file tree
Showing 9 changed files with 204 additions and 88 deletions.
29 changes: 14 additions & 15 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -47,28 +47,35 @@ if err != nil {
### What if I think it's bullshit?

I understand that each case needs to be analyzed separately,
but I hope that the linter will make you think again -
is it necessary to use an ambiguous API or is it better to do it using a sentinel error?
but I hope that the linter will make you think again
is it necessary to use an **ambiguous API** or is it better to do it using a sentinel error?
<br>

In any case, you can just not enable the linter.

## Configuration

### CLI

```shell
# command line (see help for full list of types)
# See help for full list of types.
$ nilnil --checked-types ptr,func ./...
```

### golangci-lint

https://golangci-lint.run/usage/linters/#nilnil

```yaml
# https://golangci-lint.run/usage/configuration/
nilnil:
checked-types:
- ptr
- func
- iface
- map
- chan
- uintptr
- unsafeptr
```
## Examples
Expand Down Expand Up @@ -219,20 +226,12 @@ func (r *RateLimiter) Allow() bool {

## Assumptions

<details>
<summary>Click to expand</summary>

<br>

- Linter only checks functions with two return arguments, the last of which has `error` type.
- Linter only checks functions with two return arguments, the last of which implements `error`.
- Next types are checked:
* pointers, functions & interfaces (`panic: invalid memory address or nil pointer dereference`);
* pointers (including `uinptr` and `unsafe.Pointer`), functions and interfaces (`panic: invalid memory address or nil pointer dereference`);
* maps (`panic: assignment to entry in nil map`);
* channels (`fatal error: all goroutines are asleep - deadlock!`)
- `uinptr` & `unsafe.Pointer` are not checked as a special case.
- Supported only explicit `return nil, nil`.

</details>
- Only explicit `return nil, nil` are supported.

## Check Go 1.22.2 source code

Expand Down
83 changes: 63 additions & 20 deletions pkg/analyzer/analyzer.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@ package analyzer

import (
"go/ast"
"go/token"
"go/types"
"strconv"

"golang.org/x/tools/go/analysis"
"golang.org/x/tools/go/analysis/passes/inspect"
Expand Down Expand Up @@ -74,13 +76,32 @@ func (n *nilNil) run(pass *analysis.Pass) (interface{}, error) {
return false
}

fRes1, fRes2 := ft.Results.List[0], ft.Results.List[1]
if !(n.isDangerNilField(pass, fRes1) && n.isErrorField(pass, fRes2)) {
fRes1Type := pass.TypesInfo.TypeOf(ft.Results.List[0].Type)
if fRes1Type == nil {
return false
}

rRes1, rRes2 := v.Results[0], v.Results[1]
if isNil(pass, rRes1) && isNil(pass, rRes2) {
fRes2Type := pass.TypesInfo.TypeOf(ft.Results.List[1].Type)
if fRes2Type == nil {
return false
}

ok, zv := n.isDangerNilType(fRes1Type)
if !(ok && isErrorType(fRes2Type)) {
return false
}

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 needWarn {
pass.Reportf(v.Pos(), reportMsg)
}
}
Expand All @@ -91,41 +112,47 @@ func (n *nilNil) run(pass *analysis.Pass) (interface{}, error) {
return nil, nil //nolint:nilnil
}

func (n *nilNil) isDangerNilField(pass *analysis.Pass, f *ast.Field) bool {
return n.isDangerNilType(pass.TypesInfo.TypeOf(f.Type))
}
type zeroValue int

func (n *nilNil) isDangerNilType(t types.Type) bool {
const (
zeroValueNil = iota + 1
zeroValueZero
)

func (n *nilNil) isDangerNilType(t types.Type) (bool, zeroValue) {
switch v := t.(type) {
case *types.Pointer:
return n.checkedTypes.Contains(ptrType)
return n.checkedTypes.Contains(ptrType), zeroValueNil

case *types.Signature:
return n.checkedTypes.Contains(funcType)
return n.checkedTypes.Contains(funcType), zeroValueNil

case *types.Interface:
return n.checkedTypes.Contains(ifaceType)
return n.checkedTypes.Contains(ifaceType), zeroValueNil

case *types.Map:
return n.checkedTypes.Contains(mapType)
return n.checkedTypes.Contains(mapType), zeroValueNil

case *types.Chan:
return n.checkedTypes.Contains(chanType)
return n.checkedTypes.Contains(chanType), zeroValueNil

case *types.Basic:
if v.Kind() == types.Uintptr {
return n.checkedTypes.Contains(uintptrType), zeroValueZero
}
if v.Kind() == types.UnsafePointer {
return n.checkedTypes.Contains(unsafeptrType), zeroValueNil
}

case *types.Named:
return n.isDangerNilType(v.Underlying())
}
return false
return false, 0
}

var errorIface = types.Universe.Lookup("error").Type().Underlying().(*types.Interface)

func (n *nilNil) isErrorField(pass *analysis.Pass, f *ast.Field) bool {
t := pass.TypesInfo.TypeOf(f.Type)
if t == nil {
return false
}

func isErrorType(t types.Type) bool {
_, ok := t.Underlying().(*types.Interface)
return ok && types.Implements(t, errorIface)
}
Expand All @@ -139,3 +166,19 @@ func isNil(pass *analysis.Pass, e ast.Expr) bool {
_, ok = pass.TypesInfo.ObjectOf(i).(*types.Nil)
return ok
}

func isZero(e ast.Expr) bool {
bl, ok := e.(*ast.BasicLit)
if !ok {
return false
}
if bl.Kind != token.INT {
return false
}

v, err := strconv.ParseInt(bl.Value, 0, 64)
if err != nil {
return false
}
return v == 0
}
13 changes: 13 additions & 0 deletions pkg/analyzer/analyzer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,22 @@ import (
)

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

pkgs := []string{
"examples",
"strange",
"unsafe",
}
analysistest.Run(t, analysistest.TestData(), analyzer.New(), pkgs...)
}

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

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

func newDefaultCheckedTypes() checkedTypes {
return checkedTypes{
ptrType: struct{}{},
funcType: struct{}{},
ifaceType: struct{}{},
mapType: struct{}{},
chanType: struct{}{},
ptrType: {},
funcType: {},
ifaceType: {},
mapType: {},
chanType: {},
uintptrType: {},
unsafeptrType: {},
}
}

Expand All @@ -25,15 +27,15 @@ func (t typeName) S() string {
}

const (
ptrType typeName = "ptr"
funcType typeName = "func"
ifaceType typeName = "iface"
mapType typeName = "map"
chanType typeName = "chan"
ptrType typeName = "ptr"
funcType typeName = "func"
ifaceType typeName = "iface"
mapType typeName = "map"
chanType typeName = "chan"
uintptrType typeName = "uintptr"
unsafeptrType typeName = "unsafeptr"
)

var knownTypes = []typeName{ptrType, funcType, ifaceType, mapType, chanType}

type checkedTypes map[typeName]struct{}

func (c checkedTypes) Contains(t typeName) bool {
Expand All @@ -60,7 +62,7 @@ func (c checkedTypes) Set(s string) error {
c.disableAll()
for _, t := range types {
switch tt := typeName(t); tt {
case ptrType, funcType, ifaceType, mapType, chanType:
case ptrType, funcType, ifaceType, mapType, chanType, uintptrType, unsafeptrType:
c[tt] = struct{}{}
default:
return fmt.Errorf("unknown checked type name %q (see help)", t)
Expand Down
42 changes: 11 additions & 31 deletions pkg/analyzer/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,33 +4,18 @@ import "testing"

func TestCheckedTypes(t *testing.T) {
c := newDefaultCheckedTypes()

for _, tt := range knownTypes {
assertTrue(t, c.Contains(tt))
}
assertStringEqual(t, c.String(), "chan,func,iface,map,ptr")
assertStringEqual(t, c.String(), "chan,func,iface,map,ptr,uintptr,unsafeptr")

err := c.Set("chan,iface,ptr")
assertNoError(t, err)
assertTrue(t, c.Contains(chanType))
assertTrue(t, c.Contains(ifaceType))
assertTrue(t, c.Contains(ptrType))
assertFalse(t, c.Contains(funcType))
assertFalse(t, c.Contains(mapType))
assertFalse(t, c.Contains(uintptrType))
assertFalse(t, c.Contains(unsafeptrType))
assertStringEqual(t, "chan,iface,ptr", c.String())

for _, tt := range knownTypes {
err := c.Set(tt.S())
assertNoError(t, err)

for _, tt2 := range knownTypes {
if tt2 == tt {
assertTrue(t, c.Contains(tt2))
} else {
assertFalse(t, c.Contains(tt2))
}
}

assertStringEqual(t, tt.S(), c.String())
}
}

func TestCheckedTypes_SetError(t *testing.T) {
Expand All @@ -46,45 +31,40 @@ func TestCheckedTypes_SetWithoutArg(t *testing.T) {

err := c.Set("")
assertNoError(t, err)

for _, tt := range knownTypes {
assertTrue(t, c.Contains(tt))
}
assertStringEqual(t, c.String(), "chan,func,iface,map,ptr")
assertStringEqual(t, c.String(), "chan,func,iface,map,ptr,uintptr,unsafeptr")
}

func assertError(t *testing.T, err error) {
t.Helper()
if err == nil {
t.FailNow()
t.Fatal("must be not nil")
}
}

func assertNoError(t *testing.T, err error) {
t.Helper()
if err != nil {
t.FailNow()
t.Fatalf("must be nil, got %q", err)
}
}

func assertStringEqual(t *testing.T, a, b string) {
t.Helper()
if a != b {
t.Logf("%q != %q", a, b)
t.FailNow()
t.Fatalf("%q != %q", a, b)
}
}

func assertTrue(t *testing.T, v bool) {
t.Helper()
if !v {
t.FailNow()
t.Fatal("must be true")
}
}

func assertFalse(t *testing.T, v bool) {
t.Helper()
if v {
t.FailNow()
t.Fatal("must be false")
}
}
Loading

0 comments on commit 285f2af

Please sign in to comment.