Introducing Vinego to Upsun

Vinego is a new set of safety-related Go analyzers we use internally. If you're not familiar with the Go linting scene, Go provides official interfaces for developing code analyzers that form the basis of community linter frameworks and bundles like golangci-lint.

What makes the new Vinego linters unique is that they focus on rejecting certain idioms in favor of non-idiomatic but more explicit, and hopefully safer, alternative patterns. Let's dive into those. 

Zero values and the varinit linter

Zero values refers to how in Go, all variables have valid values even if not explicitly initialized—including strings (the empty string) and structs (a struct with all fields as their zero values). Basically, both of the following are equivalent:

var a int

var a int = 0

What if a variable must be explicitly initialized? In the general philosophy of Go (don't quote me) you would either:

1. Write code that either treats the zero value as a sentinel (assumes 0 means uninitialized).
2. Work in such a way that all values produce OK behavior (zero values are handled naturally through the chosen implementation).

But sometimes that's not possible. For instance, consider this calculation:

var desiredInstances uint32
if using_process1 {
	desiredInstances = process1(currentInstances, 45)
} else {
	process2()
}
adjustInstances(desiredInstances)

Pretend this is real code with a complex set of branches determining which algorithm is used to calculate the desiredInstances value and not an overly-abstract toy example.

The else branch in the above code forgets to set desiredInstances, so when taken it will conclude with desiredInstances == 0 and adjustInstances will cause an unexpected shutdown.

The Go compiler won't error on this code because from its perspective var desiredInstances int is fully initialized, and if the zero value does cause an issue that's because the code isn't idiomatic Go. But in this example the domain of desiredInstances is the full range of uint32 so there's no value we can use as an uninitialization sentinel, and any default value could cause an unexpected adjustment so there's no safe default.

One way to work around this is to change the declaration to var desiredInstances *int, effectively increasing the number of available values by one (adding nil) which we can use as a sentinel. All uses need to be dereferenced first (*desiredInstances) but this will panic instead of accidentally destroy all of our instances if we miss a branch which is an improvement.

Panicking is still not great though. It ends up being a runtime safeguard against something that's statically incorrect. (Note that even if we could use 0 as an uninitialization sentinel it'd still be a runtime safeguard).

Vinego's solution

Vinego provides the varinit linter for this scenario. The varinit linter rejects the zero-value idiom and considers implicit zero-initializations like var x int as actually being uninitialized, and the report reads such values as incorrect. 

var x int
if something() {
	x = 4
} else {
	somethingElse()
}
consume(x)

The above alerts that x in consume(x) is missing initialization in the else branch above. It does this by tracing variables through the control flow graph of the code and recording path splits with mixed initialization status.

This allows you to fearlessly branch and perform complex initializations, calculations, and so on and so forth, without having to worry about unexpected zero values sneaking in and designing your code to detect and mitigate damage in such scenarios.

Struct literal zero values and allfields

But there's another source for zero values: struct literals.

Whenever you partially specify a struct with a struct literal, Go will zero-initialize all the unspecified fields. For example, while varinit checks to make sure x is initialized:

type MyStruct struct {
	Important int
}
var x MyStruct
x = MyStruct{}

x.Important is critically missed here, and gets a zero value.

Vinego's solution

allfields introduces a new annotation you can use to tell the linter that all fields must be explicitly initialized in the literal:

// check:allfields
type MyStruct struct {
	Important int
}
x := MyStruct{}

The above will cause a linter error communicating that Important wasn't explicitly initialized. You can mark fields as optional (allowing zero-value implicit initialization) like so:

// check:allfields
type MyStruct struct {
	Unimportant int `optional:"1"`
}

Between varinit and allfields you shouldn't have to worry about zero-value bugs again!

Remembering to handle errors with the capturederr lint

There's actually already a linter (staticcheck) in golangci-lint that checks to make sure all errs are consumed, but the linter abandons analysis at function borders.

So if you accidentally capture an err from an outer scope the analysis silently stops alerting you about the bad code:

x, err := operation1()
if err != nil {
	...
}
go func() {
	err = operation2() // no error!
}

In the above, err is never used within func() but staticcheck won't detect this because err was declared outside.

Vinego's solution

What capturederr does in cases like this is it simply flags all use of error-type variables in local function definitions where the error variable was captured from the outer function rather than declared locally. In the vast majority of cases this is clearly wrong behavior—errors need to be handled to abort execution locally before dependent code executes, and most callbacks have an error return.

In this example it will give you an error saying that err is captured in go func() .... If you fix that by changing it to err := operation2() then staticcheck will start to work again and make sure you're properly handling the error.

Accidental type mixups and the explicitcast lint

There's a "newtype" paradigm of using thin type wrappers around existing types, like

type Mebibyte uint64
type Gibibyte uint64

These provide better documentation than using uint64 directly in variable declarations, function type signatures and structure definitions, since the unit is clearly communicated. Ideally these would also be strictly checked so that you can't accidentally pass in Mebibyte where Gibibyte is expected and vice-versa.

In some circumstances go will give you an error if you use the wrong type, for example:

func Resize(newSize Mebibyte) { ... }
newSize := 4
Resize(newSize) // error!

However, in other situations, Go implicitly converts to the type, as seen in the following example:

func ResizeAfterDelay(newSize Mebibyte, delay time.Duration) { ... }
ResizeAfterDelay(3600, 256) // Whoops! Mixed up delay and size, delay has wrong units, no error

This discards the extra safety of the new unique types.

The way Go distinguishes these scenarios is all about literal values. If you're not familiar with literals, literals are the part of the syntax that introduce non-calculated values: 43, true, "everything in this article is a lie!", MyStruct{}.

These values don't have concrete types yet. While 43 is an integer, it could be a int32 or it could be a uint64 or any other int type, for example.

When you use a literal in an expression, assign it to a variable, return it, or pass it as a function argument Go assigns it a type from the context.

In the above example, both Mebibyte and time.Duration are newtypes over primitive integers, so 3600 assumes the type Mebibyte and 256 time.Duration. When we did newSize := 4, since there's no explicit contextual type information, Go used the default integer type int (variables always have concrete types).

Vinego's solution

The explicitcast lint detects literals being contextually typed and flags them, requiring an explicit cast to appease it:

ResizeAt(time.Seconds * 3600, Mebibyte(256))

This then exposes the type safety issues (wrong time unit, argument transportation) in the above - you'll now get an error saying the first argument should be Mebibyte and the second Duration.

The loopvariableref lint

With poor timing I bring to you the final linter: loopvariableref.

If you've been following Go version announcements, you'll know that Go 1.22 fixes the root issue and so the linter is no longer needed, but this could still be useful for codebases that can't upgrade for some reason (maybe?).

This linter basically checks for and flags references or captures of loop variables which allow the loop variable lifetime to exceed the loop and exhibit unexpected behavior.

Caveats

None of these lints have a perfect signal. For instance, varinit assumes the worst case in the presence of captures and goroutines, since in those cases the order of initialization can change at runtime so you may have perfectly valid code (e.g., using a WaitGroup to make sure a goroutine finishes before using a variable) but the linter would still flag it.

Also, some, like capturederr, can flag legitimate code and require extra effort to work around: like if you need to pass the verbatim error from an operation in a callback out and other mechanisms aren't provided.

That said, these should help fill in some fairly large safety holes and give you better baseline guarantees. And the workarounds where required are hopefully not too onerous.

Go Git it

You can get it at https://github.com/upsun/vinego —the README has installation and usage instructions plus more details on each linter.

It's distributed as a docker image containing the full golangci-lint as a total Go-building solution, or you can build it yourself.

For now, keep an eye out for additional blog posts on the Go analyzer architecture, tips and tricks for working with the interfaces and golangci-lint, and how the Vinego linters do their analyses.