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

Jump to bottom

os: document that IsExist, IsNotExist, IsTimeout, IsPermission should be avoided in favor of errors.Is #41122

Closed
leventov opened this issue Aug 29, 2020 · 7 comments
Closed

os: document that IsExist, IsNotExist, IsTimeout, IsPermission should be avoided in favor of errors.Is #41122

leventov opened this issue Aug 29, 2020 · 7 comments
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.16

Comments

@leventov
Copy link

leventov commented Aug 29, 2020 •
edited

I thought all these functions to be exact semantic equivalent of errors.Is(err, os.ErrExist), errors.Is(err, os.ErrnotExist),

var terr interface {
	Timeout() bool
}
ok := errors.As(err, &terr)
return ok && terr.Timeout()

and errors.Is(err, os.ErrPermission), respectively. I was highly surprised to find they don't.

@ianlancetaylor suggested that "Maybe we should simply change "the error" to "an error returned from this package.", regarding the doc for os.IsNotExist():

IsNotExist returns a boolean indicating whether the error is known to report that a file or directory does not exist. It is satisfied by ErrNotExist as well as some syscall errors.

I don't think it's enough because it still requires a great deal of attention to notice the semantic difference with errors.Is(err, os.ErrnotExist) by just reading the suggested edited doc

IsNotExist returns a boolean indicating whether an error returned from this package is known to report that a file or directory does not exist. It is satisfied by ErrNotExist as well as some syscall errors.

Not to mention that when a developer looks at the signature of this function, it appears very simple and looks like it has an obvious meaning, so I suspect most developers don't even read the doc for this function. For example, I think I didn't, before I stumbled upon the semantic mismatch.

To the attention of Golang maintainers, please consider that people who started using Golang after errors.Is() was introduced don't have the hindsight of remembering that os.Is... functions existed before errors.Is() and thus should probably mean something special or "historic".
leventov added a commit to northvolt/migrate that referenced this issue Aug 29, 2020
@leventov
Use errors.Is(...) instead of os.Is...() in migrate.go: golang/go#41122
04fb8d6
@davecheney
Copy link
Contributor

Thank you for raising this issue. You've indicated that it is a proposal. After reading the text of the issue I think your proposal would be clearer if you could call out what you would like to change in the language. It feels like the proposal you are making is to change the documentation to discourage the use of the os.Is... set of functions. Did I understand you correctly?

@leventov
Copy link
Author

I would actually prefer os.Is... implementations to be changed to be exactly as they are expected to be.

This issue is a "reopen" of #36150 because I don't understand the argument of @ianlancetaylor for closing it.

@ianlancetaylor
Copy link
Contributor

My concern is that if we change the functions in the os package, that will be an unexpected change for existing programs that are using them. There is a comment in the code that says

	// Note that this function is not errors.Is:
	// underlyingError only unwraps the specific error-wrapping types
	// that it historically did, not all errors implementing Unwrap().

CC @neild who added this comment in https://golang.org/cl/163058. @neild Is there a known problem with changing os.IsExist and friends to use errors.Is? Or is this an understandable caution about backward compatibility without a known problem?

Either way, I've said all along that we should change the docs.

dhui pushed a commit to golang-migrate/migrate that referenced this issue Aug 29, 2020
@leventov
Include file path in errors from PartialDriver.ReadUp() and ReadDown() (
f1cf81e 
#421)

* Include file path in errors from PartialDriver.ReadUp() and ReadDown()

* Use errors.Is(...) instead of os.Is...() in migrate.go: golang/go#41122
@dmitshur dmitshur changed the title os.IsExist, os.IsNotExist, os.IsTimeout, os.IsPermission violate the principle of least astonishment os: IsExist, IsNotExist, IsTimeout, IsPermission violate the principle of least astonishment Aug 31, 2020
@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 31, 2020
@dmitshur dmitshur added this to the Backlog milestone Aug 31, 2020
@rsc
Copy link
Contributor

rsc commented Oct 20, 2020

We spent a long time on this in the error designs. It is an understandable caution about backwards compatibility without a specific, known problem. It is sure to break existing code, and error-handling is not always well tested (it's just hard to test).

Now that all supported versions of Go provide errors.Is, we should update the doc comments on these functions to suggest using errors.Is instead.

@rsc rsc mentioned this issue Oct 20, 2020
Closed
@rsc rsc changed the title os: IsExist, IsNotExist, IsTimeout, IsPermission violate the principle of least astonishment os: document that IsExist, IsNotExist, IsTimeout, IsPermission should be avoided in favor of errors.Is Oct 20, 2020
@rsc rsc added NeedsFix The path to resolution is known, but the work has not been done. and removed Documentation labels Oct 20, 2020
@rsc rsc self-assigned this Oct 20, 2020
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 20, 2020
@leventov
Copy link
Author

A specific, known problem is that this violation of the principle of least astonishment causes people to misuse the functions in real code, for example, golang-migrate/migrate@04fb8d6

@bradfitz bradfitz modified the milestones: Backlog, Go1.16 Nov 2, 2020
@bradfitz
Copy link
Contributor

bradfitz commented Nov 2, 2020

Let's document this for Go 1.16 at least. This just bit me too.

I was testing an error from fmt.Errorf("... %w", ..., ioutil.ReadFile("/some/not-exist-file")) and was assuming the returned error would be os.IsNotExist, but it's not.

bradfitz added a commit to tailscale/tailscale that referenced this issue Nov 2, 2020
@bradfitz
Change some os.IsNotExist to errors.Is(err, os.ErrNotExist) for non-o…
01ee638 
…s errors.

os.IsNotExist doesn't unwrap errors. errors.Is does.

The ioutil.ReadFile ones happened to be fine but I changed them so
we're consistent with the rule: if the error comes from os, you can
use os.IsNotExist, but from any other package, use errors.Is.
(errors.Is always would also work, but not worth updating all the code)

The motivation here was that we were logging about failure to migrate
legacy relay node prefs file on startup, even though the code tried
to avoid that.

See golang/go#41122
@gopherbot
Copy link

Change https://golang.org/cl/268897 mentions this issue: os: clarify that IsExist and friends do not use errors.Is

@ianlancetaylor ianlancetaylor mentioned this issue Nov 22, 2020
Closed
aead pushed a commit to minio/kes that referenced this issue Nov 24, 2020
replace os.Is{...} error checks with errors.Is(err, ...)
fd02e15 
This commit replaces the error checks using the `os.Is{...}`
functions - e.g. `os.IsExist(err)` - with the more generic
`errors.Is(err, ...)`.

In general, the `os.Is{...}` functions should not be used.
See: golang/go#41122
@aead aead mentioned this issue Nov 24, 2020
Merged
harshavardhana pushed a commit to minio/kes that referenced this issue Nov 25, 2020
@harshavardhana
replace os.Is{...} error checks with errors.Is(err, ...)
5b86974 
This commit replaces the error checks using the `os.Is{...}`
functions - e.g. `os.IsExist(err)` - with the more generic
`errors.Is(err, ...)`.

In general, the `os.Is{...}` functions should not be used.
See: golang/go#41122
@golang golang locked and limited conversation to collaborators Nov 11, 2021
@rsc rsc removed their assignment Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.16
Development

No branches or pull requests
7 participants
@bradfitz @davecheney @rsc @leventov @dmitshur @ianlancetaylor @gopherbot