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

x/tools/go/analysis/nilness: heuristics for flagging conversions from nil *T to interface #69645

Open
adonovan opened this issue Sep 26, 2024 · 4 comments
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@adonovan
Copy link
Member

adonovan commented Sep 26, 2024

We have forever been discussing ways to make the nilness analyzer report conversions from a definitely-nil pointer of type *T to an interface type. The challenge is how to distinguish the legitimate uses (when a nil *T pointer is valid) from the mistakes. This issue is a place to publicly record some of the raw data we got from running a simple heuristic across the Go module mirror corpus and discuss potential improvements.

The analysis is just this patch to the existing code:

diff --git a/go/analysis/passes/nilness/nilness.go b/go/analysis/passes/nilness/nilness.go
index faaf12a93..8b049a4ea 100644
--- a/go/analysis/passes/nilness/nilness.go
+++ b/go/analysis/passes/nilness/nilness.go
@@ -121,6 +121,15 @@ func runFunc(pass *analysis.Pass, fn *ssa.Function) {
                        case *ssa.Send:
                                // (Not a runtime error, but a likely mistake.)
                                notNil(stack, instr, instr.Chan, "send to nil channel")
+
+                       case *ssa.MakeInterface:
+                               switch instr.X.Type().Underlying().(type) {
+                               case *types.Slice, *types.Map:
+                                       // nils of these types are fine
+                               default:
+                                       notNil(stack, instr, instr.X,
+                                               fmt.Sprintf("converting nil %s to interface %s", instr.X.Type(), instr.Type()))
+                               }
                        }
                }

