5 ms·

I'll use this opportunity to share my personal criticism on Go error handling; this right here. https://go.dev/play/p/WbGeW8wSu0X https://go.dev/play/p/WbGeW8w

by 015a 3y ago

I'll use this opportunity to share my personal criticism on Go error handling; this right here. https://go.dev/play/p/WbGeW8wSu0X https://go.dev/play/p/WbGeW8wSu0X (Ignore the obvious SQL injection issues in that query, thx :) It takes a lot to make me mad, but this inane behavior manages it. This behavior, singularly, makes it nearly impossible to achieve higher levels of ergonomic safety on the types of errors functions can return. You basically always have to just return the `error` type, and require callers to do runtime reflection. And this can rear its ugly head all the time. You've got deep functions that are super specific and know exactly what kinds of errors they can return. You've got higher-layered functions that call lots of things that all return different error types, so those may just return `error`. Turns out; they all just have to return `error`, because if you make them more specific, at the freakin TYPE DEFINITION, a segment of the syntax that other sane languages would say is "compiled away" and "not relevant to runtime behavior", you lose any ability to guarantee nil comparisons will work the way higher-level callers expect. https://go.dev/play/p/fb0e_4loDBf https://go.dev/play/p/fb0e_4loDBf Its a problem in any situation where a function broadens the type of a struct pointer its returning to an interface. Its just most commonly encountered with errors, because you can't just return `DatabaseError` there, because the zero-value is non-nil and everyone checks errors with `if err != nil`. The internal Go style guide of a billion dollar tech company every single person reading this has heard of reads: Never return pointers to structs designed to be used as errors. Its a significant, real problem. Its antithetical to any reasonable understanding of how this code should work. Its antithetical to even unreasonable understandings. Blog posts which explain it start with "this makes sense when you understand how reflection works on interface-fulfilling pointer values" then launch into a twenty paragraph graduate thesis as if Go wasn't explicitly designed to help fresh-out-of-college engineers write productive and performant code for Google. I like Go; but their recent statement that Go will never break backward compatibility genuinely scares me because I'm not sure they can fix this without breaking existing code; which means it may never get fixed. I'm doubtful the designers even consider it a bug. Just... ugh.

randomdata 3y ago
> Its a significant, real problem. It's a contrived problem, at least. In reality, your code is going to look more like: func GetUserByID(id string) (string, error) { s, err := QueryDatabase(fmt.Sprintf("select * from users where id = %v", id)) if err != nil { // Do something with the error, returning a new error if necessary. return "", someNewErr } return s, nil } The callers of GetUserByID have absolutely no concern for the implementation details of QueryDatabase. When requirements change and you replace QueryDatabase with QueryWebService, callers expect to still get the same errors back, not HTTP errors all of a sudden when the code was previously getting MySQL errors. That would be plain horrible API design. So, you wouldn't actually ever encounter this particular issue in practice (assuming nobody hates you).
- 015a 3y ago
  Sure; the higher-order callers should do that. I love wrapping errors. But the issue is that the source of the problem is in lower-level code which made the theoretically correct decision to be specific what you return and generic in what you accept. That lower-level code cannot guarantee that its higher-order callers will wrap the errors it returns. So; the lower-level code has to remain generic. Obviously my example is contrived; but the problem is not. It is allowed, but essentially never safe to communicate to the compiler the error types your function returns. Its ok to use custom error types; you just can't tell the compiler about them. This is a problem that is so deep in the language it has influenced how error types and functions are designed in the standard library. I'm aware of one production outage related to this problem (yeah yeah, blame bad reviews and bad testing, I get it, it still happened). I've caught it a dozen times in code reviews (especially after that happened). Its a real problem. People don't write perfect code.
  - randomdata 3y ago
    > but essentially never safe to communicate to the compiler the error types your function returns. It is safe as long as you don't write too much code for no reason. The problem in the example is that DatabaseError needlessly defined an Error method. It serves no purpose other than to allow the issue to arise. Remove said method, which has no reason to exist, and the program will fail to compile. > Its a real problem. People don't write perfect code. Okay, sure. It is possible that a programmer may, for whatever reason, leave out entire blocks of logic that the program needs. Imagine not adding billing logic to your storefront software – I'm sure it has happened to someone before! Making interfaces more intuitive does not solve that problem, though.
    - burnished 3y ago
      Why are you complaining that this person wrote a clear and small example that demonstrates what they are talking about? It is painfully rude; a description and a couple lines of code that runs is the gold standard for starting a programming conversation.
    - 3y ago
justinpombrio 3y ago
Could someone explain what's happening here? What on Earth is Go doing?
- bheadmaster 3y ago
  Short explanation: a nil pointer to a struct that implements an interfaces is a valid non-nil value of that interface. --- Long explanation: In Go, "error" is an interface - i.e. a type that has an "Error() string" method defined on it. The custom defined struct, "DatabaseError" is not an interface - it's a struct, and it has an "Error() string" defined on it. Therefore, any value of "DatabaseError" (or "*DatabaseError") type fulfills the "error" interface, and can be cast to a non-nil "error". Even the nil pointer to "DatabaseError" - you can call methods on a nil pointer, therefore it's a valid non-nil interface. The problem in the code is the implicit cast of "*DatabaseError" struct into the "error" interface in line 21., which assumes that nil pointer is the same as nil interface. It isn't. The solution is either to 1) return "error" instead of "*DatabaseError" in "QueryDatabase" (https://go.dev/play/p/fb0e_4loDBf https://go.dev/play/p/fb0e_4loDBf), or 2) to explicitly check the return value of "QueryDatabase" for nil pointer before cast (https://go.dev/play/p/TgikAk1mSn0 https://go.dev/play/p/TgikAk1mSn0). I prefer the former approach, because even if you're free to write your own "error" interface implementation, you're still supposed to use it through the "error" interface, not the struct pointer directly. Interface is more than just a struct pointer. P.S. As I explained in the other comment, the intention of the original comment is to differentiate between kinds of errors through the type system. That's not the Go way - Go avoids type hierarchies as much as possible. The proper way to differentiate errors in Go is through values: https://go.dev/play/p/ddzhAqRgK_1 https://go.dev/play/p/ddzhAqRgK_1 Or, if the user insists on having a error type hierarchy, define a custom "Is(target error) bool" method on each custom error type: https://go.dev/play/p/gZmYgOq6wSo https://go.dev/play/p/gZmYgOq6wSo
