6 ms·

there’s really no reason this has to be 3 lines if err != nil { return err } i’m hoping they find a way to simplify this

preseinger 5y ago
Returning an unannotated error like this is an antipattern. Every error return should include an annotation via fmt.Errorf.
- nkozyra 5y ago
  Why? Errors are annotated by definition. I wouldn't feel better about returning my own string versus the error which already includes a message. How much additional context needs to be there and how often is this convention used by simply duplicating the message with basically repetitive text? panic(err) become "Problem with date formatting: invalid date format" or similar I also think "antipattern" gets thrown around too often. This sounds more like a preference or convention.
  - srer 5y ago
    I presume as a method of manually constructing backtrace, as the same error may occur in multiple places it's helpful to understand the context. I too would label it a preference. To get out of the error handling tedium in our platform I largely opt to panic instead whenever viable, which gives a nice trace for free. (I am but human, and the error handling particularly grates once you have gotten used to just typing `?`.)
    - nkozyra 5y ago
      I also panic whenever possible from the caller, in which case I get a pretty clear stack trace. I understand you don't always want that but imo I don't have a lot of cases where I'm ultimately throwing my hands up on a non-exception error. I think V2 errors are taking more of a lead from the pkg/errors error.Wrap approach anyway.
      - preseinger 5y ago
        pkg/errors.Wrap is already accomplished in the stdlib via fmt.Errorf("annotation: %w", err). Programs which use `panic` as ersatz error mechanisms are fundamentally broken. `panic` expresses an invariant violation that's much different than normal errors.
    - preseinger 5y ago
      `panic` is not equivalent to `return err`. It expresses a much more fundamental problem than an error return, and subverts the ability of the reader to model execution control flow. `panic` should essentially never be used in application code, and when it is used it should almost always immediately terminate the program.
      - srer 5y ago
        Programming is a vast field, "essentially never" is quite a strong statement. For many errors, in many situations, terminating the process is quite reasonable. In my particular situation, the greater system will restart failed processes, and retry failed tasks. I find this useful as in many cases my program can just die when something weird happens, simplifying it's own logic.
        preseinger 4y ago
        This represents a false economy, or maybe a local optimum. It's lovely that your code can be simple in the sense that it can assume all kinds of invariants that, if violated, will simply terminate the execution, which can safely be assumed to start up again anew. But it's decidedly not lovely that you can no longer predict what effect an input will have on your code, and can't effectively reason about, well, anything beyond a trivial lifetime/callstack. If your process dies whenever something weird happens, it effectively becomes nondeterministic -- your greater system model has to assume it can die at any instant for any reason.
        srer 4y ago
        > your greater system model has to assume it can die at any instant for any reason Correct. This is something I have to design for in the system anyway, because in practice anything can (and does!) die at unpredictable times. It's typically an inevitable fact of life that a machine/kernel/program will occasionally die, and your system has to survive that.
        preseinger 4y ago
  - preseinger 5y ago
    Why do you believe errors are annotated by definition? They aren't? Panic isn't an ersatz error mechanism.
    - nkozyra 5y ago
      Pedantry on my part, but annotated does not mean including a stack trace, simply that it includes a description of the error. Given it's a non-nil check, you never gave a guarantee it's valuable, but the error is itself at minimum an annotation. panic brings important context, but you're right in that it's not an annotation in itself. It's a program flow mechanism, but I'd argue it's very often utilized as an error flow one. My larger point was that this still doesn't feel like an "antipattern" and I see that word thrown around enough as a conversation stopper that I've become pretty cynical about it.
      - preseinger 5y ago
        "Annotated" means given `err error`, you return `fmt.Errorf("annotation: %w", err)` — nothing more or less.
  - cube2222 4y ago
    Keep in mind that the usefulness of stack traces quickly breaks down in the presence of goroutines.
- jhoechtl 4y ago
  that doesn't add much value but make it more easy to identify the offending place where the error occurred. What would be great is a unified way to add context in a standard and automated manner, like a stack trace.
- foolfoolz 4y ago
  you only need to wrap errors that you did not raise yourself. if your codebase already annotates an error you hopefully are annotating well enough that it’s identifiable. library code you didn’t write is unknown and needs help. wrapping every error is even more verbose and repetitive
