Ich möchte die Ergebnisse der letzten Tage teilen, einschließlich der Git- Analyse der Geschichte einiger großer Go- Projekte mit dem Ziel, Commits zu finden, die Fehler korrigieren und sie dann zur Erkennung formalisieren, wenn sie im neuen Code erscheinen.

Im zweiten Teil des Artikels werden wir einige neue Diagnosen in Go-Critical betrachten , mit denen Sie Code finden können, der höchstwahrscheinlich einen Fehler enthält.

Suche nach Ideen in der Vergangenheit

Um Ideen für neue Code-Inspektionen zu finden, können Sie einen Blick auf statische Analysegeräte für andere Programmiersprachen werfen, Open Source-Code manuell untersuchen, um fehlerhafte Codevorlagen darin zu finden, oder den Verlauf untersuchen.

Angenommen, Sie überprüfen das Go-Files Projekt. Nach dem Starten des Linter (statischer Analysator) gibt es keine Probleme, die Prüfung der Ausgangstexte brachte ebenfalls keine guten Ergebnisse. Sollte ich mich beeilen, Go-Files beiseite zu legen? Nicht wirklich.

Während seiner Existenz hatte Go-Files eine Reihe von leicht erkennbaren Fehlern, die jetzt jedoch bereits behoben sind. Was wir tun müssen, ist mit der Analyse des Git-Repository-Verlaufs zu beginnen.

Bugs sind irgendwo in der Nähe.

Sammlung von Commits, die für uns von Interesse sind

Die Probleme:

• Es kann viele Commits geben
• Manchmal sind die Dinge in derselben Revision nicht miteinander verbunden
• Manchmal stimmt ein Kommentar zu einem Commit nicht genau mit den tatsächlichen Änderungen überein.
• Einige unterschätzen die Verwendung einer guten Commit-Nachricht (zeigen wir nicht mit den Fingern)

Zunächst möchten wir die Menge an Informationen reduzieren, die ohne Automatisierung verarbeitet werden müssen. Das Problem falsch-negativer Antworten kann durch Erhöhen der Stichprobengröße gelöst werden (mehr Go-Pakete in die Kontrollgruppe herunterladen).

Sie können es auf verschiedene Arten analysieren. Ich hatte gerade git log --grep für eine Reihe von Vorlagen, wobei die Punktzahl des Commits abhängig vom Inhalt der Commit-Nachricht überschätzt und unterschätzt wurde. Wenn das Commit zu groß ist, können Sie es sofort verwerfen, da es sehr wahrscheinlich herausfindet, was dort passiert, ist nicht trivial.

Ich möchte auch hinzufügen, dass es neben Korrekturen oder Fehlern häufig nicht sehr optimistische und manchmal nicht ganz kulturelle Beschreibungen von Patches gibt:

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

Die nutzlosesten „Fehlerkorrekturen“ sind das Bearbeiten von Syntaxfehlern oder anderen Problemen, die verhindern, dass das Projekt überhaupt durch go build kompiliert wird. Nicht alle Projekte verwenden CI oder Pre-Commit-Hook , sodass solche fehlerhaften Revisionen manchmal in der Hauptniederlassung landen.

Historische Fehler

Lassen Sie uns einige der interessantesten Fehler durchgehen, die unter dem Deckmantel von Git-Protokollen gefunden wurden.

Geben Sie Null anstelle des tatsächlichen Ergebnisses zurück

gin-gonic / gin: Fehler behoben, Fehler zurückgeben, wenn die Bindung fehlgeschlagen ist bool :

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

go-pg / pg: AppendParam-Fehlerbehebung :

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

Mangel an Rückkehr auf schnellem Weg

gonum / gonum: Fehler im schnellen Pfad von dswap behoben :

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

Goroutinen ohne wg ausführen.Add (1)

btcsuite / btcd: Behebung mehrerer Fehler beim Herunterfahren des RPC-Servers :

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

Ungültige logische Bedingung

Gorgonia / Gorgonia: Einige Fehler wurden ebenfalls behoben :

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

src-d / go-git: plumbing / idxfile: Fehlersuche in MemoryIndex behoben :

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

go-xorm / xorm: Fehler in der Summe behoben :

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

btcsuite / btcd: server: Fehler beim Trennen des Peers beim Filtern behoben :

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

btcsuite / btcd: Behebung des umgekehrten Testfehlers bei der Behandlung maximaler Operationen :

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

Aktualisieren des Empfängers (dies / selbst) als Wert übergeben

Klassisch Eine Kopie des Objekts wird innerhalb der Methode geändert, wodurch der Aufrufer die erwarteten Ergebnisse nicht sieht.

Ein Stack / Queue / Deque-Fehler (Zeigerempfänger) wurde behoben :

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

Aktualisieren von Kopien von Objekten in einer Schleife

Containous / Traefik: Weisen Sie Apps, die in Slice enthalten sind, gefilterte Aufgaben zu :

 -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: Fügen Sie Unit-Tests für die Paketsicherheit hinzu :

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

Das Ergebnis der Umhüllungsfunktion wird nicht verwendet.

gorgonia / gorgonia: Ein kleiner Fehler in Grad () wurde behoben :

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

Falsche Arbeit mit make

Manchmal machen junge Gophers " make([]T, count) ", gefolgt vom append innerhalb der Schleife. Die richtige Option ist hier " make([]T, 0, count) ".

HouzuoGuo / boundot: Behebung eines Sammlungsscan-Fehlers :

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

Der obige Fix korrigiert den ursprünglichen Fehler, weist jedoch einen Fehler auf, der in einem der folgenden Commits behoben wurde:

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

Einige Vorgänge wie copy oder ScanSlice erfordern jedoch möglicherweise die "richtige" Länge.

go-xorm / xorm: Fehlerbehebung für SumsInt leeres Slice zurückgeben :

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

Bald wird Go-Kritiker helfen, Fehler in dieser Klasse zu finden.

Andere Fehler

Die Aufzählung erweist sich als ziemlich umständlich, ich nehme den Rest unter den Spoiler. Dieser Teil ist optional, sodass Sie ihn für später belassen oder mutig weitermachen können.

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

Kreuzzug gegen verdächtigen Code

badCond

Die badCond Prüfung findet möglicherweise falsche logische Ausdrücke.

Beispiele für verdächtige logische Ausdrücke:

• Das Ergebnis ist immer true oder false
• Der Ausdruck ist in einer Form geschrieben, die vom Irrtum kaum zu unterscheiden ist

Diese Prüfung funktioniert auch bei Operationen wie " x == nil && x == y ". Eine einfache Lösung besteht darin, in " x == nil && y == nil " umzuschreiben. Die Lesbarkeit des Codes leidet nicht, aber der Linter sieht in diesem Code nichts Verdächtiges.

Hier ist ein Beispiel für eine Fehlerbehebung, die von badCond gefunden werden badCond :

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

Trophäen:

• Moby / Buildkit: Schnappschuss: Unmöglichen Zustand beheben
• tdewolff / parse: EqualFold von util.go hat einen seltsamen Zustand
• HouzuoGuo / boundot: httpapi: Verdächtiger Zustand in srv_test.go

schwache Sekunde

weakCond ähnelt badCond , aber das Konfidenzniveau von Warnungen ist etwas niedriger.

Ein schwacher Zustand ist ein Zustand, der den Eingang nicht ausreichend vollständig abdeckt.

Ein gutes Beispiel für eine schwache (unzureichende) Bedingung besteht darin, das Slice auf nil zu überprüfen und dann das erste Element zu verwenden. Die Bedingungen " s != nil " reichen hier nicht aus. Die korrekte Bedingung ist " len(s) != 0 " oder gleichwertig:

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

Trophäen:

• etcd-io / etcd: etcdserver / api / v2v3: indizierung nach nil check ohne len check
• moby / moby: registry: indizierung nach nil check ohne len check
• inkyblackness / gehackt: ss1 / content / text: indizierung nach nil check ohne len check

dupArg

Für einige Funktionen ist es wenig sinnvoll, denselben Wert (oder dieselbe Variable) als mehrere Argumente zu übergeben. Sehr oft signalisiert dies einen Kopier- / Einfügefehler .

Ein Beispiel für eine solche Funktion ist das copy . Die Ausdruckskopie copy(xs, xs) macht keinen Sinn.

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

Trophäen:

• ethereum / go-ethereum: light: Doppeltes Argument in Bytes korrigieren. Gleicher Aufruf
• openshift / origin: Reflect.DeepEqual (x, x) Fehler behoben

dupCase

Sie wissen wahrscheinlich, dass Sie in Go keine doppelten Fallwerte innerhalb des switch .

Das folgende Beispiel wird nicht kompiliert :

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

Kompilierungsfehler: Duplizieren Sie Fall 1 im Schalter.

Was aber, wenn wir ein interessanteres Beispiel nehmen:

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

Über die Variable sind doppelte Werte zulässig. Darüber hinaus bietet switch true noch mehr Raum für Fehler:

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

Und hier ist ein Beispiel für die Behebung eines echten Fehlers:

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

Trophäen:

• mellium / sasl: Entfernen Sie das doppelte Schaltergehäuse
• Gonum / Gonum: Doppelte Schaltergehäuse reparieren
• Pact-Foundation / Pact-Go: Entfernen Sie doppelte Schaltergehäuse
• minio / minio: Verdächtige doppelte Switch-Case-Klausel in cmd / object-api-error.go
• henrylee2cn / pholcus: common / bytes: Doppelte Schalterhülle entfernen
• go-ble / ble: linux / att: Behebung eines doppelten Switch-Case-Fehlers
• Docker / Distribution: Registrierung / Speicher: Doppelter Switch-Fall behoben

dupBranchBody

Bedingte Anweisungen wie if/else und switch müssen ausgeführt werden. Wenn die Bedingung erfüllt ist, wird die Steuerung an die zugehörige Betriebseinheit übertragen. Während andere Diagnosen überprüfen, ob die Bedingungen dieser Blöcke unterschiedlich sind, überprüft dupBranchBody , ob die ausführbaren Blöcke selbst nicht vollständig identisch sind.

Das Vorhandensein doppelter Körper in bedingten Anweisungen ist zumindest verdächtig, es sei denn, es handelt sich um einen Typwechsel.

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

Die Beurteilung des Richtigkeitsgrades des folgenden Codes bleibt dem Leser überlassen:

 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, `{`) } } } 

Die Körper in " if field.IsString() " und das zugehörige " else " sind identisch.

Trophäen:

• src-d / go-git: Verdächtige doppelte Zweigkörper

caseOrder

Alle Typen innerhalb des type switch nacheinander nach dem ersten kompatiblen sortiert. Wenn der Tag-Wert vom Typ *T ist und die Schnittstelle I implementiert, müssen Sie ein Label mit " case *T " vor das Label mit " case I " setzen, andernfalls wird nur " case I " ausgeführt, da *T mit I kompatibel ist (aber nicht umgekehrt).

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

Trophäen:

• cmd / guru: Korrigieren Sie die falsche Fallreihenfolge in description.go
• Nachricht / Pipeline: Reihenfolge der Schaltergehäuse korrigieren
• coyove / goflyway: proxy: Fallreihenfolge in einem Typschalter korrigieren
• go-graphite / carbonapi: pkg / parser: fix typ schalter koffer ordnung

offBy1

Fehler pro Einheit sind so beliebt, dass es sogar eine eigene Wikipedia-Seite gibt .

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

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

go-critic hat nur begrenzte Möglichkeiten, solche Fehler zu finden, aber bisher wurde kein einziger Fix an das öffentliche Repository gesendet. Hier sind einige Korrekturen, die beim Anzeigen der Geschichte gefunden wurden:

• btcsuite / btcd: Testen und Beheben von Fehlern in getaddednodeinfo
• btcsuite / btcd: Verbessern Sie die Testabdeckung und beheben Sie einige Fehler, die durch Tests festgestellt wurden

Ein bisschen Horror am Ende

go vet hat eine gute Prüfung auf Ausdrücke wie " x != a || x != b ". Es scheint, dass die Leute vielleicht nichts über gometalinter wissen, aber der go vet startet fast jeden, oder?

Mit dem Dienstprogramm gogrep habe ich eine kleine Liste ähnlicher Ausdrücke zusammengestellt, die sich noch in den Hauptzweigen einiger Projekte befinden.

 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) 

Aus den Fehlern anderer lernen

Nein, auch im letzten Teil geht es nicht um maschinelles Lernen .

Aber worüber hier geschrieben wird, ist über Golangci-Lint , der Go-Kritiker in eine der neuesten Veröffentlichungen integriert hat. golangci-lint ist ein Analogon von gometalinter , dessen Vorteile Sie beispielsweise im Artikel "Statische Analyse in Go: Wie wir beim Überprüfen von Code Zeit sparen" nachlesen können.

go-critic wurde kürzlich mit Lintpack umgeschrieben. Details finden Sie in Go lintpack: ein zusammensetzbarer Linter-Manager .

Wenn Sie noch nicht begonnen haben, Tools aktiv zu verwenden, um Ihre Programme auf mögliche stilistische und logische Fehler zu analysieren, empfehle ich Ihnen heute, bei einer Tasse Tee mit Kollegen noch einmal über Ihr Verhalten nachzudenken. Du wirst Erfolg haben.

Gut zu allen.