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

adonovan commented 1 month ago

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()))
+                               }
                        }
                }

gabyhelp commented 1 month ago

Related Issues and Documentation

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

findleyr commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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.

golang / go

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