Ineff. assignment to local variable not detected in presence of defer or *sync.Mutex

[ainar-g]

UPD: It seems that the thing that actually prevents ineffassign from detecting the error is the defer:

package main

import (
    "errors"
    "fmt"
)

func main() {
    fmt.Println(f())
}

func f() (n int, err error) {
    n, err = g()
    if err != nil {
        err = fmt.Errorf("g: %w", err)
    }

    defer h()

    return n, nil
}

func g() (n int, err error) { return 0, errors.New("test g") }

func h() {}

Removing the defer or moving it to the top of f lets ineffassign detect the issue again.

---

package main

import (
    "errors"
    "fmt"
    "sync"
)

func main() {
    fmt.Println(f(&sync.Mutex{}))
}

func f(mu *sync.Mutex) (n int, err error) {
    n, err = g()
    if err != nil {
        err = fmt.Errorf("g: %w", err)
    }

    mu.Lock()
    defer mu.Unlock()

    return n, nil
}

func g() (n int, err error) { return 0, errors.New("test g") }

Removing the mutex un/locking makes ineffassign detect the ineffectual assignment to err. I've read #81, and there it has been said that:

Any variable accessed in another goroutine is thereafter ignored by the tool.

But there are no accesses of err from other goroutines. Does the tool simply ignore any code with mutexes and other concurrency primitives in it? If so, can this be improved, or is it not on the agenda?

[gordonklaus]

It has to do with defer, yes — and also named results and the possibility of panic.

In a function that has named results, at a point where it might possibly panic, if a function has been deferred then it might possibly access one of the named results, thus making any prior assignment to that result effectual.

In your function, the tool recognized that mu.Unlock() might panic, and since it saw the defer keyword before, it marks all named results as used at that point.

However, there is a subtle error in this analysis: mu.Unlock() might panic, but that actually happens before it is deferred. Even more subtly, it is only the selector expression mu.Unlock that might panic before the defer — the call doesn't happen until the containing function returns (hence your updated function giving the same result).

I'll submit a patch that should fix the analysis of your examples.

[ainar-g]

@gordonklaus, thanks for the quick fix! I've rechecked it on the simplified examples and the original code, and while both simplified examples are now correctly detected, the original still isn't. Here is a less-simplified code that is closer to the original:

package main

import (
    "errors"
    "fmt"
    "sync"
)

func main() {
    v := &value{
        mu: &sync.Mutex{},
        n:  1,
    }

    fmt.Println(v.update())
}

type value struct {
    mu *sync.Mutex
    n  int
}

func (v *value) update() (n int, err error) {
    n, err = g()
    if err != nil {
        n = 0
        err = fmt.Errorf("g: %w", err)
    }

    v.mu.Lock()
    defer v.mu.Unlock()

    v.n = n

    return n, nil
}

func g() (n int, err error) { return 0, errors.New("test g") }

I assume that this doesn't work because the analysis considers v.n = n to be possibly panicking when v == nil even if v.mu above not panicking proves that it wouldn't?

[gordonklaus]

@ainar-g That's correct.

[ainar-g]

I see, thanks.