x/tools/go/analysis/passes/nilness: spurious "impossible/tautological condition" with cond := a != nil; if cond {...}; if cond { ... }

[hut8]

Go version

go version go1.23.0 linux/amd64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/liam/.cache/go-build'
GOENV='/home/liam/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/liam/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/liam/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.23.0'
GODEBUG=''
GOTELEMETRY='local'
GOTELEMETRYDIR='/home/liam/.config/go/telemetry'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/home/liam/nilness-bug/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build1057087488=/tmp/go-build -gno-record-gcc-switches'

What did you do?

I believe there is a bug in the nilness checker that incorrectly reports an impossible "nil != nil" and also omits an error in a different location about a tautological comparison. This code demonstrates it:

package main

import "fmt"

func TriggerBug(a *struct{}) {
	hasA := a != nil // "impossible condition: nil != nil"
	if hasA {
		fmt.Println("A is not nil")
		return
	}
	if !hasA {
		fmt.Println("A is nil")
	}
}

I checked here: https://github.com/golang/go/issues?page=1&q=is%3Aissue+is%3Aopen+nilness and I can't find anything that matches.

What did you see happen?

	hasA := a != nil // "impossible condition: nil != nil"

Here is a screenshot for clarity:

What did you expect to see?

The above error implies that a is always nil and therefore a != nil will never happen. That is not true. However, there still is a problem with the code. Because of the return, then the if !hasA is tautological -- although, that's a different error, and it's not reported, and the error that is reported is not only erroneous but also in the wrong place.

This, for example, correctly reports no errors:

func TriggerBug(a *struct{}) {
	hasA := a != nil
	if hasA {
		fmt.Println("A is not nil")
		return
	}

	fmt.Println("A is nil")
}

So I would expect to see something like this:

func TriggerBug(a *struct{}) {
	hasA := a != nil
	if hasA {
		fmt.Println("A is not nil")
		return
	}
	// at this point, hasA is definitely false
	if !hasA { //  "tautological condition: nil == nil"
		fmt.Println("A is nil")
	}
}

Interestingly, if I just get rid of hasA and put the expression directly in the if statement, I get:

func TriggerBug(a *struct{}) {
	if a != nil {
		fmt.Println("A is not nil")
		return
	}
	if a == nil { // tautological condition: nil == nil
		fmt.Println("A is nil")
	}
}

That's what I would expect to see whether or not I have the boolean value broken out into a different variable.

[gabyhelp]

Related Issues

• x/tools/go/analysis/passes/nilness: bogus "impossible condition: non-nil == nil" that is actually possible #66835 (closed)
• x/tools/go/analysis/passes/nilness: not report slice as non-nil #45177 (closed)
• affected/package: go1.20.1 When obtaining a "nil" value after defining an interface, it is not recognized as an empty value during condition checks. #59521 (closed)
• conflict behavior when comparing nil and (*SturctName)(nil) #35413 (closed)
• nil literal should not be parsed as ast.Ident #28918 (closed)
• builtin/error: late assignment of custom error type cause nil checking of the error variable not working #53768 (closed)
• `err != nil` when `err == nil` with custom `error` type #35292 (closed)
• The result of a nil pointer type not equal nil is true, is that right? #44102 (closed)
• crypto/x509: The if's condition checks that err is not nil but returns nil #49135 (closed)
• proposal: cmd/vet: do not warn about nil comparisons after for loop #28871 (closed)

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

[IlyasYOY]

Hello! I found your issue interesting.

I tried to debug the cases you provided and found this. Code assumes that left operand is nil in case of nil right operand.

Here is the commit: IlyasYOY/tools@5c4cf21 (you might want to experiment with it)

TriggerTautological stills fails, but no false positive.

---

I'd be glad to dive deeper and find the solution. If there are any hints I'd really appreciate it .

[adonovan]

Thanks for the very thorough bug report! The problem is that the nilness analyzer is not taking care to distinguish whether a condition appears directly within an if condition, or via an intervening variable. In the original test case:

func TriggerBug(a *struct{}) {
	hasA := a != nil
	if hasA {
		fmt.Println("A is not nil")
		return
	}
	if !hasA {
		fmt.Println("A is nil")
	}
}

the analyzer detects that the first if hasA, which is really of the form if a != nil in SSA, is fine; but the second if hasA, which of course has the same form if a != nil in SSA, is dominated by code that establishes that a is nil, so the analyzer reports a problem about this condition. But there is syntactically only one condition.

The analyzer will need to detect whether the condition has multiple referrers; if so, it will need to report that the condition is trivially true/false only for some of them, or, report the error at the if statement itself, if this information is available.

[adonovan]

Unfortunately ssa.If does not record the position of the ast.IfStmt. (In fact its Pos method promises to return NoPos, a youthful error: never promise to return empty information since you can't change your mind later.)

Some options:

1. Change buildssa to use Debug ssa.Mode, allowing us to use variables to infer the structure. But this is expensive.
2. Detect that the condition has multiple Referrers and suppress the diagnostic in that case. But that's sad, because it is real diagnostic reported in the wrong place.
3. Same, but instead of suppressing the error, qualify it with something like "(may apply to only some uses of the variable assigned by condition)". But it still lacks position information.
4. Change the stack of dominating facts to record not just the fact but the SSA value that it came from. Then if we notice that an SSA value is its own witness (as in the case of hasA above), we report a different error (something like "the variable assigned to this condition is checked after it has already been established/falsified"). But again it still lacks the crucial position of the second reference.
5. Find some weaselly way to make ssa.If.Pos report the position of the IfStmt. But conditional branches created by the SSA builder are pretty far abstracted from the source, so there isn't always an obvious way to express the position.

Option 5 is really the only approach that gives us the position of the second If statement, so it's my favorite. But it's a project.

[timothy-king]

Long term I think recording the ast.Node unconditionally is likely better for ssa. That extra word per Instruction buys a lot.

But I agree option 5 seems like the right short term solution for this. Changing that comment is a waste of the proposal committee's time IMO, but I'll leave that up to you.

[adonovan]

Long term I think recording the ast.Node unconditionally is likely better for ssa. That extra word per Instruction buys a lot.

Two words! And it's far from clear what the correct Node is for each Instr once the syntax is all boiled down.

[timothy-king]

You get to reclaim the token.Pos so the delta should be +1 word per Instruction (pre-padding/alignment considerations).

And it's far from clear what the correct Node is for each Instr once the syntax is all boiled down.

Good point. This is much easier if you only need to be consistent within a handful of repos, but ssa does aspire to more consistency than this.

[adonovan]

You get to reclaim the token.Pos so the delta should be +1 word per Instruction (pre-padding/alignment considerations).

The Pos is not a field, it's code:

func (s *If) Pos() token.Pos        { return token.NoPos }

And for the instructions that do have a Pos field, they would still need to keep it because the hypothetical ast.Node field would be potentially nil (or too diverse a range of things to reliably derive pos from it).

[lublak]

I dont know if my issue matches, but i have the ?same? issue with a nil pointer:

type Infos struct {
	m     map[string]string
}

// ....

var infos *Infos

for k, v := range data {
  if v.valid {
    if infos == nil { // tautological condition: nil == nil
      infos = &Infos{}
    }
  }
}

[adonovan]

type Infos struct {
	m     map[string]string
}

// ....

var infos *Infos

for k, v := range data {
  if v.valid {
    if data == nil { // tautological condition: nil == nil
      data = &Infos{}
    }
  }
}

This code isn't valid: data cannot be both the operand of a range statement and a pointer to a struct. What is its type?

Can you provide a working example that reproduces the problem?