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 directories ( check-project). For example, you can check the whole $GOROOTor $GOPATHwith the help of 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 (flag -enable). By default, all those checks that are not marked with Experimentalor are launched 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-criticIt exists to become a home for experimental tests that are easier to implement yourself than to attach them to existing static analyzers. The device itself go-criticminimizes the amount of context and actions that are needed to add a new scan - you can say that you only need to add one file (not counting the tests).

How does the go-critic

A critic is a set of rules (rules) that describe the properties of a check and micro-linters (checkers) that implement code inspection for compliance with a 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 of them above the package being examined.

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

1. Adding tests.
2. Implementing the actual check 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. In order 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 capitalizedfuncbadFunc1(IN, X int)(Y, Z int) {
    /// `V' should not be capitalized
    V := 1return V, 0
}

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

You can run tests after adding a new linter.

Implementation of the check

Create a file called Checker: lint/captLocal_checker.go.
According to the convention, all files with micro-linter have a suffix _checker.

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 the types and other metadata about the file being scanned.

The field upcaseNameswill contain a table of known names, which we will propose to replace with the 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 method Init()needs to be defined only for those linters who need to carry out 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 captLocal, we need to check all local ast.Identvariables that introduce new variables.

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

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

The list of available visitor interfaces can be found in the lint / internal / visitor.go file .
captLocalimplements 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)
}

According to the 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 method to implement 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 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 функция.funcinit() {
    addChecker(&captLocalChecker{}, attrExperimental, attrSyntaxOnly)
}

addCheckerwaiting for 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- an optional marker for linters that do not use type information in their implementation, which allows them to run without performing a check for types. golangci-lintmarks such linters with the “fast” flag (because they are executed much faster).

attrExperimental- 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, a PR from a 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 if there is no complete consensus:

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

Experimental- a potentially temporary and fixable property of the implementation. VeryOpinionated- This is a more fundamental property of the rule, which does not depend on the implementation.

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

For more details on the details of 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-criticCriticizes your Go code with the voices of all programmers involved in its development. Anyone can criticize, so - join!