We are announcing a new linter (static analyzer) for Go , which is also a sandbox for prototyping your ideas in the world of static analysis.

go-critic is built around the following observations:

• It is better to have a “good enough” verification implementation than not to have it at all
• If the check is controversial, it does not mean that it can not be useful. Mark as “opinionated” and pour in
• Writing a linter from scratch is usually more difficult than adding a new check to an existing framework if the framework itself is easy to understand.

In this post we will look at the use and architecture of the go-critic, some of the checks implemented in it , and also describe the main steps of adding our analyzer function to it.

Fast start

$ cd $GOPATH $ go get -u github.com/go-critic/go-critic/... $ ./bin/gocritic check-package strings $GOROOT/src/strings/replace.go:450:22: unslice: could simplify s[:] to s $GOROOT/src/strings/replace.go:148:2: elseif: should rewrite if-else to switch statement $GOROOT/src/strings/replace.go:156:3: elseif: should rewrite if-else to switch statement $GOROOT/src/strings/replace.go:219:3: elseif: should rewrite if-else to switch statement $GOROOT/src/strings/replace.go:370:1: paramTypeCombine: func(pattern string, value string) *singleStringReplacer could be replaced with func(pattern, value string) *singleStringReplacer $GOROOT/src/strings/replace.go:259:2: rangeExprCopy: copy of r.mapping (256 bytes) can be avoided with &r.mapping $GOROOT/src/strings/replace.go:264:2: rangeExprCopy: copy of r.mapping (256 bytes) can be avoided with &r.mapping $GOROOT/src/strings/strings.go:791:1: paramTypeCombine: func(s string, cutset string) string could be replaced with func(s, cutset string) string $GOROOT/src/strings/strings.go:800:1: paramTypeCombine: func(s string, cutset string) string could be replaced with func(s, cutset string) string $GOROOT/src/strings/strings.go:809:1: paramTypeCombine: func(s string, cutset string) string could be replaced with func(s, cutset string) string $GOROOT/src/strings/strings.go:44:1: unnamedResult: consider to give name to results $GOROOT/src/strings/strings.go:61:1: unnamedResult: consider to give name to results $GOROOT/src/strings/export_test.go:28:3: rangeExprCopy: copy of r.mapping (256 bytes) can be avoided with &r.mapping $GOROOT/src/strings/export_test.go:42:1: unnamedResult: consider to give name to results 

(The formatting of the warnings is edited, the originals are available in the gist .)

The gocritic utility can check individual packages by their import path ( check-package ), as well as recursively bypass all the directories ( check-project ). For example, you can check the entire $GOROOT or $GOPATH with one command:

 $ gocritic check-project $GOROOT/src $ gocritic check-project $GOPATH/src 

There is whitelist support for checks to explicitly list which checks need to be performed (the -enable flag). By default, all those checks that are not marked with the Experimental or VeryOpinionated icon are VeryOpinionated .

Integration into golangci-lint and gometalinter is planned .

How it all began

Conducting another Go project code review, or when auditing a certain 3-rd party library, you can notice the same problems over and over again.

Unfortunately, it was not possible to find a linter who would diagnose this class of problems.

Your first action may be an attempt to categorize the problem and contact the authors of existing linkers, inviting them to add a new check. The chances that your proposal will be accepted depends heavily on the project and can be quite low. Further, most likely, there will be months of waiting.

And what if verification is completely ambiguous and can be perceived by someone as too subjective or not accurate enough?

Maybe it makes sense to try to write this check yourself?

go-critic exists to become a home for experimental tests, which are easier to implement yourself than to attach them to existing static analyzers. The go-critic device itself minimizes the amount of context and actions that are needed to add a new check - we can say that you only need to add one file (not counting the tests).

How does the go-critic

The critic is a set of rules (rules) that describe the properties of the check and micro-linters (checkers) that implement the inspection code for compliance with the rule.

An application that embeds a linter (for example, cmd / gocritic or golangci-lint ), retrieves a list of supported rules, filters them in a certain way, creates a check function for each selected rule, and runs each one of them over the package being examined.

The job of adding a new checker comes down to three main steps:

1. Adding tests.
2. The implementation is directly validation itself.
3. Adding documentation for the linter.

Let's go through all these points using the example of the captLocal rule, which requires the absence of local names starting with a capital letter.

Adding tests

To add test data for a new check, you need to create a new directory in lint / testdata .

Each such directory should have a file positive_tests.go , which describes examples of code on which the test should work. To test the absence of false positives, the tests are supplemented with “correct” code, in which the new check should not find any problems ( negative_tests.go ).

Examples:

 // lint/testdata/positive_tests.go /// consider `in' name instead of `IN' /// `X' should not be capitalized /// `Y' should not be capitalized /// `Z' should not be capitalized func badFunc1(IN, X int) (Y, Z int) { /// `V' should not be capitalized V := 1 return V, 0 } 

 // lint/testdata/negative_tests.go func goodFunc1(in, x int) (x, y int) { v := 1 return v, 0 } 

You can run tests after adding a new linter.

Check implementation

