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
Comments
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 |
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 |
Fair points, @timothy-king. I can see the risk of that. @adonovan, I'm not on Linux, but looks like Before:
Adding nilness to the same version of Go:
FYI, it reported some issues in std. The non-test ones appear to be intentional.
(It's not obvious to me why positions for net/netip aren't reported.) |
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. |
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
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>
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. |
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 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. |
Summary
Consider enabling the nilness linter by default on
go vet
(and thereforego test
).Details
The nilness linter searches for potential nil pointer dereferences in code.
It is not enabled by default for
go vet
orgopls
. Users can opt into it forgopls
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 keyHistorically, 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
andgopls
.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.The text was updated successfully, but these errors were encountered: