Хабрахабр

go-critic: самый упрямый статический анализатор для Go

Анонсируем новый линтер (статический анализатор) для Go, который одновременно является песочницей для прототипирования ваших задумок в мире статического анализа.

go-critic построен вокруг следующих наблюдений:

  • Лучше иметь “good enough” реализацию проверки, чем не иметь её вовсе
  • Если проверка спорная, это ещё не значит, что она не может быть полезна. Помечаем как “opinionated” и вливаем
  • Писать линтер с нуля, как правило, сложнее, чем добавлять новую проверку в существующий каркас, если сам фреймворк прост для понимания

В этом посте мы рассмотрим использование и архитектуру go-critic, некоторые реализованные в нём проверки, а также опишем основные шаги добавления своей функции-анализатора в него.

$ cd $GOPATH
$ go get -u github.com/go-critic/go-critic/...
$ ./bin/gocritic check-package strings $GOROOT/src/strings/replace.go:450:22: unslice: could simplify s[:] to s
$GOROOT/src/strings/replace.go:148:2: elseif: should rewrite if-else to switch statement
$GOROOT/src/strings/replace.go:156:3: elseif: should rewrite if-else to switch statement
$GOROOT/src/strings/replace.go:219:3: elseif: should rewrite if-else to switch statement
$GOROOT/src/strings/replace.go:370:1: paramTypeCombine: func(pattern string, value string) *singleStringReplacer could be replaced with func(pattern, value string) *singleStringReplacer
$GOROOT/src/strings/replace.go:259:2: rangeExprCopy: copy of r.mapping (256 bytes) can be avoided with &r.mapping
$GOROOT/src/strings/replace.go:264:2: rangeExprCopy: copy of r.mapping (256 bytes) can be avoided with &r.mapping
$GOROOT/src/strings/strings.go:791:1: paramTypeCombine: func(s string, cutset string) string could be replaced with func(s, cutset string) string
$GOROOT/src/strings/strings.go:800:1: paramTypeCombine: func(s string, cutset string) string could be replaced with func(s, cutset string) string
$GOROOT/src/strings/strings.go:809:1: paramTypeCombine: func(s string, cutset string) string could be replaced with func(s, cutset string) string
$GOROOT/src/strings/strings.go:44:1: unnamedResult: consider to give name to results
$GOROOT/src/strings/strings.go:61:1: unnamedResult: consider to give name to results
$GOROOT/src/strings/export_test.go:28:3: rangeExprCopy: copy of r.mapping (256 bytes) can be avoided with &r.mapping
$GOROOT/src/strings/export_test.go:42:1: unnamedResult: consider to give name to results

(Форматирование предупреждений отредактировано, оригиналы доступны в gist'е.)

Например, вы можете проверить весь $GOROOT или $GOPATH с помощью одной команды: Утилита gocritic умеет проверять отдельные пакеты по их import пути (check-package), а так же рекурсивно обходить все директории (check-project).

$ gocritic check-project $GOROOT/src
$ gocritic check-project $GOPATH/src

По умолчанию запускаются все те проверки, которые не отмечены значком Experimental или VeryOpinionated. Есть поддержка “белого списка” для проверок, чтобы явно перечислить какие проверки нужно выполнять (флаг -enable).

Планируются интеграции в golangci-lint и gometalinter.

Проводя очередное код-ревью Go проекта, или при аудите некоторой 3-rd party библиотеки, вы можете раз за разом замечать одни и те же проблемы.

К вашему сожалению, найти линтер, который диагностировал бы этот класс проблем, не удалось.

Шансы на то, что ваше предложение примут, сильно зависит от проекта и могут быть довольно низкими. Первым вашим действием может быть попытка категоризировать проблему и связаться с авторами существующих линтеров, предлагая им добавить новую проверку. Далее, скорее всего, последуют месяцы ожидания.

А что, если проверка и вовсе неоднозначная и может быть кем-то воспринята как слишком субъективная или недостаточно точная?

Может, есть смысл попробовать написать эту проверку самому?

Само устройство go-critic минимизирует количество контекста и действий, которые необходимы для добавления новой проверки — можно сказать, что потребуется добавить только один файл (не считая тестов). go-critic существует для того, чтобы стать домом для экспериментальных проверок, которые проще реализовать самим, чем пристроить их в существующие статические анализаторы.

Критик — это набор правил (rules), которые описывают свойства проверки и микро-линтеры (checkers), реализующие инспекцию кода на соответствие правилу.

Приложение, которое встраивает линтер (например, cmd/gocritic или golangci-lint), получает список поддерживаемых правил, фильтрует их определённых образом, создаёт для каждого отобранного правила check функцию, и запускает каждую из них над исследуемым пакетом.

Работа по добавлению нового checker’а сводится к трём основным шагам:

  1. Добавление тестов.
  2. Реализация непосредственно самой проверки.
  3. Добавление документации для линтера.

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

Для добавления тестовых данных для новой проверки, нужно создать новую директорию в cmd/gocritic/testdata.

Чтобы тестировать отсутствие ложных срабатываний, тесты дополняются “корректным” кодом, в котором новая проверка не должна находить никаких проблем (negative_tests.go). Каждая такая директория должна иметь файл positive_tests.go, который описывает примеры кода, на которых проверка должна срабатывать.

Примеры:

// cmd/gocritic/testdata/positive_tests.go /// consider `in' name instead of `IN'
/// `X' should not be capitalized
/// `Y' should not be capitalized
/// `Z' should not be capitalized
func badFunc1(IN, X int) (Y, Z int) { /// `V' should not be capitalized V := 1 return V, 0
}

// cmd/gocritic/testdata/negative_tests.go func goodFunc1(in, x int) (x, y int) { v := 1 return v, 0
}

Запустить тесты можно после добавления нового линтера.

Создадим файл с названием чекера: lint/captLocal_checker.go.
По конвенции, все файлы с микро-линтерами имеют суффикс _checker.

package lint // Суффикс “Checker” в имени типа обязателен.
type captLocalChecker struct { checkerBase upcaseNames map[string]bool
}

checkerBase — это тип, который должен быть встроен (embedded) в каждый checker.
Он предоставляет реализации по умолчанию, что позволяет писать меньше кода в каждом линтере.
Помимо прочего, checkerBase включает указатель на lint.context, который содержит информацию о типах и другие метаданные о проверяемом файле.

ToLower(name) версию. Поле upcaseNames будет содержать таблицу известных имён, которые мы будем предлагать заменять на strings. Для тех имён, которые не содержатся в мапе, будет предлагаться не использовать заглавную букву, но корректной замены предоставляться не будет.

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

func (c *captLocalChecker) Init()
}

Ident, которые вводят новые переменные. Теперь требуется определить саму проверяющую функцию.
В случае captLocal, нам нужно проверять все локальные ast.

Для того, чтобы проверять все локальные определения имён, следует в своём чекере реализовать метод со следующей сигнатурой:

VisitLocalDef(name astwalk.Name, initializer ast.Expr)

Список доступных visitor интерфейсов может быть найден в файле lint/internal/visitor.go.
captLocal реализует LocalDefVisitor.

// Игнорируем ast.Expr, потому что нам не интересно какое значение присваивается
// локальному имени. Нас интересуют только сами названия переменных.
func (c *captLocalChecker) VisitLocalDef(name astwalk.Name, _ ast.Expr) { switch { case c.upcaseNames[name.ID.String()]: c.warnUpcase(name.ID) case ast.IsExported(name.ID.String()): c.warnCapitalized(name.ID) }
} func (c *captLocalChecker) warnUpcase(id *ast.Ident) { c.ctx.Warn(id, "consider `%s' name instead of `%s'", strings.ToLower(id.Name), id)
} func (c *captLocalChecker) warnCapitalized(id ast.Node) { c.ctx.Warn(id, "`%s' should not be capitalized", id)
}

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

Последним штрихом является регистрация нового линтера:

// Локальная для captLocal_checker.go init функция.
func init() { addChecker(&captLocalChecker{}, attrSyntaxOnly)
}

Далее идёт вариадичный аргумент, позволяющий передать ноль или несколько аттрибутов, описывающих свойства реализации правила. addChecker ожидает указатель на zero-value нового линтера.

golangci-lint отмечает такие линтеры флагом “fast” (потому что они выполняются гораздо быстрее). attrSyntaxOnly — опциональный маркер для линтеров, которые не пользуются информацией о типах в своей реализации, что позволяет запускать их без выполнения проверки на типы.

Теперь, когда новый линтер зарегистрирован через addChecker, можно запустить тесты:

# Из GOPATH:
$ go test -v github.com/go-critic/go-critic/cmd/gocritic
# Из GOPATH/src/github.com/go-critic/go-critic:
$ go test -v ./cmd/gocritic
# Оттуда же, можно запустить тесты с помощью make:
$ make test

Вот комментарий из captLocal_checker.go: Для описания нового линтера нужно добавить “магический” комментарий в файл с реализацией.

//! Detects capitalized names for local variables.
//
// @Before:
// func f(IN int, OUT *int) (ERR error) {}
//
// @After:
// func f(in int, out *int) (err error) {}

В простейшем случае, документация состоит из короткой сводки в одну строку, секций Before и After. Структура данного комментария строгая и проверяется при генерации документации.

Генерация документации

Но если вы всё же хотите проверить, как будет выглядеть выходной markdown файл, воспользуйтесь командой make docs. Повторно генерировать документацию не является обязательным требованием для нового линтера, возможно в ближайшем будущем этот шаг будет полностью автоматизирован. Файл docs/overview.md будет обновлён.

В основном это выражается в принятии тех PR, к которым у проводящего ревью могут оставаться некоторые, в частности чисто субъективные, претензии. При рассматривании pull requests мы стараемся придерживаться стратегии optimistic merging. Сразу после вливания такого патча может последовать PR от ревьювера, который исправляет эти недочёты, в CC (copy) добавляется автор исходного патча.

У нас также есть два маркера линтеров, которые могут быть использованы во избежание красных флагов в случае отсутствия полного консенсуса:

  1. Experimental: реализация может иметь большое количество false positive, быть неэффективной (при этом источник проблемы идентифицирован) или “падать” в некоторых ситуациях. Такую реализацию влить можно, если пометить её атрибутом attrExperimental. Иногда с помощью experimental обозначаются те проверки, которым не получилось подобрать хорошее название с первого коммита.
  2. VeryOpinionated: если проверка может иметь как защитников, так и врагов, стоит пометить её атрибутом attrVeryOpinionated. Таким образом мы можем избегать отклонения тех идей о стиле кода, которые могут не совпадать со вкусом некоторых гоферов.

VeryOpinionated — это более фундаментальное свойство правила, которое не зависит от реализации. Experimental — потенциально временное и исправимое свойство реализации.

Рекомендуется создавать [checker-request] тикет на github’е перед тем, как отправлять реализацию, но если вы уже отправили pull request, открыть соответствующий issue могут за вас.

Подробнее о деталях процесса разработки смотри в CONTRIBUTING.md.
Основные правила перечислены в секции main rules.

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

  • Пробовать его на своих проектах или крупных/известных open-source проектах и докладывать false positives, false negatives и другие недоработки. Будем признательны, если вы также добавите заметку о найденной/исправленной проблеме на страницу trophies.
  • Предлагать идеи для новых проверок. Достаточно создать issue на нашем трекере.
  • Добавлять тесты для существующих линтеров.

Критиковать может каждый, поэтому — присоединяйтесь! go-critic критикует ваш Go код голосами всех программистов, участвующих в его разработке.

Теги
Показать больше

Похожие статьи

Добавить комментарий

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

Кнопка «Наверх»
Закрыть