https://go-mod-viewer.appspot.com/github.com/cilium/ebpf@v0.16.0/linker.go#L319: converting nil *github.com/cilium/ebpf/btf.Func to interface github.com/cilium/ebpf/btf.Type
https://go-mod-viewer.appspot.com/github.com/cilium/ebpf@v0.16.0/linker.go#L319: converting nil *github.com/cilium/ebpf/btf.Func to interface github.com/cilium/ebpf/btf.Type
https://go-mod-viewer.appspot.com/github.com/ava-labs/avalanchego@v1.11.11/vms/platformvm/txs/add_delegator_test.go#L207: converting nil *github.com/ava-labs/avalanchego/vms/platformvm/txs.AddDelegatorTx to interface any
https://go-mod-viewer.appspot.com/github.com/ava-labs/avalanchego@v1.11.11/vms/platformvm/txs/add_permissionless_delegator_tx_test.go#L1861: converting nil *github.com/ava-labs/avalanchego/vms/platformvm/txs.AddPermissionlessDelegatorTx to interface any
https://go-mod-viewer.appspot.com/github.com/ava-labs/avalanchego@v1.11.11/vms/platformvm/txs/add_permissionless_validator_tx_test.go#L1834: converting nil *github.com/ava-labs/avalanchego/vms/platformvm/txs.AddPermissionlessValidatorTx to interface any
https://go-mod-viewer.appspot.com/github.com/ava-labs/avalanchego@v1.11.11/vms/platformvm/txs/add_subnet_validator_test.go#L215: converting nil *github.com/ava-labs/avalanchego/vms/platformvm/txs.AddSubnetValidatorTx to interface any
https://go-mod-viewer.appspot.com/github.com/ava-labs/avalanchego@v1.11.11/vms/platformvm/txs/add_subnet_validator_test.go#L221: converting nil *github.com/ava-labs/avalanchego/vms/platformvm/txs.AddSubnetValidatorTx to interface any
https://go-mod-viewer.appspot.com/github.com/ava-labs/avalanchego@v1.11.11/vms/platformvm/txs/add_subnet_validator_test.go#L227: converting nil *github.com/ava-labs/avalanchego/vms/platformvm/txs.AddSubnetValidatorTx to interface any
https://go-mod-viewer.appspot.com/github.com/ava-labs/avalanchego@v1.11.11/vms/platformvm/txs/add_validator_test.go#L224: converting nil *github.com/ava-labs/avalanchego/vms/platformvm/txs.AddValidatorTx to interface any
https://go-mod-viewer.appspot.com/github.com/go-spring/spring-base@v1.1.3/log/plugin_filter.go#L28: converting nil *github.com/go-spring/spring-base/log.AcceptAllFilter to interface github.com/go-spring/spring-base/log.Filter
https://go-mod-viewer.appspot.com/github.com/go-spring/spring-base@v1.1.3/log/plugin_filter.go#L29: converting nil *github.com/go-spring/spring-base/log.DenyAllFilter to interface github.com/go-spring/spring-base/log.Filter
https://go-mod-viewer.appspot.com/github.com/go-spring/spring-base@v1.1.3/log/plugin_filter.go#L30: converting nil *github.com/go-spring/spring-base/log.LevelFilter to interface github.com/go-spring/spring-base/log.Filter
https://go-mod-viewer.appspot.com/github.com/go-spring/spring-base@v1.1.3/log/plugin_filter.go#L31: converting nil *github.com/go-spring/spring-base/log.LevelMatchFilter to interface github.com/go-spring/spring-base/log.Filter
https://go-mod-viewer.appspot.com/github.com/go-spring/spring-base@v1.1.3/log/plugin_filter.go#L32: converting nil *github.com/go-spring/spring-base/log.LevelRangeFilter to interface github.com/go-spring/spring-base/log.Filter
https://go-mod-viewer.appspot.com/github.com/go-spring/spring-base@v1.1.3/log/plugin_filter.go#L33: converting nil *github.com/go-spring/spring-base/log.TimeFilter to interface github.com/go-spring/spring-base/log.Filter
https://go-mod-viewer.appspot.com/github.com/go-spring/spring-base@v1.1.3/log/plugin_filter.go#L34: converting nil *github.com/go-spring/spring-base/log.TagFilter to interface github.com/go-spring/spring-base/log.Filter
https://go-mod-viewer.appspot.com/github.com/go-spring/spring-base@v1.1.3/log/plugin_filter.go#L35: converting nil *github.com/go-spring/spring-base/log.CompositeFilter to interface github.com/go-spring/spring-base/log.Filter
https://go-mod-viewer.appspot.com/github.com/andersfylling/snowflake/v5@v5.0.1/snowflake_test.go#L66: converting nil *github.com/andersfylling/snowflake/v5.Snowflake to interface interface{}
https://go-mod-viewer.appspot.com/github.com/andersfylling/snowflake/v5@v5.0.1/snowflake_test.go#L88: converting nil *github.com/andersfylling/snowflake/v5.Snowflake to interface interface{}
https://go-mod-viewer.appspot.com/github.com/andersfylling/snowflake/v5@v5.0.1/snowflake_test.go#L111: converting nil *github.com/andersfylling/snowflake/v5.Snowflake to interface interface{}
https://go-mod-viewer.appspot.com/k8s.io/client-go@v0.31.1/rest/request_test.go#L1968: converting nil *k8s.io/apimachinery/pkg/apis/meta/v1.DeleteOptions to interface interface{}
https://go-mod-viewer.appspot.com/github.com/MetalBlockchain/metalgo@v1.11.9/vms/platformvm/txs/add_delegator_test.go#L207: converting nil *github.com/MetalBlockchain/metalgo/vms/platformvm/txs.AddDelegatorTx to interface any
https://go-mod-viewer.appspot.com/github.com/MetalBlockchain/metalgo@v1.11.9/vms/platformvm/txs/add_permissionless_delegator_tx_test.go#L1860: converting nil *github.com/MetalBlockchain/metalgo/vms/platformvm/txs.AddPermissionlessDelegatorTx to interface any
https://go-mod-viewer.appspot.com/github.com/MetalBlockchain/metalgo@v1.11.9/vms/platformvm/txs/add_permissionless_validator_tx_test.go#L1833: converting nil *github.com/MetalBlockchain/metalgo/vms/platformvm/txs.AddPermissionlessValidatorTx to interface any
https://go-mod-viewer.appspot.com/github.com/MetalBlockchain/metalgo@v1.11.9/vms/platformvm/txs/add_subnet_validator_test.go#L215: converting nil *github.com/MetalBlockchain/metalgo/vms/platformvm/txs.AddSubnetValidatorTx to interface any
https://go-mod-viewer.appspot.com/github.com/MetalBlockchain/metalgo@v1.11.9/vms/platformvm/txs/add_subnet_validator_test.go#L221: converting nil *github.com/MetalBlockchain/metalgo/vms/platformvm/txs.AddSubnetValidatorTx to interface any
https://go-mod-viewer.appspot.com/github.com/MetalBlockchain/metalgo@v1.11.9/vms/platformvm/txs/add_subnet_validator_test.go#L227: converting nil *github.com/MetalBlockchain/metalgo/vms/platformvm/txs.AddSubnetValidatorTx to interface any
https://go-mod-viewer.appspot.com/github.com/MetalBlockchain/metalgo@v1.11.9/vms/platformvm/txs/add_validator_test.go#L224: converting nil *github.com/MetalBlockchain/metalgo/vms/platformvm/txs.AddValidatorTx to interface any
https://go-mod-viewer.appspot.com/github.com/enbility/spine-go@v0.7.0/util/type.go#L11: converting nil *T to interface any
https://go-mod-viewer.appspot.com/github.com/cilium/cilium@v1.16.2/pkg/types/portmap_test.go#L163: converting nil *github.com/cilium/cilium/pkg/types.namedPortMultiMap to interface github.com/cilium/cilium/pkg/types.NamedPortMultiMap
https://go-mod-viewer.appspot.com/github.com/vmware/govmomi@v0.43.0/vim25/types/helpers_test.go#L352: converting nil *T to interface any
https://go-mod-viewer.appspot.com/github.com/timandy/routine@v1.1.4/thread_local_map_entry_test.go#L68: converting nil *github.com/timandy/routine.personCloneable to interface github.com/timandy/routine.entry
https://go-mod-viewer.appspot.com/github.com/timandy/routine@v1.1.4/thread_local_map_entry_test.go#L137: converting nil *github.com/timandy/routine.personCloneable to interface github.com/timandy/routine.entry
https://go-mod-viewer.appspot.com/github.com/openimsdk/tools@v0.0.49/mw/replace_nil_test.go#L42: converting nil *github.com/openimsdk/tools/mw.A to interface any
https://go-mod-viewer.appspot.com/github.com/expr-lang/expr@v1.16.9/test/deref/deref_test.go#L188: converting nil *int32 to interface any
https://go-mod-viewer.appspot.com/goki.dev/laser@v0.1.34/basic_test.go#L26: converting nil *goki.dev/laser.A to interface any
https://go-mod-viewer.appspot.com/github.com/goki/ki@v1.1.17/kit/convert_test.go#L24: converting nil *github.com/goki/ki/kit.A to interface any
https://go-mod-viewer.appspot.com/github.com/davecgh/go-xdr@v0.0.0-20161123171359-e6a2ba005892/xdr2/decode_test.go#L699: converting nil *int32 to interface interface{}
https://go-mod-viewer.appspot.com/k8s.io/apiserver@v0.31.1/pkg/admission/plugin/cel/filter_test.go#L475: converting nil *k8s.io/apimachinery/pkg/apis/meta/v1/unstructured.Unstructured to interface k8s.io/apimachinery/pkg/runtime.Object
https://go-mod-viewer.appspot.com/k8s.io/kube-openapi@v0.0.0-20240826222958-65a50c78dec5/pkg/internal/third_party/go-json-experiment/json/arshal_test.go#L3317: converting nil *k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json.valueStringer to interface any
https://go-mod-viewer.appspot.com/k8s.io/kube-openapi@v0.0.0-20240826222958-65a50c78dec5/pkg/internal/third_party/go-json-experiment/json/arshal_test.go#L3322: converting nil **k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json.valueStringer to interface any
https://go-mod-viewer.appspot.com/k8s.io/kube-openapi@v0.0.0-20240826222958-65a50c78dec5/pkg/internal/third_party/go-json-experiment/json/arshal_test.go#L3333: converting nil *k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json.pointerStringer to interface any
https://go-mod-viewer.appspot.com/k8s.io/kube-openapi@v0.0.0-20240826222958-65a50c78dec5/pkg/internal/third_party/go-json-experiment/json/arshal_test.go#L3338: converting nil **k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json.pointerStringer to interface any
https://go-mod-viewer.appspot.com/k8s.io/kube-openapi@v0.0.0-20240826222958-65a50c78dec5/pkg/internal/third_party/go-json-experiment/json/arshal_test.go#L7131: converting nil *k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json.valueStringer to interface any
https://go-mod-viewer.appspot.com/k8s.io/kube-openapi@v0.0.0-20240826222958-65a50c78dec5/pkg/internal/third_party/go-json-experiment/json/arshal_test.go#L7137: converting nil **k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json.valueStringer to interface any
https://go-mod-viewer.appspot.com/k8s.io/kube-openapi@v0.0.0-20240826222958-65a50c78dec5/pkg/internal/third_party/go-json-experiment/json/arshal_test.go#L7151: converting nil *k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json.pointerStringer to interface any
https://go-mod-viewer.appspot.com/k8s.io/kube-openapi@v0.0.0-20240826222958-65a50c78dec5/pkg/internal/third_party/go-json-experiment/json/arshal_test.go#L7157: converting nil **k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json.pointerStringer to interface any
https://go-mod-viewer.appspot.com/github.com/stellar/go-xdr@v0.0.0-20231122183749-b53fb00bcac2/xdr2/decode_test.go#L630: converting nil *int32 to interface interface{}
https://go-mod-viewer.appspot.com/github.com/stellar/go-xdr@v0.0.0-20231122183749-b53fb00bcac2/xdr3/decode_test.go#L755: converting nil *int32 to interface interface{}
https://go-mod-viewer.appspot.com/open-match.dev/open-match@v1.8.1/examples/demo/updater/updater_test.go#L47: converting nil *int to interface interface{}
https://go-mod-viewer.appspot.com/github.com/wI2L/jettison@v0.7.4/json_1.14_test.go#L96: converting nil *github.com/wI2L/jettison.niljsonm to interface encoding/json.Marshaler
https://go-mod-viewer.appspot.com/github.com/wI2L/jettison@v0.7.4/json_1.14_test.go#L104: converting nil *github.com/wI2L/jettison.niltextm to interface encoding.TextMarshaler
https://go-mod-viewer.appspot.com/github.com/wI2L/jettison@v0.7.4/json_1.14_test.go#L112: converting nil *github.com/wI2L/jettison.niljetim to interface github.com/wI2L/jettison.comboMarshaler
https://go-mod-viewer.appspot.com/github.com/wI2L/jettison@v0.7.4/json_1.14_test.go#L120: converting nil *github.com/wI2L/jettison.nilmjctx to interface github.com/wI2L/jettison.comboMarshalerCtx
https://go-mod-viewer.appspot.com/github.com/hashicorp/go-metrics@v0.5.3/circonus/circonus_test.go#L188: converting nil *github.com/hashicorp/go-metrics/circonus.CirconusSink to interface github.com/hashicorp/go-metrics.MetricSink
https://go-mod-viewer.appspot.com/github.com/hashicorp/go-metrics@v0.5.3/datadog/dogstatsd_test.go#L165: converting nil *github.com/hashicorp/go-metrics/datadog.DogStatsdSink to interface github.com/hashicorp/go-metrics.MetricSink
https://go-mod-viewer.appspot.com/github.com/hashicorp/go-metrics@v0.5.3/prometheus/prometheus_test.go#L395: converting nil *github.com/hashicorp/go-metrics/prometheus.PrometheusSink to interface github.com/hashicorp/go-metrics.MetricSink
https://go-mod-viewer.appspot.com/github.com/hashicorp/go-metrics@v0.5.3/prometheus/prometheus_test.go#L397: converting nil *github.com/hashicorp/go-metrics/prometheus.PrometheusPushSink to interface github.com/hashicorp/go-metrics.MetricSink
https://go-mod-viewer.appspot.com/bitbucket.org/ai69/amoy@v0.2.3/shell_test.go#L106: converting nil *bitbucket.org/ai69/amoy.customStruct to interface bitbucket.org/ai69/amoy.customInterface
https://go-mod-viewer.appspot.com/github.com/blend/go-sdk@v1.20240719.1/ex/ex_test.go#L343: converting nil *github.com/blend/go-sdk/ex.Ex to interface any
https://go-mod-viewer.appspot.com/github.com/blend/go-sdk@v1.20240719.1/ex/ex_test.go#L374: converting nil *github.com/blend/go-sdk/ex.Ex to interface any

@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Sep 26, 2024
@adonovan adonovan added Analysis Issues related to static analysis (vet, x/tools/go/analysis) and removed Tools This label describes issues relating to any tools in the x/tools repository. labels Sep 26, 2024
@gopherbot gopherbot added this to the Unreleased milestone Sep 26, 2024
@findleyr
Copy link
Member

Thanks for collecting this data! The first 10 links I clicked were false positives.

Arbitrary Examples:

  • target := btf.Type((*btf.Func)(nil))
  • RegisterPlugin("TagFilter", PluginTypeFilter, (Filter)((*TagFilter)(nil)))
  • params: runtime.Object(nilUnstructured)
  • var dd *DogStatsdSink; _ = metrics.MetricSink(dd)
  • {"Custom Interface Struct Nil", customInterface(structNil), true} (in a function called TestIsInterfaceNil)
  • {P{1, 4}, addr(any((**valueStringer)(nil)))}
  • var i32p *int32 ... TstDecode(bytes.NewReader(buf))(reflect.ValueOf(i32p)) ... ii32p := interface{}(i32p)

This looks like various patterns of:

  • Intentionally distinguished nils.
  • Implements checks.
  • Intentional tests for typed nil handling.
  • Imprecise analysis due to use of reflect.

Probably some of these can be avoided, but the lack of clear true positives does not bode well.

@adonovan
Copy link
Member Author

the lack of clear true positives does not bode well.

To be clear, no-one intends to merge this change; it was just a starting point for other experiments.

I was surprised the corpus contained so few matches. I suspect the much more common case involves maybe nil pointers, e.g.

func maybeOpen(filename string) io.Reader
   var f *os.FIle
   if maybe { f, _ = os.Open(filename) }
   return f // oops
}

@ianlancetaylor
Copy link
Member

Since the goal here is to identify cases of https://go.dev/doc/faq#nil_error, we shouldn't even be looking at cases where the code explicitly converts to interface type. We should only be looking at cases where the conversion is implicit. Unfortunately the current list has too many irrelevant items to judge whether this is useful or not.

@mknyszek mknyszek added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 30, 2024
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) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

6 participants