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
Closed
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@go101
Copy link

go101 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
Copy link
Contributor

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

@josharian
Copy link
Contributor

@dominikh

@dominikh
Copy link
Member

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
Copy link
Author

go101 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
Copy link
Contributor

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 the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Mar 21, 2017
@bradfitz bradfitz added this to the Unplanned milestone Mar 21, 2017
@robpike
Copy link
Contributor

robpike 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 as completed Mar 21, 2017
@golang golang locked and limited conversation to collaborators Mar 21, 2018
@rsc rsc unassigned robpike Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

7 participants