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: add vet check for shadowing builtins. #40899

Closed
lolbinarycat opened this issue Aug 19, 2020 · 6 comments
Closed

proposal: cmd/vet: add vet check for shadowing builtins. #40899

lolbinarycat opened this issue Aug 19, 2020 · 6 comments
Labels
Milestone

Comments

@lolbinarycat
Copy link

@lolbinarycat lolbinarycat commented Aug 19, 2020

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

@gopherbot gopherbot added this to the Proposal milestone Aug 19, 2020
@gopherbot gopherbot added the Proposal label Aug 19, 2020
@ALTree
Copy link
Member

@ALTree ALTree commented Aug 19, 2020

Hi,

this is pretty much #38832 (proposal: cmd/vet: Should check for assignments to inbuilt types).

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 would make the check pretty much useless, IMO. I'd bet that this kind accidental shadowing happens mostly on variable assignments inside functions, e.g. someone writes len := .... If vet only checks function declarations, it'll miss that; and if it checks everything, it'll flag too much correct code (failing the correctness requirement). Overall, it seems pretty hard to do this right.

Closing here in favour of the older thread at #38832 (feel free to post your idea about a reduced check only on function signatures there, if you wish, so we can keep all the discussion in one place).

@ALTree ALTree closed this Aug 19, 2020
@lolbinarycat
Copy link
Author

@lolbinarycat lolbinarycat commented Aug 19, 2020

@ALTree
It is related, certainly, but I feel this is definitely not the same, as both proposed checks would flag different code, of which the overlap is close to nothing. Also, whenever I've seen someone bring up their own idea in a proposal thread, they have been directed to create their own issue. Furthermore, by replying to my proposal here, you have already failed the goal of "keeping all the discussion in one place". Even if you hadn't, that issue was a repeat of a previous proposal, so it wasn't all in the same place even before I made this issue.

If there's some reason I don't know of that justifies this, I'd love to hear it, but I can't really think of anything that would.

@randall77
Copy link
Contributor

@randall77 randall77 commented Aug 19, 2020

We try to keep all discussion in one place. That doesn't mean we succeed in that attempt. It's especially hard because someone can start a new discussion about the same topic at any time. And when they are told that they should move the discussion to a place where it is already happening, they often don't.

Deciding whether something is the same issue or a different issue is a difficult problem. We'll get it wrong sometimes. Often there isn't enough information to be sure, so we recommend people open new issues with more information. Only after more information is forthcoming will we realize for sure that it is in fact a duplicate or close enough that discussion should move to a previous issue.

@lolbinarycat
Copy link
Author

@lolbinarycat lolbinarycat commented Aug 19, 2020

@randall77 I see how it is a difficult problem, but what you are asking me to do won't help. How am I supposed to reply to the points @ALTree made? am I supposed to go into this other thread, and start talking about a comment on a different issue, do I have to repost my proposal as a comment? Also, as far as I can tell, nothing about this is mentioned in CONTRIBUTING.md or in the proposal guide. Honestly, trying to keep discussion in the same place seems like a mistake, because:

  • github issues don't have threading like reddit, meaning highly active issues can be confusing to read through.
  • the issues search makes it hard to find issues related to a topic (I did a search before this and found nothing)
  • issues can easily be linked to from other issues, and are all the time.
  • new issues are easier to find, and thus will get more discussion.
@randall77
Copy link
Contributor

@randall77 randall77 commented Aug 19, 2020

am I supposed to go into this other thread, and start talking about a comment on a different issue, do I have to repost my proposal as a comment?

Yes. Repost your proposal as a comment. Maybe "hey, maybe we just do this at global scope instead of in function bodies. ..."

github issues don't have threading like reddit, meaning highly active issues can be confusing to read through.

I agree. But don't think #38832 is highly active. It has 10 comments currently.
This is a larger problem than this issue. We've been experimenting with using reddit for large discussions, see https://old.reddit.com/r/golang/comments/hitexe/qa_gobuild_draft_design/ as an example.

the issues search makes it hard to find issues related to a topic (I did a search before this and found nothing)

I also agree. @ALTree did the search for you and found the issue, then pointed you to it.
It's not a problem that you missed the previous issue in a search and opened a new one. That's totally fine. But when we point out the correct place to discuss the issue, please respect our attempt at keeping our issue tracker at least a little bit organized.

issues can easily be linked to from other issues, and are all the time.

True, but that just makes the github-lack-of-threading problem even worse.

new issues are easier to find, and thus will get more discussion.

We don't want more discussion that just repeats previously-had discussion. That's the main reason we want to keep discussion in a single issue, so that it's easy(-ish) to peruse, and not repeat, what has already been discussed.

@ALTree
Copy link
Member

@ALTree ALTree commented Aug 19, 2020

The goal of keeping the discussion about a specific problem all in one place is to be sure that the people vetting the proposals and making decisions about them don't have to keep seeing (and reading) the same thing again and again and again.

While it's true the your proposal and the one in #38832 would flag different code, the core of the matter is the same: "should vet flag functions declarations and/or variable assignments that shadow the builtins?". You probably agree that we don't need to discuss this two times: we can do it once and make a decision about what kind of vet check we want to implement. Function signatures, shadowing in assignments, maybe both, or maybe neither.

In general, if something needs to be investigated (and decided upon) only one time, then we try to have one discussion about it; unless the issue at hand is so complex and multifaceted that discussing it in one place is literally impossible.

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