Saya ingin membagikan hasil beberapa hari terakhir, yang meliputi analisis git dari sejarah beberapa proyek Go besar dengan tujuan menemukan komitmen yang memperbaiki kesalahan dan kemudian memformalkannya untuk deteksi jika muncul dalam kode baru.

Pada bagian kedua artikel, kami akan mempertimbangkan beberapa diagnostik baru dalam go-kritik , yang memungkinkan Anda menemukan kode yang sangat mungkin mengandung kesalahan.

Cari ide di masa lalu

Untuk menemukan ide untuk inspeksi kode baru, Anda dapat mengintip penganalisa statis untuk bahasa pemrograman lain, Anda dapat memeriksa kode sumber terbuka secara manual dalam upaya menemukan templat kode yang salah di dalamnya, atau Anda dapat melihat sejarah.

Misalkan Anda sedang memeriksa proyek Go-Files . Setelah memulai linter (penganalisa statis), tidak ada masalah, audit teks sumber juga tidak membawa hasil yang bagus. Haruskah saya buru-buru mengesampingkan Go-Files ? Tidak juga.

Selama keberadaannya, Go-Files memiliki sejumlah cacat yang mudah dideteksi, tetapi sekarang sudah diperbaiki. Yang perlu kita lakukan adalah mulai menganalisis sejarah repositori git.

Bug ada di suatu tempat di dekatnya.

Koleksi komitmen yang menarik bagi kami

Masalahnya:

• Mungkin ada banyak komitmen
• Terkadang dalam revisi yang sama hal-hal tidak saling terkait
• Terkadang komentar pada komit tidak sesuai dengan perubahan yang sebenarnya.
• Beberapa orang meremehkan penggunaan pesan komit yang baik (jangan arahkan jari)

Pertama-tama, kami ingin mengurangi jumlah informasi yang harus diproses tanpa otomatisasi. Masalah tanggapan negatif palsu dapat diselesaikan dengan meningkatkan ukuran sampel (unduh lebih banyak paket Go ke grup kontrol).

Anda dapat menganalisanya dengan cara yang berbeda, saya baru saja menggunakan git log --grep untuk satu set templat, dengan menaksir terlalu tinggi dan meremehkan skor komit tergantung pada isi dari pesan git log --grep . Jika komit terlalu besar, Anda dapat segera membuangnya, karena sangat mungkin untuk mengetahui apa yang terjadi akan ada nontrivial.

Saya juga ingin menambahkan bahwa di samping koreksi atau menemukan kesalahan cukup sering ada deskripsi yang tidak terlalu optimis dan kadang-kadang tidak cukup kultural tentang tambalan:

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

"Perbaikan bug" yang paling tidak berguna adalah mengedit kesalahan sintaks atau masalah lain yang mencegah proyek dari kompilasi bahkan melalui go build . Tidak semua proyek menggunakan CI atau kait pra-komitmen , sehingga revisi yang rusak tersebut terkadang berakhir di cabang utama.

Kesalahan sejarah

Mari kita lihat beberapa kesalahan paling menarik yang ditemukan di balik sampul git log.

Kembalikan nil alih-alih hasil nyata

gin-gonic / gin: fix bug, return err ketika gagal mengikat bool :

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

go-pg / pg: Perbaikan bug AppendParam :

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

Kurangnya pengembalian di jalur cepat

gonum / gonum: bug diperbaiki di jalur cepat dswap :

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

Menjalankan goroutine tanpa wg.Tambahkan (1)

btcsuite / btcd: Perbaiki beberapa bug di shutdown server RPC :

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

Kondisi logis tidak valid

gorgonia / gorgonia: memperbaiki beberapa bug juga :

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

src-d / go-git: plumbing / idxfile: perbaiki pencarian bug di MemoryIndex :

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

go-xorm / xorm: perbaiki bug pada jumlah :

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

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

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

btcsuite / btcd: Perbaiki bug uji terbalik dengan penanganan operasi maks :

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

Memperbarui penerima (ini / diri sendiri) lewat nilai

Klasik Salinan objek dimodifikasi di dalam metode, karena itu penelepon tidak akan melihat hasil yang diharapkan.

Memperbaiki bug tumpukan / antrian / deque (penerima pointer) :

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

Memperbarui salinan objek di dalam satu lingkaran

mengandung / traefik: Tetapkan tugas yang difilter ke aplikasi yang terdapat dalam 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) 

mengandung / traefik: Tambahkan unit test untuk paket yang aman :

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

Hasil dari fungsi pembungkus tidak digunakan.

gorgonia / gorgonia: Memperbaiki bug kecil yang tidak penting di Grad () :

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

Pekerjaan dengan make yang salah

Terkadang penjual muda akan " make([]T, count) " diikuti dengan append di dalam loop. Pilihan yang benar di sini adalah " make([]T, 0, count) ".

HouzuoGuo / tiedot: perbaiki bug pemindaian koleksi :

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

Perbaikan di atas memperbaiki kesalahan asli, tetapi memiliki satu kelemahan yang diperbaiki di salah satu dari komit berikut:

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

Tetapi beberapa operasi, seperti copy atau ScanSlice mungkin memerlukan panjang "benar".

go-xorm / xorm: perbaikan bug untuk SumsInt mengembalikan slice kosong :

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

Segera pergi-kritik akan membantu menemukan kesalahan di kelas ini.

Kesalahan lainnya

Enumerasi ternyata agak rumit, saya mengambil sisanya di bawah spoiler. Bagian ini opsional, jadi Anda bisa membiarkannya nanti atau melanjutkan.

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

Perang salib melawan kode yang mencurigakan

badCond

Pemeriksaan badCond menemukan ekspresi logis yang berpotensi salah.

Contoh ekspresi logis yang mencurigakan:

• Hasilnya selalu true atau false
• Ungkapan ini ditulis dalam bentuk yang hampir tidak bisa dibedakan dari yang keliru

Pemeriksaan ini juga berfungsi pada operasi seperti " x == nil && x == y ". Satu solusi sederhana adalah menulis ulang ke " x == nil && y == nil ". Keterbacaan kode tidak menderita, tetapi linter tidak akan melihat sesuatu yang mencurigakan dalam kode ini.

Berikut adalah contoh perbaikan bug yang dapat ditemukan oleh badCond :

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

Piala:

• moby / buildkit: snapshot: perbaiki kondisi yang tidak mungkin
• tdewolff / parse: EqualFold dari util.go memiliki kondisi aneh
• HouzuoGuo / tiedot: httpapi: kondisi mencurigakan di srv_test.go

lemah

weakCond mirip dengan badCond , tetapi tingkat kepercayaan peringatan sedikit lebih rendah.

Kondisi yang lemah adalah kondisi yang tidak mencakup input dengan cukup baik.

Contoh bagus dari kondisi lemah (tidak memadai) adalah memeriksa irisan untuk nil dan kemudian menggunakan elemen pertama. Kondisi " s != nil " tidak cukup di sini. Kondisi yang benar adalah " len(s) != 0 " atau yang setara:

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

Piala:

• etcd-io / etcd: etcdserver / api / v2v3: pengindeksan setelah nil check tanpa len check
• moby / moby: registry: pengindeksan setelah nil check tanpa len check
• inkyblackness / diretas: ss1 / content / text: pengindeksan setelah nil check tanpa len check

dupArg

Untuk beberapa fungsi, memberikan nilai (atau variabel) yang sama dengan beberapa argumen tidak masuk akal. Cukup sering, ini menandakan kesalahan salin / tempel .

Contoh dari fungsi tersebut adalah copy . copy(xs, xs) ekspresi copy(xs, xs) tidak masuk akal.

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

Piala:

• ethereum / go-ethereum: light: memperbaiki argumen yang digandakan dalam byte. Panggilan yang sama
• openshift / origin: perbaiki reflect.DeepEqual (x, x) bug

dupCase

Anda mungkin tahu bahwa di Go Anda tidak dapat menggunakan nilai case duplikat di dalam switch .

Contoh di bawah ini tidak dapat dikompilasi :

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

Kesalahan kompilasi: duplikat case 1 di switch.

Tetapi bagaimana jika kita mengambil contoh yang lebih menarik :

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

Nilai duplikat diizinkan melalui variabel. Selain itu, switch true memberi lebih banyak ruang untuk kesalahan:

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

Dan berikut ini adalah contoh untuk memperbaiki kesalahan nyata:

 switch { case m < n: // Upper triangular matrix. //   . case m == n: // Diagonal matrix. //   . -case m < n: // Lower triangular matrix. +case m > n: // Lower triangular matrix. //   . } 

Piala:

• mellium / sasl: menghapus case switch duplikat
• gonum / gonum: memperbaiki kasus saklar duplikat
• pact-foundation / pact-go: menghapus duplikat switch case
• minio / minio: klausa saklar kasus duplikat yang mencurigakan dalam cmd / object-api-error.go
• henrylee2cn / pholcus: common / bytes: menghapus duplikat switch case
• go-ble / ble: linux / att: memperbaiki duplikat saklar kasus kesalahan
• buruh pelabuhan / distribusi: registri / penyimpanan: memperbaiki kasus saklar digandakan

dupBranchBody

Pernyataan bersyarat seperti if/else dan switch memiliki badan untuk dieksekusi. Ketika kondisi terpenuhi, kontrol ditransfer ke unit operasi terkait. Sementara diagnostik lain memverifikasi bahwa kondisi blok ini berbeda, dupBranchBody memeriksa bahwa blok yang dapat dieksekusi sendiri tidak sepenuhnya identik.

Kehadiran duplikat badan di dalam pernyataan bersyarat, kecuali jika itu adalah type switch , setidaknya mencurigakan.

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

Penilaian tentang tingkat kebenaran kode di bawah ini diserahkan kepada pembaca:

 if field.IsMessage() || p.IsGroup(field) { //   . } else if field.IsString() { if nullable && !proto3 { p.generateNullableField(fieldname, verbose) } else { pP(`if this.`, fieldname, ` != that1.`, fieldname, `{`) } } else { if nullable && !proto3 { p.generateNullableField(fieldname, verbose) } else { pP(`if this.`, fieldname, ` != that1.`, fieldname, `{`) } } } 

Badan di dalam " if field.IsString() " dan " else " yang terkait identik.

Piala:

• src-d / go-git: Badan cabang duplikasi yang mencurigakan

caseOrder

Semua tipe di dalam type switch diurutkan secara berurutan, ke yang kompatibel pertama. Jika nilai tag bertipe *T , dan mengimplementasikan antarmuka I , maka Anda harus meletakkan label dengan " case *T " sebelum label dengan " case I ", jika tidak hanya " case I " yang akan dieksekusi, karena *T kompatibel dengan I (tetapi bukan sebaliknya).

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

Piala:

• cmd / guru: memperbaiki urutan kasus yang salah di uraikan.go
• message / pipeline: memperbaiki urutan jenis sakelar
• coyove / goflyway: proxy: memperbaiki urutan kasus dalam saklar jenis
• go-graphite / carbonapi: pkg / parser: memperbaiki jenis saklar kasus pesanan

offBy1

Kesalahan per unit sangat populer sehingga bahkan memiliki halaman wikipedia sendiri .

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

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

go-critic memiliki opsi terbatas untuk menemukan kesalahan seperti itu, tetapi sejauh ini tidak satu perbaikan telah dikirim ke repositori publik. Berikut adalah beberapa perbaikan yang ditemukan saat melihat cerita:

• btcsuite / btcd: tes dan perbaiki bug di getaddednodeinfo
• btcsuite / btcd: Tingkatkan cakupan tes dan perbaiki beberapa bug yang ditemukan oleh tes

Sedikit ngeri pada akhirnya

go vet memiliki pemeriksaan yang baik untuk ekspresi seperti " x != a || x != b ". Tampaknya orang mungkin tidak tahu tentang gometalinter , tetapi go vet meluncurkan hampir semua orang, bukan?

Menggunakan utilitas gogrep, saya mengumpulkan daftar kecil ekspresi serupa yang masih di cabang utama dari beberapa proyek.

 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) 

Belajar dari kesalahan orang lain

Tidak, bahkan pada bagian terakhir tidak akan ada apa-apa tentang pembelajaran mesin .

Tetapi yang akan ditulis di sini adalah tentang golangci-lint , yang mengintegrasikan kritikus dalam salah satu rilis terbaru. golangci-lint adalah analog dari gometalinter , yang dapat Anda baca tentang kelebihannya, misalnya, dalam artikel "Analisis statis di Go: cara kami menghemat waktu saat memeriksa kode . "

go-critic baru-baru ini ditulis ulang menggunakan lintpack . Detail dapat ditemukan di Go lintpack: manajer linter yang dapat dikompilasi .

Jika Anda belum mulai secara aktif menggunakan alat untuk menganalisis program Anda untuk kesalahan potensial, baik gaya dan logis, hari ini saya sarankan sekali lagi memikirkan perilaku Anda sambil minum teh bersama rekan kerja. Anda akan berhasil.

Baik untuk semua