Contributing to Go with go-critic

You might remember announce of go-critic last month.

We’ve verified golang/go repository and have send few patches, that are fixing found problems. In this post, we will inspect the suggested changes.

You can find list of go-critic patches on trophies page.

List of patches covered in this post:

1. net: combine append calls in reverseaddr appendCombine
2. cmd/link/internal/ld: avoid Reloc copies in range loops rangeValCopy
3. cmd/compile/internal/ssa: fix partsByVarOffset.Less method dupSubExpr
4. runtime: remove redundant explicit deref in trace.go underef
5. cmd/link/internal/sym: uncomment code for ELF cases in RelocName commentedOutCode
6. runtime: simplify slice expression to sliced value itself unslice
7. html/template: use named consts instead of their values namedConst
8. cmd/internal/obj/arm64: simplify some bool expressions boolExprSimplify
9. math,net: omit explicit true tag expr in switch switchTrue
10. archive/tar: remore redundant parens in type expressions typeUnparen

dupSubExpr

Everyone makes mistakes. In Go sometimes you may write boring and boilerplate code that increases copy/paste error probability.

CL122776 contains bugfix that was found by dupSubExpr checker:

Take a look at index on the right and on the left. They were identical before changes. dupSubExpr is created exactly for this situation, when left and right expression are identical.

commentedOutCode

Hopefully your project is under VCS and instead of disabling code via commenting it — it’s better to remove it completely. There’re exceptions, of course, but `dead code` is usually redundant and can hide bugs.

commentedOutCode has found this interesting code (CL122896):

Above that code we can see a comment:

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

Switching to go1.4 branch and uncommenting this 3 lines will prevent code from a compilation, but uncommenting them on amaster branch will be ok.

Usually, the code is commented-out should be totally deleted or uncommented. It’s a good practice to review such old parts of the code and decide their meaning for the future (It’s one of my lovely checks, but also it’s a `noisy` one).

There are a lot of false positives for math/big package and inside the compiler. In the first case, they’re explaining operations, in the second case — which AST code it is processing. Programmatically decide what comment is just a comment and what is real commented code (might be `dead` code) — isn’t an easy task. Sadly.

Here is the idea: what about to mark commented-out code which explains behaviour of program from just a commented-out code? This might be a trivial thing, but it will help future readers a lot (by example, a code in comments can be prepended with a # character).

Another category — comments with explicit TODO. If the code was commented, but it has an explanation why it’s commented. For this case it’s better not to warn. It’s already implemented, but might work better.

boolExprSimplify

Sometimes people are writing strange code. For me, boolean expressions are a pain point more often than others.

Let’s look at this code:

if !(o1 != 0) {
    break
}

“If not o1 isn’t equal to zero” — double negation, classical. If you agree with me that it’s hard to read, here is a patch CL123377 with a simplification.

boolExprSimplify is targeting on boolean expression simplification, that will improve code readability (question regarding performance will be handled by Go optimizer).

underef

If you’re using Go from early versions you might remember when ; was mandatory, the absence of automatic pointer dereference and other `features` that aren’t presented now.

In old Go code you might found something like this:

// Long time ago there was not automatic pointer dereference:
buf := (*bufp).ptr()
// ...now it can be as simple as:
buf := bufp.ptr()

Few underef checker triggers were fixed here: CL122895.

appendCombine

You might know that append can receive a variable number of arguments. In most cases this allows to improve readability but also this might improve performance too, ’cause compiler doesn’t combine a sequence of append calls (cmd/compile: combine append calls).

In Go repo appendCombine checker has found such code:

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’s not a secret that value in the range loop are copying. For a small objects (let’s say less than 64 bytes) you might not see a difference. But when it’s a `hot loop` and objects aren’t small enough — performance degradation might occur.

Go has a slightly slow linker (cmd/link) and without big changes in architecture achieving great performance boost isn’t so easy. But we’ve micro-optimizations 😉.

rangeValCopy check has found few loops with unwanted data copying. Here is the most interesting one:

Instead of copying R[i] on every iteration we’re accessing only 1 interesting field for us Sym.

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

Full patch is here CL113636.

namedConst

In Go, sadly, named constants even when grouped together aren’t connected between each other and didn’t form enumeration like the other languages (proposal: spec: add typed enum support).

One of this problem is a casting untyped const to a type that you’re using like an enum.

Suppose you have a type Color and it’s default const ColDefault Color = 0.
Which of 2 examples your like more?

// (A)
if color == 0 {
    // do smth
}
// (B)
if color == colorDefault {
    // do smth
}

If case (B) for you looks more clear namedConst will help you to find such usages and propose named const for that.

Here are the changes for context.mangle method from package html/template:

By the way, sometimes in a patch review you can find interesting discussions…CL123376 — is a one of such discussions.

unslice

One of the features of slice expression is that x[:] is always identical tox (or if x is a string). In case of a slice it works for any type.

x       // x is a slice
x[:]    // also a slice
x[:][:] // even this

unslice finds similar redundant slice expressions. They’re bad ’cause they’re giving unneeded cognitive load. x[:] makes sense when x is an array, but for a slice it’s just a noise.

Here is a patch CL123375.

switchTrue

The replacement of switch true {...} toswitch {...} is done in CL123378.
Both of forms are equivalent, but last one is more idiomatic.

Most of the style checks are targeting on similar expressions, where both of them are allowed and correct, but only one of them is more common for Go developers. Next check is from the same category.

typeUnparen

Go as many other languages likes parentheses. So heavily, that allows any number of them:

type (
    t0 int
    t1 (int)
    t2 ((int))
    // ... so, you get the idea.
)

But what will happen after running gofmt?

type (
    t0 int
    t1 (int) // <- Woah! Nothing has changed.
    t2 (int) // <- Meh..only half of work is done here...
)

For this case, we have typeUnparen checker. It finds all type expressions, where some parentheses can be omitted. We’ve tried to fix few warnings here CL123379, we will see if it will be merged.

go-critic on duty

We’ve covered only tiny part of all checks and in near future, we will have more of them with new cases, suggestions and solutions thanks to the people that are contributing to go-critic.

Send us ideas or any code improvements, inform about found warnings, maybe bugs or just ping us. You might also suggest projects for auditing, it’s a priceless experience for us. Thank you.

More where this came from

This story is published in Noteworthy, where thousands come every day to learn about the people & ideas shaping the products we love.

Follow our publication to see more product & design stories featured by the Journal team.