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: flag.failf should return an error with Cause() error #18303

Closed
tejasmanohar opened this issue Dec 14, 2016 · 14 comments

Comments

@tejasmanohar
Copy link

commented Dec 14, 2016

It would be very nice if flag.failf returned an error with a Cause() so that if you define your own flag.Value-implementing type, you don't lose context on errors you return out of its Set function here.

I would definitely be willing to open a patch if whoever's in charge agrees on the design. :)

@bradfitz bradfitz added the Proposal label Dec 14, 2016

@bradfitz bradfitz added this to the Proposal milestone Dec 14, 2016

@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 14, 2016

I suspect we'd want some overall design & plan for error provenance in the standard library & general recommendations for the community rather than just making changes ad hoc.

Related: #16968 and #15192, both about errors in a general sense.

@tejasmanohar

This comment has been minimized.

Copy link
Author

commented Dec 14, 2016

@bradfitz Thanks for the quick response! (@everyone) Are there any reasons against exposing Cause() error? It would be backwards-compatible-- just an assertable interface.

From skimming the other discussions, I get the sense that the Go team wants to hold off on refactoring any error provenance in the standard library until it's fixed everywhere, but for a standard library of this size, I don't think that's practical. Instead, I suggest analyzing what makes sense on a case-by-case basis (e.g. this issue), incrementally introducing non-breaking improvements, and shaping a "standard" around what proves worthy in Go2.

@tejasmanohar

This comment has been minimized.

Copy link
Author

commented Dec 14, 2016

Are there any reasons against exposing Cause() error?

To clarify, I don't mean we should add something just because it has no downsides-- that's a recipe for creating a mess of a standard library... but I want to focus on what's wrong with this proposal (e.g. doesn't provide enough value to go in stdlib, encourages bad design, not readable, etc.) rather than the error handling everywhere in the standard library.

@davecheney

This comment has been minimized.

Copy link
Contributor

commented Dec 14, 2016

Are there any reasons against exposing Cause() error?

What do you mean exposing? If this method already exists on the type then there is nothing to do. If the method does not exist, then that's a bigger ask and out of scope for Go 1.8.

@dsnet

This comment has been minimized.

Copy link
Member

commented Dec 14, 2016

The challenge is that this proposal is trying to add a method to a value of type error and the only way for anyone to know about its existence is if they type assert for it. There's a growing precedence in the Go community for Cause() error to be a special method on error, but this proposal is probably not the place to have that discussion.

@tejasmanohar

This comment has been minimized.

Copy link
Author

commented Dec 14, 2016

I was suggesting that the flag package returns a struct that implements non-standard Cause() error method in addition to the normal error interface.

The challenge is that this proposal is trying to add a method to a value of type error and the only way for anyone to know about its existence is if they type assert for it.

It can go in docs as well?

There's a growing precedence in the Go community for Cause() error to be a special method on error, but this proposal is probably not the place to have that discussion.

Agreed that this is not the place to talk about Cause() error as a special method, but I think it could fit well on errors returned by the flag package when it wraps errors from a flag.Value. If the Go team is considering making Cause() error special, we should start by looking at where it can be used (e.g. this one).

Curious- If this is not the right place, what is the right place? I understand that this may be out of scope for Go 1.8, but I'd still like to propose it... maybe for 1.9?

@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 19, 2016

Unless and until this becomes an accepted soft standard for errors, we should probably not start adding it to the standard library throughout, and we also shouldn't add it piecemeal. Like @bradfitz said above, the situation is far from settled.

Declining until that changes. (And if it does change, and we decide that this is something we want everywhere, we'll probably just do it, without needing to reopen this proposal.)

@tejasmanohar

This comment has been minimized.

Copy link
Author

commented Dec 19, 2016

@rsc I understand your concern about adding this piecemeal. My proposal is that the Go standard library should return a struct that satisfies Error() string and Cause() error in all cases where an error that the user (e.g. me writing Go code) is re-thrown. One example of this is flag.FlagSet.Parse([]string) error re-throwing the error returned by Set(string) error from a custom, user-provided flag.Value.

What does the Go team think of this conceptually? I understand that, in the end, this is out of my control, but what's missing / how can I help convince the Go team that this is important?

@tejasmanohar

This comment has been minimized.

Copy link
Author

commented Dec 19, 2016

Without this feature, stdlib expects users to either

  • inspect the result of Error() string (eww!); or
  • store the original error from Set() (and similar methods) in some other place... which eliminates the purpose of the error return value (could as well be a pass/fail bool)
@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 19, 2016

I understand the general problem. We should be sure of how we want to solve that before we start editing package flag. Maybe 'Cause() error' is not the solution. Until we know the solution, we shouldn't add it to package flag.

@rsc rsc closed this Dec 19, 2016

@tejasmanohar

This comment has been minimized.

Copy link
Author

commented Dec 19, 2016

@rsc Got it.

Maybe 'Cause() error' is not the solution. Until we know the solution, we shouldn't add it to package flag.

Where does discussion about that take place? What are next steps?

@tejasmanohar

This comment has been minimized.

Copy link
Author

commented Dec 20, 2016

To expand, I think it's important to have a place for open discussion about error handling in the standard library since the Go team recognizes the general problem but isn't sure what the right solution is yet. As I mentioned here, one exists for generics, which is in a similar boat, except almost certainly requires large language changes and breaking changes in general. I was hoping this issue would serve that purpose, but I'd be happy to open another one (or a mailing list thread, etc.)

Maybe 'Cause() error' is not the solution. Until we know the solution, we shouldn't add it to package flag.

^ The aforementioned issue would be a good place for pros/cons of Cause() error and other solutions, as it's still unclear to me what's wrong with the proposed solution :(

@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2016

I don't think we're ready to have a conversation about errors right now. It's on my list for next year, hopefully before the summer.

@tejasmanohar

This comment has been minimized.

Copy link
Author

commented Dec 20, 2016

Alrighty

@golang golang locked and limited conversation to collaborators Dec 20, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants
You can’t perform that action at this time.