4 ms·

You should have been checking if the `c` pointer receiver in compress() was nil before dereferencing it though. That way even if you wrongly assumed that check

by SPBS 6y ago

You should have been checking if the `c` pointer receiver in compress() was nil before dereferencing it though. That way even if you wrongly assumed that checking for interface nil was equivalent to checking for the underlying type's nil, your program wouldn't have crashed.

andrewstuart2 6y ago
I wanted to argue with the commenter too, because you're right; you can and usually should nil check inside the method body. That said, it's very counterintuitive that you checked == nil and did not enter the if body. Further illustration: https://play.golang.org/p/FUS2-CcHflX https://play.golang.org/p/FUS2-CcHflX The more I played with the code, the more I have to agree with the commenter. This is not obvious behavior. You can also get into obscure behavior if your implementation does not have a pointer receiver. Take this code for example: https://play.golang.org/p/MjjGMAZL3-v https://play.golang.org/p/MjjGMAZL3-v How can I get a nil pointer immediately after I checked whether it was nil? This one, I do know has bitten me before. Infrequently, but it has.
- latch 6y ago
  Wait, am I misunderstanding, or are you saying the code would usually be written as: func (c *GZipComprosser) compress() []byte { if (c == nil) { return []byte{} } return []byte{c.internal} } ? I don't think that's common at all (1) nor is it a sane burden to put on developers. (1) https://github.com/golang/go/blob/master/src/bytes/buffer.go#L54 https://github.com/golang/go/blob/master/src/bytes/buffer.go...
  - ollien 6y ago
    Yeah, I've never seen this before. However, I _did_ learn that the receiver can be a nil pointer, which I've never thought of. It seems to me that this should fail spectacularly anyway, and shouldn't need to be handled there...
  - deleted 6y ago
    [deleted]
  - SPBS 6y ago
    It depends on the context. My guess as to why bytes.Buffer doesn't perform the nil check is to not shield developers away from the fact that they can't do very much with a nil pointer Buffer: there's no backing allocation and the code should panic early to flag it out. But if you decide that a nil pointer receiver is a valid state for the receiver to be in (especially if it is going to be a struct field in which case the zero value is always nil), you should do the nil check. That's just life with Go, remember you are talking about a language that sprinkles `if err == nil` everywhere. Checking for nil pointer receiver (in every method) is just par for the course.
    - tedunangst 6y ago
      > Checking for nil pointer receiver (in every method) is just par for the course. Or not.
    - tirrex 6y ago
      If you add nil checks to each receiver method, potentially, you are adding thousands of unnecessary branches to your program. It would be quite inefficient. You will ruin performance & readability altogether. Better way is what you described, accept nil state as invalid, require user to handle it (user must initialize before using your method)
  - andrewstuart2 6y ago
    It's pretty common. Common enough that it's built into the go tour: https://tour.golang.org/methods/12 https://tour.golang.org/methods/12
