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

cmd/compile: add hint to type errors when builtin identifiers are shadowed #22822

Closed
otterley opened this issue Nov 20, 2017 · 18 comments

Comments

Projects
None yet
7 participants
@otterley
Copy link

commented Nov 20, 2017

Please answer these questions before submitting your issue. Thanks!

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

1.9.2

Does this issue reproduce with the latest release?

Yes

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

darwin/amd64

What did you do?

The following code containing an assignment to len fails to compile, but go vet doesn't warn about why: https://play.golang.org/p/ap8LOPyn6M

package main

import (
	"fmt"
)

func main() {
	arr := []int{0, 1}
	len := 5
	fmt.Println(fmt.Sprintf("What is life? %d", len(arr)))
}

What did you expect to see?

A warning about assignment to the len variable. (Go also allows the user to assign to other built-in-but-not-reserved names, such as cap and append.)

What did you see instead?

No warnings.

@mvdan

This comment has been minimized.

Copy link
Member

commented Nov 20, 2017

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

@ALTree

This comment has been minimized.

Copy link
Member

commented Nov 20, 2017

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.

@otterley

This comment has been minimized.

Copy link
Author

commented Nov 20, 2017

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

@dominikh

This comment has been minimized.

Copy link
Member

commented Nov 21, 2017

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.

@otterley

This comment has been minimized.

Copy link
Author

commented Nov 21, 2017

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?

@mvdan

This comment has been minimized.

Copy link
Member

commented Nov 21, 2017

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

@otterley

This comment has been minimized.

Copy link
Author

commented Nov 21, 2017

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

@mvdan mvdan changed the title cmd/vet: Add check for assignment to built-in function names cmd/compile: add hint to type errors when builtin identifiers are shadowed Nov 21, 2017

@mvdan

This comment has been minimized.

Copy link
Member

commented Nov 21, 2017

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.

@mdempsky

This comment has been minimized.

Copy link
Member

commented Nov 21, 2017

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)
@mvdan

This comment has been minimized.

Copy link
Member

commented Nov 21, 2017

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

@mvdan mvdan self-assigned this Nov 21, 2017

@mvdan mvdan added this to the Go1.11 milestone Nov 21, 2017

@mdempsky

This comment has been minimized.

Copy link
Member

commented Nov 21, 2017

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

@mvdan

This comment has been minimized.

Copy link
Member

commented Nov 21, 2017

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.

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Nov 21, 2017

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.

@mdempsky

This comment has been minimized.

Copy link
Member

commented Nov 22, 2017

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.

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Nov 22, 2017

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

@gopherbot

This comment has been minimized.

Copy link

commented Feb 27, 2018

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

@mvdan

This comment has been minimized.

Copy link
Member

commented Feb 27, 2018

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

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2018

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

@gopherbot gopherbot closed this in 1e308fb Feb 28, 2018

@golang golang locked and limited conversation to collaborators Feb 28, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.