Единорог в космосе: проверяем исходный код 'Space Engineers'
Как вы уже поняли из заголовка, речь в статье будет идти о подозрительных местах, найденных в исходном коде 'Space Engineers'. Но формат статьи несколько отличается от остальных. Помимо информации о проекте, обзора некоторых найденных подозрительных мест и ошибок, а также способов их исправления, я включил в текст небольшой раздел о правильном сценарии использования статического анализатора. Настоятельно рекомендую ознакомиться с ним, так как многие разработчики не знают или просто не задумываются о том, как правильно использовать инструменты этого класса. В результате инструменты статического анализа используются на порядок менее эффективно, чем могли бы.
Немного об игре
Space Engineers — это песочница о конструировании и обслуживании космических объектов. Игроки могут построить космические корабли и станции различных размеров и предназначений (гражданские и военные), управлять ими и добывать ресурсы. Игра использует реалистичный физический движок: любой объект может быть собран, разобран, повреждён и полностью уничтожен. Игра основана на графическом движке VRage 2.0 собственной разработки компании-создателя Keen Software House.
Исходный код игры доступен в репозитории на GitHub.
Исходный код написан на C# и проверялся с помощью статического анализатора кода PVS-Studio. Вы можете проверить интересующий вас проект, скачав и попробовав анализатор на своём или чужом коде.
С общим списком найденных ошибок и проектов, в которых они были выявлены, можно ознакомиться по соответствующей ссылке.
Ошибочные и подозрительные места
Ниже рассмотрены некоторые предупреждения, которые выдал анализатор, а также фрагменты кода, которые он счёл подозрительными. Повторюсь, что это не все предупреждения. Подробнее об общем количестве ошибок и о том, почему не все они были рассмотрены, написано в соответствующем разделе.
Но вам уже хочется взглянуть на код, да? Что ж, приступим.
Опечатки и невнимательный 'copy-paste'
void DeserializeV0(XmlReader reader)
{
....
if (property.Name == "Rotation" ||
property.Name == "AxisScale" ||
property.Name == "AxisScale")
continue;
....
}
Предупреждение PVS-Studio: V3001 There are identical sub-expressions 'property.Name == «AxisScale»' to the left and to the right of the '||' operator. Sandbox.Graphics MyParticleEmitter.cs 352
Типичная ошибка, встречающаяся и в коде на C++, и на C# и, наверняка, на многих других языках программирования. Причиной таких ошибок, как правило, является простая невнимательность. Сравнивали свойство 'property.Name' со строковыми литералами и дважды сравнили с 'AxisScale'. Скорее всего подразумевалось сравнение с другим литералом (в соседних методах производится сравнение этого же свойства с литералом 'LimitAngle', так что, думаю, здесь подразумевался он же).
Встретились не менее типичные ошибки одинаковых тел оператора 'if' в 'then' и 'else'-блоках. Такие ошибки возникают из-за невнимательности (в том числе и из-за невнимательного copy-paste). Рассмотрим несколько подобных примеров:
private void StartRespawn()
{
m_lastCountdownTime = MySandboxGame.TotalGamePlayTimeInMilliseconds;
if (m_removeAfterDeath)
m_deathCountdownMs = AgentDefinition.RemoveTimeMs;
else
m_deathCountdownMs = AgentDefinition.RemoveTimeMs;
}
Предупреждение PVS-Studio: V3004 The 'then' statement is equivalent to the 'else' statement. Sandbox.Game MyAgentBot.cs 260
Независимо от значения переменной 'm_removeAfterDeath', другой переменной — 'm_deathCountdownMs' — будет присвоено одно и то же значение. В данном случае сложно сказать, что здесь нужно изменить. Но то, что код содержит ошибку, очевидно.
Следующий подобный фрагмент:
private static bool IsTriangleDangerous(int triIndex)
{
if (MyPerGameSettings.NavmeshPresumesDownwardGravity)
{
return triIndex == -1;
}
else
{
return triIndex == -1;
}
}
Предупреждение PVS-Studio: V3004 The 'then' statement is equivalent to the 'else' statement. Sandbox.Game MyNavigationTriangle.cs 189
Ситуация аналогична предыдущей, использование оператора 'if' в данном коде не имеет смысла. И опять сложно сказать, как необходимо исправить данный код. Возможно, в зависимости от условия, необходимо использовать либо оператор '==', либо '!='. Но это только предположение.
И ещё одна схожая ситуация:
public void UpdateLight()
{
....
if (((MyCubeGrid)Parent).GridSizeEnum == MyCubeSize.Large)
Light.GlareIntensity = 0.5f + length * 2;
else
Light.GlareIntensity = 0.5f + length * 2;
....
}
Предупреждение PVS-Studio: V3004 The 'then' statement is equivalent to the 'else' statement. Sandbox.Game MyThrust.cs 149
В зависимости от условия необходимо изменить интенсивность бликов. Но из-за copy-paste она всегда будет одинаковой. Какое значение необходимо устанавливать в том или ином случае опять не скажешь, это решать авторам кода.
Потеря возвращаемых значений
Встречается в проектах код, в котором не используется возвращаемое значение метода. Это может быть связано, например, с тем, что программист забыл о том, что метод 'Replace' класса 'String' возвращает изменённую строку, а исходная остаётся нетронутой, так как объекты класса 'String' являются неизменяемыми. В данном проекте тоже встретились 2 ошибки, связанные с потерей значения, возвращаемого методом:
public void Init(string cueName)
{
....
if (m_arcade.Hash == MyStringHash.NullOrEmpty &&
m_realistic.Hash == MyStringHash.NullOrEmpty)
MySandboxGame.Log.WriteLine(string.Format(
"Could not find any sound for '{0}'", cueName));
else
{
if (m_arcade.IsNull)
string.Format(
"Could not find arcade sound for '{0}'", cueName);
if (m_realistic.IsNull)
string.Format(
"Could not find realistic sound for '{0}'", cueName);
}
}
Предупреждения PVS-Studio:
- V3010 The return value of function 'Format' is required to be utilized. Sandbox.Game MyEntity3DSoundEmitter.cs 72
- V3010 The return value of function 'Format' is required to be utilized. Sandbox.Game MyEntity3DSoundEmitter.cs 74
Статический метод 'Format' класса 'String' составляет результирующую строку на основе строки форматирования и аргументов, которые её формируют, после чего возвращает результирующую строку. Следовательно, вызов этого метода без использования возвращаемого значения не имеет смысла.
Из данного кода видно, что в случае, если не удалось обнаружить некоторые элементы, необходимо писать в лог информацию об ошибке. И последние два вызова метода 'string.Format' должны выступать аргументами у метода 'MySandboxGame.Log.WriteLine'.
Корректный код мог бы выглядеть так:
if (m_arcade.IsNull)
MySandboxGame.Log.WriteLine(string.Format(
"Could not find arcade sound for '{0}'", cueName));
if (m_realistic.IsNull)
MySandboxGame.Log.WriteLine(string.Format(
"Could not find realistic sound for '{0}'", cueName));
Некорректная проверка после использования оператора 'as'
В других своих статьях про проверку C#-проектов ('Проверяем исходный код набора C#/.NET компонентов от Sony', 'Ищем ошибки в MonoDevelop') я упоминал о том, что среди некоторых ошибок, которые допускают C#-программисты, начинают выявляться определённые паттерны. И с каждым новым проверенным проектом я всё больше и больше в этом убеждаюсь. Один из них — приведение объекта с помощью оператора 'as' к совместимому типу, а после — проверка на 'null' не полученного объекта, а исходного, что порождает вероятность возникновения исключений типа 'NullReferenceException'. 'Космические инженеры' не остались в стороне.
Давайте рассмотрим несколько примеров с подобными ошибками:
protected override void Init(MyObjectBuilder_DefinitionBase builder)
{
base.Init(builder);
var ob = builder as MyObjectBuilder_WeaponBlockDefinition;
Debug.Assert(builder != null);
WeaponDefinitionId = new MyDefinitionId(ob.WeaponDefinitionId.Type,
ob.WeaponDefinitionId.Subtype);
ResourceSinkGroup = MyStringHash.GetOrCompute(ob.ResourceSinkGroup);
InventoryMaxVolume = ob.InventoryMaxVolume;
}
Предупреждение PVS-Studio: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'builder', 'ob'. Sandbox.Game MyWeaponBlockDefinition.cs 21
Этот код отработает верно, если 'builder' будет равен 'null'. Тогда сработает 'Assert' и все будут счастливы (относительно, конечно). Если 'builder' имеет тип 'MyObjectBuilder_WeaponBlockDefinition' — опять всё хорошо. Но если 'builder' имеет значение не 'null', но при этом в результате приведения объект 'ob' будет иметь значение 'null' — проверка 'Debug.Assert (builder!= null)' пройдёт успешно, а ниже, при использовании объекта 'ob' будет сгенерировано исключение типа 'NullReferenceException'.
Я несколько подробно «разжевал» случаи, когда код отработает верно, а когда — нет, чтобы не повторять эти объяснения в последующем. Но, думаю, очевидно, что такой код содержит ошибку.
Схожая ошибка:
private void contextMenu_ItemClicked(MyGuiControlContextMenu sender,
MyGuiControlContextMenu.EventArgs args)
{
....
var actionsItem = item as MyToolbarItemActions;
if (item != null)
{
if (idx < 0 || idx >= actionsItem
.PossibleActions(ShownToolbar.ToolbarType)
.Count)
RemoveToolbarItem(slot);
....
}
....
}
Предупреждение PVS-Studio: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'item', 'actionsItem'. Sandbox.Game MyGuiControlToolbar.cs 511
Если не удалось привести объект 'item' к типу 'MyToolbarItemActions' и 'actionsItem', проверка 'item!= null' не поможет, так как проверяется не тот объект, а ниже возможно возникновение исключения типа 'NullReferenceException'.
Тогда корректно было бы исправить проверку на такую:
if (actionsItem != null)
Несколько предупреждений со схожими случаями:
- V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'ob', 'objectBuilder'. Sandbox.Game MyBlockNavigationDefinition.cs 172
- V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'Owner', 'character'. Sandbox.Game MyWelder.cs 232
Подозрительные сравнения
В версии PVS-Studio 6.01 помимо вновь добавленных, были улучшены уже существующие диагностики, причём некоторые — весьма существенно. Пример одной из таких диагностик — V3022, которая определяет, что условие всегда будет истинным или ложным.
Предлагаю рассмотреть несколько фрагментов подобного кода, отмеченные анализатором:
private long SpawnInventoryContainer(MyDefinitionId bagDefinition)
{ ... }
public override void OnCharacterDead()
{
....
var bagEntityId = SpawnInventoryContainer(
Character.Definition.InventorySpawnContainerId.Value);
if (bagEntityId != null)
....
}
Предупреждение PVS-Studio: V3022 Expression 'bagEntityId!= null' is always true. Sandbox.Game MyCharacterInventorySpawnComponent.cs 60
Так как метод 'SpawnInventoryContainer' возвращает объект типа 'long', переменная 'bagEntityId' будет иметь такой же тип. Примитивные типы вроде 'long' допустимо сравнивать с 'null' (long_var == null), но результатом этого сравнения всегда будет 'false'. Как следствие — тело оператора 'if' будет выполняться всегда. Скорее всего, здесь ожидался nullable-тип — 'long?'.
Это не единственный такой случай, встречались ещё места, где примитивные значимые типы сравнивались с 'null'. Соответствующие предупреждение анализатора:
- V3022 Expression 'info.WorkshopId == null' is always false. Sandbox.Game MyGuiBlueprintScreen.cs 326
- V3022 Expression 'info.SteamIDOwner == null' is always false. Sandbox.Game MyGuiBlueprintScreen.cs 328
- V3022 Expression 'result!= null' is always true. Sandbox.Game MyGpsCollection.cs 293
Есть и иные интересные фрагменты кода:
private new bool TestPlacement()
{
....
for (int i = 0; i < PreviewGrids.Count; ++i)
{
....
if (retval && i == 0)
{
....
var settings = i == 0 ?
m_settings.GetGridPlacementSettings(grid, false) :
MyPerGameSettings.BuildingSettings.SmallStaticGrid;
....
}
....
}
}
Предупреждение PVS-Studio: V3022 Expression 'i == 0' is always true. Sandbox.Game MyGridClipboardAdvanced.cs 790
Тоже весьма интересный случай. Тернарный оператор в коде есть, а вот толку от него нет. В условии оператора 'if' проверяется, что 'i == 0', а затем, при инициализации объекта 'settings' это условие проверяется ещё раз. Оно бы и логично, но между этими участками кода 'i' никак не изменяется, как следствие — смысла в проверке нет, 'settings' постоянно будет инициализироваться одним и тем же значением.
В том же цикле встретились ещё предупреждения:
- V3022 Expression 'i == 0? true: grid.IsStatic' is always true. Sandbox.Game MyGridClipboardAdvanced.cs 808
- V3022 Expression 'i == 0' is always true. Sandbox.Game MyGridClipboardAdvanced.cs 808
В коде встретилось несколько десятков таких предупреждений. Все их здесь рассматривать смысла нет, при желании вы можете загрузить исходный код проекта и анализатор и проверить проект самостоятельно (ссылки на код и анализатор есть в начале статьи). Благо, собирается и анализируется проект просто и быстро. Убьёте сразу нескольких зайцев — попробуете анализатор, увидите пользу от использования подобного рода инструментов, а заодно ознакомитесь с исходным кодом проекта.
И снова о разыменовании нулевых ссылок
Несмотря на то, что C# в плане обращений по нулевым ссылкам куда безопаснее, чем разыменовывание нулевых указателей в C++ (приводящих к UB), неожиданная генерация исключений типа 'NullReferenceException' всё равно крайне неприятна, особенно если эти исключения появляются у пользователей, а не проявляют себя на этапе разработки. Поэтому, если есть возможность разыменования нулевой ссылки, нужно быть крайне внимательным:
new MyEntity Entity { get; }
private static bool EnergyCritWarningMethod(out MyGuiSounds cue,
out MyStringId text)
{
....
if (MySession.ControlledEntity.Entity is MyCharacter ||
MySession.ControlledEntity == null)
....
}
Предупреждение PVS-Studio: V3027 The variable 'MySession.ControlledEntity' was utilized in the logical expression before it was verified against null in the same logical expression. Sandbox.Game MyHudWarning.cs 415
Есть необходимость выполнять какие-то действия, если 'MySession.ControlledEntity == null’или 'MySession.ControlledEntity.Entity' является типом, совместимым с 'MyCharacter'. Но так как проверка этих условий расположена не в том порядке, возможно возникновение исключения. Оно будет сгенерировано, если 'MySession.ControlledEntity == null', так как 'Entity' — экземплярное свойство. Решение — изменить порядок подвыражений:
if (MySession.ControlledEntity == null ||
MySession.ControlledEntity.Entity is MyCharacter)
Странные циклы
Встречаются в коде и ошибки с циклами, например, когда тело цикла не выполнится ни разу, всегда будет выполняться строго один раз, или же будет выполняться бесконечно. Причин много, и все они разные. Предлагаю взглянуть на один из таких циклов:
internal static void
AddDivisionForCullingStructure(List roList,
int objectCountLimit,
List resultDivision)
{
....
for (int axis = bestAxis; axis <= bestAxis; axis++)
....
}
Предупреждение PVS-Studio: V3028 Consider inspecting the 'for' operator. Initial and final values of the iterator are the same. VRage.Render MyRender-Management.cs 1034
Счётчик цикла ('axis') инициализируется значением 'bestAxis', однако условием выхода служит это же или меньшее значение, из-за чего не будет выполнено ни одной итерации цикла. Подразумевалось, что счётчик должен инициализироваться 0, тогда правильно цикл выглядел бы так:
for (int axis = 0; axis <= bestAxis; axis++)
Не менее интересный пример с циклом:
public override void Draw()
{
....
foreach (var flame in m_thrust.Flames)
{
if (m_thrust.CubeGrid.Physics == null)
continue;
....
if (m_landingEffect != null)
{
m_landingEffect.Stop(true);
m_landingEffect = null;
--m_landingEffectCount;
}
continue; // <=
....
if (m_landingEffect == null)
continue;
....
}
}
Предупреждение PVS-Studio: V3020 An unconditional 'continue' within a loop. Sandbox.Game MyRenderComponentThrust.cs 109
Ошибка заключается в том, что оператор 'continue' находится вне 'then'-ветви оператора 'if', из-за чего он будет выполняться всегда. Это означает, что весь код, следующий за этим оператором (больше 10 строк), никогда не будет выполнен. Решение проблемы очевидно — необходимо внести оператор 'continue' под условие.
Остальные предупреждения
Повторюсь, что в статьях я выписываю не все предупреждения, которые выдаёт анализатор на исходный код проекта. На анализ всех предупреждений ушло бы слишком много времени, статья по объёмам стала бы избыточно большой и читать её было бы утомительно. Но наверняка вам интересно, сколько же подозрительных мест в коде нашёл анализатор? На момент написания статьи статистика была такой:
- 75 предупреждений высокого уровня важности;
- 92 предупреждения среднего уровня важности;
- 817 предупреждений низкого уровня важности;
Стоит обязательно изучить все предупреждения высокого уровня важности и как минимум посмотреть на средний уровень. Не стоит думать, что предупреждения низкого уровня важности не содержат ничего интересного, просто на этом уровне расположились более специфичные диагностики. В любом случае, ознакомиться со списком этих сообщений стоит, так как, возможно, в нём содержится именно те специфичные диагностики, которые необходимы вашему проекту.
О пользе статического анализа и о том, как правильно использовать статический анализатор
К сожалению, приходится сталкиваться с тем, что многие люди не знают, как правильно использовать статический анализатор.
Часто считают, что приемлем следующий вариант работы: скачали анализатор, проверили проект перед релизом, что-то поправили, забыли про анализатор. О, скоро релиз! Вспомнили про анализатор, проверили проект, что-то поправили, забыли.
Это самый неправильный вариант использования. Ошибки, допущенные в момент разработки, не обнаруживаются сразу статическим анализатором, а покоятся в коде. Часть из них находит компилятор, часть — сам программист, ещё часть — тестеры. И только то, что остаётся после всего этого, находит статический анализатор, когда всё же решают прибегнуть к его помощи. Это требует массы усилий разных людей, и всё равно высока вероятность что-то пропустить. Что ещё хуже — чем дольше ошибка находится в коде, тем дороже в итоге выйдет её исправление.
Если бы анализатор использовался регулярно, большинство ошибок исправлялось бы ещё на этапе разработки, что облегчало бы задачу и программисту, и тестерам.
Вполне возможен вариант, что статический анализатор найдёт слишком много ошибок, и тогда их просто не будут править. Этого можно было бы избежать 2 способами:
- Всё те же регулярные запуски анализатора и своевременное исправление ошибок. Если ошибок изначально немного и их можно подправить, это и следует сделать, а также своевременно следить за исправлением новых. Если же сразу нашлось много ошибок, и нет возможности исправить их все, следует рассмотреть второй вариант использования;
- Скрытие (фиксация) существующих ошибок и исправление только вновь появляющихся. Это позволит не допустить появления новых ошибок и по крайней мере не увеличивать количество существующих ошибок. Зафиксированные ошибки исправляются постепенно, в ходе чего общее количество ошибок постепенно от зафиксированного значения сводится к нулю. Подробнее: практика использования анализатора PVS-Studio.
Из всего вышесказанного следует простой вывод — статический анализатор — это инструмент регулярного использования, а не разового. Именно при таком сценарии работы он полностью раскрывает свой потенциал, позволяя устранять ошибки на самых ранних этапах их появления, тогда, когда стоимость исправления ещё мала.
Заключение
Подводя итог, я не буду говорить о качестве исходного кода, о том, хороший проект или плохой — это понятия субъективные, у всех взгляды могут отличаться. Некоторое впечатление вы можете составить на основании приведённых в статье цифр (количество предупреждений) и фрагментов кода. Более полное представление вы получите, самостоятельно проверив проект и просмотрев сообщения, что я настоятельно рекомендую сделать. Это позволит составить более детальную картину о качестве кода, а также ближе познакомиться со статическим анализатором, информацию об использовании которого я пытался до вас донести в этой статье.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Sergey Vasiliev. Unicorn in Space: Analyzing the Source Code of 'Space Engineers'.
