Песочница ошибок: проверка игрового движка S&Box

• Пара слов о проекте
• Проверка проекта
  • Какие ошибки мы отобрали?
  • Время code review
  • Расставляем приоритеты
  • Опасный сценарий
  • Про кого-то забыли
  • Чрезмерная проверка
  • Забытый Deserialize
  • Релизный TODO
  • Мёртвая зона
  • Секунда Шрёдингера
• Заключение

Рынок современных игровых движков постепенно расширяется, и всё больше студий выбирают не кого-то из двух гигантов (учитывая последние события, вообще одного), а движки поменьше. Сегодня поговорим про одного из новичков индустрии — S&Box. И это случай, когда новичок не такой простой, каким кажется. Подробнее о проекте и о том, какие ошибки мы смогли найти с помощью PVS-Studio, расскажем в статье.

Пара слов о проекте

S&Box — это новенький игровой движок от довольно известной студии Facepunch, которая подарила нам, можно сказать, культовые проекты — Rust и Garry's Mod. Оба являются одними из самых продаваемых игр в Steam. Но Garry's Mod играет тут гораздо более важную роль, чем просто одна из игр студии. S&Box — это духовный наследник и полноценная версия всех идей, которые хотел видеть Гари Ньюман — создатель обоих игр — в Garry's mod.

S&Box не просто движок. Это развлекательная платформа для творчества, в которой можно одновременно играть, создавать и даже монетизировать свой контент.

Проекты в S&Box пишутся на новеньком C# 14, а сам движок разработан на базе .NET 10. В качестве фич можно выделить поддержку изменений в реальном времени, представление логики с помощью визуальных графов (no code) и графы для шейдеров (как в Blender).

Из особенностей хотелось бы выделить, что это первый сторонний проект (не от Valve), который использует Source 2, так он теперь ещё и открытый!

Естественно, мы не могли пройти мимо новенького C# проекта, использующего все мощности новейшего .NET! Если вдруг кто-то забыл или встречает нас впервые — мы команда статического анализатора PVS-Studio, и мы любим проверять открытые проекты. Это позволяет нам проверить способности анализатора и поделиться с вами результатами.

Сегодня мы узнаем, почему полезно использовать статические анализаторы кода при разработке, особенно когда вы планируете открывать ваш проект для всего интернета >:)

Код проекта доступен на GitHub (анализ проводился на одном из релизных коммитов — f69cd48).

Проверка проекта

Какие ошибки мы отобрали?

В статье мы будем разбирать срабатывания из вкладки BEST.

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

Функционал содержит достаточно простой, но эффективный механизм. Если кратко, то в анализаторе есть разделение на три уровня срабатываний (High, Medium и Low). Они отвечают за уровень достоверности.

А зачем он нужен? Всё просто. Анализатор — это инструмент, который старается понять весь контекст вашего проекта, но технические ограничения не дают ему это сделать до конца. Чтобы не перемешивать срабатывания, где анализатор почти на 100% уверен в наличии ошибки, и места, где он указывает на просто сомнительные части кода, которые могут не содержать ошибки, мы сделали разделение по трём разным уровням.

При стандартной работе с анализатором мы советуем оставлять включёнными уровни High и Medium, а Low оставлять на случаи, когда вы можете уделить время изучению этих предупреждений. В анализаторе заложен функционал, который всегда указывает на code smell, а это значит, что другой человек при работе с этим кодом тоже может принять его за ошибочный.

Если вы при работе с анализатором изучаете кодовую базу и проверяете, реально ли это ошибка или нет, то другой человек такое исследование проводить уже не будет :)

Время code review

Начнём с небольшой разминки:

public bool CollapseEdge(....)
{
  ....
  GetVerticesConnectedToHalfEdge(
    hFullEdge,
    out var hVertexA,
    out var hVertexB );

  var hEdgeA = hFullEdge;
  var hEdgeB = hFullEdge.OppositeEdge;

  var hFaceA = hEdgeA.Face;
  var hFaceB = hEdgeB.Face;
  ....
  // Disconnect the edge that is being collapsed
  // from the faces and other edges.
  Assert.True(hEdgeA.IsValid &&  hEdgeB.IsValid );
  if ( hEdgeA.IsValid && hEdgeA.IsValid )
  {
    var pNewVertex = hNewVertex;

    var hNextEdgeA = hEdgeA.NextEdge;
    var hPrevEdgeA =
      FindPreviousEdgeInFaceLoop( hEdgeA );

    var hNextEdgeB = hEdgeB.NextEdge;
    var hPrevEdgeB =
      FindPreviousEdgeInFaceLoop( hEdgeB );
  }
}

Попробуйте найти ошибку сами.

Если хорошо приглядеться, то ошибка становится очевидной.

В условии If происходит сравнение двух объектов со схожими названиями hEdgeA и hEdgeB. Но как бы не происходит... Из кода видно, что хотели именно так, но что-то случилось и сравнили уже hEdgeA и hEdgeA, а результат, я думаю, все понимают. Как раз в таких местах и происходят очепятки. Много переменных и операций над ними, так они ещё и очень похожи! Зачем мучить себя, если можно воспользоваться инструментом, который найдёт все подобные ошибки...

Расставляем приоритеты

private void RewriteTypeNames(....)
{
  ....
  foreach ( var attribute in node.Attributes )
  {
    ....
    if ( attribute.BoundAttribute?.IsGenericTypedProperty() 
         ?? false && attribute.TypeName != null ) {....}

    else if ( attribute.TypeName == null &&      
      (attribute.BoundAttribute?.IsDelegateProperty() ?? false) ) {....}

    else if ( attribute.TypeName == null &&
      (attribute.BoundAttribute?.IsEventCallbackProperty() ?? false) ) {....}

    else if ( attribute.TypeName == null ) {....}
  }
  ....
}

Предупреждение PVS-Studio: V3177 The 'false' literal belongs to the '&&' operator with a higher priority. It is possible the literal was intended to belong to '??' operator instead. ComponentGenericTypePass.cs 346

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

attribute.BoundAttribute?.IsGenericTypedProperty() 
  ?? false && attribute.TypeName != null

Скорее всего, разработчик предполагал такой порядок действий (изобразим скобочками):

(attribute.BoundAttribute?.IsGenericTypedProperty() 
  ?? false) && attribute.TypeName != null

Но вышло так:

attribute.BoundAttribute?.IsGenericTypedProperty() 
  ?? (false && attribute.TypeName != null)

Как можно догадаться из сообщения анализатора, дело в том, что оператор && имеет больший приоритет, чем оператор ??, и происходит сценарий, лишённый какого-либо смысла.

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

И, несмотря на всё это, самое неприятное в том, что ошибка повторилась. Разработчик "скопипастил" кусок кода и тем самым распространил её дальше — это можно увидеть чуть ниже текущей ошибки:

private void RewriteTypeNames(....)
{
  ....
  foreach ( var childContent in node.ChildContents )
  {
    if ( childContent.BoundAttribute?.IsGenericTypedProperty()
         ?? false && childContent.TypeName != null ) {....}
    
     else if ( childContent.IsParameterized ){....}
     else{....}
  }  
  ....
}

Опасный сценарий

public partial class Hotload
{
  public void AddUpgrader( IInstanceUpgrader upgrader )
    {....}

  public void AddUpgrader<TUpgrader>()
    where TUpgrader : IInstanceUpgrader, new()
  {
    AddUpgrader( new TUpgrader() );
  }

  public IInstanceUpgrader GetUpgrader( Type upgraderType )
    {....}

  ....

  internal static string FormatAssemblyName( AssemblyName name )
  {
    return AssemblyNameFormatter?
             .Invoke( name ) ?? name.ToString();
  }

  internal static string FormatAssemblyName( Assembly asm )
  {
  return FormatAssemblyName(asm?.GetName());
  }
}

Как думаете, что не так с этим фрагментом? В одном из сценариев можно получить NRE. К сожалению, не самая редкая ошибка.

Давайте сократим код и взглянем поближе:

internal static string FormatAssemblyName( AssemblyName name )
{
  return AssemblyNameFormatter?
           .Invoke( name ) ?? name.ToString();
}

internal static string FormatAssemblyName( Assembly asm )
{
  return FormatAssemblyName(asm?.GetName());
}

Предупреждение PVS-Studio: V3105 The result of null-conditional operator is dereferenced inside the 'FormatAssemblyName' method. NullReferenceException is possible. Inspect the first argument 'asm?.GetName()'. Hotload.cs 324

Разработчик изначально предполагает, что в методе FormatAssemblyName( Assembly asm ) параметр asm может быть null — FormatAssemblyName(asm?.GetName()). Но уже в FormatAssemblyName( AssemblyName name ) такое не ожидается. У нас есть два сценария: первый — name = null, второй — AssemblyNameFormatter = null и name = null.

В первом случае всё будет нормально, т. к. в AssemblyNameFormatter есть обработка такого сценария. А во втором уже будет ошибка, поскольку name используется без проверки, и в момент вызова метода name.ToString()происходит NRE.

Про кого-то забыли

internal void UnregisterTypes( Assembly assembly )
{
  var assetSourceType = typeof( BaseGameMount );
  var types = assembly
                .GetTypes().Where( t => 
                  assetSourceType.IsAssignableFrom( t ) 
                  && !t.IsAbstract );

  foreach ( var (ident, source) in Sources.ToArray() )
  {
    if ( source.GetType().Assembly != assembly ) continue;
    if ( source.IsMounted ){ PendingMounts.Add( ident ); }

    source.ShutdownInternal();
    Sources.Remove( ident );
  }
}

В этом коде скрыта грустная история о забытой переменной и невыполненном методе из-за отложенного выполнения.

Предупреждение PVS-Studio: V3220 The result of the 'Where' LINQ method with deferred execution is never used. The method will not be executed. MountHost.cs 114

Если взглянем на переменную types и на значение, которое ей задаёт разработчик, то увидим выборку из определённых объектов:

var types = assembly
                .GetTypes().Where( t => 
                  assetSourceType.IsAssignableFrom( t ) 
                  && !t.IsAbstract );

Но отбора не произойдёт, т. к. types больше нигде в коде не упоминается. Подобное поведение обусловлено тем, что при вызове Where фильтрация не выполняется в момент вызова, а является отложенной. Следовательно, до тех пор, пока не будет произведено итерирование по коллекции, код делегата не выполнится.

В представленном случае последствия не такие серьёзные, потому что от делегата в Where не ожидается дополнительных действий, но если код будет другим, то может потеряться важный функционал, например:

int it = 0;
var filteredNames = names.Where(name => 
                    {
                      ++it;
                      Console.WriteLine($"{it}) {name}");
                      return name.StartsWith(startLetter); 
                    });

В метод Where передаётся делегат, который, помимо условия фильтрации, выводит на консоль элементы и их позиции из исходной коллекции. Но, как вы уже поняли, filteredNames может не быть упомянут, по этой причине отфильтрованная коллекция не будет выведена на консоль. Более того, фильтрация в принципе не выполнится.

Чрезмерная проверка

public static bool IsIes( byte[] data )
{
  if ( data == null || data.Length < 10 )
    return false;

  var header = Encoding.ASCII.GetString
    ( data, 0, Math.Min( 20, data.Length ) ).Trim();
  
return 
   header.StartsWith( "IESNA" ) 
   || header.StartsWith( "IESNA:LM-63" );
}

Предупреждение PVS-Studio: V3053 An excessive expression. Examine the substrings 'IESNA' and 'IESNA:LM-63'. Bitmap.Loading.Ies.cs 242

В случае, если подстрока IESNA будет найдена, то дальнейшая проверка не выполнится. Если подстрока IESNA не будет найдена, то и поиск более длинной подстроки IESNA:LM-63 не имеет смысла. Обычно мы советуем просто убрать излишний код, но тут может быть немного другая история.

В коде идёт дополнительная проверка на IESNA:LM-63, и если разработчики хотели убедиться, что стандарт IESNA будет именно "LM-63", нужно было написать так:

return header.StartsWith("IESNA:LM-63") 
       || header.StartsWith("IES:LM-63");

Это новый и старый форматы IESNA (в современных версиях префикс просто IES:), а текущая проверка не выполняет это условие и пропустит любую кодировку/версию стандарта.

Забытый Deserialize

Попробуйте найти ошибку в коде:

JsonSerializer.Deserialize<Vector3>( ref reader );

Получилось?

Давайте дам подсказку.

Я в вас не сомневался!

Ладно, оставим шутки — надо разобраться. У нас есть модификатор ref и логика и так отработает, может проблемы-то и нет?

Дело в том, что это десериализация в этом фрагменте встречается часто. Давайте взглянем:

....
else if ( propertyName == "TextureOrigin" )
{
  if ( reader.TokenType == JsonTokenType.StartArray )
  {
    mesh
   .TextureOriginUnused
   .CopyFrom( JsonSerializer.Deserialize<Vector3[]>( ref reader ) );
  }
  else
  {
    JsonSerializer.Deserialize<Vector3>( ref reader );   // <=
  }
}
else if ( propertyName == nameof( TextureCoord )
{
  mesh
 .TextureCoord
 .CopyFrom( JsonSerializer.Deserialize<Vector2[]>( ref reader ) );
  
  hasTextureCoords = true;
}

else if ( propertyName == "TextureRotation" )
  mesh
  .TextureRotationUnused
  .CopyFrom( JsonSerializer.Deserialize<Rotation[]>( ref reader ) );

else if ( propertyName == nameof( TextureUAxis ) )
  mesh
  .TextureUAxis
  .CopyFrom( JsonSerializer.Deserialize<Vector3[]>( ref reader ) );

....

В коде ещё много такого. Смысл в том, что возвращаемое значение используется для копирования и других действий, но лишь в одном месте его пропустили. Очень подозрительный фрагмент, в котором, скорее всего, просто забыли ;_;.

Релизный TODO

public static ConCmdAttribute.AutoCompleteResult[]
  GetAutoComplete(....)
{
  var parts = partial.SplitQuotesStrings();

  List<ConCmdAttribute.AutoCompleteResult> results = new();

  // if we have more than one part, complete a specific command
  if ( parts.Length > 1 )
  {
    if ( !Members.TryGetValue( parts[0], out var command ) 
      return Array
             .Empty<ConCmdAttribute.AutoCompleteResult>();
  
  //results.Add( new ConCmd.AutoCompleteResult 
  //  { Command = command.Name, Description = command.Help } );
  
  // TODO - dig into it for auto complete
  return results.Take( count ).ToArray();
}

Предупреждение PVS-Studio: V3191 Iteration through the 'results' collection makes no sense because it is always empty. ConVarSystem.AutoComplete.cs 23

Анализатор указывает, что итерация по results не имеет смысла, т. к. коллекция всегда пустая. И с ним тяжело не согласиться! Ведь функционал, добавляющий что-либо в эту коллекцию, был закомментирован.

Учитывая остальные комментарии, можно понять две вещи: какая-то логика присутствует и в коде смысл есть; часть функционала отправили в TODO.

Казалось бы, тогда это и не ошибка, ведь всё осмысленно сделано! Но у меня вопрос: считать ли такой случай плохой практикой? Нужно учитывать, что этот проект — база для многих других. Логика и работа многих игр будет зависеть от того, как хорошо написаны исходники движка. Этот фрагмент лежит с самого релиза, а на момент написания статьи прошло уже более полугода.

Как вы считаете, это плохо или в этом ничего такого нет? Напишите своё мнение в комментариях.

Мёртвая зона

internal static void OnGameControllerAxis(
  int deviceId,
  GameControllerAxis axis,
  int value )
{

  controller.SetAxis( axis, value );

  ....

  const float triggerDeadzone = 0.75f;

  // I hate this but okay
  GamepadCode code =
    axis switch
    {
      GameControllerAxis.TriggerLeft =>
        GamepadCode.LeftTrigger,

      GameControllerAxis.TriggerRight =>
        GamepadCode.RightTrigger,

      _ =>
        GamepadCode.None,
    };

  OnGamepadCode(deviceId, code, value >= triggerDeadzone );
}

Предупреждение PVS-Studio: V3040 The 'triggerDeadzone' constant of the 'float' type is compared to a value of the 'int' type. InputRouter.Input.cs 207

Здесь переменная value имеет тип int, и сравнение этой переменной со значением 0.75f выглядит подозрительно, особенно с использованием оператора >=, т. к. ближайшие значения это 0 и 1. Тем более странно видеть это в коде, связанном с кнопками контроллера, где важна точность. И как раз тут мы работаем с "мёртвой зоной" кнопки, — важно понимать, где ТОЧНО она начинается.

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

Секунда Шрёдингера

public static string FormatSecondsLong(
  this long secs )
{
  var m =System.Math.Floor( (float)secs / 60.0f );

  var h =System.Math.Floor( (float)m / 60.0f );

  var d =System.Math.Floor( (float)h / 24.0f );

  var w =System.Math.Floor( (float)d / 7.0f );

  if ( secs < 60 )return string.Format("{0} seconds",secs );

  if ( m < 60 )return string.Format("{1} minutes,
                                     {0} seconds",
                                     secs % 60,
                                     m );

  if ( h < 48 )
    return string.Format(
      "{2} hours and {1} minutes", 
        secs % 60, 
        m % 60, 
        h );

  if ( d < 7 )
    return string.Format(
      "{3} days, {2} hours and {1} minutes",
        secs % 60,
        m % 60,
        h % 24,
        d );

  return string.Format(
    "{4} weeks, {3} days, {2} hours and {1} minutes",
      secs % 60,
      m % 60,
      h % 24,
      d % 7,
      w );
}

Предупреждение PVS-Studio: V3025 Incorrect format. A different number of format items is expected while calling 'Format' function. Arguments not used: secs % 60. NumberExtensions.cs 102

Анализатор утверждает, что количество параметров не соответствует строке формата, в которую передаётся. И мы можем в этом убедиться.

В первой строке передаются секунды, и это происходит без проблем, всё корректно:

if ( secs < 60 )return string.Format("{0} seconds",secs );

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

if ( h < 48 )
  return string.Format(
    "{2} hours and {1} minutes", 
      secs % 60, 
      m % 60, 
      h );

Если посмотреть на код целиком, то секунды передаются везде, и есть вероятность, что это было сделано для читаемости, т. к. секунды используются в получении минут, часов и т. д.

Можно сказать, что анализатор ошибся, но он же прав, не так ли? :)

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

Ну а если вы на 100% уверены в своём коде, то всегда можете просто дать понять анализатору с помощью комментария, что тут можно не ругаться:

if ( h < 48 ) return string.Format( 
  "{2} hours and {1} minutes", secs % 60, m % 60, h ); //-V3025

Заключение

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

Если вам интересно, что ещё нашёл PVS-Studio в S&Box (в отчёте более 1000 ошибок), или вы хотите проверить свой собственный проект, то скачать и попробовать анализатор можно по ссылке.