-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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: spec: handle errors with select #67316
Comments
In "Before Option 2" example, in the switch case clauses, I think the Other than this very minor issue: this seems to rather tackle mainly |
I believe this would break a large amount of existing code for very little appreciable gain. |
Added the returns. This change cuts down in verbosity in three key areas, making error handling easier to read and simpler to write.
|
@seankhliao please expound on how this would break existing code. I believe this is an entirely backwards compatible change. |
The improvement looks small, especially when you deal with lots of "library" code that sits somewhere inbetween of the whole stack, where other proposals were trying to improve: the if err != nil return fmt.Errorf pattern. Given your own highly appreciated examples, this slightly reduces the ErrorAs boilerplate, as the explicit var declarations are becoming implicit. It is probably difficult to have statistical data here, so my own experience is just a single, subjective data point: I rarely have need for ErrorAs, and in those situations I world not benefit from this proposal. But as I stress, my very subjective experience. |
It makes error handling much cleaner imo. Also, I updated the examples to better demonstrate the fmt.Errorf pattern and my solution there. |
This would no longer work: func main() {
var err error
switch err {
default:
fmt.Println("happy path")
}
} |
@seankhliao that’s a switch statement. My proposal involves select statements. |
It doesn’t overlap with a switch though. It has special behavior specific to errors only that you can’t get with a switch. I agree in some ways it may not be ideal, but it IS backwards compatible and once you’ve learned it I don’t think it is confusing at all. |
A simpler proposal is to define func handler(w http.ResponseWriter, r *http.Request) {
err := validateHeaders(r.Header)
var badRequestErr InvalidFieldsErr
switch {
case errors.Is(err, ErrUnprocessableEntity):
w.WriteHeader(http.StatusUnprocessableEntity)
return
case errors.As(err, &badRequestErr):
slog.Error("invalid headers", "fields", badRequestErr.Fields())
w.WriteHeader(http.StatusBadRequest)
return
case err?:
w.WriteHeader(http.StatusInternalServerError)
return
}
// handle happy path
} |
That covers about a quarter of the feature set that my proposal does |
This effectively makes the fmt package part of the language. Or, we have to carefully restrict the set of formats that can appear, which is confusing. |
Yes. Agreed we should be cautious doing that. I also had the thought that we could add a new language feature available only within the context of an error select where, similar to the template packages, a dot represents the current “selected” error value |
A considerable part of my Errorf's add more context information than just wrapping an error, so this would end up in the rabbithole ianlancetaylor points out. Add to that the existing code doesn't seem to get boosted, as visible in your examples. A generic error.As (ignore for the moment the name's taken) would be almost as good, if I'm not mistaken. |
Can you give me a concrete example of what additional context it is you're referring to? |
No idea what you mean by additional context, it's all in this issue. |
You said
Are you saying you typically interpolate additional values? Or what do you mean by “add more context information”? |
Interpolation. |
It is high time the Go team incorporates a "better" error/exception handling framework in Go. All the recent developer surveys atest to this dire need. I salute all the people who came up with proposals to this end, even if most if not all of them got rejected or even rebutted. How long more must Go programmers wait? Until the "IDEAL" error-handling framework comes up? My take is that rather than wait for the IDEAL, settle for one which is "not bad". It was the same with generics or parametric polymorphism. How long did it take for Ian, Griesmer, et. al. to finally come up an acceptable generics implementation in Go? Even Griesmer himself has said the current generics implementation does not entirely fit his ideal, but Go programmers everywhere have already embraced it! So, please! Don't wait for the "ideal" error handling framework for Go to appear. It won't! |
@susugagalala The Go team already tried it twice. The second proposal, which was an improvement on the first proposal based on feedback, was the most antagonized proposal in the history of Go proposals. You can find it by sorting the closed issues by most downvotes: #32437 By contrast, you can find the community's "counter proposal" (to "leave 'if err != nil' alone") by sorting the closed issues by most upvotes: #32825 So you can't blame the Go team for not wanting to repeat this experience. It's the Go community who really loves and defends the status quo. |
Appreciate the passion. Can you share your thoughts on the proposal itself though? |
This proposal is pretty interesting to me. I like that it focuses not on hiding the error handling but instead on establishing more explicit structure around it. My main reservation was already stated: using the It also uses a different syntax for declaration and assignment than the current The latter is only a small concern. The first is more concerning to me, as this situation does feel closer to being a Type switches already overload the EDIT: I shared a Putting specific syntax concerns aside, I want to state some conclusions I made from the proposal to confirm whether I understood it correctly, and perhaps to prompt further discussion if I didn't. In the In the err := validateHeaders(r.Header)
if err != nil {
if errors.Is(err, ErrUnprocessableEntity) {
// (this is the body of the first case, verbatim)
w.WriteHeader(http.StatusUnprocessableEntity)
return
}
{
badRequestErrPtr := new(InvalidFieldsErr)
var _ error = *badRequestErrPtr // (in other words: compile-time error if referent doesn't implement error)
if errors.As(err, badRequestErrPtr) {
badRequestErr := *badRequestErrPtr
{
// (this is the body of the second case, verbatim)
slog.Error("invalid headers", "fields", badRequestErr.Fields())
w.WriteHeader(http.StatusBadRequest)
return
}
}
}
{
// (this is the body of the default case, verbatim)
w.WriteHeader(http.StatusInternalServerError)
return
}
} So:
Does that seem like a correct interpretation of the proposal? Assuming I understood it correctly, I do like that it mainly just promotes existing error handling patterns to be a language feature that's harder to use incorrectly, rather than trying to hide the error handling altogether. I expect it would encourage broader use of the I feel broadly in favor of this, although I do think that overloading the meaning of |
your desugared example is accurate, as are all of your observations |
This introduces new syntax based on reuse of an existing keyword. It makes certain kinds of error handling cases slightly simpler. However, it doesn't address the biggest complaint that people have with error handling, which is the repetitive use of code like if err != nil {
return err
} It's not all that common for people to check whether an error satisfies a list of conditions. When that is needed, we already have syntax for it. The syntax here is slightly shorter, but overall this doesn't seem to be a significant enough improvement to justify the language change. Therefore, this is a likely decline. Leaving open for four weeks for final comments. |
Understood. It seems highly unlikely to me that we will ever get rid of explicit error checking for every error returned. To do so would almost inevitably lead to some kind of obscure syntax or new control flow— both of which have been reasons for many error handling proposals being declined. That being the case, it feels that the remaining options are those that leave error handling and explicit as is today, but provide marginal gains in simplicity or brevity. That is the goal of this proposal: provide a couple of small enhancements to error handling that hopefully, when added together, can alleviate distracting/noisy code from meaningful error handling |
No change in consensus, so declined. |
Go Programming Experience
Intermediate
Other Languages Experience
Go, Python, JS, C#, Java
Related Idea
Has this idea, or one like it, been proposed before?
There have been some that are probably similar, but I don't believe there have been any quite like this
Does this affect error handling?
Yes. It is different than other error handling proposal because it doesn't really cut down a ton on the "make error handling take less lines of code", but I do think it improves error handling.
Is this about generics?
No
Proposal
There have been many language changes proposed to improve error handling. Many of these have had syntax changes to hopefully get rid of the so frequently used
by adding some special syntax to quickly return. Such as
err?
. There have been a few common issues risen with these proposals:With this proposal, I aim to avoid all 4 of the above pitfalls. The proposal has three primary parts, all involving the enhancement of
select
statements. In my opinion, one of the issues with error handling in Go is that it is strictly a package level feature, not actually a language feature. This proposal would change that.I propose the
select
statement be expanded to accept one parameter which, if provided, must be a nil or non-nil value of a type which implements theerror
interface. Theselect
statement block is only entered if the error argument is non-nil. Its body syntax will be essentially identical to that of theswitch
statement, but behavior is a bit different for errors. Eachcase
can be one of two format, except thedefault
case which will have the same behavior and syntax as you would typically expect. The syntax for a case would either becase ErrIsValue: // case body
orcase varName as typedErr
where as is a new keyword, but only recognized in this specific context, and therefore still backwards compatible. Each case is evaluated in the order it is provided. If theas
keyword is absent, the behavior of matching the case is equivalent to checking whethererrors.Is(err, ErrIsValue)
== true. Similarly, if theas
keyword is present, it is functionally equivalent toerrors.As(err, &typedErr{})
Additionally, I propose a special "abbreviated"
select
syntax for errors that looks like thisselect err return "template string: %w"
, which behaves the same asselect err { default: return fmt.Errorf("detailed message: %w", err) }
.Okay, that's enough talk with no examples! Here's what it looks like before and after the change:
Before Option 1
Before Option 2
After
It can be argued that "Before Option 2" and "After" are not all that different. However, I think the code paths and cases are far easier in my proposal as each case is very simple and clear. Additionally, this proposal does more to make errors and error handling first class citizens in the Go language, rather than merely a package level feature with a built in interface. I believe cutting out the boiler plate will lead to people reaching more often to handle errors properly. Additionally, this proposal removes the need for
if err != nil {}
that bothers people so often, and provides a common syntax for handling errors in all cases.Is this change backward compatible?
I believe it is!
Cost Description
Implementation is probably somewhat costly. Runtime cost should be no different than what we have today.
Performance Costs
Compile time cost should be negligible. Runtime cost should be no different than today.
The text was updated successfully, but these errors were encountered: