Je veux partager les résultats des derniers jours, qui comprenaient l'analyse git de l'historique de certains grands projets Go dans le but de trouver des commits qui corrigeaient les erreurs, puis les formalisaient pour la détection si elles apparaissaient dans le nouveau code.

Dans la deuxième partie de l'article, nous considérerons quelques nouveaux diagnostics dans go-critique , qui vous permettent de trouver du code qui est très susceptible de contenir une erreur.

Rechercher des idées dans le passé

Pour trouver des idées pour de nouvelles inspections de code, vous pouvez jeter un œil aux analyseurs statiques pour d'autres langages de programmation, vous pouvez examiner manuellement le code open source dans le but d'y trouver des modèles de code erronés, ou vous pouvez consulter l'historique.

Supposons que vous vérifiez le projet Go-Files . Après le démarrage du linter (analyseur statique), il n'y a pas de problème; l'audit des textes sources n'a pas non plus apporté de grands résultats. Dois-je me précipiter pour mettre de côté Go-Files ? Pas vraiment.

Au cours de son existence, Go-Files avait un certain nombre de défauts facilement détectables, mais maintenant ils sont déjà corrigés. Ce que nous devons faire, c'est commencer à analyser l'historique du référentiel git.

Les insectes sont quelque part à proximité.

Collection de commits qui nous intéressent

Les problèmes:

• Il peut y avoir beaucoup de commits
• Parfois, dans la même révision, les choses ne sont pas liées les unes aux autres
• Parfois, un commentaire sur une validation ne correspond pas exactement aux modifications réelles.
• Certains sous-estiment l'utilisation d'un bon message de validation (ne pointons pas du doigt)

Tout d'abord, nous voulons réduire la quantité d'informations qui devront être traitées sans automatisation. Le problème des réponses faussement négatives peut être résolu en augmentant la taille de l'échantillon (téléchargez plus de paquets Go dans le groupe de contrôle).

Vous pouvez l'analyser de différentes manières, il me suffisait de " git log --grep " en fonction de l'ensemble des modèles, avec surestimation et sous-estimation du poids (score) du commit en fonction du contenu de son message de commit. Si le commit est trop important, vous pouvez le rejeter immédiatement, car il est très probable qu'il comprendra ce qui se passera ne sera pas trivial.

Je voudrais également ajouter qu'à côté des corrections ou des erreurs de recherche, il n'y a pas souvent de descriptions très optimistes et parfois pas tout à fait culturelles des correctifs:

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

Les «corrections de bugs» les plus inutiles sont l'édition d'erreurs de syntaxe ou d'autres problèmes qui empêchent même le projet de se compiler via go build . Tous les projets n'utilisent pas le CI ou le hook de pré-validation , de telles révisions brisées finissent parfois dans la branche principale.

Erreurs historiques

Passons en revue certaines des erreurs les plus intéressantes qui ont été trouvées sous les couvertures des journaux git.

Renvoie zéro au lieu du résultat réel

gin-gonic / gin: correction d'un bug, retourne une erreur en cas d'échec de la liaison bool :

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

go-pg / pg: correction du bogue AppendParam :

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

Manque de retour sur la voie rapide

gonum / gonum: correction d'un bug dans le raccourci dswap :

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

Exécuter des goroutines sans wg.Add (1)

btcsuite / btcd: correction de plusieurs bogues lors de l'arrêt du serveur RPC :

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

Condition logique non valide

gorgonia / gorgonia: correction de quelques bugs également :

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

src-d / go-git: plomberie / idxfile: correction d'un bug de recherche dans MemoryIndex :

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

go-xorm / xorm: correction d'un bug sur la somme :

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

btcsuite / btcd: server: Correction d'un bug de déconnexion d'homologue sur filteradd :

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

btcsuite / btcd: Correction d'un bug de test inversé avec la gestion des opérations max :

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

Récepteur de mise à jour (this / self) passé par valeur

Classique Une copie de l'objet est modifiée à l'intérieur de la méthode, à cause de quoi l'appelant ne verra pas les résultats attendus.

Correction d'un bug de pile / file d'attente / deque (récepteur de pointeur) :

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

Mise à jour des copies d'objets dans une boucle

containous / traefik: Attribuez des tâches filtrées aux applications contenues dans la tranche :

 -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: Ajouter des tests unitaires pour la sécurité du paquet :

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

Le résultat de la fonction d'habillage n'est pas utilisé.

gorgonia / gorgonia: correction d'un minuscule bogue non consécutif dans Grad () :

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

Travail incorrect avec make

Parfois, les jeunes gophers " make([]T, count) ", puis append à l'intérieur de la boucle. L'option correcte ici est " make([]T, 0, count) ".

HouzuoGuo / Tiedot: correction d'un bug d'analyse de collection :

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

Le correctif ci-dessus corrige l'erreur d'origine, mais présente un défaut qui a été corrigé dans l'une des validations suivantes:

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

Mais certaines opérations, telles que la copy ou ScanSlice peuvent nécessiter la longueur "correcte".

go-xorm / xorm: correction d'un bug pour SumsInt retourne une tranche vide :

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

Bientôt, le critique vous aidera à trouver des erreurs dans cette classe.

Autres erreurs

Le dénombrement se révèle assez lourd, je sors le reste sous le becquet. Cette partie est facultative, vous pouvez donc la laisser pour plus tard ou avancer audacieusement.

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

Croisade contre le code suspect

badCond

La vérification badCond trouve des expressions logiques potentiellement incorrectes.

Exemples d'expressions logiques suspectes:

• Le résultat est toujours true ou false
• L'expression est écrite sous une forme qui est presque impossible à distinguer de l'erreur

Cette vérification fonctionne également sur des opérations telles que " x == nil && x == y ". Une solution simple consiste à réécrire " x == nil && y == nil ". La lisibilité du code ne souffre pas, mais le linter ne verra rien de suspect dans ce code.

Voici un exemple de correction de bogue qui peut être trouvé par badCond :

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

Trophées:

• moby / buildkit: instantané: correction d'une condition impossible
• tdewolff / parse: EqualFold de util.go a une condition étrange
• HouzuoGuo / tieot: httpapi: condition suspecte dans srv_test.go

faibleCond

weakCond est similaire à badCond , mais le niveau de confiance des alertes est légèrement inférieur.

Une condition faible est une condition qui ne couvre pas suffisamment complètement l'entrée.

Un bon exemple de condition faible (insuffisante) est de vérifier la tranche pour nil puis d'utiliser le premier élément. Les conditions " s != nil " ne suffisent pas ici. La condition correcte est " len(s) != 0 " ou son équivalent:

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

Trophées:

• etcd-io / etcd: etcdserver / api / v2v3: indexation après vérification nulle sans vérification len
• moby / moby: registre: indexation après chèque nul sans chèque len
• inkyblackness / piraté: ss1 / contenu / texte: indexation après vérification nulle sans vérification len

dupArg

Pour certaines fonctions, transmettre la même valeur (ou variable) que plusieurs arguments n'a pas beaucoup de sens. Très souvent, cela signale une erreur de copier / coller .

Un exemple d'une telle fonction est la copy . La copy(xs, xs) expression copy(xs, xs) n'a pas de sens.

 -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ées:

• ethereum / go-ethereum: light: correction de l'argument dupliqué en octets. Appel égal
• openshift / origin: correction du bogue reflect.DeepEqual (x, x)

dupCase

Vous savez probablement que dans Go, vous ne pouvez pas utiliser de valeurs de case double dans le switch .

L'exemple ci - dessous ne compile pas :

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

Erreur de compilation: dupliquez le cas 1 dans le commutateur.

Mais que se passe-t-il si nous prenons un exemple plus intéressant :

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

Les valeurs en double sont autorisées via la variable. De plus, le switch true donne encore plus de place aux erreurs:

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

Et voici un exemple de correction d'une erreur réelle:

 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ées:

• mellium / sasl: retirer le boîtier de commutateur en double
• gonum / gonum: correction du boîtier de commutation dupliqué
• pact-foundation / pact-go: supprimer le boîtier de commutation en double
• minio / minio: clause de cas de commutateur dupliqué suspect dans cmd / object-api-error.go
• henrylee2cn / pholcus: commun / octets: supprimer le boîtier de commutateur dupliqué
• go-ble / ble: linux / att: correction d'une erreur de cas de commutateur dupliqué
• docker / distribution: registre / stockage: correction du boîtier de commutation dupliqué

dupBranchBody

Les instructions conditionnelles telles que if/else et switch ont des corps à exécuter. Lorsque la condition est remplie, le contrôle est transféré à l'unité opérationnelle associée. Alors que d'autres diagnostics vérifient que les conditions de ces blocs sont différentes, dupBranchBody vérifie que les blocs exécutables eux-mêmes ne sont pas complètement identiques.

La présence de corps en double dans les instructions conditionnelles, sauf s'il s'agit d'un type switch , est au moins suspecte.

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

Les jugements sur le degré d'exactitude du code ci-dessous sont laissés au lecteur:

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

Les corps à l'intérieur de " if field.IsString() " et le " else " associé sont identiques.

Trophées:

• src-d / go-git: Organes de branche dupliqués suspects

caseOrder

Tous les types à l'intérieur du type switch triés séquentiellement, selon le premier compatible. Si la valeur de la balise est de type *T et implémente l'interface I , alors vous devez mettre une étiquette avec " case *T " avant l' étiquette avec " case I ", sinon seul " case I " sera exécuté, car *T compatible avec I (mais pas l'inverse).

 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ées:

• cmd / guru: correction de l'ordre des cas incorrect dans describe.go
• message / pipeline: ordre de cas de commutateur de type fixe
• coyove / goflyway: proxy: correction de l'ordre des cas dans un commutateur de type
• go-graphite / carbonapi: pkg / parser: ordre de boîtier de commutateur de type fixe

offBy1

L'erreur par unité est si populaire qu'elle a même sa propre page wikipedia .

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

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

go-critic a des options limitées pour trouver de telles erreurs, mais jusqu'à présent, aucun correctif n'a été envoyé au référentiel public. Voici quelques correctifs qui ont été trouvés lors de la lecture de l'histoire:

• btcsuite / btcd: tester et corriger les bogues dans getaddednodeinfo
• btcsuite / btcd: améliore la couverture des tests et corrige certains bugs trouvés par les tests

Un peu d'horreur au final

go vet a une bonne vérification pour les expressions comme " x != a || x != b ". Il semblerait que les gens ne connaissent peut-être pas gometalinter , mais go vet lance presque tout le monde, non?

En utilisant l'utilitaire gogrep, j'ai rassemblé une petite liste d'expressions similaires qui se trouvent toujours dans les branches principales de certains projets.

 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) 

Apprendre des erreurs des autres

Non, même dans la dernière partie, il n'y aura rien sur l'apprentissage automatique .

Mais ce qui sera écrit ici concerne golangci-lint , qui a intégré go-critique dans l'une des dernières versions. golangci-lint est un analogue de gometalinter , que vous pouvez lire sur ses avantages, par exemple, dans l'article "Analyse statique dans Go: comment nous gagnons du temps lors de la vérification du code" .

go-critic a récemment été réécrit avec lintpack . Les détails peuvent être trouvés dans Go lintpack: un gestionnaire de linter composable .

Si vous n'avez pas encore commencé à utiliser activement des outils pour analyser vos programmes pour les erreurs potentielles, à la fois stylistiques et logiques, je vous recommande aujourd'hui de réfléchir à nouveau à votre comportement autour d'une tasse de thé avec des collègues. Vous réussirez.

Bon à tous.