uber-go / nilaway

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

Fix for making rich check effect handling unbiased for return statement ordering #254

Closed sonalmahajan15 closed 3 months ago

sonalmahajan15 commented 5 months ago

This PR fixes the bug which made rich check effect handling sensitive to return statement ordering, resulting in false positives for certain cases. The effect of ordering of statements is illustrated in the example below.

// Problematic
func retPtrErr() (*int, error) {
    if cond {
        return new(int), retNil()
    }
    return nil, retErr()
}

// Works fine
func retPtrErr() (*int, error) {
    if cond {
           return nil, retErr() 
    }   
        return new(int), retNil()
}

The problem is with no entry being created in inferredMap for non-nil error return from retErr() in StoreImplication, since we don't propagate non-nilness forward. This creates a problem when the anonymous function parameter in FilterTriggersForErrorReturn analyzes for nilability of return sites, where it incorrectly assumes that absence in inferredMap implies unknown nilability. This PR corrects that logic.

codecov[bot] commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 87.63%. Comparing base (9012b93) to head (6fb5e7a).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #254 +/- ## ========================================== - Coverage 87.64% 87.63% -0.02% ========================================== Files 63 63 Lines 7927 7926 -1 ========================================== - Hits 6948 6946 -2 - Misses 799 800 +1 Partials 180 180 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

github-actions[bot] commented 5 months ago

Golden Test

[!WARNING]
❌ NilAway errors reported on stdlib are different 📉.

3275 errors on base branch (main, 9012b93) 3271 errors on test branch (006a70c)

Diffs ```diff - /opt/hostedtoolcache/go/1.22.5/x64/src/database/sql/sql.go:560:2: Potential nil panic detected. Observed nil flow from source to dereference point: - - sql/sql.go:1313:10: literal `nil` returned from `conn()` in position 0 when the error return in position 1 is not guaranteed to be non-nil through all paths - - sql/sql.go:1619:31: result 0 of `conn()` used as receiver to call `releaseConn()` via the assignment(s): - - `db.conn(...)` to `dc` at sql/sql.go:1615:2 - - sql/sql.go:560:2: read by method receiver `dc` accessed field `db` - /opt/hostedtoolcache/go/1.22.5/x64/src/database/sql/sql.go:560:2: Potential nil panic detected. Observed nil flow from source to dereference point: - - sql/sql.go:1385:12: literal `nil` returned from `conn()` in position 0 when the error return in position 1 is not guaranteed to be non-nil through all paths - - sql/sql.go:1619:31: result 0 of `conn()` used as receiver to call `releaseConn()` via the assignment(s): - - `db.conn(...)` to `dc` at sql/sql.go:1615:2 - - sql/sql.go:560:2: read by method receiver `dc` accessed field `db` - /opt/hostedtoolcache/go/1.22.5/x64/src/net/http/transport.go:618:23: Potential nil panic detected. Observed nil flow from source to dereference point: - - http/transport.go:1419:12: literal `nil` returned from `getConn()` in position 0 when the error return in position 1 is not guaranteed to be non-nil through all paths - - http/transport.go:618:23: result 0 of `getConn()` accessed field `cacheKey` via the assignment(s): - - `t.getConn(...)` to `pconn` at http/transport.go:595:3 - /opt/hostedtoolcache/go/1.22.5/x64/src/net/http/transport.go:618:23: Potential nil panic detected. Observed nil flow from source to dereference point: - - http/transport.go:1433:10: literal `nil` returned from `getConn()` in position 0 when the error return in position 1 is not guaranteed to be non-nil through all paths - - http/transport.go:618:23: result 0 of `getConn()` accessed field `cacheKey` via the assignment(s): - - `t.getConn(...)` to `pconn` at http/transport.go:595:3 ```