staticcheck: flag ambiguous evaluation order

[tamird]

See golang/go#23188.

In short, evaluating multiple expressions in a single statement works differently in cmd/compile than it does in gccgo. This can lead to subtle bugs in code such as:

package main

import "fmt"

type foo struct{ i int }

func (f *foo) inc() int { f.i++; return f.i }

func main() {
	var f1 foo
	f2, i := f1, f1.inc()
	fmt.Println(f1, f2, i)
}

https://play.golang.org/p/r6DksPtox1T

in cmd/compile, this prints {1} {1} 1; in gccgo, it prints {1} {0} 1. It would be amazingly helpful to have staticcheck flag these ambiguities.

[dominikh]

I'll have to give the spec another read for the details on undefined evaluation order, but in general this is a check we can (and probably should) implement.

[mdempsky]

@dominikh Just curious, how were you thinking of statically detecting this? It seems quite difficult to statically detect without false positives.

I was thinking of a compiler instrumentation pass that eagerly evaluates all side-effect-free subexpressions as early as possible, and saves those values. Later, when the values are computed normally (which is typically as late as possible), the values can be compared against the older values, and raise an error if they're different.

To reduce overhead, I think we can safely ignore expressions that 1) contain only constants and local variables whose address is never taken, and 2) don't contain any dereference operations (i.e., pointer indirection or slice/map indexing).

For example, if x is a local variable whose address is never taken, then x + 2 will always evaluate to the same thing whether it's evaluated eagerly or lazily.

This is imperfect because it's vulnerable to the ABA problem, but I expect it should handle other cases well.

Another possibility might be to extend the race/msan detector with "read locks". Then we could just mark the memory ranges as "locked" from the time they're first valid to evaluate until when they're actually loaded, and any writes that happen during this time are errors. This seems more complex, but could avoid the ABA problem.

[dominikh]

I haven't given this check much thought yet, but generally speaking I'd target a small subset of patterns for which it is straightforward to do the check. Mind you that we're not trying to prove the absence of bugs, only point out whatever bugs we are able to find.

For example, Tamir's example boils down to a value read and a function call that is known to modify said value, with unspecified order between the two. Unless I am missing something obvious, this should be free of false positives, at least as long as a true positive is defined as undefined behaviour, even if it's potentially harmless.

I wouldn't attempt implementing the check for more general cases or slices.

Your first idea for a dynamic check sounds interesting and I agree with the possible optimization. And if one sees the check as a debugging aid, as opposed to bug prevention, then the ABA problem isn't an issue, either.