Welcome go-critic
We’re announcing a new linter for Go that also will be a sandbox for prototyping your ideas in the world of static code analysis.
go-critic is based on these observations:
- It’s better to have a “good enough” implementation rather than don’t have it at all.
- If a check is opinionated it doesn’t mean that it isn’t useful at all, we can mark it as “opinionated” and give it a try.
- Writing yet another linter from scratch is always harder than adding a new check to an existing platform that is simple and well supported.
In this article we’re going to describe the architecture of go-critic, some already implemented checks and write a new check from scratch.
Quick start
$ cd $GOPATH
$ go get -u github.com/go-critic/go-critic/...
$ ./bin/gocritic check-package strings
gocritic program is able to check separate packages by import path (check-package command) and recursively scan the whole tree for a given directory (check-package command). For example, you can run gocritic over whole GOROOT or GOPATH by one of this command:
$ gocritic check-project $GOROOT/src
$ gocritic check-project $GOPATH/src
There are plans to add go-critic to gometalinter & golangci-lint.
How it was started
While doing a regular code review of a Go project or auditing a 3rd party lib you can see the same problems again and again.
Unfortunately, you’ve failed to find a linter that could detect such problems.
Your first step could be problem categorization, then contacting some existing linter maintainers to propose a new check. Chances that your proposal will be approved & merged heavily depends on the project and might be quite low. Also, this might take some time…
But what if your check is controversial and someone might decide that it’s very opinionated?
go-critic was created to become a sandbox and a home for such checks, that is easier to implement rather than asking to merge into existing linter.
go-critic minimizes the amount of work to be done to create a new linter. We can say that it’s enough to add only one file (except tests 😉).
How does go-critic works?
Critic is a collection of rules that describe diagnostic properties and micro-linters (checkers) that perform source code inspection to find rule violations.
Application that embeds linter (ex. cmd/gocritic or golangci-lint) receives a list of supported rules, filters them by their properties, creates a check function and runs each of them on analyzing package.
Adding new checker consists of 3 steps:
- Adding tests
- Adding implementation of check
- Adding documentation for a linter
Let’s go through all of this steps for captLocal linter that suggests renaming parameters those name start with a capital letter.
Adding new tests
To add testing data for a new check we need to create a new directory in cmd/gocritic/testdata.
Each of this directories must have a positive_tests.go that describes code examples that must trigger check. To test absence of false positive cases we can (and should 😉) create negative_tests.go where check must not be triggered.
Examples
To run test it’s enough to run one of this command:
$ go test -v ./...
# or
$ make test
Implementing a checker
Let’s create a lint/captLocal_checker.go
By convention all files with checks have suffix _checker.go
checkerBase is a type that must be embedded in every checker. It presents default implementation of useful methods that allow to write less code in every linter.
checkerBase also includes a pointer to lint.context that contains types info and other metadata about the package being checked.
Field upcaseNames will contain a map of well-known names that we will propose to replace with a lowercase version of them. For all other names we’ll just suggest not using capitalized form.
The internal state is initialized once for each checker instance.
Init method must be defined only for those linter that needs some state initialization.
Now we’re going to implement verification function. In case of captLocal we need to verify all ast.Ident nodes that provide new variables.
To verify all local variables definitions we need to implement a method with this signature:
VisitLocalDef(name astwalk.Name, initializer ast.Expr)
List of all available visitor interfaces can be found in lint/internal/visitor.go
captLocal implements LocalDefVisitor.
By convention methods that triggers warning are excluded to separate methods. There are a rare exceptions, but following this rule is a good practice.
And the last and very important step:
addChecker expects a pointer to zero-value of a new linter and an optional variadic parameter that allows us to specify linter’s attributes.
attrSyntaxOnly is an optional attribute for linters that doesn’t depend on information about types in verifying code. For example, golang-lint marks such linters as fast (’cause they’re fast 😉).
Now, when we’ve registered new linter via addChecker it’s possible to run tests:
# From GOPATH:
$ go test -v github.com/go-critic/go-critic/cmd/gocritic
# From GOPATH/src/github.com/go-critic/go-critic:
$ go test -v ./cmd/gocritic
# or run it as a make command:
$ make test
Adding documentation
To describe new linter we need to add a “magic” comment inside implementation file, here is a comment from captLocal_checker.go
Structure of this comment is strict and is verified during documentation generation. Simplest documentation comment consist of short one-line summary and two sections Before and After.
Optimistic merging, almost…
During pull request review we’re trying to follow optimistic merging. In general, it means that we’re going to merge some PR’s with small inconsistency or places for future improvements. Right after that PR merge there might be a next PR from a reviewer that will fix leftovers from the previous PR. Original patch author will be CCed in this follow-up PR 😉.
There are two markers for linter that will avoid its rejection if a consensus wasn’t met:
Experimental— implementation might have a big amount of false positives, be ineffective (but the source of a problem is defined) or fail in some corner cases. This kind of implementation can be merged withattrExperimentalattribute. This attribute can be also used for linters with an unclear name.VeryOpinionated— for implementation of linter that has cons and pros. This will prevent linters from being rejected due to some code style preferences differences between gophers.attrVeryOpinionatedattribute is used for this linters.
We recommend to create a [checker-request] issue on github before sending an implementation of it, but if you did an implementation before and have pushed PR — no problem, someone else will make an issue for you.
More information regarding development process can be found in CONTRIBUTING.md.
Main rules are described in main rules section.
Afterwords
There are ways to contribute other than adding a new check, for example:
- Run it on your or some else’s code, on big or small projects, on open or closed source repositories, share false positives, false negatives or other features, interesting findings, whatever you think is interesting to share. We appreciate if you will add a note to trophies page.
- Propose new ideas. It is enough to add a new issue.
- Add tests for existing checks.
go-critic criticizes your code using voices of many Go developers.
Everyone can criticize — join us!
Original: https://habr.com/post/414739/
