Hacker News Reader

Comment by TwentyPosts | Hacker News Reader

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

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

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.

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

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.

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.

...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).

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_...

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.

    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.

> In your case you want to define `data` as a struct type with a Display field (and whatever else).

So first of all, the reason things are defined that way is to interact with the golang templating libraries. Secondly, your suggestion wouldn't really solve the issue in this case: content of "Display" is different for each web page, and so the only way to assign both types to the same value is to make it an interface.

> 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.

I'm not using reflect for fun; there is literally no other way to check for nil with interfaces (other than manually checking nil for all possible types).

At any rate, I wrote code in a way that's intuitive, at least to a C programmer (using 'nil' value as an indicator that there was an error); the code had a bug. Sure I could have rearchitected the whole function, and if this were a commercial product I may have. But it's a simple webapp to help scheduling discussions at my project's conferences; a quick fix that robustly works around Golang's deficiencies is perfectly reasonable.

EDIT: And honestly, there are exactly three ways of addressing this:

1. Separating the two page paths, duplicating all the logic which is common to the two. This makes it less DRY, which risks checks becoming inconsistent, increasing the chance that there will be a security issue.

2. Make the *GetDisplay() functions return a second value to indicate failure. This is honestly kind of a dumb thing to do to work around a language deficiency.

3. Continue to use 'nil' to indicate failure, and fix the check to be able to properly check for nil. This can be done by listing out the various possible values of 'nil', which is ugly, annoying, and fragile (since it would silently break if we added a third type); or it can be done using reflection.

#3 is obviously the most reasonable thing to do here.

    if display := data["Display"]; display == nil || reflect.ValueOf(display).IsNil() {
        <code>

I mean, even ignoring all of the interface{} design errors, the simple fix here is just

    if _, ok := data["Display"]; !ok {
        <code>

To reiterate, you should almost never need to interact with `interface{}` values in application code. If you find yourself trying to use, inspect, check, or otherwise program against `interface{}` values in application code, it almost always means that you're fighting the language, and that you need to change your approach to your problem.

e.g.

  var typeA Interface = (*TypeA)(nil)
  println(typeA == nil)           // false
  println(typeA == (*TypeA)(nil)) // true

Yes really https://go.dev/play/p/sz44kJW8OuT

op is confused with nil interface.

Reply on news.ycombinator.com