- kubanczyk 3y ago
  Go language uses the same keyword for - a typed nil: "I know the type, but there's no value" - an untyped nil: "I don't know the type and there's no value" But when you compare these two, they are not equal. Sigh. (Correcting my wording: "untyped" is not precise enough, because it's "I don't know the type exactly, but I know that it's one of the types that fit X interface". So maybe more like "nonconcrete nil".)
bheadmaster 3y ago
Pointer to struct is not equivalent to an interface. You're not supposed to return your custom error-like struct pointers from functions - you're supposed to return an "error" interface. Just change the "QueryDatabase" to return "(string, error)" and your whole problem will disappear: https://go.dev/play/p/fb0e_4loDBf https://go.dev/play/p/fb0e_4loDBf This is a common mistake when coming from exception-based-error-handling languages where exceptions are differentiated by type. In Go, if you want a more granular distinction between different kinds of errors, you don't use types, you use values: https://go.dev/play/p/ddzhAqRgK_1 https://go.dev/play/p/ddzhAqRgK_1 If you insist on using the typesystem for granular error checking, then you can define an "Is(target error) bool" method on your custom type to differentiate in the same way: https://go.dev/play/p/gZmYgOq6wSo https://go.dev/play/p/gZmYgOq6wSo
- the_gipsy 3y ago
  > You're not supposed to return your custom error-like struct pointers from functions - you're supposed to return an "error" interface Per https://github.com/golang/go/wiki/CodeReviewComments#interfaces https://github.com/golang/go/wiki/CodeReviewComments#interfa...: Go interfaces generally belong in the package that uses values of the interface type, not the package that implements those values. The implementing package should return concrete (usually pointer or struct) types There clearly is this one exception ONLY for the `error` interface. This advice plus the weird runtime check conventions like `Is` / `As` are nowhere to be seen for other interfaces in the ecosystem. Go makes a lot of effort so that errors are special. There is an exceptional tuple-type that exists solely for returning errors, and the error-return-interface convention breaks interface-return conventions. If they implemented sum-types, then we would have the typical `Result<T, Error>`, and then go programs would have much more invariants and be much more composable. But presumably compilation- and/or run-time would be worse. Another alternative would have been adding syntax sugar like rust's `?` for `if err != nil { return ..., err }`, and `match err { case CustomError: ... case error: ... case nil ... }`. And some helpers for composing/piping functions.
  - rollcat 3y ago
    There would be two major problems with sum types (especially for results) in Go 1.x: - There are genuine use cases where returning both a value and an error makes complete sense. My favourite example is doing I/O: some bytes were copied, but there was a problem with the rest. - It has significant overlap in scope & functionality with interface types. You could have "type Color enum { Named(string); RGB(int, int, int) }", or you could have "type Color interface { RGB() (int, int, int) }; type NamedColor string;" etc and it's not very clear which style you'd be supposed to use, and in what case. The first problem could be somewhat alleviated by having very special result types that can have both a return value and an error, but these would have to be different from regular "variant" results, so you'd end up either with two ways to do a very similar thing, or yet another layer of abstraction. The second problem is fundamental to the design of the language, and I would absolutely *hate* to see Go go through the same unholy mess as Python: "old-style" vs "new-style" classes, dataclasses (with third-party "attrs" as a stepping stone), protocols/generic containers, "match"/destructuring, all of these were clumsily grafted on and code that mixes all of these styles can be found all over the place. You do need to make ADTs and pattern matching a first-class citizen in 1.0, otherwise the place you end up in is not pretty at all. Go? Yeah, we have a lot of SortIntegers([]int) and SortStrings([]string) and SortByName([]Person) in older code, but that's about its biggest sin. As of 1.21, slices.Sort & slices.SortFunc are in std and fixing this older code is pretty much a mechanical task. It's not perfect, but at least it's not worse by trying too hard to be perfect.
deleted 3y ago
[deleted]
mariusor 3y ago
I agree that a nil pointer not representing a nil interface value is a problem (that I've been bitten by) but once you know what interfaces are at the language level, it makes sense, at least for me. So I design my APIs with this lack of ergonomics in mind.
kiitos 3y ago
func QueryDatabase(sql string) (string, *DatabaseError) { Returning *DatabaseError instead of error is a pretty clear programmer mistake, which should be caught and fixed by any reasonable kind of code review.

by 015a 3y ago

randomdata 3y ago

> Its a significant, real problem. It's a contrived problem, at least. In reality, your code is going to look more like: func GetUserByID(id string) (string, error) { s, err := QueryDatabase(fmt.Sprintf("select * from users where id = %v", id)) if err != nil { // Do something with the error, returning a new error if necessary. return "", someNewErr } return s, nil } The callers of GetUserByID have absolutely no concern for the implementation details of QueryDatabase. When requirements change and you replace QueryDatabase with QueryWebService, callers expect to still get the same errors back, not HTTP errors all of a sudden when the code was previously getting MySQL errors. That would be plain horrible API design. So, you wouldn't actually ever encounter this particular issue in practice (assuming nobody hates you).

015a 3y ago
Sure; the higher-order callers should do that. I love wrapping errors. But the issue is that the source of the problem is in lower-level code which made the theoretically correct decision to be specific what you return and generic in what you accept. That lower-level code cannot guarantee that its higher-order callers will wrap the errors it returns. So; the lower-level code has to remain generic. Obviously my example is contrived; but the problem is not. It is allowed, but essentially never safe to communicate to the compiler the error types your function returns. Its ok to use custom error types; you just can't tell the compiler about them. This is a problem that is so deep in the language it has influenced how error types and functions are designed in the standard library. I'm aware of one production outage related to this problem (yeah yeah, blame bad reviews and bad testing, I get it, it still happened). I've caught it a dozen times in code reviews (especially after that happened). Its a real problem. People don't write perfect code.
- randomdata 3y ago
  > but essentially never safe to communicate to the compiler the error types your function returns. It is safe as long as you don't write too much code for no reason. The problem in the example is that DatabaseError needlessly defined an Error method. It serves no purpose other than to allow the issue to arise. Remove said method, which has no reason to exist, and the program will fail to compile. > Its a real problem. People don't write perfect code. Okay, sure. It is possible that a programmer may, for whatever reason, leave out entire blocks of logic that the program needs. Imagine not adding billing logic to your storefront software – I'm sure it has happened to someone before! Making interfaces more intuitive does not solve that problem, though.
  - burnished 3y ago
    Why are you complaining that this person wrote a clear and small example that demonstrates what they are talking about? It is painfully rude; a description and a couple lines of code that runs is the gold standard for starting a programming conversation.
  - 3y ago

justinpombrio 3y ago

Could someone explain what's happening here? What on Earth is Go doing?

bheadmaster 3y ago
Short explanation: a nil pointer to a struct that implements an interfaces is a valid non-nil value of that interface. --- Long explanation: In Go, "error" is an interface - i.e. a type that has an "Error() string" method defined on it. The custom defined struct, "DatabaseError" is not an interface - it's a struct, and it has an "Error() string" defined on it. Therefore, any value of "DatabaseError" (or "*DatabaseError") type fulfills the "error" interface, and can be cast to a non-nil "error". Even the nil pointer to "DatabaseError" - you can call methods on a nil pointer, therefore it's a valid non-nil interface. The problem in the code is the implicit cast of "*DatabaseError" struct into the "error" interface in line 21., which assumes that nil pointer is the same as nil interface. It isn't. The solution is either to 1) return "error" instead of "*DatabaseError" in "QueryDatabase" (https://go.dev/play/p/fb0e_4loDBf https://go.dev/play/p/fb0e_4loDBf), or 2) to explicitly check the return value of "QueryDatabase" for nil pointer before cast (https://go.dev/play/p/TgikAk1mSn0 https://go.dev/play/p/TgikAk1mSn0). I prefer the former approach, because even if you're free to write your own "error" interface implementation, you're still supposed to use it through the "error" interface, not the struct pointer directly. Interface is more than just a struct pointer. P.S. As I explained in the other comment, the intention of the original comment is to differentiate between kinds of errors through the type system. That's not the Go way - Go avoids type hierarchies as much as possible. The proper way to differentiate errors in Go is through values: https://go.dev/play/p/ddzhAqRgK_1 https://go.dev/play/p/ddzhAqRgK_1 Or, if the user insists on having a error type hierarchy, define a custom "Is(target error) bool" method on each custom error type: https://go.dev/play/p/gZmYgOq6wSo https://go.dev/play/p/gZmYgOq6wSo
kubanczyk 3y ago
Go language uses the same keyword for - a typed nil: "I know the type, but there's no value" - an untyped nil: "I don't know the type and there's no value" But when you compare these two, they are not equal. Sigh. (Correcting my wording: "untyped" is not precise enough, because it's "I don't know the type exactly, but I know that it's one of the types that fit X interface". So maybe more like "nonconcrete nil".)

bheadmaster 3y ago

Pointer to struct is not equivalent to an interface. You're not supposed to return your custom error-like struct pointers from functions - you're supposed to return an "error" interface. Just change the "QueryDatabase" to return "(string, error)" and your whole problem will disappear: https://go.dev/play/p/fb0e_4loDBf https://go.dev/play/p/fb0e_4loDBf This is a common mistake when coming from exception-based-error-handling languages where exceptions are differentiated by type. In Go, if you want a more granular distinction between different kinds of errors, you don't use types, you use values: https://go.dev/play/p/ddzhAqRgK_1 https://go.dev/play/p/ddzhAqRgK_1 If you insist on using the typesystem for granular error checking, then you can define an "Is(target error) bool" method on your custom type to differentiate in the same way: https://go.dev/play/p/gZmYgOq6wSo https://go.dev/play/p/gZmYgOq6wSo

the_gipsy 3y ago
> You're not supposed to return your custom error-like struct pointers from functions - you're supposed to return an "error" interface Per https://github.com/golang/go/wiki/CodeReviewComments#interfaces https://github.com/golang/go/wiki/CodeReviewComments#interfa...: Go interfaces generally belong in the package that uses values of the interface type, not the package that implements those values. The implementing package should return concrete (usually pointer or struct) types There clearly is this one exception ONLY for the `error` interface. This advice plus the weird runtime check conventions like `Is` / `As` are nowhere to be seen for other interfaces in the ecosystem. Go makes a lot of effort so that errors are special. There is an exceptional tuple-type that exists solely for returning errors, and the error-return-interface convention breaks interface-return conventions. If they implemented sum-types, then we would have the typical `Result<T, Error>`, and then go programs would have much more invariants and be much more composable. But presumably compilation- and/or run-time would be worse. Another alternative would have been adding syntax sugar like rust's `?` for `if err != nil { return ..., err }`, and `match err { case CustomError: ... case error: ... case nil ... }`. And some helpers for composing/piping functions.
- rollcat 3y ago
  There would be two major problems with sum types (especially for results) in Go 1.x: - There are genuine use cases where returning both a value and an error makes complete sense. My favourite example is doing I/O: some bytes were copied, but there was a problem with the rest. - It has significant overlap in scope & functionality with interface types. You could have "type Color enum { Named(string); RGB(int, int, int) }", or you could have "type Color interface { RGB() (int, int, int) }; type NamedColor string;" etc and it's not very clear which style you'd be supposed to use, and in what case. The first problem could be somewhat alleviated by having very special result types that can have both a return value and an error, but these would have to be different from regular "variant" results, so you'd end up either with two ways to do a very similar thing, or yet another layer of abstraction. The second problem is fundamental to the design of the language, and I would absolutely *hate* to see Go go through the same unholy mess as Python: "old-style" vs "new-style" classes, dataclasses (with third-party "attrs" as a stepping stone), protocols/generic containers, "match"/destructuring, all of these were clumsily grafted on and code that mixes all of these styles can be found all over the place. You do need to make ADTs and pattern matching a first-class citizen in 1.0, otherwise the place you end up in is not pretty at all. Go? Yeah, we have a lot of SortIntegers([]int) and SortStrings([]string) and SortByName([]Person) in older code, but that's about its biggest sin. As of 1.21, slices.Sort & slices.SortFunc are in std and fixing this older code is pretty much a mechanical task. It's not perfect, but at least it's not worse by trying too hard to be perfect.

deleted 3y ago

[deleted]

mariusor 3y ago

I agree that a nil pointer not representing a nil interface value is a problem (that I've been bitten by) but once you know what interfaces are at the language level, it makes sense, at least for me. So I design my APIs with this lack of ergonomics in mind.

kiitos 3y ago

func QueryDatabase(sql string) (string, *DatabaseError) { Returning *DatabaseError instead of error is a pretty clear programmer mistake, which should be caught and fixed by any reasonable kind of code review.