Over a million developers have joined DZone.
{{announcement.body}}
{{announcement.title}}

go-critic: The Most Stubborn Static Analyzer for Go

DZone's Guide to

go-critic: The Most Stubborn Static Analyzer for Go

Learn how to get started with go-critic, a static analyzer, to improve your Go code's performance.

· Performance Zone ·
Free Resource

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" implementation of a check than not having it at all.

  • If the test is controversial, it does not mean that it cannot be useful. Marked as "opinionated" and infused.

  • To write 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'll look at the use and architecture of go-critic, some of the checks implemented in it, and describe the basic steps of adding its function-analyzer 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 warnings has been 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 support for the "whitelist" for the checks to explicitly list which checks need to be performed (the -enable flag). By default, all checks that are not marked with the Experimental or VeryOpinionated icon are started.

Integration with golangci-lint and gometalinter is planned.

How It All Began

By running another code-review of the Go project, or by auditing some third-party libraries, you can over and over again notice the same problems. To your regret, it was not possible to find a linter who would diagnose this class of problems.

The first step you can take is to attempt to categorize the problem and contact the authors of the existing linters, inviting them to add a new check. The chances that your proposal will be accepted strongly depend on the project and can be quite low. Next, most likely, months of waiting will follow.

And what if the test is altogether ambiguous and can someone be perceived as too subjective or inadequate? Maybe it makes sense to try to write this check yourself?

go-critic exists to become a home for experimental checks that are easier to implement by yourself than to attach them to existing static analyzers. The go-critic itself minimizes the number of contexts and actions that are required to add a new check-you can say that you only need to add one file (excluding tests).

How go-critic Works

A critic is a set of rules that describe validation properties and micro-liners (checkers) that implement code inspection for compliance with the rule. An application that builds a linter (for example, cmd/gocritic or golangci-lint) gets a list of supported rules, filters them in a certain way, creates a function for each selected check rule, and runs each of them over the package being examined.

The work of adding a new checker is reduced to three main steps:

  • Adding tests.

  • The implementation of the actual verification itself.

  • Add documentation for the linter.

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

Adding Tests

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

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

Examples:

// cmd/gocritic/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
}

// cmd/gocritic/testdata/negative_tests.go

func goodFunc1(in, x int) (x, y int) {
    v := 1
    return v, 0
}

You can run the tests after adding a new linter.

Implementation of the Audit

Create a file with the name of the checker: lint/captLocal_checker.go.
By convention, all files with micro-linters have the 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 default implementations, which allows you to write less code in each linter. Among other things, checkerBase includes a pointer to lint.context that contains information about types and other metadata about the file being checked.

The upcaseNames field will contain a table of known names that 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 an uppercase letter, but the correct replacement will not be provided.

The internal state is initialized once for each instance.

The Init () method must be defined only for those linters that need to be pre-initialized.

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

Now you need to define the test function itself. In the case of captLocal, we need to check all local ast.Ident, which introduces new variables.

In order to check all local definitions of names, it is necessary to implement in our checker a method with the following signature:

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.

// Ignore ast.Expr, because we are not interested in what value is assigned
// the local name. We are only interested in the names of the variables.
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 rendered in separate methods. There are rare exceptions, but following this rule is considered good practice.

The final touch is the registration of a new linter:

// Local for captLocal_checker.go init function.
func init() {
    addChecker(&captLocalChecker{}, attrSyntaxOnly)
}

addChecker expects a pointer to the zero-value of the new linter. Next, there is a variable argument that allows you to pass zero or several attributes describing 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 type checking. golangci-lint marks such linters with the flag "fast" (because they run much faster).

Now that the new linter is registered via addChecker, you can run the 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
# From there, you can run tests with make:
$ make test


Adding Documentation

To describe a new linter, you need to add a "magic" comment to the implementation file. Here is a comment from captLocal_checker.go:

//! Detects capitalized names for local variables.
//
// @Before:
// func f(IN int, OUT *int) (ERR error) {}
//
// @After:
// func f(in int, out *int) (err error) {}

The structure of this comment is strict and verified when generating documentation. In the simplest case, the documentation consists of a short one-line summary, Before and After sections.

Documentation Generation

Re-generate documentation is not a requirement for a new linter, perhaps in the near future, this step will be fully automated. But if you still want to check what the output markdown file looks like, use the make docs command. The docs/overview.md file will be updated.

Optimistic Merging (Almost)

When considering pull requests, we try to adhere to the strategy of optimistic merging. This is mainly expressed in the acceptance of that PR, to which the reviewer may have some, in particular purely subjective, claims. Immediately after the infusion of such a patch, you can follow the PR from the reviver, which corrects these shortcomings; in CC (copy), the author of the original patch is added.

We also have two-marker interest that can be used to avoid red flags in the absence of a full consensus:

  1. Experimental: an implementation can have a large number of false positive, be inefficient (the source of the problem is identified), or "fall" in some situations. You can pour this implementation if you mark it with the attrExperimental attribute. Sometimes with the help of experimental, those tests are indicated, which did not work out to pick a good name from the first commit.

  2. VeryOpinionated: if the check can have both defenders and enemies, it's worth to mark it with the attribute attrVeryOpinionated. Thus, we can avoid rejecting those ideas about the style of code that may not coincide with the taste of some gopher.

Experimental is a potentially temporary and irreparable property of realization. VeryOpinionated is a more fundamental property of a rule that does not depend on implementation.

It is recommended to create a [checker-request] ticket on GitHub before sending an implementation, but if you have already submitted a pull request, you can open an appropriate 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/known open-source projects and report false positives, false negatives, and other flaws. We will be grateful if you also add a note about the found/corrected problem to the page of trophies.

  • Offer ideas for new tests. It's enough to 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. Everyone can criticize, so join!

Topics:
go ,performance ,static analysis ,tutorial

Opinions expressed by DZone contributors are their own.

{{ parent.title || parent.header.title}}

{{ parent.tldr }}

{{ parent.urlSource.name }}