Tornou-se uma tradição para desenvolvedores recém-empregados da equipe PVS-Studio escrever um artigo revisando os erros encontrados pelo analisador em algum projeto de código aberto. A interface do usuário Telerik para UWP é o projeto escolhido para a revisão de hoje.
Analisador de código PVS-Studio
O PVS-Studio é uma ferramenta para detectar bugs e possíveis vulnerabilidades no código fonte de programas escritos em C, C ++, C # e Java. O analisador é executado no Windows, Linux e macOS.
O PVS-Studio pode ser executado de várias maneiras:
- como um plug-in para o Visual Studio ou IntelliJ IDEA localmente nos computadores individuais dos desenvolvedores;
- integrando ao SonarQube: a plataforma de inspeção contínua da qualidade do código;
- como um aplicativo independente para integração em um sistema de compilação;
- executando em combinação com um utilitário especial de monitoramento de compilação;
- integrando-se ao Azure DevOps, Jenkins, TeamCity, Travis CI e outros sistemas similares;
- etc.
O projeto em análise
A interface do usuário Telerik para UWP é um conjunto de controles de interface do usuário para a Plataforma Universal do Windows (UWP). O código fonte do projeto está
disponível no Github . O conjunto inclui mais de 20 componentes, permitindo aos usuários visualizar dados em forma de gráfico, criar listas e tabelas e usar um mapa para exibir o conteúdo em um contexto geográfico.
Fragmentos de código interessantes relatados pelo analisador
Mensagem de diagnóstico do PVS-Studio: V3013 É estranho que o corpo da função 'OnMinValuePropertyChanged' seja totalmente equivalente ao corpo da função 'OnMaxValuePropertyChanged'. RadGauge.cs 446
private static void OnMinValuePropertyChanged( DependencyObject sender, DependencyPropertyChangedEventArgs args) { double newVal = (double)args.NewValue; ValidateValue(newVal); RadGauge gauge = sender as RadGauge; if (gauge.panel != null) { gauge.panel.UpdateOnMinMaxValueChange(); } if(AutomationPeer.ListenerExists(AutomationEvents.PropertyChanged)) { var peer = FrameworkElementAutomationPeer.FromElement(gauge) as RadGaugeAutomationPeer; if (peer != null) { peer.RaiseMinimumPropertyChangedEvent((double)args.OldValue, (double)args.NewValue); } } } private static void OnMaxValuePropertyChanged( DependencyObject sender, DependencyPropertyChangedEventArgs args) { double newVal = (double)args.NewValue; ValidateValue(newVal); RadGauge gauge = sender as RadGauge; if (gauge.panel != null) { gauge.panel.UpdateOnMinMaxValueChange(); } if (AutomationPeer.ListenerExists(AutomationEvents.PropertyChanged)) { var peer = FrameworkElementAutomationPeer.FromElement(gauge) as RadGaugeAutomationPeer; if (peer != null) { peer.RaiseMinimumPropertyChangedEvent((double)args.OldValue, (double)args.NewValue); } } }
Dois métodos,
OnMinValuePropertyChanged e
OnMaxValuePropertyChanged , executam as mesmas ações. Eu suspeito fortemente que há um bug aqui. Observe que os dois métodos chamam o mesmo método,
RaiseMinimumPropertyChangedEvent , enquanto a classe
RadGaugeAutomationPeer implementa métodos individuais para "Minimum" e "Maximum":
internal void RaiseMaximumPropertyChangedEvent(double oldValue, double newValue) { this.RaisePropertyChangedEvent( RangeValuePatternIdentifiers.MaximumProperty, oldValue, newValue); } internal void RaiseMinimumPropertyChangedEvent(double oldValue, double newValue) { this.RaisePropertyChangedEvent( RangeValuePatternIdentifiers.MinimumProperty, oldValue, newValue); }
O método
RaiseMinimumPropertyChangedEvent é usado duas vezes, enquanto o método
RaiseMaximumPropertyChangedEvent não é usado. Isso me faz duvidar que o método
OnMaxValuePropertyChanged funcione bem ... Eu acho que deveria ter a seguinte aparência:
private static void OnMaxValuePropertyChanged( DependencyObject sender, DependencyPropertyChangedEventArgs args) { .... peer.RaiseMaximumPropertyChangedEvent((double)args.OldValue, (double)args.NewValue); .... }
Mas mesmo com essa correção, o código não parece limpo por causa dos inúmeros elementos duplicados. É difícil de ler, e as linhas de aparência semelhante chamam sua atenção, o que torna a revisão de código um trabalho difícil. As ferramentas de análise estática, pelo contrário, podem lidar com isso facilmente (o que não significa que você não deve refatorar seu código e, principalmente, eliminar linhas duplicadas).
Olhando para este fragmento e o próximo, suspeito que os autores do projeto se entregam a copiar e colar de vez em quando. Bem, todos nós fazemos ... :)
Mensagem de diagnóstico do PVS-Studio: V3001 Existem
subexpressões idênticas 'element.RenderSize == emptySize' à esquerda e à direita da '||' operador. TiltInteractionEffect.cs 181
private static bool IsPointInElementBounds(FrameworkElement element, Point position) { Size emptySize = new Size(0, 0); if (element.RenderSize == emptySize || element.RenderSize == emptySize) { return false; } return new Rect(....).Contains(position); }
Ambos os operandos do '||' O operador na expressão condicional da instrução
if é representado por subexpressões idênticas. Obviamente, a segunda subexpressão deve ser diferente. Talvez o segundo
RenderSize fosse
DesiredSize ou talvez a segunda subexpressão não devesse estar lá. De qualquer forma, esse código precisa ser corrigido.
Mensagem de diagnóstico do PVS-Studio: V3001 Existem sub-expressões idênticas 'text [0] ==' - '' à esquerda e à direita do '||' operador. RadNumericBox.cs 1057
private void ValidateText() { string text = this.textBox.Text; .... if (text.Length == 1 && (text[0] == '-' || text[0] == '-')) { if (this.isNegative) { this.isNegative = false; } else { this.SetText(string.Empty); } return; } .... }
O texto inserido no campo da caixa de texto é lido em uma variável. O primeiro caractere da string é comparado duas vezes com o caractere '-', que não parece correto. Obviamente, essa função não executa a validação de texto conforme o esperado.
Mensagem de diagnóstico do PVS-Studio: V3001 Existem sub-expressões idênticas 'newValue.HasValue' à esquerda e à direita do operador '&&'. DateTimePicker.cs 576
private static void OnValueChanged(object sender, DependencyPropertyChangedEventArgs args) { DateTimePicker picker = sender as DateTimePicker; var newValue = (DateTime?)args.NewValue; if (newValue.HasValue && newValue != null)
Ambas as expressões condicionais,
newValue.HasValue e
newValue! = Null , retornam
true se
newValue tiver um valor. O analisador aponta isso, mas se esse bug deve ser corrigido removendo uma das subexpressões ou substituindo-a por outra (no caso de haver outra coisa a ser verificada) é algo que apenas os autores desse código podem descobrir.
Mensagem de diagnóstico do PVS-Studio: V3125 O objeto '
CurrentAttachedMenu ' foi usado após a verificação contra nulo. Verifique as linhas: 98, 96. PopupService.cs 98
internal static class PopupService { .... private static void Overlay_PointerPressed(....) { if (CurrentAttachedMenu == null || !CurrentAttachedMenu.hitTestService. HitTest(e.GetCurrentPoint(CurrentAttachedMenu).Position).Any()) { CurrentAttachedMenu.IsOpen = false; HideOverlay(); } } }
Se a variável
CurrentAttachedMenu for igual a
nula , avaliar a expressão
CurrentAttachedMenu.IsOpen resultará no aumento de uma exceção. Parece que é apenas um erro de digitação e os desenvolvedores realmente significaram a operação oposta, '! =', Em vez da verificação nula, mas se for esse o caso, a condição da instrução
if gerará uma exceção se a variável
CurrentAttachedMenu for igual a
nulo .
Havia mais
37 avisos desse tipo, alguns dos quais aparentemente apontam para erros genuínos. Mas existem avisos demais para um artigo, então vou ignorá-los.
Mensagem de diagnóstico do PVS-Studio: V3019 Possivelmente uma variável incorreta é comparada a nula após a conversão do tipo usando a palavra-chave 'as'. Verifique as variáveis 'dragDropElement', 'uiDragDropElement'. DragDrop.cs 91
internal static void StartDrag(....) { var dragDropElement = sender as IDragDropElement; .... UIElement uiDragDropElement = dragDropElement as UIElement; .... if (dragDropElement == null || !dragDropElement.CanStartDrag(trigger, initializeContext)) { return; } .... }
O programador deve ter confundido uma variável para outra. A verificação nula é feita na referência de origem,
dragDropElement , em vez da referência resultante da
conversão ,
uiDragDropElement , que é a que realmente deveria ser verificada. Essa suposição é suportada pelo fato de que
uiDragDropElement é usado ainda mais sem nenhuma verificação nula.
Mensagem de diagnóstico do PVS-Studio: V3030 Verificação recorrente. A condição '! ShowIndicatorWhenNoData' já foi verificada na linha 139. RadDataBoundListBox.PullToRefresh.cs 141
internal void HandlePullToRefreshItemStateChanged(object item, ItemState state) { .... bool showIndicatorWhenNoData = this.ShowPullToRefreshWhenNoData; if (this.realizedItems.Count == 0 && !showIndicatorWhenNoData) { if (state == ItemState.Recycled && !showIndicatorWhenNoData) { this.StopPullToRefreshLoading(false); this.HidePullToRefreshIndicator(); } return; } .... }
Duas condições verificam a mesma variável
showIndicatorWhenNoData . A segunda verificação pode ser redundante, mas também é possível que uma das subexpressões duplicadas seja totalmente outra coisa.
Mensagem de diagnóstico do PVS-Studio: V3031 Uma verificação excessiva pode ser simplificada. O '||' O operador é cercado por expressões opostas. SelectedItemCollection.cs 77
internal class SelectedItemCollection : ObservableCollection<object> { .... private bool CanInsertItem(object item) { return this.suspendLevel == 0 && this.AllowSelect && ((!this.AllowMultipleSelect && this.Count == 0) || this.AllowMultipleSelect); } }
Tecnicamente falando, esse trecho está correto; o analisador apenas aponta certa redundância na condição. Mas lembre-se de que código redundante geralmente é um sinal de erro de programação, como verificar uma variável mais vezes do que o necessário, em vez de alguma outra variável.
A condição pode ser simplificada um pouco, removendo o código desnecessário da seguinte maneira:
internal class SelectedItemCollection : ObservableCollection<object> { .... private bool CanInsertItem(object item) { return this.suspendLevel == 0 && this.AllowSelect && (this.AllowMultipleSelect || this.Count == 0); } }
Outros avisos semelhantes:
- V3031 Uma verificação excessiva pode ser simplificada. O '||' O operador é cercado por expressões opostas. SelectedItemCollection.cs 93
- V3031 Uma verificação excessiva pode ser simplificada. O '||' O operador é cercado por expressões opostas. StackVirtualizationStrategy.cs 49
- V3031 Uma verificação excessiva pode ser simplificada. O '||' O operador está cercado pelas expressões opostas 'state == null' e 'state! = null'. LocalFieldDescriptionsProviderBase.cs 24
Vamos considerar outro trecho de código, para o qual o analisador emitiu o seguinte:
Mensagens de diagnóstico do PVS-Studio:- V3137 A variável 'leftMargin' é designada, mas não é usada no final da função. DragDrop.cs 87
- V3137 A variável 'topMargin' é designada, mas não é usada no final da função. DragDrop.cs 88
internal static class DragDrop { .... double leftMargin = 0d; double topMargin = 0d; if (frameworkElementSource != null) { leftMargin = frameworkElementSource.Margin.Left;
As variáveis áreas
leftMargin e
topMargin designaram alguns valores, mas nunca foram usadas depois disso. Não é necessariamente um bug, mas um código como esse ainda parece suspeito. Pode ser um sinal de erro de digitação ou refatoração incorreta.
Houve outro aviso desse tipo: V3137 A variável 'currentColumnLength' é atribuída, mas não é usada no final da função. WrapLayout.cs 824
Mensagem de diagnóstico do PVS-Studio: V3061 O parâmetro 'index' é sempre reescrito no corpo do método antes de ser utilizado. DataEngine.cs 1443
private static Tuple<Group, int> FindGroupAndItemIndex(.... int index, ....) { if (exhaustiveSearch) { .... } else { var aggregateRowGroup = rowRootGroup; var rowGroupNames = valueProvider.GetRowGroupNames(item); foreach (var groupName in rowGroupNames) { Group group; if (aggregateRowGroup.TryGetGroup(groupName, out group)) { aggregateRowGroup = group; } } index = aggregateRowGroup.IndexOf(item,
O parâmetro
index do método
FindGroupAndItemIndex é substituído antes do uso. Provavelmente, isso indica um erro do programador.
Mensagem de diagnóstico do PVS-Studio: V3083 Chamada não segura do evento 'Concluído', NullReferenceException é possível. Considere atribuir um evento a uma variável local antes de invocá-lo. ActionBase.cs 32
internal abstract class ActionBase { .... protected virtual void OnCompleted() { this.IsCompleted = true; if (this.Completed != null) { this.Completed(this, EventArgs.Empty); } } }
O manipulador de eventos é chamado de uma maneira potencialmente insegura, correndo o risco de gerar uma
NullReferenceException . Isso acontecerá se o evento não tiver mais assinantes entre a verificação nula e a chamada do manipulador de eventos.
O relatório aponta mais
49 problemas desse tipo. Eles não são muito interessantes para discutir aqui e, afinal, os autores do projeto podem encontrá-los facilmente com o PVS-Studio por conta própria, então vamos pular para os próximos exemplos.
Mensagem de diagnóstico do PVS-Studio: V3145 Desreferência não segura de um destino WeakReference, considere inspecionar info.Target. O objeto pode ter sido coletado como lixo entre a verificação de 'IsAlive' e o acesso à propriedade 'Target'. FadeAnimation.cs 84
public class RadFadeAnimation : RadAnimation { .... protected internal override void ApplyAnimationValues(PlayAnimationInfo info) { .... if (info.Target.Opacity != opacity)
Uma
NullReferenceException pode ser gerada ao endereçar a propriedade
info.Target.Opacity . Para entender melhor o problema, precisamos dar uma olhada em certos blocos da classe
PlayAnimationInfo , particularmente na propriedade
Target .
public class PlayAnimationInfo { .... private WeakReference target; .... public PlayAnimationInfo(Storyboard storyboard, RadAnimation animation, UIElement target) { .... this.target = new WeakReference(target); .... } .... public UIElement Target { get { if (this.target.IsAlive) { return this.target.Target as UIElement; } return null; } } .... }
Na verdade, quanto mais você se aprofundar nesse código, mais problemas potenciais você descobrirá. Vamos dar uma olhada no mais interessante - aquele que acionou o aviso. O problema é que, mesmo que a execução siga o ramo
else da instrução
if , não garante o retorno de uma referência não nula, mesmo se não levarmos em consideração os efeitos da conversão de tipo (o objeto é inicializado pelo construtor) .
Como isso é possível? Veja bem, se o objeto referido por
WeakReference for coletado como lixo entre a verificação
IsAlive e a chamada para
Target ,
this.target.Target retornará
nulo . Ou seja, a verificação
IsAlive não garante que o objeto ainda esteja disponível na próxima vez que você chamar
Target .
A propósito, o
retorno nulo; O problema foi detectado por outro diagnóstico: V3080 Possível desreferência nula. Considere inspecionar 'info.Target'. FadeAnimation.cs 84
Havia mais alguns defeitos assim:
- V3145 Desreferenciação insegura de um alvo WeakReference, considere inspecionar o alvo. O objeto poderia ter sido coletado como lixo antes do acesso à propriedade 'Target'. MoveXAnimation.cs 80
- V3145 Desreferenciação insegura de um alvo WeakReference, considere inspecionar o alvo. O objeto poderia ter sido coletado como lixo antes do acesso à propriedade 'Target'. MoveYAnimation.cs 80
- V3145 Desreferenciação insegura de um destino WeakReference, considere inspecionar info.Target. O objeto poderia ter sido coletado como lixo antes do acesso à propriedade 'Target'. PlaneProjectionAnimation.cs 244
- V3145 Desreferenciação insegura de um destino WeakReference. O objeto pode ter sido coletado como lixo entre a verificação de 'IsAlive' e o acesso à propriedade 'Target'. WeakEventHandler.cs 109
Vamos para o próximo exemplo.
Mensagem de diagnóstico do PVS-Studio: V3066 Possível ordem incorreta de argumentos transmitida ao construtor 'NotifyCollectionChangedEventArgs': 'oldItem' e 'newItem'. CheckedItemsCollection.cs 470
public class CheckedItemsCollection<T> : IList<T>, INotifyCollectionChanged { .... private NotifyCollectionChangedEventArgs GenerateArgs(....) { switch (action) { case NotifyCollectionChangedAction.Add: .... case NotifyCollectionChangedAction.Remove: .... case NotifyCollectionChangedAction.Replace: return new NotifyCollectionChangedEventArgs( action, oldItem, newItem, changeIndex);
Para descobrir o significado desse aviso, precisamos examinar os parâmetros do construtor
NotifyCollectionChangedEventArgs :
public NotifyCollectionChangedEventArgs( NotifyCollectionChangedAction action, object newItem, object oldItem, int index);
O analisador nos diz que as variáveis
oldItem e
newItem são trocadas na seguinte expressão:
return new NotifyCollectionChangedEventArgs( action, oldItem, newItem, changeIndex);
No entanto, a implementação do construtor tem essas variáveis listadas na ordem oposta. Você só pode se perguntar se isso foi feito de propósito ou não.
Mensagem de diagnóstico do PVS-Studio: V3102 Acesso suspeito ao elemento do objeto 'x' por um índice constante dentro de um loop. DataEngine.cs 1718
private class ObjectArrayComparer : IEqualityComparer<object[]> { public bool Equals(object[] x, object[] y) { .... for (int i = 0; i < x.Length; i++) { if (!object.Equals(x[0], y[0]))
Os elementos
x [0] e
y [0] são comparados a cada iteração do loop. Mas como apenas os primeiros elementos são comparados, o loop não faz sentido. Os desenvolvedores provavelmente pretendiam comparar os respectivos elementos das matrizes. Nesse caso, a versão correta ficaria assim:
for (int i = 0; i < x.Length; i++) { if (!object.Equals(x[i], y[i])) { return false; } }
Mensagem de diagnóstico do PVS-Studio: V3123 Talvez o operador '?:'
Funcione de maneira diferente da esperada. Sua prioridade é menor que a prioridade de outros operadores em sua condição. EditRowHostPanel.cs 35
protected override Size MeasureOverride(Size availableSize) { .... bool shouldUpdateRowHeight = editorLine == 0 || displayedElement == null ? false : displayedElement.ContainerType != typeof(DataGridGroupHeader); .... }
Este aviso trata do uso do operador '?:'. Sua precedência é menor que a de
! =, || e
== , o que significa que a ordem de avaliação da expressão acima pode ser diferente da esperada. Esse caso em particular parece ser um falso positivo, com o código realmente funcionando como pretendido. Mas um código como esse é muito difícil de ler e você nunca pode ter certeza se o entendeu corretamente. Parece que foi escrito dessa maneira deliberadamente, para que ninguém pudesse descobrir :) A melhor maneira de facilitar a leitura é usar parênteses ou uma declaração
if .
Mensagem de diagnóstico do PVS-Studio: V3078 A ordem de classificação original será perdida após a chamada repetida para o método 'OrderBy'. Use o método 'ThenBy' para preservar a classificação original. GridModel.Selection.cs 107
internal partial class GridModel { private void BuildCellSelectionRegions(....) { .... this.MergeCellSelectionRegions(selectedItemsInView .OrderBy(c => c.Column.ItemInfo.LayoutInfo.Line) .OrderBy(c => c.RowItemInfo.LayoutInfo.Line)); } }
Esse bug tem a ver com uma chamada recorrente do método
OrderBy em uma coleção do tipo
IOrderedEnumerable . A coleção é classificada primeiro por colunas e depois por linhas. O problema é que o resultado da primeira classificação - por colunas - não é armazenado em nenhum lugar e será perdido quando a classificação por linhas for iniciada. Se você deseja manter o resultado da classificação em colunas e fazer a classificação com vários critérios, use o método
ThenBy :
this.MergeCellSelectionRegions(selectedItemsInView .OrderBy(c => c.Column.ItemInfo.LayoutInfo.Line) .ThenBy(c => c.RowItemInfo.LayoutInfo.Line));
Mensagem de diagnóstico do PVS-Studio: V3008 A variável 'currentColumnLength' recebe valores duas vezes sucessivamente. Talvez isso seja um erro. Verifique as linhas: 791, 785. WrapLayout.cs 791
private void OnAvailableLengthChanged(double oldValue, double newValue) { .... if (....) { if (currentColumnLength > 0) { var paddingValue = Math.Max(0, newValue - currentColumnLength); this.paddingRenderInfo.Add(paddingValue); currentColumnLength = 0;
O analisador achou estranho que a variável
currentColumnLength receba um valor duas vezes, mas não fosse usada em nenhum lugar entre essas duas atribuições. Independentemente da condição, a variável acabará sendo
nula . Este código está com defeito ou redundante.
Mensagem de diagnóstico do PVS-Studio: V3127 Dois fragmentos de código semelhantes foram encontrados. Talvez esse seja um erro de digitação e a variável 'emptyIconContainer' deva ser usada em vez de 'preenchidoIconContainer' RadRatingItem.cs 234
public class RadRatingItem : RadContentControl { .... protected override void OnApplyTemplate() { .... this.filledIconContainer = this.GetTemplateChild( "FilledIconContainer") as Border; if (this.filledIconContainer == null)
As duas condições idênticas acima apareceram como resultado de um erro de digitação. A exceção lançada por esse código sugere que a segunda condição deve ser assim:
if (this.emptyIconContainer == null) { throw new MissingTemplatePartException( "EmptyIconContainer", typeof(Border)); }
Mensagem de diagnóstico do PVS-Studio: V3020 Uma 'quebra' incondicional dentro de um loop. NodePool.cs 189
public IEnumerable<KeyValuePair<int, List<T>>> GetUnfrozenDisplayedElements() { foreach (var item in this.generatedContainers) { foreach (var pair in item.Value) { if (!pair.IsFrozen) { yield return item; } break; } } }
A instrução
break não faz parte da instrução
if . Ele será executado independentemente do valor armazenado em
pair.IsFrozen , portanto o loop
foreach repetirá apenas uma vez.
Isso é tudo para a minha análise de bugs encontrados em Telerik. Estamos prontos para fornecer aos desenvolvedores uma licença temporária gratuita, para que eles possam fazer uma análise mais completa e corrigir os defeitos. Eles também podem fazer uso das
opções de licenciamento gratuitas do PVS-Studio disponíveis para desenvolvedores de código aberto.
Conclusão
Embora os autores da Telerik UI para UWP tenham feito um grande trabalho no desenvolvimento de seu projeto, eles ainda deixam vários erros de digitação surgirem, como normalmente acontece conosco :). Todos esses bugs poderiam ter sido facilmente capturados e corrigidos usando um analisador estático, mas o ponto crucial a ser lembrado sobre a análise estática é que ela deve ser usada
da maneira correta e regularmente .