If this mistake is already a compile error, wouldn't a vet warning be a bit redundant?

A warning about assignment to the len variable

Also we can't flag these indiscriminately, you can write valid code that does that. Vet checks need to satisfy a 'precision' requirement. I'm not sure how easy it'll be to avoid false positives here.

@mvdan: it's only a compile error if len etc. are subsequently used in their idiomatic forms as documented. Otherwise, no warning will be issued.

@ALTree: I thought the point of go vet was to check for syntatically valid but unwise/error-prone code. Am I mistaken? It seems like the precision requirement can be met by simply comparing against all the functions documented at https://golang.org/pkg/builtin/.

These built-in functions are identifiers, not keywords, for good reason. One of the reasons is to allow adding new built-ins in the future without breaking existing code. The other is to allow users to shadow them at their discretion. In neither case should vet warn about perfectly fine code.

it's only a compile error if len etc. are subsequently used in their idiomatic forms as documented. Otherwise, no warning will be issued.

If they're being used incorrectly, it will cause a compile time error. If they are being used correctly, the user intended to use them that way. Neither case warrants a vet check IMHO.

Hi @dominikh - Thanks for the feedback. The reason why I filed this issue in the first place was because I came across this situation myself via a compilation error and it took me some not-insignificant-amount of time to figure out what the root cause was.

I suspect that others have encountered this problem at times, so I'm not sure that the user always intends to "shadow" len and other built-in functions. My suspicion is that while -- as you claim -- it is sometimes intended, equally or more often, it is just a mistake. (I'm not sure how to practically prove this one way or another.)

There's a maintainability argument to be made, too. If a user shadows len without good reason today, a future maintainer may attempt to call the len builtin, possibly months later after the original maintainer has left, possibly many lines down (more than a screenful) in the same scope, and struggle (as I did) to figure out why the compiler was throwing an error.

The other is to allow users to shadow them at their discretion.

First, I'm not sure why anyone would intentionally shadow a function with a variable. Shadowing it with a substitute function, perhaps. Is this common practice among Go programs?

Second, was this actually intended? Is there any documentation or history behind this?

@otterley shadowing builtin functions isn't particularly encouraged, but it can be useful sometimes. Similarly, it is sometimes useful to name a function parameter url, clashing with the net/url package name.

Perhaps the right solution here would be to improve the compile error messages, by adding something like did you mean to shadow the builtin 'len' function?. That is usually the right answer to "it took me five minutes to figure out the problem behind a compile error".

@mvdan Augmenting the error message in the compiler would be a fine substitute!

I have rewritten the issue title. I don't think a full-blown proposal is necessary for this, but I would like some input from others like @griesemer and @mdempsky.

I have to say that in my early days of Go development I did shadow builtin names like len, new, and copy sometimes. Now it's in my muscle memory not to do so, but these hints would have certainly helped me back then.

I'm open to expanding the compiler error message to include extra information if folks want to suggest something concrete (e.g., which existing error message to change, how to change it, and when exactly we should print something else).

The most likely candidate in my mind is this error message:

			yyerror("cannot call non-function %v (type %v)", l, t)

@mdempsky that's the error that the example in the original post shows, and I agree that it would be the most useful as a start. More can be added later.

The message for this error would be changed when the identifier name matches a builtin function. How exactly to change the message can be fine-tuned later, but I was thinking something along the lines of what I mentioned before - did you mean to shadow the builtin 'len' function?.

I'm not familiar with the norm and style for these error hints, so pointers are welcome.

I'll give this a look in the 1.11 window.

We don't have any other "did you mean X?" error messages currently. That feels non-idiomatic to me.

I'm thinking maybe something like "builtin len shadowed at foo.go:42".

I thought I remembered similar hints, such as in the if a == T{} { ... } case - but cannot find any examples now.

I'm thinking maybe something like "builtin len shadowed at foo.go:42".

You mean a second, separate error, or as an addition to the original error? The position info seems useful in any case, hadn't though about that.

As @mdempsky already said, the compiler doesn't provide hints in error messages, at least not in the suggested form. We do have hints for misspelled identifiers (e.g. https://play.golang.org/p/8PJCltlBau ).

Any improvement here should be in the error reporting and in the same vain. I think reporting the location of the len declaration (in this case) would be sufficient. For instance:

cannot call non-function len (type int), declared at main.go:9

But even this is a slippery slope. Are we going to do this everywhere we refer to an object by name? Why not?

Maybe it's good enough to do it for shadowed built-ins.

You mean a second, separate error, or as an addition to the original error?

Shadowing a builtin is not itself an error, so it would have to be an addition to the original. We occasionally print multiline errors when there's additional information to include relevant to an error.

No, not an additional error. Simply making the existing error clearer by adding position information for the object in question so it's clear where it's defined.

For instance, if len is locally declared, because one might think it's a built-in, in that case the error message should also say where it's declared (which will make it clear that it's not the built-in).

Change https://golang.org/cl/97455 mentions this issue: cmd/compile: improved error message when calling a shadowed builtin

@griesemer do you think we should mirror this better error message in go/types? I'm happy to submit a CL for it too.

I also wonder if this should be extended to other scenarios. For example, shadowing a type can result in failed type conversions and declarations:

$ cat f1.go
package main
func main() {
        int := 3
        println(int(3))
}
$ go run f1.go
# command-line-arguments
./f.go:4:13: cannot call non-function int (type int)

$ cat f2.go
package main
func main() {
        int := 3
        var i int
}
$ go run f2.go
# command-line-arguments
./f.go:4:6: int is not a type

The first case could be caught by having my CL cover type builtins too, not just the funcs. The second would require another separate fix.

@mvdan Let's leave it as is for now. Also, go/types already reports not just the type but also what it is (variable, constant, etc.) which makes it clearer.