Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
leighmcculloch opened this issue Jul 8, 2020 · 4 comments

Comments

@leighmcculloch
Copy link
Contributor

@leighmcculloch leighmcculloch commented Jul 8, 2020

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: stellar/go#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

@gopherbot gopherbot added the Tools label Jul 8, 2020
@gopherbot gopherbot added this to the Unreleased milestone Jul 8, 2020
@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented Jul 11, 2020

/cc @matloob per owners.

@matloob
Copy link
Contributor

@matloob matloob commented Jul 13, 2020

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
Copy link
Contributor Author

@leighmcculloch leighmcculloch commented Jul 13, 2020

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

@matloob
Copy link
Contributor

@matloob matloob commented Jul 24, 2020

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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants