x/tools/go/analysis/passes/shadow/cmd/shadow: fixing shadowed errs results in code more likely to cause errors

leighmcculloch commented 4 years ago

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

$ go version
Go 1.14.4

Does this issue reproduce with the latest release?

Yes

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

go env Output

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN="/home/leighmcculloch/local/bin"
GOCACHE="/home/leighmcculloch/.cache/go-build"
GOENV="/home/leighmcculloch/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/leighmcculloch/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/leighmcculloch/local/bin/go/1.14.4"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/leighmcculloch/local/bin/go/1.14.4/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build157507746=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Fixed code according to the shadow analyzer, I renamed a variable from err to be distinguishable from another variable also named err that was in a higher scope.

What did you expect to see?

I expected using unique names for variables in nested scopes to be a best practice since the shadow analyzer recommends it.

What did you see instead?

I'm experiencing that renaming err variables in code is more likely to create more human errors than to use shadowed errvariables.

In Go code my colleagues and I have seen we have definitely found value in renaming shadowed variables, it's actually caught some bugs for us on a couple occassions, but when it comes to errors the opposite has been true. On one occasion the error lived on for a while in production code. Code had been written, a shadow was detected, a variable was renamed, the scope was removed and then this was the bug and fix when we discovered the bug that had been created by the attempt to not shadow: https://github.com/stellar/go/pull/2469. I've caught myself on another occasion making the same mistake. This might just be my weakness but I suspect others might make the same mistake as me.

This issue isn't about that specific issue but rather treating a shadowed err can easily create buggy code because developers may be accustomed to seeing the error named err, making it really easy to forget that in this one rare scope the error is named dbErr and to do things like return the error from the wrong scope.

Proposal

Make err of type error an exception to the shadow checker. The shadow checker already makes several exceptions on the basis of those patterns being common and idiomatic. This is another one of those patterns.

cc @stamblerre

dmitshur commented 4 years ago

/cc @matloob per owners.

matloob commented 4 years ago

I think this is reasonable, though I don't plan to work on the shadow analyzer myself.

It's important to note that the shadow analyzer is experimental so its recommendations should not be considered best practices.

leighmcculloch commented 4 years ago

I'd be open to contributing a change to exclude error values. Would a contribution doing this be welcome?

matloob commented 4 years ago

I thought about this some more and I think we'd want to do something a bit more sophisticated: the example in Shadow's doc string uses err as an example of a shadowed variable... so I think it was intended for that variable to be flagged. I'm not sure exactly what a more sophisticated approach would look like...

golang / go