-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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
wiki: CodeReviewComments: Error documentation #49254
Comments
Handle ErrorsDo not discard errors using var db *sql.DB
func QueryUsername(ctx context.Context, query string) (string, error) {
var username string
err := db.QueryRowContext(ctx, `SELECT username FROM users WHERE username ILIKE ?`, query).Scan(&s)
if errors.Is(err, sql.ErrNoRows) {
// Handle the error.
return "", nil
}
if err != nil {
// Wrap the error with additional context.
// Use %v to avoid importing database/sql errors into your API.
return "", fmt.Errorf("query username: %v", id, err)
}
return username, nil
} Avoid handling errors by Read more about examining errors with Is and As. NotesThe example has been altered from database/sql.DB.QueryRowContext to emphasize error handling. In the actual example code, a switch statement is used. This seems to be a matter of preference, but there is some subtlety with breaking out of a switch statement which I wanted to avoid. Thoughts? In #49356 (comment), @dottedmag points out that there are some opaque errors where the only option is to use string comparison. Should we recommend something like strings.Contains in the event that you really know what you’re doing? Or will that confuse beginners? |
In-Band Errors… And encourages more robust and readable code: value, ok := Lookup(key)
if !ok {
return fmt.Errorf("%w for %q, ErrNoValue, key)
}
return Parse(value) … NotesActually, I’m no longer sure that adding the error sentinel to |
Sentinel ErrorsExport sentinel errors from your package when your users need to check for an identifiable error condition: var ErrNotFound = errors.New("not found")
// FetchItem returns the named item.
//
// If no item with the name exists,
// FetchItem returns an error wrapping ErrNotFound.
func FetchItem(name string) (*Item, error) {
if itemNotFound(name) {
return nil, fmt.Errorf("fetch item %q: %w", name, ErrNotFound)
}
// ...
} Callers can then handle this condition when the item is not found: item, err := FetchItem(name)
if errors.Is(err, ErrNotFound) {
item = defaultItem
} else if err != nil {
return fmt.Errorf("additional context: %w", err)
} You should define your own sentinel errors unless you are willing to support third-party errors as part of your public API. This is true even for the sentinel errors defined in the standard library. Note that standard interfaces like io.Reader are defined to return the standard io.EOF sentinel, not a wrapped version: // EmptyReader represents an empty reader.
// In practice, use strings.NewReader("") instead.
type EmptyReader struct{}
// Read implements the io.Reader interface.
// For an EmptyReader, Read always returns 0, io.EOF.
func (r EmptyReader) Read(p []byte) (n int, err error) {
return 0, io.EOF // Read must never wrap io.EOF.
} Read more about errors and package APIs. NotesThe first example is also the example from errors and package APIs. I have added an example for how the caller would handle a sentinel error. For the third example, I tried to generate something minimal to illustrate why we cannot wrap |
Error TypesExport a custom error type from your package when your users need to check for an identifiable error that includes details describing the error: type QueryError struct {
Query string
Line int
Column int
Err error
}
// Error implements the error interface.
func (e *QueryError) Error() string {
return fmt.Sprintf("%d:%d: %s: %v", e.Line, e.Column, e.Query, e.err)
}
// Unwrap is called by errors.Unwrap to unpack the wrapped err.
func (e *QueryError) Unwrap() error {
return e.err
} Callers can then handle the bad query by unwrapping the error: data, err := Query(q)
if err != nil {
var qerr *QueryError
if errors.As(err, &qerr) {
// Handle the query error by updating the UI
form.SetQuery(qerr.Query)
form.SetPos(qerr.Line, qerr.Column)
form.SetTooltip(qerr.Err.Error())
} else {
return fmt.Errorf("query failed: %v", err)
}
} You should define your own error types unless you are willing to support third-party errors as part of your public API. This is true even for the error types defined in the standard library. Read more about errors in Go 1.13. For an example of a richer error type, see Error handling in Upspin. NotesThe example I didn’t mention making the Error a pointer receiver to prevent equality checks, should I? I also mention upspin.io/errors even though that package doesn’t implement Unwrap. Is that going to confuse people? |
Wrapping ErrorsUse package gzip
func Decompress(name string) error {
f, err := os.Open(filename)
if err != nil {
// Return an error which unwraps to err.
return fmt.Errorf("gzip: decompress %v: %w", name, err)
}
} When formatting errors, carefully consider whether to use NotesI’m wondering how useful the advice in this section will be, given the other sections I’ve proposed. Comments? |
@sfllaw, thanks for these suggestions. I'm going to try to find time this week to give them a careful read. |
I see this is still open. Is it still relevant? Should one resolve to the current description (https://github.com/golang/go/wiki/CodeReviewComments) or should we consider this discussion here to have more viable solutions? |
Problem
Go Code Review Comments contains various bits of advice around errors:
However, there isn’t any good advice on how to deal with:
errors.Is
,errors.As
,.Errorf
.In addition, the Handle Errors redirects readers to some obsolete advice in Effective Go: Errors, which is highlighted by #45535. Since Effective Go will likely be frozen in the near future, see #28782, we should update the link with something more modern.
This work is motivated by #49172 (comment) where we realized that we need to discourage dependencies from doing error string comparisons, like
.Error() == "..."
, so that corrections to error strings won’t break downstream programs.Proposal
The following items are proposed changes to consider, in no particular order:
errors.Is
anderrors.As
, with examples for common idioms.fmt.Errorf
, since the current example encourages consumers to check the Error string for a substring.io.EOF
andsql.ErrNoRows
, declaring custom sentinels witherrors.New
, and recommenderrors.Is
for checking errors. I acknowledge that many of these errors are actually == equal, but that is subtly brittle for new Go programmers.fmt.Errorf
with%w
to wrap an error with additional context, instead of returning the naked error.Implementation
Drafts of each section will be included in the comments below. Please feel free to suggest changes to any of the proposed sections. Please also vote 👍 or 👎 to indicate whether we should include a given section.
The text was updated successfully, but these errors were encountered: