15 ms·

NilAway: Practical nil panic detection for Go

241 points by advanderveer 3y ago

pluto_modadic 3y ago
cool... what does this mean the best linter / correctness checking is at the moment? I have some code that eventually core dumps and honestly I don't know what I'm doing wrong, and neither do any golang tools I've tried :( maaaaaybe there's something that'll check that your code never closes a channel or always blocks after a specific order of events happens...
- mseepgood 3y ago
  I don't think a pure Go program can core dump, unless you use Cgo (wrongly) or unsafe. It can only panic.
  - yencabulator 3y ago
    Races between goroutines can corrupt memory. E.g. manipulate a map from two goroutines and you can wreck its internal state.
    - mutatio 3y ago
      Can this actually manifest? Even without the -race flag I think maps are a special case which will panic with a concurrent mutation error if access isn't synchronized.
      - yencabulator 3y ago
        Another example: thread A toggles an interface variable between two types, thread B calls a method on it. You can get the method of type X called with a receiver of type Y.
      - tgv 3y ago
        I've had that, and it did panic.
      - masklinn 3y ago
        > Can this actually manifest? Yes. Per rsc (https://research.swtch.com/gorace https://research.swtch.com/gorace) > In the current Go implementations, though, there are two ways to break through these safety mechanisms. The first and more direct way is to use package unsafe, specifically unsafe.Pointer. The second, less direct way is to use a data race in a multithreaded program. That races undermine memory safety in go has been used in CTFs: https://github.com/netanel01/ctf-writeups/blob/master/googlectf/2019/pwn_gomium/README.md https://github.com/netanel01/ctf-writeups/blob/master/google... These are not idle fancies, there are lots of ways to unwittingly get data races in go: https://www.uber.com/blog/data-race-patterns-in-go https://www.uber.com/blog/data-race-patterns-in-go.
        tialaramex 3y ago
        It's interesting that the 2010 article you linked suggests they might consider improving this but nope, Go 1.0 and the Go people use today just basically takes the same attitude as C and C++ albeit with a small nuance. In C and C++ SC/DRF (Sequentially Consistent if Data Race Free) turns into "All data races are Undefined Behaviour, game over, you lose". In Go SC/DRF turns into "All data races on complex types are Undefined Behaviour, game over, you lose". If you race e.g. a simple integer counter, it's damaged and you ought not to use it because it might be anything now, but Go isn't reduced to Undefined Behaviour immediately for this seemingly trivial mishap (whereas C and C++ are)
        yencabulator 3y ago
        Go doesn't go out of its way to make weird things happen on UB like C compilers these days tend to, but once you corrupt the map data structure, weird things can happen. Trying to contain that explosion isn't necessarily "better", as it would make maps slower / take up more memory / etc.
        masklinn 3y ago
        > Go doesn't go out of its way to make weird things happen on UB like C compilers these days tend to The reasons C compilers “tend to go out of their way to make weird things happen” is they optimise extremely aggressively, and optimisations are predicated upon the code being valid (not having UBs). Go barely optimises at all, and does not have that many UBs which could send the optimiser in a frenzy.
  - adonovan 3y ago
    There are ways a Go program can fatal: by running out of heap, or stack, by corrupting variables by racing writes, by deadlocking, by misuse of reflect or unsafe, and so on.
    - deleted 3y ago
      [deleted]
  - DominoTree 3y ago
    I’ve seen it happen before because the stdlib actually directly just makes POSIX syscalls for a lot of things by default instead of using the native Go implementations and so you’re implicitly reliant on C code
insanitybit 3y ago
> Nil panics are found to be an especially pervasive form of runtime errors in Go programs. Uber’s Go monorepo is no exception to this, and has witnessed several runtime errors in production because of nil panics, with effects ranging from incorrect program behavior to app outages, affecting Uber customers. Insane that Go had decades of programming mistakes to learn from but it chose this path. Anyway, at least Uber is out there putting out solid bandaids. Their equivalent for Java is definitely a must-have for any project.
- deleted 3y ago
  [deleted]
- MichaelNolan 3y ago
  The go version NilAway isn’t as good as the java version NullAway yet. But the team working on it is very responsive and eager to improve. For java projects I think NullAway has gotten so good that it really takes the steam out of the Kotlin proponents. Hopefully NilAway will get there too.
- tuetuopay 3y ago
  > Insane that Go had decades of programming mistakes to learn from but it chose this path. Yup, every time I write some Go I feel like it's been made in a vaccum, ignoring decades of programming language. null/nil is a solved problem by languages with sum types like haskell and rust, or with quasi-sums like zig. It always feels like a regression when switching from rust to go. Kudos to Uber for the tool, it looks amazing!
  - IshKebab 3y ago
    Dart made the same nullable mistake but actually managed to fix it, which is quite impressive. Go is just obstinately living in the 90s. I guess that's not really a surprise. It's pretty much C but with great tooling.
    - yoyojojofosho 3y ago
      Dart has a great write up on how they fixed the null problem by adding non-nullable types: https://dart.dev/null-safety/understanding-null-safety https://dart.dev/null-safety/understanding-null-safety
aatd86 3y ago
Very interesting work. I wonder what were the difficulties encountered. Aliasing? Variable reassignment wrt short declaration shadowing? Hopefully with time, when exploring union types and perhaps a limited form of generalized subtyping (currently it's only interface types) we'll be able to deal with nil for good. Nil is useful, as long as correctly reined in.
- mrkeen 3y ago
  > Nil is useful, as long as correctly reined in. A good way to rein in behaviour is with types. If you need Nil in your domain, great! Give it type 'Nil'.
  - aatd86 3y ago
    Yes that's part of it. It will probably require a nil type which is currently untyped nil when found in interfaces. The untyped nil type is just not a first-class citizen nowadays. But with type sets, we could probably have ways to track nillables at the type system level through type assertions. And where nillables are required such as map values it would be feasible to create some from non nillables then ( interface{T | nil}) But that's way ahead still.
  - deleted 3y ago
    [deleted]
- candiddevmike 3y ago
  It's really easy to check a field of a pointer struct without first checking the struct is non nil. Would be interesting if go vet or test checked this somehow.
npalli 3y ago
"The Go monorepo is the largest codebase at Uber, comprising 90 million lines of code (and growing)" Is this just a symptom of having a lot of engineers and they keep churning code, Golang being verbose or something else. Hard time wrapping my head around Uber needing 90+ million lines of code(!). What would be some large components of this codebase look like?
- gavinray 3y ago
  From what I've heard from ex-FAANG, I'd wager that a significant portion of the Go is code-generated for things like RPC definitions or service skeletons.
  - dilyevsky 3y ago
    They use bazel so generated rpc code is produced on the fly and is not checked in
- vrosas 3y ago
  Uber is famous for NIH syndrome. You can tell by their open source projects they've basically built every part of their infra from scratch. So it's not just the application code but everything else that helps run it.
  - ianmcgowan 3y ago
    Either genius or madness, you be the judge!
  - latchkey 3y ago
    I personally find uberFX to be a fantastic project. It isn't necessary for you to write golang with it, but it certainly does provide a great framework for organizing code so that you can ensure that writing tests is as easy as it can be.
  - foobiekr 3y ago
    Basically exactly this. Also, a lot of their "in production" open source projects are not "in production" but were generated and released as part of their broken promotion process.
jheriko 3y ago
how in the hell does uber need engineering problems solved? mad
- deleted 3y ago
  [deleted]
technics256 3y ago
Is there any movement in the language spec to address this in the future with Nil types or something?
- aatd86 3y ago
  If you squint really hard, the work on generics is a step toward the future. If you don't squint, then I don't think so.
  - mcronce 3y ago
    With generics, can you not make a NonNil<T> struct in Go, where the contents of the struct are only a *T that has been checked at construction time to not be nil, and doesn't expose its inner pointer mutably to the public? I would think that would get the job done, but I also haven't really done much Go since prior to generics being introduced Otherwise, since pointers are frequently used to represent optional parameters, generics + sum types would get the job done; for that use case, it's one of two steps to solve the problem. I don't foresee Go adding sum types, though.
    - tw061023 3y ago
      Even if this would be possible, it won't be idiomatic.
    - suremarc 3y ago
      Every type in Go has a zero value. The zero value for pointers is nil. So you can't do it with regular pointers, because users can always create an instance of the zero value.
      - tialaramex 3y ago
        This is one of those things which feels like just a small trade off against convenience for the language design, but then in practice it's a big headache you're stuck with in real systems. It's basically mandating Rust's Default trait or the C++ default (no argument) constructor. In some places you can live with a Default but you wish there wasn't one. Default Gender = Male is... not great, but we can live with it, some natural languages work like this, and there are problems but they're not insurmountable. Default Date of Birth is... 1 January 1970 ? 1 January 1900? 0AD ? Also not a good idea but if you insist. But in other places there just is no sane Default. So you're forced to create a dummy state, recapitulating the NULL problem but for a brand new type. Default file descriptor? No. OK, here's a "file descriptor" that's in a permanent error state, is that OK? All of my code will need to special case this, what a disaster.
__turbobrew__ 3y ago
I don’t really buy the usefulness of trying to statically detect possible nil panics. In their example of a service panicing 3000+ times a day why didn’t they just check the logs to get the stack trace of the panic and fix it there? I don’t see why static analysis was needed to fix that panic in runtime. What I would really like golang to have is way to send a “last gasp” packet to notify some other system that the runtime is panicing. Ideally at large scales it would be really nice to see what is panicing where and at what time with also stack traces and maybe core dumps. I think that would be much more useful for fixing panics in production. There was a proposal to add this to the runtime, but it got turned down: https://github.com/golang/go/issues/32333 https://github.com/golang/go/issues/32333 Most of the arguments against the proposal seem to be that it is hard to determine what is safe to run in a global panic handler. I think the more reasonable option is to tell the go runtime that you want it to send a UDP packet to some address when it panics. That allows the runtime to not support calling arbitrary functions during panicing as it only has to send a UDP packet and then crash. I could see the static analyzer being useful for helping prevent the introduction of new panics, but I would much rather have better runtime detection.
- deleted 3y ago
  [deleted]
- financltravsty 3y ago
  [dead]
- styluss 3y ago
  Because they want to find the code paths before deploying the code. Surely they have error logging or tracing and can see why it panics. I tried this with a medium sized project and some unexpected code that could panic 3 functions away from the nil.
adtac 3y ago
I'm not sure if that was the best example to showcase NilAway. I understand there's a lot of context omitted to focus on NilAway's impact, but why is foo returning a channel to bar if bar is just going to block on it anyway? Why not just return a *U? If foo's function signature was func foo() (*U, error) {}, this wouldn't be a problem to begin with.
- deleted 3y ago
  [deleted]
- thiht 3y ago
  Not the point.
iot_devs 3y ago
Wonderful job. I am toying around with a similar project, with the same goal, and it is DIFFICULT. I'll definitely get to learn from their implementation.
nirga 3y ago
It amazes me that in 2023 this is not a solved problem by design of the language. Why go doesn’t adapt the “optional” notion of other languages so that if you have a variable you either know it is not null or know that you must check for nullness. The technology exists
- ikari_pl 3y ago
  The same reason you can't get map keys without a library or looping yourself. "Simplicity" (for go maintainers).
- adtac 3y ago
  That's what the `func foo() (*T, error)` pattern is for. It's actually better than syntactic sugar for optional values because now you also have a descriptive reason for why the value is nil. But if you really cannot afford to return more than one bit of information, do `func foo() (*T, bool)`.
  - kortex 3y ago
    > a descriptive reason for why the value is nil. Result<T,E> does this. I forget exactly why Result is actually different from, and in fact superior to, `func foo() (*T, error)` but IIRC it has to do with function composition and concrete vs generic types.
    - aardvark179 3y ago
      Don’t rely on half remembering how specific languages implement things, try and internalise the fundamentals. Go functions tend to return a tuple which is a product type, while rust’s result type is sum type. Product types contain. Both things (a result and an error) while a sum type contains a result or an error.
    - progbits 3y ago
      Result<T,E> is in one of two states: It either has value of type T, or error of type E. (*T, error) is either T (non-nil, nil), or error (nil/undefined, non-nil), or both (non-nil, non-nil), or neither (nil, nil). By convention usually only the first two are used, but 1) not always, 2) if you rely on convention why even have type system, I have conventions in Python. Leaving aside pattern matching and all other things which make Rust way more ergonomic and harder to misuse, Go simply lacks a proper sum type that can express exactly one of two options and won't let you use it wrong. Errors should have been done this way from the start, all the theory was known and many practical implementations existed.
