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

errors: how should formatting be handled? #35929

Open
JavierZunzunegui opened this issue Dec 2, 2019 · 8 comments
Open

errors: how should formatting be handled? #35929

JavierZunzunegui opened this issue Dec 2, 2019 · 8 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@JavierZunzunegui
Copy link

JavierZunzunegui commented Dec 2, 2019 •
edited

This is a follow up to the adopted proposal #29934, the go1.13 errors. It is not a proposal, but an attempt to raise awareness of some issues with it.

I am concerned about two omissions:

  1. error formatting: Flexibility on how errors are formatted, err1 + ": " + err2 may be the default, but should not be the only way to do it. It was a significant part of the initial proposal (Error Values — Problem Overview, Error Printing — Draft Design), but was silently dropped as a requirement after flaws were identified in the implementation put forward.
  2. string allocation: To produce an error chain err1 + ": " + err2 + ": " + err3 + ": err4, all intermediate strings (err4, err3 + ": " + err4, err2 + ": " + err3 + ": " + err4) are also allocated (benchmark, may be a bit outdated, ignore the alternative presented there). While, to my knowledge, this is common to the other popular error libraries before go1.13, it seems natural to consider improving on it, particularly given the longer error chains expected as this becomes more popular and potentially stack frames are added.

My ultimate concern is that the go1.13 errors changes, particularly interface {Unwrap() error} can't be made to tackle the above without requiring all such errors to implement additional, non-optional functionality, a breaking change of difficult enforcement. The longer the community is left without guidance of this, and the more custom errors are added with the Unwrap method, the bigger the required breaking change becomes and consequently the less likely it is that go errors may ever have such functionality.

My questions to the Go 2 review meeting are:

  1. Has the requirement to implement error fomatting been abandonned for good? If so, why?
  2. Is the concern about excessive string allocation shared by the go team?
  3. Do you agree that the above can't be achieved without breaking changes, and that further adoption of the current standard contributes to making the breaking change bigger?

Full disclosure, I have raised this concerns before in the official proposal issue and my own counter-proposal (and am grateful for the time go team members did spend on them), but I failed to highlight these concerns with sufficient clarity.
I am also addressing the Go 2 review meeting specifically, since I believe the above are not of concern to some memeber of the go team, but I'd like to know if that is the consensus.
And, in passing, thanks to the go team for introducing the Go 2 review meeting ❤️ - it provides much appreciated visibility and accountability!
@cagedmantis cagedmantis changed the title Proposal concerns: Go 2 error values proposal: Go 2: concerns about error values Dec 2, 2019
@gopherbot gopherbot added this to the Proposal milestone Dec 2, 2019
@ianlancetaylor
Copy link
Contributor

CC @jba @neild

It seems to me very unlikely that we would make any breaking changes in error values at this point.

@neild
Copy link
Contributor

neild commented Dec 2, 2019

It would be nice to figure out some form of improvements to formatting error chains. Any such improvements cannot be a breaking change. We do not have any specific proposals on the table at this time.

Errors have a user-facing component (the string returned by the Error method) and a semantic component (everything else). The Unwrap method applies strictly to semantic errors; it has no influence on the user-facing component of an error, which is still represented as always by the result of the Error method. Error implementations are free to return whatever they want from the Error method, and to make whatever optimizations they see fit in the construction of the returned string.

To address the specific questions (although I'm not the review meeting, and have no official status here):

  1. We have no current proposals for changes to error formatting. I personally think that there is scope here for something, but that it should be first proven outside the standard library.
  2. I do not believe anything in Go's current error design requires excessive string allocation. It is possible to write code that allocates a lot; it is also possible to write efficient code.
  3. I do not believe we should make any breaking changes involving error values.

@JavierZunzunegui
Copy link
Author

JavierZunzunegui commented Dec 3, 2019 •
edited

Breaking changes are clearly the biggest concern. I'll expand on those.

I'll use a basic example: say there is an error chain of 4 errors, with .Error() being msg1 + ": " + msg2 + ": " + msg3 + ": " + msg4, we want to format it to use " - ", hence want msg1 + " - " + msg2 + " - " + msg3 + " - " + msg4. Say errors 1, 3 and 4 have some way of formatting themselves (keeping that abstract for now), but error 2 doesn't. Then the best we can do is msg1 + " - " + msg2 + ": " + msg3 + ": " + msg4 (mixing ":" and "-"), since error 2 can neither format itself nor call whatever format method error 3 has. Alternatively, the fomat-enabled errors can be made to skip non format-enabled errors, in which case the output is msg1 + " - " + msg3 + " - " + msg4 (msg2 is missing). Either way, the output is not the desired one.

Which brings me to my point about the breaking change: whatever new functionality is added to errors to support fomatting, it is not optional for wrapping errors. All errors implementing the new go1.13 Unwrap() errror pattern need changing (presumably prompted by the vet tool), and while this can be gradual, it is required before error fomartting is can be done properly. This was also an issue in the original error printing proposal (explanation), which I don't think is a coincidende, it just can't be done with Unwrap() error alone.

So my question is:
Would prompting for all errors with an Unwrap() error method to be updated (what to, still to be debated) be considered an acceptable breaking change?

Luckily, the interface was not named so hopefully it's adoption is limited, and those using it via golang.org/x/xerrors are using an experimental package and as such should tolerate breaking changes. But it still highlights my point that if we are to change it at all, better sooner than later.

@JavierZunzunegui
Copy link
Author

I should list, for completeness, that the other option is to rely (and make part of the contract) that Error() string returns ": " separated messages (for all wrapping errors) and use strings.ReplaceAll to format it otherwise. I hope we can agree this is a terrible way of doing it, though.

@ianlancetaylor
Copy link
Contributor

We can't make a change to go vet to require that errors that implement an Unwrap method also implement some other method. We could make such a change to golint.

@ianlancetaylor
Copy link
Contributor

There is no proposal here so I'm going to drop this out of the proposal process for now. Please feel free to continue discussing.

@ianlancetaylor ianlancetaylor changed the title proposal: Go 2: concerns about error values errors: how should formatting be handled? Dec 3, 2019
@ianlancetaylor ianlancetaylor added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed Proposal labels Dec 3, 2019
@ianlancetaylor ianlancetaylor modified the milestones: Proposal, Backlog Dec 3, 2019
@JavierZunzunegui JavierZunzunegui mentioned this issue Feb 3, 2020
Closed
@JavierZunzunegui
Copy link
Author

Concrete proposal put forward in #36994

@JavierZunzunegui
Copy link
Author

I released a library with the discussed features: https://github.com/JavierZunzunegui/zerrors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
4 participants
@neild @ianlancetaylor @JavierZunzunegui @gopherbot