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

all: updating existing errors to work with the Go 1.13 error values #30322

Open
jimmyfrasche opened this issue Feb 20, 2019 · 11 comments
Open

all: updating existing errors to work with the Go 1.13 error values #30322

jimmyfrasche opened this issue Feb 20, 2019 · 11 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@jimmyfrasche
Copy link
Member

This issue is for discussing how to update errors in the standard library and golang.org/x packages to work with the Go 2 error values changes proposed in #29934.

That issue is for discussing the proposal itself. This is for discussing how best to update existing error types to make the most use of the new additions to the errors package, assuming that it is accepted.

All calls to errors.New will be updated automatically.

All calls to fmt.Errorf that do not wrap errors will be updated automatically. Those that wrap errors will not be.

No custom types, outside of the errors package itself, will be updated automatically.

How will the errors that would require manual updating be updated?

For fmt.Errorf the only question is whether to turn a %s/%v to a %w.

For custom types, the questions are

  • should they have an Unwrap method?
  • should they collect stack frames?
  • should they have a FormatError method?
  • Do any require an Is method?
  • An As Method?

If one of the above obviates an older approach (like an Err error field on a struct) should the older approach be marked deprecated?

Even with a general policy, there will likely be exceptions.

net.Error's extra methods vs os.IsTimeout and co. is a particular wrinkle.

The os.IsX predicates test the underlying error of a finite set of error types for certain (platform specific) error values. This does not work with arbitrarily wrapped errors. @neild's https://golang.org/cl/163058 contains a proof-of-concept for changing it so that one can write the more general errors.Is(err, os.ErrTimeout) instead of os.IsTimeout(err).

The predicate methods defined in net.Error offer a similar approach. In the case of IsTimeout, overlapping. To test a general error for this method you can write

var timeout interface { IsTimeout() bool }
if errors.As(err, &timeout) && timeout.IsTimeout() { //etc.

This is slightly more verbose than the Is construct.

Adding an Is method to net.Errors that respond to ErrTimeout and an ErrTemporary could make it more like os. Adding predicate methods to the os errors could make them more like net. Ideally they would be the same because if they're wrapped in further errors the code inspecting them may not know which package the original comes from and would have to handle both.

The approaches are not equivalent when there are multiple errors in the chain that can have the same property. Say error A wraps error B and either may be temporary but only B is. The As construct sets timeout to A whose method returns false (unless it inspects the chain on its own) but the Is construct returns true. If the intent is to override the temporary-ness of B an Is method on A that returns false for ErrTemporary is easy enough. It seems both more flexible and concise.

@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 22, 2019
@dmitshur dmitshur added this to the Unplanned milestone Feb 22, 2019
@rsc rsc modified the milestones: Unplanned, Go1.13 Feb 22, 2019
@neild
Copy link
Contributor

neild commented Feb 25, 2019

Thanks for writing up this list! (And sorry for taking so long to respond.)

My current plan is to add os.ErrTimeout and make every value which returns true for Timeout() also return true for errors.Is(err, os.ErrTimeout). (And the same for Temporary.)

Taking each point in turn, my personal opinion:

For fmt.Errorf the only question is whether to turn a %s/%v to a %w.

Existing fmt.Errorf calls didn't wrap, so leaving them as-is is always correct.

  • should they have an Unwrap method?

Wrapper error types should have Unwrap methods. This means, in particular, all types which forward IsTimeout and IsTemporary to a wrapped type should also have an Unwrap method.

  • should they collect stack frames?

In general, yes, although there might be exceptions where location information adds no useful context and can be omitted.

  • should they have a FormatError method?

In general, yes.

  • Do any require an Is method?

Types with an IsTimeout or IsTemporary method that returns true should have an Is method declaring them as equivalent to os.ErrTimeout/os.ErrTemporary.

  • An As Method?

Not in any cases that I've seen yet.

If one of the above obviates an older approach (like an Err error field on a struct) should the older approach be marked deprecated?

I'm ambivalent on whether we should mark the older approach as deprecated or not.

@jimmyfrasche
Copy link
Member Author

I wrote a program to analyze the 1.12 release's use of errors. It inspects each .go file, regardless of build tags, skipping testdata and cmd directories. It collects custom error types, use of Errorf that may wrap an error, direct calls to an Error method.

The report and program are in this gist: https://gist.github.com/jimmyfrasche/51b83022f83ccbe240092dcd884605cd

While I plan to, I have yet to explore the larger packages: crypto/{tls, x509}, encoding/{json, xml}, or anything under go, internal/x, net, os, or syscall

I went through everything else in the stdlib, though. I'm sure a lot of this on the radar and I'm even surer that I've missed things as this was a quick and superficial exploration, but another pair of eyes never hurts. Here's what I've found so far:

Types with an Err field that could use an Unwrap method:

  • compress/flate.{Read,Write}Error
  • encoding/csv.ParseError
  • strconv.NumError
  • text/template.ExecError

Types with Temporary()/Timeout() methods:

  • context.deadlineExceededError
  • internal/poll.TimeoutError (assuming that ever makes it way outside of internal/)

For errors that get wrapped without a way to get to the underlying error (mostly via fmt.Printf, but there is one exception below), most seem benign but there are a number that wrap IO errors. Arguably that's correct, but I note them regardless.

In image/png an IO error is wrapped in a FormatError but that type's represented as a string so I suspect there's little to be done about that.

Packages that wrap IO errors by calls to fmt.Errorf:

  • internal/trace
  • debug/pe
  • image/gif
  • mime/multipart

There's one other case where I found errors wrapped with fmt.Errorf: database/sql conversions. These wrap errors provided by the database driver. It might be a good idea to change these to %w. For example, postgres has a rich type system and data type conversion errors can contain user defined messages so being able to get to a driver defined error type could be a win.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/163058 mentions this issue: os: make errors.Is work with ErrPermission et al.

gopherbot pushed a commit that referenced this issue Mar 20, 2019
As proposed in Issue #29934, update errors produced by the os package to
work with errors.Is sentinel tests. For example,
errors.Is(err, os.ErrPermission) is equivalent to os.IsPermission(err)
with added unwrapping support.

Move the definition for os.ErrPermission and others into the syscall
package. Add an Is method to syscall.Errno and others. Add an Unwrap
method to os.PathError and others.

Updates #30322
Updates #29934

Change-Id: I95727d26c18a5354c720de316dff0bffc04dd926
Reviewed-on: https://go-review.googlesource.com/c/go/+/163058
Reviewed-by: Marcel van Lohuizen <mpvl@golang.org>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/170037 mentions this issue: all: add Unwrap and Is methods to various error types

@rsc rsc changed the title all: updating existing errors to work with the Go 2 error values proposal all: updating existing errors to work with the Go 1.13 error values Apr 24, 2019
gopherbot pushed a commit that referenced this issue May 4, 2019
Add Unwrap methods to types which wrap an underlying error:

  "encodinc/csv".ParseError
  "encoding/json".MarshalerError
  "net/http".transportReadFromServerError
  "net".OpError
  "net".DNSConfigError
  "net/url".Error
  "os/exec".Error
  "signal/internal/pty".PtyError
  "text/template".ExecError

Add os.ErrTemporary. A case could be made for putting this error
value in package net, since no exported error types in package os
include a Temporary method. However, syscall errors returned from
the os package do include this method.

Add Is methods to error types with a Timeout or Temporary method,
making errors.Is(err, os.Err{Timeout,Temporary}) equivalent to
testing the corresponding method:

  "context".DeadlineExceeded
  "internal/poll".TimeoutError
  "net".adrinfoErrno
  "net".OpError
  "net".DNSError
  "net/http".httpError
  "net/http".tlsHandshakeTimeoutError
  "net/pipe".timeoutError
  "net/url".Error

Updates #30322
Updates #29934

Change-Id: I409fb20c072ea39116ebfb8c7534d493483870dc
Reviewed-on: https://go-review.googlesource.com/c/go/+/170037
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Marcel van Lohuizen <mpvl@golang.org>
@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/194563 mentions this issue: strconv: add Unwrap to custom error types

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/194599 mentions this issue: cmd/go/internal/modfetch/codehost: add Unwrap to custom error types

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/194817 mentions this issue: cmd/go/internal/modload: add an Unwrap method on ImportMissingError

gopherbot pushed a commit that referenced this issue Sep 12, 2019
Jay suggested this in CL 189780, and it seems semantically correct.

As far as I can tell this has no impact one way or the other right
now, but might prevent confusion (or at least give us more experience
with error handling!) in future changes.

Updates #30748
Updates #28459
Updates #30322

Change-Id: I5d7e9a08ea141628ed6a8fd03c62d0d3c2edf2bb
Reviewed-on: https://go-review.googlesource.com/c/go/+/194817
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
gopherbot pushed a commit that referenced this issue Sep 30, 2019
Updates #30322

This change adds the Unwrap method to NumError. NumError is the only custom error type of the strconv that has a nested exported error.

Change-Id: I8774886348880365a83f72a1d106276def27dffe
GitHub-Last-Rev: 712f3df
GitHub-Pull-Request: #34213
Reviewed-on: https://go-review.googlesource.com/c/go/+/194563
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@TheHackerDev
Copy link

TheHackerDev commented Dec 20, 2019

x509.CertificateInvalidError doesn't appear to have an Unwrap() method either, nor do the other Error types in the x509/verify.go file.

@neild
Copy link
Contributor

neild commented Dec 20, 2019

x509.CertificateInvalidError doesn't wrap an error:

type CertificateInvalidError struct {
        Cert   *Certificate
        Reason InvalidReason
        Detail string
}

InvalidReason is an enum of error causes. It would be possible to make InvalidReason implement error and make CertificateInvalidError unwrap to it, permitting:

if errors.Is(err, x509.Expired) { ... }

as an alternative to:

if e, ok := err.(*x509.CertificateInvalidError); ok && e.Reason == x509.Expired { ... }

That actually looks rather nice.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/217132 mentions this issue: doc/go1.14: mention new method strconv.NumError.Unwrap

gopherbot pushed a commit that referenced this issue Jan 31, 2020
Updates #30322
Updates #36878

Change-Id: I8b33eb6a8fb7c0ecf365940a1c3ae88dc807ebcd
Reviewed-on: https://go-review.googlesource.com/c/go/+/217132
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/262343 mentions this issue: crypto/x509: add Unwrap to SystemRootsError

gopherbot pushed a commit that referenced this issue Nov 6, 2020
This change modifies Go to add the Unwrap method to SystemRootsError

Updates #30322

Change-Id: Ibe63d1d0bc832fc0607f09053908d55275a6f350
GitHub-Last-Rev: 9a95bc6
GitHub-Pull-Request: #41981
Reviewed-on: https://go-review.googlesource.com/c/go/+/262343
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

8 participants
@neild @rsc @andybons @jimmyfrasche @dmitshur @TheHackerDev @gopherbot and others