nxm 5y ago
Explicit is better than implicit
- didibus 5y ago
  I think that was the comment about Java checked exception, it's one line and it's explicit. Though personally I feel in this case defaulting to rethrowing uncaught errors is better. Since that's the 99% case. I'd rather it be zero line.
  - throwaway894345 5y ago
    Unless something has changed, Java checked exceptions aren't explicit--it's not generally not possible to tell whether a given function call can raise an exception without inspecting the signature of that function. > Though personally I feel in this case defaulting to rethrowing uncaught errors is better. Since that's the 99% case. I'd rather it be zero line. This is ambiguous with the "doesn't error at all" case. If you're looking at source code `foo()` you can't tell whether that's equivalent to `if err := foo(); err != nil { return err }` or just `foo()`. You have to check the function signature to see what the return arguments are (or in Java's case, whether or not it throws).
    - avgcorrection 4y ago
      The error being part of the method signature seems very explicit, no? You will get a compiler error if you don’t handle it in some way in the client code. Unchecked exceptions are not explicit.
      - throwaway894345 4y ago
        I suppose it becomes part of the signature of the caller, so maybe? I guess I was thinking "locally at the call site, I don't see anything corresponding to `if err != nil { return err }`", but implicit vs explicit error handling probably isn't well-defined, so I suppose it's up to interpretation. I usually think of it more in a local context ("is it evident right at the call site") but the caller's function signature is certainly local-ish. (shrug)
throwaway894345 5y ago
No idea what people have against newline characters, but `if err != nil { return err }` is valid Go code.
- pstuart 5y ago
  go fmt will change that, and code should be uniformly formatted. Go proverbs: "Gofmt's style is no one's favorite, yet gofmt is everyone's favorite."
  - throwaway894345 5y ago
    agreed, but then what’s the issue?
heavyset_go 5y ago
In Rust, that could be a Result::unwrap() or propagated using the ? operator.
dmustillo 5y ago
I struggled with this a lot. Drove me nuts we couldn't get decent code coverage because of error handling for errors I don't even know how to replicate in the first place. Plus once your code throws one error, every bit of calling code also needs to handle that error. The problem cascades through a codebase quickly. Seemed like a huge violation of DRY principles. Rob Pike (my understanding as one of the main guys who created the language) has actually addressed this, it's a good read: https://go.dev/blog/errors-are-values https://go.dev/blog/errors-are-values Tldr refactor your error handling to treat it like code. DRY and SOLID principles would apply and etc. Article makes example of handle your errors in one place rather than 20 by using no-ops on remaining operations after an error occurs. I don't actually agree with the choice, as it takes one key library which throws errors at every call (like I'm dealing with now) for this to just become a huge pain to do. I had to completely change business logic to implement his suggestion, which isn't always viable (and I'm subsequently finding that out that that wasn't completely viable for us first hand now). Also a lot more boilerplatey type no value add type code needs to be written. I much prefer unchecked exceptions for the most part, but at least I can understand WHY error handling is the way it is in Go.
- stouset 4y ago
  Approximately 0% of code bases do this in golang practice. I would be floored if someone provided an example of a large project that does.

preseinger 5y ago

Returning an unannotated error like this is an antipattern. Every error return should include an annotation via fmt.Errorf.

nkozyra 5y ago
Why? Errors are annotated by definition. I wouldn't feel better about returning my own string versus the error which already includes a message. How much additional context needs to be there and how often is this convention used by simply duplicating the message with basically repetitive text? panic(err) become "Problem with date formatting: invalid date format" or similar I also think "antipattern" gets thrown around too often. This sounds more like a preference or convention.
- srer 5y ago
  I presume as a method of manually constructing backtrace, as the same error may occur in multiple places it's helpful to understand the context. I too would label it a preference. To get out of the error handling tedium in our platform I largely opt to panic instead whenever viable, which gives a nice trace for free. (I am but human, and the error handling particularly grates once you have gotten used to just typing `?`.)
  - nkozyra 5y ago
    I also panic whenever possible from the caller, in which case I get a pretty clear stack trace. I understand you don't always want that but imo I don't have a lot of cases where I'm ultimately throwing my hands up on a non-exception error. I think V2 errors are taking more of a lead from the pkg/errors error.Wrap approach anyway.
    - preseinger 5y ago
      pkg/errors.Wrap is already accomplished in the stdlib via fmt.Errorf("annotation: %w", err). Programs which use `panic` as ersatz error mechanisms are fundamentally broken. `panic` expresses an invariant violation that's much different than normal errors.
  - preseinger 5y ago
    `panic` is not equivalent to `return err`. It expresses a much more fundamental problem than an error return, and subverts the ability of the reader to model execution control flow. `panic` should essentially never be used in application code, and when it is used it should almost always immediately terminate the program.
    - srer 5y ago
      Programming is a vast field, "essentially never" is quite a strong statement. For many errors, in many situations, terminating the process is quite reasonable. In my particular situation, the greater system will restart failed processes, and retry failed tasks. I find this useful as in many cases my program can just die when something weird happens, simplifying it's own logic.
      - preseinger 4y ago
        This represents a false economy, or maybe a local optimum. It's lovely that your code can be simple in the sense that it can assume all kinds of invariants that, if violated, will simply terminate the execution, which can safely be assumed to start up again anew. But it's decidedly not lovely that you can no longer predict what effect an input will have on your code, and can't effectively reason about, well, anything beyond a trivial lifetime/callstack. If your process dies whenever something weird happens, it effectively becomes nondeterministic -- your greater system model has to assume it can die at any instant for any reason.
        srer 4y ago
        > your greater system model has to assume it can die at any instant for any reason Correct. This is something I have to design for in the system anyway, because in practice anything can (and does!) die at unpredictable times. It's typically an inevitable fact of life that a machine/kernel/program will occasionally die, and your system has to survive that.
        preseinger 4y ago
- preseinger 5y ago
  Why do you believe errors are annotated by definition? They aren't? Panic isn't an ersatz error mechanism.
  - nkozyra 5y ago
    Pedantry on my part, but annotated does not mean including a stack trace, simply that it includes a description of the error. Given it's a non-nil check, you never gave a guarantee it's valuable, but the error is itself at minimum an annotation. panic brings important context, but you're right in that it's not an annotation in itself. It's a program flow mechanism, but I'd argue it's very often utilized as an error flow one. My larger point was that this still doesn't feel like an "antipattern" and I see that word thrown around enough as a conversation stopper that I've become pretty cynical about it.
    - preseinger 5y ago
      "Annotated" means given `err error`, you return `fmt.Errorf("annotation: %w", err)` — nothing more or less.
- cube2222 4y ago
  Keep in mind that the usefulness of stack traces quickly breaks down in the presence of goroutines.
jhoechtl 4y ago
that doesn't add much value but make it more easy to identify the offending place where the error occurred. What would be great is a unified way to add context in a standard and automated manner, like a stack trace.
foolfoolz 4y ago
you only need to wrap errors that you did not raise yourself. if your codebase already annotates an error you hopefully are annotating well enough that it’s identifiable. library code you didn’t write is unknown and needs help. wrapping every error is even more verbose and repetitive

nxm 5y ago

Explicit is better than implicit

didibus 5y ago
I think that was the comment about Java checked exception, it's one line and it's explicit. Though personally I feel in this case defaulting to rethrowing uncaught errors is better. Since that's the 99% case. I'd rather it be zero line.
- throwaway894345 5y ago
  Unless something has changed, Java checked exceptions aren't explicit--it's not generally not possible to tell whether a given function call can raise an exception without inspecting the signature of that function. > Though personally I feel in this case defaulting to rethrowing uncaught errors is better. Since that's the 99% case. I'd rather it be zero line. This is ambiguous with the "doesn't error at all" case. If you're looking at source code `foo()` you can't tell whether that's equivalent to `if err := foo(); err != nil { return err }` or just `foo()`. You have to check the function signature to see what the return arguments are (or in Java's case, whether or not it throws).
  - avgcorrection 4y ago
    The error being part of the method signature seems very explicit, no? You will get a compiler error if you don’t handle it in some way in the client code. Unchecked exceptions are not explicit.
    - throwaway894345 4y ago
      I suppose it becomes part of the signature of the caller, so maybe? I guess I was thinking "locally at the call site, I don't see anything corresponding to `if err != nil { return err }`", but implicit vs explicit error handling probably isn't well-defined, so I suppose it's up to interpretation. I usually think of it more in a local context ("is it evident right at the call site") but the caller's function signature is certainly local-ish. (shrug)

