Hacker Newsnew | past | comments | ask | show | jobs | submitlogin

I think this is the way to bubble up error messages that I like the most. Simple, not needing any additional tools, and very practical (sometimes even better than a stack trace).

The idea is to only add information that the caller isn't already aware of. Error messages shouldn't include the function name or any of its arguments, because the caller will include those in its own wrapping of that error.

This is done with fmt.Errorf():

    userId := "A0101"
    err := database.Store(userId);
    if err != nil {
        return fmt.Errorf("database.Store({userId: %q}): %w", userId, err)
    }
If this is done consistently across all layers, and finally logged in the outermost layer, the end result will be nice error messages with all the context needed to understand the exact call chain that failed:

    fmt.Printf("ERROR %v\n", err)
Output:

    ERROR app.run(): room.start({name: "participant5"}): UseStorage({type: "sqlite"}): Store({userId: "A0101"}): the transaction was interrupted
This message shows at a quick glance which participant, which database selection, and which integer value where used when the call failed. Much more useful than Stack Traces, which don't show argument values.

Of course, longer error messages could be written, but it seems optimal to just convey a minimal expression of what function call and argument was being called when the error happened.

Adding to this, the Go code linter forbids writing error messages that start with Upper Case, precisely because it assumes that all this will be done and error messages are just parts of a longer sentence:

https://staticcheck.dev/docs/checks/#ST1005



> This message shows at a quick glance which participant, which database selection, and which integer value where used when the call failed.

And it's completely useless for looking up the errors linked to a participant in an aggregator, which is pretty much the first issue the article talks about, unless you add an entire parsing and extraction layer overtop.

> Much more useful than Stack Traces, which don't show argument values.

No idea how universal it is, but there are at least some languages where you can get full stackframes out of the stacktrace.

That's how pytest can show the locals of the leaf function out of the box in case of traceback:

      def test_a():
  >       a(0)
  
  test.py:11: 
  test.py:2: in a
      b(f)
  test.py:5: in b
      c(f, 5/g)
  
  f = 0, x = 5.0
  
      def c(f, x):
  >       assert f
  E       assert 0
  
  test.py:8: AssertionError
and can do so in every function of the trace if requested:

      def test_a():
  >       a(0)
  
  test.py:11: 
  f = 0
  
      def a(f):
  >       b(f)
  
  test.py:2: 
  
  f = 0, g = 1
  
      def b(f, g=1):
  >       c(f, 5/g)
  
  test.py:5: 
  
  f = 0, x = 5.0
  
      def c(f, x):
  >       assert f
  E       assert 0
  
  test.py:8: AssertionError
So this is just a matter of formatting.


You have to add all of that detail manually which sucks. You can get the function name from the runtime package and generate that metadata easily with a helper function. Otherwise when you rename the function, you have to rename all of the error messages.


I agree except for the part about using strings, as you lose structure they way. You should instead return structured errors:

    return CouldNotStoreUser{
      UserID: userId,
    }
and now this struct is available to anyone looking up the chain of wrapped errors.


This is covered by the “ergonomics” section of TFA:

> With custom error structs however, it's a lot of writing to create your own error type and thus it becomes more of a burden to encourage your team members to do this.

Because you need a type per layer, and that type needs to implement both error and unwrap.


In practice, I have found the benefit of explicit typing to outweigh the downsides of needing to declare the types.

As a concrete example, it means you can target types with precision in the API layer:

    switch e := err.(type) {
      case UserNotFound:
        writeJSONResponse(w, 404, "User not found")
      case interface { Timeout() bool }:
        if e.Timeout() {
          writeJSONResponse(w, 503, "Timeout")
        }
    }
I skimmed the article and didn't see the author proposing a way to do that with their arbitrary key/value map.

Of course, you could use something else like error codes to translate groups of errors. But then why not just use types?

But as I suggested in my other comment, you could also generalize it. For example:

    return meta.Wrap(err, "storing user", "userID", userID)
Here, Wrap() is something like:

    func Wrap(err error, msg string, kvs ...any) {
      return &KV{
        KV:    kvs,
        cause: err,
        msg:   msg,
      }
    }
This is the inverse of the context solution. The point is to provide data at the point of error, not at every call site.

You can always merge these later into a single map and pay the allocation cost there:

    var fields map[string]any
    for err != nil {
      if e, ok := err.(*KV); ok {
        for i := 0; i < len(KV.KV); i += 2 {
          fields[e.KV[i].(string)] = e.KV[i+1]
        }
      }
      err = errors.Unwrap(err)
    }


You can also just assign your errors to variables, and typically you only need to do so for the innermost error. Wrapping calls can just use Errorf with the %w operator.

Horses for courses.


err.(type) fails when errors are wrapped, which is common and needs to be accommodated. To do what you're trying to do, you need to use errors.As.


Sure. It was not intended to be a complete example.


It's not that the example is incomplete, it's that it's incorrect! :)


No, it's just incomplete. The same code just needs to unwrap:

    for err != nil {
      switch e := err.(type) {
        case UserNotFound:
          writeJSONResponse(w, 404, "User not found")
          return
        case interface { Timeout() bool }:
          if e.Timeout() {
            writeJSONResponse(w, 503, "Timeout")
            return
          }
      }
      err = errors.Unwrap(err)
    }


