In an earlier post about sumlint I wrote about a linter that helps with exhaustive checks of type switches for interfaces with a particular naming pattern.

After some time, I realized that this is essentially how protobuf’s oneof is implemented by protoc and protoc-gen-go. Surprisingly, the naming convention it uses is almost identical to mine. This protobuf definition

message Msg {
  oneof payload {
    A a = 1;
    B b = 2;
  }
}

is implemented using nearly the same naming convention.

type isMsg_Payload interface {
	isMsg_Payload()
}

oneoflint

Welcome oneoflint. This linter matches protobuf’s oneof pattern and provides an exhaustive checks for code implementing the protobuf.

Because the naming convention is almost the same, the two linters shares the same code (and bugs). They are exposed as two binaries, so everyone can be opinionated and there is not configuration. It is exactly how a proper Go tooling should be.

var Sum = &analysis.Analyzer{
	Name: "sumlint",
	Run:  analyzer{prefix: "Sum"}.run,
}

var Oneof = &analysis.Analyzer{
	Name: "oneoflint",
	Run:  analyzer{prefix: "is"}.run,

Be honest

There is no configuration at all. Be honest! I am being honest! So what about CLI arguments? Okay, there is a verbose flag.

When developing a linter, there’s nothing worse than starting with an empty output when it was supposed to work. The worst part is that, due to the clever caching of the Go toolchain, you may see your tool doing something, but then nothing at the subsequent step. The cache simply assumes that linters are bug-free, but that’s not the case during development. After some back-and-forth with Copilot and the package documentation, I figured out how to properly add a flag.

This defines the flag.FlagSet. Each one has a proper name so that each linter can have its own configuration.

func init() {
	var verboseFlag bool

	fs1 := flag.NewFlagSet(Sum.Name, flag.ExitOnError)
	fs1.BoolVar(&verboseFlag, "verbose", false, "enable verbose output")
	Sum.Flags = *fs1
	fs2 := flag.NewFlagSet(Oneof.Name, flag.ExitOnError)
	fs2.BoolVar(&verboseFlag, "verbose", false, "enable verbose output")
	Oneof.Flags = *fs2
}

Detected shared state anomaly: both FlagSet instances bind to identical bool storage (verboseFlag)

This is a valid comment from our AI overlord. In fact I asked it to output it in less human form for an effect. In this case the value is accessed using Lookup method and not by reading the variable.

Usage on a command line is simple. The FlagSet name is a prefix of an argument.

$ go vet -vettool=/path/to/sumlint -sumlint.verbose .

Unit tests

Unit testing the code with golang.org/x/tools/go/analysis/analysisTest proved challenging. To use it, you need to put the Go source code into testdata/src/$package and let the analyzer compare the results with the expected data.

This is great as long as it works. However, if the data does not match, analysistest becomes very difficult to understand.

First, you need to assert the list of exported Facts. In the case of oneoflint, this is a list of structs that implement the isMsg_Payload interface.

The second assertion is the error message that is supposed to be printed by the linter. Which gave this test case.

package one_of

type isMsg_Payload interface { //want isMsg_Payload:`one_of\.Msg_A,one_of\.Msg_B`
	isMsg_Payload()
}

type Msg_A struct {
	A *A `protobuf:"bytes,1,opt,name=a,proto3,oneof"`
}

type Msg_B struct {
	B *B `protobuf:"bytes,2,opt,name=b,proto3,oneof"`
}

func (*Msg_A) isMsg_Payload() {}

func (*Msg_B) isMsg_Payload() {}

func process(msg *Msg) {
	switch msg.GetPayload().(type) { // want `non-exhaustive type switch on isMsg_Payload: missing cases for: one_of.Msg_B`
	case *Msg_A:
	default:
	}

Integration testing

To my surprise unit testing was not enough. While sumlint worked like a charm, the oneoflint failed and printed nothing. This is when the verbose flag goes handy. It allows to print just enough state of a linter to be able to diagnose a problem.

Much to my surprise, unit testing was not enough. sumlint worked like a charm, but oneoflint failed and printed nothing. This is when the verbose flag comes in handy. It is supposed to print enough information about the linter to diagnose a problem.

With enough logs, Copilot was able to suggest a fix. The issue was related to the fact that the proto interface was not exported, and Copilot suggested adding a new pass for this case.

However, my trust in unit tests was shaken. The first problem I had with this approach was that the analyzer test could not load the external package. The source code for the unit test was generated by protoc, but due to its inability to import non-standard library packages, it had to be manually changed. Of course, not being able to check a real world code is not good.

The github.com/gomoni/sumlint/test package contains realistic test data with a file generated by protoc. The test file then imports real Go code, tests calls to go vet via os/exec. And compares the results.

package test

import "github.com/gomoni/sumlint/test/one_of"

type oneofTest struct{}

func (oneofTest) good(msg *one_of.Msg) {
	switch msg.GetPayload().(type) {
	case *one_of.Msg_A:
	case *one_of.Msg_B:
	default:
	}
}

func (oneofTest) noDefault(msg *one_of.Msg) {
	switch msg.GetPayload().(type) {
	case *one_of.Msg_A:
	case *one_of.Msg_B:
	}
}

func (oneofTest) noB(msg *one_of.Msg) {
	payload := msg.GetPayload()
	switch payload.(type) {
	case *one_of.Msg_A:
	default:
	}
}

In other words, integration test

  1. build linters
  2. cd test
  3. run go vet -vettool=/path/to/oneoflint .
  4. compare the stderr with expected output
$ (cd test; go vet -vettool=/path/to/oneoflint .)
# github.com/gomoni/sumlint/test
./oneof.go:16:2: missing default case on isMsg_Payload: code cannot handle nil interface
./oneof.go:24:2: non-exhaustive type switch on isMsg_Payload: missing cases for: github.com/gomoni/sumlint/test/one_of.Msg_B