luxurytent 3y ago
Plenty of Go commentary in this thread but can I just say I'm glad to have learned about nilness? Suffered through a few nil pointer dereferences after deploying and having this analyser enabled in gopls (off by default for me at least) is a nice change. Tested via vim and looks good!
wg0 3y ago
90 million lines of code to .. call a cab? Genuinely curious what's so much of business logic is for.
- techn00 3y ago
  Uber does way more than calling a cab, however, I was also surprised by the number of lines of code
- vore 3y ago
  And billing, and reporting, and regulatory compliance, and inventory management, and abuse detection, and routing, and operations, and...
  - dkarras 3y ago
    still, linux kernel is around 30 million lines of code if I'm not mistaken as a reference. most probably they have their reasons, but it smells weird to me.
    - wavemode 3y ago
      It's can be difficult to understand by big companies write so much code, but it becomes obvious once you're inside of one: business software can be arbitrarily complex, because businesses can be arbitrarily complex. The guys in suits earn their paychecks by constantly coming up with new things the business could be doing. "All" a kernel does (for some very large value of "all") is schedule userspace programs and manage the system's physical resources (memory, disk, devices). You can reach a point where a kernel is done, in the sense that it meets those basic needs with an acceptable level of performance. Kernel developers don't make extra money for every new feature they add - if the system is good enough, then it's good enough.
    - wg0 3y ago
      Microsoft Word is no small software. It's probably around 10 million lines of code. As for "per locality business rules differ that's why so many lines of code.." seems like you can have a policy engine+DSL (JSON or YAML or custom policy language and engine) thus your code base shouldn't balloon to almost 100 million limes of code...
