Рынок современных игровых движков постепенно расширяется, и всё больше студий выбирают не кого-то из двух гигантов (учитывая последние события, вообще одного), а движки поменьше. Сегодня поговорим про одного из новичков индустрии — 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).
Примечание. Если вы хотите узнать о нововведениях C# 14, то у нас есть обзорная статья.
Из особенностей хотелось бы выделить, что это первый сторонний проект (не от 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 ); } }
Попробуйте найти ошибку сами.
Ответ где-то рядом...
Предупреждение PVS-Studio: V3001 There are identical sub-expressions ‘hEdgeA.IsValid’ to the left and to the right of the ‘&&’ operator. HalfEdgeMesh.cs 2094
Если хорошо приглядеться, то ошибка становится очевидной.
В условии 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 );
Получилось?
Давайте дам подсказку.
Сообщение PVS-Studio...
V3010 The return value of function ‘Deserialize’ is required to be utilized. PolygonMesh.Serialize.cs 54
Я в вас не сомневался!
Ладно, оставим шутки — надо разобраться. У нас есть модификатор 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 ошибок), или вы хотите проверить свой собственный проект, то скачать и попробовать анализатор можно по ссылке.
Примечание. А для тех, кто дочитал статью до конца, у нас есть бонус: возможность записаться на программу раннего доступа к нашим новым анализаторам! EAP доступно по направлениям JavaScript, TypeScript и Go.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Gleb Aslamov. S&Box game engine: looking closer at grains of sand.
