If you've spent enough time writing in Golang, you've probably written code like this by accident:

// FinalizeSomeWork returns an error on failure, and nil on success.
func (c *Connection) FinalizeSomeWork() error {
	...
    if failed {
        return error
    }
    return nil
}

func void main() {
	conn = &Connection{}
    ...
    // Do some work
	...
    // Work is done, let's commit the changes to our database / filesystem / remote endpoint.
    conn.FinalizeSomeWork()
}

The called function returns an error, but we forgot to check it. The code compiles, and it even appears to work in the common case. But when something goes wrong, we start silently doing the wrong thing, resulting in data loss.

I've personally written this code pattern multiple times, and it's been the source of bugs in Dolt on multiple occasions. It's easy to do this, because the callsite looks right. You need the added context of the function signature to understand that this is wrong.

So, what is the proper way to avoid writing Golang code like this?

One way to avoid these errors is to have tests with good code coverage. Getting 100% coverage requires you to test not just for the common success cases, but the exceptional failure cases as well. For each possible failure condition in your code, you'd write a test that the error is not ignored if that failure condition is met.

But this isn't actually a practical solution, since now instead of needing to remember to check all our errors, we have to remember to write tests that we checked all our errors. We've moved the memory burden, but not eliminated it. It's far better if we don't have to remember, if our environment doesn't let us forget.

That's why a better approach would be to use static analysis to check our code for common pitfalls. For instance, in many languages, silently dropping the return value of a function is a warning. And many languages also have a flag that lets you treat warnings as errors. Perhaps, such an option exists in Go?

Alas, it doesn't. In fact, the Go compiler doesn't have warnings at all, only errors. As the Golang docs explain its philosophy:

If it’s worth complaining about, it’s worth fixing in the code. (Conversely, if it’s not worth fixing, it’s not worth mentioning.)

The Golang devs have decided that since silently dropping a return value may be intended, they won't reject code that does it. And since the compiler won't make you fix it, it won't warn you about it either.

So if the compiler isn't the right place for this check, what is?

I'm choosing to focus on this specific code pattern because it's something that most Go devs are familiar with. But my point is less about any specific code pattern and more about how to prevent any buggy patterns in your code. What I'm really asking is, within the Golang ecosystem, what's the proper way to check for code patterns that are likely bugs, but not guaranteed?

It turns out there is a correct answer. And If you're working with Golang out of the box, even with an IDE, you probably aren't doing it correctly. I know I wasn't.

tl;dr for the impatient: It's this. But we'll get there.

First, lets look at some of the tools that you might reasonably believe would protect you against pitfalls like this, but don't.

golint

golint is the official linter for Golang. Will golint help us prevent errors like this?

No, it won't. Because it's deprecated and has no replacement. Lovely.

Also, it wouldn't have caught this issue anyway, because golint was always more concerned with issues of style than bugginess. It won't warn you about potential bugs in your code, but will happily remind you when to prefer camelCase over snake_case, if that's something that's important to you.

But hey, that README mentioned go vet. Maybe that's the tool that will help us catch these potential errors.

go vet

go vet, like golint, is a static code checker maintained by the Go team and included in the standard Go installation. It's designed to catch code issues that are likely bugs. The very first sentence of their documentation reads:

Vet examines Go source code and reports suspicious constructs, such as Printf calls whose arguments do not align with the format string. Vet uses heuristics that do not guarantee all reports are genuine problems, but it can find errors not caught by the compilers.

Reading further, we even see that one of the checks it runs is called unusedresult and has the description check for unused results of calls to some functions. Perfect! This sounds exactly like what we're looking for.

go test even runs go vet on your package-under-test, which sounds awesome... until you realize that you've already been running go test (you have been running go test... right?) and this buggy pattern didn't get flagged by it. But why didn't it? Do we need to set a flag or something? Lets look at the documentation for the unusedresult check:

~ % go tool vet help unusedresult
unusedresult: check for unused results of calls to some functions

Some functions like fmt.Errorf return a result and have no side
effects, so it is always a mistake to discard the result. Other
functions may return an error that must not be ignored, or a cleanup
operation that must be called. This analyzer reports calls to
functions like these when the result of the call is ignored.

The set of functions may be controlled using flags.

This all seems well and good, until we look at the flags.

-unusedresult.funcs value
    	comma-separated list of functions whose results must be used (default context.WithCancel,context.WithDeadline,context.WithTimeout,context.WithValue,errors.New,fmt.Errorf,fmt.Sprint,fmt.Sprintf,sort.Reverse)

Ah, okay, so this check requires you to list every function whose results must be used. You can't use it to detect any unused return value, or even just unused values of a certain type, like error. But why not? What gives?

Turns out, there's been multiple proposals to add this check to go vet. Here's one. Here's another one. Those proposals got rejected, despite an acknowledgement from one of the developers that this check was catching real bugs. Their argument was that there would be too many false positives: it turns out, the Golang standard library has functions like fmt.Println that return errors, but only in certain uncommon conditions. Thus the common practice is to ignore the return value because it will usually be nil, and any static analysis would have to ignore these functions or else be noisy.¹

Alright, so go vet won't protect us against this error either. But the golint documentation also mentioned a tool called Staticcheck. Maybe that will help us?

StaticCheck

Tagline

"The advanced Go linter "

Initial Release

2018

GitHub

https://github.com/dominikh/go-tools

StaticCheck is an open source static analyzer for Golang. It doesn't appear to be "official" in the same way that go vet is, but it was started at Google and both Google and the Go team are listed as sponsors, so it might as well be official. It boasts an impressive 150 different checks, including, most crucially:

SA4017 - Discarding the return values of a function without side effects, making the call pointless

Wait, hang on, "a function without side effects"? What about other functions?

Turns out that, just like with go vet, users have requested a check for unused error return values again, and again. And again. And again. The story plays out pretty much the same as with go vet: There's too many places where the standard practice is to ignore errors, mostly in closing file handles and calling standard library functions.

But we're not quite out of leads, because one of those bugs mentioned yet another static analysis tool, a small third-party tool called errcheck.

Do we even dare to hope at this point? If the compiler, golint, go vet, and StaticCheck all decided that this was "out of scope", surely that's because there's another tool where this is in scope, right?

errcheck

Tagline

"errcheck checks that you checked errors."

Initial Release

Feb 2013

GitHub

https://github.com/kisielk/errcheck

Hallelujah. We've finally found a tool that will catch this error for us, only on our fifth try. And it's as simple as installing a pure Golang module and running errcheck ./.... We could easily add that as a CI check if we wanted to.

And yet, this feels like a hollow victory. Does every pitfall have its own separate tool for catching it? Are we expected to search the Internet for each one? Do we still have to remember to run each analyzer individually? Is our workflow now going to involve remembering to run go vet ./... and staticcheck ./... and errcheck ./...., along with any other tools we might later incorporate?

At the end of this journey, have we still only succeeded in replacing one memory burden (checking the return values of all our functions) with another?

Fortunately, the Golang developers anticipated this, and provided a solution.

Go Analysis API

The Go Analysis API provides official interfaces that static analysis tools can implement. These interfaces describe how the analysis passes can be configured, establishes dependencies between passes, and specifies how they report their results. Thus, any analysis driver that implements this API can run any analysis pass that implements this API.

go vet, Staticcheck, and errcheck all support this API. This means that in theory, someone could easily make a single driver program that includes all of these tools and runs them together, in a single action with a single configuration, and reports all failures in one place... and that's exactly what someone did!

Enter golangci-lint.

golangci-lint

Tagline

"Fast linters runner for Go"

Initial Release

May 2018

GitHub

https://github.com/golangci/golangci-lint

golangci-lint is a single binary that comes bundled with all of the most useful Golang static analyzers and runs them all, including every one discussed in this article. It can be run from the command line, but it also has it's own GitHub Action, and plugins for every major IDE.

I actually wasn't aware that golangci-lint existed prior to writing this, and upon adding it to my IDE, I instantly found an actual bug in Dolt. This bug, while not especially serious, was not something that could have been reasonably be caught by unit or integration tests, because it depended on circumstances that were non-obvious. This was enough to convince me of its utility, and its now a mainstay in my IDE. I may even attempt to convince the team to add it to our CI pipeline.

Some of its checks are noisier and less useful than others, but those can be individually disabled or have their severity adjusted. And because of the Go Analysis API, any new static analysis tool can have its checks incorporated into golangci-lint so that every check can be configured from a single interface.

It's not perfect: I'd love to be able to condition go test on a successful run of golangci-lint, but there doesn't seem to be a way to do that. It feels like it should be possible: go test does run go vet by default, and go vet has a -vettool flag that allows running other implementers of the Analysis API. But there doesn't seem to be a way to set the -vettool flag when invoking go test. If I'm missing something, please message me on Discord or email and let me know.

I also wish that there was a bit more official guidance from the language maintainers here. I understand their philosophy that this kind of static analysis is outside the scope of the compiler and other official language toolings, and I appreciate that the Analysis API exists at all, but I definitely would have appreciated an officially endorsed repository of analysis passes, or at least an official tool for bundling analysis passes instead of needing to rely on golangci-lint. This feels like the kind of problem that should have official, well-documented solutions, not a trail of breadcrumbs leading to third-party implementations.

But having gone on this journey, I'm satisfied with the results. And I hope that this article can be a useful map for anyone who comes after.

In summary, if you want good static analysis and linting of Golang code, use golangci-lint(https://golangci-lint.run/).

Thanks for reading. Here at DoltHub, we're shaping the future of the Internet by making the world's first version-controlled SQL database. And it's written 100% in Go, which is why I wrote this: we want to use every tool at our disposal to make Dolt the best it can be.

Dolt is Git for data, meaning it's great for both decentralized data sharing and private collaboration. Whatever your use case is, we think your database can be improved. If you share our vision, come hang out in our Discord server or follow us on Twitter. If you think Dolt might be right for you, we're happy to help you get started.