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

Add modeling for `errors.As` in the generalized hook framework #281

Closed yuxincs closed 1 month ago

yuxincs commented 2 months ago

This PR introduces a new hook point in the CFG preprocessing logic to replace conditionals with equivalent expressions (along with a few simple renames and comment additions for better readability).

With the new hook, we are introducing modeling for errors.As(err, &target) since it is equivalent to errors.As(err, &target) && target != nil. This makes the implication explicit such that NilAway is able to understand it during the analysis.

Note that technically target can still be nil even if errors.As(err, &target) is true. For example, if err is a typed nil (e.g., var err *exec.ExitError), then errors.As would actually find a match, but target would be set to the typed nil value, resulting in a nil target. However, in practice this should rarely happen such that even the official documentation assumes the target is non-nil after such check [1]. So here we make this assumption as well.

[1] https://pkg.go.dev/errors#As

Fixes #95

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 87.50000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 87.66%. Comparing base (3c8ea06) to head (fca4e01). Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
hook/replace_conditional.go 66.66% 3 Missing and 3 partials :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #281 +/- ## ========================================== + Coverage 87.63% 87.66% +0.02% ========================================== Files 65 66 +1 Lines 7916 7943 +27 ========================================== + Hits 6937 6963 +26 Misses 798 798 - Partials 181 182 +1 ```

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

github-actions[bot] commented 2 months ago

Golden Test

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

3271 errors on base branch (main, 3c8ea06) 3263 errors on test branch (934834a)

Diffs ```diff - /opt/hostedtoolcache/go/1.22.7/x64/src/crypto/tls/handshake_server_test.go:373:32: Potential nil panic detected. Observed nil flow from source to dereference point: - - tls/handshake_server_test.go:373:32: unassigned variable `opErr` accessed field `Err` - /opt/hostedtoolcache/go/1.22.7/x64/src/errors/example_test.go:93:35: Potential nil panic detected. Observed nil flow from source to dereference point: - - errors/example_test.go:93:35: unassigned variable `pathError` accessed field `Path` - /opt/hostedtoolcache/go/1.22.7/x64/src/go/types/api_test.go:2516:6: Potential nil panic detected. Observed nil flow from source to dereference point: - - types/api_test.go:2516:6: unassigned variable `argErr` accessed field `Index` - - (Same nil source could also cause potential nil panic(s) at 1 other place(s): "types/api_test.go:2517:85".) - /opt/hostedtoolcache/go/1.22.7/x64/src/io/fs/readdir_test.go:101:9: Potential nil panic detected. Observed nil flow from source to dereference point: - - fs/readdir_test.go:101:9: unassigned variable `perr` accessed field `Path` - /opt/hostedtoolcache/go/1.22.7/x64/src/net/dnsclient_unix.go:859:33: Potential nil panic detected. Observed nil flow from source to dereference point: - - net/dnsclient_unix.go:859:33: unassigned variable `dnsErr` accessed field `IsNotFound` - /opt/hostedtoolcache/go/1.22.7/x64/src/net/dnsclient_unix_test.go:2677:34: Potential nil panic detected. Observed nil flow from source to dereference point: - - net/dnsclient_unix_test.go:2677:34: unassigned variable `dnsErr` accessed field `Err` - /opt/hostedtoolcache/go/1.22.7/x64/src/net/lookup_test.go:1435:8: Potential nil panic detected. Observed nil flow from source to dereference point: - - net/lookup_test.go:1435:8: unassigned variable `dnsErr` accessed field `IsNotFound` - - (Same nil source could also cause potential nil panic(s) at 1 other place(s): "net/lookup_test.go:1440:7".) - /opt/hostedtoolcache/go/1.22.7/x64/src/os/os_test.go:852:29: Potential nil panic detected. Observed nil flow from source to dereference point: - - os/os_test.go:852:29: unassigned variable `pe` accessed field `Path` ```