search

golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
124.05k stars 17.68k forks source link

proposal: cmd/vet: enable nilness checker by default #59714

Closed abhinav closed 5 months ago

abhinav commented 1 year ago

Summary

Consider enabling the nilness linter by default on go vet (and therefore go test).

Details

The nilness linter searches for potential nil pointer dereferences in code. It is not enabled by default for go vet or gopls. Users can opt into it for gopls with the analyses setting.

It is not compiled into go vet, so users cannot opt-into it as part of a build process easily. Today, two options are:

Historically, the linter was disabled in gopls by default because memory consumption from its dependence on SSA analysis was too high. It was brought up in the golang-tools call today that this may not be less of an issue now.

This proposal suggests enabling the nilness linter by default in go vet and gopls. If there are still good reasons not to do that, this proposal can be downgraded to building nilness into go vet and disabling it by default, allowing opt-in with a -nilness flag.

ianlancetaylor commented 1 year ago

CC @adonovan @timothy-king

adonovan commented 1 year ago

Historically, the linter was disabled in gopls by default because memory consumption from its dependence on SSA analysis was too high. It was brought up in the golang-tools call today that this may not be less of an issue now.

Memory usage during SSA construction was indeed the reason nilness was never added to vet, but I'm not aware of any reason things might have changed since we made that decision. Would you like to try adding nilness to vet and measuring its performance (e.g. on go vet std) before and after? If you're on Linux, you can use command time -f '%e %M' go vet std to measure its elapsed time and peak memory usage.

timothy-king commented 1 year ago

Historically, the linter was disabled in gopls by default because memory consumption from its dependence on SSA analysis was too high. It was brought up in the golang-tools call today that this may not be less of an issue now.

Memory is certainly something to be concerned about, but it is not the blocker.

The concern is importing x/tools/go/ssa into cmd/vet and by extension the go release. ssa+x/tools/go/packages+go/ast+go/types put together create a compiler frontend for go. Right now the ssa package is maintained at a different cadence than the go compiler. Turning nilness on by default in cmd/vet requires confidence that x/tools/go/ssa is roughly as correct and as robust as the main go compiler and standard libraries go/types and go/ast. (Also with acceptable performance.) Any crashes in nilness coming from ssa would otherwise become a blocker for the rest of cmd/vet, which would be unacceptable. It would also need to handle any new go constructs and features at the same time as they are released for the language. The current balance is to have ssa (and its dependents like nilness) be [somewhat] easily available to other tools.

I think my main point is that you just need to be aware that this proposal does require a more significant ongoing staffing effort than just adding nilness to the list of checkers run in vet.

abhinav commented 1 year ago

Fair points, @timothy-king. I can see the risk of that.

@adonovan, I'm not on Linux, but looks like /usr/bin/time -l provides similarly useful information.

Before:

% go version
go version go1.20.3 darwin/arm64
% /usr/bin/time -l go vet std
        2.21 real         8.82 user         4.27 sys
           118652928  maximum resident set size
                   0  average shared memory size
                   0  average unshared data size
                   0  average unshared stack size
              590602  page reclaims
                2799  page faults
                   0  swaps
                   0  block input operations
                   0  block output operations
                   0  messages sent
                   0  messages received
               10172  signals received
               15612  voluntary context switches
              150439  involuntary context switches
         12489495277  instructions retired
          8517906894  cycles elapsed
            46400960  peak memory footprint

Adding nilness to the same version of Go:

% (cd src && ./make.bash)
# ...
% GOROOT=$(pwd) /usr/bin/time -l ./bin/go vet std
# ...
        2.56 real        11.95 user         4.38 sys
           158105600  maximum resident set size
                   0  average shared memory size
                   0  average unshared data size
                   0  average unshared stack size
              655031  page reclaims
                1811  page faults
                   0  swaps
                   0  block input operations
                   0  block output operations
                   0  messages sent
                   0  messages received
               12776  signals received
               15422  voluntary context switches
              159276  involuntary context switches
         12162802284  instructions retired
          8102358971  cycles elapsed
            46417344  peak memory footprint

