Артём Ровенский

03 Апр 2024

Теги:

#CSharp #GameDev

Быстро и легко ищем баги в играх на Unity (для C# разработчиков)

03 Апр 2024

Автор: Артём Ровенский

Как получить Best Warnings?
Разберём предупреждения
Заключение

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

Игровая индустрия становится всё больше и больше. С каждым годом растёт стоимость разработки, и вместе с ней цена на сами игры. У нас есть статья на эту тему: "Зачем разработчикам игр на Unity использовать статический анализ?".

Когда вы её прочитали и поняли, какую пользу разработчики игр получают от инструментов статического анализа кода, поговорим про Best Warnings. Это один из способов быстрого знакомства со статическим анализатором. "А зачем это знакомство вообще нужно?" — спросите вы.

Отвечаем. Часто на крупных проектах отчёт анализатора может содержать сотни или даже тысячи предупреждений. Разбираться с ними бывает довольно трудно. Когда вы уже выбрали статический анализатор, неактуальные предупреждения можно просто подавить и видеть только новые. Но что, если вы только знакомитесь с инструментом? Для таких ситуации и создавался механизм Best Warnings.

Как получить Best Warnings?

Для примера возьмём Open Source проект TowerDefense-GameFramework-Demo. Его исходный код доступен на GitHub. К сожалению, очень мало игр на Unity имеют открытый исходный код, который нам как раз и нужен для демонстрации. Но даже на этом небольшом проекте мы всё равно увидим работу Best Warnings.

Первым делом у вас уже должен быть установлен анализатор и введена лицензия. Если же вы ещё этого не сделали, то вам поможет эта страница.

Откройте ваш проект в Unity Hub. Далее в Unity нажмите: Assets > Open C# Project. У вас откроется IDE, заданная в настройках редактора Unity. Она указывается здесь:

Я буду использовать IDE Visual Studio 2022, в которой привык работать. Чтобы проанализировать проект в этой версии IDE, можно использовать пункт меню Extensions > PVS-Studio > Check Solution.

После анализа потребуется лишь нажать на кнопку Best. После этого вы увидите выборку из 10 предупреждений анализатора:

Разберём предупреждения

Как мы уже знаем, раздел Best Warnings содержит 10 предупреждений, указывающих на код, который, вероятнее всего, содержит ошибку. Рассмотрим все 10. Спойлер: будут и реальные ошибки, и просто подозрительный код, и, конечно же, False Positive предупреждения. Естественно, что ложноположительные срабатывания — это нормально для технологии статического анализа. Главное, чтобы процент подобных предупреждений был небольшим. Ну что ж, приступим!

Фрагмент 1

public void OnRecycle()
{
  transform.SetParent(initRoot, false);
  transform.localPosition = transform.localPosition;
  transform.eulerAngles = initRotation;
  transform.localScale = initScale;
  ....
}

Предупреждение PVS-Studio: V3005 The 'transform.localPosition' variable is assigned to itself. Item.cs 150

Как видно из кода, свойство transform.localPosition присваивается само себе. Это довольно распространённая ошибка, которую мы часто видим в анализируемых проектах.

Фрагмент 2

public static int Read7BitEncodedInt32(this BinaryReader binaryReader)
{
  int value = 0;
  int shift = 0;
  byte b;
  do
  {
    if (shift >= 35)
    {
      throw new GameFrameworkException("7 bit encoded int is invalid.");
    }

    b = binaryReader.ReadByte();
    value |= (b & 0x7f) << shift;
    shift += 7;
  } while ((b & 0x80) != 0);

  return value;
}

Предупреждение PVS-Studio: V3134 Shift by [32..34] bits is greater than the size of 'Int32' type of expression '(b & 0x7f)'. BinaryExtension.cs 37

А вот в этом случае анализатор ошибся. Он говорит о неправильном сдвиге. К сожалению, анализатор не увидел, что shift каждый раз увеличивается на 7, т.е. значений 32, 33 и 34 у него быть не может. Код работает как надо, но могу добавить, что проверку shift >= 35 можно заменить на shift >= 32. Тогда другим разработчикам и анализатору будут понятны возможные диапазоны значений.

Если вы не знаете, как работает анализ потока данных, или вам интересно узнать про технологии, используемые в PVS-Studio, то предлагаю прочитать нашу статью.

Фрагмент 3

public void RemoveData(Data data)
{
  if (data == null)
  {
    throw new GameFrameworkException(Utility.Text.Format("Data '{0}' is null.", 
                                                         data.Name.ToString()));
  }

  ....
}

Предупреждение PVS-Studio: V3080 Possible null dereference. Consider inspecting 'data'. DataManager.cs 122

Разработчик проверяет параметр на валидность. Если параметр data равен null, то генерируется исключение. Вот только при формировании сообщения исключения используется data, который как раз равен null. В итоге вместо GameFrameworkException получаем исключение типа NullReferenceException.

Фрагмент 4

internal override void Update(float elapseSeconds, float realElapseSeconds)
{
  while (m_RecycleQueue.Count > 0)
  {
    EntityInfo entityInfo = m_RecycleQueue.Dequeue();
    IEntity entity = entityInfo.Entity;
    EntityGroup entityGroup = (EntityGroup)entity.EntityGroup;
    if (entityGroup == null)
    {
      throw new GameFrameworkException("Entity group is invalid.");
    }

    entityInfo.Status = EntityStatus.WillRecycle;
    entity.OnRecycle();
    entityInfo.Status = EntityStatus.Recycled;
    ....
  }

  ....
}

Предупреждение PVS-Studio: V3008 The 'entityInfo.Status' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 176, 174. EntityManager.cs 176

В этом случае анализатор ошибся. Он не увидел, что переменные entityInfo и entity связаны друг с другом. Выписали себе на доработку.

Фрагмент 5

public static FileSystem Load(....)
{
  ....

  for (int i = 0; i < fileSystem.m_BlockDatas.Count; i++)
  {
    BlockData blockData = fileSystem.m_BlockDatas[i];
    if (blockData.Using)
    {
      StringData stringData = fileSystem.ReadStringData(blockData.StringIndex);
      fileSystem.m_StringDatas.Add(blockData.StringIndex, stringData);
      fileSystem.m_FileDatas.Add(stringData.GetString(....);          // <=
    }
    else
    {
      fileSystem.m_FreeBlockIndexes.Add(blockData.Length, i);
    }
  }

  return fileSystem;
}

Предупреждение PVS-Studio: V3156 The first argument of the 'Add' method is not expected to be null. FileSystem.cs 206

При разработке анализатора учитывается логика работы некоторых популярных методов. Этот механизм мы называем "аннотирование методов". Как раз здесь анализатор использует информацию, полученную с помощью этого механизма. Анализатор знает, что метод Dictionary<TKey, TValue>.Add не может принимать null.

Взглянем на метод GetString. Результат его работы передаётся в метод Add.

public string GetString(byte[] encryptBytes)
{
    if (m_Length <= 0)
    {
        return null;
    }

  ....
}

Из всего метода нам интересно только начало, когда возвращается null. В результате, если m_Length будет иметь значение меньше или равное 0, то будет выброшено исключение типа ArgumentNullException.

Фрагмент 6

private void RefreshCheckInfoStatus()
{
  ....
  IFileSystem fileSystem = m_ResourceManager.GetFileSystem(....);
  if (!fileSystem.SaveAsFile(resourceFullName, resourcePath))
  {
    throw new GameFrameworkException(Utility.Text.Format(
      "Save as file '{0}' to '{1}' from file system '{2}' error.",
      resourceFullName,
      fileSystem.FullPath));
  }

  fileSystem.DeleteFile(resourceFullName);
  ....
}

Предупреждение PVS-Studio: V3025 The 1st argument '"Save as file '{0}' to '{1}' from file system '{2}' error."' is used as incorrect format string inside method. A different number of format items is expected while calling 'Format' function. Format items not used: {2}. ResourceManager.ResourceChecker.cs 152

А здесь видно работу межпроцедурного анализа. Анализатор "проследил" за строкой формата с тремя плейсхолдерами для аргументов.

public static string Format(string format, object arg0, object arg1)
{
  if (format == null)
  {
    throw new GameFrameworkException("Format is invalid.");
  }

  CheckCachedStringBuilder();
  s_CachedStringBuilder.Length = 0;
  s_CachedStringBuilder.AppendFormat(format, arg0, arg1);  // <=
  return s_CachedStringBuilder.ToString();
}

Эта строка формата используется в качестве первого аргумента метода StringBuilder.AppendFormat. Как мы уже знаем, плейсхолдера три, а вот аргументов всего два. В результате использования такой строки формата будет выброшено исключение типа FormatException.

Предположу, что подобная ошибка могла возникнуть из-за copy-paste. Ниже в коде есть похожее место:

if (!fileSystem.WriteFile(resourceFullName, resourcePath))
{
  throw new GameFrameworkException(Utility.Text.Format(
    "Write resource '{0}' to file system '{1}' error.",
    resourceFullName,
    fileSystem.FullPath));
}

Разработчик мог просто скопировать похожую обработку, изменить строку формата, но не изменить количество аргументов.

Фрагмент 7

public void ShowPreviewTower(TowerData towerData)
{
  ....

  TowerLevelData towerLevelData = towerData.GetTowerLevelData(0);
  if (towerLevelData == null)    // <=
  {
    Log.Error("Tower '{0}' Level '{1}' data is null.", towerData.Name, 0);
  }

  EntityDataRadiusVisualiser entityDataRadiusVisualiser = 
    EntityDataRadiusVisualiser.Create(towerLevelData.Range); // <=
  ....
}

Предупреждение PVS-Studio: V3125 The 'towerLevelData' object was used after it was verified against null. Check lines: 122, 117. LevelControl.cs 122

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

Фрагмент 8

public DataTableProcessor(....)
{
  ....
  m_Strings = strings.OrderBy(value => value.Key)
                     .OrderByDescending(value => value.Value)
                     .Select(value => value.Key).ToArray();
  ....
}

Предупреждение PVS-Studio: V3078 Sorting keys priority will be reversed relative to the order of 'OrderBy...' method calls. Perhaps, 'ThenByDescending' should be used instead. DataTableProcessor.cs 179

Не все разработчики знают, что паттерн сортировки .OrderBy.OrderBy работает "в обратном" порядке. На эту тему у нас есть отдельная статья "Сортировки в C#: OrderBy.OrderBy или OrderBy.ThenBy? Разбираемся, что эффективнее и почему". Кажется, что лучше использовать .OrderBy.ThenBy. Это не только делает код более понятным, но и более производительным.

Фрагмент 9

public bool BuildResources()
{
  ....
  bool isSuccess = false;
  isSuccess = BuildResources(Platform.Windows, ....);
  if (!watchResult || isSuccess)
  {
    isSuccess = BuildResources(Platform.Windows64, ....);
  }

  ....

  if (!watchResult || isSuccess)
  {
    isSuccess = BuildResources(Platform.WebGL, ....);
  }
  ....
}

Предупреждение PVS-Studio: V3137 The 'isSuccess' variable is assigned but is not used by the end of the function. ResourceBuilderController.cs 748

PVS-Studio предупреждает о том, что переменной isSuccess присваивается значение. Сама переменная после этого не используется. В isSuccess записывается результат работы метода BuildResources. Под вторым '....' скрыты ещё шесть присваиваний. Сложно сказать, действительно ли это ошибка, но если последнее возвращаемое значение метода должно игнорироваться, то лучше использовать '_':

_ = BuildResources(Platform.WebGL, ....);

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

Фрагмент 10

private void RefreshSourceVersionCount()
{
  m_SourceVersionIndexes[m_TargetVersionIndex + 1] = false; // <=
  m_SourceVersionCount = 0;
  if (m_SourceVersionIndexes == null)     // <=
  {
    return;
  }

  for (int i = 0; i < m_SourceVersionIndexes.Length; i++)
  {
    if (m_SourceVersionIndexes[i])
    {
      m_SourceVersionCount++;
    }
  }
}

Предупреждение PVS-Studio: V3095 The 'm_SourceVersionIndexes' object was used before it was verified against null. Check lines: 338, 340. ResourcePackBuilder.cs 338

А вот это предупреждение сложно как-то расписать :) Поле m_SourceVersionIndexes проверяют на null уже после доступа к элементам массива по индексу. Проверку стоит перенести в начало метода.

Заключение

Вот и всё :) Как по мне, мы не потратили много времени, и было довольно легко разобрать эти ошибки. Теперь вы знаете, как можно быстро найти ошибки в коде. Но стоит помнить, что механизм Best Warnings больше подходит для первого знакомства. Статический анализ нужно использовать на постоянной основе. Для этого можно использовать проверки коммитов и запуски анализа на компьютерах разработчиков. Именно так инструменты статического анализа раскрываются в полной мере.

Удачи!

#CSharp #GameDev