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

proposal: Go 2 error values #29934

Open
jba opened this Issue Jan 25, 2019 · 229 comments

Comments

Projects
None yet
@jba
Copy link
Contributor

jba commented Jan 25, 2019

This issue is for discussion of our Go 2 error values proposal, which is based on our draft designs for error information and formatting.

This proposal will follow the process outlined in the "Go 2, here we come" blog post: we will have everything ready to go and checked in at the start of the Go 1.13 cycle (Feb 1), we will spend the next three months using those features and soliciting feedback based on actual usage, and then at the start of the release freeze (May 1), we will make the "launch decision" about whether to include the work in Go 1.13 or not.

@gopherbot gopherbot added this to the Proposal milestone Jan 25, 2019

@gopherbot gopherbot added the Proposal label Jan 25, 2019

@mvdan

This comment has been minimized.

Copy link
Member

mvdan commented Jan 25, 2019

Assuming this is accepted and implemented before generics, would the design be adapted in a potential redesign of std with generics?

@mpvl

This comment has been minimized.

Copy link
Member

mpvl commented Jan 25, 2019

@mvdan We think the design not using generics resulted in a slightly nicer API in the end. So we will probably not do so. That said, it can't be ruled out there will be additional API using generics.

@networkimprov

This comment has been minimized.

Copy link

networkimprov commented Jan 25, 2019

Errors constructed with errors.New and fmt.Errorf will display differently with %+v

Could a change in output break consumers which compare it with a hard-coded string, e.g. test code?

@gopherbot gopherbot added the Go2 label Jan 25, 2019

@neild

This comment has been minimized.

Copy link
Contributor

neild commented Jan 26, 2019

Errors constructed with errors.New and fmt.Errorf will display differently with %+v

Could a change in output break consumers which compare it with a hard-coded string, e.g. test code?

Conceivable, but since the fmt package currently treats %+v identically to %v for any type which implements the error interface there isn't a strong reason for users to use the former to format errors at the moment.

@Sherlock-Holo

This comment has been minimized.

Copy link

Sherlock-Holo commented Jan 27, 2019

I wonder why xerrors.Errorf("new error: %w", baseErr) %w must at the end, I think when print the error, it should be

baseErr: some error

not

some error: baseErr

@networkimprov

This comment has been minimized.

Copy link

networkimprov commented Jan 27, 2019

Agreed it's odd that the format string is an output-type controller, rather than a flag in the verb like %$v.

Also, have the authors considered providing this, which is conspicuously absent in package fmt:

func Error(a ...interface{}) error

This works like errors.New(fmt.Sprint(a, b))
Just as Errorf works like errors.New(fmt.Sprintf(s, a, b))

@neild

This comment has been minimized.

Copy link
Contributor

neild commented Jan 27, 2019

Agreed it's odd that the format string is an output-type controller, rather than a flag in the verb like %$v.

What are the arguments for a flag over a verb? I don't see a strong argument for one over the other and %w has some mnemonic value, but perhaps I'm missing something.

func Error(a ...interface{}) error

I think this is orthogonal to the rest of the proposal, but it doesn't seem unreasonable. On the other hand, I don't think I've ever felt the absence of this function.

@networkimprov

This comment has been minimized.

Copy link

networkimprov commented Jan 28, 2019

I'm referring to this output-type control via the format string: "if ... the format string ends with : %s, : %v, or : %w, then the returned error will implement FormatError." Why not a flag for %s & %v?

The easiest way to print an error is: fmt.Println("this happened: ", err)
The easiest way to make an error should be: fmt.Error("this happened: ", err)

@jba

This comment has been minimized.

Copy link
Contributor Author

jba commented Jan 28, 2019

Why not a flag for %s & %v?

The only existing flags that don't have a meaning for %s and %v are space and 0. But those both feel wrong, so we'd need to make up a new flag. That means one could potentially use that flag with any verb, but it would be meaningless except for %s and %v. That feels wasteful—flags and verbs should combine orthogonally to (almost) always produce a useful result (with space and 0 themselves being the notable exceptions).

@ericlagergren

This comment has been minimized.

Copy link
Contributor

ericlagergren commented Jan 28, 2019

To be clear: does this mean each call to fmt.Errorf will collect a stack trace?

@neild

This comment has been minimized.

Copy link
Contributor

neild commented Jan 28, 2019

To be clear: does this mean each call to fmt.Errorf will collect a stack trace?

A single stack frame, not a full trace.

@tandr

This comment has been minimized.

Copy link

tandr commented Jan 28, 2019

To be clear: does this mean each call to fmt.Errorf will collect a stack trace?

A single stack frame, not a full trace.

Would it be possible to make it "adjustable" somehow? In our home-made logging wrapper we have noticed that we have to pull a couple or 3 frames to get to "the caller of interest", so printout looks more or less pointing to the place where logwrap.Print(err) function was called.
If this is becoming a part of the standard, in case when wrapper (or utility method) is created to simplify an error creation, I would like to report the place where that method was called from, and not the utility method itself.

We did it by (arguable not so elegant) way as adding a parameter "how big is the stack offset" to the logging function, but I am not sure if that's the only option here. Adding special format symbol for stack, with number of frames interesting might be one of possible approaches.

@neild

This comment has been minimized.

Copy link
Contributor

neild commented Jan 28, 2019

The intended usage is that additional frames be added as necessary by annotating or wrapping the error, possibly with additional context.

For example,

func inner() error { return errors.New("inner error") }
func outer() error { return fmt.Errorf("outer error: %w", inner()) }

fmt.Fprintf("%+v", outer())
// outer error:
//     /path/to/file.go:123
//   - inner error:
//     /path/to/file.go:122

To attach information about where a utility function was called from, you do so in the same way as today: By annotating the error with additional information.

We considered capturing a full stack rather than a single frame, but think the single-frame approach has some advantages: It's light-weight enough to be feasible to do for every error and it deals well with error flows that pass between goroutines.

That said, this proposal makes it easy for error implementations to add whatever detail information they want in a compatible way, be it full stacks, offset stack frames, or something else.

@networkimprov

This comment has been minimized.

Copy link

networkimprov commented Jan 28, 2019

[the flag] would be meaningless except for %s and %v. That feels wasteful—flags and verbs should combine orthogonally to (almost) always produce a useful result (with space and 0 themselves being the notable exceptions).

Erm, I think you've refuted your own argument by raising exceptions :-)

: %s is magical, invisible, and readily broken without compiler complaint. And no other format string has similar effect. That's not Goish to my eye.

@neild

This comment has been minimized.

Copy link
Contributor

neild commented Jan 29, 2019

: %s is magical, invisible, and readily broken without compiler complaint. And no other format string has similar effect. That's not Goish to my eye.

I'm not certain if you're making an case about verbs vs. flags (%v/%w vs. %v/%$v) or about using fmt.Errorf to annotate/wrap errors in general. Could you clarify your understanding of the proposal and what you're suggesting instead?

@networkimprov

This comment has been minimized.

Copy link

networkimprov commented Jan 29, 2019

As I said above, I'm referring to this output-type control in the format string:

if ... the format string ends with : %s, : %v, or : %w, then the returned error will implement FormatError.

The control should be a flag e.g. %$s & %$v, and not the contents of the format string.

@neild

This comment has been minimized.

Copy link
Contributor

neild commented Jan 29, 2019

The control should be a flag e.g. %$s & %$v, and not the contents of the format string.

Thanks; I think I understand you now.

The current design of applying special-case handling in fmt.Errorf to a : %v et al. suffix is informed by a couple of factors.

Firstly, we want to permit existing code to take as much advantage of the new error formatting features as possible. So, for example, errors.New and fmt.Errorf now capture the caller's stack frame by and print this information in detail mode. Existing code that creates errors does not need to change to include this information. But what about annotation of errors? Consider the following:

func f1() error { return errors.New("some error") }
func f2() error { return fmt.Errorf("f2: %v", f1()) }

func main() {
  fmt.Printf("%+v", f2())
}

We want this to print:

some error:
    main.go:1
  - f2:
    main.go:2

But if annotation is only enabled with a special format directive like %$v, then this will instead print:

some error: f2
    main.go:2

The innermost stack frame is lost.

The other consideration is that error annotation is linear. The errors.Formatter interface allows an error to format itself and return the next error to format. By design, it does not provide a way for an error to interleave its output with that of another error. This limitation makes the formatted output more consistent and predictable, but it means that there is no way to insert a hypothetical %$v into the middle of a format string:

// What would this error's `FormatError` method do?
return fmt.Errorf("some information about error: %$v (with some more information)", err)

These two considerations led to the current design of automatically annotating errors produced by fmt.Errorf calls that follow the most common pattern of adding information to an error, but only when the original error appears at the end of the string.

@networkimprov

This comment has been minimized.

Copy link

networkimprov commented Jan 29, 2019

Re the stack frame, annotation could be enabled by any use of an error argument with Errorf(), instead of a magical, brittle format string.

there is no way to insert a hypothetical %$v into the middle of a format string

Would a format string "abc %v 123" work as expected given annotation enablement on use of an error argument? If not, I believe you need to rethink this...

@jba

This comment has been minimized.

Copy link
Contributor Author

jba commented Jan 29, 2019

Would a format string "abc %v 123" work as expected given annotation enablement on use of an error argument?

No; we will only treat an error specially if the format string ends with ": %s" or ": %v".

If not, I believe you need to rethink this...

Can you explain why? Our goal with this feature is to instantly and painlessly bring the new formatting into most existing Go code. We'd rethink that if, for example, it turned out that most existing calls to fmt.Errorf that included an error did not put the error at the end with ": %s" or ": %v". If that is true, we'd like to know.

As Damien pointed out, if you do like to put errors in the middle of your format strings, you're going to have bigger problems with our proposal than its fmt.Errorf behavior. Even if you roll your own formatting with the FormatError method, you're going to have a hard time getting wrapped errors to display in the middle.

One more point: while we do expect people to continue to write fmt.Errorf calls that wrap errors, we also encourage custom error-creating functions that build specific errors and might incorporate other features, like frame depth (as in @tandr's request). E.g.

type MyError ...
func MyErrorf(wrapped error, frameDepth int, format string, args ...interface{}) *MyError
@tandr

This comment has been minimized.

Copy link

tandr commented Jan 29, 2019

No; we will only treat an error specially if the format string ends with ": %s" or ": %v".

My concern here would be that
a) not every message fits this format, and
b) not every message is going to be in English.

"Something happened: This is What" is an ok way to produce a short human-readable error, but... Even in English, if I would want some text explaining "What to do now?" (remediation suggestion, "contact support" etc), that : %v might not be the last item in the formatting string. And the whole "at the end" logic will be completely broken for languages that are using Right-to-Left writing order (Arabic, Hebrew, Farsi, Urdu etc.)

Would it be easier to introduce a special formatter "print this as 'error'" and a modifier to make it "print this as an error with a stack"? I think there are enough letters in English alphabet left to cover one more case.

Also, if I may ask - no magic, please.

@glibsm

This comment has been minimized.

Copy link

glibsm commented Jan 29, 2019

The Unwrap function is a convenience for calling the Unwrap method if one exists.

// Unwrap returns the result of calling the Unwrap method on err, if err implements Unwrap.
// Otherwise, Unwrap returns nil.
func Unwrap(err error) error

It would also be nice to have a standard helper that can get to the root cause, i.e. the last error in the chain.

Similar to https://godoc.org/github.com/pkg/errors#hdr-Retrieving_the_cause_of_an_error.

@neild

This comment has been minimized.

Copy link
Contributor

neild commented Jan 29, 2019

There are two points here, and it's worth considering them independently. The first is the manner in which error annotations compose.

The proposal adds a new errors.Formatter interface which errors may optionally implement. This interface provides two main pieces of functionality: Error detail in which an error may provide additional information when formatted with the %+v directive, and wrapping in which one error may add additional information to another.

A primary goal of the errors.Formatter interface is to allow different error implementations to interoperate. You should be able to create an error with errors.New, add some information to it with "github.com/pkg/errors".Wrap, add more information to that with "gopkg.in/errgo.v2/errors".Wrap, and get reasonable behavior.

Detail formatting of an error (%+v) produces multi-line output, with each step in the error chain represented as a section of one or more lines. For example:

could not adumbrate elephant:  - these three lines are the outermost error
    (some additional detail)   |
    labrynth.go:1986           /
  - out of peanuts             - and these two lines are the innermost error
    elephant.go:15             / 

Displaying a chain of errors in a consistent fashion requires making certain decisions about formatting: Do we print errors from the outermost to innermost, or vice-versa? How do we indicate boundaries between errors? If we leave these formatting decisions up to individual error implementations, we are likely to have situations where, for example, part of an error chain is printed inner-to-outer and another part is outer-to-inner; very confusing and not a good user experience.

To avoid confusion and simplify the work required to write a custom error type, we put the formatting of the error chain under the control of the error printer. Errors provide three pieces of information: The error text, detail text, and the next error in the chain. The printer does the rest. So, for example, the outermost error in the above example might have an ErrorFormatter implementation like:

func (e errType) FormatError(p Printer) (next error) {
  p.Print("could not adumbrate elephant") // error text
  if p.Detail() {
    p.Print("(some additional detail)") // error text only visible in detail (%+v)
    p.frame.FormatError(p) // print stack frame as detail
  }
  return e.wrappedErr // return the next error in the chain
}

Note again that an important property of this design is that an error doesn't choose how to format the error that it wraps; it returns that error to the printer, which then determines how to format the error chain. Not only does this simplify the error implementation and ensure consistency, but this property is important for localization of errors in RTL languages (as mentioned by @tandr), because it permits a localizing printer to adjust the output order as appropriate. (Most users will print errors using the fmt package, but note that this design deliberately allows for other implementations of errors.Printer.)

Given the above, the question then is how to make it simple for users to produce errors that satisfy these interfaces, which is where we get to the special handling of a suffix : %v. It will be helpful if critiques of this proposal are clear about whether a comment is about the above formatting interfaces, or fmt.Errorf specifically.

@neild

This comment has been minimized.

Copy link
Contributor

neild commented Jan 29, 2019

It would also be nice to have a standard helper that can get to the root cause, i.e. the last error in the chain.

The equivalent in this proposal is the errors.Is function:

if errors.Is(err, io.EOF) { ... }

We believe this has several nice properties over a function returning the last element in a chain:

  • Harder to accidentally discard context (e.g., return errors.Cause(err)).
  • An error can implement an Is(target error) bool) method to declare that it is equivalent to another error.
  • Nicely analogous to errors.As, which converts an error to a type.
@velovix

This comment has been minimized.

Copy link

velovix commented Jan 29, 2019

I like the motivation of the new Errorf behavior being a way to grandfather in old code. Near as I can tell, it could only make old code better. However, I'm less into the idea of Errorf being the main way to wrap errors. I could see programmers writing some error handling logic and, due to force of habit, accidentally writing fmt.Errorf("context: %v", err) instead of fmt.Errorf("context: %w", err) and missing out on potentially valuable information or effecting the intended error handling path. Since errors often find themselves in uncommon circumstances, it's possible to miss problems like these until an error in production occurs and you have less diagnostic information than you were hoping. For that reason, I think having a stronger, more compiler friendly wrapping solution available makes sense. Maybe something like github.com/pkg/errors.Wrap(err, message)?

To be a bit more concrete, I have more than once found myself in a similar situation with Python, where I raise an exception using f-strings but forget to put the "f".

raise RuntimeError("oh no! Something happened: {important_information}")

... instead of ...

raise RuntimeError(f"oh no! Something happened: {important_information}")

And since there's nothing syntactically wrong with what I did, I might not know until later when I need important_information that I've made a mistake.

@networkimprov

This comment has been minimized.

Copy link

networkimprov commented Jan 29, 2019

Our goal with this feature is to instantly and painlessly bring the new formatting into most existing Go code.

Although that seems like a laudable goal, modifying the way existing code creates output is inherently risky. I suspect most of us would rather have an opt-in mechanism. Let project owners trial a feature and switch over (via go fix) when convenient for them.

IOW, new error-construction functions are needed, which always return errors.Formatter.

@PieterD

This comment has been minimized.

Copy link
Contributor

PieterD commented Jan 31, 2019