FYI, it reported some issues in std. The non-test ones appear to be intentional.

``` # bytes_test src/bytes/buffer_test.go:270:8: panic with nil value # crypto/internal/bigmod src/crypto/internal/bigmod/nat_test.go:184:10: tautological condition: nil == nil # encoding/xml src/encoding/xml/xml_test.go:1312:10: impossible condition: non-nil == nil # net/netip -: impossible condition: non-nil == nil -: impossible condition: non-nil == nil -: impossible condition: non-nil == nil -: impossible condition: non-nil == nil -: impossible condition: non-nil == nil # os_test src/os/timeout_test.go:371:10: impossible condition: non-nil == nil # net src/net/timeout_test.go:840:10: impossible condition: non-nil == nil # testing src/testing/example.go:97:28: tautological condition: nil == nil # runtime src/runtime/panic.go:988:2: nil dereference in store src/runtime/panic.go:1133:2: nil dereference in store src/runtime/panic.go:1175:2: nil dereference in store src/runtime/proc.go:277:3: nil dereference in store src/runtime/stack.go:1124:2: nil dereference in store # runtime_test src/runtime/callers_test.go:230:5: nil dereference in load src/runtime/callers_test.go:278:2: nil dereference in dynamic function call src/runtime/callers_test.go:306:3: nil dereference in dynamic function call src/runtime/crash_test.go:387:5: nil dereference in map update src/runtime/stack_test.go:321:2: nil dereference in store src/runtime/stack_test.go:938:2: nil dereference in store ``` (It's not obvious to me why positions for net/netip aren't reported.)
adonovan commented 1 year ago

Thanks. Seems like a substantial increase in max RSS (+32%). I'm not sure that's a dealbreaker, but Tim's response is cogent, as is the fact that the standard library is not "clean", and that some of those violations are intentional. I mailed https://go.dev/cl/486675 to fix the good ones.

rsc commented 1 year ago

In the long term we are aiming for all of go vet to be enabled during go test, which means vet must be fast. My understanding is that SSA is below the performance bar. I wonder if the nilness check can be written using the simpler cfg analysis.

adonovan commented 10 months ago

This summer we did some work to reduce the asymptotic cost of constructing the go/ssa representation, in particular eliminating the term that was proportional to the total number of dependencies. I just remeasured the cost of go vet -vettool=x std with and without nilness in the multichecker, and to the nearest half second I consistently see 4.0s real 15s user with and 3.0s real 12s user without nilness, so still around 25%-33% cost, so no apparent benefit in this case. (The benefit higher up the application dependency graph is enormous though.)

We have plans for to improve things further, but the cost of SSA is still to high to justify adding nilness to vet for now.

adonovan commented 6 months ago

Besides performance, the other aspect of this problem is that when new language features are added to the compiler, they need to be added to golang.org/x/go/ssa as well, and this can sometimes take a while; in the meantime, the vendored version of go/ssa is unable to process the dialect of Go understood by the compiler. While this not fundamentally different from any other analyzer, go/ssa is a more complex body of code, and as an example, the range-over-func feature of go1.23 only just landed in go/ssa this week (mostly my fault, but it doesn't change my point).

So, I think it would be impractical from a process perspective to add ssa-based analyzers to go vet, and I think we should decline this proposal.

rsc commented 5 months ago

This proposal has been declined as infeasible. — rsc for the proposal review group

mitar commented 5 months ago

I was also surprised that nilness is not enabled by default in go vet or golangci-lint but it is in VS Code so I was surprised that it finds issues my linting CI setup does not. Now I wonder, are there any other tools available which are not enabled in go vet one could opt-in? Is that documented somewhere?

adonovan commented 5 months ago

Now I wonder, are there any other tools available which are not enabled in go vet one could opt-in? Is that documented somewhere?

The gopls documentation explains the categories of analyzers it includes:

You can find gopls' internal analyzer configuration, which lists the particulars, here: