Quero compartilhar os resultados dos últimos dias, que incluíram a análise git da história de alguns projetos Go grandes, com o objetivo de encontrar confirmações que corrigiam erros e formalizá-las para detecção, se aparecessem no novo código.
Na segunda parte do artigo, consideraremos alguns novos diagnósticos no go-critical , que permitem encontrar código com grande probabilidade de conter um erro.
Pesquise ideias no passado
Para encontrar idéias para novas inspeções de código, você pode espiar analisadores estáticos para outras linguagens de programação, examinar manualmente o código-fonte aberto na tentativa de encontrar modelos de código incorretos nele ou pesquisar o histórico.
Suponha que você esteja verificando o projeto Go-Files . Depois de iniciar o linter (analisador estático), não há problemas; a auditoria dos textos de origem também não trouxe grandes resultados. Devo me apressar em deixar Go-Files lado? Na verdade não.
Durante a sua existência, o Go-Files teve vários defeitos facilmente detectáveis, mas agora eles já estão corrigidos. O que precisamos fazer é começar a analisar o histórico do repositório git.
Erros estão em algum lugar próximo.
Coleção de commits de interesse para nós
Os problemas:
- Pode haver muitos commits
- Às vezes, na mesma revisão, as coisas não estão relacionadas entre si
- Às vezes, um comentário em uma confirmação não corresponde exatamente às alterações reais.
- Alguns subestimam o uso de boas mensagens de confirmação (não vamos apontar o dedo)
Antes de tudo, queremos reduzir a quantidade de informações que precisarão ser processadas sem automação. O problema das respostas falso-negativas pode ser resolvido aumentando o tamanho da amostra (baixe mais pacotes Go para o grupo de controle).
Você pode analisá-lo de diferentes maneiras: eu tinha o git log --grep para um conjunto de modelos, superestimando e subestimando a pontuação do commit, dependendo do conteúdo de sua mensagem de commit. Se o commit for muito grande, você poderá descartá-lo imediatamente, porque é muito provável que descubra o que acontece, não será trivial.
Eu também gostaria de acrescentar que, além de correções ou erros de localização, muitas vezes não há descrições muito otimistas e, às vezes, não muito culturais de patches:
- "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)
As “correções de bugs” mais inúteis são a edição de erros de sintaxe ou outros problemas que impedem que o projeto seja compilado através da go build . Nem todos os projetos usam IC ou gancho de pré-confirmação , portanto, essas revisões quebradas às vezes acabam na ramificação principal.
Erros históricos
Vamos passar por alguns dos erros mais interessantes que foram encontrados nas capas dos logs do git.
Retorno nulo em vez de resultado real
gin-gonic / gin: corrige um erro, retorna um erro quando falha na ligação do bool :
if err == nil { field.SetBool(boolVal) } - return nil + return err }
go-pg / pg: Correção de bug AppendParam :
return method.AppendValue(b, m.strct.Addr(), 1), true } - return nil, false + return b, false }
Falta de retorno no caminho rápido
gonum / gonum: bug corrigido no caminho rápido dswap :
for i := 0; i < n; i++ { x[i], y[i] = y[i], x[i] } + return }
Executando goroutines sem wg.Add (1)
btcsuite / btcd: corrija vários erros no desligamento do servidor RPC :
+s.wg.Add(1) go s.walletListenerDuplicator()
Condição lógica inválida
gorgonia / gorgonia: também foram corrigidos alguns erros :
-for i := 0; i > start; i++ { +for i := 0; i < start; i++ { retVal[i] = 0 }
src-d / go-git: plumbing / idxfile: corrige a pesquisa de erros no MemoryIndex :
-if low < high { +if low > high { break }
go-xorm / xorm: corrige um erro na soma :
-if !strings.Contains(colName, " ") && strings.Contains(colName, "(") { +if !strings.Contains(colName, " ") && !strings.Contains(colName, "(") {
btcsuite / btcd: server: Correção de bug ao desconectar o par no filteradd :
-if sp.filter.IsLoaded() { +if !sp.filter.IsLoaded() {
btcsuite / btcd: Corrija o bug de teste invertido com o manuseio máximo de operações :
-if pop.opcode.value < OP_16 { +if pop.opcode.value > OP_16 { s.numOps++
Atualizando receptor (este / próprio) passado por valor
Clássico Uma cópia do objeto é modificada dentro do método, pelo que o chamador não verá os resultados esperados.
Corrigido um erro de pilha / fila / deque (receptor de ponteiro) :
-func (s Stack) Push(x interface{}) { - s = append(s, x) +func (s *Stack) Push(x interface{}) { + *s = append(*s, x) }
Atualizando cópias de objetos dentro de um loop
contains / traefik: atribua tarefas filtradas aos aplicativos contidos na fatia :
-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)
containsous / traefik: adicione testes de unidade para o pacote seguro :
-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() }) }
O resultado da função de quebra automática não é usado.
gorgonia / gorgonia: Corrigido um pequeno erro não-conseqüente em Grad () :
if !n.isInput() { - errors.Wrapf(err, ...) - // return + err = errors.Wrapf(err, ...) + return nil, err }
Trabalho incorreto com make
Às vezes, os jovens esquilos " make([]T, count) ", seguidos por append dentro do loop. A opção correta aqui é " make([]T, 0, count) ".
HouzuoGuo / tiedot: corrija um bug de verificação de coleção :
-ids := make([]uint64, benchSize) +ids := make([]uint64, 0)
A correção acima corrige o erro original, mas tem uma falha que foi corrigida em uma das seguintes confirmações:
-ids := make([]uint64, 0) +ids := make([]uint64, 0, benchSize)
Mas algumas operações, como copy ou ScanSlice podem exigir o comprimento "correto".
go-xorm / xorm: correção de bug para SumsInt retornar fatia vazia :
-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 {
Em breve , o crítico irá ajudar a encontrar erros nesta classe.
Outros erros
A enumeração acaba sendo bastante complicada, eu tiro o resto por baixo do spoiler. Esta parte é opcional, portanto você pode deixá-la para mais tarde ou seguir em frente com ousadia.
Eu quero mais!
O fragmento a seguir é interessante, apesar de corrigir o erro, a chave iterável é nomeada após o mesmo nome que foi usado para o valor iterável. O novo código não parece correto e pode confundir o próximo programador.
gonum / gonum: Corrigido o erro nas funções de subconjunto :
-for _, el := range *s1 { +for el := range *s1 { if _, ok := (*s2)[el]; !ok { return false }
Os resultados nomeados são quase as mesmas variáveis comuns e, portanto, são inicializados de acordo com as mesmas regras. Os ponteiros terão o valor nil e, se você quiser trabalhar com esse objeto, precisará inicializar explicitamente esse ponteiro (por exemplo, via new ).
dgraph-io / dgraph: corrigindo um erro de referência de ponteiro nulo no server / main.go :
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)
Na maioria das vezes e na maioria das vezes, as sombras sombreadas ao lidar com erros
btcsuite / btcd: blockchain / indexadores: corrija um bug na re-org do indexador
-block, err := btcutil.NewBlockFromBytes(blockBytes) +block, err = btcutil.NewBlockFromBytes(blockBytes) if err != nil { return err }
go-xorm / xorm: corrija o erro de inserção e erro :
-err = session.Rollback() +err1 := session.Rollback() +if err1 == nil { + return lastId, err +} +err = err1
gin-gonic / gin: Corrigido o problema de nova linha com as funções debugPrint in Run * :
-debugPrint("Listening and serving HTTP on %s", addr) +debugPrint("Listening and serving HTTP on %s\n", addr)
A implementação do mutex no exemplo abaixo me levou à ideia de escrever uma verificação que relata o uso de *sync.Mutex como membro da estrutura. Dave Cheney recomenda sempre usar um valor mutex , não um ponteiro para ele.
tealeg / xlsx: corrige um erro ao carregar a planilha :
styleCache map[int]*Style `-` + lock *sync.RWMutex }
Cruzada contra código suspeito
badCond
A verificação badCond localiza expressões lógicas potencialmente incorretas.
Exemplos de expressões lógicas suspeitas:
- O resultado é sempre
true ou false - A expressão é escrita de uma forma quase indistinguível da incorreta
Essa verificação também funciona em operações como " x == nil && x == y ". Uma solução simples é reescrever para " x == nil && y == nil ". A legibilidade do código não sofre, mas o linter não verá nada suspeito neste código.
Aqui está um exemplo de uma correção de bug que pode ser encontrada pelo badCond :
-if err1 := f(); err != nil && err == nil { +if err1 := f(); err1 != nil && err == nil { err = err1 }
Troféus:
fracoCond
weakCond é semelhante ao badCond , mas o nível de confiança dos alertas é um pouco menor.
Uma condição fraca é uma condição que não cobre a entrada suficientemente completamente.
Um bom exemplo de uma condição fraca (insuficiente) é verificar a fatia para nil e, em seguida, usar o primeiro elemento. As condições " s != nil " não são suficientes aqui. A condição correta é " len(s) != 0 " ou equivalente:
func addRequiredHeadersToRedirectedRequests( req *http.Request, via []*http.Request) error { - if via != nil && via[0] != nil { + if len(via) != 0 && via[0] != nil {
Troféus:
dupArg
Para algumas funções, passar o mesmo valor (ou variável) que vários argumentos não faz muito sentido. Muitas vezes, isso indica um erro de copiar / colar .
Um exemplo dessa função é copy . A copy(xs, xs) expressão copy(xs, xs) não faz sentido.
-if !bytes.Equal(i2.Value, i2.Value) { +if !bytes.Equal(i1.Value, i2.Value) { return fmt.Errorf("tries differ at key %x", i1.Key) }
Troféus:
dupCase
Você provavelmente sabe que no Go não é possível usar valores de case duplicados dentro do switch .
O exemplo abaixo não compila :
switch x := 0; x { case 1: fmt.Println("first") case 1: fmt.Println("second") }
Erro de compilação: caso duplicado 1 no comutador.
Mas e se dermos um exemplo mais interessante :
one := 1 switch x := 1; x { case one: fmt.Println("first") case one: fmt.Println("second") }
Valores duplicados são permitidos através da variável Além disso, switch true oferece ainda mais espaço para erros:
switch x := 0; true { case x == 1: fmt.Println("first") case x == 1: fmt.Println("second") }
E aqui está um exemplo de correção de um erro real:
switch { case m < n: // Upper triangular matrix. // . case m == n: // Diagonal matrix. // . -case m < n: // Lower triangular matrix. +case m > n: // Lower triangular matrix. // . }
Troféus:
dupBranchBody
Instruções condicionais como if/else e switch têm corpos para executar. Quando a condição é atendida, o controle é transferido para a unidade de operações associada. Enquanto outros diagnósticos verificam se as condições desses blocos são diferentes, o dupBranchBody verifica se os próprios blocos executáveis não são completamente idênticos.
A presença de corpos duplicados dentro de instruções condicionais, a menos que seja uma type switch , é pelo menos suspeita.
-if s, err := r.ReceivePack(context.Background(), req); err != nil { - return s, err -} else { - return s, err -} +return r.ReceivePack(context.Background(), req)
Os julgamentos sobre o grau de correção do código abaixo são deixados ao leitor:
if field.IsMessage() || p.IsGroup(field) {
Os corpos dentro de " if field.IsString() " e o associado " else " são idênticos.
Troféus:
caseOrder
Todos os tipos dentro da type switch classificados sequencialmente, para o primeiro compatível. Se o valor do tag for do tipo *T e implementar a interface I , será necessário colocar um rótulo com " case *T " antes do rótulo com " case I ", caso contrário, apenas " case I " será executado, pois *T compatível com I (mas não vice-versa).
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))
Troféus:
offBy1
O erro por unidade é tão popular que até possui sua própria página da 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 tem opções limitadas para encontrar esses erros, mas até agora nenhuma correção foi enviada ao repositório público. Aqui estão algumas correções encontradas durante a exibição da história:
Um pouco de horror no final
go vet faz uma boa verificação de expressões como " x != a || x != b ". Parece que as pessoas podem não saber sobre o gometalinter , mas go vet lança quase todo mundo, certo?
Usando o utilitário gogrep, montei uma pequena lista de expressões semelhantes que ainda estão nos ramos principais de alguns projetos.
Para os gatos mais corajosos
Proponho considerar esta lista e enviar solicitações pull.
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)
Aprendendo com os erros dos outros
Não, mesmo na última parte, não haverá nada sobre aprendizado de máquina .
Mas o que será escrito aqui é sobre golangci-lint , que integrou o crítico em um dos últimos lançamentos. golangci-lint é um análogo do gometalinter , que você pode ler sobre as vantagens dele, por exemplo, no artigo "Análise estática no Go: como economizamos tempo ao verificar o código" .
go-critic foi reescrito recentemente usando o lintpack . Detalhes podem ser encontrados no Go lintpack: um gerenciador de linter composível .
Se você ainda não começou a usar ativamente as ferramentas para analisar seus programas quanto a possíveis erros, tanto estilísticos quanto lógicos, hoje recomendo mais uma vez pensar em seu comportamento durante uma xícara de chá com os colegas. Você terá sucesso.
Bom para todos.