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'е.)


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


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

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


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


    Как всё начиналось


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


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


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


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


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


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


    Как устроен go-critic


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


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


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


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

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



    Добавление тестов


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


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


    Примеры:


    // lint/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
    }

    // lint/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, который содержит информацию о типах и другие метаданные о проверяемом файле.


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


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


    func (c *captLocalChecker) Init() {
        c.upcaseNames = map[string]bool{
            "IN":    true,
            "OUT":   true,
            "INOUT": true,
        }
    }

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


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


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

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


    Добавление документации


    Ещё одним необходимым к реализации методом является InitDocumentation:


    func (c *captLocalChecker) InitDocumentation(d *Documentation) {
        d.Summary = "Detects capitalized names for local variables"
        d.Before = `func f(IN int, OUT *int) (ERR error) {}`
        d.After = `func f(in int, out *int) (err error) {}`
    }

    Обычно, достаточно заполнить 3 поля:


    • Summary — описание действия проверки в одно предложение.
    • Before — код до исправления.
    • After — код после исправления (не должен вызывать предупреждения).

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

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


    Регистрация нового линтера и запуск тестов


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


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

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


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


    attrExperimental — атрибут, присваиваемый всем новым реализациям. Удаление этого атрибута возможно только после стабилизации реализуемой проверки.


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


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

    Optimistic merging (почти)


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


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


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

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


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


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


    Напутствия


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


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

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


    • +26
    • 4,7k
    • 7
    Поделиться публикацией
    Комментарии 7
      +2

      А что такое "линтер"? Статический анализатор?

        0
        Есть общепризнанный и лаконичный аналог на Русском, или только полная форма «статический анализатор»? Возможно, «анализатор» был бы уместным компромиссом.

        +1
        Идея этакого тестового полигона для новых проверок интересная, но особой разницы между добавлением проверки в go-critic и любой другой проект с точки зрения пользователя может и не быть, к тому же не очень понятно как эту идею пользователям «продать».

        Ну и отсутствие интеграции с редакторами/gometalinter делает ваш проект заметно менее привлекательным, ведь если уже кто-то заморочился с новой проверкой она должна быть полностью интегрирована в процесс разработки.

        Про сам linter — опробовал на своем pet project, весьма полезной показалась rangeExprCopy, а paramTypeCombine скорее вредной. И если вы хотите иметь opinionated проверки, то надо чтоб их было легко отключить, а пока есть только опция -enable, тобишь явный whitelist.

        А вообще желаю удачи с проектом, мало статического анализа не бывает.
          0
          paramTypeCombine

          В ней присутствуют false positive срабатывания для безымянных параметров. Если бы их не было, всё равно был бы вредным?


          И если вы хотите иметь opinionated проверки

          В недавнем обновлении по умолчанию experimental и opinionated выключены из -enable=all. В идеале eventually придём к более-менее нормальному default набору, но в качестве интеграции куда-то всё равно лучше явный список использовать.


          Ну и отсутствие интеграции с редакторами/gometalinter делает ваш проект заметно менее привлекательным, ведь если уже кто-то заморочился с новой проверкой она должна быть полностью интегрирована в процесс разработки.

          Уже в golangci-lint добавиться планируем, там есть интеграции в том числе с редакторами. В gometalinter, наверное, тоже можно. Для проекта эти интеграции важны, согласен полностью.

          0
          case ast.IsExported(name.ID.String()):
          c.warnCapitalized(name.ID)


          Вот это не совсем понял. По логике вещей, предупреждение должно выдаваться если переменная НЕ экспортируемая (т.е. локальная) и Capitalized. А здесь как-то наоборот. Это ошибка или я что-то не понял?
            0

            Понимаю ваше недоумение.


            Но фукнция IsExported(s) проверяет лишь то, что переданная ей строка начинается с заглавной буквы. Метод VisitLocalDef() вызывается только для локальных определений,
            поэтому если мы уже внутри и IsExported(s) вернул true, то это локальная переменная, которая начинается с заглавное буквы, что не имеет особого смысла, потому что это не влияет на семантику.

              +1
              Ясно. Спасибо! Я подозревал нечто подобное. :)

              Шикарный инструмент! В ближайшее время проверю весь свой код. :) Может и в разработке правил поучаствую. :)

          Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

          Самое читаемое