x/tools/go/analysis/passes/nilness: false negative with pointers and type assertions

[ainar-g]

Currently, as of golang.org/x/tools v0.1.6-0.20210820212750-d4cc65f0b2ff and go1.17, nilness doesn't mark the following code as invalid despite tt being provably nil:

package main

import "fmt"

type T struct {
	a int
}

func main() {
	n := 0
	var v interface{} = &n

	tt, ok := v.(*T)
	if ok {
		return
	}

	fmt.Println(tt.a)
}

If ok is false then tt is nil, since that is the default value for pointers.

Playground: https://play.golang.org/p/0ni1nXpLBFK.

[timothy-king]

The original example one could plausible infer that ok is always false as the set of types of v is {*int}. Inferring the set of types of v is probably not a good road to go down. Here is a simpler example that illustrates what could be supported:

func foo(i interface{}) {
	tt, ok := i.(*int)
	if ok {
		return
	}
	_ = *tt
}

Handling just the control flow looks feasible for easy cases. It could use roughly the same trick as here:

				for _, d := range b.Dominees() {
					...
					if len(d.Preds) == 1 {
						if d == tsucc {

Brainstorming:
The checker would look for if c then else instructions where (tt, c) are the ssa.Extracts for indices 0 and 1 of an *ssa.TypeAssert instruction and the underlying type of tt is a pointer (/map/slice/interface) type. When the above trick is present on the else branch, one would strengthen the else branch with tt being nil. The true branch gains no information.

[ainar-g]

Yes, my example was both over- and undersimplified. In the original code the type set isn't as clear. Your example illustrates the problem better, thanks!

[bcmills]

Inferring the set of types of v is probably not a good road to go down.

But then the unreachable analyzer ought to flag the code in the example, right? So it seems win–win. 😉

Or, I suppose: something analogous to the nilness check could flag type-assertions that always succeed, in much the same way that the nilness analyzer flags nil-checks that always succeed (look for tautological condition in the test data).

[timothy-king]

@bcmills The foo example #47938 (comment) kinda suggests why this is hard. To claim the branch is unreachable/tautological one needs to know a superset of all types that can flow in i. We would not know all of the types when if the function is exported, e.g. func Foo(i interface{}) { ... }. And similarly if an interface value flows from outside of the package into thei argument we cannot conclude we know the superset. We can still infer the type sets in some cases, like if these were the only calls to foo:

func Bar(b bool) { foo(&b) }
func Baz(s string) { foo(&s) }

This is all doable but handling this requires some amount of inter-procedural dataflow analysis. Not really clear this complexity is worth it for nilness or unreachable yet. (This is a fascinating problem that I should put on my back burner though.) If folks used interface variables purely intra-procedurally (like the original example), we could do more here fairly easily. But I don't think they do often. This is all a really long winded version of "Inferring the set of types of v is probably not a good road to go down" 😉

[gopherbot]

Change https://go.dev/cl/560075 mentions this issue: go/analysis/passes/nilness: handle type assertions too