openlynt: a linter for go

February 2020 ยท 6 minute read

At Openly, Inc, we talk as a team every day. We do our best to provide the smoothest experience for our customers - and to improve our codebase and ourselves as developers as much as possible.

We have a lot of code written in Go, and thankfully, the developer tooling for the language is fairly mature.

Gone are the days where one had to manage versions of dependencies and change workspaces if you had multiple distinct projects. Code-completion is supported by the Go team via gopls. Go modules are a mostly-good solution to the dependency problem.

For linting, there are several tools, but the one I see used most is golangci-lint. It offers a large number of vendored lint tools and nearly all of them are configurable.

We found ourselves wanting more control over specific rules - or completely new ones that no current linting tool offers.

Instead of forking golangci-lint and thus the linters it uses - and carrying the burden of maintaining our own fork - we decided to add to the pile of lint tools available for Go - in the hopes that it could be flexible enough to enforce our specific rules and potentially be useful to the wider community.

Enter openlynt

openlynt is our first-pass at a linter. It’s a work-in-progress; the idea was born out of an internal discussion related to how we implement certain features - one particularly easy to accidentally miss.

This is a contrived example - and not exactly what we do, but at least illustrates what I’m talking about.

package v1

var ExportedValue = 2
package main

import (
	xyzV1 "company/pkg/xyz/v1"
)

...

Simple enough. v1 works fine, but we need to make some changes while also supporting xyz v1. An easy way for us to do this is by making a new package, bootstrapping it from v1. Now, we have a v2:

package v2

var ExportedValue = 3

Generally, these packages are expected to work the same. One might accidentally copy the v1 import without noticing. While we haven’t had this occur in a release, it’s easy to get lost during development when something isn’t working as expected. Here’s what our main.go looks like when we screw up.

package main

import (
	xyzV1 "company/pkg/xyz/v1"
	xyzV2 "company/pkg/xyz/v1"
)

v1 gets imported twice but with different names. gocritic could catch this with -enable=dupImport, but this is disabled by default. We could enable it, which would catch this issue, but this only works if both imports are in the same file - which isn’t always the case. Additionally, it does not help us to enforce a specific naming scheme for these packages - which, depending on the type of package - is very important to us.

What we ended up implementing in openlynt for this was a lint rule that checks all import statements’ path value. If it matches a configured path, it must be named a certain way.

An example rule:

rules:
  rule_pkgprefix:
    type: import
    if:
      path: \/pkg\/type\/v(?P<version>[0-9]+)

    require:
      name: TYPEv${version}

If package pkg/type/v1 is imported anywhere, it must be a named import and must be named TYPEv1. Failure to follow this standard results in unmergable code by way of our CI.

package main

import (
	"pkg/type/v1" // unnamed, will fail
	typeV2 "pkg/type/v2" // named incorrectly
)

The linter will also attempt to be helpful by telling us what name this should have been, eg:

main.go:4: expected import "pkg/type/v1" to be named "TYPEv1", but it was ""
main.go:5: expected import "pkg/type/v2" to be named "TYPEv2", but it was "typeV2"

While the current configuration scheme uses regexps to match, the tool does work with the go/ast package to find certain parts of your code - it isn’t using regexps to find import statements.

Another Example: enforcing comment structure

Personally, I think it is good practice to place TODOs in code where applicable. There may be something that needs to be done in a section of a codebase that isn’t required now or simply isn’t ready to be done.

However, not all TODOs are created equal: maybe there’s not enough details for another developer who comes in to work on something and it isn’t clear exactly what the problem is. Maybe there’s no link to the issue tracker that would further illuminate the problem.

Previously, we used godox to catch all TODOs and filtered them if they contained a link to our issue tracker. Unfortunately, godox does not care if you have a comment section like this:

// TODO: we need to fob `x` with `y` in order to ensure that the `foobar` will
// remain stable in all situations. This isn't a problem now, but it will be
// once #999 is implemented.
// ref: https://issuetracker.com/issues/999
// ref: https://issuetracker.com/issues/1000

Because godox checks comments individually and prints them, we can’t filter them out even if the whole section contains a link.

An additional problem is that of context: we want some amount of context to be placed in TODO comments. While a link to the issue tracker is useful, we don’t feel that another developer should have to hop into a separate system, potentially losing their place in their work (or review) just to read about the nature of the problem. Maybe the issue tracker isn’t super specific about the code. The developer writing the TODO, however, can (should) leave implementation-specific information.

openlnyt solves this problem for us by treating comments as groups, whether they are multiple lines of // or a single grouped /*.

An example rule that enforces both length of the comment (requring context) and a link to the issue tracker:

rules:
  rule_todo:
    type: comment_group
    name: TODO requirements

    if:
      text: TODO

    require:
      text: "https?://issue-tracker.com/issues/\\d+"
      len: 2

The following code would fail to pass this lint with an error because it does not include the required link nor does it met the minimum line count (context):

// TODO: this is broken, fix it

As would this, as it does not meet the minimum line count (context):

// TODO: fix this, see https://issue-tracker.com/issues/1000

While our example from earlier would pass with flying colors:

// TODO: we need to fob `x` with `y` in order to ensure that the `foobar` will
// remain stable in all situations. This isn't a problem now, but it will be
// once #999 is implemented.
// ref: https://issuetracker.com/issues/999
// ref: https://issuetracker.com/issues/1000

Where is openlynt going?

As a small team, we’re very fortunate that we can discuss things together easily and come to consensus. For larger teams, this requires much more buy-in and discussion.

We’re still working on coming up with our own standards, so it will take time to work up exactly what we need to be able to support. The codebase itself still needs to be cleaned up and properly architected - the idea came up as a proof-of-concept during one of our discussions, so a proper design is a must.

At the very least, openlynt serves as another example of how fast one can get going with Go’s ast.

If you have comments, suggestions, or things you’d like to see implemented, feel free to visit our project’s issue tracker and talk about it. We very much want to give back to the Go community.