Stride – это мощный, бесплатный и активно развивающийся игровой движок, реализованный на C#. Он вполне может стать альтернативой Unity, но насколько качественный исходный код Stride? Узнаем это с помощью статического анализатора PVS-Studio.

Введение

Следствием активного развития проекта является не только внедрение нового функционала и возможностей, но и возникновение новых ошибок. В этой статье мы разберём самые интересные из них. К слову, разбор ошибок в проекте — отличный способ оценить его качество. Однако проверка крупных проектов отнюдь не простая задача, поэтому найти ошибки в игровом движке нам поможет статический анализатор PVS-Studio.

Проверка проводилась на релизе Stride Game Engine 4.1.0.1734. Исходный код доступен на GitHub.

Разбор ошибок Stride Game Engine

Использование анонимной функции для отмены подписки на событие

Issue 1

private void RemoveEditor(....)
{
  ....
  if (....)
  {
    multiEditor.OpenedAssets.CollectionChanged -= (_, e) => 
      MultiEditorOpenAssetsChanged(multiEditor, e);
    ....
  }
  ....
}

Срабатывание PVS-Studio: V3084. Anonymous function is used to unsubscribe from 'CollectionChanged' event. No handlers will be unsubscribed, as a separate delegate instance is created for each anonymous function declaration. AssetEditorsManager.cs 444

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

Риск null-разыменования при обращении к свойству

Issue 2

private void SetIsVisible(bool isVisible)
{
  if (isVisible)
    PreviewService.OnShowPreview(); //<=
  else
    PreviewService.OnHidePreview(); //<=
}

Срабатывание PVS-Studio: V3080. Possible null dereference. Consider inspecting 'PreviewService'. PreviewViewModel.cs 109

В данном случае анализатор предупреждает о возможном разыменовании нулевой ссылки. Обратите внимание на метод SetIsVisible. Здесь отсутствуют проверки PreviewService на null. На их необходимость указывает реализация этого свойства:

private IAssetPreviewService PreviewService
{
  get
  {
    if (previewService != null)
      return previewService;

    previewService = ServiceProvider.TryGet<IAssetPreviewService>();
    if (previewService == null)
      return null;                   

    previewService.PreviewAssetUpdated += PreviewAssetUpdated;
      return previewService;
  }
}

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

private void SetIsVisible(bool isVisible)
{
  if (PreviewService != null)       //<=
  {
    if (isVisible)
      PreviewService.OnShowPreview(); 
    else
      PreviewService.OnHidePreview(); 
  }
}

Лишнее присваивание значения

Issue 3

public TexAtlas(...., TexImage atlas) : base(....)
{
  ....
  Name = atlas.Name;
  ....
  Name = "";
}

Срабатывание PVS-Studio: V3008. The 'Name' variable is assigned values twice successively. Perhaps this is a mistake. TexAtlas.cs 48, 43

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

Потерялась проверка на null

Issue 4

public override void EndScreenDeviceChange(....)
{
  ....
  window.FullscreenIsBorderlessWindow = FullscreenIsBorderlessWindow; // <=
  if (....)
  {
    ....
    if (window != null)
    {
      window.ClientSize = new Size2(clientWidth, clientHeight);
    }
    ....
  }
  else
  {
    ....
    if (window != null)
    {
      window.ClientSize = new Size2(clientWidth, clientHeight);
      window.Location = savedFormLocation;
      ....
      window.BringToFront();
    }

  }
  ....
}

Срабатывание PVS-Studio: V3095. The 'window' object was used before it was verified against null. GameWindowSDL.cs 64, 70.

В начале метода EndScreenDeviceChange объект window используется без предварительной проверки на null, о чём и предупреждает анализатор. На актуальность этого предупреждения указывает последующий код: все обращения к переменной выполняются внутри блока условного оператора, реализующего такую проверку.

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

Issue 5

private async Task Update()
{
  while (....)
  {
    ....
    if (....)
    {
      ....
      materialFilterRenderFeature.MaterialFilter =
        (materialFilterRenderFeature != null && ....)? ....
      ....
    }
  }
}

Срабатывание PVS-Studio: V3095. The 'materialFilterRenderFeature' object was used before it was verified against null. EditorGameRenderModeService.cs 106, 107.

В отличие от предыдущего случая, в этом примере есть проверка на null, но реализована она несколько... странно. Выполняется обращение к свойству MaterialFilter объекта materialFilterRenderFeature. Далее этому свойству присваивается результат выполнения тернарного оператора. Условное выражение этого оператора содержит проверку materialFilterRenderFeature на null. Однако в ней нет никакой необходимости. В случае если materialFilterRenderFeature равен null, выполнение программы благополучно завершится исключением NullReferenceException ещё при обращении к свойству MaterialFilter :).

Когда проверка на null приводит к NullReferenceException

Issue 6

public Palette GlobalPalette
{
  get {....}
  set
  {
    SetTagValue("GlobalPalette", (value != null) ? null : value.Data);
  }
}

Срабатывание PVS-Studio: V3080. Possible null dereference. Consider inspecting 'value'. MetadataModels.cs 132

Методу SetTagValue в качестве второго аргумента передаётся тернарное выражение. Это выражение проверяет наличие значения null у переменной value. Если value равно null, в качестве аргумента методу передаётся value.Data. Обращение к свойству этой переменной неизбежно приведёт к вызову исключения NullReferenceException. Предположительно, разработчик перепутал операнды местами. Таким образом, корректная реализация тернарного оператора может выглядеть так:

(value != null) ? value.Data : null

Одинаковое условие, но разное предназначение

Issue 7

internal IObjectNode SetTarget(....)
{
  if (....)
  {
    var targetValue = targetNode.Retrieve();

    if (targetValue != null && !type.IsInstanceOfType(targetValue))
      throw new InvalidOperationException(@"The type of the retrieved" +
                                           "node content does not match" +
                                           "the type of this reference");

    if (targetValue != null && !type.IsInstanceOfType(targetValue))
      throw new InvalidOperationException("TargetNode type does not " +
                                          "match the reference type.");

    ....
  }
  ....
}

Срабатывание PVS-Studio: V3021. There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless.ObjectReference.cs 134

Анализатор обнаружил два похожих оператора if с идентичными условиями. В теле обоих условных операторов вызывается исключение, тем самым прерывая выполнение программы. На первый взгляд, здесь нет ничего страшного, просто немного лишнего кода. Однако обратите внимание на сообщения, передаваемые в исключения, — они разные. Можно предположить, что разработчик хотел реализовать две различные проверки. Возможно, он скопировал одну из них и поменял сообщение, но забыл изменить условное выражение. Таким образом, одна из возможных ошибок осталась необработанной и всё ещё представляет угрозу для корректной работы программы.

Исходя из сообщения второго исключения, можно предположить, что корректная реализация соответствующего if должна выглядеть так:

if (targetNode!= null && !type.IsInstanceOfType(targetNode))
  throw new InvalidOperationException("TargetNode type does not " +
                                      "match the reference type.");

Рассмотрим аналогичный случай, найденный анализатором в этом игровом движке.

Issue 8

private GraphViewModel(....) : base(serviceProvider)
{
  ....
  if (rootPresenters == null) 
    throw new ArgumentNullException(nameof(rootPresenters));
  ....
  if (rootPresenters == null) 
    throw new ArgumentNullException(nameof(rootNode));
  ....
}

Срабатывание PVS-Studio: V3021. There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless. GraphViewModel.cs 40

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

if (rootNode == null)
  throw new ArgumentNullException(nameof(rootNode));

Copy paste ошибки или какой case лишний?

Issue 9

public override void AddPoint(....)
{
  ....
  switch (Component)
  {
    case VectorComponent.X:
      value.X = realPoint.Y;
      break;

    case VectorComponent.Y:
      value.Y = realPoint.Y;
      break;

    case VectorComponent.Z:
      value.Z = realPoint.Y; // <=
      break;

    case VectorComponent.W:
      value.Z = realPoint.Y; // <=
      break;

    default:
      throw new NotSupportedException();
  }
  ....
}

Срабатывание PVS-Studio: V3139. Two or more case-branches perform the same actions. RotationComponentCurveViewModel.cs 53

Перед вами switch, операторы case которого очень похожи друг на друга. Метки каждого из них соответствуют шаблону: VectorComponent.{0}, где вместо {0} может быть любое из следующих свойств: X, Y, Z или W. В теле каждого оператора case выполняется присваивание вида value.{0} = realPoint.Y. А теперь обратите внимание на последний из них. Он имеет метку VectorComponent.W, а в теле производится операция value.Z = realPoint.Y. Она идентична присваиванию в предыдущем case. Согласно выявленному нами шаблону, вместо value.Z в данном выражении должно быть value.W.

Таким образом, корректная реализация последнего case может выглядеть так:

case VectorComponent.W:
  value.W = realPoint.Y; 
  break;

Не стоит жалеть кода на обработку NullReferenceException

Issue 10

protected override Size ArrangeOverride(....)
{
  ....
  if (....)
  {
    for (....)
    {
      var child = itemsControl.ItemContainerGenerator
                              .ContainerFromIndex(i) as TreeViewItem;
      ....
      if (child != null)
      {
        child.Arrange(....);
        currentY += child.ActualHeight;
      }
      ....
    }
  }
  else
  {
    for (....)
    {
      var child = itemsControl.ItemContainerGenerator
                              .ContainerFromIndex(i) as UIElement;
            
      child?.Arrange(....);
      currentY += child.DesiredSize.Height; //<=
    }
  }
  ....
}

Срабатывание PVS-Studio: V3125. The 'child' object was used after it was verified against null. VirtualizingTreePanel.cs 430, 429

Обратите внимание на блоки then и else условного оператора. Они чем-то похожи, не так ли? В обоих выполняется цикл, внутри которого переменная child инициализируется с помощью оператора as. Но есть одно существенное отличие – в первом цикле все обращения к переменной child защищены от NullReferenceException соответствующей проверкой, а вот во втором случае – не совсем. Можно предположить, что разработчик намеренно не стал добавлять явную проверку, обработав первое обращение к child оператором null-conditional:

child?.Arrange(....);

При этом он не обратил внимания на второе обращение:

currentY += child.DesiredSize.Height;

Результатом этой операции все также может стать выбрасывание исключения NullReferenceException.

Заключение

Потенциально опасные фрагменты кода, найденные в Stride Game Engine, оказались вполне интересными и определённо заслуживают внимания. Но насколько же оказались верны срабатывания анализатора? Мне кажется, точно ответить на этот вопрос могут только разработчики игрового движка, проведя собственное исследование с помощью PVS-Studio.

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

Вы легко можете в этом убедиться, скачав PVS-Studio и опробовав его на своих проектах в течение пробного периода.

Для наилучшего пользовательского опыта рекомендую ознакомиться с основами использования анализатора. Это можно сделать, прочитав соответствующие разделы в документации.

На этом статья завершается. Надеюсь, она была вам интересна :).

Чистого вам кода и успешных проектов! До встречи в следующих статьях!

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Moskalev. Stride Game Engine error review.