That is a really obscure bug and really shows how nasty go variable shadowing can be - I thought go vet had a check for this kind of thing though?

Sadly not by default:

https://golang.org/cmd/vet/#hdr-Shadowed_variables

$ go vet -help
usage: vet [-n] [-x] [build flags] [packages]

Vet runs the Go vet command on the packages named by the import paths.

For more about vet, see 'go doc cmd/vet'.
For more about specifying packages, see 'go help packages'.

To run the vet tool with specific options, run 'go tool vet'.

The -n flag prints commands that would be executed.
The -x flag prints commands as they are executed.

For more about build flags, see 'go help build'.

See also: go fmt, go fix.

You need to do something like:

go list -f '{{.Dir}}' ./... | xargs go tool vet -shadow

It might be worth running this in our CI script, along with the go fmt check: https://github.com/gopherjs/gopherjs/blob/master/circle.yml#L17

@shurcooL great find!

I've just tried this out. For a particular staleness/caching problem I'm seeing this fix unfortunately doesn't help... will continue to investigate.

@flimzy We already run go tool vet on all GopherJS packages, just without -shadow. Sadly, -shadow gives many false positives like:

build/build.go:110: declaration of "err" shadows declaration at build/build.go:77
build/build.go:535: declaration of "err" shadows declaration at build/build.go:506
build/build.go:546: declaration of "err" shadows declaration at build/build.go:506
...

It did also spot the pkg shadow (when run on previous commit). Maybe if we filter out instances of "err" being shadowed, it'd be viable to include it?

I've just tried this out. For a particular staleness/caching problem I'm seeing this fix unfortunately doesn't help... will continue to investigate.

@myitcv If you can make a minimal reproduce sample and report that, I'd be happy to investigate. I didn't expect this fix to necessarily resolve all problems, this was just the first step before others.

@shurcooL Are those really false positives? I know I've seen bugs (in code @ work) where err being shadowed was the culprit. My personal preference is to avoid all shadowed variables, even when they seem innocuous (as most shadowed errs do).

I've never made it a goal for myself to not shadow err variables, so it'd be a false positive in most code I write. For me, if a variable is named err, its value must be consumed on next immediate line, or the one after (to allow inserting foo.Close()), and after that err becomes available for reuse only.

If I need to use a value of err in later code, I always call it somethingError instead. See https://dmitri.shuralyov.com/idiomatic-go#error-variable-naming.

So, I don't know whether they're false positives or not in this codebase, but I expect them to be if other people follow similar practice (and as far as I know, most people do).

For me, if a variable is named err, its value must be consumed on next immediate line

I agree with this, and follow the same rule. But of course the compiler doesn't know or enforce this rule. And I've seen it violated (almost certainly by accident, due to copy-paste errors, or similar, leaving both unchecked, or double-checked err values). I don't really expect this sort of problem in a codebase like this one, though, where good coding practices, including peer review, are routine.