Вы, возможно, помните недавний анонс нового статического анализатора для Go под названием go-critic.

Я проверил с его помощью проект golang/go и отправил несколько патчей, которые исправляют некоторые найденные там проблемы.

В этой статье мы разберём исправленный код, а также будем мотивироваться отправлять ещё больше подобных изменений в Go.

Для самых нетерпеливых: обновляемый список трофеев.

dupSubExpr

Все мы допускаем ошибки, и, довольно часто, по невнимательности. Go, будучи языком, в котором временами приходится писать скучный и шаблонный код, порой способствует опечаткам и/или ошибкам категории copy/paste.

CL122776 содержит исправление бага, найденного проверкой 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]])
//                                     ^__________________________________^
}

Обратите внимание на индекс слева и справа. До исправления, LHS и RHS оператора < были идентичны, на что и сработал dupSubExpr.

commentedOutCode

Если ваш проект под эгидой системой контроля версий, то вместо отключения кода путём оборачивания его в комментарий, стоит удалять его полностью. Есть исключения, но чаще такой "мёртвый" код мешает, путает и может скрывать ошибки.

commentedOutCode смог найти вот такой занимательный фрагмент (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")
}

Немного выше есть комментарий:

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

Если переключиться на ветку go1.4 и убрать эти 3 строки из-под комментария, код не будет компилироваться, однако если раскомментировать их на мастере, всё будет работать.

Обычно код, спрятанный в комментарий, требует либо удаления, либо обратной активации.

Полезно, время от времени, навещать такие отголоски прошлого в вашем коде.

boolExprSimplify

Иногда люди пишут странный код. Возможно мне кажется, но особо странно иногда выглядят логические (булевы) выражения.

У Go отличный бэкенд ассемблера x86 (тут упала слеза), а вот ARM провинился по-настоящему:

if !(o1 != 0) {
    break
}

"Если не o1 не равно 0"… Двойное отрицание — это классика. Если вам понравилось, приглашаю ознакомиться с CL123377. Там вы сможете увидеть исправленный вариант.

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

boolExprSimplify нацелен на упрощения, которые повышают читабельность (а с вопросом производительности оптимизатор Go справился бы и без этого).

underef

Если вы используете Go с его ранних версий, то можете помнить обязательные точки с запятыми, отсутсвие автоматического разыменования указателей, и прочие особенности, которые сегодня увидеть в новом коде практически невозможно.

В старом же коде до сих пор можно встретить нечто подобное:

// Когда-то явное разыменование было обязательным:
buf := (*bufp).ptr()
// ...теперь же можно упростить выражение до:
buf := bufp.ptr()

Несколько срабатываний underef анализатора исправлены в CL122895.

appendCombine

Возможно, вы знаете, что append может принимать несколько аргументов в качестве элементов, добавляемых в целевой слайс. В некоторых ситуациях это позволяет немного улучшить читабельность кода, но, что может быть интереснее, это также может ускорить вашу программу, поскольку компилятор не выполняет схлапывание совместимых вызовов append (cmd/compile: combine append calls).

В Go проверка appendCombine нашла следующий участок:

- 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)

Подробности в CL117615.

rangeValCopy

Ни для кого не секрет, что значения, перебираемые в range цикле, копируются. Для объектов малого размера, скажем, меньше 64 байт, вы можете этого даже не заметить. Однако если подобный цикл лежит на "горячем" пути, или, над которым вы итерируетесь, содержит очень большое количество элементов, накладные расходы могут быть осязаемы.

В Go довольно медленный линковщик (cmd/link), и без значительных изменений в его архитектуре, сильного прироста производительности добиться нельзя. Но зато можно немного уменьшать его неэффективность с помощью микрооптимизаций. Каждый процент-два на счету.

Проверка rangeValCopy нашла сразу несколько циклов с нежелательным копированием данных. Вот наиболее интересный из них:

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

Вместо того, чтобы копировать сам R[i] на каждой итерации, мы лишь обращаемся к единственному интересующему нас члену, Sym.

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

Полная версия патча доступена по ссылке: CL113636.

namedConst

В Go, к сожалению, именованные константы, даже собранные в группы, не связаны между собой и не образуют перечисление (proposal: spec: add typed enum support).

Одной из проблем является приведение untyped констант к типу, который вы хотели бы использовать как enum.

Допустим, вы определили тип Color, у него есть значение const ColDefault Color = 0.
Какой из этих двух фрагментов кода вам нравится больше?

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

Если (B) вам кажется более уместным, проверка namedConst поможет вам отследить использование значений именованных констант в обход самой именованной константе.

Вот так преобразился метод context.mangle из пакета html/template:

    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

Кстати, иногда по ссылкам на патчи можно найти интересные обсуждения…
CL123376 — один из таких случаев.

unslice

Особенность slice expression в том, что x[:] всегда идентично x, если тип x — это слайс или строка. В случае слайсов это работает для любого типа элемента []T.

Всё в списке ниже — одно и то же (x — слайс):

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

unslice находит подобные избыточные выражения срезов. Вредны эти выражения прежде всего лишней когнитивной нагрузкой. x[:] имеет вполне весомую семантику в случае взятия среза у массива. Срез среза с диапазонами по умолчанию не вносит ничего, кроме шума.

За патчем прошу в CL123375.

switchTrue

В CL123378 выполнена замена "switch true {...}" на "switch {...}".
Обе формы эквивалентны, но вторая является более идиоматичной.
Найдено с помощью проверки switchTrue.

Большинство стилистических проверок выявляют именно подобные случаи, где допустимы оба варианта, но один из них является более распространённым и привычным большему количеству Go программистов. Следующая проверка из этой же серии.

typeUnparen

Go, как и многие другие языки программирования, любит скобочки. Настолько, что готов принять любое их количество:

type (
    t0 int
    t1 (int)
    t2 ((int))
    // ... ну, вы поняли.
)

Но что произойдёт, если запустить gofmt?

type (
    t0 int
    t1 (int) // <- Эй! Ничего не изменилось.
    t2 (int) // <- Выполнена только половина работы...
)

Вот поэтому и существует typeUnparen. Он находит в программе все выражения типов, в которых можно уменьшить количество скобочек. Я попробовал выслать CL123379, посмотрим, как его примет сообщество.

go-critic на службе Go

Мы рассмотрели только малую часть реализованных проверок. При этом их количество и качество будет только эволюционировать со временем, в том числе благодаря тем людям, которые присоединились к разработке.

go-critic абсолютно бесплатен для любого использования (лицензия MIT), а также открыт для вашего участия в развитии проекта. Присылайте нам идеи для проверок, можно сразу с реализацией, докладывайте о найденных ошибках и недоработках, делитесь впечатлениями. Можете также предлагать проекты для аудита или докладывать о проведённых вами ревью Go кода, этот опыт для нас бесценен.

Тема контрибьютинга в Go

Видели статью Go contribution workshop в России? Этой осенью будет второй раунд. И на этот раз, кроме более удачного формата и спонсоров, у нас будет секретное оружие — замечательный статический анализатор. Контрибьюций хватит на всех!

Но на самом деле, начинать вы можете прямо сейчас (хотя лучше — немного позже, после code freeze). Если получится освоиться до следующего воркшопа, будет очень здорово, потому что нам сильно нехватает менторов в России.