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

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

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

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.

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

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

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.

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.

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

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?

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)

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.

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.

FWIW it looks like there is already some groundwork for this with len.

package main

import "fmt"

func main() {
	fmt.Println(len) // ./prog.go:6:14: use of builtin len not in function call
}

https://go.dev/play/p/Z0-c_HJtkxw

@uhthomas That's not what this issue is about, though. You didn't reassign or shadow the builtin, so you're simply getting the usual compilation error for misusing a builtin. If you add a len := 1 line, the program compiles.

I suppose that's true. It's still perfectly valid to assign len to something else.

package main

import "fmt"

func main() {
	len := "abc"
	fmt.Println(len) // "abc"
}

https://go.dev/play/p/La5n7a5QTkQ

I think vet warning on shadowing constant identifiers (nil, true, and false) seems okay to me. It is hard to imagine this being done intentionally. Or leading to easier to understand code. (https://go.dev/play/p/eywKbpwkbfm). And they can lead to correctness issues. I can imagine using some builtins len or cap intentionally as a local variable. I personally think this is bad practice, but I understand it is potentially a good name. I think we are likely above the threshold for correctness and precision on constant identifiers.

The only vet requirement I am worried about is frequency.

A concern I have with targeting package level variables only is that we would (1) be on the wrong side of frequency as these will be even less common (2) be on the wrong side of precision specifically "a check that misses too many of the cases it's looking for will give a false sense of security".

Suppose somebody redefines print or len. It seems to me that in order for this to be misused, the new definition of print and len need to have the same type as the original builtins. Otherwise, one has a compilation error. In many cases that I have seen where the signatures match, the redefined builtins have a very similar semantics to the original functions.

There are examples that contradict my observations, but I am also concerned about their frequency.

I see not a single match for \bnil\b.*:= in k8s.io, which is a substantial repository. I'm very skeptical that the frequency of this problem justifies the cost of scanning for it.