Cet article est le résultat de la vérification du projet d'interface utilisateur Avalonia à l'aide de l'analyseur statique PVS-Studio. Avalonia UI est une plate-forme d'interface utilisateur open-source multiplateforme basée sur XAML. Il s'agit de l'un des projets les plus importants sur le plan technologique dans l'histoire de .NET, car il vous permet de créer des interfaces multiplateformes basées sur le système WPF. J'espère que cet article aidera les auteurs à corriger certaines erreurs et à les convaincre d'utiliser des analyseurs statiques à l'avenir.
À propos d'Avalonia UI
Le projet d'interface utilisateur Avalonia (anciennement appelé Perspex) offre la possibilité de créer des interfaces utilisateur qui s'exécutent sur Windows, Linux et MacOS. Il existe également un support expérimental pour Android et iOS pour le moment. L'interface utilisateur d'Avalonia n'est pas un wrapper sur des wrappers, mais fait référence à l'API native. Contrairement à Xamarin Forms, qui encapsule les wrappers Xamarin. Dans l'une des vidéos de démonstration, j'ai été frappé par la possibilité d'apporter le contrôle à la console Debian. De plus, le projet offre plus de fonctionnalités pour la mise en page et la conception que les concepteurs d'interface conventionnels, grâce à l'utilisation du balisage XAML.
Les projets qui utilisent déjà Avalonia UI incluent
AvalonStudio (un IDE multiplateforme pour le développement en C # et C / C ++) et
Core2D (éditeur de diagrammes et diagrammes 2D). En tant que projet commercial, vous pouvez apporter un
portefeuille Wasabi (portefeuille Bitcoin).
La lutte contre la nécessité d'avoir plusieurs bibliothèques différentes lors de la création d'une application multiplateforme est d'une grande importance. Nous voulions aider le projet, et j'ai téléchargé le projet et l'ai vérifié avec notre analyseur. J'espère que les auteurs prêteront attention à cet article et apporteront les modifications nécessaires au code, ou introduiront peut-être une analyse statique régulière dans le processus de développement. Pour ce faire, ils peuvent profiter de l'option de licence gratuite de PVS-Studio pour les projets open source. L'utilisation régulière d'un analyseur statique permet d'éviter de nombreux problèmes et réduit le coût de détection et de correction de nombreuses erreurs.
Résultats de validation
PVS-Studio Warning: V3001 Il existe des sous-expressions identiques 'contrôléeFlags' à 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; .... }
Je vais commencer symboliquement par nos premiers diagnostics C #. L'analyseur a détecté une utilisation étrange de l'opérateur OR au niveau du bit. Permettez-moi d'expliquer sur les chiffres ce qui se passe ici:
expression
1100 0011 | 1111 0000 ^ 1111 0000
similaire à ceci:
1100 0011 | 0000 0000
Un OU exclusif ("^") a une priorité plus élevée qu'un OU au niveau du bit ("|"). Très probablement, un ordre d'opérations différent était impliqué ici. Dans ce cas, vous devez prendre la première expression entre parenthèses:
private void UpdateWMStyles(Action change) { .... style = (style | controlledFlags) ^ controlledFlags; .... }
Avant les deux prochains avertissements, je dois admettre: les faux positifs. Cela est dû à l'utilisation de l'API publique, la méthode
TransformToVisual . Dans notre cas,
VisualRoot est toujours le parent de
visual . Je n'ai pas compris cela lors de l'analyse de la réponse, m'a dit l'auteur du projet après avoir écrit l'article. Ainsi, les modifications proposées dans l'article ne visent pas à se protéger contre une chute réelle, mais contre une révision potentielle qui briserait cette logique.
PVS-Studio Warning: 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; }
Une petite méthode. L'analyseur considère que le déréférencement du résultat d'un appel à TranslatePoint n'est pas sûr. Jetez 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 y a un retour
nul .
Cette méthode a 6 appels. Dans trois cas, la valeur est vérifiée et, dans le reste, PVS-Studio détecte un déréférencement potentiel et émet des avertissements. J'ai cité le premier ci-dessus, et les deux autres avertissements sont ici:
Je propose de le
corriger par analogie en ajoutant la
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 ....; }
PVS-Studio Warning: V3080 Déréférence nulle possible de la valeur de retour de la méthode. Pensez à inspecter: TransformToVisual (...). ViewportManager.cs 381
Un cas très similaire à l'exemple précédent:
private void OnEffectiveViewportChanged(TransformedBounds? bounds) { .... var transform = _owner.GetVisualRoot().TransformToVisual(_owner).Value; .... }
La méthode
TransformToVisual ressemble à ceci:
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 utilisée à neuf endroits; il y a des contrôles à sept. Le premier avertissement à utiliser sans vérification est plus élevé, et le dernier est ici:
V3080 Déréférence nulle possible. Pensez à inspecter «transformer». MouseDevice.cs 80
Avertissement PVS-Studio: l' expression
V3022 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; }
Étrange vérification. Dans l'énumération
NavigationDirection , il existe 9 types et
PageDown est le dernier d'entre eux. Ce n'était peut-être pas toujours le cas, ou s'agit-il d'une assurance contre l'apparition soudaine de nouvelles options de référence. Il me semble que le premier chèque suffit ici. Je laisserai la décision aux auteurs du projet.
Avertissement PVS-Studio: V3066 Ordre incorrect possible des arguments passés au constructeur 'SelectionChangedEventArgs': 'removeSelectedItems' et 'addedSelectedItems'. DataGridSelectedItemsCollection.cs 338
internal SelectionChangedEventArgs GetSelectionChangedEventArgs() { .... return new SelectionChangedEventArgs (DataGrid.SelectionChangedEvent, removedSelectedItems, addedSelectedItems) { Source = OwningGrid }; }
Dans ce cas, l'analyseur a suggéré que les deuxième et troisième arguments du constructeur sont confus. Regardons le constructeur appelé:
public SelectionChangedEventArgs(RoutedEvent routedEvent, IList addedItems, IList removedItems) : base(routedEvent) { AddedItems = addedItems; RemovedItems = removedItems; }
Deux conteneurs de type
IList sont acceptés, faciles à mélanger. A en juger par le commentaire du début du cours, il s'agit d'une erreur dans le code de contrôle copié depuis Microsoft et modifié sous Avalonia. Mais il me semble qu'il vaut la peine de corriger l'ordre des arguments de la méthode, du moins de ne pas chercher une éventuelle erreur en soi lors de l'arrivée d'un rapport de bug.
L'analyseur a trouvé trois autres erreurs similaires:
Avertissement 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));
Même constructeur
SelectionChangedEventArgs.Avertissements du 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 opérations dans 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 vu que dans les
deux ItemsRepeaterElementIndexChangedEventArgs et dans la méthode
Update , les arguments
oldIndex et
newIndex ont un ordre différent:
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 le code a été écrit par différents programmeurs, et pour l'un, il est plus important de savoir ce qui s'est passé, et pour l'autre - ce qui va arriver :)
Comme dans le cas précédent, vous ne devez pas le modifier tout de suite, vous devez vérifier s'il y a vraiment une erreur.
PVS-Studio Warning: 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); } }
Une implémentation extrêmement intéressante de la méthode
ThenBy . L'interface
IEnumerable , dont l'héritage
seq est hérité
, a une méthode
ThenBy ; Je suppose que son utilisation était implicite. Comme ça:
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); } }
Avertissement 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 estime que l'
indice peut avoir une valeur de -1. La variable est obtenue à partir de la méthode
FindClosestBeforeKeyFrame , regardez-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 nous pouvons le voir, la condition est vérifiée dans la boucle et la valeur précédente de l'itérateur est retournée. La condition est assez difficile à vérifier, et je ne peux pas dire exactement ce qu'est
CueValue , mais selon la description, elle prend une valeur de 0,0 à 1,0. Nous pouvons dire quelque chose sur le
temps , c'est
animationTime de la méthode appelante, c'est certainement plus de zéro et moins d'un. Sinon, l'exécution du programme irait sur une branche différente. S'il s'agit des méthodes appelées pour rendre l'animation, la situation ressemble à un bon bug flottant. J'ajouterais une vérification pour le résultat de
FindClosestBeforeKeyFrame si une manipulation spéciale est nécessaire dans ce cas. Ou - si le premier élément ne remplit pas d'autres conditions - supprimez-le de la boucle. Ne sachant pas comment tout cela devrait fonctionner, je choisirai la deuxième option comme exemple de correction:
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."); }
Avertissement PVS-Studio: le paramètre constructeur "téléphones"
V3117 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; }
Un bon exemple de la différence entre le fonctionnement de l'analyseur et la révision manuelle du code. Treize arguments constructeurs, un non utilisé. En fait, Visual Studio note également un argument inutilisé, mais au troisième niveau d'avertissements (ils sont souvent désactivés). Dans ce cas, il s'agit d'une erreur évidente, car la classe possède également treize propriétés pour chaque argument, et aucune valeur n'est affectée nulle part dans les
téléphones . Le montage est évident, je ne l'apporterai pas.
Avertissement 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érait qu'il était dangereux de déréférencer le résultat de l'appel de
CreateContainer .
Jetez 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; } }
L'analyseur peut voir l'affectation de
null à une variable même lors du passage d'une valeur à travers une chaîne de cinquante méthodes. Mais il ne peut pas dire si l'exécution se fera sur ce fil au moins une fois. Oui, et je ne pouvais pas non plus, en général ... Les appels de méthode sont perdus parmi les méthodes surchargées et virtuelles. Je suggère donc juste d'être en sécurité avec une vérification supplémentaire:
protected override IControl CreateContainer(object item) { var tabItem = (TabItem)base.CreateContainer(item); if(tabItem == null) return null; tabItem.ParentTabControl = Owner; .... }
Avertissement PVS-Studio: V3142 Code inaccessible détecté. Il est possible qu'une erreur soit présente. DevTools.xaml.cs 91
Je n'écrirai pas beaucoup de code ici pour créer une intrigue, je dirai tout de suite: l'avertissement est faux. L'analyseur a vu un appel à une méthode qui lève une exception inconditionnelle. Le voici:
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"); }
Il était impossible de ne pas faire attention à trente-cinq avertissements (!) À propos du code inaccessible situé après les appels à cette méthode. J'ai demandé à l'un des développeurs du projet: comment ça marche? Et ils m'ont parlé d'un moyen de remplacer les appels d'une méthode par des appels d'autres en utilisant la bibliothèque
Mono.Cecil . Il vous permet de remplacer les appels directement dans le code IL.
L'analyseur ne prend pas en charge cette bibliothèque, par conséquent, nous avons de nombreux faux positifs, il est donc préférable de désactiver ces diagnostics sur ce projet. C'est un peu gênant d'admettre, c'est moi qui ai fait ce diagnostic ... mais, comme tout outil, l'analyse statique doit être configurée.
Par exemple, nous développons actuellement des diagnostics sur la conversion de type non sécurisée. Et cela donne un peu moins d'un millier d'opérations sur le projet de jeu, dans lequel le contrôle de frappe est effectué côté moteur.
Avertissement 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 toujours
true . Peut-être que le but de la méthode a changé depuis que la signature a été écrite, mais c'est probablement une erreur. Il s'agit également de la classe de contrôle copiée à partir de Microsoft, à en juger par le commentaire au début de la classe. À mon avis,
DataGrid est généralement l'un des contrôles les plus instables, et il me semble que cela vaut la peine d'être considéré, est-il nécessaire de confirmer le défilement s'il ne remplit pas les conditions?
Conclusion
Certaines de ces erreurs ont été introduites dans le projet non pas par les développeurs de l'interface utilisateur d'Avalonia eux-mêmes, mais par du code copié à partir des contrôles WPF. Cependant, pour l'utilisateur de l'interface, la source de l'erreur ne joue généralement pas de rôle. Une interface plantée ou défectueuse gâche l'opinion de l'ensemble du programme.
Dans l'article, j'ai mentionné la nécessité de configurer l'analyseur, il y a des faux positifs qui sont inévitables en raison des principes de fonctionnement des algorithmes d'analyse statique. Quiconque connaît le
problème de l'arrêt est conscient des limites mathématiques lors de l'utilisation d'un autre code. Mais dans ce cas, nous parlons de désactiver un diagnostic sur presque cent cinquante. Nous ne parlons donc pas de la perte de sens dans l'analyse statique (ou la question n'en vaut pas la peine). De plus, ce diagnostic aurait pu avoir de bonnes réponses, il est juste plus difficile à trouver parmi la masse des faux positifs.
N'oubliez pas de noter la haute qualité du code du projet! J'espère que les développeurs maintiendront le rythme et le niveau de qualité du code. Malheureusement, plus le projet est volumineux, plus il contient de bogues. Une des façons possibles de réduire les bogues est de configurer correctement CI \ CD, avec la connexion d'analyses statiques et dynamiques. Et si vous souhaitez simplifier le travail avec de grands projets et réduire le temps nécessaire au débogage,
téléchargez et essayez PVS-Studio!
Si vous souhaitez partager cet article avec un public anglophone, veuillez utiliser le lien vers la traduction: Alexander Senichkin.
Notre petite contribution à la lutte d'Avalonia UI pour moins de plateformes .