Don't flag self–assignments with side effects

[ksurent]

Fix SA4018 to not flag self–assignments of the form x[e] = x[e] where e may have side effects (e.g. a function call we know isn't pure or a channel read).

This also includes a change that unties the implementation of the purity fact from SA4017, making it available for reuse in SA4018 and potentially other checks in the future.

[dominikh]

How about a pure function taking an argument that is impure or has side effects? Recursively? Also what about methods?

[ksurent]

Good catch regarding pure functions taking impure arguments. Addressed in 1a9134f.

Can you clarify what you mean about methods? Currently we just conservatively assume that all method calls may have side effects and the purity fact for methods is always false.

[ksurent]

So I was thinking about this recently and I realised that this PR confuses terminology quite a bit and does not do quite what it's meant to do.

To elaborate, the original issue says that we shouldn't flag assignments where indexing has side effects. What I think we really want is to stop flagging cases where indexing is non–deterministic.
And just to be clear, this is what I'm referring to:

// Pure and deterministic.
func foo(x int) int { return x + 42 }

// Impure but still deterministic.
func bar(x int) int {
    println(x)
    return x + 42
}

I've built everything around the purity fact, and since pure functions are a subset of deterministic, this will only cover a subset of what we want.
Detecting determinism seems much harder than purity. So perhaps this is still a useful improvement, but probably warrants a clarifying note.

Thoughts?

[dominikh]

Purity is the correct attribute to check for, for two reasons:

• we really do care about purity. If the function has side-effects, we can't really suggest removing the assignment, not unless we also point out its side-effects and how to preserve them.
• purity is our approximation to what you refer to as deterministic. It's the subset of Go for which we can know that identical arguments to a function result in identical outputs.

On a side-note, your definition of "deterministic" is more restricted than the general definition. Something like a PRNG (math/rand) is also deterministic, it's just that its inputs include state other than function arguments, namely the values stored in struct fields.

[dominikh]

@ksurent I propose the following solution instead: https://github.com/dominikh/go-tools/compare/impure-self-assignment?expand=1

Of note: it correctly handles more complex and nested syntax, such as foo(x)[0] = foo(x)[0] by not flagging it if foo is impure. I've also moved the handling of stubs back into the purity calculation itself. We want the same handling of stubs in this check, that is, x[stub()] = x[stub()] shouldn't get flagged.

[dominikh]

Merged as 9774f3b.