Cet article est une revue des bogues trouvés dans le projet d'interface utilisateur Avalonia avec l'analyseur statique PVS-Studio. Avalonia UI est un framework UI multiplateforme open source basé sur XAML. Il s'agit de l'un des projets les plus importants sur le plan technologique de l'histoire de .NET car il permet aux développeurs de créer des interfaces multiplateformes basées sur le système WPF. Nous espérons que les auteurs du projet trouveront cet article utile pour corriger certains bogues et suffisamment convaincant pour intégrer l'analyse statique à leur processus de développement.
À propos d'Avalonia UI
L'interface utilisateur d'Avalonia (précédemment connue sous le nom de Perspex) permet aux développeurs de créer des interfaces utilisateur pouvant s'exécuter sur Windows, Linux et MacOS. En tant que fonctionnalité expérimentale, il prend également en charge Android et iOS. Avalonia UI n'est pas un wrapper autour d'autres wrappers, comme Xamarin Forms, qui encapsule les wrappers Xamarin, mais accède directement à l'API native. En regardant l'une des vidéos de démonstration, j'ai été étonné d'apprendre que vous pouvez sortir un contrôle sur la console Debian. De plus, grâce à l'utilisation du langage de balisage XAML, Avalonia UI offre plus de capacités de conception et de mise en page par rapport à d'autres constructeurs d'interface utilisateur.
Pour ne citer que quelques exemples, Avalonia UI est utilisé dans
AvalonStudio (un IDE multiplateforme pour le développement de logiciels C # et C / C ++) et
Core2D (un éditeur de diagramme 2D).
Wasabi Wallet (un portefeuille bitcoin) est un exemple de logiciel commercial qui utilise l'interface utilisateur d'Avalonia.
La lutte contre la nécessité de conserver un tas de bibliothèques lors de la création d'une application multiplateforme est extrêmement importante. Nous voulions aider les auteurs d'Avalonia UI avec cela, alors j'ai téléchargé le code source du projet et l'ai vérifié avec notre analyseur. J'espère qu'ils verront cet article et apporteront les correctifs suggérés et commenceront même à utiliser régulièrement l'analyse statique dans le cadre de leur processus de développement. Cela peut être facilement réalisé grâce à l'option de licence gratuite PVS-Studio disponible pour les développeurs open source. L'utilisation régulière de l'analyse statique permet d'éviter de nombreux problèmes et de rendre la détection et la correction des bogues beaucoup moins chères.
Résultats d'analyse
Message de diagnostic de PVS-Studio: V3001 Il existe des sous-expressions identiques 'controllFlags' à gauche et à droite de l'opérateur '^'. WindowImpl.cs 975TwitterClientMessageHandler.cs 52
private void UpdateWMStyles(Action change) { .... var style = (WindowStyles)GetWindowLong(....); .... style = style | controlledFlags ^ controlledFlags; .... }
Pour ajouter du symbolisme, commençons par notre premier diagnostic C #. L'analyseur a détecté une expression étrange avec l'opérateur OR au niveau du bit. Permettez-moi d'expliquer cela en utilisant des nombres:
l'expression
1100 0011 | 1111 0000 ^ 1111 0000
est équivalent à
1100 0011 | 0000 0000
La priorité du OU exclusif ("^") est supérieure à celle du OU au niveau du bit ("|"). Le programmeur n'avait probablement pas l'intention de cette commande. Le code peut être corrigé en mettant la première expression entre parenthèses:
private void UpdateWMStyles(Action change) { .... style = (style | controlledFlags) ^ controlledFlags; .... }
Quant aux deux prochains avertissements, je dois admettre: ce sont des faux positifs. Vous voyez, les développeurs utilisent l'API publique de la méthode
TransformToVisual . Dans ce cas,
VisualRoot est toujours un élément parent de
visual . Je n'ai pas compris cela lors de l'examen de l'avertissement; ce n'est qu'après avoir terminé l'article que l'un des auteurs du projet m'en a parlé. Par conséquent, les correctifs suggérés ci-dessous visent en fait à protéger le code contre des modifications potentielles brisant cette logique plutôt qu'un plantage réel.
Message de diagnostic PVS-Studio: V3080 Déréférence nulle possible de la valeur de retour de la méthode. Pensez à inspecter: TranslatePoint (...). VisualExtensions.cs 23
public static Point PointToClient(this IVisual visual, PixelPoint point) { var rootPoint = visual.VisualRoot.PointToClient(point); return visual.VisualRoot.TranslatePoint(rootPoint, visual).Value; }
Cette méthode est petite. L'analyseur estime que la déréférence de la valeur renvoyée par l'appel de
TranslatePoint n'est pas sûre. Jetons un œil à cette méthode:
public static Point? TranslatePoint(this IVisual visual, Point point, IVisual relativeTo) { var transform = visual.TransformToVisual(relativeTo); if (transform.HasValue) { return point.Transform(transform.Value); } return null; }
En effet, il pourrait retourner
null .
Cette méthode est appelée six fois: trois fois avec une vérification de la valeur renvoyée et les trois autres sans vérification, déclenchant ainsi l'avertissement concernant une éventuelle déréférence. Le premier est celui ci-dessus, et voici les deux autres:
Je suggère de corriger ces bogues en suivant le modèle utilisé dans les versions sûres, c'est-à-dire en ajoutant une
vérification Nullable <Struct> .HasValue à l'intérieur de la méthode
PointToClient :
public static Point PointToClient(this IVisual visual, PixelPoint point) { var rootPoint = visual.VisualRoot.PointToClient(point); if (rootPoint.HasValue) return visual.VisualRoot.TranslatePoint(rootPoint, visual).Value; else throw ....; }
Message de diagnostic PVS-Studio: V3080 Déréférence nulle possible de la valeur de retour de la méthode. Pensez à inspecter: TransformToVisual (...). ViewportManager.cs 381
Ce bug est très similaire au précédent:
private void OnEffectiveViewportChanged(TransformedBounds? bounds) { .... var transform = _owner.GetVisualRoot().TransformToVisual(_owner).Value; .... }
Voici le code de la méthode
TransformToVisual :
public static Matrix? TransformToVisual(this IVisual from, IVisual to) { var common = from.FindCommonVisualAncestor(to); if (common != null) { .... } return null; }
Par ailleurs, la méthode
FindCommonVisualAncestor peut en effet retourner
null comme valeur par défaut pour les types de référence:
public static IVisual FindCommonVisualAncestor(this IVisual visual, IVisual target) { Contract.Requires<ArgumentNullException>(visual != null); return ....FirstOrDefault(); }
La méthode
TransformToVisual est appelée neuf fois, avec seulement sept vérifications. Le premier appel avec déréférence non sécurisée est celui ci-dessus, et voici le second:
V3080 Déréférence nulle possible. Pensez à inspecter «transformer». MouseDevice.cs 80
Message de diagnostic PVS-Studio: V3022 L' expression est toujours vraie. L'opérateur '&&' devrait probablement être utilisé ici. NavigationDirection.cs 89
public static bool IsDirectional(this NavigationDirection direction) { return direction > NavigationDirection.Previous || direction <= NavigationDirection.PageDown; }
Cette vérification est étrange. L'énumération
NavigationDirection contient 9 types, le type
PageDown étant le dernier. Peut-être que cela n'a pas toujours été ainsi, ou peut-être que c'est une protection contre l'ajout soudain de nouvelles options de direction. À mon avis, le premier contrôle devrait suffire. Quoi qu'il en soit, laissons cela aux auteurs pour décider.
Message de diagnostic PVS-Studio: V3066 Ordre incorrect possible des arguments transmis au constructeur 'SelectionChangedEventArgs': 'removeSelectedItems' et 'addedSelectedItems'. DataGridSelectedItemsCollection.cs 338
internal SelectionChangedEventArgs GetSelectionChangedEventArgs() { .... return new SelectionChangedEventArgs (DataGrid.SelectionChangedEvent, removedSelectedItems, addedSelectedItems) { Source = OwningGrid }; }
L'analyseur met en garde contre le mauvais ordre des deuxième et troisième arguments du constructeur. Jetons un coup d'œil à ce constructeur:
public SelectionChangedEventArgs(RoutedEvent routedEvent, IList addedItems, IList removedItems) : base(routedEvent) { AddedItems = addedItems; RemovedItems = removedItems; }
Il prend deux conteneurs de type
IList comme arguments, ce qui facilite leur écriture dans le mauvais ordre. Un commentaire au début de la classe suggère qu'il s'agit d'une erreur dans le code du contrôle emprunté à Microsoft et modifié pour une utilisation dans Avalonia. Mais j'insisterais toujours sur la correction de l'ordre des arguments, ne serait-ce que pour éviter d'obtenir un rapport de bogue dessus et perdre du temps à chercher un bogue dans votre propre code.
Il y avait trois autres erreurs de ce type:
Message de diagnostic PVS-Studio: V3066 Ordre incorrect possible des arguments passés au constructeur 'SelectionChangedEventArgs': 'supprimé' et 'ajouté'. AutoCompleteBox.cs 707
OnSelectionChanged(new SelectionChangedEventArgs(SelectionChangedEvent, removed, added));
C'est le même constructeur
SelectionChangedEventArgs.Messages de diagnostic PVS-Studio V3066 :
- Ordre incorrect possible des arguments passés au constructeur «ItemsRepeaterElementIndexChangedEventArgs»: «oldIndex» et «newIndex». ItemsRepeater.cs 532
- Ordre incorrect possible des arguments passés à la méthode «Update»: «oldIndex» et «newIndex». ItemsRepeater.cs 536
Deux avertissements sur une méthode d'appel d'événement.
internal void OnElementIndexChanged(IControl element, int oldIndex, int newIndex) { if (ElementIndexChanged != null) { if (_elementIndexChangedArgs == null) { _elementIndexChangedArgs = new ItemsRepeaterElementIndexChangedEventArgs(element, oldIndex, newIndex); } else { _elementIndexChangedArgs.Update(element, oldIndex, newIndex); } ..... } }
L'analyseur a remarqué que les arguments
oldIndex et
newIndex sont écrits dans un ordre différent dans les deux méthodes
ItemsRepeaterElementIndexChangedEventArgs et
Update :
internal ItemsRepeaterElementIndexChangedEventArgs( IControl element, int newIndex, int oldIndex) { Element = element; NewIndex = newIndex; OldIndex = oldIndex; } internal void Update(IControl element, int newIndex, int oldIndex) { Element = element; NewIndex = newIndex; OldIndex = oldIndex; }
Peut-être que ce code a été écrit par différents programmeurs, dont l'un était plus intéressé par le passé et l'autre à l'avenir :)
Tout comme le numéro précédent, celui-ci n'appelle pas à une correction immédiate; il reste à déterminer si ce code est réellement défectueux.
Message de diagnostic PVS-Studio: V3004 L'
instruction 'then' est équivalente à l'instruction 'else'. DataGridSortDescription.cs 235
public override IOrderedEnumerable<object> ThenBy(IOrderedEnumerable<object> seq) { if (_descending) { return seq.ThenByDescending(o => GetValue(o), InternalComparer); } else { return seq.ThenByDescending(o => GetValue(o), InternalComparer); } }
Il s'agit d'une implémentation assez curieuse de la méthode
ThenBy . L'interface
IEnumerable , dont hérite l'argument
seq , contient la méthode
ThenBy , qui était apparemment destinée à être utilisée de la manière suivante:
public override IOrderedEnumerable<object> ThenBy(IOrderedEnumerable<object> seq) { if (_descending) { return seq.ThenByDescending(o => GetValue(o), InternalComparer); } else { return seq.ThenBy(o => GetValue(o), InternalComparer); } }
Message de diagnostic PVS-Studio: V3106 Valeur d'index négative possible. La valeur de l'indice «index» pourrait atteindre -1. Animator.cs 68
protected T InterpolationHandler(double animationTime, T neutralValue) { .... if (kvCount > 2) { if (animationTime <= 0.0) { .... } else if (animationTime >= 1.0) { .... } else { int index = FindClosestBeforeKeyFrame(animationTime); firstKeyframe = _convertedKeyframes[index]; } .... } .... }
L'analyseur est sûr que la variable d'
index peut se retrouver avec la valeur -1. Cette variable se voit attribuer la valeur renvoyée par la méthode
FindClosestBeforeKeyFrame , alors examinons-la:
private int FindClosestBeforeKeyFrame(double time) { for (int i = 0; i < _convertedKeyframes.Count; i++) if (_convertedKeyframes[i].Cue.CueValue > time) return i - 1; throw new Exception("Index time is out of keyframe time range."); }
Comme vous pouvez le voir, la boucle contient une condition suivie d'une instruction return retournant la valeur précédente de l'itérateur. Il est difficile de vérifier si cette condition est vraie, et je ne peux pas dire avec certitude quelle valeur
CueValue aura, mais la description suggère qu'elle prend une valeur de 0,0 à 1,0. Mais nous pouvons encore dire quelques mots sur le
temps : c'est la variable
animationTime passée à la méthode appelante, et elle est nettement supérieure à zéro et inférieure à un. S'il n'en était pas ainsi, l'exécution suivrait une branche différente. Si ces méthodes sont utilisées pour l'animation, cette situation ressemble beaucoup à un Heisenbug décent. Je recommande de vérifier la valeur renvoyée par
FindClosestBeforeKeyFrame si ce cas nécessite un traitement spécial ou de supprimer le premier élément de la boucle s'il ne remplit pas d'autres conditions. Je ne sais pas exactement comment tout cela devrait fonctionner, alors je choisirais la deuxième solution comme exemple:
private int FindClosestBeforeKeyFrame(double time) { for (int i = 1; i < _convertedKeyframes.Count; i++) if (_convertedKeyframes[i].Cue.CueValue > time) return i - 1; throw new Exception("Index time is out of keyframe time range."); }
Message de diagnostic PVS-Studio: le paramètre de constructeur
V3117 'téléphones' n'est pas utilisé. Country.cs 25
public Country(string name, string region, int population, int area, double density, double coast, double? migration, double? infantMorality, int gdp, double? literacy, double? phones, double? birth, double? death) { Name = name; Region = region; Population = population; Area = area; PopulationDensity = density; CoastLine = coast; NetMigration = migration; InfantMortality = infantMorality; GDP = gdp; LiteracyPercent = literacy; BirthRate = birth; DeathRate = death; }
C'est un bon exemple de la façon dont l'analyse statique est meilleure que les revues de code. Le constructeur est appelé avec treize arguments, dont l'un n'est pas utilisé. En fait, Visual Studio pourrait également le détecter, mais uniquement à l'aide de diagnostics de troisième niveau (qui sont souvent désactivés). Nous avons certainement affaire à un bogue ici parce que la classe contient également treize propriétés - une par argument - mais il n'y a aucune affectation à la variable
Phones . Étant donné que le correctif est évident, je ne l'expliquerai pas.
Message de diagnostic PVS-Studio: V3080 Déréférence nulle possible. Pensez à inspecter «tabItem». TabItemContainerGenerator.cs 22
protected override IControl CreateContainer(object item) { var tabItem = (TabItem)base.CreateContainer(item); tabItem.ParentTabControl = Owner; .... }
L'analyseur considère que la déréférence de la valeur renvoyée par la méthode
CreateContainer n'est pas sûre. Jetons un œil à cette méthode:
protected override IControl CreateContainer(object item) { var container = item as T; if (item == null) { return null; } else if (container != null) { return container; } else { .... return result; } }
PVS-Studio peut suivre une affectation de
null même à travers une chaîne de cinquante méthodes, mais il ne peut pas dire si l'exécution suivra jamais cette branche. Je ne pouvais pas non plus, d'ailleurs ... Les appels sont perdus parmi les méthodes surchargées et virtuelles, donc je suggère simplement d'écrire un chèque supplémentaire au cas où:
protected override IControl CreateContainer(object item) { var tabItem = (TabItem)base.CreateContainer(item); if(tabItem == null) return; tabItem.ParentTabControl = Owner; .... }
Message de diagnostic PVS-Studio: V3142 Code inaccessible détecté. Il est possible qu'une erreur soit présente. DevTools.xaml.cs 91
Il ne sert à rien de citer trop de code pour maintenir le suspense; Je vais vous le dire tout de suite: cet avertissement est un faux positif. L'analyseur a détecté un appel de la méthode qui lève une exception inconditionnelle:
public static void Load(object obj) { throw new XamlLoadException($"No precompiled XAML found for {obj.GetType()}, make sure to specify x:Class and include your XAML file as AvaloniaResource"); }
Trente-cinq (!) Avertissements concernant le code inaccessible suite aux appels à cette méthode étaient trop à ignorer, j'ai donc demandé à l'un des développeurs ce qui se passait ici. Il m'a dit qu'ils utilisaient une technique, où vous remplacez les appels à une méthode par des appels à d'autres méthodes à l'aide de la bibliothèque
Mono.Cecil . Cette bibliothèque vous permet de remplacer les appels directement dans le code IL.
Notre analyseur ne prend pas en charge cette bibliothèque, d'où l'énorme quantité de faux positifs. Cela signifie que ce diagnostic doit être désactivé lors de la vérification de l'interface utilisateur d'Avalonia. C'est un peu gênant, mais je dois avouer que c'est moi qui ai fait ce diagnostic ... Mais, comme tout autre outil, un analyseur statique a besoin d'un réglage fin.
Par exemple, nous travaillons actuellement sur un diagnostic détectant les conversions de type non sécurisées. Il produit environ mille faux positifs sur un projet de jeu où la vérification de type est effectuée du côté du moteur.
Message de diagnostic PVS-Studio: V3009 Il est étrange que cette méthode renvoie toujours une seule et même valeur de «vrai». DataGridRows.cs 412
internal bool ScrollSlotIntoView(int slot, bool scrolledHorizontally) { if (....) { .... if (DisplayData.FirstScrollingSlot < slot && DisplayData.LastScrollingSlot > slot) { return true; } else if (DisplayData.FirstScrollingSlot == slot && slot != -1) { .... return true; } .... } .... return true; }
La méthode renvoie
vrai tout le temps. Peut-être que son objectif a changé depuis sa première écriture, mais il ressemble plus à un bug. A en juger par le commentaire du début de la classe, il s'agit d'une autre classe de contrôle empruntée à Microsoft. Si vous me demandez,
DataGrid est l'un des contrôles les moins stables, donc ce n'est peut-être pas une bonne idée de confirmer le défilement lorsqu'il ne remplit pas les conditions.
Conclusion
Certains des bogues décrits ci-dessus ont été empruntés avec le code copié à partir des contrôles WPF, et les auteurs d'Avalonia UI n'ont rien à voir avec eux. Mais cela ne fait aucune différence pour l'utilisateur: une interface qui plante ou qui glisse laisse une mauvaise impression de la qualité globale du programme.
J'ai mentionné la nécessité de régler finement l'analyseur: les faux positifs sont tout simplement inévitables en raison des principes de fonctionnement des algorithmes d'analyse statique. Ceux qui connaissent le
problème de l'
arrêt savent qu'il existe des contraintes mathématiques dans le traitement d'un morceau de code avec un autre. Dans ce cas, cependant, nous parlons de désactiver un diagnostic sur près de cent et demi. Il n'y a donc pas de problème de perte de sens dans le cas de l'analyse statique. En outre, ce diagnostic pourrait également produire des avertissements pointant vers de véritables bogues, mais ceux-ci seraient difficiles à remarquer parmi des tonnes de faux positifs.
Je dois mentionner la qualité remarquable du projet d'interface utilisateur Avalonia! J'espère que les développeurs continueront ainsi. Malheureusement, le nombre de bogues augmente inévitablement avec la taille du programme. Un réglage fin et sage des systèmes CI \ CD, soutenu par une analyse statique et dynamique, est l'un des moyens de garder les bogues à distance. Et si vous voulez faciliter le développement de grands projets et passer moins de temps à déboguer,
téléchargez et essayez PVS-Studio!