Главная » Хабрахабр » Контрибьютим в Go с помощью статического анализатора go-critic

Контрибьютим в Go с помощью статического анализатора go-critic

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

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

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

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

Список разбираемых патчей в Go

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

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

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

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

О сложностях детектирования

Эта одна из самых моих любимых проверок, но она же является одной из самых "шумных".

В первом случае обычно это поясняющие комментарии к выполняемым операциям, а во втором — описание кода, который описывает фрагмент AST. Очень много ложных срабатываний для пакетов, которые используют math/big и внутри компилятора. Различить такие комментарии от реального "мёртвого" кода, без внесения false negatives — нетривиально.

Тогда статический анализ упростится. Отсюда возникает идея: а что если нам договориться как-то по-особому оформлять код внутри комментариев, который является пояснительным? Это может быть любая мелочь, которая или позволит легко определить такой пояснительный комментарий, или сделает его невалидным Go кодом (например, если добавлять в начало строки знак решётки, #).

Если код убран под комментарий, но при этом есть чёткое описание почему это сделано и когда планируется починить этот кусок кода, то лучше не выдавать предупреждения. Другая категория — это комментарии с явным TODO. Это уже реализовано, но могло бы работать более надёжно.

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

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

if !(o1 != 0) { break
}

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

Исправленный вариант (для тех, кого не заманить на go-review)

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

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

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

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

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

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

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

В 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.

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

Но зато можно немного уменьшать его неэффективность с помощью микрооптимизаций. В 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.

В 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
- 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 — один из таких случаев.

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

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

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

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

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

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

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

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

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

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

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

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

Лиспы не любят скобочки

Так что в языках, у которых синтаксис основан на S-выражениях, написать ничего не делающую программу, но имеющую огромное количество скобочек, сложнее, чем на некоторых других языках. В отличие от C-подобных языков, в Лиспах не так просто вставить бесполезные скобочки в произвольное место.

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

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

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

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

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


Оставить комментарий

Ваш email нигде не будет показан
Обязательные для заполнения поля помечены *

*

x

Ещё Hi-Tech Интересное!

Равномерно распределяем точки по сфере в pytorch и tensorflow

А ещё можно научиться делать мультфильм из данных, визуализированных с помощью PovRay и vapory. Этот текст написан для тех, кто интересуется глубоким обучением, кто хочет использовать разные методы библиотек pytorch и tensorflow для минимизации функции многих переменных, кому интересно научиться ...

[Перевод] Университет Карнеги-Меллона спасает старые программы от забвения

Прототип архивной системы Olive позволяет запускать на современных компьютерах винтажный код С таким большим долгом, по их мнению, рост должен стать отрицательным. В начале 2010 года гарвардские экономисты Кармен Рейнхарт и Кеннет Рогов опубликовали анализ экономических данных разных стран, и ...