cmd/vet: shadow false positive when using exported function

[tamird]

go tool vet --shadow returns a false positive when the variable being shadowed is declared from the return of an exported user function from another package. That's a mouthful, so here's some code:

./other/other.go:

package other

func Foo() (int, error) {
    return 0, nil
}

./shadow_test.go:

package shadow

import (
    "errors"
    "testing"

    "github.com/tamird/shadow/other"
)

func foo() (int, error) {
    return 0, nil
}

func TestVetShadow(t *testing.T) {
    // Local varibles: this passes.
    // a, err := "a", errors.New("Foo")

    // Function from same package: this passes.
    // a, err := foo()

    // Function from standard library: this passes.
    // r := &bytes.Buffer{}
    // a, err := r.Read(nil)

    // Function from different package: this triggers shadowing warning.
    a, err := other.Foo()

    if err != nil {

    }

    if _, err := other.Foo(); err != nil {
    }

    b, err := "b", errors.New("Foo")
    if err != nil {
    }

    _, _ = a, b
}

The comments in the code describe the problem. Use other.Foo() and you get a false positive; use any of the others (local function, local variables, exported method from the stdlib) and the warning goes away. Weird!

All the code is also available here: https://github.com/tamird/shadow

cc @mberhault

[josharian]

If you run go install github.com/tamird/shadow/other and then re-run vet, does the false positive still show up?

[tamird]

Yes.

[josharian]

Thanks.

/cc @robpike

[robpike]

The shadow code is marked experimental. It has too many false positives to be enabled by default, so this is not entirely unexpected, but don't expect a fix soon. The right way to detect shadowing without flow analysis is elusive.

[tamird]

@robpike flow analysis doesn't seem relevant here. @mberhault and I dug into this a bit and discovered that go/types (underlying vet) regards the return of other.Foo() to be of unknown type. We didn't get to the bottom of it, but there doesn't seem to be anything requiring more sophisticated tools in this example; this is an honest bug.