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

os: IsExist, IsNotExist, IsTimeout, IsPermission violate the principle of least astonishment #41122

Open
leventov opened this issue Aug 29, 2020 · 3 comments
Milestone

Comments

@leventov
Copy link

@leventov leventov commented Aug 29, 2020

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".

@davecheney
Copy link
Contributor

@davecheney davecheney commented Aug 29, 2020

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

@leventov leventov commented Aug 29, 2020

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

@ianlancetaylor ianlancetaylor commented Aug 29, 2020

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
#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 this to the Backlog milestone Aug 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.