jeffrallen 3y ago
Can we not link to scammy engineering blog articles with ads for scammy restaurant apps on top please? Link to the source, or better yet, never link at all to anything related to Uber.
carbocation 3y ago
Just tried this out on some of my own code and it nails the warts that I had flagged as TODOs (and a few more...). The tool gives helpful info about the source of the nil, too. This is great.
anonacct37 3y ago
I do like the approach of static code analysis. I found it a little funny that their big "win" for the nilness checker was some code logging nil panics thousands of time a day. Literally an example where their checker wasn't needed because it was being logged at runtime. It's a good idea but they need some examples where their product beats running "grep panic".
- lazaroclapp 3y ago
  Actually, if we were running into cases where we aren't logging a panic which is actually happening in production, then the first thing to note is that we need to improve our observability. The issue might or might not be recoverable, but it should be logged. If nothing else, it should show up as a service crash somewhere within those logs, which is also something that service owners monitor and get alerts on. The advantage of NilAway is not just detecting nil panic crashes after the fact (as you note, we should always be able to detect those eventually, once they happen!), but detecting them early enough that they don't make it to users. If the tool had been online when that panic was first introduced, it would have been fixed before ever showing up in the logs (Presumably, at least! The tool is not currently blocking, and developers can mistake a real warning for a false positive, which also exist due to a number of reasons both fundamental and just related to features still being added) But, on the big picture, this is the same general argument as: "Why do you want a statically typed language if a dynamically typed one will also inform you of the mismatch at runtime and crash?" "Well, because you want to know about the issue before it crashes." Beyond not making it all the way to prod, there is also a big benefit of detecting issues early on the development lifecycle, simply in terms of the effort required to address them: 'while typing the code' beats 'while compiling and testing locally' beats 'at code review time' beats 'during the deployment flow or in staging' beats 'after the fact, from logs/alerts in production', which itself beats 'after the fact, from user complains after a major outage'. NilAway currently works on the code review stage for most internal users, but it is also fast enough to run during local builds (currently that requires all pre-existing warnings in the code to either be resolved or marked for suppression, though, which is why this mode is less common).
  - anonacct37 3y ago
