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

proposal: cmd/vet: Should check for assignments to inbuilt types #38832

Open
uhthomas opened this issue May 3, 2020 · 13 comments
Open

proposal: cmd/vet: Should check for assignments to inbuilt types #38832

uhthomas opened this issue May 3, 2020 · 13 comments
Labels
Milestone

Comments

@uhthomas
Copy link

@uhthomas uhthomas commented May 3, 2020

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

$ go version
go version go1.14.2 darwin/amd64

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="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/thomas/Library/Caches/go-build"
GOENV="/Users/thomas/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/thomas/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.14.2_1/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.14.2_1/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/dev/null"
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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/l5/501mrll12w33yh9vjvlvzfjc0000gn/T/go-build233345544=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I was doing some code review and noticed the snippet:

// ...

s, nil := a()
if err != nil {
    return err
}

// ... 

return nil

What did you expect to see?

go vet to give some warnings for assignments to inbuilt types such nil, true, false, etc...

What did you see instead?

No warnings, and hard to read code.

@gopherbot gopherbot added this to the Proposal milestone May 3, 2020
@gopherbot gopherbot added the Proposal label May 3, 2020
@uhthomas
Copy link
Author

@uhthomas uhthomas commented May 3, 2020

Just to give a more concise example of where things can really go wrong, here's another not-nice snippet:

package main

import (
	"errors"
	"fmt"
)

func a() (string, error) { return "", errors.New("some error") }

func do() (err error) {
	s, nil := a()
	if err != nil {
		return err
	}
	fmt.Println(s)
	return nil
}

func main() {
	fmt.Println(do())
}

// output: <nil>

https://play.golang.org/p/hyhCLA6XDPK

@ALTree
Copy link
Member

@ALTree ALTree commented May 3, 2020

Note that this has been proposed (and rejected) in the past: #22822.

@uhthomas
Copy link
Author

@uhthomas uhthomas commented May 3, 2020

That's a major shame - I feel this should be revisited because this isn't the first time I've seen it, and can have some very major consequences, especially to people new to the language who don't know the idiom is s, _ := as opposed to s, nil :=.

@mvdan
Copy link
Member

@mvdan mvdan commented May 4, 2020

Unfortunately, while the pieces of code you shared above are weird, they're correct Go. Shadoing is a somewhat common source of errors for new Go programmers, but we need shadowing to not have to come up with new variable names every few lines (imagine x, err1 := f1(); x, err2 := f2(); ...).

We fixed that earlier bug report by providing a better compiler error message (1e308fb), but that was fine because the program failed to compile anyway.

There are some linters out there that complain about shadowing, like https://godoc.org/golang.org/x/tools/go/analysis/passes/shadow, but they're not common nor enabled by default simply because the rate of false positives would be too large.

If you think a linter could be written to only catch this kind of mistake in a precise manner, writing a Go tool isn't all that hard. You could start with a copy of the shadow analyzer I showed above, and perhaps restrict it to builtin names.

@uhthomas
Copy link
Author

@uhthomas uhthomas commented May 5, 2020

Are there any real world use cases for shadowing built-in types? At that - idiomatic examples?

@mvdan
Copy link
Member

@mvdan mvdan commented May 5, 2020

Note that your example is shadowing a built-in variable, not a type. There is just one of those, and it's nil.

I don't say that shadowing it is common or recommended/idiomatic. I'm saying that it's valid Go, so vet should probably not flag it - it has strict requirements for checks, which include correctness (real bugs), frequency, and precision (no false positives).

@uhthomas
Copy link
Author

@uhthomas uhthomas commented May 5, 2020

Thanks for the clarification. I understand the notion, but I struggle to see any example where someone would ever mean to shadow nil. There are lots of examples of vet warning about valid Go, but that doesn't mean it's intentional.

@mvdan
Copy link
Member

@mvdan mvdan commented May 5, 2020

Indeed, "valid Go" can sometimes be a bit of a blurry line. Do we consider shadowing nil to be "incorrect execution"? I guess I'm leaning towards not, even when I struggle to imagine why anyone would ever do that.

There's also the frequency requirement, though. All vet checks are required to find common errors, so proposals for new checks should try to provide evidence that it's the case. I personally have never seen any code ever shadowing nil. Scanning a large corpus of Go code could be a good way to get some data.

@bcmills
Copy link
Member

@bcmills bcmills commented May 5, 2020

I could see someone writing that code if they mistook Go's multiple-assignment for functional-style pattern matching.

(That said, I doubt that it is common to mistake multiple assignment for functional-style pattern matching, since it doesn't work for Go data structures in general.)

@lolbinarycat
Copy link

@lolbinarycat lolbinarycat commented Aug 19, 2020

Alright, I was told to put this here: my proposal for doing a similar thing with builtin functions at the global scope:

Go allows you to "shadow" (define a function with the same name as) builtin functions. As I understand, this for backward compatibility, allowing new builtins to be defined without breaking existing functions. This makes sense.

However, shadowing builtin functions, especially at the global scope, is generally bad. Due to go's lack of methods on builtin types (and as of now, generics), it has a few builtin functions that are rarely used, and easily forgotten or never learned about by novice programmers (e.g. copy and delete). Because of this, someone could inadvertently redefine a builtin without knowing it. They could then realize they needed that builtin, and have to go back and change everywhere they used the function that was shadowing it to a new name.

Because of go vet's high requirements for accuracy (i.e. lack of false positives), This rule should probably only check global function declarations (this also helps with performance, requiring less ast traversal).

Is this more clear?

@bcmills
Copy link
Member

@bcmills bcmills commented Aug 19, 2020

Do we consider shadowing nil to be "incorrect execution"? I guess I'm leaning towards not, even when I struggle to imagine why anyone would ever do that.

On the other hand, assigning to nil very well could produce unintended behavior if the same scope subsequently includes a nil-check — especially if nil is bound to a variable that implements the error interface.

(https://play.golang.org/p/WLwW_gQLNdt)

@seebs
Copy link
Contributor

@seebs seebs commented Aug 19, 2020

I think that shadowing is responsible for a significant majority of "weird" bugs I encounter in Go code. Unfortunately, it's also sometimes intentional, and it's definitely allowed. It's possible that it should have been prohibited-or-restricted, but I don't know how to express a better intent, and completely eliminating it results in things like if foo, err3 := func(); err3 != nil, and I think that's not great.

@MatthewJamesBoyle
Copy link

@MatthewJamesBoyle MatthewJamesBoyle commented Aug 19, 2020

I agree with @uhthomas. I also find shadowing is responsible for a lot of bugs and this is a good proposal. Even if its just a warning, it could be helpful.

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
8 participants
You can’t perform that action at this time.