While the fmt.Errorf changes to wrap the error is neat and conserves backwards compatibility with previous versions, a slightly nicer option going forward might be good?

When using pkg/errors, errors.Wrapf(err, "error reticulating spline %d", splineID) is the most used pattern by far. If that is also introduced, both of the most common patterns are maintained (aside from a switch from %v to %w in fmt.Errorf, which might cause trouble if one is missed.)

gopherbot pushed a commit to golang/net that referenced this issue Feb 28, 2019

http2: remove use of DeepEqual for testing errors
Comparing errors using DeepEqual breaks if frame information
is added as proposed in golang/go#29934.

Updates golang/go#29934

Change-Id: Ia2eb3f5ae2bdeac322b34e12d8e732174b9bd355
Reviewed-on: https://go-review.googlesource.com/c/164517
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
@nhooyr

This comment has been minimized.

Copy link
Contributor

nhooyr commented Feb 28, 2019

Just had another senior engineer also think xerrors.Unwrap recursively unwrapped.

@dfawley

This comment has been minimized.

Copy link

dfawley commented Mar 1, 2019

Taking the discussion about the name of "Wrapper" out of the CL that added it.

@davecheney said:

Let me offer a counter argument.

What is the name for the plastic that covers a piece of candy? A wrapper. What do you do when you want to take the wrapper off? Unwrap the candy.

The important part of the interface is the name of the method, Unwrap, describing the behaviour that the value has.

That is an argument against the entire naming convention of Go's interfaces. A "Reader" is not "a thing that reads" but "a thing that allows others to Read from it". When thinking that way, a "Wrapper" should not be thought of as "a thing that wraps", but "a thing that allows others to Wrap things".

So consider if, in the future, we want to add an interface in this package for a thing that produces wrapped errors. This would most likely contain the method: Wrap(error) error. What should we name this interface?

There needs to be strong justification for going against convention, which is what this proposal does. Not the other way around.

@nhooyr

This comment has been minimized.

Copy link
Contributor

nhooyr commented Mar 1, 2019

#29934 (comment)

Regarding my comment here, I guess grpc's status.Code helper could be fixed to use xerrors.As to grab the error but it seems wrong that I have to wait for grpc-go. to update the library. I should be able to easily use that helper.

I've also come across packages where the error type isn't exported, so you can't do a direct comparison or even use xerrors.As. There is just an exported function to query a trait about an error, sort of like os.IsNotExist. Without a helper function to get the root error, I have to wait for all these functions to be upgraded and support grabbing the root error via xerrors.

@williammartin

This comment has been minimized.

Copy link

williammartin commented Mar 8, 2019

I would just like to chime in here that the addition of frames will break users of gomega. Currently, gomega tests are broken.

Gomega supports assertions of the form:

Expect(err).To(MatchError(MyError{ ... })

Which internally uses reflect.DeepEqual.

Just to provide some numbers across a selection of test suites I have hanging around:

Gomega: 5/239 tests
CF CLI: 6/93 tests
Garden Server: 2/55 tests
Garden Backend: 3/147 tests
...a few more with 1 failure

I also had a few suites that had zero failures.

Most of the projects I tested that I knew used gomega had at least one failure as result of this. While it may be true that using reflect.DeepEqual to compare errors is not correct, it's worked for the last 5 years. I find it unfortunate that when users bump to the version of Go that include this change, they will have to move gomega in lockstep.

I don't really have good suggestions other than those already offered, and I understand the desire not to make DeepEqual really mean SortOfEqual, I just wanted to add some more data to this. Thanks for your work!

@jba

This comment has been minimized.

Copy link
Contributor Author

jba commented Mar 12, 2019

Our current plan is to modify reflect.DeepEqual to ignore errors.Frame. @neild will update when that lands at tip.

@dsnet

This comment has been minimized.

Copy link
Member

dsnet commented Mar 12, 2019

@jba, is this for all time or only for a few releases?

@jba

This comment has been minimized.

Copy link
Contributor Author

jba commented Mar 12, 2019

All time.

@dsnet

This comment has been minimized.

Copy link
Member

dsnet commented Mar 12, 2019

That is unfortunate to hear. reflect.DeepEqual is too easy to abuse, and I'm not fond of the idea that we're hacking (yes, this is really a hack in the worse sense of the term) the function to be more compatible with existing abuse (and also future abuse).

@networkimprov

This comment has been minimized.

Copy link

networkimprov commented Mar 12, 2019

The benefit of transforming all errors constructed by existing code, without any oversight by package or app authors, has substantial -- and perpetual -- costs.

@dsnet

This comment has been minimized.

Copy link
Member

dsnet commented Mar 12, 2019

@networkimprov

There's no doubt that fixing existing code has a cost, but I don't follow your argument that performing a transformation has "perpetual" cost. You do the transformation once and you're done with it. Also, if reflect.DeepEqual didn't work on pseudo-equivalent errors, then that's a good thing. It prevents future abuse, and so carries future benefits.

Today, as a library owner, it's terrible enough that when I change the text of an internal error, I break people because someone thought it be a great idea to use reflect.DeepEqual on an "equivalent" error to one of mine. Users should not be allowed to do this with reflect.DeepEqual and the change in behavior just makes it easier to persist that bad practice. There's a perpetual cost to keeping the hack in reflect.DeepEqual.

I'm not arguing against hacking reflect.DeepEqual as a temporary stop gap, but I am arguing against keeping this hack forever.

@networkimprov

This comment has been minimized.

Copy link

networkimprov commented Mar 12, 2019

But this transformation occurs implicitly on every build, not once.

However it's the choice to modify behavior of the stdlib API, in lieu of allowing oversight by app and/or package authors, that has permanent cost.

It may be a mistake to compare errors directly with DeepEqual, but is it a mistake to compare objects which contain an error?

@dsnet

This comment has been minimized.

Copy link
Member

dsnet commented Mar 12, 2019

I argue that the only proper way to interact with any value of a type that you do not own is through it's public API (that's the whole point of visibility rules in Go). Therefore, it is a mistake to use reflect.DeepEqual on a data structure containing an errors.errorString since it compares the unexported fields of errors.errorString, providing an illusion of equality that doesn't really exist.

@tandr

This comment has been minimized.

Copy link

tandr commented Mar 12, 2019

There should be some guidance on how packages (and reflect is just one of them) express their expectations of packages/types that are using it. I think it is usually called an "interface", and it is actually a 2 way street of expectations - packages can expect external types to adhere to one, and package users expect some interfaces (in a wider term) to be exposed by the package.
In this case, having

package reflect
type DeepEqualable interface {
   DeepEqual(interface{}) bool
}

(and maybe some sort of pretty printer) would solve it without hidden hacks - reflect would just have to

  1. check if item implements this interface.
  2. use it for for these types that provide it
  3. provide "default" behaviour (before last commits) for everything else.

Even in case of hard insistence on "we want special treatment for errors", handling of error-representing types could be internalized in the reflect package itself as "defaulted" to be have this interface implemented for them (or for some "CallStack" type that sits inside of the "new generation" errors).
At least there could be an escape route for implementations that desire to deep=compare errors themselves. I think @dsnet wants to have a bit better comparison for protobuf messages (which are already painfully not comparable by simple == , and require proto.Equal instead)

@IngCr3at1on

This comment has been minimized.

Copy link

IngCr3at1on commented Mar 12, 2019

which are already painfully not comparable by simple ==

You can't compare really any structs directly by this manner (except pointers I suppose) so I wouldn't expect it to work for proto messages (unless they're pointers to the same heap address).

As for using DeepEqual on a protobuf message I have a feeling that would choke on some of the private protobuf fields regardless (though personally I've never tried it); I don't actually see how the two are related.

Perhaps I'm missing something but I didn't read any of the objections to using DeepEqual on errors as being related to comparing other non-error types such as protobuf messages.


Having said that while I agree with the general sentiment expressed by @dsnet; in particular in the sense of:

the only proper way to interact with any value of a type that you do not own is through it's public API

I don't see how ignoring something that is specific to errors (IE: frames) really creates a risk for other comparison's using DeepEqual (a potential risk would imply that frames are being used somewhere other than xerrors (which I don't believe they are lol)) but rather if anything makes DeepEqual slightly more appropriate for the use and therefore less of an "abuse" in the first place.

To that end, what is the real 'danger' in making this change (I've read most of this thread several times and even took the time to reread the documentation for DeepEqual itself and am having issues understanding an argument against this change beyond simply the general rules of Go privacy already mentioned by @dsnet (which DeepEqual already at least semi-blatantly ignores))?

@tandr

This comment has been minimized.

Copy link

tandr commented Mar 12, 2019

Thank you @IngCr3at1on. Yes, I have deviated a bit from "error comparison" to a bit wider problem that becomes more visible here, sorry about it. I was trying to express that issue with DeepEqual doing what it thinks is right to do with comparison of values (error is just one of them) is probably stemming from lack of interfaces expected from values in general. proto.Equal is actually a good example here - if I have 2 structs holding protobuf generated messages many levels below, since last March I cannot use DeepEqual to compare them, and I have to write my own code all the way down. Now the same will be applicable to anything that holds anything "error"-y, which I think is going to be much, much more painful.
DeepEqual has expectation of what it does, even if only by the name given to it.

@go101

This comment has been minimized.

Copy link

go101 commented Mar 13, 2019

A little off topic. Maybe Go needs an annotation alike feature to specify type properties.
For example,

  • we can specify the property of the type of "frames" as "its values are all equal with each other".
  • we can specify a type as no-copy type.
  • ...
@go101

This comment has been minimized.

Copy link

go101 commented Mar 13, 2019

or annotations for struct fields.

@IngCr3at1on

This comment has been minimized.

Copy link

IngCr3at1on commented Mar 13, 2019

@go101 you can (and in fact should) easily write package documentation for externally available struct values which is already fully supported by godoc. I'm not sure another annotation method is needed?


edit: sorry I reread that comment again and you are talking about slightly different annotation (to be read by the runtime instead of by the developer) I apologize for confusion...

I'm just wondering if struct tags might work for that as well...

@Sherlock-Holo

This comment has been minimized.

Copy link

Sherlock-Holo commented Mar 14, 2019

when use log.Printf("%+v") to print xerrors stack, can the sequence follow panic sequence?

@go101

This comment has been minimized.

Copy link

go101 commented Mar 14, 2019

@IngCr3at1on

I'm just wondering if struct tags might work for that as well...

I don't know. I have never seen examples of struct field tags serve for compiler/runtime.

gopherbot pushed a commit that referenced this issue Mar 20, 2019

os: make errors.Is work with ErrPermission et al.
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

This comment has been minimized.

Copy link

gopherbot commented Mar 20, 2019

Change https://golang.org/cl/168438 mentions this issue: reflect: ignore errors.Frame in DeepEqual

@mpvl

This comment has been minimized.

Copy link
Member

mpvl commented Mar 20, 2019

@Sherlock-Holo I'm not entirely clear what you mean, but printing errors relies on package fmt, which recovers panics and always prints something. So log.Printf should not panic if, say, an implementation of FormatError panics.

@Sherlock-Holo

This comment has been minimized.

Copy link

Sherlock-Holo commented Mar 21, 2019

func do() error {
	return xerrors.New("err 1")
}

func main() {
	log.Printf("%+v",xerrors.Errorf("err 2: %w", do()))
}

will print

2019/03/21 09:36:24 err 2:
    main.main
        /home/sherlock/go/src/go-learn/test3.go:14
  - err 1:
    main.do
        /home/sherlock/go/src/go-learn/test3.go:10
func do() error {
	panic("err 1")
	// return xerrors.New("err 1")
}

func main() {
	do()
	// log.Printf("%+v",xerrors.Errorf("err 2: %w", do()))
}

will print

panic: err 1

goroutine 1 [running]:
main.do(...)
	/home/sherlock/go/src/go-learn/test3.go:4
main.main()
	/home/sherlock/go/src/go-learn/test3.go:9 +0x3a

I like the second print style, when in log, I want to see which function has error at first, and then find out the head of error chain. Now xerrors print the head of error chain at last line, print the function which log the error at first line. It is a bit diffcult to debug the codes

@jba

This comment has been minimized.

Copy link
Contributor Author

jba commented Mar 22, 2019

The vet check for errors.As is at https://go-review.googlesource.com/c/tools/+/168938.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.