Create a file with the name of the checker: lint/captLocal_checker.go .
By convention, all micro-linter files have the _checker suffix.

 package lint //  “Checker”    . type captLocalChecker struct { checkerBase upcaseNames map[string]bool } 

checkerBase is a type that must be embedded in each checker.
It provides a default implementation, which allows you to write less code in each linter.
Among other things, checkerBase includes a pointer to lint.context , which contains information about types and other metadata about the file being scanned.

The upcaseNames field will contain a table of known names, which we will propose to replace with strings.ToLower(name) version. For those names that are not contained in the map, it will be suggested not to use a capital letter, but no correct replacement will be provided.

The internal state is initialized once for each instance.
The Init() method is required to be defined only for those linters who need to perform preliminary initialization.

 func (c *captLocalChecker) Init() { c.upcaseNames = map[string]bool{ "IN": true, "OUT": true, "INOUT": true, } } 

Now we need to determine the checking function itself.
In the case of captLocal , we need to check all local ast.Ident , which introduce new variables.

In order to check all local definitions of names, you should implement a method with the following signature in your checker:

 VisitLocalDef(name astwalk.Name, initializer ast.Expr) 

A list of available visitor interfaces can be found in the lint / internal / visitor.go file .
captLocal implements LocalDefVisitor .

 //  ast.Expr,         //  .      . func (c *captLocalChecker) VisitLocalDef(name astwalk.Name, _ ast.Expr) { switch { case c.upcaseNames[name.ID.String()]: c.warnUpcase(name.ID) case ast.IsExported(name.ID.String()): c.warnCapitalized(name.ID) } } func (c *captLocalChecker) warnUpcase(id *ast.Ident) { c.ctx.Warn(id, "consider `%s' name instead of `%s'", strings.ToLower(id.Name), id) } func (c *captLocalChecker) warnCapitalized(id ast.Node) { c.ctx.Warn(id, "`%s' should not be capitalized", id) } 

By convention, the methods that generate warnings are usually put into separate methods. There are rare exceptions, but following this rule is considered good practice.

Adding documentation

Another necessary implementation method is InitDocumentation :

 func (c *captLocalChecker) InitDocumentation(d *Documentation) { d.Summary = "Detects capitalized names for local variables" d.Before = `func f(IN int, OUT *int) (ERR error) {}` d.After = `func f(in int, out *int) (err error) {}` } 

Usually, it is enough to fill in 3 fields:

• Summary - description of the validation action in one sentence.
• Before - code before the correction.
• After - code after correction (should not cause warnings).

Register a new linter and run tests

The final touch is the registration of the new linter:

 //   captLocal_checker.go init . func init() { addChecker(&captLocalChecker{}, attrExperimental, attrSyntaxOnly) } 

addChecker expects a pointer to the zero-value of the new linter. Next comes a variable argument that allows you to pass zero or more attributes that describe the properties of the implementation of the rule.

attrSyntaxOnly is an optional marker for linters that do not use type information in their implementation, which allows them to run without performing a type check. golangci-lint marks such linters with the “fast” flag (because they are executed much faster).

attrExperimental is an attribute assigned to all new implementations. The removal of this attribute is possible only after stabilization of the implemented check.

Now that the new linter is registered through addChecker, you can run the tests:

 #  GOPATH: $ go test -v github.com/go-critic/go-critic/lint #  GOPATH/src/github.com/go-critic/go-critic: $ go test -v ./lint #  ,      make: $ make test 

Optimistic merging (almost)

When considering pull requests, we try to adhere to the optimistic merging strategy. This is mainly reflected in the acceptance of those PRs to which some reviewers may have some, in particular purely subjective, claims. Immediately after the infusion of such a patch, the PR from the revisioner, which corrects these shortcomings, may follow, the author of the original patch is added to CC (copy).

We also have two linter markers that can be used to avoid red flags in the absence of full consensus:

1. Experimental : an implementation can have a large number of false-positive, inefficient (the source of the problem is identified) or “fall” in some situations. This implementation can be attrExperimental if you mark it with the attribute attrExperimental . Sometimes with the help of experimental those checks that could not find a good name from the first commit are identified.
2. VeryOpinionated : if the check can have both defenders and enemies, it is worth attrVeryOpinionated it with the attrVeryOpinionated attribute. In this way, we can avoid rejecting those ideas about code style that may not match the taste of some gophers.

Experimental is a potentially temporary and fixable property of an implementation. VeryOpinionated is a more fundamental property of the rule that does not depend on the implementation.

It is recommended to create a [checker-request] ticket on github before submitting an implementation, but if you have already sent a pull request, you can open the corresponding issue for you.

For more details about the development process, see CONTRIBUTING.md .
Basic rules are listed in the main rules section.

Parting words

You can participate in the project not only by adding new linters.
There are many other ways:

• Try it on your projects or large / well-known open-source projects and report false positives, false negatives and other flaws. We would be grateful if you also add a note about the problem found / fixed on the trophies page.
• Offer ideas for new checks. Just create an issue on our tracker.
• Add tests for existing linters.

go-critic criticizes your Go code with the votes of all the programmers involved in its development. Anyone can criticize, so - join!