Se ha convertido en una tradición para los desarrolladores recién empleados en el equipo de PVS-Studio comenzar escribiendo un artículo que revisa los errores encontrados por el analizador en algún proyecto de código abierto. Telerik UI para UWP es el proyecto elegido para la revisión de hoy.
Analizador de código PVS-Studio
PVS-Studio es una herramienta para detectar errores y vulnerabilidades potenciales en el código fuente de programas escritos en C, C ++, C # y Java. El analizador se ejecuta en Windows, Linux y macOS.
PVS-Studio se puede ejecutar de varias maneras:
- como un complemento para Visual Studio o IntelliJ IDEA localmente en las computadoras individuales de los desarrolladores;
- mediante la integración con SonarQube: la plataforma de inspección de calidad de código continuo;
- como una aplicación independiente para integrarse en un sistema de compilación;
- ejecutándose en combinación con una utilidad de monitoreo de compilación especial;
- al integrarse con Azure DevOps, Jenkins, TeamCity, Travis CI y otros sistemas similares;
- etc.
El proyecto bajo análisis.
Telerik UI para UWP es un conjunto de controles de UI para la Plataforma universal de Windows (UWP). El código fuente del proyecto está
disponible en Github . El conjunto incluye más de 20 componentes que permiten a los usuarios visualizar datos en forma de gráfico, crear listas y tablas, y usar un mapa para mostrar contenido en un contexto geográfico.
Fragmentos de código interesantes informados por el analizador
Mensaje de diagnóstico de PVS-Studio: V3013 Es extraño que el cuerpo de la función 'OnMinValuePropertyChanged' sea completamente equivalente al cuerpo de la función '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); } } }
Dos métodos,
OnMinValuePropertyChanged y
OnMaxValuePropertyChanged , realizan las mismas acciones. Sospecho fuertemente que hay un error aquí. Tenga en cuenta que ambos métodos llaman al mismo método,
RaiseMinimumPropertyChangedEvent , mientras que la clase
RadGaugeAutomationPeer implementa métodos individuales para "Mínimo" y "Máximo":
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); }
El método
RaiseMinimumPropertyChangedEvent se usa dos veces, mientras que el método
RaiseMaximumPropertyChangedEvent no se usa en absoluto. Esto me hace dudar que el método
OnMaxValuePropertyChanged funcione bien ... Supongo que estaba destinado a verse así:
private static void OnMaxValuePropertyChanged( DependencyObject sender, DependencyPropertyChangedEventArgs args) { .... peer.RaiseMaximumPropertyChangedEvent((double)args.OldValue, (double)args.NewValue); .... }
Pero incluso con esta solución, el código no se ve bien debido a los numerosos elementos duplicados. Es difícil de leer, y las líneas de aspecto similar atenúan su atención, lo que hace que la revisión de códigos sea un trabajo difícil. Las herramientas de análisis estático, por el contrario, pueden manejarlo fácilmente (lo que no significa que no deba refactorizar su código y especialmente eliminar líneas duplicadas).
Mirando este fragmento y el siguiente, sospecho que los autores del proyecto se entregan a copiar y pegar de vez en cuando. Bueno, todos lo hacemos ... :)
Mensaje de diagnóstico de PVS-Studio: V3001 Hay sub-expresiones idénticas 'element.RenderSize == emptySize' a la izquierda y a la derecha de '||' 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 operandos del '||' El operador en la expresión condicional de la instrucción
if está representado por subexpresiones idénticas. Obviamente, la segunda subexpresión debería ser diferente. Tal vez el segundo
RenderSize estaba destinado a ser
DesiredSize o tal vez la segunda subexpresión no debería estar allí en absoluto. En cualquier caso, este código necesita ser reparado.
Mensaje de diagnóstico de PVS-Studio: V3001 Hay
subexpresiones idénticas 'texto [0] ==' - '' a la izquierda y a la derecha de '||' 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; } .... }
El texto ingresado en el campo del cuadro de texto se lee en una variable. El primer carácter de la cadena se compara dos veces con el carácter '-', que no se ve bien. Obviamente, esta función no realiza la validación de texto según lo previsto.
Mensaje de diagnóstico de PVS-Studio: V3001 Hay
subexpresiones idénticas 'newValue.HasValue' a la izquierda y a la derecha del 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 expresiones condicionales,
newValue.HasValue y
newValue! = Null , devuelven
true si
newValue tiene un valor. El analizador señala esto, pero si este error debe solucionarse eliminando una de las subexpresiones o reemplazándola por otra (en caso de que haya algo más que verificar) es algo que solo los autores de este código pueden resolver.
Mensaje de diagnóstico de PVS-Studio: V3125 El objeto 'CurrentAttachedMenu' se usó después de que se verificó contra nulo. Líneas de verificación: 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(); } } }
Si la variable
CurrentAttachedMenu es igual a
nula , la evaluación de la expresión
CurrentAttachedMenu.IsOpen generará una excepción. Parece que es solo un error tipográfico y los desarrolladores realmente se referían a la operación opuesta, '! =', En lugar de la verificación nula, pero si ese es el caso, la condición de la instrucción
if arrojará una excepción si la variable
CurrentAttachedMenu es igual a
nulo .
Hubo
37 advertencias más de este tipo, algunas de las cuales aparentemente apuntan a errores genuinos. Pero son demasiadas advertencias para un artículo, así que las omitiré.
Mensaje de diagnóstico de PVS-Studio: V3019 Posiblemente, una variable incorrecta se compara con nula después de la conversión de tipo usando la palabra clave 'as'. Verifique las variables '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; } .... }
El programador debe haber confundido una variable por otra. La comprobación nula se realiza en la referencia de origen,
dragDropElement , en lugar de en la referencia resultante del reparto,
uiDragDropElement , que es el que realmente debía comprobarse. Esta suposición está respaldada por el hecho de que
uiDragDropElement se usa aún más sin ninguna comprobación nula.
Mensaje de diagnóstico de PVS-Studio: V3030 Verificación
periódica . La condición '! ShowIndicatorWhenNoData' ya se verificó en la línea 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; } .... }
Dos condiciones comprueban la misma variable
showIndicatorWhenNoData . La segunda comprobación puede ser redundante, pero también es posible que una de las subexpresiones duplicadas sea completamente diferente.
Mensaje de diagnóstico de PVS-Studio: V3031 Se puede simplificar una verificación excesiva. El '||' El operador está rodeado de expresiones opuestas. 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); } }
Técnicamente hablando, este fragmento es correcto; el analizador solo señala cierta redundancia en la condición. Pero tenga en cuenta que el código redundante a menudo es un signo de un error de programación, como verificar una variable más veces de lo necesario en lugar de otra variable.
La condición se puede simplificar un poco eliminando el código innecesario de la siguiente manera:
internal class SelectedItemCollection : ObservableCollection<object> { .... private bool CanInsertItem(object item) { return this.suspendLevel == 0 && this.AllowSelect && (this.AllowMultipleSelect || this.Count == 0); } }
Otras advertencias similares:
- V3031 Se puede simplificar una verificación excesiva. El '||' El operador está rodeado de expresiones opuestas. SelectedItemCollection.cs 93
- V3031 Se puede simplificar una verificación excesiva. El '||' El operador está rodeado de expresiones opuestas. StackVirtualizationStrategy.cs 49
- V3031 Se puede simplificar una verificación excesiva. El '||' El operador está rodeado de expresiones opuestas 'state == null' y 'state! = null'. LocalFieldDescriptionsProviderBase.cs 24
Consideremos otro código, al cual el analizador emitió lo siguiente:
Mensajes de diagnóstico de PVS-Studio:- V3137 La variable 'leftMargin' se asigna pero no se usa al final de la función. DragDrop.cs 87
- V3137 La variable 'topMargin' se asigna pero no se usa al final de la función. DragDrop.cs 88
internal static class DragDrop { .... double leftMargin = 0d; double topMargin = 0d; if (frameworkElementSource != null) { leftMargin = frameworkElementSource.Margin.Left;
Las variables
leftMargin y
topMargin áreas firmaron algunos valores pero nunca se usaron después de eso. No es necesariamente un error, pero un código como ese todavía parece sospechoso. Podría ser un signo de un error tipográfico o una mala refactorización.
Hubo otra advertencia de este tipo: V3137 La variable 'currentColumnLength' se asigna pero no se usa al final de la función. WrapLayout.cs 824
Mensaje de diagnóstico de PVS-Studio: V3061 El parámetro 'índice' siempre se reescribe en el cuerpo del método antes de usarse. 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,
El parámetro de
índice del método
FindGroupAndItemIndex se sobrescribe antes de su uso. Lo más probable es que esto indique un error del programador.
Mensaje de diagnóstico de PVS-Studio: V3083 Invocación insegura del evento 'Completado', NullReferenceException es posible. Considere asignar un evento a una variable local antes de invocarlo. ActionBase.cs 32
internal abstract class ActionBase { .... protected virtual void OnCompleted() { this.IsCompleted = true; if (this.Completed != null) { this.Completed(this, EventArgs.Empty); } } }
El controlador de eventos se llama de una manera potencialmente insegura, con el riesgo de generar una
NullReferenceException . Esto sucederá si al evento no le quedan suscriptores entre la verificación nula y la llamada del controlador de eventos.
El informe señala
49 problemas más de este tipo. No son muy interesantes para discutir aquí, y después de todo, los autores del proyecto pueden encontrarlos fácilmente con PVS-Studio por su cuenta, así que pasemos a los siguientes ejemplos.
Mensaje de diagnóstico de PVS-Studio: V3145 Desreferencia insegura de un objetivo WeakReference, considere inspeccionar la información. El objeto podría haber sido basura recolectada entre verificar 'IsAlive' y acceder a la propiedad 'Destino'. FadeAnimation.cs 84
public class RadFadeAnimation : RadAnimation { .... protected internal override void ApplyAnimationValues(PlayAnimationInfo info) { .... if (info.Target.Opacity != opacity)
Se puede
generar una
NullReferenceException al direccionar la propiedad
info.Target.Opacity . Para comprender mejor de qué se trata el problema, debemos echar un vistazo a ciertos bloques de la clase
PlayAnimationInfo , en particular la propiedad
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; } } .... }
En realidad, cuanto más profundice en este código, más problemas potenciales descubrirá. Echemos un vistazo al más interesante: el que activó la advertencia. El problema es que incluso si la ejecución sigue la rama
else de la instrucción
if , no garantiza que se devuelva una referencia no nula, incluso si no estamos teniendo en cuenta los efectos de la conversión de tipos (el constructor inicializa el objeto) .
¿Cómo es eso posible? Verá, si el objeto al que hace referencia
WeakReference se recolecta basura entre la verificación
IsAlive y la llamada a
Target ,
this.target.Target devolverá
nulo . Es decir, la verificación
IsAlive no garantiza que el objeto seguirá estando disponible la próxima vez que llame a
Target .
Por cierto, el
retorno nulo; otro problema detecta el problema: V3080 Posible desreferencia nula. Considere inspeccionar 'info.Target'. FadeAnimation.cs 84
Hubo algunos defectos más como ese:
- V3145 Desreferencia insegura de un objetivo WeakReference, considere inspeccionar el objetivo. El objeto podría haberse recolectado como basura antes de acceder a la propiedad 'Destino'. MoveXAnimation.cs 80
- V3145 Desreferencia insegura de un objetivo WeakReference, considere inspeccionar el objetivo. El objeto podría haberse recolectado como basura antes de acceder a la propiedad 'Destino'. MoveYAnimation.cs 80
- V3145 Desreferencia insegura de un objetivo WeakReference, considere inspeccionar la información. El objeto podría haberse recolectado como basura antes de acceder a la propiedad 'Destino'. PlaneProjectionAnimation.cs 244
- V3145 Desreferencia insegura de un objetivo WeakReference. El objeto podría haber sido basura recolectada entre verificar 'IsAlive' y acceder a la propiedad 'Destino'. WeakEventHandler.cs 109
Pasemos al siguiente ejemplo.
Mensaje de diagnóstico de PVS-Studio: V3066 Posible orden incorrecto de argumentos pasados al constructor 'NotifyCollectionChangedEventArgs': 'oldItem' y '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 descubrir el significado de esta advertencia, debemos mirar los parámetros del constructor
NotifyCollectionChangedEventArgs :
public NotifyCollectionChangedEventArgs( NotifyCollectionChangedAction action, object newItem, object oldItem, int index);
El analizador nos dice que las variables
oldItem y
newItem se intercambian en la siguiente expresión:
return new NotifyCollectionChangedEventArgs( action, oldItem, newItem, changeIndex);
Sin embargo, la implementación del constructor tiene esas variables enumeradas en el orden opuesto. Solo puede preguntarse si esto se hizo a propósito o no.
Mensaje de diagnóstico de PVS-Studio: V3102 Acceso sospechoso al elemento del objeto 'x' por un índice constante dentro de un bucle. 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]))
Los elementos
x [0] e
y [0] se comparan en cada iteración del bucle. Pero como solo se comparan los primeros elementos, el bucle no tiene sentido. Los desarrolladores probablemente intentaron comparar los elementos respectivos de las matrices en su lugar. En ese caso, la versión correcta se vería así:
for (int i = 0; i < x.Length; i++) { if (!object.Equals(x[i], y[i])) { return false; } }
Mensaje de diagnóstico de PVS-Studio: V3123 Quizás el operador '?:' Funciona de una manera diferente a la esperada. Su prioridad es inferior a la prioridad de otros operadores en su condición. EditRowHostPanel.cs 35
protected override Size MeasureOverride(Size availableSize) { .... bool shouldUpdateRowHeight = editorLine == 0 || displayedElement == null ? false : displayedElement.ContainerType != typeof(DataGridGroupHeader); .... }
Esta advertencia trata sobre el uso del operador '?:'. Su precedencia es menor que la de
! =, || y
== , lo que significa que el orden de evaluación de la expresión anterior puede ser diferente del esperado. Este caso particular parece ser un falso positivo, con el código realmente funcionando según lo previsto. Pero un código como ese es muy difícil de leer, y nunca puedes estar seguro de haberlo entendido correctamente. Parece que fue escrito de esa manera deliberadamente para que nadie pueda entenderlo :) La mejor manera de que sea más fácil de leer es usar paréntesis o una declaración
if .
Mensaje de diagnóstico de PVS-Studio: V3078 El orden de clasificación original se perderá después de una llamada repetitiva al método 'OrderBy'. Use el método 'ThenBy' para preservar la clasificación 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)); } }
Este error tiene que ver con una llamada recurrente del método
OrderBy en una colección de tipo
IOrderedEnumerable . La colección se ordena primero por columnas y luego por filas. El problema es que el resultado del primer ordenamiento, por columnas, no se almacena en ningún lugar y se perderá cuando comience el ordenamiento por filas. Si desea mantener el resultado del ordenamiento en columnas y ordenar múltiples criterios, use el método
ThenBy :
this.MergeCellSelectionRegions(selectedItemsInView .OrderBy(c => c.Column.ItemInfo.LayoutInfo.Line) .ThenBy(c => c.RowItemInfo.LayoutInfo.Line));
Mensaje de diagnóstico de PVS-Studio: V3008 La variable 'currentColumnLength' recibe valores dos veces consecutivas. Quizás esto sea un error. Líneas de verificación: 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;
Al analizador le pareció extraño que a la variable
currentColumnLength se le asigne un valor dos veces mientras no se usa en ningún lugar entre estas dos asignaciones. No importa la condición, la variable eventualmente terminará como
nula . Este código es defectuoso o redundante.
Mensaje de diagnóstico de PVS-Studio: V3127 Se
encontraron dos fragmentos de código similares. Tal vez, este es un error tipográfico y se debe usar la variable 'emptyIconContainer' en lugar de 'filledIconContainer' RadRatingItem.cs 240
public class RadRatingItem : RadContentControl { .... protected override void OnApplyTemplate() { .... this.filledIconContainer = this.GetTemplateChild( "FilledIconContainer") as Border; if (this.filledIconContainer == null)
Las dos condiciones idénticas anteriores aparecieron como resultado de un error tipográfico. La excepción lanzada por este código sugiere que la segunda condición debería verse así:
if (this.emptyIconContainer == null) { throw new MissingTemplatePartException( "EmptyIconContainer", typeof(Border)); }
Mensaje de diagnóstico de PVS-Studio: V3020 Un '
corte ' incondicional dentro de un bucle. 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; } } }
La declaración de
ruptura no es parte de la declaración
if . Se ejecutará sin importar el valor almacenado en
pair.IsFrozen , por lo que el bucle
foreach solo se repetirá una vez.
Eso es todo por mi revisión de errores encontrados en Telerik. Estamos listos para proporcionar a los desarrolladores una licencia temporal gratuita para que puedan hacer un análisis más completo y corregir los defectos. También pueden hacer uso de las
opciones de licencia gratuitas de PVS-Studio disponibles para desarrolladores de código abierto.
Conclusión
Aunque los autores de Telerik UI para UWP han hecho un gran trabajo al desarrollar su proyecto, todavía dejan que se introduzcan varios errores tipográficos, como suele ocurrir con nosotros :). Todos esos errores podrían haberse detectado y reparado fácilmente con un analizador estático, pero lo más importante que debe recordar sobre el análisis estático es que debe usarse
de la manera correcta y de forma regular .