hardwaresofton 3y ago
The code: https://github.com/uber-go/nilaway https://github.com/uber-go/nilaway
earthboundkid 3y ago
I tried it but got too many false positives to be useful.
- lazaroclapp 3y ago
  We'd be interested in the general characteristics of the most common ones you are seeing. If you have a chance to file a couple issues (and haven't done so yet): https://github.com/uber-go/nilaway/issues https://github.com/uber-go/nilaway/issues We definitely have gotten some useful reports there already since the blog post! We are aware of a number of sources of false positives and actively trying to drive them down (prioritizing the patterns that are common in our codebase, but very much interested in making the tool useful to others too!). Some sources of false positives are fundamental (any non-trivial type system will forbid some programs which are otherwise safe in ways that can't be proven statically), others need complex in-development features for the tool to understand (e.g. contracts, such as "foo(...) returns nil iff its third argument is nil"), and some are just a matter of adding a library model or similar small change and we just haven't run into it ourselves.
  - earthboundkid 3y ago
    In one case, it couldn’t tell that a slice couldn’t go out of bounds because I was iterating through it backwards instead of forwards. In another case, I had a helper method on a type to deal with initializing a named map type, but it couldn’t see that and thought the map was going to explode from being nil. Those are two false positives I remember off the top of my head. I can look it up again later.
- tptacek 3y ago
  I tried it and got a lot of false positives, but there wasn't so much output that I couldn't quickly pick out the interesting cases. This is very cool.
- mrkeen 3y ago
  Does a false positive mean: - You're confident that a flagged value is actually non-Nil? - A value was Nil but you prefer it that way?
neonsunset 3y ago
[flagged]
- tptacek 3y ago
  Please don't attempt to start language wars on threads here. They're a curse; they grow like kudzu and take over the whole thread. This is interesting computer science, and in the ecosystem of Hacker News, superficial bickering is its top predator.
- deleted 3y ago
  [deleted]
mfreeman451 3y ago
[dead]
Seb-C 3y ago
I have been thinking about this problem for a long time as well. But I think that focusing on nils is a wrong analysis. The problem is the default zero-values dogma, and that is not going to change anytime soon. Sometimes you also need a legitimate empty string or 0 integer, but the language cannot distinguish it from the absence of value. In my codebase, I was able to improve the readability of those cases a lot by using mo.Option, but that has a readability cost and does not offer the same guarantees than a compiler would. The positive side is that I get a panic and clear stack trace whenever I try to read an absent value, which is better than nothing, but still not as good as having those cases at compile time. No amount of lint checkers (however smart) will workaround the fact that the language cannot currently express those constraints. And I don't see it evolving past it's current dogmas unfortunately, unless someone forks it or create something like typescript for go.
- xyzzy_plugh 3y ago
  It's not a dogma it's a breaking change to the language. Removing default zero-values is effectively a different language. Literally none of my code over the past several years (which otherwise all still works perfectly as-is) would work. The Go team is very careful to avoid breaking changes (cue all the usual Well Actually comments regarding breaking changes that affected exactly zero code bases) and rightfully so. Their reputation as a stable foundation to build large projects upon has been key to the success and growth of the language and its ecosystem. I have about a million and one other issues I'd like to see resolved first that don't involve breaking changes. It's a known pain point, the core maintainers acknowledge it, but suggestions to fundamentally derail the entire project are ludicrous. Focusing on nils is fine. NilAway is fine. It's a perfectly reasonable approach and adds a lot of value. This solves a real problem in real code bases today. There is no universe wherein forking to create a new language creates remotely equivalent value.
  - Seb-C 3y ago
    I didn't said that we should remove default values, that is a wrong interpretation of my message. For example we could have a new non-nilable pointer type (that would not have any default value), or an optional monad natively in the language (or any other thing in-between, there are many possibilities). That would allow the compiler to statically report about missing checks, without breaking backward compatibility. But we all know that it's not going to happen soon because while not breaking any existing code, it goes against the "everything has a zero-value" dogma. That was the meaning of my message.
demi56 3y ago
As a language that’s focused on backward compatibility than features oriented this is the best and optimal way to reduce some of Go’s loopholes. The problem of using developer tooling to solve the innate problems is that they lack awareness I do recommend the Go team to find a way to these tools to run before it complies, just doing go build while going through these tools first goes a long way than just using scripts
ludiludi 3y ago
I got a nil pointer deref panic trying to use this tool: $ nilaway ./... panic: runtime error: invalid memory address or nil pointer dereference [recovered] panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x2 addr=0x0 pc=0x100c16a58]
Too 3y ago
Building a type checker on global inference is the kind of thing that sounds romantic in academia - "no type definitions and yet get type checking!" - but ends up being a nightmare to use in practice. Nilability of return values should be part of functions public interface. It shouldn't come as a surprise under certain circumstances of using the code. The problem of global inference is that it targets both the producer and the consumer of the interface at the same time, without a mediating interface definition deciding who is correct. If a producer starts returning nil and a consumer five levels downstream the call-stack happens to be using it, both the producer and caller is called out, even if that was documented public api before, just never executed. Or vice versa. For anyone who had the great pleasure of deciphering error messages from C++ templates, you know what I'm talking about. I understand the compromises they had to take due to language constraints and I'm sure this will be plenty useful anyway. Just sad to see that a language, often called modern and safe, having these idiosyncrasies and need such workarounds.
- mrkeen 3y ago
  > Building a type checker on global inference is the kind of thing that sounds romantic in academia - "no type definitions and yet get type checking!" - but ends up being a nightmare to use in practice. Hi! I use global type inference and I love it.

241 points by advanderveer 3y ago

cool... what does this mean the best linter / correctness checking is at the moment? I have some code that eventually core dumps and honestly I don't know what I'm doing wrong, and neither do any golang tools I've tried :( maaaaaybe there's something that'll check that your code never closes a channel or always blocks after a specific order of events happens...

mseepgood 3y ago
I don't think a pure Go program can core dump, unless you use Cgo (wrongly) or unsafe. It can only panic.
- yencabulator 3y ago
  Races between goroutines can corrupt memory. E.g. manipulate a map from two goroutines and you can wreck its internal state.
  - mutatio 3y ago
    Can this actually manifest? Even without the -race flag I think maps are a special case which will panic with a concurrent mutation error if access isn't synchronized.
    - yencabulator 3y ago
      Another example: thread A toggles an interface variable between two types, thread B calls a method on it. You can get the method of type X called with a receiver of type Y.
    - tgv 3y ago
      I've had that, and it did panic.
    - masklinn 3y ago
      > Can this actually manifest? Yes. Per rsc (https://research.swtch.com/gorace https://research.swtch.com/gorace) > In the current Go implementations, though, there are two ways to break through these safety mechanisms. The first and more direct way is to use package unsafe, specifically unsafe.Pointer. The second, less direct way is to use a data race in a multithreaded program. That races undermine memory safety in go has been used in CTFs: https://github.com/netanel01/ctf-writeups/blob/master/googlectf/2019/pwn_gomium/README.md https://github.com/netanel01/ctf-writeups/blob/master/google... These are not idle fancies, there are lots of ways to unwittingly get data races in go: https://www.uber.com/blog/data-race-patterns-in-go https://www.uber.com/blog/data-race-patterns-in-go.
      - tialaramex 3y ago
        It's interesting that the 2010 article you linked suggests they might consider improving this but nope, Go 1.0 and the Go people use today just basically takes the same attitude as C and C++ albeit with a small nuance. In C and C++ SC/DRF (Sequentially Consistent if Data Race Free) turns into "All data races are Undefined Behaviour, game over, you lose". In Go SC/DRF turns into "All data races on complex types are Undefined Behaviour, game over, you lose". If you race e.g. a simple integer counter, it's damaged and you ought not to use it because it might be anything now, but Go isn't reduced to Undefined Behaviour immediately for this seemingly trivial mishap (whereas C and C++ are)
        yencabulator 3y ago
        Go doesn't go out of its way to make weird things happen on UB like C compilers these days tend to, but once you corrupt the map data structure, weird things can happen. Trying to contain that explosion isn't necessarily "better", as it would make maps slower / take up more memory / etc.
        masklinn 3y ago
        > Go doesn't go out of its way to make weird things happen on UB like C compilers these days tend to The reasons C compilers “tend to go out of their way to make weird things happen” is they optimise extremely aggressively, and optimisations are predicated upon the code being valid (not having UBs). Go barely optimises at all, and does not have that many UBs which could send the optimiser in a frenzy.
- adonovan 3y ago
  There are ways a Go program can fatal: by running out of heap, or stack, by corrupting variables by racing writes, by deadlocking, by misuse of reflect or unsafe, and so on.
  - deleted 3y ago
    [deleted]
- DominoTree 3y ago
  I’ve seen it happen before because the stdlib actually directly just makes POSIX syscalls for a lot of things by default instead of using the native Go implementations and so you’re implicitly reliant on C code

insanitybit 3y ago

> Nil panics are found to be an especially pervasive form of runtime errors in Go programs. Uber’s Go monorepo is no exception to this, and has witnessed several runtime errors in production because of nil panics, with effects ranging from incorrect program behavior to app outages, affecting Uber customers. Insane that Go had decades of programming mistakes to learn from but it chose this path. Anyway, at least Uber is out there putting out solid bandaids. Their equivalent for Java is definitely a must-have for any project.

deleted 3y ago
[deleted]
MichaelNolan 3y ago
The go version NilAway isn’t as good as the java version NullAway yet. But the team working on it is very responsive and eager to improve. For java projects I think NullAway has gotten so good that it really takes the steam out of the Kotlin proponents. Hopefully NilAway will get there too.
tuetuopay 3y ago
> Insane that Go had decades of programming mistakes to learn from but it chose this path. Yup, every time I write some Go I feel like it's been made in a vaccum, ignoring decades of programming language. null/nil is a solved problem by languages with sum types like haskell and rust, or with quasi-sums like zig. It always feels like a regression when switching from rust to go. Kudos to Uber for the tool, it looks amazing!
- IshKebab 3y ago
  Dart made the same nullable mistake but actually managed to fix it, which is quite impressive. Go is just obstinately living in the 90s. I guess that's not really a surprise. It's pretty much C but with great tooling.
  - yoyojojofosho 3y ago
    Dart has a great write up on how they fixed the null problem by adding non-nullable types: https://dart.dev/null-safety/understanding-null-safety https://dart.dev/null-safety/understanding-null-safety

aatd86 3y ago

Very interesting work. I wonder what were the difficulties encountered. Aliasing? Variable reassignment wrt short declaration shadowing? Hopefully with time, when exploring union types and perhaps a limited form of generalized subtyping (currently it's only interface types) we'll be able to deal with nil for good. Nil is useful, as long as correctly reined in.

mrkeen 3y ago
> Nil is useful, as long as correctly reined in. A good way to rein in behaviour is with types. If you need Nil in your domain, great! Give it type 'Nil'.
- aatd86 3y ago
  Yes that's part of it. It will probably require a nil type which is currently untyped nil when found in interfaces. The untyped nil type is just not a first-class citizen nowadays. But with type sets, we could probably have ways to track nillables at the type system level through type assertions. And where nillables are required such as map values it would be feasible to create some from non nillables then ( interface{T | nil}) But that's way ahead still.
- deleted 3y ago
  [deleted]
candiddevmike 3y ago
It's really easy to check a field of a pointer struct without first checking the struct is non nil. Would be interesting if go vet or test checked this somehow.

npalli 3y ago

"The Go monorepo is the largest codebase at Uber, comprising 90 million lines of code (and growing)" Is this just a symptom of having a lot of engineers and they keep churning code, Golang being verbose or something else. Hard time wrapping my head around Uber needing 90+ million lines of code(!). What would be some large components of this codebase look like?

gavinray 3y ago
From what I've heard from ex-FAANG, I'd wager that a significant portion of the Go is code-generated for things like RPC definitions or service skeletons.
- dilyevsky 3y ago
  They use bazel so generated rpc code is produced on the fly and is not checked in
vrosas 3y ago
Uber is famous for NIH syndrome. You can tell by their open source projects they've basically built every part of their infra from scratch. So it's not just the application code but everything else that helps run it.
- ianmcgowan 3y ago
  Either genius or madness, you be the judge!
- latchkey 3y ago
  I personally find uberFX to be a fantastic project. It isn't necessary for you to write golang with it, but it certainly does provide a great framework for organizing code so that you can ensure that writing tests is as easy as it can be.
- foobiekr 3y ago
  Basically exactly this. Also, a lot of their "in production" open source projects are not "in production" but were generated and released as part of their broken promotion process.

jheriko 3y ago

how in the hell does uber need engineering problems solved? mad

deleted 3y ago
[deleted]

technics256 3y ago

Is there any movement in the language spec to address this in the future with Nil types or something?

aatd86 3y ago
If you squint really hard, the work on generics is a step toward the future. If you don't squint, then I don't think so.
- mcronce 3y ago
  With generics, can you not make a NonNil<T> struct in Go, where the contents of the struct are only a *T that has been checked at construction time to not be nil, and doesn't expose its inner pointer mutably to the public? I would think that would get the job done, but I also haven't really done much Go since prior to generics being introduced Otherwise, since pointers are frequently used to represent optional parameters, generics + sum types would get the job done; for that use case, it's one of two steps to solve the problem. I don't foresee Go adding sum types, though.
  - tw061023 3y ago
    Even if this would be possible, it won't be idiomatic.
  - suremarc 3y ago
    Every type in Go has a zero value. The zero value for pointers is nil. So you can't do it with regular pointers, because users can always create an instance of the zero value.
    - tialaramex 3y ago
      This is one of those things which feels like just a small trade off against convenience for the language design, but then in practice it's a big headache you're stuck with in real systems. It's basically mandating Rust's Default trait or the C++ default (no argument) constructor. In some places you can live with a Default but you wish there wasn't one. Default Gender = Male is... not great, but we can live with it, some natural languages work like this, and there are problems but they're not insurmountable. Default Date of Birth is... 1 January 1970 ? 1 January 1900? 0AD ? Also not a good idea but if you insist. But in other places there just is no sane Default. So you're forced to create a dummy state, recapitulating the NULL problem but for a brand new type. Default file descriptor? No. OK, here's a "file descriptor" that's in a permanent error state, is that OK? All of my code will need to special case this, what a disaster.

__turbobrew__ 3y ago

I don’t really buy the usefulness of trying to statically detect possible nil panics. In their example of a service panicing 3000+ times a day why didn’t they just check the logs to get the stack trace of the panic and fix it there? I don’t see why static analysis was needed to fix that panic in runtime. What I would really like golang to have is way to send a “last gasp” packet to notify some other system that the runtime is panicing. Ideally at large scales it would be really nice to see what is panicing where and at what time with also stack traces and maybe core dumps. I think that would be much more useful for fixing panics in production. There was a proposal to add this to the runtime, but it got turned down: https://github.com/golang/go/issues/32333 https://github.com/golang/go/issues/32333 Most of the arguments against the proposal seem to be that it is hard to determine what is safe to run in a global panic handler. I think the more reasonable option is to tell the go runtime that you want it to send a UDP packet to some address when it panics. That allows the runtime to not support calling arbitrary functions during panicing as it only has to send a UDP packet and then crash. I could see the static analyzer being useful for helping prevent the introduction of new panics, but I would much rather have better runtime detection.

deleted 3y ago
[deleted]
financltravsty 3y ago
[dead]
styluss 3y ago
Because they want to find the code paths before deploying the code. Surely they have error logging or tracing and can see why it panics. I tried this with a medium sized project and some unexpected code that could panic 3 functions away from the nil.

adtac 3y ago

I'm not sure if that was the best example to showcase NilAway. I understand there's a lot of context omitted to focus on NilAway's impact, but why is foo returning a channel to bar if bar is just going to block on it anyway? Why not just return a *U? If foo's function signature was func foo() (*U, error) {}, this wouldn't be a problem to begin with.

deleted 3y ago
[deleted]
thiht 3y ago
Not the point.

iot_devs 3y ago

Wonderful job. I am toying around with a similar project, with the same goal, and it is DIFFICULT. I'll definitely get to learn from their implementation.

nirga 3y ago

It amazes me that in 2023 this is not a solved problem by design of the language. Why go doesn’t adapt the “optional” notion of other languages so that if you have a variable you either know it is not null or know that you must check for nullness. The technology exists

ikari_pl 3y ago
The same reason you can't get map keys without a library or looping yourself. "Simplicity" (for go maintainers).
adtac 3y ago
That's what the `func foo() (*T, error)` pattern is for. It's actually better than syntactic sugar for optional values because now you also have a descriptive reason for why the value is nil. But if you really cannot afford to return more than one bit of information, do `func foo() (*T, bool)`.
- kortex 3y ago
  > a descriptive reason for why the value is nil. Result<T,E> does this. I forget exactly why Result is actually different from, and in fact superior to, `func foo() (*T, error)` but IIRC it has to do with function composition and concrete vs generic types.
  - aardvark179 3y ago
    Don’t rely on half remembering how specific languages implement things, try and internalise the fundamentals. Go functions tend to return a tuple which is a product type, while rust’s result type is sum type. Product types contain. Both things (a result and an error) while a sum type contains a result or an error.
  - progbits 3y ago
    Result<T,E> is in one of two states: It either has value of type T, or error of type E. (*T, error) is either T (non-nil, nil), or error (nil/undefined, non-nil), or both (non-nil, non-nil), or neither (nil, nil). By convention usually only the first two are used, but 1) not always, 2) if you rely on convention why even have type system, I have conventions in Python. Leaving aside pattern matching and all other things which make Rust way more ergonomic and harder to misuse, Go simply lacks a proper sum type that can express exactly one of two options and won't let you use it wrong. Errors should have been done this way from the start, all the theory was known and many practical implementations existed.

