uber-go / nilaway

Static analysis tool to detect potential nil panics in Go code
Apache License 2.0
3.04k stars 59 forks source link

False positive on typed constant error #108

Open lbjarre opened 10 months ago

lbjarre commented 10 months ago

Hello,

First of all, thanks for open sourcing this! I'm testing it out in some codebases and it seems to be very capable of detecting lots of error already.

I have a codebase where using type wrappers on top of strings seems to trigger a false positive. Minimal reproducing program:

package main

type Error string

func (err Error) Error() string { return string(err) }

const AnError Error = "this is an error"

func action(x bool) (*bool, error) {
    if x {
        return nil, AnError
    }
    var b bool
    return &b, nil
}

func useAction() error {
    ptr, err := action(true)
    if err != nil {
        return err
    }
    _ = *ptr
    return nil
}

Results in this analysis:

❯ nilaway .
/tmp/nilawayt/t.go:22:7: error: Potential nil panic detected. Observed nil flow from source to dereferenc
e point: 
        -> nilawayt/t.go:11:10: literal `nil` returned from `action()` in position 0 when the error retur
n in position 1 is not guaranteed to be non-nil through all paths
        -> nilawayt/t.go:22:7: result 0 of `action()` dereferenced
dolmen commented 10 months ago

Confirming the issue is related to analysis of the value of the AnError expression in func action.

The following variation of program doesn't raise the error:

package main

import "errors"

var AnError = errors.New("this is an error")

func action(x bool) (*bool, error) {
    if x {
        return nil, AnError
    }
    var b bool
    return &b, nil
}

func useAction() error {
    ptr, err := action(true)
    if err != nil {
        return err
    }
    _ = *ptr
    return nil
}
sonalmahajan15 commented 9 months ago

@lbjarre: ACK. Thanks for reporting the issue. This is an interesting case. We do support error type for variables, return values, etc. (as confirmed by @dolmen), but not for named types. We will extend our support for handling such cases.

cheny-alf commented 4 months ago

@sonalmahajan15 I am interested in this. Can I take this up?

sonalmahajan15 commented 4 months ago

@cheny-alf: Sure, go ahead. Let us know if you need any help :)