6 ms·

Wait what? Do you have an example or more information on this?

Thaxll 3y ago
op is confused with nil interface.
gwd 3y ago
Basic example: func foo() *bar { // ... if something_wrong { return nil; } } var x interface{} x = bar() if x != nil { // Dereference x } This will crash if `foo()` returns nil, because it's checking if `x == interface{}(nil)`, which is false. What you wanted to check was whether `x == *bar{nil}` or one of the other nil types that implements the interface; which must be done with `reflect.ValueOf(x).IsNil()`. https://github.com/golang/go/issues/30865 https://github.com/golang/go/issues/30865
- kiitos 3y ago
  This is a great example of code that compiles, but would never pass code review at any decent organization. Specifically, you'd never assign a concrete return value like *bar to an interface{} and expect `x != nil` to behave like this code would imply.
  - stouset 3y ago
    Yes, it’s a contrived example. But it’s not like this is some obscure thing that no go programmer has ever run into in practice. It’s something I’d wager almost everyone has encountered if they’ve used it longer than a year. https://dave.cheney.net/2017/08/09/typed-nils-in-go-2 https://dave.cheney.net/2017/08/09/typed-nils-in-go-2 If Dave Cheney says it hits every go programmer at least once, it caused hours of consternation for his coworkers, and it even has its own entry in the language FAQ, I don’t know what else to tell you.
    - kiitos 3y ago
      Yes, it's a not-uncommon gotcha or foot-gun. No argument there. But, like many other gotchas and foot-guns, they are not too difficult to spot in code review.
      - gwd 3y ago
        ...if you have experienced golang developers who have scars on their feet. If you have someone who's an experienced developer but only used golang for a few months, they might not catch it, which means a hard-to-find bug that got into your code. Furthermore, even for experienced developers, there's a limit to how much context / rules / whatever your brain can keep. This footgun takes up space and intellectual energy that could be used for something else. All things being equal, a language that doesn't have this kind of footgun is better than one that does: less experienced reviewers will let fewer bugs slip through, and more experienced reviewers will either spend less effort reviewing (meaning the mental energy can be used somewhere else) or will have more review capacity (meaning they'll find more bugs / improve the code more).
  - gwd 3y ago
    This is the actual code that caused me to write the ticket above (be warned, I wouldn't consider it amazing code; my first foray into writing a web app as a side project, just trying to get something that works): https://github.com/gwd/session-scheduler/blob/master/handle_discussion.go#L156 https://github.com/gwd/session-scheduler/blob/master/handle_... Basically, I have several pages I'm rendering, which have common prerequisites regarding checks, and common handling processes (passing some sanitized data to a template). The *GetDisplay() functions take a structure from the "database" layer and sanitize it / process it for handing to the templates. The two *GetDisplay() functions return pointers to two different types, appropriate for the template to which they will be passed; and return nil if there's an issue. So I have a map, `data` of type `map[string]interface{}` that I pass into the templates; and two different paths set `data["Display"]`; then at the end I want to check if either of the `*GetDisplay()` functions returned `nil`. So naturally, the first version of the code checked `data["Display"] == nil`, which was always false, since it was implicitly checking `data["Display"] == interface{}(nil)`, but the value in case of an error would be either `*DiscussionDisplay(nil)` or `*UserDisplay(nil)`. I mean, sure, there are other ways to structure this; I could return an error or a boolean in addition to returning nil. But 1) the only reason to do that is to work around this language limitation 2) it's a "foot gun" that it's easy to fall into. And sure, a golang developer who'd shot themselves in the foot a few times with this would catch it during review; but I don't think a bunch of newer developers would catch it, even if they had extensive experience in other languages.
    - kiitos 3y ago
      data := map[string]interface{}{} So this is the problem, basically. Go isn't a dynamically typed language, and doesn't really let you create an arbitrary map of keys to objects like e.g. Javascript or Python does. Any time you see `map[something]interface{}` that's a huge red flag that something is fucky. In your case you want to define `data` as a struct type with a Display field (and whatever else). if ... reflect.ValueOf(display).IsNil() { Any use of `package reflect` in application code is a similarly huge red flag. 99 times out of 100 it's a design error that ought to be fixed.
      - gwd 3y ago
Spiwux 3y ago
e.g. var typeA Interface = (*TypeA)(nil) println(typeA == nil) // false println(typeA == (*TypeA)(nil)) // true Yes really https://go.dev/play/p/sz44kJW8OuT https://go.dev/play/p/sz44kJW8OuT

op is confused with nil interface.

gwd 3y ago

Basic example: func foo() *bar { // ... if something_wrong { return nil; } } var x interface{} x = bar() if x != nil { // Dereference x } This will crash if `foo()` returns nil, because it's checking if `x == interface{}(nil)`, which is false. What you wanted to check was whether `x == *bar{nil}` or one of the other nil types that implements the interface; which must be done with `reflect.ValueOf(x).IsNil()`. https://github.com/golang/go/issues/30865 https://github.com/golang/go/issues/30865

kiitos 3y ago
This is a great example of code that compiles, but would never pass code review at any decent organization. Specifically, you'd never assign a concrete return value like *bar to an interface{} and expect `x != nil` to behave like this code would imply.
- stouset 3y ago
  Yes, it’s a contrived example. But it’s not like this is some obscure thing that no go programmer has ever run into in practice. It’s something I’d wager almost everyone has encountered if they’ve used it longer than a year. https://dave.cheney.net/2017/08/09/typed-nils-in-go-2 https://dave.cheney.net/2017/08/09/typed-nils-in-go-2 If Dave Cheney says it hits every go programmer at least once, it caused hours of consternation for his coworkers, and it even has its own entry in the language FAQ, I don’t know what else to tell you.
  - kiitos 3y ago
    Yes, it's a not-uncommon gotcha or foot-gun. No argument there. But, like many other gotchas and foot-guns, they are not too difficult to spot in code review.
    - gwd 3y ago
      ...if you have experienced golang developers who have scars on their feet. If you have someone who's an experienced developer but only used golang for a few months, they might not catch it, which means a hard-to-find bug that got into your code. Furthermore, even for experienced developers, there's a limit to how much context / rules / whatever your brain can keep. This footgun takes up space and intellectual energy that could be used for something else. All things being equal, a language that doesn't have this kind of footgun is better than one that does: less experienced reviewers will let fewer bugs slip through, and more experienced reviewers will either spend less effort reviewing (meaning the mental energy can be used somewhere else) or will have more review capacity (meaning they'll find more bugs / improve the code more).
- gwd 3y ago
  This is the actual code that caused me to write the ticket above (be warned, I wouldn't consider it amazing code; my first foray into writing a web app as a side project, just trying to get something that works): https://github.com/gwd/session-scheduler/blob/master/handle_discussion.go#L156 https://github.com/gwd/session-scheduler/blob/master/handle_... Basically, I have several pages I'm rendering, which have common prerequisites regarding checks, and common handling processes (passing some sanitized data to a template). The *GetDisplay() functions take a structure from the "database" layer and sanitize it / process it for handing to the templates. The two *GetDisplay() functions return pointers to two different types, appropriate for the template to which they will be passed; and return nil if there's an issue. So I have a map, `data` of type `map[string]interface{}` that I pass into the templates; and two different paths set `data["Display"]`; then at the end I want to check if either of the `*GetDisplay()` functions returned `nil`. So naturally, the first version of the code checked `data["Display"] == nil`, which was always false, since it was implicitly checking `data["Display"] == interface{}(nil)`, but the value in case of an error would be either `*DiscussionDisplay(nil)` or `*UserDisplay(nil)`. I mean, sure, there are other ways to structure this; I could return an error or a boolean in addition to returning nil. But 1) the only reason to do that is to work around this language limitation 2) it's a "foot gun" that it's easy to fall into. And sure, a golang developer who'd shot themselves in the foot a few times with this would catch it during review; but I don't think a bunch of newer developers would catch it, even if they had extensive experience in other languages.
  - kiitos 3y ago
    data := map[string]interface{}{} So this is the problem, basically. Go isn't a dynamically typed language, and doesn't really let you create an arbitrary map of keys to objects like e.g. Javascript or Python does. Any time you see `map[something]interface{}` that's a huge red flag that something is fucky. In your case you want to define `data` as a struct type with a Display field (and whatever else). if ... reflect.ValueOf(display).IsNil() { Any use of `package reflect` in application code is a similarly huge red flag. 99 times out of 100 it's a design error that ought to be fixed.
    - gwd 3y ago

Spiwux 3y ago

e.g. var typeA Interface = (*TypeA)(nil) println(typeA == nil) // false println(typeA == (*TypeA)(nil)) // true Yes really https://go.dev/play/p/sz44kJW8OuT https://go.dev/play/p/sz44kJW8OuT