Ya se ha convertido en una tradición que los programadores que reponen el equipo PVS-Studio comiencen su trabajo escribiendo un artículo sobre el análisis de un proyecto de código abierto. Esta vez, un proyecto tan probado será la interfaz de usuario de Telerik para UWP.
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. Se ejecuta en Windows, Linux y macOS.
El analizador proporciona varios escenarios de uso:
- puede usarse localmente en máquinas de desarrollo, integrándose como un complemento con Visual Studio o IntelliJ IDEA;
- puede integrarse con la plataforma de garantía de calidad continua SonarQube;
- se puede usar de forma independiente, integrándose en el sistema de ensamblaje;
- es posible usar la utilidad de monitoreo de compilación;
- es posible la integración con Azure DevOps, Jenkins, TeamCity, Travis CI y sistemas similares;
- Y así sucesivamente.
Proyecto auditado
Telerik UI para UWP es un conjunto de componentes de interfaz de usuario para la Plataforma universal de Windows (UWP). El código fuente del proyecto se puede
encontrar en Github . El conjunto incluye más de 20 componentes que le permiten visualizar datos en forma de gráficos, crear listas y tablas, usar un mapa para demostrar los datos que están asociados con una ubicación.
Fragmentos que llamaron la atención al estudiar el informe del analizador.
Advertencia 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); } } }
El analizador detectó dos métodos,
OnMinValuePropertyChanged y
OnMaxValuePropertyChanged , que realizan las mismas acciones. Tengo fuertes sospechas de que se ha introducido un error en este código. Tenga en cuenta que tanto el método
OnMinValuePropertyChanged como el método
OnMaxValuePropertyChanged usan
RaiseMinimumPropertyChangedEvent . Al mismo tiempo, en la clase
RadGaugeAutomationPeer puede encontrar un método para "Mínimo" y para "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
RaiseMaximumPropertyChangedEvent nunca se usa en el código, pero
RaiseMinimumPropertyChangedEvent se usa dos veces. Y, ya sabes, el rendimiento del método
OnMaxValuePropertyChanged plantea preguntas ... Creo que se suponía que debía escribir lo siguiente:
private static void OnMaxValuePropertyChanged( DependencyObject sender, DependencyPropertyChangedEventArgs args) { .... peer.RaiseMaximumPropertyChangedEvent((double)args.OldValue, (double)args.NewValue); .... }
Pero aun así, el código no se ve muy bien debido a la gran cantidad de elementos que se repiten. Es difícil de entender, y las líneas repetidas opacan la atención del programador, y se vuelve más difícil llevar a cabo la revisión del código en tales condiciones. Pero las herramientas de análisis estático hacen un excelente trabajo al verificar dicho código (pero esto no significa que pueda hacerlo sin refactorizar y especialmente sin reducir el número de líneas repetidas).
Del fragmento de código anterior y siguiente, podemos suponer que los autores no son reacios a copiar y pegar. Sin embargo, como todos nosotros ... :)
Advertencia de PVS-Studio :
V3001 Hay
subexpresiones 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); }
El analizador detectó un fragmento de código de error en el que a la derecha y a la izquierda del operador '||' la
instrucción if usa las mismas subexpresiones. La segunda subexpresión claramente debería haber parecido diferente. Quizás en lugar del segundo
RenderSize debería ser
DesiredSize . O no debería haber una segunda subexpresión en absoluto. En cualquier caso, este fragmento de código debe ser reparado.
Advertencia de PVS-Studio :
V3001 Hay
subexpresiones idénticas 'text [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; } .... }
Aquí, el desarrollador escribe el texto ingresado en el campo del cuadro de texto en una variable. Luego, el primer carácter de la cadena se compara dos veces con el mismo carácter '-', que es una decisión sospechosa. Obviamente, la validación de texto en esta función no funciona como se pretendía originalmente.
Advertencia 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)
La expresión
newValue.HasValue devuelve
verdadero si
newValue contiene algún valor, y la expresión
newValue! = Null hace lo mismo. El analizador presta atención a esto, y lo que debe hacer es eliminar una de las subexpresiones o reemplazarla por otra (si se hubiera verificado algo más), corresponde a los desarrolladores decidir.
Advertencia 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 resulta ser
nula , entonces evaluar la expresión
CurrentAttachedMenu.IsOpen generará una excepción. A primera vista, parece que este es un error tipográfico simple y no significó una comparación con
nulo , sino la operación inversa - '! ='. Pero luego se producirá una excepción en la condición de la
instrucción if si la variable
CurrentAttachedMenu es
nula .
Además en el código había otras
37 de las mismas advertencias, algunas de las cuales parecen indicar errores. Pero describirlos en el marco de un artículo sigue siendo demasiado, por lo que los dejaré desatendidos.
Advertencia 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; } .... }
Es muy probable que el autor mezcle las variables. La desigualdad
nula se verifica no por el enlace obtenido como resultado de la
conversión , sino por el original (
dragDropElement ). Lo más probable
es que se suponía que el enlace
uiDragDropElement estaba
marcado . La conjetura también se confirma por el hecho de que el programador usó
uiDragDropElement sin verificar
nulo .
Advertencia 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; } .... }
El analizador encontró un fragmento de código en el que, bajo dos condiciones, se vuelve a verificar la misma variable
showIndicatorWhenNoData . Quizás la verificación sea simplemente redundante, pero también existe la posibilidad de que una de las subexpresiones duplicadas sea diferente.
Advertencia 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); } }
Este fragmento de código no es formalmente erróneo. El analizador sugiere alguna redundancia de código en la condición. Sin embargo, vale la pena recordar que el código adicional a veces es el resultado de un error, por ejemplo, cuando en lugar de una variable, otra se verifica varias veces.
Puede reducir esta condición un poco y eliminar el código adicional. Por ejemplo, así:
internal class SelectedItemCollection : ObservableCollection<object> { .... private bool CanInsertItem(object item) { return this.suspendLevel == 0 && this.AllowSelect && (this.AllowMultipleSelect || this.Count == 0); } }
Otros mensajes 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
Considere otra pieza de código donde el analizador devolvió lo siguiente:
Advertencias 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;
El analizador informa que a las
variables leftMargin y
topMargin se les han asignado valores, pero después de eso estas variables no se usan hasta el final del método. Probablemente no haya ningún error aquí, pero dicho código parece sospechoso. Esto puede deberse a un error tipográfico o una refactorización fallida.
El mismo problema se encontró en otro lugar: V3137 La variable 'currentColumnLength' se asigna pero no se usa al final de la función. WrapLayout.cs 824
Advertencia de PVS-Studio : el parámetro 'índice'
V3061 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 usarse. Lo más probable es que esto indique un error del programador.
Advertencia de PVS-Studio :
V3083 Invocación insegura del evento 'Completado', es posible NullReferenceException. 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 programador permitió una llamada potencialmente insegura al controlador de eventos en este método, lo que puede conducir a una excepción de tipo
NullReferenceException . Se lanzará una excepción siempre que entre la verificación
nula y la llamada de los controladores de eventos, este evento no permanezca.
Hay otros
49 problemas similares en el código. No será interesante verlos todos en este artículo, y los autores podrán encontrarlos fácilmente por su cuenta utilizando PVS-Studio, por lo que pasaremos a otros errores.
Advertencia 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)
El analizador advierte sobre el peligro de una excepción de tipo
NullReferenceException al acceder a la propiedad
info.Target.Opacity . Para comprender mejor la esencia del problema, debe buscar fragmentos 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 general, cuanto más cava este código, más problemas potenciales puede encontrar en él. Veamos quizás la más interesante: aquella a la que el analizador emitió una advertencia. El hecho es que incluso si la ejecución sigue la rama de la
instrucción if , esto no garantiza que se devolverá una referencia distinta de cero. Independientemente de las conversaciones sobre el elenco, aquí consideramos que todo está permitido debido a la inicialización del objeto en el constructor.
¿Cómo es esto posible? El hecho es que si entre la verificación
IsAlive y la llamada
Target se realiza la recolección de basura, bajo la cual
cae el objeto al que hace referencia
WeakReference ,
this.target.Target devolverá
nulo . Es decir, la verificación
IsAlive no garantiza que la próxima vez que se acceda al
Destino , el objeto todavía esté disponible.
Por cierto, la situación es
nula; detecta otra regla de diagnóstico: V3080 Posible desreferencia nula. Considere inspeccionar 'info.Target'. FadeAnimation.cs 84
Problemas similares en el código ocurrieron varias veces más:
- 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.
Advertencia 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 comprender lo que significa esta advertencia del analizador, vale la pena mirar los
parámetros del constructor
NotifyCollectionChangedEventArgs :
public NotifyCollectionChangedEventArgs( NotifyCollectionChangedAction action, object newItem, object oldItem, int index);
El analizador advierte que en la expresión
return new NotifyCollectionChangedEventArgs( action, oldItem, newItem, changeIndex);
intercambiaron las variables
oldItem y
newItem . En la definición del constructor, se enumeran en un orden diferente. Si esto se hizo conscientemente o no, uno solo puede adivinar.
Advertencia 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]))
En cada iteración en el bucle, el programador compara
x [0] e
y [0]. Sin embargo, el bucle no tiene sentido en este código, ya que solo se comparan los primeros elementos. Lo más probable es que esto signifique una comparación de los elementos correspondientes de las matrices. Entonces el código correcto será así:
for (int i = 0; i < x.Length; i++) { if (!object.Equals(x[i], y[i])) { return false; } }
Advertencia 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); .... }
Una advertencia está asociada con el uso del operador '?:'. Tiene una prioridad menor que
! =, ||, ==. Por lo tanto, una expresión puede no ser evaluada según lo planeado por el programador. Aparentemente, en este caso este es un falso positivo y el código funciona correctamente. Pero leer dicho código es muy difícil y nunca hay certeza de que se entienda correctamente. Parece que el desarrollador escribió de tal manera que nadie entendió nada :) La mejor manera de hacerlo es más legible: use corchetes o la
declaración if .
Advertencia 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)); } }
El error está relacionado con llamar nuevamente a
OrderBy para una colección de tipo
IOrderedEnumerable . Aquí la colección se ordena primero por columnas, luego por filas. Además, en el momento de la clasificación por filas, la clasificación anterior por columnas no se tiene en cuenta en ninguna parte. Para no perder la clasificación por columnas y clasificar la colección por varios criterios a la vez, use
ThenBy :
this.MergeCellSelectionRegions(selectedItemsInView .OrderBy(c => c.Column.ItemInfo.LayoutInfo.Line) .ThenBy(c => c.RowItemInfo.LayoutInfo.Line));
Advertencia de PVS-Studio :
V3008 A la variable 'currentColumnLength' se le asignan valores dos veces seguidas. 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ó sospechoso que la variable
currentColumnLength tuviera asignado un valor dos veces. La variable no se usa entre asignaciones. Independientemente de la condición, esta variable finalmente será cero. Este código es incorrecto o redundante.
Advertencia 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)
Debido a un error tipográfico, aparecieron dos condiciones idénticas en el código. A juzgar por la excepción generada, la segunda condición debería verse así:
if (this.emptyIconContainer == null) { throw new MissingTemplatePartException( "EmptyIconContainer", typeof(Border)); }
Advertencia de PVS-Studio :
V3020 Una 'interrupción' 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; } } }
El analizador encontró que aquí la declaración de
ruptura no pertenece a la
declaración if .
El corte se ejecutará independientemente del valor de
pair.IsFrozen y debido a esto en
foreach , solo se realizará una iteración.
Eso concluye mi consideración de las advertencias. Para que los desarrolladores de Telerik puedan realizar un análisis de código más completo y corregir defectos, estamos listos para proporcionarles una licencia temporal. Además, pueden aprovechar el
uso gratuito de la opción
PVS-Studio proporcionada a los autores de proyectos de código abierto.
Conclusión
Aunque los desarrolladores de Telerik UI para UWP hicieron un gran trabajo, no estuvo exento de errores tipográficos, como suele suceder :). Todos estos errores podrían ser fácilmente encontrados por un analizador estático y corregidos. Lo más importante es usar el analizador
correcta y regularmente .
Si desea compartir este artículo con una audiencia de habla inglesa, utilice el enlace a la traducción: Ekaterina Nikiforova.
Verificación de la interfaz de usuario de Telerik para UWP como una forma de comenzar con PVS-Studio .