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: vet should recommend "os.IsNotExist(err)" over "err == os.ErrNotExist" #19267

Closed
go101 opened this issue Feb 24, 2017 · 6 comments

Comments

Projects
None yet
7 participants
@go101
Copy link

commented Feb 24, 2017

It looks the ErrNotExist variable in os package shouldn't be exported at all.

@ianlancetaylor ianlancetaylor changed the title govet should recommend "os.IsNotExist(err)" over "err == os.ErrNotExist" cmd/vet: vet should recommend "os.IsNotExist(err)" over "err == os.ErrNotExist" Feb 24, 2017

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2017

Is this mistake really common enough that it deserves a vet check?

@josharian

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2017

@dominikh

This comment has been minimized.

Copy link
Member

commented Feb 25, 2017

In my own corpus (most public Go packages), there are 229 matches for [!=]= os\.ErrNotExist and 36618 matches for os\.IsNotExist\(.

Among the 229 results are matches in camlistore, rkt and docker.

@go101

This comment has been minimized.

Copy link
Author

commented Feb 26, 2017

@ianlancetaylor, I seldom write file io programs. I think it is so natural that use "err == os.ErrNotExist" to check if a file exists or not when I used the io.Open function the first time. But I was wrong.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Mar 21, 2017

Among the 229 results are matches in camlistore, rkt and docker.

But in Camlistore we have interfaces documenting that os.ErrNotExist is the contract between our own interfaces and our own implementations.

A sample:

make.go:// The error is os.ErrNotExist if GOPATH is unset or the directory
make.go:                return path, os.ErrNotExist
make.go:        return path, os.ErrNotExist
pkg/blob/fetcher.go:    // os.ErrNotExist should be returned for the error (not a wrapped
pkg/blob/fetcher.go:    // offset or length is negative, or os.ErrNotExist if the blob
pkg/blobserver/b2/b2.go:                return nil, 0, os.ErrNotExist
pkg/blobserver/blobpacked/subfetch.go:// check. The returned error should be os.ErrNotExist if the blob

I think this vet check would be noisier than it would be helpful.

@robpike?

@bradfitz bradfitz added this to the Unplanned milestone Mar 21, 2017

@robpike

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2017

I don't think this merits a vet check. Remember it must satisfy the criteria specified in the readme of correctness, frequency, and precision. It fails for correctness and is marginal for frequency and precision.

@robpike robpike closed this Mar 21, 2017

@golang golang locked and limited conversation to collaborators Mar 21, 2018

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.