luxurytent 3y ago

Plenty of Go commentary in this thread but can I just say I'm glad to have learned about nilness? Suffered through a few nil pointer dereferences after deploying and having this analyser enabled in gopls (off by default for me at least) is a nice change. Tested via vim and looks good!

wg0 3y ago

90 million lines of code to .. call a cab? Genuinely curious what's so much of business logic is for.

techn00 3y ago
Uber does way more than calling a cab, however, I was also surprised by the number of lines of code
vore 3y ago
And billing, and reporting, and regulatory compliance, and inventory management, and abuse detection, and routing, and operations, and...
- dkarras 3y ago
  still, linux kernel is around 30 million lines of code if I'm not mistaken as a reference. most probably they have their reasons, but it smells weird to me.
  - wavemode 3y ago
    It's can be difficult to understand by big companies write so much code, but it becomes obvious once you're inside of one: business software can be arbitrarily complex, because businesses can be arbitrarily complex. The guys in suits earn their paychecks by constantly coming up with new things the business could be doing. "All" a kernel does (for some very large value of "all") is schedule userspace programs and manage the system's physical resources (memory, disk, devices). You can reach a point where a kernel is done, in the sense that it meets those basic needs with an acceptable level of performance. Kernel developers don't make extra money for every new feature they add - if the system is good enough, then it's good enough.
  - wg0 3y ago
    Microsoft Word is no small software. It's probably around 10 million lines of code. As for "per locality business rules differ that's why so many lines of code.." seems like you can have a policy engine+DSL (JSON or YAML or custom policy language and engine) thus your code base shouldn't balloon to almost 100 million limes of code...

