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

08 Апр 2022

Теги:

#CSharp #GameDev

Повторная проверка Unity статическим анализатором PVS-Studio

08 Апр 2022

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

Введение
Результаты проверки
Заключение

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

Введение

Ранее мы уже выпускали статью про проверку Unity. Думаю, вам будет интересно ознакомиться с ней. Это действительно большой проект, который ежедневно используют тысячи разработчиков. Не стоит забывать и про пользователей, которые проводят своё время в играх, разрабатываемых с помощью Unity. Я считаю, что подобного масштаба проекты не стоит оставлять надолго без внимания, ведь ошибки в них могут отразиться на большом количестве людей.

Анализировать будем исходный код движка Unity и редактора версии 2022.1.0b8. Давайте перейдём непосредственно к результатам проверки.

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

Issue 1

private void Draw(Rect windowRect)
{
  var rect = new Rect(....);
  ....
  if (m_NumFilteredVariants > 0)
  {
    ....        
    if (m_NumFilteredVariants > maxFilteredLength)
    {
      GUI.Label(....);
      rect.y += rect.height;
    }
  }
  else
  {
    GUI.Label(rect, "No variants with these keywords");
    rect.y += rect.height;                               // <=
  }

  rect.y = windowRect.height - kMargin - kSpaceHeight – 
    EditorGUI.kSingleLineHeight;                         // <=
  ....
}

V3008 The 'rect.y' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 370, 366. ShaderVariantCollectionInspector.cs 370

Анализатор сообщает о том, что одной и той же переменной rect.y дважды подряд присваивается значение, и при этом между присваиваниями эта переменная не используется. Если присмотреться, то можно увидеть, что сформированное для этой переменной значение чуть выше под условием m_NumFilteredVariants > maxFilteredLength тоже будет теряться.

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

Issue 2

public static string FetchBuiltinDescription(....)
{
  return string.IsNullOrEmpty(version?.packageInfo?.description) ?
    string.Format(L10n.Tr(....), version.displayName) :
    version.packageInfo.description.Split(....)[0];
}

V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'version' object UpmPackageDocs.cs 38

Анализатор обнаружил два способа обращения к членам одного объекта. Если значение version будет null, то метод IsNullOrEmpty вернёт true, и при обращении к displayName будет выброшено исключение NullReferenceException.

Issue 3

public void SetScaleFocused(Vector2 focalPoint,
                            Vector2 newScale,
                            bool lockHorizontal,
                            bool lockVertical)
{
  if (uniformScale)
    lockHorizontal = lockVertical = false;
  else
  {
    if (hZoomLockedByDefault)
      lockHorizontal = !lockHorizontal;

    if (hZoomLockedByDefault)
      lockVertical = !lockVertical;
  }
....
}

V3029 The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 930, 933. ZoomableArea.cs 930

Разработчики осуществляют одинаковую проверку два раза подряд. hZoomLockedByDefault является полем класса и если мы посмотрим на место его объявления, то увидим, что рядом есть поле vZoomLockedByDefault.

internal class ZoomableArea
{
  ....
  // Zoom lock settings
  public bool hZoomLockedByDefault = false;
  public bool vZoomLockedByDefault = false;
  ....
}

Всё выглядит как банальная опечатка, на которую как раз и указывает анализатор.

Issue 4

private void UpdateTextFieldVisibility()
{
  if (showInputField)
  {
    ....
  }
  else if (inputTextField != null && inputTextField.panel != null)
  {
    if (inputTextField.panel != null)                         // <=
      inputTextField.RemoveFromHierarchy();

    inputTextField.UnregisterValueChangedCallback(OnTextFieldValueChange);
    inputTextField.UnregisterCallback<FocusOutEvent>(OnTextFieldFocusOut);
    inputTextField = null;
  }
}

V3022 Expression 'inputTextField.panel != null' is always true. BaseSlider.cs 648

Анализатор сообщает о том, что выражение inputTextField.panel != null всегда истинно.

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

Issue 5

Был найден следующий код:

public enum EventType
{
  ....
  // Mouse button was released.
  MouseUp = 1,
  ....
  // Already processed event.
  Used = 12,
  ....
}
public static void MinMaxScroller(....)
{
  ....
  if (   Event.current.type == EventType.MouseUp 
      && Event.current.type == EventType.Used) 
  {
    scrollControlID = 0;
  }

  ....
}

V3022 Expression is always false. Probably the '||' operator should be used here. EditorGUIExt.cs 141

В данном случае анализатор нашёл выражение, которое всегда ложно. Какое бы значение ни вернуло свойство, одно из сравнений будет ложно.

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

public static void MinMaxScroller(....)
{
  ....
  if (   Event.current.type == EventType.MouseUp 
      || Event.current.type == EventType.Used) 
  {
    scrollControlID = 0;
  }

  ....
}

Issue 6

private List<T> GetChildrenRecursively(....)
{
  if (result == null)
    result = new List<T>();
  if (m_Children.Any())
  {
    var children = sorted ? (....)m_Children.OrderBy(c => c.key)
                                            .OrderBy(c => c.m_Priority) 
                          : m_Children;
    foreach (var child in children)
      child.GetChildrenRecursively(sorted, result);
  }
  else if (value != null)
    result.Add(value);
  return result;
}

V3078 Original sorting order will be lost after repetitive call to 'OrderBy' method. Use 'ThenBy' method to preserve the original sorting. MenuService.cs 499

Анализатор обнаружил в коде ситуацию, в которой происходит подряд два вызова OrderBy.

Как по мне, так очень интересное предупреждение. Естественно, что два вызова OrderBy не являются паттерном ошибки. Это скорее код, который может привести к ошибке, если неправильно понимать, как он работает. Если программист хотел отсортировать коллекцию сначала по ключу, а потом по приоритету, то в данном случае будет ошибка. Почему?

Давайте разбираться. В данном коде два вызова OrderBy отсортируют коллекцию сначала по приоритету, а после по ключу. Не совсем очевидно, не так ли? Думаю, что вместо второго OrderBy правильнее использовать ThenBy, чтобы сортировка выполнялась не "наоборот". Написание с ThenBy будет лучше читаться и не будет вызывать лишних вопросов. За подробностями предлагаю вам перейти к небольшой заметке.

Кстати, PVS-Studio нашёл ещё одно такое же подозрительное место: V3078 Original sorting order will be lost after repetitive call to 'OrderBy' method. Use 'ThenBy' method to preserve the original sorting. SearchSelector.cs 177

Issue 7

public void IconSectionGUI(NamedBuildTarget namedBuildTarget,....)
{
  ....
  if (platformUsesStandardIcons)
  {
    var selectedDefault = (m_SelectedPlatform < 0);
    // Set default platform variables
    BuildPlatform platform = null;
    namedBuildTarget = NamedBuildTarget.Standalone;
    ....
  }
  ....
}

V3061 Parameter 'namedBuildTarget' is always rewritten in method body before being used. PlayerSettingsIconsEditor.cs 396

Довольно странный фрагмент кода. Первый параметр метода перезаписывается перед тем, как используется. Причём данный параметр используется только внутри условия if (platformUsesStandardIcons). В итоге значение, которое передаётся в метод, всегда теряется.

Issue 8

internal void BeginNamingNewAsset(....)
{
  m_State.m_NewAssetIndexInList = m_LocalAssets.IndexOfNewText(....);
  if (m_State.m_NewAssetIndexInList != -1)
  {
    Frame(instanceID, true, false);
    GetRenameOverlay().BeginRename(newAssetName, instanceID, 0f);
  }
  else
  {
    Debug.LogError("Failed to insert new asset into list");
  }

  Repaint();
}

V3022 Expression 'm_State.m_NewAssetIndexInList != -1' is always true. ObjectListArea.cs 511

Анализатор обнаружил выражение, которое всегда истинно. m_State.m_NewAssetIndexInList присваивается возвращаемое значение метода IndexOfNewText. Давайте взглянем на реализацию этого метода:

public int IndexOfNewText(....)
{
  int idx = 0;
  if (m_ShowNoneItem)
    idx++;

  for (; idx < m_FilteredHierarchy.results.Length; ++idx)
  {
    FilteredHierarchy.FilterResult r = m_FilteredHierarchy.results[idx];
                    
    if (foldersFirst && r.isFolder && !isCreatingNewFolder)
      continue;
                    
    if (foldersFirst && !r.isFolder && isCreatingNewFolder)
      break;
                    
    string propertyPath = AssetDatabase.GetAssetPath(r.instanceID);
    if (EditorUtility.NaturalCompare(....) > 0)
    {
      return idx;
    }
  }
  return idx;
}

Можно заметить, что метод возвращает idx, который всегда больше или равен 0.

Как итог ветвь else никогда не будет выполняться. Возможно, ошибка может быть в методе IndexOfNewText. При его использовании ожидали, что он может вернуть -1.

Issue 9

public static Overlay CreateOverlay(Type type)
{
  ....
  if (overlay == null)
  {
    Debug.LogWarning("Overlay of type {type} can not be instantiated." + ....);
    return null;
  }
  ....
}

V3138 String literal contains potential interpolated expression. Consider inspecting: type. OverlayUtilities.cs 116

PVS-Studio указывает на то, что пропущен символ интерполяции строк. Подобные недочёты часто усложняют поиск проблемы, т. к. полученное сообщение будет содержать неточную информацию.

Issue 10

int GetCurveAtPosition(Vector2 viewPos, out Vector2 closestPointOnCurve)
{
  ....
  for (int i = m_DrawOrder.Count - 1; i >= 0; --i)
  {
    CurveWrapper wrapper = GetCurveWrapperFromID(m_DrawOrder[i]);

    if (wrapper.hidden || wrapper.readOnly || wrapper.curve.length == 0)
      continue;
    ....
  }
}

V3080 Possible null dereference. Consider inspecting 'wrapper'. CurveEditor.cs 1889

Анализатор обнаружил фрагмент кода, который может привести к разыменованию ссылки, значение которой равно null.

Метод GetCurveWrapperFromID может возвращать null:

internal CurveWrapper GetCurveWrapperFromID(int curveID)
{
  if (m_AnimationCurves == null)
    return null;

  int index;
  if (curveIDToIndexMap.TryGetValue(curveID, out index))
    return m_AnimationCurves[index];

  return null;
}

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

Issue 11

internal static void MaterialShaderReferences(....)
{
  var material = context.target as Material;
  if (material == null || !material.shader)
    return;

  indexer.AddReference(context.documentIndex, "shader", material.shader);

  if (!indexer.settings.options.properties)
    return;

  var ownerPropertyType = typeof(Shader);
  var shaderName = $"{material.shader.name}/" ?? string.Empty;   // <=
  ....
}

V3022 Expression '$"{material.shader.name}/"' is always not null. The operator '??' is excessive. IndexerExtensions.cs 190

Анализатор сигнализирует о том, что выражение $"{material.shader.name}/" всегда не null. Думаю, что с этим утверждением довольно трудно не согласиться. Следовательно, проверка на null с помощью оператора '??' является бесполезной.

Issue 12

static int CountIntersections(....)
{
  ....
  int hitLength = s_RayCastHits.Length;
  float maxDist = 0;
  if (hitLength > 0)
    maxDist = s_RayCastHits[s_RayCastHits.Length - 1].distance;

  physicsScene.Raycast(....);
  if (s_RayCastHits.Length > 0)
  {
    float len = length - s_RayCastHits[0].distance;
    if (len > maxDist)
    {
      maxDist = len;                                 // <=
    }
  }

  return hitLength + s_RayCastHits.Length;
}

V3137 The 'maxDist' variable is assigned but is not used by the end of the function. TreeAOImporter.cs 142

Анализатор указывает на то, что локальной переменной присваивается значение, но после она нигде не используется. Также можно заметить, что код, начиная с if (s_RayCastHits.Length > 0), не делает ничего осмысленного. Все присваивания в этом фрагменте выполняются локальным переменным, которые никак не влияют на возвращаемое значение.

Issue 13

public override DragAndDropVisualMode DoDrag(....)
{
  var hierarchyTargetItem = targetItem as GameObjectTreeViewItem;

  if (m_CustomDragHandling != null)
  {
    DragAndDropVisualMode dragResult = 
      m_CustomDragHandling(parentItem as GameObjectTreeViewItem,
                           hierarchyTargetItem,
                           ....);
    ....
  }
  DragAndDropVisualMode dragSceneResult =
    DoDragScenes(parentItem as GameObjectTreeViewItem,
                 hierarchyTargetItem,
                 ....);

  if (   targetItem != null 
      && !IsDropTargetUserModifiable(hierarchyTargetItem, dropPos)) // <=
  {
    return DragAndDropVisualMode.Rejected;
  }
  ....
}

V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'targetItem', 'hierarchyTargetItem'. AssetOrGameObjectTreeViewDragging.cs 153

Анализатор указывает на то, что targetItem приводят к производному типу GameObjectTreeViewItem с использованием оператора as. Однако на равенство с null проверяют не полученную ссылку, а исходную.

В случае если не выйдет преобразовать с помощью as, тогда hierarchyTargetItem будет содержать значение null. И при передаче hierarchyTargetItem в IsDropTargetUserModifiable внутри метода произойдёт выбрасывание всеми любимого NullReferenceException.

Упрощённый код этого метода:

static bool IsDropTargetUserModifiable(GameObjectTreeViewItem targetItem, ....)
{
  if (targetItem.isSceneHeader && !targetItem.scene.isLoaded)
    return false;
  ....
}

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

Issue 14

static Vector3 ResizeHandlesGUI(....)
{
  ....
  Vector3 scale = Vector3.one; 
  ....
  if (uniformScaling)                                 // <=
  {
    float refScale = (xHandle == 1 ? scale.y : scale.x);
    scale = Vector3.one * refScale;
  }

  if (uniformScaling)                                 // <=
  {
    float refScale = (xHandle == 1 ? scale.y : scale.x);
    scale = Vector3.one * refScale;
  }
  ....
}

V3029 The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 722, 728. BuiltinTools.cs 722

Анализатор обратил внимание на подозрительный код, в котором два if с одинаковыми условиями идут друг за другом. Можно было предположить, что второй if – просто лишний код, который ни на что не влияет. Это предположение будет ошибочным, так как при формировании значения refScale используется значение scale. Поэтому второй блок всё же влияет на результат.

Стоит заметить, что значение uniformScaling не меняется внутри условных блоков. То есть все расчёты можно было бы записать под один if.

Заключение

Полагаю, что я не зря решил вновь проверить этот проект. Повторная проверка Unity позволила найти несколько достаточно интересных мест. Что из перечисленного – ошибки, а что – просто недостатки? Судить об этом уже придётся разработчикам. Со стороны, увы, бывает, сложно оценить критичность предупреждения.

В любом случае спасибо команде Unity за их труд! Хочется верить, что эта статья помогла внести небольшой вклад в качество проекта.

Вы также можете проверить свой проект анализатором PVS-Studio. Для этого можно взять пробный ключик у нас на сайте.

#CSharp #GameDev