Il est déjà devenu une tradition que les programmeurs reconstituant l'équipe PVS-Studio commencent leur travail en écrivant un article sur l'analyse d'un projet open source. Cette fois, un tel projet éprouvé sera Telerik UI pour UWP.
Analyseur de code PVS-Studio
PVS-Studio est un outil pour détecter les erreurs et les vulnérabilités potentielles dans le code source des programmes écrits en C, C ++, C # et Java. Fonctionne sur Windows, Linux et macOS.
L'analyseur propose différents scénarios d'utilisation:
- peut être utilisé localement sur des machines de développement, s'intégrant en tant que plug-in avec Visual Studio ou IntelliJ IDEA;
- peut s'intégrer à la plate-forme d'assurance qualité continue SonarQube;
- peut être utilisé indépendamment, s'intégrant dans le système d'assemblage;
- il est possible d'utiliser l'utilitaire de surveillance de compilation;
- l'intégration avec Azure DevOps, Jenkins, TeamCity, Travis CI et des systèmes similaires est possible;
- et ainsi de suite.
Projet audité
Telerik UI for UWP est un ensemble de composants d'interface utilisateur pour la plateforme Windows universelle (UWP). Le code source du projet se
trouve sur Github . L'ensemble comprend plus de 20 composants qui vous permettent de visualiser des données sous forme de graphiques, de créer des listes et des tableaux, d'utiliser une carte pour afficher les données associées à un emplacement.
Fragments qui ont attiré l'attention lors de l'étude du rapport de l'analyseur
Avertissement PVS-Studio :
V3013 Il est étrange que le corps de la fonction 'OnMinValuePropertyChanged' soit entièrement équivalent au corps de la fonction '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); } } }
L'analyseur a détecté deux méthodes,
OnMinValuePropertyChanged et
OnMaxValuePropertyChanged , qui effectuent les mêmes actions. Je soupçonne fortement qu'une erreur s'est glissée dans ce code. Notez que la méthode
OnMinValuePropertyChanged et la méthode
OnMaxValuePropertyChanged utilisent
RaiseMinimumPropertyChangedEvent . Dans le même temps, dans la classe
RadGaugeAutomationPeer ,
vous pouvez trouver à la fois une méthode pour "Minimum" et pour "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); }
La méthode
RaiseMaximumPropertyChangedEvent n'est jamais utilisée dans le code, mais
RaiseMinimumPropertyChangedEvent est utilisée deux fois. Et, vous savez, les performances de la méthode
OnMaxValuePropertyChanged soulèvent des questions ... Je pense qu'elle était censée écrire ce qui suit:
private static void OnMaxValuePropertyChanged( DependencyObject sender, DependencyPropertyChangedEventArgs args) { .... peer.RaiseMaximumPropertyChangedEvent((double)args.OldValue, (double)args.NewValue); .... }
Mais même ainsi, le code n'a pas l'air très soigné en raison du grand nombre d'éléments répétitifs. Il est difficile à comprendre et les lignes répétées ternissent l’attention du programmeur, et il devient plus difficile de passer en revue le code dans de telles conditions. Mais les outils d'analyse statique font un excellent travail de vérification de ce code (mais cela ne signifie pas que vous pouvez le faire sans refactorisation et surtout sans réduire le nombre de lignes répétées).
D'après ce qui précède et l'extrait de code suivant, nous pouvons supposer que les auteurs ne sont pas opposés au copier-coller. Cependant, comme nous tous ... :)
PVS-Studio Warning :
V3001 Il existe des sous-expressions identiques 'element.RenderSize == emptySize' à gauche et à droite de '||' opérateur. 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); }
L'analyseur a détecté un fragment de code d'erreur dans lequel, à droite et à gauche de l'opérateur «||» l'
instruction if utilise les mêmes sous-expressions. La deuxième sous-expression aurait clairement dû être différente. Peut-être à la place du deuxième
RenderSize devrait être
DesiredSize . Ou il ne devrait pas y avoir du tout de deuxième sous-expression. Dans tous les cas, ce morceau de code doit être corrigé.
PVS-Studio Warning :
V3001 Il existe des sous-expressions identiques 'text [0] ==' - '' à gauche et à droite de '||' opérateur. 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; } .... }
Ici, le développeur écrit le texte entré dans le champ de la zone de texte dans une variable. Ensuite, le premier caractère de la chaîne est comparé deux fois avec le même caractère «-», ce qui est une décision suspecte. De toute évidence, la validation de texte dans cette fonction ne fonctionne pas comme prévu à l'origine.
PVS-Studio Warning :
V3001 Il existe des sous-expressions identiques 'newValue.HasValue' à gauche et à droite de l'opérateur '&&'. 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)
L'expression
newValue.HasValue renvoie
true si
newValue contient une valeur et l'expression
newValue! = Null fait de même. L'analyseur fait attention à cela, et que faire est de supprimer l'une des sous-expressions ou de la remplacer par une autre (si quelque chose d'autre aurait dû être vérifié), c'est aux développeurs de décider.
PVS-Studio Warning :
V3125 L'objet 'CurrentAttachedMenu' a été utilisé après avoir été vérifié par rapport à null. Vérifiez les lignes: 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 se révèle être
nulle , l'évaluation de l'expression
CurrentAttachedMenu.IsOpen déclenche une exception. À première vue, il semble que ce soit une simple faute de frappe et ne signifie pas une comparaison avec
null , mais l'opération inverse - '! ='. Mais alors une exception se produira dans la condition de l'
instruction if si la variable
CurrentAttachedMenu est
null .
Plus loin dans le code, il y avait
37 autres avertissements identiques, dont certains semblent indiquer des erreurs. Mais les décrire dans le cadre d'un article est encore trop, alors je vais les laisser sans surveillance.
PVS-Studio Warning :
V3019 Il est possible qu'une variable incorrecte soit comparée à null après la conversion de type à l'aide du mot clé 'as'. Vérifiez les 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; } .... }
Il est très probable que l'auteur ait mélangé les variables. L'inégalité
nulle est vérifiée non pas par le lien obtenu à la suite de la
conversion , mais par l'original (
dragDropElement ). Très probablement, le lien
uiDragDropElement était censé être
vérifié . La conjecture est également confirmée par le fait que le programmeur a en outre utilisé
uiDragDropElement sans vérifier
null .
Avertissement PVS-Studio :
vérification récurrente
V3030 . La condition «! ShowIndicatorWhenNoData» a déjà été vérifiée à la ligne 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; } .... }
L'analyseur a trouvé un morceau de code dans lequel, dans deux conditions, la même variable
showIndicatorWhenNoData est
revérifiée . La vérification est peut-être simplement redondante, mais il est également possible que l'une des sous-expressions en double soit différente.
PVS-Studio Warning :
V3031 Un contrôle excessif peut être simplifié. Le '||' L'opérateur est entouré d'expressions opposées. 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); } }
Ce morceau de code n'est pas formellement erroné. L'analyseur indique une certaine redondance de code dans la condition. Cependant, il convient de se rappeler qu'un code supplémentaire est parfois le résultat d'une erreur, par exemple lorsqu'au lieu d'une variable, une autre est vérifiée plusieurs fois.
Vous pouvez réduire un peu cette condition et supprimer le code supplémentaire. Par exemple, comme ceci:
internal class SelectedItemCollection : ObservableCollection<object> { .... private bool CanInsertItem(object item) { return this.suspendLevel == 0 && this.AllowSelect && (this.AllowMultipleSelect || this.Count == 0); } }
Autres messages similaires:
- V3031 Un contrôle excessif peut être simplifié. Le '||' L'opérateur est entouré d'expressions opposées. SelectedItemCollection.cs 93
- V3031 Un contrôle excessif peut être simplifié. Le '||' L'opérateur est entouré d'expressions opposées. StackVirtualizationStrategy.cs 49
- V3031 Un contrôle excessif peut être simplifié. Le '||' L'opérateur est entouré d'expressions opposées 'state == null' et 'state! = null'. LocalFieldDescriptionsProviderBase.cs 24
Considérez un autre morceau de code où l'analyseur a renvoyé ce qui suit:
Avertissements de PVS-Studio :
- V3137 La variable 'leftMargin' est affectée mais n'est pas utilisée à la fin de la fonction. DragDrop.cs 87
- V3137 La variable 'topMargin' est affectée mais n'est pas utilisée à la fin de la fonction. DragDrop.cs 88
internal static class DragDrop { .... double leftMargin = 0d; double topMargin = 0d; if (frameworkElementSource != null) { leftMargin = frameworkElementSource.Margin.Left;
L'analyseur signale que les
variables leftMargin et
topMargin ont reçu des valeurs, mais après cela, ces variables ne sont utilisées qu'à la fin de la méthode. Il n'y a probablement pas d'erreur ici, mais un tel code semble suspect. Cela peut être dû à une faute de frappe ou à une refactorisation infructueuse.
Le même problème a été trouvé ailleurs: V3137 La variable 'currentColumnLength' est affectée mais n'est pas utilisée à la fin de la fonction. WrapLayout.cs 824
PVS-Studio Warning :
V3061 Le paramètre 'index' est toujours réécrit dans le corps de la méthode avant d'être utilisé. 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,
Le paramètre d'
index de la méthode
FindGroupAndItemIndex est écrasé avant d'être utilisé. Très probablement, cela indique une erreur de programmation.
PVS-Studio Warning :
V3083 Invocation non
sûre de l'événement 'Completed', NullReferenceException est possible. Pensez à affecter un événement à une variable locale avant de l'invoquer. ActionBase.cs 32
internal abstract class ActionBase { .... protected virtual void OnCompleted() { this.IsCompleted = true; if (this.Completed != null) { this.Completed(this, EventArgs.Empty); } } }
Le programmeur a autorisé un appel potentiellement dangereux au gestionnaire d'événements dans cette méthode, ce qui peut entraîner une exception de type
NullReferenceException . Une exception sera levée à condition qu'entre la vérification
nulle et l'appel des gestionnaires d'événements, cet événement ne reste pas.
Il y a
49 autres problèmes similaires dans le code. Il ne sera pas intéressant de les regarder tous dans cet article, et les auteurs pourront facilement les trouver par eux-mêmes en utilisant PVS-Studio, nous allons donc passer à d'autres erreurs.
PVS-Studio Warning :
V3145 Déréférencement non
sûr d'une cible WeakReference, pensez à inspecter info.Target. L'objet aurait pu être récupéré entre la vérification de «IsAlive» et l'accès à la propriété «Target». FadeAnimation.cs 84
public class RadFadeAnimation : RadAnimation { .... protected internal override void ApplyAnimationValues(PlayAnimationInfo info) { .... if (info.Target.Opacity != opacity)
L'analyseur met en garde contre le danger d'une exception de type
NullReferenceException lors de l'accès à la propriété
info.Target.Opacity . Afin de mieux comprendre l'essence du problème, vous devez examiner des fragments de la classe
PlayAnimationInfo , en particulier la propriété
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 général, plus vous creusez ce code, plus vous pouvez y trouver de problèmes potentiels. Regardons peut-être le plus intéressant - celui auquel l'analyseur a émis un avertissement. Le fait est que même si l'exécution suit la branche alors de l'
instruction if , cela ne garantit pas qu'une référence non nulle sera retournée. Indépendamment des conversations sur le casting, nous considérons ici tout ce qui est permis en raison de l'initialisation de l'objet dans le constructeur.
Comment est-ce possible? Le fait est que si entre la vérification
IsAlive et l'appel
Target le garbage collection est effectué, sous lequel
tombe l'objet référencé par
WeakReference ,
this.target.Target renverra
null . En d'autres
termes , la vérification
IsAlive ne garantit pas que lors d'un accès ultérieur à
Target, l' objet est toujours disponible.
Soit dit en passant, la situation est de
retour nulle; intercepte une autre règle de diagnostic: V3080 Déréférence nulle possible. Pensez à inspecter 'info.Target'. FadeAnimation.cs 84
Des problèmes similaires dans le code se sont produits plusieurs fois:
- V3145 Déréférencement non sûr d'une cible WeakReference, pensez à inspecter la cible. L'objet aurait pu être récupéré avant d'accéder à la propriété «Target». MoveXAnimation.cs 80
- V3145 Déréférencement non sûr d'une cible WeakReference, pensez à inspecter la cible. L'objet aurait pu être récupéré avant d'accéder à la propriété «Target». MoveYAnimation.cs 80
- V3145 Déréférencement dangereux d'une cible WeakReference, pensez à inspecter info.Target. L'objet aurait pu être récupéré avant d'accéder à la propriété «Target». PlaneProjectionAnimation.cs 244
- V3145 Déréférence non sûre d'une cible WeakReference. L'objet aurait pu être récupéré entre la vérification de «IsAlive» et l'accès à la propriété «Target». WeakEventHandler.cs 109
Passons à l'exemple suivant.
Avertissement PVS-Studio :
V3066 Ordre incorrect possible des arguments passés au constructeur 'NotifyCollectionChangedEventArgs': 'oldItem' et '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);
Pour comprendre ce que signifie cet avertissement de l'analyseur, il convient de
consulter les
paramètres du constructeur
NotifyCollectionChangedEventArgs :
public NotifyCollectionChangedEventArgs( NotifyCollectionChangedAction action, object newItem, object oldItem, int index);
L'analyseur avertit que dans l'expression
return new NotifyCollectionChangedEventArgs( action, oldItem, newItem, changeIndex);
échangé les variables
oldItem et
newItem . Dans la définition du constructeur, ils sont répertoriés dans un ordre différent. Que cela ait été fait consciemment ou non, on ne peut que deviner.
PVS-Studio Warning :
V3102 Accès suspect à l'élément de l'objet 'x' par un index constant à l'intérieur d'une boucle. 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]))
À chaque itération de la boucle, le programmeur compare
x [0] et
y [0]. Cependant, la boucle n'a pas de sens dans ce code car seuls les premiers éléments sont comparés. Très probablement, cela signifiait une comparaison des éléments correspondants des tableaux. Ensuite, le code correct sera comme ceci:
for (int i = 0; i < x.Length; i++) { if (!object.Equals(x[i], y[i])) { return false; } }
Avertissement PVS-Studio :
V3123 L'opérateur '?:'
Fonctionne peut-être d'une manière différente de celle attendue. Sa priorité est inférieure à la priorité des autres opérateurs dans son état. EditRowHostPanel.cs 35
protected override Size MeasureOverride(Size availableSize) { .... bool shouldUpdateRowHeight = editorLine == 0 || displayedElement == null ? false : displayedElement.ContainerType != typeof(DataGridGroupHeader); .... }
Un avertissement est associé à l'utilisation de l'opérateur '?:'. Il a une priorité inférieure à
! =, ||, ==. Par conséquent, une expression peut ne pas être évaluée comme prévu par le programmeur. Apparemment, dans ce cas, c'est un faux positif et le code fonctionne correctement. Mais la lecture d'un tel code est très difficile et il n'est jamais certain qu'il soit compris correctement. On dirait que le développeur a écrit de telle manière que personne n'a rien compris :) La meilleure façon de le faire est plus lisible - utilisez des crochets ou l'
instruction if .
PVS-Studio Avertissement :
V3078 L'ordre de tri d'origine sera perdu après un appel répétitif à la méthode 'OrderBy'. Utilisez la méthode 'ThenBy' pour conserver le tri d'origine. 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)); } }
L'erreur est liée à l'appel à nouveau de
OrderBy pour une collection de type
IOrderedEnumerable . Ici, la collection est triée d'abord par colonnes, puis par lignes. De plus, au moment du tri par lignes, le tri précédent par colonnes n'est pris en compte nulle part. Afin de ne pas perdre le tri par colonnes et trier la collection selon plusieurs critères à la fois, utilisez alors
ThenBy :
this.MergeCellSelectionRegions(selectedItemsInView .OrderBy(c => c.Column.ItemInfo.LayoutInfo.Line) .ThenBy(c => c.RowItemInfo.LayoutInfo.Line));
PVS-Studio Warning :
V3008 La variable 'currentColumnLength' reçoit des valeurs successives deux fois. C'est peut-être une erreur. Vérifiez les lignes: 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;
Il semblait suspect à l'analyseur que la variable
currentColumnLength se voit attribuer une valeur deux fois. La variable n'est pas utilisée entre les affectations. Quelle que soit la condition, cette variable sera finalement nulle. Ce code est incorrect ou redondant.
Avertissement PVS-Studio :
V3127 Deux fragments de code similaires ont été trouvés. Il s'agit peut-être d'une faute de frappe et la variable «emptyIconContainer» devrait être utilisée à la place de «illedIconContainer »RadRatingItem.cs 240
public class RadRatingItem : RadContentControl { .... protected override void OnApplyTemplate() { .... this.filledIconContainer = this.GetTemplateChild( "FilledIconContainer") as Border; if (this.filledIconContainer == null)
En raison d'une faute de frappe, deux conditions identiques sont apparues dans le code. A en juger par l'exception générée, la deuxième condition devrait ressembler à ceci:
if (this.emptyIconContainer == null) { throw new MissingTemplatePartException( "EmptyIconContainer", typeof(Border)); }
PVS-Studio Warning :
V3020 Une «pause» inconditionnelle dans une boucle. 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; } } }
L'analyseur a constaté qu'ici l'instruction
break n'appartient pas à l'
instruction if .
Break sera exécuté quelle que soit la valeur de
pair.IsFrozen et de ce fait dans
foreach , une seule itération sera effectuée.
Cela met fin à mon examen des avertissements. Afin que les développeurs Telerik puissent effectuer une analyse de code plus approfondie et corriger les défauts, nous sommes prêts à leur fournir une licence temporaire. De plus, ils peuvent profiter de l'
utilisation gratuite de l' option
PVS-Studio offerte aux auteurs de projets open source.
Conclusion
Bien que les développeurs de Telerik UI pour UWP aient fait un excellent travail, ce n'était pas sans fautes de frappe, comme cela arrive généralement :). Toutes ces erreurs peuvent être facilement détectées par un analyseur statique et corrigées. Le plus important est d'utiliser
correctement et régulièrement l'analyseur.
Si vous souhaitez partager cet article avec un public anglophone, veuillez utiliser le lien vers la traduction: Ekaterina Nikiforova.
Vérification de l'interface utilisateur de Telerik pour UWP comme moyen de démarrer avec PVS-Studio .