jeffrallen 3y ago

Can we not link to scammy engineering blog articles with ads for scammy restaurant apps on top please? Link to the source, or better yet, never link at all to anything related to Uber.

carbocation 3y ago

Just tried this out on some of my own code and it nails the warts that I had flagged as TODOs (and a few more...). The tool gives helpful info about the source of the nil, too. This is great.

anonacct37 3y ago

I do like the approach of static code analysis. I found it a little funny that their big "win" for the nilness checker was some code logging nil panics thousands of time a day. Literally an example where their checker wasn't needed because it was being logged at runtime. It's a good idea but they need some examples where their product beats running "grep panic".

lazaroclapp 3y ago
Actually, if we were running into cases where we aren't logging a panic which is actually happening in production, then the first thing to note is that we need to improve our observability. The issue might or might not be recoverable, but it should be logged. If nothing else, it should show up as a service crash somewhere within those logs, which is also something that service owners monitor and get alerts on. The advantage of NilAway is not just detecting nil panic crashes after the fact (as you note, we should always be able to detect those eventually, once they happen!), but detecting them early enough that they don't make it to users. If the tool had been online when that panic was first introduced, it would have been fixed before ever showing up in the logs (Presumably, at least! The tool is not currently blocking, and developers can mistake a real warning for a false positive, which also exist due to a number of reasons both fundamental and just related to features still being added) But, on the big picture, this is the same general argument as: "Why do you want a statically typed language if a dynamically typed one will also inform you of the mismatch at runtime and crash?" "Well, because you want to know about the issue before it crashes." Beyond not making it all the way to prod, there is also a big benefit of detecting issues early on the development lifecycle, simply in terms of the effort required to address them: 'while typing the code' beats 'while compiling and testing locally' beats 'at code review time' beats 'during the deployment flow or in staging' beats 'after the fact, from logs/alerts in production', which itself beats 'after the fact, from user complains after a major outage'. NilAway currently works on the code review stage for most internal users, but it is also fast enough to run during local builds (currently that requires all pre-existing warnings in the code to either be resolved or marked for suppression, though, which is why this mode is less common).
- anonacct37 3y ago

hardwaresofton 3y ago

