I want to share the results of the work of the last few days, which included an analysis of the git history of some large Go projects with the aim of finding commits, which corrected errors and then formalized them for detection if they appeared in the new code.

In the second part of the article, we will look at some new diagnostics in the go-critic , which allow us to find code that is highly error-prone.

Search for ideas in the past

To find ideas for new code inspections, you can pry into static analyzers for other programming languages, you can manually explore open source code with an attempt to find erroneous code templates in it, or you can look into history.

Suppose you are testing a Go-Files project. After launching the linter (static analyzer) the problems are not found, the audit of the source texts also did not bring much results. Should I hurry to put Go-Files aside? Not really.

During the existence of the Go-Files had some number of easily detected defects, but now they have already been fixed. What we need to do is start analyzing the git repository history.

Bugs somewhere nearby.

Collecting commits of interest

Problems:

• There can be a lot of commits.
• Sometimes unrelated things are performed in the same audit.
• Sometimes a comment on a commit does not quite match the actual changes.
• Some people underestimate the benefits of a good commit message (we will not point the finger)

First of all, we want to reduce the amount of information that will have to be processed without automation. The problem of false-negative responses can be solved by increasing the sample size (download more Go packets to the control group).

You can analyze it in different ways, it was enough for me to " git log --grep " on a set of templates, with overstatement and understating of the weight (score) of the commit depending on the content of its commit message. If the commit is too huge, you can immediately drop it, because it is very likely to figure out what is happening there will be nontrivial.

I would also like to add that, along with corrections or finding errors, quite often there are not very optimistic and sometimes not quite cultural descriptions of patches:

 - "We check length later, but we assumed it was always 1 bytes long. Not always the case. I'm a little depressed that this bug was there" - "Really fixed the bug now" - "WTF, the changes that actually fixed the bug for the accesspattern wasn't actually committed" - "how do i pronounce this damn project" ( traefic) - "damnit travis" - "one f*cking dot" (./.. => ./...  .travis.yml) 

The most useless "bug fixes" are editing syntax errors or other problems that prevent the project from even go build through the go build . Not all projects use CI or pre-commit hooks , so such broken revisions sometimes fall into the master branch.

Historical errors

Let's go over some of the most interesting bugs that were found under a blanket of git logs.

Return nil instead of the actual result

gin-gonic / gin: fix bug, return err when failed binding bool :

  if err == nil { field.SetBool(boolVal) } - return nil + return err } 

go-pg / pg: AppendParam bug fix :

  return method.AppendValue(b, m.strct.Addr(), 1), true } - return nil, false + return b, false } 

Absence of return in the fast path

gonum / gonum: fixed bug in dswap fast path :

  for i := 0; i < n; i++ { x[i], y[i] = y[i], x[i] } + return } 

Run gorutiny without wg.Add (1)

btcsuite / btcd: Fix several bugs in the RPC server shutdown :

 +s.wg.Add(1) go s.walletListenerDuplicator() 

Incorrect logical condition

gorgonia / gorgonia: fixed a few bugs as well :

 -for i := 0; i > start; i++ { +for i := 0; i < start; i++ { retVal[i] = 0 } 

src-d / go-git: plumbing / idxfile: fix bug searching in MemoryIndex :

 -if low < high { +if low > high { break } 

go-xorm / xorm: fix bug on sum :

 -if !strings.Contains(colName, " ") && strings.Contains(colName, "(") { +if !strings.Contains(colName, " ") && !strings.Contains(colName, "(") { 

btcsuite / btcd: server: Fix bug disconnecting peer on filteradd :

 -if sp.filter.IsLoaded() { +if !sp.filter.IsLoaded() { 

btcsuite / btcd: Fix reversed test bug with the max operations handling :

 -if pop.opcode.value < OP_16 { +if pop.opcode.value > OP_16 { s.numOps++ 

Update the receiver (this / self) transmitted by value

Classic. Inside the method, a copy of the object is modified, which is why the caller will not see the expected results.

Fixed a stack / queue / deque bug (pointer receiver) :

 -func (s Stack) Push(x interface{}) { - s = append(s, x) +func (s *Stack) Push(x interface{}) { + *s = append(*s, x) } 

Updating copies of objects within a loop

containous / traefik: Assign filtered tasks to apps contained in slice :

 -for _, app := range filteredApps { - app.Tasks = fun.Filter(func(task *marathon.Task) bool { +for i, app := range filteredApps { + filteredApps[i].Tasks = fun.Filter(func(task *marathon.Task) bool { return p.taskFilter(*task, app) }, app.Tasks).([]*marathon.Task) 

containous / traefik: Add unit tests for package safe :

 -for _, routine := range p.routines { +for i := range p.routines { p.waitGroup.Add(1) - routine.stop = make(chan bool, 1) + p.routines[i].stop = make(chan bool, 1) Go(func() { - routine.goroutine(routine.stop) + p.routines[i].goroutine(p.routines[i].stop) p.waitGroup.Done() }) } 

The result of the wrapping function is not used.

gorgonia / gorgonia: Fixed a tiny nonconsequential bug in Grad () :

 if !n.isInput() { - errors.Wrapf(err, ...) - // return + err = errors.Wrapf(err, ...) + return nil, err } 

Wrong work with make

Sometimes young gophers make " make([]T, count) " followed by append inside the loop. The correct option here is " make([]T, 0, count) ".

HouzuoGuo / tiedot: fix a collection scan bug :

 -ids := make([]uint64, benchSize) +ids := make([]uint64, 0) 

The correction above corrects the original error, but has one flaw that was corrected in one of the following commits:

 -ids := make([]uint64, 0) +ids := make([]uint64, 0, benchSize) 

But some operations, such as copy or ScanSlice may require the "correct" length.

go-xorm / xorm: bug fix for SumsInt return empty slice :

 -var res = make([]int64, 0, len(columnNames)) +var res = make([]int64, len(columnNames), len(columnNames)) if session.IsAutoCommit { err = session.DB().QueryRow(sqlStr, args...).ScanSlice(&res) } else { 

Soon go-critic will help in finding errors of this class.

The remaining errors

The listing turns out to be rather cumbersome, the rest of which I carry under the spoiler. This part is optional, so you can leave it for later or safely move on.

 -for _, el := range *s1 { +for el := range *s1 { if _, ok := (*s2)[el]; !ok { return false } 

 func (s *server) Query(ctx context.Context, - req *graph.Request) (resp *graph.Response, err error) { + req *graph.Request) (*graph.Response, error) { + resp := new(graph.Response) 

 -block, err := btcutil.NewBlockFromBytes(blockBytes) +block, err = btcutil.NewBlockFromBytes(blockBytes) if err != nil { return err } 

 -err = session.Rollback() +err1 := session.Rollback() +if err1 == nil { + return lastId, err +} +err = err1 

 -debugPrint("Listening and serving HTTP on %s", addr) +debugPrint("Listening and serving HTTP on %s\n", addr) 

  styleCache map[int]*Style `-` + lock *sync.RWMutex } 

Crusade against suspicious code

badCond

The badCond check finds potentially incorrect logical expressions.

Examples of suspicious logical expressions:

• The result is always true or false
• The expression is written in a form that is almost indistinguishable from the erroneous

This check also works on operations like " x == nil && x == y ". One simple solution is to rewrite to " x == nil && y == nil ". The readability of the code does not suffer, but the linter will not see anything suspicious in this code.

Here is an example of fixing a bug that can be found badCond :

 -if err1 := f(); err != nil && err == nil { +if err1 := f(); err1 != nil && err == nil { err = err1 } 

Trophies:

• moby / buildkit: snapshot: fix impossible condition
• tdewolff / parse: EqualFold from util.go has weird condition
• HouzuoGuo / tiedot: httpapi: suspicious condition in srv_test.go

weakCond

weakCond is similar to badCond , but the confidence level of warnings is somewhat lower.

A weak condition is a condition that does not sufficiently cover the input data.

A good example of a weak (insufficient) condition is to check a slice for nil with the subsequent use of the first element. The " s != nil " conditions are not enough. The correct condition is " len(s) != 0 " or its equivalent:

 func addRequiredHeadersToRedirectedRequests( req *http.Request, via []*http.Request) error { - if via != nil && via[0] != nil { + if len(via) != 0 && via[0] != nil { 

Trophies:

• etcd-io / etcd: etcdserver / api / v2v3: indexing after nil check without len ​​check
• moby / moby: registry: indexing after nil check without len ​​check
• inkyblackness / hacked: ss1 / content / text: indexing after nil check without len ​​check

dupArg

For some functions, passing the same value (or variable) as several arguments does not make much sense. Quite often this signals a copy / paste error.

An example of such a function is copy . The expression copy(xs, xs) does not make sense.

 -if !bytes.Equal(i2.Value, i2.Value) { +if !bytes.Equal(i1.Value, i2.Value) { return fmt.Errorf("tries differ at key %x", i1.Key) } 

Trophies:

• ethereum / go-ethereum: light: fix duplicated argument in bytes.Equal call
• openshift / origin: fix reflect.DeepEqual (x, x) bug

dupCase

You probably know that in Go you cannot use duplicate case values ​​inside a switch .

The example below does not compile :

 switch x := 0; x { case 1: fmt.Println("first") case 1: fmt.Println("second") } 

Compile error: duplicate case 1 in switch.

But what if you take the example more interesting :

 one := 1 switch x := 1; x { case one: fmt.Println("first") case one: fmt.Println("second") } 

Through the variable to use duplicate values ​​are allowed. In addition, the switch true gives even more room for errors:

 switch x := 0; true { case x == 1: fmt.Println("first") case x == 1: fmt.Println("second") } 

Here is an example of correcting a real error:

 switch { case m < n: // Upper triangular matrix. //   . case m == n: // Diagonal matrix. //   . -case m < n: // Lower triangular matrix. +case m > n: // Lower triangular matrix. //   . } 

Trophies:

• mellium / sasl: remove duplicated switch case
• gonum / gonum: fix duplicated switch case
• pact-foundation / pact-go: remove duplicated switch case
• minio / minio: suspicious duplicated switch case clause in cmd / object-api-error.go
• henrylee2cn / pholcus: common / bytes: remove duplicated switch case
• go-ble / ble: linux / att: fix duplicated switch case error
• docker / distribution: registry / storage: fix duplicated switch case

dupBranchBody

In conditional statements such as if/else and switch there are bodies for execution. When the condition is fulfilled, control is transferred to the associated block of operations. While other diagnostics check that the conditions of these blocks are different, dupBranchBody checks that the executable blocks themselves are not completely identical.

The presence of duplicate bodies within conditional statements, unless it is a type switch , is at least suspicious.

 -if s, err := r.ReceivePack(context.Background(), req); err != nil { - return s, err -} else { - return s, err -} +return r.ReceivePack(context.Background(), req) 

The judgments about the degree of correctness of the code below are left to the readers:

 if field.IsMessage() || p.IsGroup(field) { //   . } else if field.IsString() { if nullable && !proto3 { p.generateNullableField(fieldname, verbose) } else { pP(`if this.`, fieldname, ` != that1.`, fieldname, `{`) } } else { if nullable && !proto3 { p.generateNullableField(fieldname, verbose) } else { pP(`if this.`, fieldname, ` != that1.`, fieldname, `{`) } } } 

The bodies inside " if field.IsString() " and its associated " else " are identical.

Trophies:

• src-d / go-git: Suspicious duplicated branch bodies

caseOrder

All types inside the type switch iterated sequentially to the first compatible one. If the tag value is of type *T , and implements interface I , then you need to put a label with " case *T " up to the label with " case I ", otherwise only " case I " will be executed, since *T compatible with I (but not the other way around).

 case string: res = append(res, NewTargetExpr(v).toExpr().(*expr)) -case Expr: - res = append(res, v.toExpr().(*expr)) case *expr: res = append(res, v) +case Expr: + res = append(res, v.toExpr().(*expr)) 

Trophies:

• cmd / guru: fix incorrect order order in describe.go
• message / pipeline: fix type switch case order
• coyove / goflyway: proxy: fix case order in a type switch
• go-graphite / carbonapi: pkg / parser: fix type switch case order

offBy1

Error on the unit is so popular that it even has its own page on Wikipedia .

 if len(optArgs) > 1 { return nil, ErrTooManyOptArgs } -node = optArgs[1] +node = optArgs[0] 

 -last := xs[len(xs)] +last := xs[len(xs) - 1] 

There are limited possibilities to find such errors in the go-critic , but so far no corrections have been sent to the public repository. Here are some corrections that were found when viewing the story:

• btcsuite / btcd: test and fix bugs in getaddednodeinfo
• btcsuite / btcd: Improve test coverage and fix some bugs found by tests

A bit of horror in the end

go vet has a good check for expressions like " x != a || x != b ". It would seem that people might not know about the gometalinter , but almost everyone runs gometalinter go vet , right?

Using the gogrep utility , I compiled a small list of similar expressions that are still in the master branches of some projects.

 cloud.google.com/go/trace/trace_test.go:943:7: expectTraceOption != ((o2&1) != 0) || expectTraceOption != ((o3&1) != 0) github.com/Shopify/sarama/mocks/sync_producer_test.go:36:5: offset != 1 || offset != msg.Offset github.com/Shopify/sarama/mocks/sync_producer_test.go:44:5: offset != 2 || offset != msg.Offset github.com/docker/libnetwork/api/api_test.go:376:5: id1 != i2e(i2).ID || id1 != i2e(i3).ID github.com/docker/libnetwork/api/api_test.go:408:5: "sh" != epList[0].Network || "sh" != epList[1].Network github.com/docker/libnetwork/api/api_test.go:1196:5: ep0.ID() != ep1.ID() || ep0.ID() != ep2.ID() github.com/docker/libnetwork/api/api_test.go:1467:5: ep0.ID() != ep1.ID() || ep0.ID() != ep2.ID() github.com/docker/libnetwork/ipam/allocator_test.go:1261:5: len(indices) != len(allocated) || len(indices) != num github.com/esimov/caire/grayscale_test.go:27:7: r != g || r != b github.com/gogo/protobuf/test/bug_test.go:99:5: protoSize != mSize || protoSize != lenData github.com/gogo/protobuf/test/combos/both/bug_test.go:99:5: protoSize != mSize || protoSize != lenData github.com/gogo/protobuf/test/combos/marshaler/bug_test.go:99:5: protoSize != mSize || protoSize != lenData github.com/gogo/protobuf/test/combos/unmarshaler/bug_test.go:99:5: protoSize != mSize || protoSize != lenData github.com/gonum/floats/floats.go:65:5: len(dst) != len(s) || len(dst) != len(y) github.com/gonum/lapack/testlapack/dhseqr.go:139:7: wr[i] != h.Data[i*h.Stride+i] || wr[i] != h.Data[(i+1)*h.Stride+i+1] github.com/gonum/stat/stat.go:1053:5: len(x) != len(labels) || len(x) != len(weights) github.com/hashicorp/go-sockaddr/ipv4addr_test.go:659:27: sockaddr.IPPort(p) != test.z16_portInt || sockaddr.IPPort(p) != test.z16_portInt github.com/hashicorp/go-sockaddr/ipv6addr_test.go:430:27: sockaddr.IPPort(p) != test.z16_portInt || sockaddr.IPPort(p) != test.z16_portInt github.com/nats-io/gnatsd/server/monitor_test.go:1863:6: v.ID != c.ID || v.ID != r.ID github.com/nbutton23/zxcvbn-go/adjacency/adjcmartix.go:85:7: char != "" || char != " " github.com/openshift/origin/pkg/oc/cli/admin/migrate/migrator_test.go:85:7: expectedInfos != writes || expectedInfos != saves github.com/openshift/origin/test/integration/project_request_test.go:120:62: added != deleted || added != 1 github.com/openshift/origin/test/integration/project_request_test.go:126:64: added != deleted || added != 4 github.com/openshift/origin/test/integration/project_request_test.go:132:62: added != deleted || added != 1 gonum.org/v1/gonum/floats/floats.go:60:5: len(dst) != len(s) || len(dst) != len(y) gonum.org/v1/gonum/lapack/testlapack/dhseqr.go:139:7: wr[i] != h.Data[i*h.Stride+i] || wr[i] != h.Data[(i+1)*h.Stride+i+1] gonum.org/v1/gonum/stat/stat.go:1146:5: len(x) != len(labels) || len(x) != len(weights) 

Learn from the mistakes of others

No, even in the last part there will be nothing about machine learning .

But what it will be written here about is golangci-lint , which integrated go-critic in one of the latest releases. golangci-lint is an analogue of gometalinter , which you can read about the merits of, for example, in the article "Static analysis in Go: how we save time when checking code . "

The go-critic recently rewritten using lintpack . For details, go to "Go lintpack: composable linters manager" .

If you have not yet begun to actively use the tools to analyze your programs for potential errors, both stylistic and logical, I recommend today to think again about your behavior while having a cup of tea with your colleagues. You will succeed.

All good.