Небольшой вклад в борьбе Avalonia UI против зоопарка платформ

Эта статья – результат проверки проекта Avalonia UI с помощью статического анализатора PVS-Studio. Avalonia UI – это кроссплатформенная платформа пользовательского интерфейса на основе XAML, с открытым исходным кодом. Это один из технологически значимых проектов в истории .NET, так как позволяет создавать кроссплатформенные интерфейсы на основе WPF системы. Надеюсь, эта статья поможет авторам исправить некоторые ошибки и убедит использовать статические анализаторы в будущем.

О проекте Avalonia UI

Проект Avalonia UI (ранее назывался Perspex) предоставляет возможность для создания пользовательских интерфейсов, работающих на Windows, Linux и MacOS. Также есть экспериментальная на данный момент поддержка Android и iOS. Avalonia UI не является обёрткой над обёртками, а обращается к нативному API. В отличие от Xamarin Forms, оборачивающего Xamarin обёртки. В одном из демонстрационных видео меня поразила возможность вывести контрол в консоль Debian. Кроме того, проект предлагает больше возможностей для вёрстки и дизайна, чем у обычных конструкторов интерфейсов, благодаря использованию XAML разметки.

В качестве проектов, уже использующих Avalonia UI, можно привести AvalonStudio (кроссплатформенная IDE для разработки на C# и C/C++) и Core2D (редактор 2D-схем и диаграмм). В качестве коммерческого проекта можно привести Wasabi Wallet (биткойн-кошелек).

Борьба с необходимостью иметь по несколько различных библиотек при создании кроссплатформенного приложения имеет огромную важность. Нам хотелось помочь проекту, и я скачал проект и проверил нашим анализатором. Надеюсь, что авторы обратят внимание на данную статью и внесут необходимые правки в код, а может и внедрят регулярный статический анализ в процесс разработки. Для этого они могут воспользоваться вариантом бесплатного лицензирования PVS-Studio для открытых проектов. Регулярное использование статического анализатора помогает избежать множества проблем и сократит стоимость обнаружения и исправления многих ошибок.

Результаты проверки

Предупреждение PVS-Studio: 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;
  ....
}

Символично начну с нашей первой C# диагностики. Анализатор обнаружил странное применение оператора побитового ИЛИ. Поясню на цифрах, что тут происходит:

выражение

1100 0011 | 1111 0000 ^ 1111 0000 

аналогично вот такому:

1100 0011 | 0000 0000

Исключающее ИЛИ ("^") имеет более высокий приоритет, чем побитовое И ("|"). Скорее всего, тут подразумевался другой порядок операций. В данном случае следует взять первое выражение в скобки:

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

Перед следующими двумя предупреждениями должен признаться: срабатывания ложные. Это связано с использованием публичного API, метода TransformToVisual. В нашем случае VisualRoot всегда является родительским элементом для visual. Я не понял этого при анализе срабатывания, мне сказал об этом автор проекта уже после написания статьи. Так что предложенные в статье правки не для защиты от фактического падения, а от потенциальной правки, ломающей эту логику.

Предупреждение PVS-Studio: 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;
}

Небольшой метод. Анализатор считает, что разыменование результата вызова TranslatePoint небезопасно. Посмотрим на этот метод:

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

Действительно, есть возвращаемый null.

У этого метода 6 вызовов. В трёх случаях значение проверяется, а в остальных PVS-Studio обнаруживает потенциальное разыменование и выдаёт предупреждения. Первое я привёл выше, а другие два предупреждения находятся тут:

• V3080 Possible null dereference. Consider inspecting 'p'. VisualExtensions.cs 35
• V3080 Possible null dereference. Consider inspecting 'controlPoint'. Scene.cs 176

Исправлять предлагаю по аналогии, добавив внутри метода PointToClient проверку Nullable<Struct>.HasValue:

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

Случай, очень похожий на предыдущий пример:

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

Метод TransformToVisual выглядит так:

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

Кстати, метод FindCommonVisualAncestor действительно может вернуть null, как значение по умолчанию для ссылочных типов:

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

Метод TransformToVisual используется в девяти местах, проверки есть в семи. Первое предупреждение на использование без проверки находится выше, а последнее здесь:

V3080 Possible null dereference. Consider inspecting 'transform'. MouseDevice.cs 80

Предупреждение PVS-Studio: 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;
}

Странная проверка. В перечислении NavigationDirection 9 типов и PageDown является последним из них. Возможно, так было не всегда, или это страховка от ВНЕЗАПНОГО появления новых вариантов направления. Мне кажется, здесь достаточно первой проверки. Оставлю решение авторам проекта.

Предупреждение PVS-Studio: 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
      };
}

В данном случае анализатор предположил, что второй и третий аргументы конструктора перепутаны местами. Посмотрим на вызываемый конструктор:

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

Принимаются два контейнера типа IList, перепутать легко. Судя по комментарию в начале класса, это ошибка в коде контрола, скопированного у Microsoft, и доработанного под Avalonia. Но мне кажется, стоит поправить порядок аргументов метода, хотя бы чтобы не искать возможную ошибку у себя, когда придёт баг репорт.

Анализатор нашел ещё три подобных ошибки:

Предупреждение PVS-Studio: V3066 Possible incorrect order of arguments passed to 'SelectionChangedEventArgs' constructor: 'removed' and 'added'. AutoCompleteBox.cs 707

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

Тот же конструктор SelectionChangedEventArgs.

Предупреждения PVS-Studio 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

Два срабатывания в одном методе вызова события.

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

Анализатор увидел, что и в ItemsRepeaterElementIndexChangedEventArgs, и в методе Update аргументы 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;
}

Возможно, код писался разными программистами и для одного важнее, что было, а для другого – что будет :)

Как и в предыдущем случае, править сходу не стоит, надо проверить, действительно ли здесь есть ошибка.

Предупреждение PVS-Studio: 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);
  }
}

Крайне любопытная реализация метода ThenBy. В интерфейсе IEnumerable, от которого наследуется аргумент seq, есть метод ThenBy; полагаю, подразумевалось его использование. Вот так:

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

Анализатор полагает, что index может иметь значение: -1. Переменная получается из метода FindClosestBeforeKeyFrame, посмотрим на него:

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

Как мы видим, в цикле проверяется условие и возвращается предыдущее значение итератора. Условие довольно трудно проверить, и я не могу точно сказать, чему равен CueValue, но по описанию он принимает значение от 0.0 до 1.0. Можем кое-что сказать про time, это animationTime из вызывающего метода, он точно больше нуля и меньше единицы. Иначе выполнение программы пошло бы по другой ветке. Если это методы, вызываемые для отрисовки анимации, то ситуация выглядит как хороший плавающий баг. Я бы добавил проверку результата FindClosestBeforeKeyFrame, если в этом случае нужна особенная обработка. Или – если первый элемент не удовлетворяет каким-то другим условиям – убрать его из цикла. Не зная, как всё это должно работать, в качестве примера исправления выберу второй вариант:

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

Хороший пример отличия работы анализатора от ручного обзора кода. Тринадцать аргументов конструктора, один не используется. На самом деле Visual Studio тоже отмечает неиспользуемый аргумент, но на третьем уровне предупреждений (они часто бывают отключены). В данном случае это явная ошибка, потому что в классе также есть тринадцать свойств на каждый аргумент, и значение в Phones нигде не присваивается. Правка очевидна, не буду её приводить.

Предупреждение PVS-Studio: 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;
  ....
}

Анализатор посчитал опасным разыменование результата вызова CreateContainer.

Посмотрим на этот метод:

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

Анализатор может увидеть присвоение null в переменную даже при передаче значения через цепочку из пятидесяти методов. Но не может сказать, пойдёт ли выполнение по этой ветке хоть раз. Да и я тоже не смог, в общем-то… Вызовы метода теряются среди переопределённых и виртуальных методов. Так что предлагаю просто подстраховаться дополнительной проверкой:

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

Предупреждение PVS-Studio: V3142 Unreachable code detected. It is possible that an error is present. DevTools.xaml.cs 91

Не буду писать тут много кода для создания интриги, скажу сразу: предупреждение ложное. Анализатор увидел вызов метода, выбрасывающего безусловное исключение. Вот он:

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

Нельзя было не обратить внимание на тридцать пять предупреждений (!) о недостижимом коде, расположенном после вызовов этого метода. Я спросил у одного из разработчиков проекта: как это работает? И мне рассказали о способе заменять вызовы одного метода вызовами других при помощи библиотеки Mono.Cecil. Она позволяет заменять вызовы прямо в IL коде.

Анализатор не поддерживает эту библиотеку, следовательно, у нас есть много ложных срабатываний, значит эту диагностику на данном проекте лучше отключить. Немного неловко признаваться, именно я делал эту диагностику… но, как и любой инструмент, статический анализ нуждается в настройке.

Например, в настоящий момент мы разрабатываем диагностику о небезопасном приведении типов. И она даёт чуть меньше тысячи срабатываний на проекте игры, в которой контроль типизации осуществляется на стороне движка.

Предупреждение PVS-Studio: 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;
}

Метод всегда возвращает true. Возможно, назначение метода изменилось с момента написания сигнатуры, но скорее всего, это ошибка. Это тоже класс контрола, скопированного у Microsoft, судя по комментарию в начале класса. На мой взгляд, DataGrid – обычно один из самых нестабильных контролов, и мне кажется, тут стоит подумать, а надо ли подтверждать скролл, если он не удовлетворяет условиям?

Заключение

Некоторые из приведённых ошибок внесены в проект не самими разработчиками Avalonia UI, а кодом, скопированным из контролов WPF. Однако для пользователя интерфейса источник ошибки обычно не играет роли. Упавший или некорректно работающий интерфейс портит мнение обо всей программе.

В статье я упомянул необходимость настройки анализатора, есть ложно позитивные срабатывания, которые неизбежны в силу принципов работы алгоритмов статического анализа. Кто знаком с проблемой остановки знают о математических ограничениях при работе кода с другим кодом. Но в данном случае мы говорим об отключении одной диагностики из почти полутора сотен. Так что речь о потере смысла в статическом анализе не идет (либо вопрос не стоит). Кроме того, у этой диагностики могли быть и хорошие срабатывания, просто их тяжелее найти в массе ложно позитивных.

Обязательно надо отметить высокое качество кода проекта! Надеюсь, разработчики сохранят темп и уровень качества кода. К сожалению, чем больше проект, тем больше багов в нём. Один из возможных путей уменьшения багов – грамотная настройка CI\CD, с подключением статического и динамического анализов. А если вы хотите упростить работу с большими проектами и уменьшить количество времени, необходимого для отладки — скачайте и попробуйте PVS-Studio!

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Alexander Senichkin. Our Small Contribution to Avalonia UI's Fight for Fewer Platforms.