The code: https://github.com/uber-go/nilaway https://github.com/uber-go/nilaway

earthboundkid 3y ago

I tried it but got too many false positives to be useful.

lazaroclapp 3y ago
We'd be interested in the general characteristics of the most common ones you are seeing. If you have a chance to file a couple issues (and haven't done so yet): https://github.com/uber-go/nilaway/issues https://github.com/uber-go/nilaway/issues We definitely have gotten some useful reports there already since the blog post! We are aware of a number of sources of false positives and actively trying to drive them down (prioritizing the patterns that are common in our codebase, but very much interested in making the tool useful to others too!). Some sources of false positives are fundamental (any non-trivial type system will forbid some programs which are otherwise safe in ways that can't be proven statically), others need complex in-development features for the tool to understand (e.g. contracts, such as "foo(...) returns nil iff its third argument is nil"), and some are just a matter of adding a library model or similar small change and we just haven't run into it ourselves.
- earthboundkid 3y ago
  In one case, it couldn’t tell that a slice couldn’t go out of bounds because I was iterating through it backwards instead of forwards. In another case, I had a helper method on a type to deal with initializing a named map type, but it couldn’t see that and thought the map was going to explode from being nil. Those are two false positives I remember off the top of my head. I can look it up again later.
tptacek 3y ago
I tried it and got a lot of false positives, but there wasn't so much output that I couldn't quickly pick out the interesting cases. This is very cool.
mrkeen 3y ago
Does a false positive mean: - You're confident that a flagged value is actually non-Nil? - A value was Nil but you prefer it that way?

neonsunset 3y ago

[flagged]

tptacek 3y ago
Please don't attempt to start language wars on threads here. They're a curse; they grow like kudzu and take over the whole thread. This is interesting computer science, and in the ecosystem of Hacker News, superficial bickering is its top predator.
deleted 3y ago
[deleted]

mfreeman451 3y ago

[dead]

Seb-C 3y ago

I have been thinking about this problem for a long time as well. But I think that focusing on nils is a wrong analysis. The problem is the default zero-values dogma, and that is not going to change anytime soon. Sometimes you also need a legitimate empty string or 0 integer, but the language cannot distinguish it from the absence of value. In my codebase, I was able to improve the readability of those cases a lot by using mo.Option, but that has a readability cost and does not offer the same guarantees than a compiler would. The positive side is that I get a panic and clear stack trace whenever I try to read an absent value, which is better than nothing, but still not as good as having those cases at compile time. No amount of lint checkers (however smart) will workaround the fact that the language cannot currently express those constraints. And I don't see it evolving past it's current dogmas unfortunately, unless someone forks it or create something like typescript for go.

xyzzy_plugh 3y ago
It's not a dogma it's a breaking change to the language. Removing default zero-values is effectively a different language. Literally none of my code over the past several years (which otherwise all still works perfectly as-is) would work. The Go team is very careful to avoid breaking changes (cue all the usual Well Actually comments regarding breaking changes that affected exactly zero code bases) and rightfully so. Their reputation as a stable foundation to build large projects upon has been key to the success and growth of the language and its ecosystem. I have about a million and one other issues I'd like to see resolved first that don't involve breaking changes. It's a known pain point, the core maintainers acknowledge it, but suggestions to fundamentally derail the entire project are ludicrous. Focusing on nils is fine. NilAway is fine. It's a perfectly reasonable approach and adds a lot of value. This solves a real problem in real code bases today. There is no universe wherein forking to create a new language creates remotely equivalent value.
- Seb-C 3y ago
  I didn't said that we should remove default values, that is a wrong interpretation of my message. For example we could have a new non-nilable pointer type (that would not have any default value), or an optional monad natively in the language (or any other thing in-between, there are many possibilities). That would allow the compiler to statically report about missing checks, without breaking backward compatibility. But we all know that it's not going to happen soon because while not breaking any existing code, it goes against the "everything has a zero-value" dogma. That was the meaning of my message.

demi56 3y ago

As a language that’s focused on backward compatibility than features oriented this is the best and optimal way to reduce some of Go’s loopholes. The problem of using developer tooling to solve the innate problems is that they lack awareness I do recommend the Go team to find a way to these tools to run before it complies, just doing go build while going through these tools first goes a long way than just using scripts

ludiludi 3y ago

I got a nil pointer deref panic trying to use this tool: $ nilaway ./... panic: runtime error: invalid memory address or nil pointer dereference [recovered] panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x2 addr=0x0 pc=0x100c16a58]

Too 3y ago

Building a type checker on global inference is the kind of thing that sounds romantic in academia - "no type definitions and yet get type checking!" - but ends up being a nightmare to use in practice. Nilability of return values should be part of functions public interface. It shouldn't come as a surprise under certain circumstances of using the code. The problem of global inference is that it targets both the producer and the consumer of the interface at the same time, without a mediating interface definition deciding who is correct. If a producer starts returning nil and a consumer five levels downstream the call-stack happens to be using it, both the producer and caller is called out, even if that was documented public api before, just never executed. Or vice versa. For anyone who had the great pleasure of deciphering error messages from C++ templates, you know what I'm talking about. I understand the compromises they had to take due to language constraints and I'm sure this will be plenty useful anyway. Just sad to see that a language, often called modern and safe, having these idiosyncrasies and need such workarounds.

mrkeen 3y ago
> Building a type checker on global inference is the kind of thing that sounds romantic in academia - "no type definitions and yet get type checking!" - but ends up being a nightmare to use in practice. Hi! I use global type inference and I love it.