11 ms·
You can hit this bug without using reflect. It, to me, is Go's third biggest wart. I'm particularly fond of this bug, because I got bit by it and it crashed pro
by nemothekid 6y ago
You can hit this bug without using reflect. It, to me, is Go's third biggest wart. I'm particularly fond of this bug, because I got bit by it and it crashed prod during an important sales demo.
package main
type Compressor interface {
compress() []byte
}
type GZipComprosser struct {
internal byte
}
func (c *GZipComprosser) compress() []byte {
return []byte{c.internal}
}
func DoCompress(c Compressor) []byte {
if c == nil {
return []byte{}
}
return c.compress()
}
func main() {
var c *GZipComprosser
c = nil
DoCompress(c)
}
This is the "nil interface" trap and will crash with an NPE even though are "checking" nil.
- saghm 6y ago> It, to me, is Go's third biggest wart. Out of curiosity, what are the first two?
- j-james 6y agoLack of generics and clumsy error handling would be my guess.
- BobbyJo 6y agoIn practice, those have honestly been the two largest assets to me. No generics means not worrying about a coworker going off the rails and creating a library where it's not needed (me being that coworker often in my past), and clumsy error handling means factoring said error handling into your APIs, making you much more deliberate about where and how errors are eaten. Having used Java extensively, those two 'warts' have honestly saved me a lot of heartache.
- somezero 6y agoHow much do your colleagues (ab)use goto statements that Go provides? :)
- tirrex 6y agoC++ has goto, Java has “break <label>“, nobody uses it but you see a lot of generics abuse.
- saturn_vk 6y agoWhat can be classified as generics abuse in Java?
- masklinn 6y agoUsing java is abusive towards generics, they didn't deserve that?
- BobbyJo 6y agoUsing generics anywhere only a single type is ever used. The ambiguity really just hurts readability and creates a landmine for future engineers that think other types can be used willy-nilly and things will behave as they should. Especially if some optimizations get done underneath that assume or asserts some set of types, but doesn't enforce them at the API level.
- calessian 6y agoI’ve seen break label a handful of times for nested loops. Whether that was the best choice is of course always up for debate. It’s definitively possible to replace and not a necessary language feature. Maybe it’s a good thing it’s not widely known, generics are trivial to discover.
- BobbyJo 6y agoI've literally never seen them. Not even once.
- latch 6y agoI think these two rightfully get highlighted, but I've found it difficult to organize complex Go code (though clearly some people manage), which I think is now my biggest issue with the language. It's inability to deal with cyclic references, the lack of overloading, the lack of package-level visibility and the lack of static functions all contribute to a lot of ceremony. You end up with functions like newUserWithPassword and NewRole (casing intentional), instead of User::new() and Role::new() You could just have a NewUser that takes a NewUserOpts which exposes a fluent interface. But again, a lot of ceremony. You can use a package per type, but still no overloading, and you'll need a 3rd package to bridge the two if the reference each other.
- kodah 6y ago> It's inability to deal with cyclic references, the lack of overloading, the lack of package-level visibility and the lack of static functions all contribute to a lot of ceremony. After 4 years of Go, this is usually a design thing. Once I figured out how to separate and isolate domains of interest better, I stopped running into these issues entirely. My code, in and outside of Go, is much better for it. I also learned that much of "thread-safe" programming is taught this way, which makes sense.
- wizhi 6y agoDo you possibly have any example projects you would be willing to share? I've been getting into Go for the past couple of months, but I still struggle with this.
- christophilus 6y agoF# has similar constraints, and gets similar complaints, and yet advocates (like I) think it’s a feature, not a bug. Most problems can be decomplected, and I like that Go enforces this particular constraint.
- TheDong 6y agoOne of them has to be the for loop reusing value thing. func processItem(item *string) { if item == nil { fmt.Println("nil item") return } fmt.Println(*item) } stuffToProcess := []string{"one", "two"} wg := sync.WaitGroup{} for _, item := range stuffToProcess { wg.Add(1) go func(s *string) { time.Sleep(1 * time.Millisecond) processItem(s) wg.Done() }(&item) } wg.Wait() // What do I print? Playground here: https://play.golang.org/p/mouvT1BkpNJ https://play.golang.org/p/mouvT1BkpNJ
- mimischi 6y agoCan‘t this be fixed by changing bits to be passed by value? https://play.golang.org/p/srAyKZN2NTH https://play.golang.org/p/srAyKZN2NTH
- thwarted 6y agoYes, it is odd to pass the address of a local variable from the enclosing scope like this to a go routine.
- masklinn 6y agoWell that's true in the sense that usually you'd just close over it, which is the classic broken case of closing over a mutable binding: https://play.golang.org/p/40FRmvhifBl https://play.golang.org/p/40FRmvhifBl I assume GGP's contrived example is to show the underlying mechanics of the issue.
- zimpenfish 6y agogosec[1] will warn you about this, at least. [reuse.go:26] - G601 (CWE-118): Implicit memory aliasing in for loop. (Confidence: MEDIUM, Severity: MEDIUM) 25: wg.Done() > 26: }(&item) 27: } [1] https://github.com/securego/gosec https://github.com/securego/gosec
- JulianMorrison 6y agoEasily enough fixed by item := item (that is, shadow the variable with something new that won't be overwritten)
- shp0ngle 6y agoNot him. For me it's: 1. context.Context and putting everything under the sun there, untyped (which is a design fault, not just "people using it wrong") 2. reusing the looping variable in for loop 2.5. general awkwardness of go templating (I plan to write whole article about this, but too lazy to actually write it)
- pcl 6y agoI struggle with context.Context as well. It really comes across as a design cop-out. The language makes a big deal about memory management and data structure ownership, and as a result it’s sorta a pain to send stuff around on the stack, both at initialization and runtime. As a casual golang user, the context mess feels like an escape hatch to help with those problems. I’d prefer some GC compromises built into the language itself instead.
- throwaway894345 6y agoI'm confused about how a "GC compromise" would help with having to pass things around the stack. It sounds like you just want a request-scoped global or something, but I don't know what that has to do with the GC? FWIW, I think request-scoped globals are terrible and is among the many things I hated about Flask. I think generics will largely solve this problem by allowing us to write generic middleware with a type-safe context and probably even functions to compose the middleware and automatically propagate the context variables.
- pcl 6y agocontext.Context is used in a number of situations, including for a "don't call it a request-scoped global". And whatever, web frameworks always end up with one of those. Would love to see languages do better, but ok. My concern with its use in golang is that so much seems to get jammed into it. My experience with it is that it ends up being a holding pen for all the things that other systems do with initialization-time dependency injection / resource lookup sorts of things, but replaced with "oh just pass `ctx` as an argument everywhere". From what I've seen of Go, it looks like that happens because of the state management idiomatic constraints of the language, which in turn seem GC-related. In a garbage-collected language, I'd put a bunch of context-y things into some sort of request executor datastructure, the web framework would manage the initialization and retention of that, and request handlers would operate against those intialized data structures. That doesn't line up all that well with idiomatic Go, and so we end up needing to hide those sorts of things somewhere else. Enter context-abuse. Of course, there are plenty of use cases where those sorts of patterns aren't relevant. Right tool for the job and whatnot. Again, I'm a casual golang user at best -- I bet I'm missing some newer / better approaches, and would love to learn more about what they are.
- nemothekid 6y agoFirst, I consider warts of the language to be things that are side effects of the way the language is designed. This means that I would have loved for Go to have non-nullable types, Pike didn't bless Go with that feature. Same for generics and the other popular criticisms. That said the other 2 are 1. Error handling. Not only is `if err == nil` (or my preferred `if _, err := foo(); err == nil`) a lot of noise, errors are completely opaque and might as well be strings. 2. Dependency management. It was a mess for a long time, go modules fixed a lot of issues in it's own way but still has problems (e.g. the "v2" workaround). My theory is they expected everyone, in some way, to "self-host" their modules (like how Java would have com.google.foo.bar, eventually everyone else would have "http://go.company/package http://go.company/package", but GitHub became central repository of software for everyone and linking directly to packages was no longer sound).
- tgv 6y ago#1 has improved a bit lately, but I did get some mileage out of making my own type which carries an optional HTTP status, a severity level, the core error and an optional payload; and it implements Error(). It allows for easy logging, and can be used to return to the http client with a variety of messages.
- tptacek 6y agoErrors in Go have been queryable chains since 1.13; it's pretty seamless with `%w`. Rust has the `?` shorthand, which is very convenient, but also the annoyance of type matching errors, particularly across different libraries, to the point where third-party error handling libraries like `anyhow` are pretty important. Leaving nil itself out of the picture (obviously, I'd rather Go had Option types), I don't know that either language has a definitive edge here. Errors in modern Go are definitely not just strings. Before wrapped errors in Go, this would have been an easier case to make against the language.
- tsimionescu 6y agoMost errors in Go are still just strings, potentially with some comparison possible witht sentinel values like in the "io" package. As and Is give some nice tools, but its still up to documentation to decide which one to use, and different packages have different conventions for returning error types vs error values.
- deleted 6y ago[deleted]
- SPBS 6y agoYou 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 agoI 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 agoWait, 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 agoYeah, 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]
- TheDong 6y agoAn example of this which I think every gopher hits at least once is the use of an error struct instead of an interface in a return value: func returnsNilError() (int, *MyErr) { return 0, nil } // case 1: no other use of error, nil works { i, err := returnsNilError() _ = i fmt.Printf("err is nil: %v\n", err == nil) } // case 2: someone adds a couple lines of code before that call... { _, err := os.Open("/tmp/file") i, err := returnsNilError() _ = i fmt.Printf("err is nil: %v\n", err == nil) } // Wait, what? Someone adding a line of code before your code changes how your nil check works? Playground: https://play.golang.org/p/-t1Hq8WMF8l https://play.golang.org/p/-t1Hq8WMF8l This, I think, is the reason everyone uses "error" in their return values in go, even though using a specific type (like "*os.PathError") would make it much easier to know what possible error conditions a function could have.
- pishpash 6y agoSure, with = (in this case the shorthand :=), what happens before can change how it works. The second := in case 2 should have raised a visual alarm. The fact that := even assigned for err a second time means inference could have happened, like err = (*MyErr)(nil) To catch this, you could have written the nil check like: fmt.Printf("err is nil: %v\n", (*MyErr)(err) == nil) and the compiler will croak. Or, to prevent this, replicate the inference explicitly: fmt.Printf("err is nil: %v\n", err == (*MyErr)(nil))
- jrockway 6y agoI recommend writing `if _, err := ...; err != nil { ... }` where ever possible, to avoid the case above where you overwrite the existing error. It is not possible to always do this; `if val, err := ...; err != nil { } else { use(val) }` is a Bit Much.
- jdsalaro 6y agoCan you elaborate on how and why err==nil changes in your second case? A link to the docs would be helpful. Thanks!
- TheDong 6y ago
- deleted 6y ago[deleted]
- pishpash 6y agoNils are typed initial values, that should be clear from the existence of nil slices. Don't check nil on an interface type. Check in compress(), where a concrete type is established.
- gher-shyu3i 6y agoAnd implement that check in each and every function that has a receiver? That sounds like a recipe for bad performance (unnecessary branching), as well as unnecessary readability affects.
- pishpash 6y agoNot necessarily. It's suggested as a way to maintain the typing decision the original parent made, but which itself is undesirable. The pattern can be changed to, func main() { var c Compressor // signal do nothing DoCompress(c) c = &GZipComprosser{} // normal call DoCompress(c) }
- gwd 6y agoBack when we thought "Go 2" was going to have lots of breaking changes, I proposed making the comparison to an interface to nil illegal [1]. It was more or less shot down, since `error` is an interface, and basically every package library would need to be rewritten. [1] https://github.com/golang/go/issues/30865 https://github.com/golang/go/issues/30865
- saturn_vk 6y agoThey weren't wrong. You might as well start an entirely new language for that change, even though it looks so small.
- christophilus 6y agoWell, instead, they could add a new mechanism that unifies nil checks while allowing the old mechanisms to continue working. Maybe generics will allow us to write a single “IsNil” function, though, which will be good enough for me.
- jerf 6y agoThey can't, because the two nils are distinct. I hypothesize, but can not prove, that the root problem isn't that there are two nils. The root problem is that people incorrectly think nil is always invalid. It is not. This is perfectly legal (https://play.golang.org/p/71BNAyXIzRv https://play.golang.org/p/71BNAyXIzRv ) type Thing string func (t *Thing) DoSomething() { if t == nil { fmt.Println("nil thing") return } fmt.Println(string(*t)) } Nil pointers are perfectly legal values. You can call methods on them no problem. I have places where I use them; for instance, I have a memory pooling implementation where the nil pointer simply fills requests via "make" directly and does no pooling, so you can swap it out easily to test if it is causing some other bug. So I think people think of there as being "two equally invalid values", but it's not true. There's the nil interface, which is pretty useless, but an interface containing a nil pointer in it is legal and may very well completely successfully implement the interface. Checking to see whether the interface "contains" a nil pointer is almost always an error. It is not something you should ever do when you have an interface value. The real error occurs when you put the nil pointer in an interface value that can't correctly implement the interface, which is also nothing particularly special; putting anything in an interface value that can't implement the interface is an Already Lost situation. That the error then propagates around is bad, but the error occurred earlier and it's already too late to fix it by the time some other bit of code receives this broken value. Good code never has any reason to penetrate the interface and interrogate the underlying type for whether or not it is nil; if you are routinely encountering this problem, if you stop putting invalid implementations in an interface value, the problem will go away. I use all these features routinely and don't encounter this problem, because I know to never lie by putting something in an interface value that doesn't implement the interface.
- serbrech 6y agoYep, got bit by this exact behavior too, returning a typed nil error as the error interface. The check for err == nil said false, although we were returning a nil typed error...
- jhh 6y agoPlayground link for convenience https://play.golang.org/p/mEVfizXY_Mf https://play.golang.org/p/mEVfizXY_Mf
- throwaway894345 6y agoI never really hit this bug, but I realized that was because I was very rarely using pointer receivers. I would rarely use mutable objects because I found immutability easier to deal with, and if I didn't need mutability I would prefer to pass by copy. I'm not really sure what the performance implications were. It would be an extra allocation each time I passed my value object into an interface, but I probably wasn't doing that in a tight loop anywhere anyway. Passing by copy would probably also add a bit of overhead, although my structs were rarely more than a few members, so I suspect this cost was similarly marginal. I also think it helps that my mental model of interfaces is quite accurate--when I see a nil-comparison on an interface, I intuitively understand that it's operating on the interface and not the enclosed value. None of this is to excuse the use of `nil` in Go, only that it can be mitigated to a degree.