Путешествие gocritic'а в прошлое12.12.2018 09:20

Хочу поделиться результатами работы последних нескольких дней, которые включали в себя анализ git истории некоторых крупных Go проектов с целью нахождения коммитов, которые исправляли ошибки c последующей их формализацией для детектирования в случае их появления в новом коде.

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

Чтобы находить идеи для новых инспекций кода, можно подглядывать в статические анализаторы для других языков программирования, можно вручную исследовать open source код с попыткой найти в нём ошибочные шаблоны кода, а можно заглянуть в историю.

Предположим, вы проверяете проект Go-Files. После запуска линтера (статического анализатора) проблемы не находятся, аудит исходных текстов тоже не принёс больших результатов. Стоит ли торопиться откладывать Go-Files в сторону? Не совсем.

За время существования Go-Files имел какое-то количество легко детектируемых дефектов, но сейчас они уже исправлены. То, что нам нужно сделать — это начать анализировать git историю репозитория.

Баги где-то рядом.

Проблемы:

• Коммитов может быть очень много
• Иногда в одной и той же ревизии выполнены не связанные друг с другом вещи
• Иногда комментарий к коммиту не совсем соответствует фактическим изменениям
• Некоторые недооценивают пользу хороших commit message (не будем показывать пальцем)

В первую очередь мы хотим снизить количество информации, которую придётся обрабатывать без автоматизации. Проблему false-negative срабатываний можно решить увеличением объёма выборки (скачать больше Go пакетов в контрольную группу).

Анализировать можно по-разному, мне было достаточно »git log --grep» по набору шаблонов, с завышением и занижением веса (score) коммита в зависимости от содержимого его commit message. Если коммит слишком огромный, его сразу можно отбросить, потому что с большой вероятностью разобраться что там происходит будет нетривиально.

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

- "We check length later, but we assumed it was always 1 bytes long. Not always the case. I'm a little depressed that this bug was there"

- "Really fixed the bug now"

- "WTF, the changes that actually fixed the bug for the accesspattern wasn't actually committed"

- "how do i pronounce this damn project" (проект traefic)

- "damnit travis"

- "one f*cking dot" (./.. => ./... в .travis.yml)

Самые бесполезные «исправления ошибок» — это правка синтаксических ошибок или прочих проблем, которые не дают проекту даже собраться через go build. Не во всех проектах используется CI или pre-commit hook, поэтому такие сломанные ревизии иногда попадают в master ветку.

Пройдёмся по некоторым наиболее интересным ошибкам, которые были найдены под одеялом git логов.

Возврат nil вместо реального результата

gin-gonic/gin: fix bug, return err when failed binding bool:

    if err == nil {
        field.SetBool(boolVal)
    }
-   return nil
+   return err
}

go-pg/pg: AppendParam bug fix:

        return method.AppendValue(b, m.strct.Addr(), 1), true
    }
-   return nil, false
+   return b, false
}

Отсутствие return в fast path

gonum/gonum: fixed bug in dswap fast path:

        for i := 0; i < n; i++ {
            x[i], y[i] = y[i], x[i]
        }
+       return
    }

Запуск горутины без wg.Add (1)

btcsuite/btcd: Fix several bugs in the RPC server shutdown:

+s.wg.Add(1)
go s.walletListenerDuplicator()

Некорректное логическое условие

gorgonia/gorgonia: fixed a few bugs as well:

-for i := 0; i > start; i++ {
+for i := 0; i < start; i++ {
    retVal[i] = 0
}

src-d/go-git: plumbing/idxfile: fix bug searching in MemoryIndex:

-if low < high {
+if low > high {
    break
}

go-xorm/xorm: fix bug on sum:

-if !strings.Contains(colName, " ") && strings.Contains(colName, "(") {
+if !strings.Contains(colName, " ") && !strings.Contains(colName, "(") {

btcsuite/btcd: server: Fix bug disconnecting peer on filteradd:

-if sp.filter.IsLoaded() {
+if !sp.filter.IsLoaded() {

btcsuite/btcd: Fix reversed test bug with the max operations handling:

-if pop.opcode.value < OP_16 {
+if pop.opcode.value > OP_16 {
    s.numOps++

Обновление receiver’а (this/self), передаваемого по значению

Классика. Внутри метода модифицируется копия объекта, из-за чего вызывающая сторона не увидит ожидаемых результатов.

Fixed a stack/queue/deque bug (pointer receiver):

-func (s Stack) Push(x interface{}) {
-   s = append(s, x)
+func (s *Stack) Push(x interface{}) {
+   *s = append(*s, x)
}

Обновление копий объектов внутри цикла

containous/traefik: Assign filtered tasks to apps contained in slice:

-for _, app := range filteredApps {
-   app.Tasks = fun.Filter(func(task *marathon.Task) bool {
+for i, app := range filteredApps {
+   filteredApps[i].Tasks = fun.Filter(func(task *marathon.Task) bool {
        return p.taskFilter(*task, app)
    }, app.Tasks).([]*marathon.Task)

containous/traefik: Add unit tests for package safe:

-for _, routine := range p.routines {
+for i := range p.routines {
    p.waitGroup.Add(1)
-   routine.stop = make(chan bool, 1)
+   p.routines[i].stop = make(chan bool, 1)
    Go(func() {
-       routine.goroutine(routine.stop)
+       p.routines[i].goroutine(p.routines[i].stop)
        p.waitGroup.Done()
    })
}

Результат оборачивающей функции не используется

gorgonia/gorgonia: Fixed a tiny nonconsequential bug in Grad ():

if !n.isInput() {
-   errors.Wrapf(err, ...)
-   // return
+   err = errors.Wrapf(err, ...)
+   return nil, err
}

Неправильная работа с make

Иногда молодые гоферы делают »make([]T, count)» с последующим append внутри цикла. Правильным вариантом здесь является »make([]T, 0, count)».

HouzuoGuo/tiedot: fix a collection scan bug:

-ids := make([]uint64, benchSize)
+ids := make([]uint64, 0)

Исправление выше правит исходную ошибку, но имеет один изъян, который поправили в одном из следующих коммитов:

-ids := make([]uint64, 0)
+ids := make([]uint64, 0, benchSize)

Но некоторые операции, например copy или ScanSlice могут требовать «правильной» длины.

go-xorm/xorm: bug fix for SumsInt return empty slice:

-var res = make([]int64, 0, len(columnNames))
+var res = make([]int64, len(columnNames), len(columnNames))
if session.IsAutoCommit {
    err = session.DB().QueryRow(sqlStr, args...).ScanSlice(&res)
} else {

Скоро go-critic будет помогать в нахождении ошибок этого класса.

Остальные ошибки

Перечисление получается довольно громоздким, оставшуюся часть уношу под спойлер. Эта часть опциональна, так что можете оставить её на потом или смело двигаться дальше.

-for _, el := range *s1 {
+for el := range *s1 {
    if _, ok := (*s2)[el]; !ok {
        return false
    }

func (s *server) Query(ctx context.Context,
-   req *graph.Request) (resp *graph.Response, err error) {
+   req *graph.Request) (*graph.Response, error) {
+   resp := new(graph.Response)

-block, err := btcutil.NewBlockFromBytes(blockBytes)
+block, err = btcutil.NewBlockFromBytes(blockBytes)
if err != nil {
    return err
}

-err = session.Rollback()
+err1 := session.Rollback()
+if err1 == nil {
+   return lastId, err
+}
+err = err1

-debugPrint("Listening and serving HTTP on %s", addr)
+debugPrint("Listening and serving HTTP on %s\n", addr)

    styleCache map[int]*Style `-`
+   lock       *sync.RWMutex
}

badCond

Проверка badCond находит потенциально некорректные логические выражения.

Примеры подозрительных логических выражений:

• Результат всегда true или false
• Выражение написано в той форме, которая практически неотличима от ошибочной

Эта проверка также срабатывает на операциях вроде »x == nil && x == y». Одним из простых решений является переписывание в »x == nil && y == nil». Читабельность кода не страдает, зато линтер не будет видеть в этом коде что-то подозрительное.

Вот пример исправления бага, которая может быть найдена badCond:

-if err1 := f(); err != nil && err == nil {
+if err1 := f(); err1 != nil && err == nil {
    err = err1
}

Трофеи:

weakCond

weakCond похож на badCond, но уровень достоверности предупреждений несколько ниже.

Слабое условие — это такое условие, которое не покрывает входные данные достаточно полно.

Хороший пример слабого (недостаточного) условия — это проверка слайса на nil с последующим использованием первого элемента. Условия »s != nil» здесь недостаточно. Корректным условием будет »len(s) != 0» или её эквивалент:

func addRequiredHeadersToRedirectedRequests(
        req *http.Request, 
        via []*http.Request) error {
-   if via != nil && via[0] != nil {
+   if len(via) != 0 && via[0] != nil {

Трофеи:

dupArg

Для некоторых функций передача одного и того же значения (или переменной) в качестве нескольких аргументов не имеет большого смысла. Довольно часто это сигнализирует о copy/paste ошибке.

Примером такой функции является copy. Выражение copy(xs, xs) не имеет смысла.

-if !bytes.Equal(i2.Value, i2.Value) {
+if !bytes.Equal(i1.Value, i2.Value) {
    return fmt.Errorf("tries differ at key %x", i1.Key)
}

Трофеи:

dupCase

Наверное вы знаете, что в Go нельзя использовать повторяющиеся case значения внутри switch.

Пример ниже не компилируется:

switch x := 0; x {
case 1:
    fmt.Println("first")
case 1:
    fmt.Println("second")
}

Ошибка компиляции: duplicate case 1 in switch.

Но что если взять пример поинтереснее:

one := 1
switch x := 1; x {
case one:
    fmt.Println("first")
case one:
    fmt.Println("second")
}

Через переменную использовать дублирующиеся значения разрешены. Помимо этого, switch true даёт ещё больше простора для ошибок:

switch x := 0; true {
case x == 1:
    fmt.Println("first")
case x == 1:
    fmt.Println("second")
}

А вот и пример исправления реальной ошибки:

switch {
case m < n: // Upper triangular matrix.
    // Вырезано для краткости.
case m == n: // Diagonal matrix.
    // Вырезано для краткости.
-case m < n: // Lower triangular matrix.
+case m > n: // Lower triangular matrix.
    // Вырезано для краткости.
}

Трофеи:

dupBranchBody

В условных операторах, таких как if/else и switch есть тела для исполнения. Когда условие выполнено, управление передаётся ассоциированному блоку операций. В то время как другие диагностики проверяют что условия этих блоков различны, dupBranchBody проверяет что сами исполняемые блоки не являются полностью идентичными.

Наличие дублирующихся тел внутри условных операторов, если только это не type switch, как минимум подозрительно.

-if s, err := r.ReceivePack(context.Background(), req); err != nil { 
-   return s, err 
-} else { 
-   return s, err 
-}
+return r.ReceivePack(context.Background(), req)

Суждения о степени корректности кода ниже оставляю на откуп читателям:

if field.IsMessage() || p.IsGroup(field) {
        // Вырезано для краткости.
    } else if field.IsString() {
        if nullable && !proto3 {
            p.generateNullableField(fieldname, verbose)
        } else {
            p.P(`if this.`, fieldname, ` != that1.`, fieldname, `{`)
        }
    } else {
        if nullable && !proto3 {
            p.generateNullableField(fieldname, verbose)
        } else {
            p.P(`if this.`, fieldname, ` != that1.`, fieldname, `{`)
        }
    }
}

Тела внутри »if field.IsString()» и связанного с ним »else» идентичны.

Трофеи:

caseOrder

Все типы внутри type switch перебираются последовательно, до первого совместимого. Если tag-значение имеет тип *T, и реализует интерфейс I, то ставить метку с »case *T» нужно до метки с »case I», иначе выполняться будет только »case I», так как *T совместим с I (но не наоборот).

case string:
    res = append(res, NewTargetExpr(v).toExpr().(*expr))
-case Expr:
-   res = append(res, v.toExpr().(*expr))
case *expr:
    res = append(res, v)
+case Expr:
+   res = append(res, v.toExpr().(*expr))

Трофеи:

offBy1

Ошибка на единицу так популярна, что у неё даже есть своя страница на википедии.

if len(optArgs) > 1 {
    return nil, ErrTooManyOptArgs
}
-node = optArgs[1]
+node = optArgs[0]

-last := xs[len(xs)]
+last := xs[len(xs) - 1]

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

В go vet есть хорошая проверка для выражений типа »x != a || x != b». Казалось бы, люди могут не знать про gometalinter, но go vet запускает почти каждый, да?

С помощью утилиты gogrep я собрал небольшой список похожих выражений, которые до сих пор находятся в master ветках некоторых проектов.

cloud.google.com/go/trace/trace_test.go:943:7:
  expectTraceOption != ((o2&1) != 0) || expectTraceOption != ((o3&1) != 0)
github.com/Shopify/sarama/mocks/sync_producer_test.go:36:5:
  offset != 1 || offset != msg.Offset
github.com/Shopify/sarama/mocks/sync_producer_test.go:44:5:
  offset != 2 || offset != msg.Offset
github.com/docker/libnetwork/api/api_test.go:376:5:
  id1 != i2e(i2).ID || id1 != i2e(i3).ID
github.com/docker/libnetwork/api/api_test.go:408:5:
  "sh" != epList[0].Network || "sh" != epList[1].Network
github.com/docker/libnetwork/api/api_test.go:1196:5:
  ep0.ID() != ep1.ID() || ep0.ID() != ep2.ID()
github.com/docker/libnetwork/api/api_test.go:1467:5:
  ep0.ID() != ep1.ID() || ep0.ID() != ep2.ID()
github.com/docker/libnetwork/ipam/allocator_test.go:1261:5:
  len(indices) != len(allocated) || len(indices) != num
github.com/esimov/caire/grayscale_test.go:27:7:
  r != g || r != b
github.com/gogo/protobuf/test/bug_test.go:99:5:
  protoSize != mSize || protoSize != lenData
github.com/gogo/protobuf/test/combos/both/bug_test.go:99:5:
  protoSize != mSize || protoSize != lenData
github.com/gogo/protobuf/test/combos/marshaler/bug_test.go:99:5:
  protoSize != mSize || protoSize != lenData
github.com/gogo/protobuf/test/combos/unmarshaler/bug_test.go:99:5:
  protoSize != mSize || protoSize != lenData
github.com/gonum/floats/floats.go:65:5:
  len(dst) != len(s) || len(dst) != len(y)
github.com/gonum/lapack/testlapack/dhseqr.go:139:7:
  wr[i] != h.Data[i*h.Stride+i] || wr[i] != h.Data[(i+1)*h.Stride+i+1]
github.com/gonum/stat/stat.go:1053:5:
  len(x) != len(labels) || len(x) != len(weights)
github.com/hashicorp/go-sockaddr/ipv4addr_test.go:659:27:
  sockaddr.IPPort(p) != test.z16_portInt || sockaddr.IPPort(p) != test.z16_portInt
github.com/hashicorp/go-sockaddr/ipv6addr_test.go:430:27:
  sockaddr.IPPort(p) != test.z16_portInt || sockaddr.IPPort(p) != test.z16_portInt
github.com/nats-io/gnatsd/server/monitor_test.go:1863:6:
  v.ID != c.ID || v.ID != r.ID
github.com/nbutton23/zxcvbn-go/adjacency/adjcmartix.go:85:7:
  char != "" || char != " "
github.com/openshift/origin/pkg/oc/cli/admin/migrate/migrator_test.go:85:7:
  expectedInfos != writes || expectedInfos != saves
github.com/openshift/origin/test/integration/project_request_test.go:120:62:
  added != deleted || added != 1
github.com/openshift/origin/test/integration/project_request_test.go:126:64:
  added != deleted || added != 4
github.com/openshift/origin/test/integration/project_request_test.go:132:62:
  added != deleted || added != 1
gonum.org/v1/gonum/floats/floats.go:60:5:
  len(dst) != len(s) || len(dst) != len(y)
gonum.org/v1/gonum/lapack/testlapack/dhseqr.go:139:7:
  wr[i] != h.Data[i*h.Stride+i] || wr[i] != h.Data[(i+1)*h.Stride+i+1]
gonum.org/v1/gonum/stat/stat.go:1146:5:
  len(x) != len(labels) || len(x) != len(weights)

Нет, даже в последней части не будет ничего про машинное обучение.

Зато о чём здесь будет написано, так это о golangci-lint, который в одном из последних релизов интегрировал go-critic. golangci-lint — это аналог gometalinter, почитать про достоинства которого можно, например, в статье «Статический анализ в Go: как мы экономим время при проверке кода».

Сам go-critic недавно был переписан с помощью lintpack. За подробностями можно проследовать в «Go lintpack: менеджер компонуемых линтеров».

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

Всем добра.

© Habrahabr.ru