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

Comments

Projects
None yet
7 participants
@campoy
Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

commented Nov 15, 2016

sort.Slice panics, as documented.

@campoy

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

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.

This will panic, not might.

Why's it dangerous?

@robpike

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

commented Nov 15, 2016

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

@minux

This comment has been minimized.

Copy link
Member

commented Nov 15, 2016

@campoy

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor

commented Nov 16, 2016

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

@quentinmit quentinmit closed this Nov 16, 2016

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