Qualidade é importante para nós. E ouvimos sobre o PVS-Studio. Tudo isso levou ao desejo de verificar o Docotic.Pdf e descobrir o que mais pode ser melhorado.
Docotic.Pdf é uma biblioteca de uso geral para trabalhar com arquivos PDF. Está escrito em C #, não há código inseguro, nenhuma dependência externa que não seja o .NET runtime. Ele funciona no .NET 4+ e no .NET Standard 2+.
A biblioteca está em desenvolvimento há pouco mais de 10 anos e possui 110 mil linhas de código sem levar em conta testes, exemplos e outras coisas. Para análise estática, usamos constantemente a Análise de código e o StyleCop. Vários milhares de testes automatizados nos protegem de regressões. Nossos clientes de diferentes países e indústrias diferentes confiam na qualidade da biblioteca.
Quais problemas o PVS-Studio detectará?
Instalação e primeira impressão
Eu baixei a versão de teste no site do PVS-Studio. Agradavelmente surpreso com o pequeno tamanho do instalador. Instalado com configurações padrão: mecanismos de análise, um ambiente PVS-Studio separado, integração ao Visual Studio 2017.
Após a instalação, nada foi iniciado e dois atalhos com os mesmos ícones foram adicionados ao menu Iniciar: Autônomo e PVS-Studio. Por um momento, pensei no que começar. Lançou o Standalone e ficou desagradavelmente surpreso com a interface. A escala de 200% definida para o meu Windows é suportada de forma torta. Parte do texto é muito pequena, parte do texto não cabe no espaço fornecido para ele. O nome, o unicórnio e a lista de ações são cortados para qualquer tamanho de janela. Mesmo com tela cheia.
Bem, eu decidi abrir meu arquivo de projeto. De repente, o menu Arquivo não encontrou essa oportunidade. Lá me ofereceram apenas para abrir arquivos individuais. Obrigado, pensei, prefiro tentar outra opção. Lançado o PVS-Studio - eles me mostraram uma janela com texto embaçado. A escala de 200% voltou a ser sentida. O texto informava:
procure-me em Três Coroas, procure o menu PVS-Studio no Visual Studio. Ok, abriu o Studio.
Solução aberta. De fato, existe um menu do PVS-Studio e ele pode verificar o "Projeto atual". Ele fez o projeto que eu preciso atual e lançou um cheque. Uma janela apareceu no Studio com os resultados da análise. Uma janela apareceu em segundo plano com o andamento da verificação, mas não a encontrei imediatamente. A princípio, havia a sensação de que a verificação não começou ou terminou imediatamente.
Resultado da primeira verificação
O analisador verificou todos os 1253 arquivos do projeto em aproximadamente 9 minutos e 30 segundos. No final da verificação, o contador de arquivos não mudou tão rápido quanto no início. Talvez exista alguma dependência não linear da duração da verificação do número de arquivos verificados.
Informações sobre os avisos 81 Alto, 109 Médio e 175 Baixo apareceram na janela de resultados. Se você calcular a frequência, receberá 0,06 Altos avisos / arquivo, 0,09 Altos médios / arquivo e 0,14 Altos avisos / arquivo. Ou
0,74 Altos avisos por mil linhas de código, 0,99 Alertas médios por mil linhas de código e 1,59 Altos avisos por mil linhas de código.
Aqui
neste artigo , é
indicado que no CruiseControl.NET, com suas 256 mil linhas de código, o analisador encontrou 15 avisos de Alta, 151 Média e 32 Baixa.
Acontece que, em termos percentuais para o Docotic.Pdf, foram emitidos mais avisos em cada um dos grupos.
O que é encontrado?
Decidi ignorar avisos baixos nesta fase.
Classifiquei os avisos pela coluna Código e verificou-se que o detentor absoluto de registro da frequência era
V3022 “Expressão sempre verdadeira / falsa” e
V3063 “Uma parte da expressão condicional é sempre verdadeira / falsa se for avaliada”. Na minha opinião, eles são sobre uma coisa. No total, esses dois avisos fornecem 92 de 190 casos. Frequência relativa = 48%.
A lógica da divisão em Alto e Médio não é totalmente clara. Eu estava esperando
V3072 "A classe 'A' contendo membros IDisposable não implementa ela mesma IDisposable" e
V3073 "Nem todos os membros IDisposable estão descartados corretamente. Chame 'Dispose' ao descartar a classe 'A' ”no grupo High, por exemplo. Mas isso é gosto, é claro.
Surpreendeu-se que o
V3095 “O objeto foi usado antes de ser verificado em relação a nulo. Verifique as linhas: N1, N2 ”está marcado duas vezes como Alto e uma vez como Médio. Bug?
Confie, mas verifique
Chegou a hora de verificar a razoabilidade dos avisos. Existem erros reais encontrados? Existem avisos incorretos?
Dividi os avisos encontrados nos grupos abaixo.
Avisos importantes
Sua correção aumentou a estabilidade, resolveu problemas com vazamentos de memória, etc. Erros / imperfeições reais.
16 deles foram emitidos, o que representa cerca de 8% de todos os avisos.
Vou dar alguns exemplos.
V3019 "Possivelmente uma variável incorreta é comparada com nulo após a conversão do tipo usando a palavra-chave 'as'. Verifique as variáveis 'cor', 'indexado' »
public override bool IsCompatible(ColorImpl color) { IndexedColorImpl indexed = color as IndexedColorImpl; if (color == null) return false; return indexed.ColorSpace.Equals(this); }
Como você pode ver, em vez de indexada, a variável cor é comparada com nula. Isso está incorreto e pode levar ao NRE.
V3080 “Possível desreferência nula. Considere inspecionar 'cstr_index.tile_index' »
Um pequeno fragmento para ilustrar:
if (cstr_index.tile_index == null) { if (cstr_index.tile_index[0].tp_index == null) {
Obviamente, a primeira condição implicava! = Nulo. No formulário atual, o código lançará NRE a cada chamada.
V3083 “
Chamada não segura do evento 'OnProgress', NullReferenceException é possível. Considere atribuir um evento a uma variável local antes de invocá-lo. ”
public void Updated() { if (OnProgress != null) OnProgress(this, new EventArgs()); }
Um aviso ajudou a corrigir uma possível exceção. Por que isso pode surgir? Stackoverflow tem uma
boa explicação .
V3106 “Possivelmente o índice está fora dos limites. O índice '0' está apontando além do limite 'v' »
var result = new List<FontStringPair>(); for (int i = 0; i < text.Length; ++i) { var v = new List<FontStringPair>(); createPairs(text[i].ToString(CultureInfo.InvariantCulture)); result.Add(v[0]); }
O erro é que o resultado de createPairs é ignorado e, em vez disso, uma lista vazia é acessada. Aparentemente, o createPairs aceitou a lista como parâmetro, mas ocorreu um erro no processo de alteração do método.
V3117 'Parâmetro do construtor' validateType 'não é usado
Um aviso foi emitido para um código semelhante a este
public SomeClass(IDocument document, bool validateType = true) : base(document, true) { m_provider = document; }
O aviso em si não parece importante. Mas o problema é mais sério do que parece à primeira vista. No processo de adição do parâmetro validateType opcional, eles esqueceram de corrigir a chamada ao construtor da classe base.
V3127 „Dois fragmentos de código semelhantes foram encontrados. Talvez seja um erro de digitação e a variável 'range' deva ser usada em vez de 'domain' "
private void fillTransferFunction(PdfStreamImpl function) {
Talvez um aviso não seja emitido se as partes do código forem ligeiramente diferentes. Mas, neste caso, um erro foi detectado ao usar copiar e colar.
Advertências teóricas / formais
Eles estão corretos, mas sua correção não corrige erros específicos e não afeta a legibilidade do código. Ou eles apontam para lugares onde pode haver um erro, mas ele não existe. Por exemplo, a ordem dos parâmetros é alterada intencionalmente. Para esses avisos, você não precisa alterar nada no programa.
Destas, 57 foram emitidas, o que representa cerca de 30% de todos os avisos. Vou dar exemplos de casos que merecem atenção.
V3013 „É estranho que o corpo da função 'BeginText' seja totalmente equivalente ao corpo da função 'EndText' (166, linha 171)“
public override void BeginText() { m_state.ResetTextParameters(); } public override void EndText() { m_state.ResetTextParameters(); }
Ambas as funções do corpo são realmente as mesmas. Mas está certo. E é realmente tão estranho se os corpos de funções de uma linha coincidem?
V3106 „Possível valor negativo do índice. O valor do índice 'c1' pode chegar a -1 "
freq[256] = 1;
Concordo, dei um pedaço do algoritmo não tão claro. Mas, na minha opinião, neste caso, o analisador se preocupa em vão.
V3107 “Expressão idêntica 'neighsum' à esquerda e à direita da atribuição composta”
O aviso é causado por um código bastante comum:
neighsum += neighsum;
Sim, pode ser reescrito através da multiplicação. Mas não há erro.
V3109 „A subexpressão 'l_cblk.data_current_size' está presente nos dois lados do operador. A expressão está incorreta ou pode ser simplificada. "
if ((l_cblk.data_current_size + l_seg.newlen) < l_cblk.data_current_size) {
Os comentários no código esclarecem a intenção. Novamente alarme falso.
Avisos justificados
Sua correção afetou positivamente a legibilidade do código. Ou seja, reduziu condições desnecessárias, verificações etc. O efeito sobre como o código funciona não é óbvio.
Destas, 103 foram emitidas, o que representa cerca de 54% de todos os avisos.
V3008 „A variável 'l_mct_deco_data' recebe valores duas vezes sucessivamente. Talvez isso seja um erro "
if (m_nb_mct_records == m_nb_max_mct_records) { ResizeMctRecords(); l_mct_deco_data = (OPJ_INT32)m_nb_mct_records; } l_mct_deco_data = (OPJ_INT32)m_nb_mct_records;
Analisador de direitos: atribuição dentro, se não for necessário.
V3009 „É estranho que esse método sempre retorne um e o mesmo valor“
private static bool opj_dwt_decode_tile(opj_tcd_tilecomp_t tilec, OPJ_UINT32 numres) { if (numres == 1U) return true;
A conselho do analisador, o método foi alterado e não retorna mais nada.
V3022 "A expressão '! Add' é sempre verdadeira"
private void addToFields(PdfDictionaryImpl controlDict, bool add) {
De fato, não há sentido no segundo se. A condição sempre será verdadeira.
V3029 "A expressão condicional das instruções 'if' localizadas uma ao lado da outra é idêntica"
if (stroke) extGState.OpacityStroke = opacity; if (stroke) state.AddReal(Field.CA, opacity);
Não está claro como esse código se originou. Mas agora nós consertamos.
V3031 „Uma verificação excessiva pode ser simplificada. O '||' operador está cercado por expressões opostas "
Esta é uma condição de pesadelo:
if (!(cp.m_enc.m_tp_on != 0 && ((!opj_codec_t.OPJ_IS_CINEMA(cp.rsiz) && (t2_mode == J2K_T2_MODE.FINAL_PASS)) || opj_codec_t.OPJ_IS_CINEMA(cp.rsiz)))) {
Após as mudanças, ficou muito melhor
if (!(cp.m_enc.m_tp_on != 0 && (opj_codec_t.OPJ_IS_CINEMA(cp.rsiz) || t2_mode == J2K_T2_MODE.FINAL_PASS))) {
V3063 “Uma parte da expressão condicional sempre é verdadeira se for avaliada: x! = Nulo”
V3022 “A expressão 'x! = Nulo' sempre é verdadeira”
Aqui incluímos avisos de que a verificação de nulo não faz sentido. Se é certo fazê-lo é uma questão controversa. Abaixo, descrevi com mais detalhes a essência do problema.
Avisos infundados
Falsos positivos Devido a erros na implementação de um teste específico ou algum tipo de falha do analisador.
Destes, 14 foram emitidos, o que representa cerca de 7% de todos os avisos.
V3081 “O contador 'i' não é usado dentro de um loop aninhado. Considere inspecionar o uso do contador 'j'
Uma versão ligeiramente simplificada do código para o qual este aviso foi emitido:
for (uint i = 0; i < initialGlyphsCount - 1; ++i) { for (int j = 0; j < initialGlyphsCount - i - 1; ++j) {
Obviamente, i é usado em um loop aninhado.
V3125 „O objeto foi usado depois de verificado contra nulo“
Código para o qual é emitido um aviso:
private static int Compare_SecondGreater(cmapEncodingRecord er1, cmapEncodingRecord er2) { if (er1 == er2) return 0; if (er1 != null && er2 == null) return 1; if (er1 == null && er2 != null) return -1; return er1.CompareTo(er2); }
er1 não pode ser nulo quando CompareTo () é chamado.
Outro código para o qual este aviso é emitido:
private static void realloc(ref int[][] table, int newSize) { int[][] newTable = new int[newSize][]; int existingSize = 0; if (table != null) existingSize = table.Length; for (int i = 0; i < existingSize; i++) newTable[i] = table[i]; for (int i = existingSize; i < newSize; i++) newTable[i] = new int[4]; table = newTable; }
A tabela não pode ser nula em um loop.
V3134 „A mudança de [32..255] bits é maior que o tamanho do tipo de expressão 'UInt32' '(uint) 1'“
Um pedaço de código para o qual este aviso é emitido:
byte bitPos = (byte)(numBits & 0x1F); uint mask = (uint)1 << bitPos;
Pode-se observar que o bitPos pode ter um valor no intervalo [0..31], mas o analisador acredita que pode ter um valor no intervalo [0..31], o que está incorreto.
Não darei outros casos semelhantes, pois são equivalentes.
Pensamentos adicionais sobre algumas verificações
Pareceu-me indesejável avisar que 'x! = Null' sempre é verdadeiro nos casos em que x é o resultado de chamar algum método. Um exemplo:
private X GetX(int y) { if (y > 0) return new X(...); if (y == 0) return new X(...); throw new ArgumentOutOfRangeException(nameof(x)); } private Method() {
Sim, formalmente, o analisador está certo: x nem sempre será nulo, porque o GetX retornará uma instância completa ou lançará uma exceção. Mas o código melhorará a remoção da verificação por nulo? E se o GetX mudar depois? O Method precisa conhecer a implementação do GetX?
Dentro da equipe, as opiniões foram divididas. Foi sugerido que o método atual possui um contrato pelo qual não deve retornar nulo. E não faz sentido escrever código redundante "para o futuro" a cada chamada. E se o contrato mudar, o código de chamada deve ser atualizado.
Para sustentar essa opinião, foi feito o seguinte julgamento: verificar nulo é como agrupar todas as chamadas em try / catch, caso o método comece a lançar exceções no futuro.
Como resultado, de acordo com o princípio
YAGNI , eles decidiram não se apegar aos cheques e os excluíram. Todos os avisos foram transferidos de teórico / formal para justificado.
Ficarei feliz em ler sua opinião nos comentários.
Conclusões
A análise estática é uma coisa boa. O PVS-Studio permite encontrar erros reais.
Sim, existem avisos não razoáveis / incorretos. Mas o PVS-Studio ainda encontrou erros reais em um projeto que já usa a Análise de código. Nosso produto é razoavelmente bem coberto por testes, de uma forma ou de outra testado por nossos clientes, mas os
robôs fazem isso melhor. A análise estática ainda é benéfica.
Finalmente, algumas estatísticas.
Os 3 principais avisos irracionais
V3081 „O contador 'X' não é usado dentro de um loop aninhado. Considere inspecionar o uso do contador 'Y' "
1 de 1 consideraram razoável
V3125 „O objeto foi usado depois de verificado contra nulo. Verifique as linhas: N1, N2 "
9 em 10 são declarados improcedentes
V3134 "A
troca por N bits é maior que o tamanho do tipo"
4 de 5 considerados infundados
Os 3 principais avisos importantes
V3083 “
Chamada insegura do evento, NullReferenceException é possível. Considere atribuir um evento a uma variável local antes de invocá-la "
5 em 5 foram considerados importantes.
V3020 „Uma 'interrupção / continuação / retorno / goto' incondicional dentro de um loop“
V3080 “Possível desreferência nula”
2 de 2 foram reconhecidos como importantes.
V3019 „É possível que uma variável incorreta seja comparada com nulo após a conversão do tipo usando a palavra-chave 'as'
V3127 „Dois fragmentos de código semelhantes foram encontrados. Talvez seja um erro de digitação e a variável 'X' deva ser usada em vez de 'Y'
1 em 1 foi considerado importante.