Это вторая статья про связанные переменные и их поддержку в PVS-Studio. В этот раз мы расскажем об улучшении созданного механизма, разберём примеры из реальных проектов и увидим, какие проблемы пользователей анализатора это решает.
Что такое связанные переменные?
Говоря о связанных переменных, мы имеем в виду случаи, когда значение одной переменной зависит от значения другой. К примеру, в переменную логического типа может быть записан результат сравнения другой переменной с null.
var variable = GetPotentialNull();
bool flag = variable != null;
В таком случае проверка flag в то же время будет является и проверкой значения variable.
Переменные могут быть связаны множеством способов, и чуть далее мы разберём несколько примеров таких связей. Сейчас же давайте порассуждаем – чем такие связи мешают статическому анализатору?
Дело в том, что PVS-Studio использует технологию анализа потока данных, чтобы отслеживать возможные значения выражений. Если в условии переменная проверена на неравенство null, то анализатор понимает – в then-ветке переменная точно не хранит нулевую ссылку.
Куда сложнее дела обстоят в случае, когда проверка значения одной переменной подразумевает неявную проверку другой. Если анализ потока данных не может корректно обработать такие проверки, то анализатор будет делать неверные предположения о возможных значениях переменных. Это приводит к появлению ложных срабатываний.
Под этим термином понимается предупреждение анализатора, которое выдаётся на безошибочный код. Связанные переменные являются одной из причин появления ложных срабатываний.
Например, когда переменная неявно проверяется на неравенство null и далее разыменовывается.
public void Test()
{
var variable = GetPotentialNull();
bool check = variable != null;
if (check)
{
_ = variable.GetHashCode(); // <=
}
}
Если анализатор выдаст предупреждение на отмеченную строку, это будет false positive.
Ложные срабатывания усложняют чтение отчёта анализатора, а в некоторых случаях провоцируют программиста на совершение ошибочных правок. Подробнее о ложных срабатываниях и необходимости борьбы с ними можно прочитать в нашей статье.
Существует и другая сторона медали. Если анализатор не имеет информации о значении переменной, то он может не выдать хорошее предупреждение. Следовательно, ошибка в коде не будет вовремя обнаружена :(.
Вышедшая в апреле статья про связанные переменные и их поддержку в PVS-Studio была довольно большой. Она содержит в себе описание некоторых наших механизмов, примеры связей, причины поддержки и результат. Поэтому я рекомендую её прочитать, чтобы в этой статье всё было понятно.
В этот раз мы решили поддержать связи, которые строятся благодаря тернарному оператору и конструкции if...else. И, если вы читаете данную заметку, то мы молодцы и справились с задачей :).
Синтетика
Легче всего в проблеме связей для анализатора можно разобраться на синтетических примерах. После синтетики посмотрим на примеры кода из реальных проектов.
public void TestRelations(bool condition)
{
object variable = condition ? "notNull" : GetPotentialNull();
if (condition)
_ = variable.GetHashCode();
}
Код метода, который может возвращать null:
private static string GetPotentialNull()
{
return random.NextDouble() > 0.5 ? "str" : null;
}
Ранее PVS-Studio выдавал ложное предупреждение о потенциальном разыменовании нулевой ссылки в теле if. Очевидно, что, когда condition равен true, переменная variable имеет значение, отличное от null. Вот только очевидно это для нас, но не для анализатора. Благодаря новым правкам он понимает, что переменная condition связана с переменной variable.
С точки зрения анализатора значение variable зависит от значения condition:
если condition == true, то в variable точно не null;
если condition == false, то в variable потенциально записана нулевая ссылка.
Таким образом, когда анализатор узнаёт значение condition, то он узнаёт и значение variable. В данном примере это происходит при переходе в тело условной конструкции. Переменная condition там равна true, а значит variable точно не равна null.
Следующей проблемой были связи, которые строились благодаря if. Рассмотрим простой случай.
public void TestRelations2(bool condition)
{
object variable;
if (condition)
variable = "notNull";
else
variable = GetPotentialNull();
if (condition)
_ = variable.GetHashCode();
}
PVS-Studio выдавал предупреждение о том, что может произойти разыменование нулевой ссылки. Здесь принцип такой же, что я описал ранее с тернарным оператором. Под вторым if переменная variable не равна null. Теперь PVS-Studio учитывает и этот тип связей.
Как же мы это тестируем?
Работу анализатора мы тестируем не только на синтетике, но и на реальном коде. Для этого мы используем специальный набор Open Source проектов. Тестирование проходит в несколько этапов:
мы анализируем эти проекты текущей стабильной версией анализатора и генерируем отчёты для каждого проекта;
затем в анализатор вносятся правки, и с помощью получившейся версии создаются новые отчёты;
далее производится сравнение новых и старых отчётов и генерируются файлы с информацией о различиях.
В результате мы имеем отчёт с двумя типами записей: missing – предупреждение исчезло, additional – появилось.
Хочется отметить, что над новыми или пропавшими предупреждениями приходится подумать. При просмотре результатов я почти на каждом срабатывании анализатора задавался вопросами: хорошее ли это предупреждение? должно ли оно было пропасть или появиться? как анализатор это понял?
Так стало ли лучше?
Поддержка связанных переменных делалась для борьбы с ложными срабатываниями. Однако реализация связей помогла не только убрать плохие срабатывания, но и добавить хорошие. "Распутывание" связей позволяет PVS-Studio находить ещё больше потенциальных ошибок. Разработчик мог не подумать про связь или не понять её, да просто не заметить. В своей работе программистам приходится заниматься правкой кода и, не всегда своего. Подправил одну строчку, а у тебя уже всё не так работает, так как где-то переменные связаны. С подобными проблемами может помочь статический анализ.
Вышло интересно, так что не будем терять время и перейдём к делу :).
Additional
Сначала рассмотрим срабатывания, которые появились благодаря поддержке новых связей.
Issue 1
Первое рассматриваемое предупреждение было выдано на код проекта SpaceEngineers.
public bool RemovePilot()
{
bool usePilotOriginalWorld = false;
....
Vector3D? allowedPosition = null;
if (!usePilotOriginalWorld)
{
allowedPosition = FindFreeNeighbourPosition();
if (!allowedPosition.HasValue)
allowedPosition = PositionComp.GetPosition();
}
RemovePilotFromSeat(m_pilot);
EndShootAll();
if (usePilotOriginalWorld || allowedPosition.HasValue) // <=
{
....
}
}
V3022 Expression 'usePilotOriginalWorld || allowedPosition.HasValue' is always true. MyCockpit.cs 666
Сообщение анализатора говорит о том, что выражение usePilotOriginalWorld || allowedPosition.HasValue всегда имеет значение true. Давайте разбираться, почему же так происходит.
Поднимемся по коду чуть выше. Замечаем, что если переменная usePilotOriginalWorld равна false, то переменной allowedPosition присваивается возвращаемое значение метода FindFreeNeighbourPosition. Он возвращает nullable-структуру.
После этого возможно следующее:
allowedPosition.HasValue равно true;
allowedPosition.HasValue равно false. Тогда allowedPosition присваивается результат вызова метода GetPosition. Данный метод возвращает обычную структуру, поэтому HasValue у allowedPosition точно будет true.
Метод GetPosition:
public Vector3D GetPosition()
{
return this.m_worldMatrix.Translation;
}
Таким образом, если переменная usePilotOriginalWorld равна false, то в allowedPosition всегда будет записана nullable-структура, у которой свойство HasValue будет равно true.
Получается два варианта:
если usePilotOriginalWorld равно true, то условие истинно;
если usePilotOriginalWorld равно false, то allowedPosition.HasValue вернёт true и условие тоже будет истинно.
Кстати, ещё одно предупреждение анализатора было выдано на тот же метод.
if (usePilotOriginalWorld || allowedPosition.HasValue)
{
....
return true;
}
return false; // <=
V3142 Unreachable code detected. It is possible that an error is present. MyCockpit.cs 728
Теперь анализатор знает, что данное условие всегда истинно. В конце тела условия имеется оператор return. Следовательно, return false является недостижимым кодом. Действительно ли всё так задумано разработчиком?
Issue 2
Ещё одно новое предупреждение появилось на проекте... PVS-Studio. Да, благодаря новым доработкам мы нашли недочёт у себя же. В этом нам помогли наши ночные тесты, когда PVS-Studio ищет ошибки в PVS-Studio.
private static bool? IsTrivialProperty_internal(....)
{
AssignmentExpressionSyntax setBody = null;
if (!checkOnlyRead)
{
var setBodyFirst = setAccessorBody?.ChildNodes().FirstOrDefault();
setBody = ....;
if (setBody == null)
return false;
....
}
getValue = ....;
try
{
if (checkOnlyRead)
{
return IsTrivialGetterField(model, ref getValue, maybeTrue);
}
else
{
ExpressionSyntax setValue = setBody?.Left.SkipParenthesize(); // <=
....
}
}
catch (ArgumentException)
{....}
}
V3022 Expression 'setBody' is always not null. The operator '?.' is excessive. TypeUtils.cs 309
Предупреждение анализатора говорит о том, что в момент получения значения свойства Left переменная setBody никогда не равна null. Давайте разбираться.
Если мы попали в ветвь else, то checkOnlyRead имеет значение false. Поднимемся чуть выше, к самому первому if. Можно заметить, что при значении checkOnlyRead, равном false, осуществляется проверка setBody == null. Если это выражение имеет значение true, то произойдёт выход из метода, и до следующего if поток выполнения не дойдёт. Следовательно, если checkOnlyRead имеет значение false, то переменная setBody не может быть равна null.
Таким образом, оператор '?.' не имеет смысла и его стоит убрать. Что мы и сделали :).
Issue 3
А вот это появившееся предупреждение на проекте Umbraco заставило меня подумать. Сначала я даже полагал, что оно ложное.
private PublishResult CommitDocumentChangesInternal(....)
{
....
if (unpublishing)
{
....
if (content.Published)
{
unpublishResult = StrategyCanUnpublish(....);
if (unpublishResult.Success)
{
unpublishResult = StrategyUnpublish(....);
}
else{....}
}
else
{
throw new InvalidOperationException("Concurrency collision.");
}
}
....
if (unpublishing)
{
if (unpublishResult?.Success ?? false) // <=
{
....
}
....
}
....
}
V3022 Expression 'unpublishResult' is always not null. The operator '?.' is excessive. ContentService.cs 1553
Анализатор считает, что оператор '?.' избыточен. Давайте разбираться. Обращение к свойству Success происходит только когда переменная unpublishing равна true. Давайте проанализируем, как код метода будет выполняться в таком случае.
Поднимемся повыше и увидим такое же условие, которое, как мы выяснили, должно быть true. Заходим внутрь и встречаем на своём пути if (content.Published). Считаем, что свойство вернёт true, так как в противном случае будет сгенерировано исключение. Под этим условием локальной переменной unpublishResult в двух случаях присваивается возвращаемое значение метода. Оба вызова всегда возвращают значения, отличные от null.
Метод StrategyCanUnpublish:
private PublishResult StrategyCanUnpublish(....)
{
if (scope.Notifications.PublishCancelable(....)
{
....
return new PublishResult(....);
}
return new PublishResult(....);
}
Метод StrategyUnpublish:
private PublishResult StrategyUnpublish(....)
{
var attempt = new PublishResult(....);
if (attempt.Success == false)
{
return attempt;
}
....
return attempt;
}
Получается, что если переменная unpublishing равна true, то возможны два варианта:
будет выброшено исключение;
переменной unpublishResult будет присвоено значение, отличное от null.
Соответственно, при обращении к свойству проверку на null можно опустить. Фух, надеюсь никто не запутался.
Самые внимательные заметили, что оператор '??' в том же месте тоже не имеет смысла. Анализатор выдал для этого соответствующее сообщение:
V3022 Expression 'unpublishResult?.Success' is always not null. The operator '??' is excessive. ContentService.cs 1553
Missing
Следующие ложные срабатывания исчезли из-за поддержки связанных переменных.
Issue 1
Первым примером выступит фрагмент кода из проекта Unity:
public void DoGUI(....)
{
using (var iter = fetchData ? new ProfilerFrameDataIterator() : null)
{
int threadCount = fetchData ? iter.GetThreadCount(frameIndex) : 0; // <=
iter?.SetRoot(frameIndex, 0);
....
}
}
V3095 The 'iter' object was used before it was verified against null. Check lines: 2442, 2443. ProfilerTimelineGUI.cs 2442
Ранее PVS-Studio выдавал предупреждение о том, что в указанном месте не проверили iter на null, а вот на следующей строке проверили. Теперь же анализатор знает, что переменная iter точно не равна null в then-ветке тернарного оператора. Всё дело в том, что iter имеет значение null только в случае, когда переменная fetchData равна false, а разыменование производится лишь при fetchData == true.
Issue 2
Следующее ложное срабатывание на PascalABC.NET исчезло благодаря новым доработкам.
private void ConvertTypeHeader(ICommonTypeNode value)
{
....
TypeInfo ti = helper.GetTypeReference(value);
bool not_exist = ti == null;
....
if (not_exist)
{
ti = helper.AddType(value, tb);
}
if (value.type_special_kind == type_special_kind.array_wrapper)
{
ti.is_arr = true; // <=
}
....
}
V3080 Possible null dereference. Consider inspecting 'ti'. NETGenerator.cs 2391
Анализатор выдавал предупреждение о потенциальном разыменовании нулевой ссылки. Исчезло оно, кстати, не благодаря поддержке новых типов связей, которые я описывал на синтетике. Представленная тут связь была описана нами в прошлой статье, посвящённой связанным переменным. Почему же оно пропало только сейчас? Всё просто: мы слегка допилили общий механизм учёта подобных связей.
До места, в котором сработал анализатор, есть проверка if (not_exist). Если переменная имеет значение true, то ti присваивается возвращаемое значение метода AddType.
public TypeInfo AddType(ITypeNode type, TypeBuilder tb)
{
TypeInfo ti = new TypeInfo(tb);
defs[type] = ti;
return ti;
}
Как мы видим, данный метод не возвращает null.
На данном укороченном коде всё кажется понятным, но эти две части разделяет большое количество строк. Это усложняет понимание связи переменных, причём даже для автора кода. Данное ложное срабатывание могло запутать программиста, и даже привести к добавлению в код настоящих ошибок. Именно так поддержка связей делает жизнь пользователей анализатора проще.
Issue 3
Два следующих предупреждения на проекте PascalABC.NET я объединю в одно, так как рассматривать их лучше вместе.
public common_type_node instance(....)
{
class_definition cl_def = tc.type_dec.type_def as class_definition;
template_type_name ttn = tc.type_dec.type_name as template_type_name;
if (!tc.is_synonym)
{
if (cl_def == null)
{
throw new CompilerInternalError(....);
}
if (cl_def.template_args == null || cl_def.template_args.idents == null)
{
throw new CompilerInternalError(....);
}
}
else
{
if (ttn == null) // <=
{
throw new CompilerInternalError("No template name.");
}
}
List<SyntaxTree.ident> template_formals = (tc.is_synonym) ?
ttn.template_args.idents : cl_def.template_args.idents; // <=
if (template_formals.Count != ttn.template_args.idents.Count)
{
....
}
}
Сначала рассмотрим ложное предупреждение, которое пропало после доработок.
V3125 The 'ttn' object was used after it was verified against null. Check lines: 18887, 18880. syntax_tree_visitor.cs 18887
PVS-Studio заметил, что сначала переменная проверяется на null, а потом используется уже без проверки. Разыменование ttn происходит в случае, когда условие тернарного оператора истинно, то есть tc.is_synonym имеет значение true. Выше мы видим, что имеется конструкция if, в которой проверяется выражение !tc.is_synonim.
В рассматриваемой ситуации tc.is_synonym имеет значение true, следовательно, управление перейдёт в ветвь else. В ней ttn проверяется на равенство null. Если выражение ttn == null будет истинно, то сгенерируется исключение и поток выполнения не дойдёт до места разыменования ttn.
Противоположная ситуация происходит с cl_def. В этом случае tc.is_synonym должна иметь значение false. Получается, что обе переменные разыменовываются только в случаях, когда они не равны null.
Вместо ложного срабатывания анализатор стал выдавать новое предупреждение, которое ложным уже не является. Оно появилось на строчку ниже прошлого.
if (template_formals.Count != ttn.template_args.idents.Count)
{
....
}
V3125 The 'ttn' object was used after it was verified against null. Check lines: 18888, 18880. syntax_tree_visitor.cs 18888
Предупреждение стало выдаваться в другом месте, так как теперь PVS-Studio учитывает связи и знает, что разыменование ttn в тернарном операторе безопасно. Однако следующее обращение к ttn может приводить к исключению, так как производится безусловно. Подозрительная ситуация выходит.
Вы можете спросить: "А почему же этого предупреждения не было раньше?". Как было сказано выше, вместо него анализатор выдавал предупреждение на ситуацию в тернарном операторе. Выдавать же кучу предупреждений о потенциальном разыменовании одной и той же переменной нет смысла.
Заключение
Борьба с ложными срабатываниями и улучшение работы анализатора – наша основная задача. Поддержка связанных переменных сделает опыт использования PVS-Studio ещё лучше, чем прежде. И, конечно же, мы продолжим разработку в этом направлении, чтобы научить анализатор распутывать ещё больше связей.
Если в ваших проектах много связей между переменными, предлагаем проверить, как с ними справится новая версия PVS-Studio. Она уже доступна для скачивания на нашем сайте.
Удачного использования!
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Artem Rovenskii. PVS-Studio's data flow analysis untangles more and more related variables.