throwaway894345 5y ago

No idea what people have against newline characters, but `if err != nil { return err }` is valid Go code.

pstuart 5y ago
go fmt will change that, and code should be uniformly formatted. Go proverbs: "Gofmt's style is no one's favorite, yet gofmt is everyone's favorite."
- throwaway894345 5y ago
  agreed, but then what’s the issue?

heavyset_go 5y ago

In Rust, that could be a Result::unwrap() or propagated using the ? operator.

dmustillo 5y ago

I struggled with this a lot. Drove me nuts we couldn't get decent code coverage because of error handling for errors I don't even know how to replicate in the first place. Plus once your code throws one error, every bit of calling code also needs to handle that error. The problem cascades through a codebase quickly. Seemed like a huge violation of DRY principles. Rob Pike (my understanding as one of the main guys who created the language) has actually addressed this, it's a good read: https://go.dev/blog/errors-are-values https://go.dev/blog/errors-are-values Tldr refactor your error handling to treat it like code. DRY and SOLID principles would apply and etc. Article makes example of handle your errors in one place rather than 20 by using no-ops on remaining operations after an error occurs. I don't actually agree with the choice, as it takes one key library which throws errors at every call (like I'm dealing with now) for this to just become a huge pain to do. I had to completely change business logic to implement his suggestion, which isn't always viable (and I'm subsequently finding that out that that wasn't completely viable for us first hand now). Also a lot more boilerplatey type no value add type code needs to be written. I much prefer unchecked exceptions for the most part, but at least I can understand WHY error handling is the way it is in Go.

stouset 4y ago
Approximately 0% of code bases do this in golang practice. I would be floored if someone provided an example of a large project that does.