About Avalonia UI

Analysis results

private void UpdateWMStyles(Action change) { .... var style = (WindowStyles)GetWindowLong(....); .... style = style | controlledFlags ^ controlledFlags; .... }

1100 0011 | 1111 0000 ^ 1111 0000

1100 0011 | 0000 0000

private void UpdateWMStyles(Action change) { .... style = (style | controlledFlags) ^ controlledFlags; .... }

public static Point PointToClient(this IVisual visual, PixelPoint point) { var rootPoint = visual.VisualRoot.PointToClient(point); return visual.VisualRoot.TranslatePoint(rootPoint, visual).Value; }

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; }

V3080 Possible null dereference. Consider inspecting 'p'. VisualExtensions.cs 35

V3080 Possible null dereference. Consider inspecting 'controlPoint'. Scene.cs 176

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 ....; }

private void OnEffectiveViewportChanged(TransformedBounds? bounds) { .... var transform = _owner.GetVisualRoot().TransformToVisual(_owner).Value; .... }

public static Matrix? TransformToVisual(this IVisual from, IVisual to) { var common = from.FindCommonVisualAncestor(to); if (common != null) { .... } return null; }

public static IVisual FindCommonVisualAncestor(this IVisual visual, IVisual target) { Contract.Requires<ArgumentNullException>(visual != null); return ....FirstOrDefault(); }

public static bool IsDirectional(this NavigationDirection direction) { return direction > NavigationDirection.Previous || direction <= NavigationDirection.PageDown; }

internal SelectionChangedEventArgs GetSelectionChangedEventArgs() { .... return new SelectionChangedEventArgs (DataGrid.SelectionChangedEvent, removedSelectedItems, addedSelectedItems) { Source = OwningGrid }; }

public SelectionChangedEventArgs(RoutedEvent routedEvent, IList addedItems, IList removedItems) : base(routedEvent) { AddedItems = addedItems; RemovedItems = removedItems; }

OnSelectionChanged(new SelectionChangedEventArgs(SelectionChangedEvent, removed, added));

Possible incorrect order of arguments passed to 'ItemsRepeaterElementIndexChangedEventArgs' constructor: 'oldIndex' and 'newIndex'. ItemsRepeater.cs 532

Possible incorrect order of arguments passed to 'Update' method: 'oldIndex' and 'newIndex'. ItemsRepeater.cs 536

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); } ..... } }

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; }

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); } }

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); } }

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]; } .... } .... }

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."); }

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."); }

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; }

protected override IControl CreateContainer(object item) { var tabItem = (TabItem)base.CreateContainer(item); tabItem.ParentTabControl = Owner; .... }

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; } }

protected override IControl CreateContainer(object item) { var tabItem = (TabItem)base.CreateContainer(item); if(tabItem == null) return; tabItem.ParentTabControl = Owner; .... }

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"); }

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; }

Conclusion

This article is a review of the bugs found in the Avalonia UI project with the static analyzer PVS-Studio. Avalonia UI is an open-source cross-platform XAML-based UI framework. This is one of the most technologically significant projects in the history of .NET as it enables developers to create cross-platform interfaces based on the WPF system. We hope the project's authors will find this article helpful in fixing some of the bugs, and convincing enough to make static analysis part of their development process.Avalonia UI (previously known as Perspex) allows developers to create user interfaces that can run on Windows, Linux, and MacOS. As an experimental feature, it also provides support of Android and iOS. Avalonia UI is not a wrapper around other wrappers, like Xamarin Forms, which wraps Xamarin wrappers, but directly accesses the native API. While watching one of the demo videos, I was astonished to learn that you can output a control to the Debian console. Moreover, thanks to the use of the XAML markup language, Avalonia UI provides more design and layout capabilities in comparison with other UI constructors.To name a few examples, Avalonia UI is used in AvalonStudio (a cross-platform IDE for C# and C/C++ software development) and Core2D (a 2D diagram editor). Wasabi Wallet (a bitcoin wallet) is an example of commercial software that makes use of Avalonia UI.The fight against the necessity of keeping a bunch of libraries when creating a cross-platform application is extremely important. We wanted to help the authors of Avalonia UI with that, so I downloaded the project's source code and checked it with our analyzer. I hope they will see this article and make the suggested fixes and even start using static analysis regularly as part of their development process. This can be easily done thanks to the PVS-Studio free licensing option available to open-source developers. Using static analysis on a regular basis helps avoid lots of problems and make bug detection and fixing much cheaper.There are identical sub-expressions 'controlledFlags' to the left and to the right of the '^' operator. WindowImpl.cs 975TwitterClientMessageHandler.cs 52To add some symbolism, let's start with our first C# diagnostic. The analyzer has detected a strange expression with the bitwise OR operator. Let me explain this using numbers:the expressionis equivalent toThe precedence of the exclusive OR ("^") is higher than that of the bitwise OR ("|"). The programmer didn't probably intend this order. The code can be fixed by enclosing the first expression in parentheses:As for the next two warnings, I have to admit: these are false positives. You see, the developers are using the public API of themethod. In this case,is always a parent element to. I didn't understand that when examining the warning; it was only after I had finished the article that one of the project's authors told me about that. Therefore, the fixes suggested below actually aim at protecting the code against potential modifications breaking this logic rather than an actual crash.Possible null dereference of method return value. Consider inspecting: TranslatePoint(...). VisualExtensions.cs 23This method is a small one. The analyzer believes the dereference of the value returned by the call ofis unsafe. Let's take a look at this method:Indeed, it could returnThis method is called six times: three times with a check of the returned value, and the other three without a check, thus triggering the warning about potential dereference. The first is the one above, and here are the other two:I suggest fixing these bugs following the pattern used in the safe versions, i.e. by adding acheck inside themethod: V3080 Possible null dereference of method return value. Consider inspecting: TransformToVisual(...). ViewportManager.cs 381This bug is very similar to the previous one:This is the code of themethod:By the way, themethod can indeed returnas the default value for reference types:Themethod is called nine times, with only seven checks. The first call with unsafe dereference is the one above, and here's the second: V3080 Possible null dereference. Consider inspecting 'transform'. MouseDevice.cs 80Expression is always true. Probably the '&&' operator should be used here. NavigationDirection.cs 89This check is a strange one. Theenumeration contains 9 types, with thetype being the last. Maybe it hasn't always been that way, or maybe this is a protection against SUDDEN addition of new direction options. In my opinion, the first check should be enough. Anyway, let's leave this to the authors to decide. V3066 Possible incorrect order of arguments passed to 'SelectionChangedEventArgs' constructor: 'removedSelectedItems' and 'addedSelectedItems'. DataGridSelectedItemsCollection.cs 338The analyzer is warning about the wrong order of the second and third arguments of the constructor. Let's take a look at that constructor:It takes two containers of typeas arguments, which makes it very easy to write them in the wrong order. A comment at the beginning of the class suggests that this is a mistake in the code of the control borrowed from Microsoft and modified for use in Avalonia. But I'd still insist on fixing the argument order if only to avoid getting a bug report on it and wasting time looking for a bug in your own code.There were three more errors of this type: V3066 Possible incorrect order of arguments passed to 'SelectionChangedEventArgs' constructor: 'removed' and 'added'. AutoCompleteBox.cs 707It's the same constructorTwo warnings on one event call method.The analyzer noticed that the argumentsandare written in a different order in both methodsandPerhaps this code was being written by different programmers, one of whom was interested more in the past, and the other in the future :)Just like the previous issue, this one doesn't call for immediate fixing; it has yet to be determined if this code is actually faulty. V3004 The 'then' statement is equivalent to the 'else' statement. DataGridSortDescription.cs 235This is a pretty curious implementation of themethod. Theinterface, which theargument is inherited from, contains the method, which was apparently intended to be used in the following way: V3106 Possible negative index value. The value of 'index' index could reach -1. Animator.cs 68The analyzer is sure that thevariable can end up with the value -1. This variable is assigned the value returned by themethod, so let's take a look at it:As you can see, the loop contains a condition followed by a return statement returning the previous value of the iterator. It's difficult to check if this condition is true, and I can't tell for sure what valuewill have, but the description suggests that it takes a value from 0.0 to 1.0. But we still can say a few words about: it's thevariable passed to the calling method, and it's definitely larger than zero and less than one. If it weren't so, the execution would follow a different branch. If these methods are used for animation, this situation looks much like a decent Heisenbug. I'd recommend checking the value returned byif this case needs some special treatment or remove the first element from the loop if it doesn't meet some other conditions. I don't know how exactly all this should work, so I'd go for the second solution as an example: V3117 Constructor parameter 'phones' is not used. Country.cs 25This is a good example of how static analysis is better than code reviews. The constructor is called with thirteen arguments, one of which isn't used. Actually, Visual Studio could detect it too, but only with the help of third-level diagnostics (which are often turned off). We're definitely dealing with a bug here because the class also contains thirteen properties – one per argument – but there's no assignment to thevariable. Since the fix is obvious, I won't spell it out. V3080 Possible null dereference. Consider inspecting 'tabItem'. TabItemContainerGenerator.cs 22The analyzer considers the dereference of the value returned by themethod unsafe. Let's take a look at this method:PVS-Studio can track an assignment ofeven through a chain of fifty methods, but it can't say if execution would ever follow that branch. Neither could I, for that matter… The calls are lost among overridden and virtual methods, so I'd simply suggest writing an additional check just in case: V3142 Unreachable code detected. It is possible that an error is present. DevTools.xaml.cs 91There's no use citing too much code trying to keep up the suspense; I'll just tell you right away: this warning is a false positive. The analyzer detected a call of the method that throws an unconditional exception:Thirty-five (!) warnings about unreachable code following the calls to this method were too much to ignore, so I asked one of the developers what was happening here. He told me they used a technique, where you replace calls to one method with calls to other methods using the Mono.Cecil library. This library allows you to replace calls right in the IL code.Our analyzer doesn't support this library, hence the huge amount of false positives. It means this diagnostic should be turned off when checking Avalonia UI. It feels somewhat awkward, but I have to confess that it's me who made this diagnostic… But, like any other tool, a static analyzer needs some fine tuning.For instance, we are currently working on a diagnostic detecting unsafe type conversions. It produces about one thousand false positives on a game project where type checking is done on the engine's side. V3009 It's odd that this method always returns one and the same value of 'true'. DataGridRows.cs 412The method returnsall the time. Perhaps its purpose has changed since it was first written, but it looks more like a bug. Judging by the comment at the beginning of the class, this is another control class borrowed from Microsoft. If you ask me,is one of the least stable controls, so perhaps it's not a good idea to confirm the scroll when it doesn't meet the conditions.Some of the bugs described above were borrowed along with the code copied from the WPF controls, and the authors of Avalonia UI have nothing to do with them. But it doesn't make a difference to the user: a crashing or glitching interface leaves a bad impression of the program's overall quality.I mentioned the necessity of fine tuning the analyzer: false positives are just unavoidable due to the working principles behind static analysis algorithms. Those familiar with the halting problem know that there are mathematical constraints in processing one piece of code with another. In this case, though, we are talking about disabling one diagnostic out of almost one hundred and a half. So, there's no problem of loss of meaning in the case of static analysis. Besides, this diagnostic could as well produce warnings pointing at genuine bugs, but those would be hard to notice among tons of false positives.I must mention the remarkable quality of the Avalonia UI project! I hope the developers will keep it that way. Unfortunately, the number of bugs inevitably grows along with the program's size. Wise fine tuning of the CI\CD systems, backed up with static and dynamic analysis, is one of the ways to keep bugs at bay. And if you want to make the development of large projects easier and spend less time debugging, download and try PVS-Studio!