cmd/vet: obvious shadowing is not detected

[onokonem]

What version of Go are you using (go version)?

go version go1.9 darwin/amd64

Does this issue reproduce with the latest release?

Yes it does

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/test//go"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/rm/sngdtnn11x3b_zp_zzyw6jl40000gn/T/go-build303980560=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"

What did you do?

I run go vet --shadow for the following code

func main() {
	strList := make([]string, 0)

	for _, v := range strList {
		v := []byte(v)
		_ = v
	}
}

What did you expect to see?

Shadowing warning

What did you see instead?

No warnings

[cznic]

v := v is technically also shadowing, but it's also a useful idiom inside a loop. I think that's the reason vet doesn't complian about this case.

[onokonem]

it's also a useful idiom inside a loop

please be more specific :)

what this idiom is useful for?

[dominikh]

Making the variable local to the loop body, primarily for closures:

for _, v := range s {
  v := v
  go func() {
    fn(v)
  }()
}

[AlekSi]

That is mentioned in the last part of https://golang.org/doc/faq#closures_and_goroutines.

[onokonem]

Making the variable local to the loop body, primarily for closures:

is it any better than

for _, v := range s {
  localV := v
  go func() {
    fn(localV)
  }
}

[AlekSi]

You can accidentally use v inside closure that way.

[onokonem]

fair enough.

so vet should check the types I guess. same type ok, not the same type should cause a warning...

what do you think?

[quasilyte]

so vet should check the types I guess. same type ok, not the same type should cause a warning...
what do you think?

When you iterate over some interface types that represent ADT-like values (see example below), you may want to introduce type-asserted version of it inside inner scope:

	// A.
	for _, decl := range f.Decls {
		decl, ok := decl.(*ast.FuncDecl)
		if !ok {
			continue
		}
		// Use func decl.
	}

	// B: makes code more nested. Also does shadowing.
	for _, decl := range f.Decls {
		if decl, ok := decl.(*ast.FuncDecl); ok {
			// Use func decl.
		}
	}

At least for me, it feels consistent with type switch idiom where you do assignment to the variable of the same name:

switch n := n.(type) {
case A:
	// n is of type A.
case B:
	// n is of type B
}

[as]

@AlekSi how about this one?

for _, v := range s {
  v = v
  go func() {
    fn(v)
  }()
}

[rsc]

cmd/vet/README lists the requirements for a vet check.
This proposal seems to me to fail "Correctness" and "Precision".

There is plenty of code that shadows variables explicitly to make a local copy, and reuses the same name to ensure that it is impossible to refer to the outer "wrong" one anymore. (I'm just repeating the conversation above, with @dominikh and @onokonem).

Rejecting "obvious" shadowing like this will generate far too many false positives, which makes people not trust vet output (even output from other checks).