`err.(type)` is incorrect, at least in general. Calling `errors.Unwrap` in application code like this is almost always a red flag indicating a design error. And in this case it definitely is!


That is literally what errors.As() does, finding a value up the cause chain that can be coerced to the target type.


If you're re-implementing errors.As's unwrapping behavior in your application code in order to parse/evaluate errors, that's a mistake and a design error. You'd never call Unwrap outside of a custom error type's method set, and even then you'd never use a `for` loop like you're doing in your example.

> it means you can target types with precision in the API layer

The only situation where you need to get precise error types is when you need to provide specific details from those specific types to the consumer, which is rare. And even in those rare cases, user code does that work via errors.As, not this manual Unwrap loop process you're suggesting here.


> The only situation where you need to get precise error types is when you need to provide specific details from those specific types to the consumer, which is rare.

It's not rare in my experience. All they apps I work on have a central unhandled error handler in the API that converts Go errors to HTTP or gRPC error responses, and then falls back to a general "internal error" if no specific error could be mapped. I can think of many other instances where we have a switch over half a dozen error typed in order to translate them into other types across RPC or pub/sub boundaries.

> And even in those rare cases, user code does that work via errors.As, not this manual Unwrap loop process you're suggesting here.

As() does not work with switch statements unless you pre-declare a ton (in our case, often a couple of dozen) error variables. Secondly, it is deeply inefficient. As() traverses the cause tree recursively for every single error, so if you have 30 possible error types to compare, and an error typically wraps 3 layers deep, that's a worst case of 30 loop iterations with 90 cases, as opposed to my method, which is 3 loops.


> t is deeply inefficient. As() traverses the cause tree recursively for every single error, so if you have 30 possible error types to compare, and an error typically wraps 3 layers deep, that's a worst case of 30 loop iterations with 90 cases ...

I have no idea how you came to this conclusion. It's certainly not what happens when you call errors.As in your application code.

There's no situation where your application code would ever have 30 error types to compare against, if that were ever the case you have seriously fucked up!


If you do this:

  var a, b, c error1, error2, error3
  switch {
    case errors.As(&a):
      ...
    case errors.As(&b):
      ...
    case errors.As(&c):
      ...
  }
…then yes, you will be doing 3 searches, each of which will do a loop (sometimes recursively if Unwrap() returns []error) over the chain of causes.

> There's no situation where your application code would ever have 30 error types to compare against, if that were ever the case you have seriously fucked up!

That is your opinion. In my experience, that is not the case, because there are lots of cases where you want to centrally translate a canonical set of errors into another canonical set.

> It literally is not. Is and As are not merely convenience functions, they're canonical …

This is just your opinion. If you actually read the documentation, you will see that it merely says Is() and As() are "preferable" to checking.


Any code that looks like that is almost certainly broken. If the things you're describing as "my opinion" are counter-indicated in the code that you're used to working with, then you're working with code that's seriously unconventional. Do with that feedback what you will.


You're wrong. You've not offered any evidence for why this is not just your opinion, and your claims are easily contradicted by examples.

As an example, say we have an API implemented on top of a complex data store. Every data store implementation can return errors like ObjectNotFound, InsufficientPermissions, and a dozen others. Every data store call can potentially return these. As well as, of course, standard Go errors like DeadlineExceeded or internal errors that cannot be exposed as user-facing API responses. However, some translation error has to translate those errors into API responses.

This cannot conveniently and consistently be done in each API handler, as it would repeat the same error translation for the same errors. An InsufficientPermissions error may happen in a "create" route as well as an in a "update" route, but also in any other route that deals with objects not being accessible.

Therefore it must be done in a central error translator. By definition. And this translation must either do a dozen+ Is() and As() calls, or it can be done efficiently, as I've described.

Anyway, I've said all I have needed to say and won't respond any further.


> it must be done in a central error translator. By definition. And this translation must either do a dozen+ Is() and As() calls, or it can be done efficiently, as I've described.

These claims are, bluntly, incorrect. There are no widely-used modules that work this way, and there are no properties of the language or its conventions that would suggest that this is a viable way to design an API. errors.Is and errors.As provide capabilities that type assertions -- as you've described -- factually do not provide. They're not equivalent, they're not normally used, they're not anything other than red flags in bad code that should be eliminated.

I'm not trying to pick a fight with you, I'm honestly just trying to prevent other people, reading this comment thread, from making the kinds of design mistakes that you're describing here as viable and efficient. They truly aren't.


That is just your opinion. There is absolutely nothing wrong with doing so, and there is nothing in the documentation that asserts what you claim.

The documentation is clear that comparing an error value or casting it without following the Unwrap() chain is only an antipattern because it would not work with wrapped errors.

Is() and As() are merely convenience functions, and the documentation is clear that all they're doing is calling Unwrap(), which you can do yourself.


It literally is not. Is and As are not merely convenience functions, they're canonical and conventional expectations of Go code, if you're doing that work yourself you're almost certainly doing it wrong.




Guidelines | FAQ | Lists | API | Security | Legal | Apply to YC | Contact

Search: