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.

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

Analysis results

PVS-Studio diagnostic message: V3001 There are identical sub-expressions 'controlledFlags' to the left and to the right of the '^' operator. WindowImpl.cs 975TwitterClientMessageHandler.cs 52

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

1100 0011 | 1111 0000 ^ 1111 0000 

is equivalent to

1100 0011 | 0000 0000

The precedence of the exclusive OR ("^") is higher than that of the bitwise AND ("|"). The programmer didn't probably intend this order. The code can be fixed by enclosing the first expression in parentheses:

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 Possible null dereference of method return value. Consider inspecting: 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;
}

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:

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 Possible null dereference. Consider inspecting 'p'. VisualExtensions.cs 35
• 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:

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 Possible null dereference of method return value. Consider inspecting: TransformToVisual(...). ViewportManager.cs 381

This bug is very similar to the previous one:

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

This is the code of the TransformToVisual method:

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:

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 Possible null dereference. Consider inspecting 'transform'. MouseDevice.cs 80

PVS-Studio diagnostic message: V3022 Expression is always true. Probably the '&&' operator should be used here. NavigationDirection.cs 89

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 Possible incorrect order of arguments passed to 'SelectionChangedEventArgs' constructor: 'removedSelectedItems' and 'addedSelectedItems'. DataGridSelectedItemsCollection.cs 338

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:

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 Possible incorrect order of arguments passed to 'SelectionChangedEventArgs' constructor: 'removed' and 'added'. AutoCompleteBox.cs 707

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

It's the same constructor SelectionChangedEventArgs.

PVS-Studio diagnostic messages 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.

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:

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 The 'then' statement is equivalent to the 'else' statement. 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);
  }
}

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:

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 Possible negative index value. The value of 'index' index could reach -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]; 
    }
    ....
  }
  ....
}

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:

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:

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 Constructor parameter 'phones' is not used. 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;
}

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 Possible null dereference. Consider inspecting 'tabItem'. TabItemContainerGenerator.cs 22

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:

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:

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

PVS-Studio diagnostic message: 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:

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 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 It's odd that this method always returns one and the same value of 'true'. 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;
}

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 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 CICD 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!

Автор: HobbitSam

Источник