-
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: check - a minimal error handling improvement for readability #58520
Comments
Fully understand that you are tired of error handling proposals @seankhliao :) - but I think it is unfortunate that this was closed so quickly as there are some differences from the other proposals you mentioned. The proposal in #49091 was: As for #52175 - @ianlancetaylor asked a couple of technical questions regarding how it should work - I think this proposal answers all such technical issues in the "Details" section above. The issue was then closed by the creator with this comment: #52175 (comment) without the issue being reviewed by the proposal team. I think this might have been premature as there seemed to be continuing discussion and interest after the closing of the issue and the "serious problem" referenced in that comment does arguably not apply to this proposal since the "check" keyword directly after the assignment operator makes it quite clear what is happening. I would also like to mention that the purpose of this proposal is not only to reduce boiler plate, but to make go code in general more readable and reduce potential bugs with shadowing/reassigned error variables. |
Thanks for your reply, I appreciate you taking your time. #49091 uses anonymous functions and function handlers everywhere, this proposal uses statement blocks. While the general idea is the same, there is a large difference in usability. It seems 49091 was rejected because of the issues in this comment: #49091 (comment). Of the 4 points in that comment only the first one is directly applicable to this proposal, the rest are fairly irrelevant because they are more about issues in the specific example code used in that proposal rather than the proposal itself. #32848 and #41908 both have the err variable on the left side of ":=" - which pollutes the namespace with the variable which is one of the main issues this proposal tries to solve. The main reason 32848 was closed was due to "The scoping issue is not resolved." (#32848 (comment)) - scoping is clearly defined in this proposal so that reason does not apply. In 41908 davecheney's critique was that the proposed change does not improve readability, in this proposal I provide an example of how readability is (IMO) improved by moving error handling inside blocks and spacially separating it from the core logic. @ianlancetaylor asked if the error blocks can be used on intermediary results and seemed to suggest that was not a good thing - in this proposal they can not. Yes, all of these proposals you mention are very similar, i totally agree - but when I read the discussions on them they are all closed because of arguments which do not necessarily apply to this proposal. Sometimes small differences make a difference - or a fresh perspective / convincing argument. My main argument is that this proposal would make go code in general more readable to an acceptable cost in increased complexity of the language specification. |
@mgustavsson Thanks for the proposal. This introduces a new kind of variable declaration and a new kind of code block. I'll reopen it since it's slightly different and we can see if people have comments. But it's quite unlikely that we would accept this as written. |
Thanks, appreciate it! Again the main argument is that this: func readFile(name string) ([]byte, error) {
f := check Open(name)
defer f.Close()
contents := check readContents(f); err {
return nil, fmt.Errorf("error reading %s: %w", name, err)
}
check validate(contents)
return contents, nil
} is more readable than this: func readFile(name string) ([]byte, error) {
f, err := Open(name)
if err != nil {
return nil, err
}
defer f.Close()
contents, err := readContents(f)
if err != nil {
return nil, fmt.Errorf("error reading %s: %w", name, err)
}
if err := validate(contents); err != nil {
return nil, err
}
return contents, nil
} while also always keeping err variables local to the error handling scope, thanks. |
Note that due to lexical semicolon insertion this: contents := check readContents(f); err {
return nil, fmt.Errorf("error reading %s: %w", name, err)
} is equivalent to this: contents := check readContents(f)
err {
return nil, fmt.Errorf("error reading %s: %w", name, err)
} So I'm not sure this syntax quite works. |
If semicolon is not an option i propose to use else instead (alternative 1b in the proposal): contents := check readContents(f) else err {
return nil, fmt.Errorf("error reading %s: %w", name, err)
} That being said, for loops and if statements use semicolons to mark optional statements before a block so maybe it is possible here too. |
This idea is very similar to #21161 (comment) (although as far as I know that was never filled out into a full proposal). |
It seems like the closest current form to what's being proposed here is the if err := validate(contents); err != nil {
return nil, err
} The form above already confines the I believe the following is the closest equivalent to the above example under your proposal: check validate(contents); err {
return nil, err
} This has:
I would agree that there's less boilerplate in this form than in the A typical reason I've heard for not using this Based just on the above I don't feel super convinced this is enough of an improvement to justify the exception in the language. I do agree that it is an improvement, but the improvement feels marginal by this criteria. I think you've buried the lede a bit in your proposal because what you suggested does introduce something new that's materially different from the typical I missed it on my first read because the example you chose for the I see two main ways to lay it out. The first would be to indent both the happy path and the error path so that neither appears in the parent scope: if contents, err := readContents(f); err == nil {
// happy path using "contents"
} else {
// error path using "err"
} I'm sure most would agree that the above is not idiomatic because we typically expect the happy path to be unindented and only the error branch to be indented. The other way is to hoist one of the variable declarations out of the var contents []byte
if contents, err := readContents(f); err != nil {
// error path using "err"
}
// happy path using "contents" Of course in practice I'd probably write this just like you wrote it in your original example, with the whole call on the line above the One thing I quite like about your proposal then is that it quite neatly handles this "scope splitting" goal, allowing contents := check readContents(f); err {
// error path using "err"
}
// happy path using "contents" I will say that the exact details of the syntax feel a little awkward to me -- it isn't super clear that the But I think it's probably more important to discuss what the desirable properties of a solution are than to debate the exact details of syntax, so I won't quibble too much about the syntax details. If it seems (to others in this discussion) like this "scope splitting" behavior is desirable then I'm sure there are various other possible syntax shapes to consider that would make different tradeoffs of terseness vs. similarity to existing language constructs. I'm also not super convinced that this new capability is sufficiently useful to justify the extra language complexity, but I could imagine myself getting used to this pattern and enjoying it if it were in the language. |
One issue with the old try as an expression proposal is that you can do
|
@ianlancetaylor Yes that comment seems the same as this proposal's "Alternative 2" with "::" instead of "?" and with the code block required and not optional. guard x, err := f() {
// err is in scope here
}
// but not here @carlmjohnson very similar to this, but with that syntax it seems confusing to me that x and err have different scopes.
@apparentlymart I think there is significantly less noise around the core logic here: check validate(contents); err {
return nil, err
}
// or just:
check validate(contents) than here: if err := validate(contents); err != nil {
return nil, err
} so IMO there is a fairly big readability difference, especially if you take into account that you could apply this in a lot of places. It could probably shrink many code bases by 10-20%. var contents []byte
if contents, err := readContents(f); err != nil {
// error path using "err"
}
// happy path using "contents" That will actually not work, it creates a new contents variable instead of assigning to the outer one: https://go.dev/play/p/oHMR4b0hS_j |
Just as an example, I noticed that @rsc posted the following code in this comment #58000 (comment): func tarAddFS(w *tar.Writer, fsys fs.FS) error {
return fs.WalkDir(fsys, ".", func(name string, d fs.DirEntry, err error) error {
if err != nil {
return err
}
if d.IsDir() {
return nil
}
info, err := d.Info()
if err != nil {
return err
}
h, err := tar.FileInfoHeader(info, "")
if err != nil {
return err
}
h.Name = name
if err := w.WriteHeader(h); err != nil {
return err
}
f, err := fsys.Open(name)
if err != nil {
return err
}
defer f.Close()
_, err = io.Copy(w, f)
return err
})
} To me, the sheer amount of error handling here obscures the core logic. With this proposal it could be written like this: func tarAddFS(w *tar.Writer, fsys fs.FS) error {
return fs.WalkDir(fsys, ".", func(name string, d fs.DirEntry, err error) error {
check err
if d.IsDir() {
return nil
}
info := check d.Info()
h := check tar.FileInfoHeader(info, "")
h.Name = name
check w.WriteHeader(h)
f := check fsys.Open(name)
defer f.Close()
_, err = io.Copy(w, f)
return err
})
} That is a reduction of non-empty lines in the function body of 52% (from 25 to 12). Not sure if it was intended to add any error wrapping to this code or not, with error handling applied the reduction in lines would be less dramatic (but still significant) - but readability would still be improved significantly IMO. |
@ianlancetaylor yes - the check statement in this proposal is a fairly large addition to the language. But improving error handling has been identified as one of the top priorities along with generics and the community has been looking for a solution for half a decade. Any viable proposal to error handling is going to be a major change. |
I like the implicit non The following would be a 'smaller' language change, could work with other types than func readFile(name string) ([]byte, error) {
f, check err := Open(name) {
return nil, err
}
defer f.Close()
contents, check err := readContents(f) {
return nil, fmt.Errorf("error reading %s: %w", name, err)
}
// the validate call is in the middle of a lot of boilerplate, impacting readability
// here a new err variable shadows the existing one, a potential for confusion/bugs
check err := validate(contents) {
return nil, err
}
return contents, nil
} Since it is impossible to confuse assignments and comparissons in declarations because you cannot compare a not declared variable. This could actually use the existing func readFile(name string) ([]byte, error) {
f, if err := Open(name) {
return nil, err
}
defer f.Close()
contents, if err := readContents(f) {
return nil, fmt.Errorf("error reading %s: %w", name, err)
}
// the validate call is in the middle of a lot of boilerplate, impacting readability
// here a new err variable shadows the existing one, a potential for confusion/bugs
if err := validate(contents) {
return nil, err
}
return contents, nil
} |
Thanks for the extra context! I think on first read I didn't notice the possibility of using And indeed you're right about my example of declaring a variable in the parent scope and then shadowing it in the |
@apparentlymart the ability to omit the error handling block could be dropped from this proposal if the consensus is that that is preferable. However, I think having the option to omit them is worthwhile - it is not realistic or even desirable to handle/annotate all errors multiple times at every level of the call stack. As an example, the code by Russ Cox I posted above exclusively used bare returns. Another interesting statistic I found was this: #31442 (comment) - according to this comment the number of annotated error returns in the kubernetes code base was 2,800 while the number of bare error returns was 30,000 ... |
Regarding how this would be integrated into the language grammar - there are of course several possible ways to do it depending on how many places you would want to allow checks to be used. Given that one of the criticisms of proposal #32437 was that try could be used in any expression and thus lead to very complicated code my suggestion would be the following:
";" would be replaced with "else" if that is considered preferable. From a grammar point of view it would be cleaner to put the check keyword first in the statement to be more similar to "if", "for" etc.. in that case the grammar would be:
This is not as clean from a readability standpoint IMO, but it would on the other hand make check always be first on the line so that it becomes as visible as return. |
As discussed, this is similar to other ideas that have been proposed before. While error handling remains something to consider, this change doesn't bring us enough benefit for the cost of the new syntax. It introduces a new keyword, a new declaration syntax, new control flow if there is no block, and an unusual use of semicolon or Therefore, this is a likely decline. Leaving open for four weeks for final comments. |
I like Pretty ofter I would like to use if a, err := test(); err != nil {
/// err here
}
// use a here but it's not possible without defining So by using check syntax it would be lovely to have a, b, c := check test(); err {
// error handling
}
// using a, b, c return values Also, instead of new keyword we could use check function, but it's not possible to make return from lambda like this: func readFile(name string) ([]byte, error) {
check := func(e err) {
break nil, fmt.Errorf("readfile: %w", e) // returns from the readFile
}
f, err := Open(name)
check(err) // returns from here if error
defer f.Close()
contents, err := readContents(f)
check(err) // returns from here if error
err := validate(contents)
check(err) // returns from here if error
return contents, nil
} In C you can use |
Why don’t make it as a directive ? like func readFile(name string) (content []byte, err error) {
//go:check err
f, err := Open(name)
defer f.Close()
content, err = func(fs *File) (f, []byte, err error) {
//go:check f, err
f, err = fs.Body()
}
//go:check err
err = validate(contents)
return
} Since |
No change in consensus. |
Author:
I have been programming in Go since before the 1.0 release, have also used C/C++, java and python and other languages professionally.
Background:
There have been many proposals regarding error handling, such as #32437 which this proposal have some similarities with. Most Go programmers agree that something could be done to improve error handling, but so far no proposal has been accepted.
What makes this proposal different:
In my opinion, current error handling in Go have these 3 problems:
Most error handling proposals have focused on solving problem 1 - this proposal aims to solve all 3 problems. If this proposal is not accepted (statistically likely given the history of error handling proposals :) I at least hope that it will influence other proposals to pay more attention to problem 2 and 3.
Example:
Let's take this example code which IMO illustrates the 3 problems mentioned above:
Here is how the code would look like with this proposal accepted:
Further discussion:
This proposal aims to:
Details:
This is obviously not up to language spec standard, but I have tried to address all the specific details I could think of:
The check keyword can only be used as a top-level statement in a code block or on the right hand side of a assignment statement.
The check keyword is followed by a function call which has a last return parameter of type error OR an expression which has type error.
The resulting values of a check statement on a function call is the remaining return parameters without the last error return parameter.
If the check statement is applied to an expression with type error there is no resulting values and it is a compile error to try to use it on the right side of an assignment statement.
The function call/expression can be followed by a semicolon, a variable name and a code block. If the error value that was extracted by the check statement is not nil, the variable name will be assigned the error value and the code block will be executed. The variable is local to the check statements code block. If the error value is nil, no assignment will happen and the code block is not executed.
If the check statement does not have a code block then it behaves like this:
Where ... is replaced with the zero values of the remaining return types of the enclosing function. If a return parameter in ... is a named returned parameter, the current value of that parameter is returned.
Omitting the error handling block is only allowed inside a function which has a last return parameter of type error.
Alternative 1a/b:
Check could be replaced with "try". The semicolon could be replaced with else.
Alternative 2:
Alternatively a postfix operator could be used instead of the "check" keyword:
Alternative 3:
Put the check keyword first on the line to simplify the grammar and maximize clarity of where the function can return.
The text was updated successfully, but these errors were encountered: