# Our small contribution to Avalonia UI's fight for fewer platforms

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\.

![0701_avalonia/image1.png](https://import.viva64.com/docx/blog/0701_avalonia/image1.png)

## About Avalonia UI

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](https://github.com/VitalElement/AvalonStudio) \(a cross\-platform IDE for C\# and C/C\+\+ software development\) and [Core2D](https://github.com/wieslawsoltes/Core2D) \(a 2D diagram editor\)\. [Wasabi Wallet](https://wasabiwallet.io/) \(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\.

## Analysis results

**PVS\-Studio diagnostic message:** [V3001](https://pvs-studio.com/en/docs/warnings/v3001/) There are identical sub\-expressions 'controlledFlags' to the left and to the right of the '^' operator\. WindowImpl\.cs 975TwitterClientMessageHandler\.cs 52

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

To 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 expression

```cpp
1100 0011 | 1111 0000 ^ 1111 0000
```

is equivalent to

```cpp
1100 0011 | 0000 0000
```

The 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:

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

As for the next two warnings, I have to admit: these are false positives\. You see, the developers are using the public API of the _TransformToVisual_ method\. In this case, _VisualRoot_ is always a parent element to _visual_\. 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\.

**PVS\-Studio diagnostic message:** [V3080](https://pvs-studio.com/en/docs/warnings/v3080/) Possible null dereference of method return value\. Consider inspecting: TranslatePoint\(\.\.\.\)\. VisualExtensions\.cs 23

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

This method is a small one\. The analyzer believes the dereference of the value returned by the call of _TranslatePoint_ is unsafe\. Let's take a look at this method:

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

Indeed, it could return _null_\. 

This 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:

* [V3080](https://pvs-studio.com/en/docs/warnings/v3080/) Possible null dereference\. Consider inspecting 'p'\. VisualExtensions\.cs 35
* [V3080](https://pvs-studio.com/en/docs/warnings/v3080/) Possible null dereference\. Consider inspecting 'controlPoint'\. Scene\.cs 176

I suggest fixing these bugs following the pattern used in the safe versions, i\.e\. by adding a _Nullable<Struct\>\.HasValue_ check inside the _PointToClient_ method:

```cpp
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 diagnostic message:** [V3080](https://pvs-studio.com/en/docs/warnings/v3080/) Possible null dereference of method return value\. Consider inspecting: TransformToVisual\(\.\.\.\)\. ViewportManager\.cs 381

This bug is very similar to the previous one:

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

This is the code of the _TransformToVisual_ method:

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

By the way, the _FindCommonVisualAncestor_ method can indeed return _null_ as the default value for reference types:

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

The _TransformToVisual_ method is called nine times, with only seven checks\. The first call with unsafe dereference is the one above, and here's the second:

[V3080](https://pvs-studio.com/en/docs/warnings/v3080/) Possible null dereference\. Consider inspecting 'transform'\. MouseDevice\.cs 80

**PVS\-Studio diagnostic message:** [V3022](https://pvs-studio.com/en/docs/warnings/v3022/) Expression is always true\. Probably the '&&' operator should be used here\. NavigationDirection\.cs 89

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

This check is a strange one\. The _NavigationDirection_ enumeration contains 9 types, with the _PageDown_ type 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\.

**PVS\-Studio diagnostic message:** [V3066](https://pvs-studio.com/en/docs/warnings/v3066/) Possible incorrect order of arguments passed to 'SelectionChangedEventArgs' constructor: 'removedSelectedItems' and 'addedSelectedItems'\. DataGridSelectedItemsCollection\.cs 338

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

The analyzer is warning about the wrong order of the second and third arguments of the constructor\. Let's take a look at that constructor:

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

It takes two containers of type _IList_ as 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:

**PVS\-Studio diagnostic message:** [V3066](https://pvs-studio.com/en/docs/warnings/v3066/) Possible incorrect order of arguments passed to 'SelectionChangedEventArgs' constructor: 'removed' and 'added'\. AutoCompleteBox\.cs 707

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

It's the same constructor _SelectionChangedEventArgs\._ 

**PVS\-Studio diagnostic messages** [V3066](https://pvs-studio.com/en/docs/warnings/v3066/):

* 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

Two warnings on one event call method\.

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

The analyzer noticed that the arguments _oldIndex _and_ newIndex_ are written in a different order in both methods _ItemsRepeaterElementIndexChangedEventArgs_ and _Update_:

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

Perhaps 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\.

**PVS\-Studio diagnostic message:** [V3004](https://pvs-studio.com/en/docs/warnings/v3004/) The 'then' statement is equivalent to the 'else' statement\. DataGridSortDescription\.cs 235

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

This is a pretty curious implementation of the _ThenBy_ method\. The _IEnumerable_ interface, which the _seq _argument is inherited from, contains the method _ThenBy_, which was apparently intended to be used in the following way:

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

**PVS\-Studio diagnostic message:** [V3106](https://pvs-studio.com/en/docs/warnings/v3106/) Possible negative index value\. The value of 'index' index could reach \-1\. Animator\.cs 68

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

The analyzer is sure that the _index_ variable can end up with the value \-1\. This variable is assigned the value returned by the _FindClosestBeforeKeyFrame_ method, so let's take a look at it:

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

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 value _CueValue_ will 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 _time_: it's the _animationTime_ variable 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 by _FindClosestBeforeKeyFrame_ if 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:

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

**PVS\-Studio diagnostic message:** [V3117](https://pvs-studio.com/en/docs/warnings/v3117/) Constructor parameter 'phones' is not used\. Country\.cs 25

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

This 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 the _Phones_ variable\. Since the fix is obvious, I won't spell it out\.

**PVS\-Studio diagnostic message:** [V3080](https://pvs-studio.com/en/docs/warnings/v3080/) Possible null dereference\. Consider inspecting 'tabItem'\. TabItemContainerGenerator\.cs 22

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

The analyzer considers the dereference of the value returned by the _CreateContainer_ method unsafe\. Let's take a look at this method:

```cpp
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 can track an assignment of _null_ even 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:

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

**PVS\-Studio diagnostic message:** [V3142](https://pvs-studio.com/en/docs/warnings/v3142/) Unreachable code detected\. It is possible that an error is present\. DevTools\.xaml\.cs 91

There'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:

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

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](https://www.mono-project.com/docs/) 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\. 

**PVS\-Studio diagnostic message:** [V3009](https://pvs-studio.com/en/docs/warnings/v3009/) It's odd that this method always returns one and the same value of 'true'\. DataGridRows\.cs 412

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

The method returns _true_ all 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, _DataGrid_ 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\.

## Conclusion

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](https://en.wikipedia.org/wiki/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](https://pvs-studio.com/en/pvs-studio/download/) PVS\-Studio\!