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/passes/nilness: false negative with pointers and type assertions #47938

Closed
ainar-g opened this issue Aug 24, 2021 · 5 comments
Assignees
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@ainar-g
Copy link
Contributor

ainar-g commented Aug 24, 2021

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.

@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Aug 24, 2021
@gopherbot gopherbot added this to the Unreleased milestone Aug 24, 2021
@toothrot toothrot added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Aug 24, 2021
@timothy-king timothy-king added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Aug 24, 2021
@timothy-king
Copy link
Contributor

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

ainar-g commented Aug 25, 2021

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

bcmills commented Aug 25, 2021

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

timothy-king commented Aug 25, 2021

@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" 😉

@adonovan adonovan self-assigned this Jan 31, 2024
@gopherbot
Copy link

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

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) NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

6 participants