Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
abhinav opened this issue Apr 19, 2023 · 7 comments
Open

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

abhinav opened this issue Apr 19, 2023 · 7 comments
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) Proposal
Milestone

Comments

@abhinav
Copy link
Contributor

abhinav commented Apr 19, 2023

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:

  • Install it manually with a fairly long import path, and use it with go vet -vettool

    go install golang.org/x/tools/go/analysis/passes/nilness/cmd/nilness@latest
    go vet -vettool $(which nilness) ./...
  • Enable it in golangci-lint by adding it under the linter-settings.govet.enable configuration key

    linters-settings:
      govet:
        enable: [nilness]

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.

@gopherbot gopherbot added this to the Proposal milestone Apr 19, 2023
@seankhliao seankhliao changed the title proposal: vet: Enable nilness by default proposal: cmd/vet: Enable nilness by default Apr 19, 2023
@ianlancetaylor ianlancetaylor changed the title proposal: cmd/vet: Enable nilness by default proposal: cmd/vet: enable nilness checker by default Apr 19, 2023
@ianlancetaylor
Copy link
Contributor

CC @adonovan @timothy-king

@adonovan
Copy link
Member

adonovan commented Apr 19, 2023

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
Copy link
Contributor

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
Copy link
Contributor Author

abhinav commented Apr 19, 2023

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
Copy link
Member

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.

@adonovan adonovan added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Apr 23, 2023
abhinav added a commit to pulumi/pulumi that referenced this issue May 19, 2023
govet includes a nilness linter that detects a few nil issues.
This linter is not enabled by default in govet because the `go` tool
does not want a dependency on the libraries necessary to implement this
(golang/go#59714).

Enable the nilness linter and fix the following issues found by it
(commentary below each check mine):

    pkg/cmd/pulumi/new.go:283:9: nilness: impossible condition: nil != nil (govet)
        ^- the error was very likely supposed to be the `os.Remove`
    pkg/cmd/pulumi/new.go:633:10: nilness: impossible condition: nil != nil (govet)
        ^- same error checked on the previous line
    pkg/cmd/pulumi/preview.go:190:11: nilness: impossible condition: nil != nil (govet)
        ^- same error checked a few blocks above
    pkg/codegen/pcl/binder_component.go:101:64: nilness: nil dereference in dynamic method call (govet)
        ^- err is guaranteed nil
    pkg/codegen/pcl/binder_component.go:133:10: nilness: impossible condition: nil != nil (govet)
        ^- err is guaranteed nil
    sdk/go/auto/errors_test.go:374:34: nilness: nil dereference in index operation (govet)
        ^- this is intentional; I replaced it with a deliberate panic
    tests/integration/construct_component_methods_resources/testcomponent-go/random.go:30:10: nilness: impossible condition: non-nil == nil (govet)
        ^- args is rejected if it's nil
bors bot added a commit to pulumi/pulumi that referenced this issue May 22, 2023
12990: lint(govet): Enable nilness linter r=abhinav a=abhinav

govet includes a nilness linter that detects a few nil issues.
This linter is not enabled by default in govet because the `go` tool
does not want a dependency on the libraries necessary to implement this
(golang/go#59714).

Enable the nilness linter and fix the following issues found by it
(commentary below each check mine):

    pkg/cmd/pulumi/new.go:283:9: nilness: impossible condition: nil != nil (govet)
        ^- the error was very likely supposed to be the `os.Remove`
    pkg/cmd/pulumi/new.go:633:10: nilness: impossible condition: nil != nil (govet)
        ^- same error checked on the previous line
    pkg/cmd/pulumi/preview.go:190:11: nilness: impossible condition: nil != nil (govet)
        ^- same error checked a few blocks above
    pkg/codegen/pcl/binder_component.go:101:64: nilness: nil dereference in dynamic method call (govet)
        ^- err is guaranteed nil
    pkg/codegen/pcl/binder_component.go:133:10: nilness: impossible condition: nil != nil (govet)
        ^- err is guaranteed nil
    sdk/go/auto/errors_test.go:374:34: nilness: nil dereference in index operation (govet)
        ^- this is intentional; I replaced it with a deliberate panic
    tests/integration/construct_component_methods_resources/testcomponent-go/random.go:30:10: nilness: impossible condition: non-nil == nil (govet)
        ^- args is rejected if it's nil


Co-authored-by: Abhinav Gupta <abhinav@pulumi.com>
@rsc
Copy link
Contributor

rsc commented Jun 28, 2023

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) Proposal
Projects
Status: Incoming
Development

No branches or pull requests

6 participants