You may remember the recent announcement of a new static analyzer for Go called go-critic .

I checked the golang / go project with it and sent several patches that fix some of the problems found there.

In this article, we will analyze the corrected code, and we will also be motivated to send even more similar changes to Go.

For the most impatient: updated list of trophies .

dupSubExpr

We all make mistakes, and, quite often, through carelessness. Go, being a language in which sometimes you have to write boring and patterned code, sometimes contributes to typos and / or copy / paste errors.

CL122776 contains a fix for a bug found by checking dupSubExpr :

 func (a partsByVarOffset) Less(i, j int) bool { - return varOffset(a.slots[a.slotIDs[i]]) < varOffset(a.slots[a.slotIDs[i]]) + return varOffset(a.slots[a.slotIDs[i]]) < varOffset(a.slots[a.slotIDs[j]]) // ^__________________________________^ } 

Pay attention to the index on the left and right. Before the fix, the LHS and RHS operator < were identical, which was what dupSubExpr worked dupSubExpr .

commentedOutCode

If your project is under the auspices of the version control system , then instead of disabling the code by wrapping it in a comment, you should delete it completely. There are exceptions, but more often such a "dead" code interferes, confuses and can hide errors.

commentedOutCode could find such an interesting fragment ( CL122896 ):

 switch arch.Family { // ...  case clause. case sys.I386: return elf.R_386(nr).String() case sys.MIPS, sys.MIPS64: // return elf.R_MIPS(nr).String() // <- 1 case sys.PPC64: // return elf.R_PPC64(nr).String() // <- 2 case sys.S390X: // return elf.R_390(nr).String() // <- 3 default: panic("unreachable") } 

A bit higher there is a comment:

 // We didn't have some relocation types at Go1.4. // Uncomment code when we include those in bootstrap code. 

If you switch to the go1.4 branch and remove these 3 lines from the comment, the code will not compile, but if you uncomment them on the wizard, everything will work.

Typically, code hidden in a comment requires either removal or reverse activation.

It is useful, from time to time, to visit such echoes of the past in your code.

boolExprSimplify

Sometimes people write strange code. Perhaps it seems to me, but logical ( boolean ) expressions sometimes look particularly strange.

Go has a great x86 assembler backend (a tear drops here), but the ARM is really guilty:

 if !(o1 != 0) { break } 

"If not o1 is not equal to 0" ... Double negation is a classic. If you like, I invite you to familiarize yourself with CL123377 . There you can see the corrected version.

 - if !(o1 != 0) { + if o1 == 0 { 

boolExprSimplify is aimed at simplifications that increase readability (and the Go optimizer would have coped with the performance issue without it).

underef

If you use Go from its earlier versions, you can remember the mandatory semicolons, the lack of automatic pointer dereferencing, and other features that are almost impossible to see in the new code today.

In the old code, you can still find something like this:

 // -    : buf := (*bufp).ptr() // ...     : buf := bufp.ptr() 

Several underef analyzer triggers are fixed in CL122895 .

appendCombine

You may know that append can take multiple arguments as elements added to the target slice. In some situations, this allows you to slightly improve the readability of the code, but, what may be more interesting, it can also speed up your program, since the compiler does not execute the append compatible calls ( cmd / compile: combine append calls ).

In Go, an appendCombine check found the following site:

 - for i := len(ip) - 1; i >= 0; i-- { - v := ip[i] - buf = append(buf, hexDigit[v&0xF]) - buf = append(buf, '.') - buf = append(buf, hexDigit[v>>4]) - buf = append(buf, '.') - } + for i := len(ip) - 1; i >= 0; i-- { + v := ip[i] + buf = append(buf, hexDigit[v&0xF], + '.', + hexDigit[v>>4], + '.') + } 

 name old time/op new time/op delta ReverseAddress-8 4.10µs ± 3% 3.94µs ± 1% -3.81% (p=0.000 n=10+9) 

Details in CL117615 .

rangeValCopy

It is no secret to anyone that the values ​​enumerated in the range cycle are copied. For objects of small size, say, less than 64 bytes, you may not even notice. However, if such a cycle lies on a “hot” path, or, over which you are iterating, contains a very large number of elements, the overhead can be tangible.

Go is a fairly slow linker (cmd / link), and without significant changes in its architecture, a strong performance increase cannot be achieved. But it is possible to slightly reduce its inefficiency with the help of micro-optimizations. Each percentage is two in the account.

Check rangeValCopy found several cycles at once with unwanted copying of data. Here are the most interesting ones:

 - for _, r := range exports.R { - d.mark(r.Sym, nil) - } + for i := range exports.R { + d.mark(exports.R[i].Sym, nil) + } 

Instead of copying R[i] at each iteration, we only refer to the only member of interest to us, Sym .

 name old time/op new time/op delta Linker-4 530ms ± 2% 521ms ± 3% -1.80% (p=0.000 n=17+20) 

The full version of the patch is available at the link: CL113636 .

namedConst

In Go, unfortunately, named constants, even when assembled into groups, are not related to each other and do not form an enumeration ( proposal: proposal: typed enum support ).

One of the problems is casting untyped constants to the type that you would like to use as enum.

Suppose you have defined the type Color , it has the value const ColDefault Color = 0 .
Which of these two code snippets do you like more?

 // (A) if color == 0 { return colorBlack } // (B) if color == colorDefault { return colorBlack } 

If (B) seems more appropriate to you, checking namedConst will help you track the use of values ​​of named constants bypassing the named constant itself.

This is how the context.mangle method from the html/template package changed:

  s := templateName + "$htmltemplate_" + c.state.String() - if c.delim != 0 { + if c.delim != delimNone { s += "_" + c.delim.String() } - if c.urlPart != 0 { + if c.urlPart != urlPartNone { s += "_" + c.urlPart.String() } - if c.jsCtx != 0 { + if c.jsCtx != jsCtxRegexp { s += "_" + c.jsCtx.String() } - if c.attr != 0 { + if c.attr != attrNone { s += "_" + c.attr.String() } - if c.element != 0 { + if c.element != elementNone { s += "_" + c.element.String() } return s 

By the way, sometimes on the links to the patches you can find interesting discussions ...
CL123376 is one such case.

unslice

The feature of slice expression is that x[:] always identical to x , if type x is a slice or string. In the case of slices, this works for any type of []T element.

Everything on the list below is the same thing ( x is a slice):

• x
• x[:]
• x[:][:]
• ...

unslice finds similar redundant slice expressions. These expressions are harmful, above all, by an extra cognitive load. x[:] has quite significant semantics in the case of taking a slice from an array. A slice cut with ranges by default does nothing but noise.

For the patch I ask in CL123375 .

switchTrue

In CL123378 , the " switch true {...} " was replaced with " switch {...} ".
Both forms are equivalent, but the second is more idiomatic.
Found by checking switchTrue .

Most stylistic checks reveal exactly such cases, where both options are permissible, but one of them is more common and familiar to more Go programmers. Next check in the same series.

typeUnparen

Go, like many other programming languages, loves parentheses. So much so that he is ready to accept any number of them:

 type ( t0 int t1 (int) t2 ((int)) // ... ,  . ) 

But what happens if you run gofmt ?

 type ( t0 int t1 (int) // <- !   . t2 (int) // <-    ... ) 

That is why there is a typeUnparen . He finds in the program all type expressions in which the number of parentheses can be reduced. I tried to send CL123379 , let's see how the community will accept it.

go-critic in the service of Go

We considered only a small part of the implemented checks. However, their quantity and quality will only evolve over time, including thanks to those people who have joined the development .

The go-critic is absolutely free for any use ( MIT license ), and is also open to your participation in the development of the project. Send us ideas for checks, you can immediately with the implementation, report on errors found and flaws, share your impressions. You can also suggest projects for an audit or report on the Go code review you have conducted; this experience is invaluable to us.

Contributing topic in Go

Have you seen the article Go contribution workshop in Russia ? This fall will be the second round. And this time, besides a more successful format and sponsors, we will have a secret weapon - a wonderful static analyzer. There will be enough money for everyone!

But in fact, you can start right now (although it is better - a little later, after the code freeze ). If you manage to get used to the next workshop, it will be very great, because we lack a lot of mentors in Russia.