Uncaught ineffectual assignment when using closure

[jpcoenen]

Hi there!

First of all, thanks for creating this awesome linter! It has caught many bugs in our code.

A short while ago, I ran into an issue where an ineffectual assignment was only caught during a code review because the linter did not catch it. So I was wondering whether this is a known false-negative case and whether it can be fixed.

The case has to do with the use of closures. I have recreated three scenarios as tests for testdata.go.

Cases where the ineffectual assignment is not detected:

func _() {
    x := 0
    func() {
        x = 0
    }()
    x = 0 //x
    x = 0
}

func _() {
    x := 0
    _ = func() {
        x = 0
    }
    x = 0 //x
    x = 0
}

func _() {
    x := 0
    x = 0 //x
    x = 0
    _ = func() {
        x = 0

        _ = x
    }
    _ = x
}

Case where it is correctly detected:

func _() {
    x := 0
    _ = func() {
        x := 0

        _ = x
    }
    x = 0 //x
    x = 0

    _ = x
}

As you can see, if a closure uses a variable, the linter then does not detect any ineffectual assignments for that variable. To be clear, it is not about whether the assignment was ineffectual because of what happens within the closure (I can imagine that that is tricky if not impossible to detect). In all of the three missed cases, the ineffectual assignment is directly followed by another assignment.

I'd love to hear if there is anything that can be done about this.

[gordonklaus]

Hi!

Thanks for your kind words :)

I don't think there's a lot of room for improvement here, unfortunately. In the absence of synchronization the Go memory model makes almost no guarantees about what writes may or may not be observed by a read in another goroutine. Your examples don't actually spawn goroutines, but generally speaking it's a safe bet that a func literal will end up running in its own goroutine, and determining whether or not it does would require analysis that is beyond the scope of this tool (a pointer analysis can be quite expensive, as I understand it).

In any case, I was able to make a fix for your third example, in which the ineffectual assignment precedes the func literal.

Let me know if you have any other ideas.

[jpcoenen]

Hi!

Wow, that's a quick turn-around! Good to hear one case has been fixed.

Regarding the other cases: I was already kind of afraid that this would be the case. However, I am still not 100% it is. One important thing to note is that the closures only assign to x, they do not read it. So wouldn't it be possible to determine that x is not read anywhere and all assignments, therefore, must be ineffectual?

[gordonklaus]

That's a good idea. Good enough, in fact, that the compiler already treats it as an error if a variable is never used!