Проект Unreal Engine развивается - добавляется новый код и изменятся уже написанный. Неизбежное следствие развития проекта - появление в коде новых ошибок, которые желательно выявлять как можно раньше. Одним из способов сокращения количества ошибок является использование статического анализатора кода PVS-Studio. Причем анализатор также быстро развивается и учится находить новые паттерны ошибок, некоторые из которых будут рассмотрены в этой статье. Если вас заботит качество кода ваших проектов, то эта статья для вас.
Публикацию подготовил Андрей Карпов, примеры кода предоставили Илья Иванов и Сергей Васильев из команды PVS-Studio. Первоисточником является Unreal Engine Blog: "Static Analysis as Part of the Development Process in Unreal Engine".
Статический анализ кода - это процесс выявления ошибок и недочетов в исходном коде программ. Статический анализ можно рассматривать как автоматизированный процесс обзора кода. Остановимся на обзоре кода чуть подробнее.
Обзор кода (code review) – один из самых старых и полезных методов выявления дефектов. Он заключается в совместном внимательном чтении исходного кода и высказывании рекомендаций по его улучшению. В процессе чтения кода выявляются ошибки или участки кода, которые могут стать ошибочными в будущем. Также считается, что автор кода во время обзора не должен давать объяснений, как работает та или иная часть программы. Алгоритм должен быть понятен непосредственно из текста программы и комментариев. Если это условие не выполняется, то код должен быть доработан.
Как правило, обзор кода хорошо работает, так как программисты намного легче замечают ошибки в чужом коде. Более подробно с методикой обзора кода можно познакомиться в замечательной книге Стива Макконнелла "Совершенный код" (Steve McConnell, "Code Complete").
У методологии обзора кода можно выделить два недостатка:
С одной стороны, хочется регулярно осуществлять обзор кода. С другой - это слишком дорого. Компромиссным решением являются инструменты статического анализа кода. Они обрабатывают исходные тексты программ и выдают программисту рекомендации обратить повышенное внимание на определенные участки кода. При этом анализаторы не устают и проверяют весь код, который затрагивается правками в заголовочных файлах. Конечно, программа не заменит полноценного обзора кода, выполняемого коллективом программистов. Однако, соотношение польза/цена делает использование статического анализа весьма полезной практикой, применяемой многими компаниями.
Как и у любой другой методологии выявления ошибок, у статического анализа есть свои сильные и слабые стороны. Нет одного идеального метода тестирования программ. Наилучший результат можно получить, используя сочетание различных подходов, таких как: хороший стиль кодирования, статический анализ кода, динамический анализ кода, юнит-тестирование, регрессионное тестирование и так далее.
Важное преимущество статического анализа состоит в возможности найти многие ошибки сразу после их появления в коде, а значит, их исправление обойдётся очень дешево.
Дело в том, что чем раньше ошибка выявлена, тем меньше стоимость ее исправления. Так, согласно данным, приведенным в книге Макконнелла "Совершенный Код", исправление ошибки на этапе тестирования обойдется в десять раз дороже, чем на этапе кодирования (написания кода):
Таблица 1. Средняя стоимость исправления дефектов в зависимости от времени их обнаружения (данные для таблицы взяты из книги С. Макконнелла "Совершенный Код").
Инструменты статического анализа позволяют выявить большое количество ошибок, характерных для этапа написания кода, что существенно снижает стоимость разработки всего проекта.
Актуальность использования статических анализаторов со временем будет только расти. Это связано с постепенным ростом размера кода современных приложений. Программы становятся всё больше, всё сложнее. При этом плотность ошибок в коде зависит от его размера нелинейно.
Чем больше проект, тем больше ошибок на 1000 строк кода он содержит. Взгляните на эту интересную таблицу:
Таблица 2. Размер проекта и типичная плотность ошибок. Источники данных: "Program Quality and Programmer Productivity" (Jones, 1977), "Estimating Software Costs" (Jones, 1998).
Чтобы было легче воспринимать данные, построим графики.
График 1. Типичная плотность ошибок в проекте. Синий - максимальное количество. Красный - среднее количество. Зелёный - наименьшее количество.
Из графика следует, что с ростом проекта программисты вынуждены использовать всё больше средств, которые позволят сохранить им требуемое качество проекта. Нельзя делать всё тоже самое для создания качественного кода, что делалось, скажем, 8 лет назад. Это может быть неприятным открытием для команды: вроде бы они пишут код как всегда, а ситуация с качеством ухудшается.
Нужно осваивать новые методологии и инструменты, иначе, с ростом проекта, старых технологий окажется недостаточно. И одна из крайне полезных методик, которую стоит использовать – это рассмотренный нами статический анализ кода.
Если читатель ещё не был знаком с методологией статического анализа кода, я надеюсь, мне удалось его заинтересовать. Для более подробного знакомства с ней я предлагаю несколько ссылок:
Теперь время перейти от теории к практике и посмотреть, как статический анализ помогает такому проекту, как игровой движок Unreal Engine.
Нашей команде вновь выпала честь поработать с кодом Unreal Engine!
Хотя мы это уже делали два года назад, но за прошедшее время для нас набралась еще работа по правке и улучшению кода. Всегда полезно и интересно посмотреть на кодовую базу такого проекта после двухлетнего перерыва. На это есть несколько причин.
Прежде всего, нам интересно посмотреть на ложные срабатывания анализатора. Что-то можно поправить в нашем инструменте, и это уменьшит количество лишних сообщений. Борьба с ложными срабатываниями – это постоянная задача разработчиков любых анализаторов кода. Тех, кого интересует эта тема, предлагаю ознакомиться со статьёй "Как и почему статические анализаторы борются с ложными срабатываниями".
За два года кодовая база Unreal Engine менялась достаточно сильно. Какие-то фрагменты пропадали, какие-то добавлялись. Иногда даже целые папки. Поэтому не все части кода получали достаточно внимания, а значит, там тоже есть работа для PVS-Studio.
Хочется похвалить компанию Epic Games за то, что она уделяет большое внимание качеству кода и использует такие инструменты как PVS-Studio. Читатель, конечно, может усмехнуться: "Конечно, ваша команда должна похвалить компанию Epic Games, ведь она является вашим клиентом". Не скрою, у нас есть мотив оставить положительный отзыв о разработчиках из Epic Games. Однако, похвалил я компанию совершенно искренне. То, что используются инструменты статического анализа кода, говорит о зрелости цикла разработки проектов и заботе компании о надёжности и безопасности кода.
Почему я уверен, что использование PVS-Studio способно заметно повысить качество кода? Потому что это один из самых мощных статических анализаторов, который легко обнаруживает ошибки даже в таких проектах, как:
Использование PVS-Studio поднимает качество кода на одну дополнительную ступеньку. Компания Epic Games заботится обо всех, кто использует движок Unreal Engine в своих проектах. Каждая исправленная ошибка уменьшит чью-то головную боль.
Рассказывать обо всех найденных и исправленных нами ошибках в Unreal Engine я не буду, а выделю только несколько, которые, на мой взгляд, заслуживают внимания. Желающие могут ознакомиться с другими ошибками, заглянув в pull request на GitHub. Для доступа к исходному коду и указанному pull request у вас должен быть доступ к репозиторию Unreal Engine на GitHub. Для этого необходимо иметь учётные записи GitHub и EpicGames, которые должны быть связаны на сайте unrealengine.com. После этого нужно принять приглашение на вступление в сообщество Epic Games на GitHub. Инструкция.
Развитие анализатора PVS-Studio связано не только с появлением новых диагностик, но и с совершенствованием уже имеющихся. Например, улучшаются алгоритмы вычисления возможных значений переменных. Благодаря этому анализатор около года назад начал выявлять ошибки вот такого вида:
uint8* Data = (uint8*)PointerVal;
if (Data != nullptr || DataLen == 0)
{
NUTDebug::LogHexDump(Data, DataLen);
}
else if (Data == nullptr)
{
Ar.Logf(TEXT("Invalid Data parameter."));
}
else // if (DataLen == 0)
{
Ar.Logf(TEXT("Invalid DataLen parameter."));
}Предупреждение анализатора PVS-Studio: V547 Expression 'Data == nullptr' is always true. unittestmanager.cpp 1924
Если условие (Data != nullptr || DataLen == 0) не выполнилось, то это означает, что указатель Data точно равен nullptr. Следовательно, дальнейшая проверка (Data == nullptr) не имеет смысла.
Правильный вариант кода:
if (Data != nullptr && DataLen > 0)Диагностика V547 существует в анализаторе с 2010 года. Однако, несовершенство механизма вычисления значений переменных не позволяло найти эту ошибку. Анализатор видел наличие в первом условии проверки значения переменной DataLen, и не мог разобраться, чему равны значения переменных в различных условиях. Для человека анализ приведённого кода не составляет никаких проблем, а вот с точки зрения написания алгоритмов для поиска таких ошибок, не всё так просто.
Я продемонстрировал усовершенствование внутренних механизмов PVS-Studio, которые позволили выявить новую ошибку. Образно говоря, эти улучшения были сделаны "вглубь", т.е. анализатор начал работать более точно.
Делаем мы и улучшения "вширь", поддерживая новые конструкции, которые появляются в новых версиях языка C++. Недостаточно учиться парсить код C++11, C++14 и так далее. Столь же важно совершенствовать старые диагностики и реализовывать новые, для выявления ошибок в новых конструкциях языка. В качестве примера приведу диагностику V714, которая ищет неправильные range-based циклы. В Unreal Engine диагностика V714 указывает на следующий цикл:
for (TSharedPtr<SWidget> SlateWidget : SlateWidgets)
{
SlateWidget = nullptr;
}Предупреждение PVS-Studio: V714 Variable is not passed into foreach loop by a reference, but its value is changed inside of the loop. vreditorradialfloatingui.cpp 170
Программист хотел присвоить значение nullptr всем элементам в контейнере SlateWidgets. Ошибка в том, что SlateWidget - это обыкновенная локальная переменная, создаваемая вновь на каждой итерации цикла. Запись в эту переменную значения не приводит к изменению элемента в контейнере. Чтобы код работал правильно, нужно создавать ссылку:
for (TSharedPtr<SWidget> &SlateWidget : SlateWidgets)
{
SlateWidget = nullptr;
}Мы, конечно, постепенно добавляем в анализатор и диагностики, которые мало связаны с языком. Например, диагностика V767 ещё не существовала в 2015 году, когда наша команда писала предыдущую статью про проверку проекта Unreal Engine. Диагностика появилась в PVS-Studio версии 6.07 (8 августа 2016). Благодаря её появлению была выявлена вот такая ошибка:
for(int i = 0; i < SelectedObjects.Num(); ++i)
{
UObject* Obj = SelectedObjects[0].Get();
EdObj = Cast<UEditorSkeletonNotifyObj>(Obj);
if(EdObj)
{
break;
}
}Предупреждение PVS-Studio: V767 Suspicious access to element of 'SelectedObjects' array by a constant index inside a loop. skeletonnotifydetails.cpp 38
В цикле должен осуществляться поиск элемента, который имеет тип UEditorSkeletonNotifyObj. Из-за случайной опечатки при выборе элемента вместо переменной i написан числовой литерал 0.
Корректный вариант кода:
UObject* Obj = SelectedObjects[i].Get();Давайте рассмотрим ещё одну из не так давно созданных диагностик V763, которая, как и V767, появилась в PVS-Studio версии 6.07. Ошибка интересная, но, чтобы её показать, мне придётся привести в статье достаточно длинное тело функции RunTest:
bool FCreateBPTemplateProjectAutomationTests::RunTest(
const FString& Parameters)
{
TSharedPtr<SNewProjectWizard> NewProjectWizard;
NewProjectWizard = SNew(SNewProjectWizard);
TMap<FName, TArray<TSharedPtr<FTemplateItem>> >& Templates =
NewProjectWizard->FindTemplateProjects();
int32 OutMatchedProjectsDesk = 0;
int32 OutCreatedProjectsDesk = 0;
GameProjectAutomationUtils::CreateProjectSet(Templates,
EHardwareClass::Desktop,
EGraphicsPreset::Maximum,
EContentSourceCategory::BlueprintFeature,
false,
OutMatchedProjectsDesk,
OutCreatedProjectsDesk);
int32 OutMatchedProjectsMob = 0;
int32 OutCreatedProjectsMob = 0;
GameProjectAutomationUtils::CreateProjectSet(Templates,
EHardwareClass::Mobile,
EGraphicsPreset::Maximum,
EContentSourceCategory::BlueprintFeature,
false,
OutMatchedProjectsMob,
OutCreatedProjectsMob);
return ( OutMatchedProjectsDesk == OutCreatedProjectsDesk ) &&
( OutMatchedProjectsMob == OutCreatedProjectsMob );
}Из приведённого кода нам важно следующее:
Далее следует проверка, что значения этих переменных удовлетворяют условию:
return ( OutMatchedProjectsDesk == OutCreatedProjectsDesk ) &&
( OutMatchedProjectsMob == OutCreatedProjectsMob );Не ищите ошибки в теле рассмотренной функции, их там нет. Этот код я привёл для того, чтобы показать, что от функции CreateProjectSet ожидают, что она запишет значения в две переменные, переданные ей как два последних фактических аргумента.
Ошибка подстерегает нас как раз в функции CreateProjectSet:
static void CreateProjectSet(.... int32 OutCreatedProjects,
int32 OutMatchedProjects)
{
....
OutCreatedProjects = 0;
OutMatchedProjects = 0;
....
OutMatchedProjects++;
....
OutCreatedProjects++;
....
}Инструмент PVS-Studio выдаст здесь два предупреждения:
Анализатор совершенно прав, когда предупреждает, что значения аргументов OutCreatedProjects и OutMatchedProjects никак не используются, а сразу перезаписываются значением 0.
Ошибка проста: забыли сделать переменные ссылками. Правильный вариант кода:
static void CreateProjectSet(.... int32 &OutCreatedProjects,
int32 &OutMatchedProjects)Выше я привёл ошибки, которые требуют хоть какой-то внимательности для своего обнаружения. Но намного больше совсем простых ошибок. Например, пропущены операторы break:
{
case EWidgetBlendMode::Opaque:
ActualBackgroundColor.A = 1.0f;
case EWidgetBlendMode::Masked:
ActualBackgroundColor.A = 0.0f;
}Или неправильное сравнение нескольких переменных на равенство:
checkf(GPixelFormats[PixelFormat].BlockSizeX
== GPixelFormats[PixelFormat].BlockSizeY
== GPixelFormats[PixelFormat].BlockSizeZ
== 1,
TEXT("Tried to use compressed format?"));Если кто-то является новичком в C++ и не понял, что в этом сравнении неправильного, предлагаю заглянуть в описание диагностики V709.
Таких ошибок большинство, среди обнаруженных PVS-Studio. Но раз они так просто выглядят, почему остаются незамеченными?
Эти ошибки просты, когда они вот так вот выделены в статье для читателя. В коде реальных приложений заметить их крайне сложно. Даже при обзорах кода взгляд может скользить по блоку
{
case EWidgetBlendMode::Opaque:
ActualBackgroundColor.A = 1.0f;
case EWidgetBlendMode::Masked:
ActualBackgroundColor.A = 0.0f;
}и не замечать в нем ошибки. Код кажется столь простым, что программист даже не вчитывается в него, считая его совершенно корректным.
Давайте теперь немного обсудим вопрос: можно ли как-то сократить количество таких ошибок?
Описанные в статье ошибки найдены с помощью PVS-Studio и, скорее всего, читатель ожидает, что я порекомендую использовать инструменты статического анализа кода. Да, я рекомендую интегрировать в процесс разработки статический анализатор кода PVS-Studio. Незачем отказывать себе в возможности найти ряд ошибок сразу после написания кода.
Однако, в этом разделе я хотел обсудить очень важный момент, который, как правило, не упоминается в статьях, посвящённых качеству кода.
Невозможно достичь высокого качества проекта, пока коллектив программистов не признает для себя, что он допускает ошибки, в том числе, простые.
Звучит эта фраза банально, но она очень важна. Пока программист не осознает, что это утверждение относится не к абстрактному программисту, а именно к нему самому, никакой инструмент или методология не пойдёт на пользу. Другими словами, программисты часто слишком горды, чтобы признать, что им нужны вспомогательные инструменты и методики для написания качественного кода.
Все программисты знают, что в программах есть ошибки. Однако считают, что правила, рекомендации и инструменты существуют не для них, так как они - профессиональные разработчики и пишут хороший код.
Это проблема переоценки человеком своего уровня. Про нее хорошо написано в статье "Программисты 'выше среднего'" (en). Процитирую отрывок:
Как вы оцениваете свой уровень как программиста (ниже среднего, средний, выше среднего)?
Согласно психологическим опросам среди разных групп, около 90% программистов отвечают "Выше среднего".
Очевидно, это не может быть правдой. В группе из 100 человек 50 всегда будут выше и 50 — ниже среднего. Этот эффект известен как иллюзия превосходства. Он описан во многих областях, но даже если вы об этом слышали, вы всё равно, вероятно, ответите "выше среднего".
Это проблема, которая мешает программистам осваивать новые технологии и методологии. И самая главная моя рекомендация - попробовать переосмыслить свое отношение к собственной работе и работе команды в целом. Позиция "я/мы пишем отличный код" контрпродуктивна. Людям свойственно ошибаться, и программистам - тоже.
Осмыслив всё это, можно сделать самый большой шаг в направлении качественного программного обеспечения.
Примечание. Менеджерам проектов я дополнительно предлагаю заглянуть в эту статью.
Хотел предостеречь от одной ошибки рассуждений. Статические и динамические анализаторы, в основном, находят достаточно простые ошибки и опечатки. Да, они не найдут высокоуровневую ошибку в алгоритме, так как искусственный интеллект пока не изобрели. Однако, простая ошибка может нанести большой вред и отнять много времени/сил/денег. Подробнее: "Простая ошибка при кодировании - не значит нестрашная ошибка".
И ещё: не ищите серебряную пулю. Используйте сочетание различных элементов, таких как:
Я не призываю начать использовать сразу всё перечисленное. В разных проектах что-то будет более полезным, что-то менее. Главное - не надеяться на что-то одно, а использовать разумное сочетание методик. Только это даст повышение качества и надёжности кода.
Разработчики Unreal Engine заботятся о качестве своего кода, а команда PVS-Studio, по мере своих сил, помогает им в этом.
Команда PVS-Studio готова поработать с кодом и ваших проектов. Помимо продажи анализатора PVS-Studio и его сопровождения, мы проводим аудит кода, миграцию кода и т.д.
Желаю всем как можно меньше багов в программах.