Работа с графикой невозможна без специальных инструментов — графических редакторов. Но что если ваш редактор вылетит прям во время работы из-за багов? Давайте с помощью статического анализатора поищем потенциальные ошибки и странные места в исходном коде открытого проекта PixiEditor.

Введение

Что же такое PixiEditor? Легко догадаться, что это 2D-редактор , который ориентирован преимущественно на пиксельную графику. Однако он также подходит для процедурной графики, редактирования изображений, векторной графики и анимации. Стоит отметить, что это open source проект, который активно обновляется, и буквально любой может приложить руку к его дальнейшему развитию.

Не могу не упомянуть крутую фишку PixiEditor — Node Graph. Благодаря этому инструменту можно буквально программировать сцену.

Команда PVS-Studio активно участвует в развитии открытых проектов и помогает повышать качество и надёжность их кода. Поэтому давайте поищем ошибки в этом редакторе версии 2.0.1.15 с помощью нашего статического анализатора, затем откроем issue и расскажем разработчикам о потенциальных проблемах.

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

Проверка будет осуществляться с помощью статического анализатора PVS-Studio версии 7.38.

Проверяем проект

Фрагмент кода 1

private void WriteName(StringBuilder sb, string name)
{
  sb.Append('"');
  sb.Append(name);
  sb.Append('"');

  for (int j = name.Length; j < name.Length; j++)
  {
    sb.Append(' ');
  }
}

Сообщение PVS-Studio: V3028 Consider inspecting the 'for' operator. Initial and final values of the iterator are the same. CorelDrawPalParser.cs 191

В качестве начального и конечного значения итератора задано одно и то же значение. Следовательно, код внутри цикла никогда не отработает.

Почему так произошло? Возможно, разработчик, хотел начать итерации с name.Length и закончить нулём, но опечатался и в условии также написал name.Length. Однако не будем спешить с выводами, лучше давайте посмотрим единственное место, в котором вызывается этот метод.

public override async Task<bool> Save(string path, PaletteFileData data)
{
  StringBuilder sb = new StringBuilder(SWATCH_NAME.Length + 20);

  try
  {
    await using (Stream stream = File.OpenWrite(path))
    {
      using (TextWriter writer = new StreamWriter(stream, 
                                                  Encoding.ASCII, 
                                                  1024, 
                                                  true))
      {
        foreach (var color in data.Colors)
        {
          this.ConvertRgbToCmyk(color, 
                                out byte c, 
                                out byte m, 
                                out byte y, 
                                out byte k);

          this.WriteName(sb, SWATCH_NAME);
          this.WriteNumber(sb, c);
          this.WriteNumber(sb, m);
          this.WriteNumber(sb, y);
          this.WriteNumber(sb, k);

          writer.WriteLine(sb.ToString());
          sb.Length = 0;
        }
      }
    }
    return true;
  }
  catch
  {
    return false;
  }
}

Метод осуществляет сохранение палитры с цветами в файл. Каждый цвет переводится в CMYK (Cyan, Magenta, Yellow, Key/Black) схему и сохраняется в таком формате:

"PixiEditor Color"  0   0   0 100

Теперь мы можем прийти к выводу, что добавление символа пробела в строку в методе WriteName ненужно, поэтому цикл for является лишним. И хоть код внутри него никогда не вызывается, это портит его качество и читаемость. Сделать код чище можно следующим образом:

private void WriteName(StringBuilder sb, string name)
{
  sb.Append('"');
  sb.Append(name);
  sb.Append('"');
}

Фрагмент кода 2

public bool IsPointInside(VecD point)
{
  var top = TopLeft - TopRight;
  var right = TopRight - BottomRight;
  var bottom = BottomRight - BottomLeft;
  var left = BottomLeft - TopLeft;

  var deltaTopLeft = point - TopLeft;
  var deltaTopRight = point - TopRight;
  var deltaBottomRight = point - BottomRight;
  var deltaBottomLeft = point - BottomLeft;

  if (   deltaTopRight.IsNaNOrInfinity() 
      || deltaTopLeft.IsNaNOrInfinity() 
      || deltaBottomRight.IsNaNOrInfinity()
      || deltaBottomRight.IsNaNOrInfinity())
        return false;

  ....
}

Сообщение PVS-Studio: V3001 There are identical sub-expressions 'deltaBottomRight.IsNaNOrInfinity()' to the left and to the right of the '||' operator. ShapeCorners.cs 155

Анализатор обнаружил использование в условии двух одинаковых подвыражений deltaBottomRight.IsNanOrInfinity(). Скорее всего, произошла опечатка, и на самом деле вместо второго подвыражения должно быть deltaBottomLeft.IsNanOrInfinity(), поскольку deltaBottomLeft сейчас не используется:

public bool IsPointInside(VecD point)
{
    ....

  if (   deltaTopRight.IsNaNOrInfinity() 
      || deltaTopLeft.IsNaNOrInfinity() 
      || deltaBottomRight.IsNaNOrInfinity()
      || deltaBottomLeft.IsNaNOrInfinity())
        return false;

  ....
}

Фрагмент кода 3

public override RectD? GetPreviewBounds(int frame, 
                                        string elementToRenderName = "")
{
  ....

  RectD? totalBounds = null;

  if (   Top.Connection != null 
      && Top.Connection.Node is IPreviewRenderable topPreview)
  {
    var topBounds = topPreview.GetPreviewBounds(frame, elementToRenderName);
    if (topBounds != null)
    {
      totalBounds = totalBounds?.Union(topBounds.Value) ?? topBounds;
    }
  }

  ....
}

Сообщение PVS-Studio: V3022 Expression 'totalBounds' is always null. The operator '?.' is meaningless. MergeNode.cs 87

В этом случае totalBounds всегда будет null до момента его переприсваивания, потому что переменная инициализирована как RectD? totalBounds = null. Поэтому в выражении totalBounds?.Union(topBound.Value) нет смысла.

Фрагмент кода 4

private List<IChangeInfo> ApplyToFrame(Document target, 
                                       StructureNode targetLayer, 
                                       int frame)
{
  ....

  foreach (var guid in ordererd)
  {
    var layer = target.FindMemberOrThrow<StructureNode>(guid);

    AddMissingKeyFrame(targetLayer, frame, layer, changes, target);

    if (layer is not IRasterizable or ImageLayerNode)
      continue;

    if (layer is ImageLayerNode imageLayerNode)
    {
      ....
    }
        ....
    }
}

Сообщение PVS-Studio: V3207 The 'not IRasterizable or ImageLayerNode' logical pattern may not work as expected. The 'not' pattern is matched only to the first expression from the 'or' pattern. CombineStructureMembersOnto_Change.cs 160

Разработчик при написании условия допустил логическую ошибку. Сейчас оно работает следующим образом: условие вернёт true, если layer НЕ реализует интерфейс IRasterizable или layer-экземпляр класса ImageLayerNode. В таком случае следующее за ним условие layer is ImageLayerNode всегда будет ложно. Его можно исправить следующим образом:

if (layer is not IRasterizable or not ImageLayerNode)
      continue;

Такой паттерн ошибки достаточно распространён. Недавно подобная ошибка была обнаружена в файловом менеджере Files, о чём мы написали в нашей статье. Всему виной то, что в нашем естественном языке отрицательная частица not применяется ко всем словам, которые мы перечисляем за ней. В C# же not является унарным оператором, который имеет соответственный приоритет и будет применён до or или and. Почитать об этом поведении можно в документе.

Фрагмент кода 5

public class CommandNameListGenerator : IIncrementalGenerator
{
  private const string Commands = 
  "PixiEditor.Models.Commands.Attributes.Commands";

  private const string Evaluators = 
  "PixiEditor.Models.Commands.Attributes.Evaluators";

  private const string Groups = 
  "PixiEditor.Models.Commands.Attributes.Commands";

  private const string InternalNameAttribute = 
  "PixiEditor.Models.Commands.Attributes.InternalNameAttribute";

  ....
}

Сообщение PVS-Studio: V3091 Empirical analysis. It is possible that a typo is present inside the string literal. The 'Commands' word is suspicious. CommandNameListGenerator.cs 17

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

Фрагмент кода 6

public static Tweener<double> Double(....)
{
    return new Tweener<double>(property, control, endValue, duration,
        (start, end, t) => start + (end - start) * easing?.Ease(t) ?? t);
}

Сообщение PVS-Studio: V3123 Perhaps the '??' operator works in a different way than it was expected. Its priority is lower than priority of other operators in its left part. Tweener.cs 14

Здесь бинарное выражение, которое может дать не тот результат из-за путаницы в приоритете операций. Сначала будут применены арифметические операторы, в результате которых потенциально получится null, а затем будет применён оператор ??, и всё выражение вернёт t. Короче говоря, выражение будет обработано таким образом:

(start + (end - start) * easing?.Ease(t)) ?? t

Возможно, правильная логика заключается в том, чтобы сначала обработать выражение easing?.Ease(t) ?? t, а затем арифметические операции слева:

start + (end - start) * (easing?.Ease(t) ?? t)

Для лучшего понимания в таком случае рекомендуется использовать скобки.

Фрагмент кода 7

private static void UseLanguageFlowDirectionPropertyChanged(....)
{
  ....

  if (!obj.NewValue.Value)
  {
    obj.Sender.SetValue(Control.FlowDirectionProperty, 
                        FlowDirection.LeftToRight);
    ILocalizationProvider.Current.OnLanguageChanged -= 
    (lang) => OnLanguageChangedFlowDirection(obj.Sender);
  }
  else
  {
    OnLanguageChangedFlowDirection(obj.Sender);
    ILocalizationProvider.Current.OnLanguageChanged += 
    (lang) => OnLanguageChangedFlowDirection(obj.Sender);
  }
}

Сообщение PVS-Studio: V3084 Anonymous function is used to unsubscribe from 'OnLanguageChanged' event. No handlers will be unsubscribed, as a separate delegate instance is created for each anonymous function declaration. Translator.cs 78

Приведённая выше отписка от события не будет работать. Она происходит с помощью анонимной функции и приводит к созданию отдельного экземпляра делегата, отличного от того, который используется ниже в подписке. Таким образом, для подписки используется один делегат, а для отписки совсем другой, несмотря на то, что визуально они ничем не отличаются.

Для того, чтобы корректно использовать анонимные функции при подписке на события (если потребуется дальнейшая отписка), можно сохранять обработчик-лямбду в отдельную переменную и использовать её как при подписке, так и при отписке от события.

Фрагмент кода 8

public static async void CommandChanged(AvaloniaPropertyChangedEventArgs e)
{
  ....

  RelayCommand<object> wrapper = new RelayCommand<object>(parameter =>
  {
    if (!ShortcutController.ShortcutExecutionBlocked)
    {
      if (   command?.Shortcut != null 
          && command.Shortcut.Gesture != null 
          && command.Shortcut.Gesture.Key != Key.None 
          || command.Shortcut.Gesture.KeyModifiers != KeyModifiers.None)
      {
        ViewModelMain.Current.ShortcutController.KeyPressed(
        false,
        command.Shortcut.Key,
        command.Shortcut.Modifiers);
      }
      else
      {
        if (iCommand.CanExecute(parameter))
        {
          iCommand.Execute(parameter);
        }
      }       
    }
    
    ....
       
  });

  ....
}

Сообщение PVS-Studio: V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'command' object NativeMenu.cs 68

Анализатор сообщает нам о возможном NullReferenceException из-за разыменования переменной command, которая может оказаться null. Первое условие в if учитывает, что command?.Shortcut может быть null, поэтому в других условиях, объединённых с помощью &&, опасное разыменование не возникнет. Но это не учтено в условии command.Shortcut.Gesture.KeyModifiers != KeyModifiers.None, которое связано с другими с помощью ||. Исправим, добавив в выражение оператор ?:

if (   command?.Shortcut != null 
    && command.Shortcut.Gesture != null 
    && command.Shortcut.Gesture.Key != Key.None 
    || command?.Shortcut?.Gesture.KeyModifiers != KeyModifiers.None)
{
  ....
}

Или можно правильно расставить скобки подобным образом:

if (   command?.Shortcut != null && 
    (command.Shortcut.Gesture != null 
     && command.Shortcut.Gesture.Key != Key.None 
     || command.Shortcut.Gesture.KeyModifiers != KeyModifiers.None))
{
  ....
}

В таком случае, если command?.Shortcut будет null, то другие условия не будут проверяться, и разыменования не возникнет.

Фрагмент кода 9

private void PointerMoved(object? sender, PointerEventArgs e)
{
  if (targetStart > draggedStart && draggedDeltaEnd >= targetMid)
  {
    ....
  }
  else if (targetStart < draggedStart && draggedDeltaStart <= targetMid)
  {
    ....
  }
  else
  {
    if (orientation == Orientation.Horizontal)
    {
        SetTranslateTransform(targetContainer, 0, 0);          // <=
    }
    else
    {
        SetTranslateTransform(targetContainer, 0, 0);          // <=
    }
  }
}

Сообщение PVS-Studio: V3004 The 'then' statement is equivalent to the 'else' statement. DockableTabDragBehavior.cs 431

Странно, что then-выражение такое же, как у else. Скорее всего, строку SetTranslateTransorm(targetContainer, 0, 0) в else-ветке забыли изменить после копипаста. На это также указывают похожие куски кода выше, в которых есть такое же условие orientation == Orientation.Horizontal:

if (targetStart > draggedStart && draggedDeltaEnd >= targetMid)
{
  if (orientation == Orientation.Horizontal)
  {
    SetTranslateTransform(targetContainer, -draggedBounds.Width, 0);
  }
  else
  {
    SetTranslateTransform(targetContainer, 0, -draggedBounds.Height);
  }
  ....
}
else if (targetStart < draggedStart && draggedDeltaStart <= targetMid)
{
  if (orientation == Orientation.Horizontal)
  {
    SetTranslateTransform(targetContainer, draggedBounds.Width, 0);
  }
  else
  {
    SetTranslateTransform(targetContainer, 0, draggedBounds.Height);
  }
  ....
}

Возможно, что в любом случае действительно должно быть SetTranslateTransorm(targetContainer, 0, 0), и условие избыточно.

Фрагмент кода 10

protected override void OnSelected(bool restoring)
{
  if (restoring) return;

  var document = ViewModelMain.Current?.DocumentManagerSubViewModel
                                       .ActiveDocument;
  document.Tools.UseRasterLineTool();
}

Сообщение PVS-Studio: V3105 The 'document' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. RasterLineToolViewModel.cs 69

Переменная document инициализирована с помощью оператора ?, поэтому она может быть null, и в результате последующего разыменования может возникнуть NullReferenceException. Обезопасить код можно с помощью проверки document на null:

protected override void OnSelected(bool restoring)
{
  if (restoring) return;

  var document = ViewModelMain.Current?.DocumentManagerSubViewModel
                                       .ActiveDocument;

  if (document != null)
    document.Tools.UseRasterLineTool();
}

Фрагмент кода 11

public override object ProvideValue(IServiceProvider serviceProvider)
{
    return (ViewModelMain.Current?.ToolsSubViewModel.ActiveToolSet?.Tools)
            .FirstOrDefault(tool => tool.GetType().Name == TypeName);
}

Сообщение PVS-Studio: V3153 Dereferencing the result of null-conditional access operator can lead to NullReferenceException. Consider removing parentheses around null-conditional access expression. ToolVM.cs 14

Разыменование ViewModelMain.Current?.ToolsSubViewModel.ActiveToolSet?.Tools может привести к выбросу исключения, поскольку выражение получено с помощью оператора ?. Можно не допустить этого, добавив оператор ?:

public override object ProvideValue(IServiceProvider serviceProvider)
{
    return (ViewModelMain.Current?.ToolsSubViewModel.ActiveToolSet?.Tools)
            ?.FirstOrDefault(tool => tool.GetType().Name == TypeName);
}

Фрагмент кода 12

public void DrawLine(VecD from, VecD to, Paint paint)
{
  ShapeCorners corners = new ShapeCorners(from, to, paint.StrokeWidth);
  IDisposable? reset = null;
  if (paint?.Paintable != null)
  {
    ....
  }

  ....
}

Сообщение PVS-Studio: V3095 The 'paint' object was used before it was verified against null. Check lines: 272, 274. Canvas.cs 272

Здесь разработчик предполагает, что параметр paint может оказаться null, и проверяет это с помощью условия paint?.Paintable != null. Но до этого условия происходит разыменование paint.StrokeWidth, и может возникнуть NullReferenceException.

Фрагмент кода 13

protected override void OnApplyTemplate(TemplateAppliedEventArgs e)
{
  base.OnApplyTemplate(e);
  var popupPart = e.NameScope.Find<Popup>("popup");
  popupPart.Closed += PopupPartOnClosed;
  if (popupPart != null)
  {
    popupPart.PointerPressed += (sender, args) => { args.Handled = true; };
  }
}

Сообщение PVS-Studio: V3095 The 'popupPart' object was used before it was verified against null. Check lines: 90, 91. PortableColorPicker.cs 90

Та же ситуация, как в предыдущем фрагменте кода. Следует сначала проверить переменную popupPart на null, а затем произвести разыменование:

protected override void OnApplyTemplate(TemplateAppliedEventArgs e)
{
  base.OnApplyTemplate(e);
  var popupPart = e.NameScope.Find<Popup>("popup");

  if (popupPart != null)
  {
    popupPart.Closed += PopupPartOnClosed;
    popupPart.PointerPressed += (sender, args) => { args.Handled = true; };
  }
}

Подводим итоги

Проект достаточно хорошо написан и содержит не так много багов. И большинство ошибок было допущено, скорее всего, по невнимательности.

Однако следует понимать, что с увеличением кодовой базы проекта количество багов и ошибок также будет расти. Поэтому будет очень полезно использовать инструменты статического анализа.

Стоит отметить, что наш анализатор PVS-Studio можно использовать совершенно бесплатно. Если вы участвуете в разработке открытого проекта (например, PixiEditor), то можете получить бесплатную версию. Подробности описаны в статье про бесплатное лицензирование.

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

Берегите себя и свой код!

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Georgii Tormozov. Pixel to pixel: Checking the PixiEditor project.