Following the announcement of Go 2 design drafts related to error handling (a.k.a. "if err != nil { return err }" pattern), I tried to convert a fragment of real nontrivial code (with some censoring) to the new tentative syntax. (Originally I remembered this code as particularly tricky, but I think it was rewritten in the meantime to more straightforward, so it has less impact for me than I thought it would. Still, this is some real code full well.)
The old code looked like below:
| func (t *X) initializeAndActivate(ctx context.Context) error { | |
| t.logger.Info(fmt.Sprintf("activating X for transaction [%q] @%s", xxx.TransactionCodeFrom(ctx), t.millis())) | |
| t.bus.Lock() | |
| defer t.bus.Unlock() | |
| if t.fooing { | |
| return fmt.Errorf("trying to activate X for transaction %q while already active", xxx.TransactionCodeFrom(ctx)) | |
| } | |
| response, err := t.queryOnly(cmd_foo1) | |
| if err != nil { | |
| t.hardReset() | |
| return err | |
| } | |
| if response[0] != cmd_foo1[0] { | |
| t.hardReset() | |
| return fmt.Errorf("invalid response to FOO1 [% 02X]", response) | |
| } | |
| t.fooTable, err = parseFooTable(response[1:]) | |
| if err != nil { | |
| t.hardReset() | |
| return err | |
| } | |
| t.logger.Info(fmt.Sprintf("X parsed foo table [% 02x] as: %s", response, yyy.JsonString(t.fooTable))) | |
| err = t.queryAndExpect(cmd_foo2, cmd_foo2) | |
| if err != nil { | |
| t.hardReset() | |
| return err | |
| } | |
| err = t.queryAndExpect(cmd_foo3, cmd_foo3) | |
| if err != nil { | |
| t.hardReset() | |
| return err | |
| } | |
| err = t.queryAndExpect(cmd_foo4, cmd_foo4) | |
| if err != nil { | |
| t.hardReset() | |
| return err | |
| } | |
| t.logger.Info(fmt.Sprintf("activated X for transaction [%q] @%s", xxx.TransactionCodeFrom(ctx), t.millis())) | |
| t.fooing = true | |
| return nil | |
| } |
| func (t *X) initializeAndActivate_Go2(ctx context.Context) error { | |
| t.logger.Info(fmt.Sprintf("activating X for transaction [%q] @%s", xxx.TransactionCodeFrom(ctx), t.millis())) | |
| t.bus.Lock() | |
| defer t.bus.Unlock() | |
| if t.fooing { | |
| return fmt.Errorf("trying to activate X for transaction %q while already active", xxx.TransactionCodeFrom(ctx)) | |
| } | |
| handle err { | |
| t.hardReset() | |
| return err | |
| } | |
| response := check t.queryOnly(cmd_foo1) | |
| if response[0] != cmd_foo1[0] { | |
| check fmt.Errorf("invalid response to FOO1 [% 02X]", response) | |
| } | |
| t.fooTable = check parseFooTable(response[1:]) | |
| t.logger.Info(fmt.Sprintf("X parsed foo table [% 02x] as: %s", response, yyy.JsonString(t.fooTable))) | |
| check t.queryAndExpect(cmd_foo2, cmd_foo2) | |
| check t.queryAndExpect(cmd_foo3, cmd_foo3) | |
| check t.queryAndExpect(cmd_foo4, cmd_foo4) | |
| t.logger.Info(fmt.Sprintf("activated X for transaction [%q] @%s", xxx.TransactionCodeFrom(ctx), t.millis())) | |
| t.fooing = true | |
| return nil | |
| } |
Notably, the block in lines 15-17 was quite interesting. At first, I left it as:
if response[0] != cmd_foo1[0] {
t.hardReset()
return fmt.Errorf("invalid response to FOO1 [% 02X]", response)
}At this point, the t.hardReset() fragment was somewhat standing out in the flow of code,
feeling like something that I could easily forget to call when writing the code from scratch.
But then I remembered of seeing a check err pattern used somewhere in the design draft,
and realized I could use this here.
The resulting code is even more surprising.
I like that it introduces some reuse,
and hides the t.hardReset() as a "default behavior".
At the same time, I dislike two things about it:
return,
so I don't easily see it's a 100% sure exit point from the function;check Error("some message")?...
there's nothing to check here,
there's just an error to return
(and t.hardReset() to be called).
(That said, try fmt.Errorf(...) would have the same issue.)Personally, I'm not 100% sure what I think of the change.
Certainly, the code looks shorter and a bit more terse; no more "if err != nil" boilerplate. The block doing additional (substantial) verification in l.15-17 now stands out more, which seems a good thing.
On the other hand, the "handle" block looks a bit weird.
Now that I think of it,
it somehow feels to be missing symmetry with defer blocks syntactically —
while being most similar to defer blocks semantically
(more so than to if, switch, for, etc.
— as "handle" is not immediate as these are).
Also, I don't like that it feels like I have to do a bit more jumping around in the code with my eyes,
and/or carry more context in memory when reading and analyzing it
(namely, contents of the handle block).
But again on a positive note,
I can worry less if I haven't made a typo in one of the "if err != nil" blocks.
Any outstanding behaviors from the default handle contents would have to be made explicit now.
I found another fragment of code with non-trivial error handling.
However, I didn't manage to find a reasonable approach how to apply handle & check to "if err != nil" blocks there.
I don't know if it's bad or good,
just reporting.
(There are two places with "if err != nil": l. 13 and l. 27.
Both of them seem to add significant new context to the returned message.)
Maybe the "error values" design draft could help here somehow?
func (b *Bus) readAndUnpack() ([]byte, error) {
n, err := io.ReadAtLeast(b.port, b.buf[:], 2)
got := b.buf[:n]
if err != nil {
return nil, newError("xxx: cannot read 2-byte preamble [got: % 02X] - error: %s", got, err)
}
if got[0] != magic_number {
return nil, newError("xxx: bad MAGIC NUMBER in response - message starts with: [% 02X] (expected XX...)", got)
}
length := int(got[1])
if length < 5 {
return nil, newError("xxx: response too short: len=%d < 5 [% 02X]", length, got)
}
// Now that we know the total length, we can read the remaining bytes of the response
if n < length {
n, err = io.ReadAtLeast(b.port, b.buf[n:], length-n)
if err != nil {
return nil, newError("xxx: cannot read remaining bytes of a packet [prefix=% 02X][rest=% 02X] - error: %s",
got, b.buf[len(got):][:n], err)
}
got = b.buf[:len(got)+n]
}
crc := crc(got[:length-2])
if crc != [2]byte{got[length-2], got[length-1]} {
return nil, newError("xxx: bad CRC [% 02X], expected [...% 02X]", got, crc[:])
}
payload := append([]byte(nil), got[2:length-2]...)
return payload, nil
}One extra detail I realized I forgot to mention before,
is that the first example uses an error library based on log15.v2.
This is important because the errors created with this library automatically collect stack trace information.
This makes the return err lines so trivial in the original example.
And apparently, this makes it work cleanly with check+handle.
The second example uses more "classic Go" approach,
where if an error is passed,
an extra message with some context and description is prepended to it.
The message tends to be different on each line,
so that detailed information about the current phase of the operation can be described with appropriate wording.
Surprisingly, this "classic Go" approach seems to make the code not work well with check+handle.
Trying to add a separate handle block before each newError would seem to explode the line count and increase messiness.
The "detailed message" as a string, a.k.a. "classic Go" approach, has pros and cons.
It's nice that it doesn't depend on particular line in source code,
so that searching for a message in codebase will usually show correct location even if the code was modified.
But it also makes the final error message potentially super long and overdetailed,
if the error object passed through a stack of numerous functions.
It could be valuable to consider how this could be improved,
possibly by using some mechanisms proposed in the "error values" design draft.
Maybe the check and/or handle keyword could be extended,
so that a list of "extra details" to be added to the "error value" could be listed by user?
Also, I had a thought regarding stack traces and their performance impact.
What if the default handle block would add just the information (PC) about only the current stack frame,
as an "error detail"?
In such case, the building of the stack trace would be potentially amortized over the whole call stack.
Early capture and discarding of the error could maybe reduce the overhead in such case,
by limiting the number of analyzed stack frames.
One disadvantage of this approach,
is that this would make the default handle block quite complex,
which doesn't feel very Go-like to me.
@pborman Excellent point
Sorry for a late reply, github didn't seem to notify me of comments here! (Or did I just overlook them? :/ it's first time I'm publishing a gist like this...)
@pborman Interesting, thanks! (I tend to avoid named return values, at least at my workplace we kinda have a policy against them for now, as we felt they're somewhat tricky. Now I see this results in me having little experience with them and what they enable.) Does it mean that handle err is essentially equivalent to the pattern you show — with one crucial exception being that handle is lexically scoped, while defer is function scoped?
A thought experiment, with a tentative working syntax // assert: (it is in comment and in column 1 to make it fade out with current syntax highlighter on GitHub). I've been having thoughts on "how to make code show more of the algorithm essence" for some time, this is one current exploration of what I've found interesting to ponder upon:
func (b *Bus) readAndUnpack() ([]byte, error) {
n, err := io.ReadAtLeast(b.port, b.buf[:], 2)
got := b.buf[:n]
// assert: err == nil, newError("xxx: cannot read 2-byte preamble [got: % 02X] - error: %s", got, err)
// assert: got[0] == magic_number, newError("xxx: bad MAGIC NUMBER in response - message starts with: [% 02X] (expected XX...)", got)
length := int(got[1])
// assert: length >= 5, newError("xxx: response too short: len=%d < 5 [% 02X]", length, got)
// Now that we know the total length, we can read the remaining bytes of the response
if n < length {
n, err = io.ReadAtLeast(b.port, b.buf[n:], length-n)
// assert: err == nil, newError("xxx: cannot read remaining bytes of a packet [prefix=% 02X][rest=% 02X] - error: %s", got, b.buf[len(got):][:n], err)
got = b.buf[:len(got)+n]
}
crc := crc(got[:length-2])
// assert: crc == [2]byte{got[length-2], got[length-1]}, newError("xxx: bad CRC [% 02X], expected [...% 02X]", got, crc[:])
payload := append([]byte(nil), got[2:length-2]...)
return payload, nil
}
I don't know why exactly they want to change it anyway. if err != nil boilerplate can get in the way sometimes, yes, but nothing beats its simplicity and precision. And as admitted by the proposal draft, "It's easy to skip" while reading code. The best thing about current (Go1) error handling is that it doesn't introduce any type of magic flow like exception handling does in other languages. Go creators were, obviously, against that kind of flow initially, but now they want to add it in. Again, why, exactly?
I think it's to allure more people who are familiar with such (try/catch) flow from other languages, hoping that it'll boost the language adaptation some more. But they also don't like to admit this, which makes the change possibly worse. I mean try/catch? check/handle? Come on!
In your original code, if you name your return value, after line 8, you can have:
At this point you don't need a handle at all. Named return values and defer are very powerful. I then see
checkas a way to eliminate, errandif err != nil { return err }from the code, and nothing more.