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/vet: detect bad types for calls to sort.Slice #17923

Closed
campoy opened this issue Nov 15, 2016 · 10 comments
Closed

cmd/vet: detect bad types for calls to sort.Slice #17923

campoy opened this issue Nov 15, 2016 · 10 comments

Comments

@campoy
Copy link
Contributor

campoy commented Nov 15, 2016

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

go version devel +866e014 Fri Nov 11 17:07:07 2016 +0000 darwin/amd64

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

darwin/amd64

What did you do?

Given this program:

func main() {
	m := map[string]bool{}
	sort.Slice(m, func(i, j int) bool { return a[i] < a[j] })
}

What did you expect to see?

I'd expect go vet to complain about the type of m not being a slice in the call to sort.Slice.

What did you see instead?

Panic

@mvdan
Copy link
Member

mvdan commented Nov 15, 2016

What panics? Vet or the execution of the program? If it's the latter, maybe a better error would suffice.

@bradfitz
Copy link
Contributor

sort.Slice panics, as documented.

@campoy
Copy link
Contributor Author

campoy commented Nov 15, 2016

Yes, sort.Slice panics, and I think this is a good candidate to be reported by vet.

It's a very simple check, already wrote the code, and there's 0 chance of false positives.

@robpike
Copy link
Contributor

robpike commented Nov 15, 2016

I don't want to set up a tradition where everything that might panic gets a vet check. That's dangerous.

Also, this feature hasn't even been released yet, which means the 'frequency' metric is unknown now. So it's not going into vet, at least not yet.

@bradfitz
Copy link
Contributor

I don't want to set up a tradition where everything that might panic gets a vet check. That's dangerous.

This will panic, not might.

Why's it dangerous?

@robpike
Copy link
Contributor

robpike commented Nov 15, 2016

It's a dangerous precedent because vet is being used more and more and if it's going to take on the every possible failure, it may become too expensive to run, or at least require rearchitecture.

Besides, I'm not convinced it should assume the role of catching things that are guaranteed to fail the first time you run the program.

The frequency question still exists. At the moment the problem is purely hypothetical and, I think, unimportant.

@bradfitz
Copy link
Contributor

So the frequency question is mostly about vet performance? I've never measured it.

@minux
Copy link
Member

minux commented Nov 15, 2016 via email

@campoy
Copy link
Contributor Author

campoy commented Nov 16, 2016

That'd be an interesting check and I'm happy to implement it too.

But after the conversation with @robpike I think it's better to wait to see if people really make those mistakes before adding extra checks.

Keeping the code I wrote in case we ever needed
DO NOT REVIEW https://go-review.googlesource.com/#/c/33334/

@quentinmit
Copy link
Contributor

I'm going to close this for now. We can reopen it (or refile it) if it turns out this is a common mistake.

@golang golang locked and limited conversation to collaborators Nov 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants