A publicação do código fonte da Microsoft para seus projetos é um bom motivo para verificá-los. Desta vez não foi excepção, e hoje analisamos os locais suspeitos encontrados no código Infer.NET. Abaixo a anotação - vá direto ao ponto!
Um pouco sobre o projeto e o analisador
O Infer.NET é um sistema de aprendizado de máquina desenvolvido por especialistas da Microsoft. O código fonte do projeto ficou recentemente disponível no
GitHub , que foi o motivo da sua verificação. Mais detalhes sobre o projeto podem ser encontrados, por exemplo,
aqui .
O projeto foi verificado usando o analisador estático PVS-Studio versão 6.26. Deixe-me lembrá-lo de que o PVS-Studio está procurando erros no código em C \ C ++ \ C # (e logo em Java) no Windows, Linux, macOS. No momento, o código C # está sendo analisado apenas no Windows. O analisador pode ser
baixado e testado em seu projeto.
O cheque em si era extremamente simples e sem problemas. Anteriormente, descarreguei o projeto do GitHub, restaurei os pacotes necessários (dependências) e verifiquei se o projeto foi construído com êxito. Isso é necessário para que o analisador tenha acesso a todas as informações necessárias para uma análise completa. Após reunir alguns cliques, lancei a análise da solução por meio do plug-in PVS-Studio para Visual Studio.
A propósito, este não é o primeiro projeto da Microsoft testado usando o PVS-Studio - havia outros:
Roslyn ,
MSBuild ,
PowerShell ,
CoreFX e
outros .
Nota Se você ou seus conhecidos estiverem interessados em analisar o código Java, escreva-nos para o
suporte selecionando “Quero um analisador para Java”. Não existe uma versão beta pública do analisador, mas ela deverá estar disponível em breve. Em algum lugar de um laboratório secreto (através da parede), os caras estão trabalhando ativamente nele.
Mas chega de conversa abstrata - vamos ver os problemas no código.
Isso é um bug ou um recurso?
Sugiro tentar encontrar o erro você mesmo - uma tarefa completamente solucionável. Não há brincadeiras no espírito do que estava no artigo "
Os 10 principais erros nos projetos C ++ para 2017 ", honestamente. Portanto, não se apresse em ler o aviso do analisador fornecido após o trecho de código.
private void MergeParallelTransitions() { .... if ( transition1.DestinationStateIndex == transition2.DestinationStateIndex && transition1.Group == transition2.Group) { if (transition1.IsEpsilon && transition2.IsEpsilon) { .... } else if (!transition1.IsEpsilon && !transition2.IsEpsilon) { .... if (double.IsInfinity(transition1.Weight.Value) && double.IsInfinity(transition1.Weight.Value)) { newElementDistribution.SetToSum( 1.0, transition1.ElementDistribution, 1.0, transition2.ElementDistribution); } else { newElementDistribution.SetToSum( transition1.Weight.Value, transition1.ElementDistribution, transition2.Weight.Value, transition2.ElementDistribution); } .... }
PVS-Studio Warning :
V3001 Existem
subexpressões idênticas 'double.IsInfinity (transit1.Weight.Value)' à esquerda e à direita do operador '&&'. Runtime Automaton.Simplification.cs 479
Como você pode ver no snippet de código, o método está trabalhando com um par de variáveis -
transição1 e
transição2 . O uso de nomes semelhantes às vezes é bastante justificado, mas vale lembrar que, neste caso, a probabilidade de cometer um erro acidental em algum lugar com o nome aumenta.
Foi o
que aconteceu ao verificar os números para o infinito (
double.IsInfinity ). Devido a um erro, verificamos o valor da mesma variável 2 vezes -
transição1.Peso.Valor . O valor verificado na segunda subexpressão deveria ser a variável
transit2.Weight.Value .
Outro código suspeito semelhante.
internal MethodBase ToMethodInternal(IMethodReference imr) { .... bf |= BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Instance; .... }
PVS-Studio Warning :
V3001 Existem sub-expressões idênticas 'BindingFlags.Public' à esquerda e à direita da '|' operador. Compilador CodeBuilder.cs 194
Ao formar o valor da variável
bf , o elemento de enumeração
BindingFlags.Public é usado duas vezes. Esse código contém uma operação de sinalização extra ou, em vez do segundo uso de
BindingFlags.Public, deve haver um valor de enumeração diferente.
A propósito, no código-fonte, esse código é escrito em uma linha. Parece-me que, se estiver formatado no estilo de tabela (como aqui), o problema é mais fácil de detectar.
Vamos seguir em frente. Trago todo o corpo do método e, novamente, sugiro que você encontre o erro (ou talvez o erro).
private void ForEachPrefix(IExpression expr, Action<IExpression> action) {
Você encontrou? Estamos verificando!
Avisos do PVS-Studio :
- V3003 O uso do padrão 'if (A) {...} else if (A) {...}' foi detectado. Há uma probabilidade de presença de erro lógico. Verifique as linhas: 1719, 1727. Compilador CodeRecognizer.cs 1719
- V3003 O uso do padrão 'if (A) {...} else if (A) {...}' foi detectado. Há uma probabilidade de presença de erro lógico. Verifique as linhas: 1721, 1729. Compilador CodeRecognizer.cs 1721
Simplifique um pouco o código para tornar os problemas mais óbvios.
private void ForEachPrefix(IExpression expr, Action<IExpression> action) { if (....) .... else if (expr is IUnaryExpression) ForEachPrefix(((IUnaryExpression)expr).Expression, action); else if (expr is IAddressReferenceExpression) ForEachPrefix(((IAddressReferenceExpression)expr).Expression, action); .... else if (expr is IUnaryExpression) ForEachPrefix(((IUnaryExpression)expr).Expression, action); else if (expr is IAddressReferenceExpression) ForEachPrefix(((IAddressReferenceExpression)expr).Expression, action) .... }
Expressões condicionais e ramificações de várias
instruções if se duplicadas. Talvez esse código tenha sido escrito usando o método copiar e colar, e é por isso que o problema surgiu. Agora acontece que os ramos das duplicatas nunca serão executados, pois:
- se a expressão condicional for verdadeira, o corpo da primeira instrução if do par correspondente será executada;
- se a expressão condicional for falsa no primeiro caso, será falsa no segundo.
Como os ramos contêm as mesmas ações, agora parece um código redundante que é confuso. É possível que este seja um tipo diferente de problema - em vez de tomadas, outras verificações deveriam ter sido realizadas.
Nós continuamos.
public int Compare(Pair<int, int> x, Pair<int, int> y) { if (x.First < y.First) { if (x.Second >= y.Second) {
Avisos do PVS-Studio :
- V3004 A instrução 'then' é equivalente à instrução 'else'. Tempo de execução RegexpTreeBuilder.cs 1080
- V3004 A instrução 'then' é equivalente à instrução 'else'. Tempo de execução RegexpTreeBuilder.cs 1093
O código parece extremamente suspeito, pois contém duas instruções condicionais com corpos idênticos dos ramos
then e
else . Nos dois casos, provavelmente vale a pena retornar valores diferentes. Ou, se esse for um comportamento concebido, será útil remover instruções condicionais redundantes.
Houve ciclos interessantes. Exemplo abaixo:
private static Set<StochasticityPattern> IntersectPatterns(IEnumerable<StochasticityPattern> patterns) { Set<StochasticityPattern> result = new Set<StochasticityPattern>(); result.AddRange(patterns); bool changed; do { int count = result.Count; AddIntersections(result); changed = (result.Count != count); break; } while (changed); return result; }
PVS-Studio Warning :
V3020 Uma 'interrupção' incondicional dentro de um loop. Compilador DefaultFactorManager.cs 474
Devido à instrução incondicional de
interrupção , exatamente uma iteração do loop é executada e a variável de controle
alterada nem é usada. Em geral, o código parece estranho e suspeito.
O mesmo método (cópia exata) foi encontrado em outra classe. Aviso correspondente do analisador:
V3020 Uma 'quebra' incondicional dentro de um loop. Visualizers.Windows FactorManagerView.cs 350
A propósito, um método encontrou uma declaração
continue incondicional em um loop (foi encontrada pelo analisador com o mesmo diagnóstico), mas houve um comentário acima confirmando que esta é uma solução temporária especial:
Lembro-me de que não houve tais comentários perto da declaração de
quebra incondicional.
Vamos seguir em frente.
internal static DependencyInformation GetDependencyInfo(....) { .... IExpression resultIndex = null; .... if (resultIndex != null) { if (parameter.IsDefined( typeof(SkipIfMatchingIndexIsUniformAttribute), false)) { if (resultIndex == null) throw new InferCompilerException( parameter.Name + " has SkipIfMatchingIndexIsUniformAttribute but " + StringUtil.MethodNameToString(method) + " has no resultIndex parameter"); .... } .... } .... }
Aviso do PVS-Studio :
A expressão V3022 'resultIndex == null' é sempre falsa. Compiler FactorManager.cs 382
Imediatamente, observo que entre a declaração e a verificação acima, o valor da variável
resultIndex pode mudar. No entanto, entre as verificações
resultIndex! = Null e
resultIndex == null, o valor já não pode ser alterado. Portanto, o resultado da expressão
resultIndex == null sempre será
falso , o que significa que uma exceção nunca será lançada.
Espero que você tenha interesse em encontrar erros, sem as minhas sugestões, para encontrar um problema, mas, apenas no caso, proponho fazê-lo novamente. O código do método é pequeno, eu darei na íntegra.
public static Tuple<int, string> ComputeMovieGenre(int offset, string feature) { string[] genres = feature.Split('|'); if (genres.Length < 1 && genres.Length > 3) { throw new ArgumentException(string.Format( "Movies should have between 1 and 3 genres; given {0}.", genres.Length)); } double value = 1.0 / genres.Length; var result = new StringBuilder( string.Format( "{0}:{1}", offset + MovieGenreBuckets[genres[0]], value)); for (int i = 1; i < genres.Length; ++i) { result.Append( string.Format( "|{0}:{1}", offset + MovieGenreBuckets[genres[i].Trim()], value)); } return new Tuple<int, string>(MovieGenreBucketCount, result.ToString()); }
Vamos ver o que acontece aqui. A sequência de entrada é analisada pelo caractere '|'. Se o comprimento da matriz não for o esperado, uma exceção deverá ser lançada.
Espere um segundo ...
gêneros.Comprimento <1 e& gêneros.Comprimento> 3 ? Como não há número que caia imediatamente no intervalo de valores exigido pela expressão (
[int.MinValue..1) e
(3..int.MaxValue] ), o resultado da expressão sempre será
falso . Portanto, essa verificação não protege contra nada e a exceção esperada não será lançada.
É exatamente isso que o analisador adverte:
V3022 A expressão 'genres.Length <1 && genres.Length> 3' é sempre falsa. Provavelmente o '||' operador deve ser usado aqui. Recursos do avaliador.cs 242
Operação de fissão suspeita atendida.
public static void CreateTrueThetaAndPhi(....) { .... double expectedRepeatOfTopicInDoc = averageDocLength / numUniqueTopicsPerDoc; .... int cnt = Poisson.Sample(expectedRepeatOfTopicInDoc); .... }
PVS-Studio Warning :
V3041 A expressão foi convertida implicitamente do tipo 'int' para o tipo 'double'. Considere utilizar uma conversão de tipo explícita para evitar a perda de uma parte fracionária. Um exemplo: double A = (double) (X) / Y; LDA Utilities.cs 74
Isso é suspeito aqui: a divisão inteira é executada (as variáveis
averageDocLength e
numUniqueTopicsPerDoc são do tipo
int ) e o resultado é gravado em uma variável do tipo
double . A pergunta é implícita: isso foi feito especialmente ou ainda estava implícita a divisão de números reais? Se a variável
esperadoRepeatOfTopicInDoc fosse do tipo
int , isso esclareceria possíveis perguntas.
Em outros lugares, o método
Poisson.Sample , cujo argumento é a variável suspeita
expectRepeatOfTopicInDoc , é usado, por exemplo, conforme descrito abaixo.
int numUniqueWordsPerTopic = Poisson.Sample((double)averageWordsPerTopic);
averageWordsPerTopic é do tipo
int , que já foi convertido para
dobrar no local de uso.
E aqui está outro local de uso:
double expectedRepeatOfWordInTopic = ((double)numDocs) * averageDocLength / numUniqueWordsPerTopic; .... int cnt = Poisson.Sample(expectedRepeatOfWordInTopic);
Observe que as variáveis têm os mesmos nomes que no exemplo original, apenas a divisão de números reais é usada para inicializar o
esperadoRepeatOfWordInTopic (devido à conversão explícita de
numDocs para o
dobro ).
Em geral, vale a pena observar o ponto de partida em que o analisador emitiu um aviso.
Mas reflexões sobre se vale a pena editar e como deixar os autores do código (eles sabem melhor), mas vamos além. Para a próxima divisão suspeita.
public static NonconjugateGaussian BAverageLogarithm(....) { .... double v_opt = 2 / 3 * (Math.Log(mx * mz / Ex2 / 2) - m); if (v_opt != v) { .... } .... }
PVS-Studio Warning :
V3041 A expressão foi convertida implicitamente do tipo 'int' para o tipo 'double'. Considere utilizar uma conversão de tipo explícita para evitar a perda de uma parte fracionária. Um exemplo: double A = (double) (X) / Y; Runtime ProductExp.cs 137
O analisador detectou novamente a operação suspeita da divisão inteira, como
2 e
3 são literais numéricos inteiros, e o resultado da expressão 2/3 será
0 . Como resultado, toda a expressão assume a forma:
double v_opt = 0 * expr;
Concordo, um pouco estranho. Várias vezes voltei a esse aviso, tentando encontrar algum tipo de problema e não tentando adicioná-lo ao artigo. O método está cheio de matemática e várias fórmulas (que, francamente, eu realmente não queria desmontar), você nunca sabe o que esperar. Além disso, tento ser o mais cético possível sobre os avisos que escrevo no artigo e os descrevo somente depois de estudá-los melhor.
Mas então me dei conta - por que preciso de um fator
0 , escrito como
2/3 ? Portanto, este lugar vale uma olhada de qualquer maneira.
public static void WriteAttribute(TextWriter writer, string name, object defaultValue, object value, Func<object, string> converter = null) { if ( defaultValue == null && value == null || value.Equals(defaultValue)) { return; } string stringValue = converter == null ? value.ToString() : converter(value); writer.Write($"{name}=\"{stringValue}\" "); }
PVS-Studio Warning :
V3080 Possível desreferência nula. Considere inspecionar 'valor'. Compilador WriteHelpers.cs 78
Afirmação bastante justa do analisador com base na condição. A desreferenciação de uma referência nula pode ocorrer na expressão
value.Equals (defaultValue) se
value == null . Como essa expressão é o operando direito do operador ||, para calculá-lo, o operando esquerdo deve ser
falso e, para isso, basta que pelo menos uma das variáveis
defaultValue \
value não seja
nula . Como resultado, se
defaultValue! = Null e
value == null :
- defaultValue == null -> false ;
- defaultValue == null && value == null -> false ; (verificação de valor não ocorreu)
- value.Equals (defaultValue) -> NullReferenceException , já que o valor é nulo .
Vejamos um caso semelhante:
public FeatureParameterDistribution( GaussianMatrix traitFeatureWeightDistribution, GaussianArray biasFeatureWeightDistribution) { Debug.Assert( (traitFeatureWeightDistribution == null && biasFeatureWeightDistribution == null) || traitFeatureWeightDistribution.All( w => w != null && w.Count == biasFeatureWeightDistribution.Count), "The provided distributions should be valid and consistent in the number of features."); .... }
PVS-Studio Warning :
V3080 Possível desreferência nula. Considere inspecionar 'traitFeatureWeightDistribution'. Recomendador FeatureParameterDistribution.cs 65
Jogamos fora o excesso, deixando apenas a lógica para calcular valores booleanos, para que seja mais fácil descobrir:
(traitFeatureWeightDistribution == null && biasFeatureWeightDistribution == null) || traitFeatureWeightDistribution.All( w => w != null && w.Count == biasFeatureWeightDistribution.Count)
Novamente, o operando certo do || É calculado apenas se o resultado do cálculo esquerdo for
falso . O operando esquerdo pode ser
falso , incluindo quando
traitFeatureWeightDistribution == null e
biasFeatureWeightDistribution! = Null . Em seguida, o operando correto do operador || será calculado e uma chamada para
traitFeatureWeightDistribution.All gerará um
ArgumentNullException .
Outro pedaço interessante de código:
public static double GetQuantile(double probability, double[] quantiles) { .... int n = quantiles.Length; if (quantiles == null) throw new ArgumentNullException(nameof(quantiles)); if (n == 0) throw new ArgumentException("quantiles array is empty", nameof(quantiles)); .... }
Aviso do PVS-Studio :
V3095 O objeto 'quantiles' foi usado antes de ser verificado com relação a nulo. Verifique as linhas: 91, 92. Runtime OuterQuantiles.cs 91
Observe que a propriedade
quantiles.Length é
acessada primeiro e, em seguida, os
quantis são verificados quanto a
nulo . Como resultado, se
quantis == nulos , o método lançará uma exceção, apenas um pouco errada e um pouco fora do local necessário. Aparentemente, eles estragaram as linhas em alguns lugares.
Se você lidou com êxito com a detecção dos erros anteriores, sugiro que você prepare uma xícara de café e tente repetir o feito, encontrando o erro no método abaixo. Para torná-lo um pouco mais interessante, cito todo o código do método.
(
Link para tamanho real )
Ok, ok, isso foi uma piada (ou você conseguiu?!). Vamos simplificar um pouco a tarefa:
if (sample.Precision < 0) { precisionIsBetween = true; lowerBound = -1.0 / v; upperBound = -mean.Precision; } else if (sample.Precision < -mean.Precision) { precisionIsBetween = true; lowerBound = 0; upperBound = -mean.Precision; } else {
Melhorou? O analisador emitiu o seguinte aviso para este código:
V3008 A variável 'lowerBound' recebe valores duas vezes sucessivamente. Talvez isso seja um erro. Verifique as linhas: 324, 323. Tempo de execução GaussianOp.cs 324
De fato, na última ramificação
else , o valor da variável
lowerBound é atribuído duas vezes seguidas. Aparentemente (e a julgar pelo código acima), a variável
upperBound deve estar envolvida em uma das atribuições.
Seguimos adiante.
private void WriteAucMatrix(....) { .... for (int c = 0; c < classLabelCount; c++) { int labelWidth = labels[c].Length; columnWidths[c + 1] = labelWidth > MaxLabelWidth ? MaxLabelWidth : labelWidth; for (int r = 0; r < classLabelCount; r++) { int countWidth = MaxValueWidth; if (countWidth > columnWidths[c + 1]) { columnWidths[c + 1] = countWidth; } } .... }
PVS-Studio Warning :
V3081 O contador 'r' não é usado dentro de um loop aninhado. Considere inspecionar o uso do contador 'c'. CommandLine ClassifierEvaluationModule.cs 459
Observe que o contador do loop interno -
r - não é usado no corpo desse loop. Por causa disso, acontece que durante todas as iterações do loop interno, as mesmas operações são executadas nos mesmos elementos - afinal, os índices também usam o contador do loop externo (
c ) e não o interno (
r ).
Vamos ver o que mais foi encontrado interessante.
public RegexpFormattingSettings( bool putOptionalInSquareBrackets, bool showAnyElementAsQuestionMark, bool ignoreElementDistributionDetails, int truncationLength, bool escapeCharacters, bool useLazyQuantifier) { this.PutOptionalInSquareBrackets = putOptionalInSquareBrackets; this.ShowAnyElementAsQuestionMark = showAnyElementAsQuestionMark; this.IgnoreElementDistributionDetails = ignoreElementDistributionDetails; this.TruncationLength = truncationLength; this.EscapeCharacters = escapeCharacters; }
PVS-Studio Warning : O parâmetro do construtor V3117 'useLazyQuantifier' não é usado. Tempo de execução RegexpFormattingSettings.cs 38
O construtor não usa um parâmetro -
useLazyQuantifier . Isso parece particularmente suspeito no contexto do fato de que uma propriedade com o nome e o tipo correspondente é definida na classe -
UseLazyQuantifier . Aparentemente, eles esqueceram de inicializá-lo através do parâmetro correspondente.
Conheci vários manipuladores de eventos potencialmente perigosos. Um exemplo de um deles é dado abaixo:
public class RecommenderRun { .... public event EventHandler Started; .... public void Execute() {
PVS-Studio Warning :
V3083 Chamada não segura do evento 'Started', NullReferenceException é possível. Considere atribuir um evento a uma variável local antes de invocá-lo. Recomendador do avaliador
O fato é que, entre a verificação de desigualdade
nula e a chamada do manipulador, um evento pode ser cancelado e, se no momento entre a verificação de
nulo e a chamada dos manipuladores, o evento não tiver assinantes, uma
NullReferenceException será
lançada . Para evitar esses problemas, por exemplo, você pode salvar o link na cadeia de delegados em uma variável local ou usar o operador '?.' chamar manipuladores.
Além do snippet de código acima, havia 35 desses locais.
A propósito,
785 avisos do
V3024 também foram atendidos. O aviso
V3024 é emitido ao comparar números reais usando os operadores '! =' Or '=='. Não vou me debruçar aqui sobre por que essas comparações nem sempre são corretas - mais sobre isso está escrito na documentação, também há um link para o
StackOverflow (é isso).
Considerando que as fórmulas e cálculos foram frequentemente atendidos, esses avisos também podem ser importantes, embora sejam levados ao nível 3 (pois estão longe de ser relevantes em todos os projetos).
Se tiver certeza de que esses avisos são irrelevantes, é possível removê-los com
quase um clique , reduzindo o número total de operações do analisador.
Conclusão
De alguma forma, durante muito tempo eu não escrevi artigos sobre verificação de projetos, e tocar nesse processo novamente foi bastante agradável. Espero que você também tenha aprendido algo novo / útil neste artigo, ou pelo menos tenha lido com interesse.
Desejo aos desenvolvedores uma correção precoce das áreas problemáticas e lembro que cometer erros é normal, mas somos pessoas. Para isso, são necessárias ferramentas adicionais, como analisadores estáticos, para encontrar o que uma pessoa perdeu, certo? Enfim - boa sorte com o projeto, e obrigado pelo trabalho!
E lembre-se de que o benefício máximo de um analisador estático é alcançado com seu
uso regular .
Tudo de bom!
Se você deseja compartilhar este artigo com um público que fala inglês, use o link para a tradução: Sergey Vasiliev.
Quais erros ocultam no código Infer.NET?