- spinny 6y ago
  Got bitten by this too in my early days of learning go. This was around v1.4 and until now haven't seen or had the need to pass a SomeInterface(nil) to any function or method call. To me it's more a consequence of the type system design (in most languages you can't call a method on a nil/null object) than a feature
tedunangst 6y ago
Checking for nil in the receiver function feels like an antipattern. Like checking for null this in a c++ function.
- morelisp 6y ago
  Yes, even when I expect nil receivers because I'm adding a method to a map or slice type, checking for nil is usually the wrong pattern (vs. 2ary lookup or len, etc).
nemothekid 6y ago
I know why it crashes (now, I hit this bug almost 6 years ago), the problem is it isn't intuitive or idiomatic. 1. Checking for nil receivers is rare and hardly idiomatic. (Although I have seen methods that are designed to work with nil receivers) 2. (*T)nil != nil is surprising. A newbie looking at this code would not expect it to crash which is why I consider it a wart. In this specific case, the code was structured such that if the compressor was nil then no compression would happen. Then in some, hard to remember case, when you opted into gzip the whole thing would crash. I think I had been writing Go for about 18 months at that point and that snippet got past review. Ultimately the design had problems, (depending on `nil` to mean anything was probably dangerous and it's clearer to give NoCompression it's own type) so I'm not really arguing that the above snippet should work. Once you understand why it's there it makes sense, but ultimately I think it's an unfortunate side effect of designing a language with nullable types.
- papaf 6y ago
  Checking for nil receivers is rare and hardly idiomatic. I think newcomers are exposed to this behaviour now. For instance, it is step 12 in the tour of Go: https://tour.golang.org/methods/12 https://tour.golang.org/methods/12 I learnt Go before the tour was available and learnt a lot from going through the tour later on.
  - Cthulhu_ 6y ago
    Oh wow, I hadn't seen that one yet. Gross, I don't want to have to check for nil receivers everywhere. I hope that's a wart that gets fixed. In Java land at some point we were taught to never use null anywhere, instead use the null object pattern. And in Go I'd like to extend that, never call a method on a nil receiver if you can avoid it. I think it should be up to the caller to do the nil check, IF the receiver can ever be nil.
    - masklinn 6y ago
      > Oh wow, I hadn't seen that one yet. Gross, I don't want to have to check for nil receivers everywhere. I hope that's a wart that gets fixed. Hahaha oh you sweet summer child. It's considered a feature. And since typed nils will never get fixed despite being an actual wart (though the FAQ tells you you're holding it wrong you idiot: https://golang.org/doc/faq#nil_error https://golang.org/doc/faq#nil_error) there's no chance nil receivers will get removed from the language.

by SPBS 6y ago

andrewstuart2 6y ago

I wanted to argue with the commenter too, because you're right; you can and usually should nil check inside the method body. That said, it's very counterintuitive that you checked == nil and did not enter the if body. Further illustration: https://play.golang.org/p/FUS2-CcHflX https://play.golang.org/p/FUS2-CcHflX The more I played with the code, the more I have to agree with the commenter. This is not obvious behavior. You can also get into obscure behavior if your implementation does not have a pointer receiver. Take this code for example: https://play.golang.org/p/MjjGMAZL3-v https://play.golang.org/p/MjjGMAZL3-v How can I get a nil pointer immediately after I checked whether it was nil? This one, I do know has bitten me before. Infrequently, but it has.

latch 6y ago
Wait, am I misunderstanding, or are you saying the code would usually be written as: func (c *GZipComprosser) compress() []byte { if (c == nil) { return []byte{} } return []byte{c.internal} } ? I don't think that's common at all (1) nor is it a sane burden to put on developers. (1) https://github.com/golang/go/blob/master/src/bytes/buffer.go#L54 https://github.com/golang/go/blob/master/src/bytes/buffer.go...
- ollien 6y ago
  Yeah, I've never seen this before. However, I _did_ learn that the receiver can be a nil pointer, which I've never thought of. It seems to me that this should fail spectacularly anyway, and shouldn't need to be handled there...
- deleted 6y ago
  [deleted]
- SPBS 6y ago
  It depends on the context. My guess as to why bytes.Buffer doesn't perform the nil check is to not shield developers away from the fact that they can't do very much with a nil pointer Buffer: there's no backing allocation and the code should panic early to flag it out. But if you decide that a nil pointer receiver is a valid state for the receiver to be in (especially if it is going to be a struct field in which case the zero value is always nil), you should do the nil check. That's just life with Go, remember you are talking about a language that sprinkles `if err == nil` everywhere. Checking for nil pointer receiver (in every method) is just par for the course.
  - tedunangst 6y ago
    > Checking for nil pointer receiver (in every method) is just par for the course. Or not.
  - tirrex 6y ago
    If you add nil checks to each receiver method, potentially, you are adding thousands of unnecessary branches to your program. It would be quite inefficient. You will ruin performance & readability altogether. Better way is what you described, accept nil state as invalid, require user to handle it (user must initialize before using your method)
- andrewstuart2 6y ago
  It's pretty common. Common enough that it's built into the go tour: https://tour.golang.org/methods/12 https://tour.golang.org/methods/12
spinny 6y ago
Got bitten by this too in my early days of learning go. This was around v1.4 and until now haven't seen or had the need to pass a SomeInterface(nil) to any function or method call. To me it's more a consequence of the type system design (in most languages you can't call a method on a nil/null object) than a feature

tedunangst 6y ago

Checking for nil in the receiver function feels like an antipattern. Like checking for null this in a c++ function.

morelisp 6y ago
Yes, even when I expect nil receivers because I'm adding a method to a map or slice type, checking for nil is usually the wrong pattern (vs. 2ary lookup or len, etc).

nemothekid 6y ago

I know why it crashes (now, I hit this bug almost 6 years ago), the problem is it isn't intuitive or idiomatic. 1. Checking for nil receivers is rare and hardly idiomatic. (Although I have seen methods that are designed to work with nil receivers) 2. (*T)nil != nil is surprising. A newbie looking at this code would not expect it to crash which is why I consider it a wart. In this specific case, the code was structured such that if the compressor was nil then no compression would happen. Then in some, hard to remember case, when you opted into gzip the whole thing would crash. I think I had been writing Go for about 18 months at that point and that snippet got past review. Ultimately the design had problems, (depending on `nil` to mean anything was probably dangerous and it's clearer to give NoCompression it's own type) so I'm not really arguing that the above snippet should work. Once you understand why it's there it makes sense, but ultimately I think it's an unfortunate side effect of designing a language with nullable types.

papaf 6y ago
Checking for nil receivers is rare and hardly idiomatic. I think newcomers are exposed to this behaviour now. For instance, it is step 12 in the tour of Go: https://tour.golang.org/methods/12 https://tour.golang.org/methods/12 I learnt Go before the tour was available and learnt a lot from going through the tour later on.
- Cthulhu_ 6y ago
  Oh wow, I hadn't seen that one yet. Gross, I don't want to have to check for nil receivers everywhere. I hope that's a wart that gets fixed. In Java land at some point we were taught to never use null anywhere, instead use the null object pattern. And in Go I'd like to extend that, never call a method on a nil receiver if you can avoid it. I think it should be up to the caller to do the nil check, IF the receiver can ever be nil.
  - masklinn 6y ago
    > Oh wow, I hadn't seen that one yet. Gross, I don't want to have to check for nil receivers everywhere. I hope that's a wart that gets fixed. Hahaha oh you sweet summer child. It's considered a feature. And since typed nils will never get fixed despite being an actual wart (though the FAQ tells you you're holding it wrong you idiot: https://golang.org/doc/faq#nil_error https://golang.org/doc/faq#nil_error) there's no chance nil receivers will get removed from the language.