# Анализ потока данных PVS\-Studio распутывает всё больше связанных переменных

Это вторая статья про связанные переменные и их поддержку в PVS\-Studio\. В этот раз мы расскажем об улучшении созданного механизма, разберём примеры из реальных проектов и увидим, какие проблемы пользователей анализатора это решает\. 

![0976_Related_variables_upgrade_ru/image1.png](https://import.viva64.com/docx/blog/0976_Related_variables_upgrade_ru/image1.png)

## Что такое связанные переменные?

Говоря о связанных переменных, мы имеем в виду случаи, когда значение одной переменной зависит от значения другой\. К примеру, в переменную логического типа может быть записан результат сравнения другой переменной с _null_\.

```cpp
var variable = GetPotentialNull();
bool flag = variable != null;
```

В таком случае проверка _flag_ в то же время будет является и проверкой значения _variable_\.

Переменные могут быть связаны множеством способов, и чуть далее мы разберём несколько примеров таких связей\. Сейчас же давайте порассуждаем – чем такие связи мешают статическому анализатору?

Дело в том, что PVS\-Studio использует технологию [анализа потока данных](https://pvs-studio.ru/ru/blog/terms/7004/), чтобы отслеживать возможные значения выражений\. Если в условии переменная проверена на неравенство _null_, то анализатор понимает – в then\-ветке переменная точно не хранит нулевую ссылку\. 

Куда сложнее дела обстоят в случае, когда проверка значения одной переменной подразумевает неявную проверку другой\. Если анализ потока данных не может корректно обработать такие проверки, то анализатор будет делать неверные предположения о возможных значениях переменных\. Это приводит к появлению ложных срабатываний\.

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

Например, когда переменная неявно проверяется на неравенство _null_ и далее разыменовывается\.

```cpp
public void Test()
{
  var variable = GetPotentialNull();
  bool check = variable != null;
  if (check)
  {
    _ = variable.GetHashCode(); // <=
  }
}
```

Если анализатор выдаст предупреждение на отмеченную строку, это будет false positive\.

Ложные срабатывания усложняют чтение отчёта анализатора, а в некоторых случаях провоцируют программиста на совершение ошибочных правок\. Подробнее о ложных срабатываниях и необходимости борьбы с ними можно прочитать в нашей [статье](https://pvs-studio.ru/ru/blog/posts/cpp/0488/)\. 

Существует и другая сторона медали\. Если анализатор не имеет информации о значении переменной, то он может не выдать хорошее предупреждение\. Следовательно, ошибка в коде не будет вовремя обнаружена :\(\.

Вышедшая в апреле [статья](https://pvs-studio.ru/ru/blog/posts/csharp/0942/) про связанные переменные и их поддержку в PVS\-Studio была довольно большой\. Она содержит в себе описание некоторых наших механизмов, примеры связей, причины поддержки и результат\. Поэтому я рекомендую её прочитать, чтобы в этой статье всё было понятно\.

В этот раз мы решили поддержать связи, которые строятся благодаря тернарному оператору и конструкции _if\.\.\.else_\. И, если вы читаете данную заметку, то мы молодцы и справились с задачей :\)\.

## Синтетика

Легче всего в проблеме связей для анализатора можно разобраться на синтетических примерах\. После синтетики посмотрим на примеры кода из реальных проектов\.

```cpp
public void TestRelations(bool condition)
{
  object variable = condition ? "notNull" : GetPotentialNull();
  if (condition)
    _ = variable.GetHashCode();
}
```

Код метода, который может возвращать _null_:

```cpp
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_\. Рассмотрим простой случай\.

```cpp
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](https://github.com/KeenSoftwareHouse/SpaceEngineers)\.

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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_:

```cpp
public Vector3D GetPosition()
{
  return this.m_worldMatrix.Translation;
}
```

Таким образом, если переменная _usePilotOriginalWorld_ равна _false_, то в _allowedPosition_ всегда будет записана nullable\-структура, у которой свойство _HasValue_ будет равно _true_\.

Получается два варианта: 

* если _usePilotOriginalWorld _равно _true_, то условие истинно; 
* если _usePilotOriginalWorld _равно _false_, то _allowedPosition\.HasValue_ вернёт _true_ и условие тоже будет истинно\.

Кстати, ещё одно предупреждение анализатора было выдано на тот же метод\.

```cpp
if (usePilotOriginalWorld || allowedPosition.HasValue)
{
  ....
  return true;
}
return false;    // <=
```

[V3142](https://pvs-studio.ru/ru/docs/warnings/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\. 

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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](https://github.com/umbraco/Umbraco-CMS) заставило меня подумать\. Сначала я даже полагал, что оно ложное\.

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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_:

```cpp
private PublishResult StrategyCanUnpublish(....)
{
  if (scope.Notifications.PublishCancelable(....)
  {
    ....
    return new PublishResult(....);
  }
  return new PublishResult(....);
}
```

Метод _StrategyUnpublish_:

```cpp
private PublishResult StrategyUnpublish(....)
{
  var attempt = new PublishResult(....);
  if (attempt.Success == false)
  {
    return attempt;
  }
  ....
  return attempt;
}
```

Получается, что если переменная _unpublishing_ равна _true_, то возможны два варианта:

* будет выброшено исключение;
* переменной _unpublishResult_ будет присвоено значение, отличное от _null_\.

Соответственно, при обращении к свойству проверку на _null_ можно опустить\. Фух, надеюсь никто не запутался\.

Самые внимательные заметили, что оператор '??' в том же месте тоже не имеет смысла\. Анализатор выдал для этого соответствующее сообщение:

[V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'unpublishResult?\.Success' is always not null\. The operator '??' is excessive\. ContentService\.cs 1553

### Missing

Следующие ложные срабатывания исчезли из\-за поддержки связанных переменных\.

**Issue 1**

Первым примером выступит фрагмент кода из проекта [Unity](https://github.com/Unity-Technologies/UnityCsReference):

```cpp
public void DoGUI(....)
{
  using (var iter = fetchData ? new ProfilerFrameDataIterator() : null)
  {
    int threadCount = fetchData ? iter.GetThreadCount(frameIndex) : 0; // <=
    iter?.SetRoot(frameIndex, 0);
    ....
  }
}
```

[V3095](https://pvs-studio.ru/ru/docs/warnings/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](https://github.com/pascalabcnet/pascalabcnet) исчезло благодаря новым доработкам\.

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v3080/) Possible null dereference\. Consider inspecting 'ti'\. NETGenerator\.cs 2391

Анализатор выдавал предупреждение о потенциальном разыменовании нулевой ссылки\. Исчезло оно, кстати, не благодаря поддержке новых типов связей, которые я описывал на синтетике\. Представленная тут связь была описана нами в прошлой [статье](https://pvs-studio.ru/ru/blog/posts/csharp/0942/), посвящённой связанным переменным\. Почему же оно пропало только сейчас? Всё просто: мы слегка допилили общий механизм учёта подобных связей\.

До места, в котором сработал анализатор, есть проверка _if \(not\_exist\)_\. Если переменная имеет значение _true_, то _ti_ присваивается возвращаемое значение метода _AddType_\.

```cpp
public TypeInfo AddType(ITypeNode type, TypeBuilder tb)
{
  TypeInfo ti = new TypeInfo(tb);
  defs[type] = ti;
  return ti;
}
```

Как мы видим, данный метод не возвращает _null_\. 

На данном укороченном коде всё кажется понятным, но эти две части разделяет большое количество строк\. Это усложняет понимание связи переменных, причём даже для автора кода\. Данное ложное срабатывание могло запутать программиста, и даже привести к добавлению в код настоящих ошибок\. Именно так поддержка связей делает жизнь пользователей анализатора проще\.

**Issue 3**

Два следующих предупреждения на проекте [PascalABC\.NET](https://github.com/pascalabcnet/pascalabcnet) я объединю в одно, так как рассматривать их лучше вместе\.

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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_\.

Вместо ложного срабатывания анализатор стал выдавать новое предупреждение, которое ложным уже не является\. Оно появилось на строчку ниже прошлого\.

```cpp
if (template_formals.Count != ttn.template_args.idents.Count)
{
  ....
}
```

[V3125](https://pvs-studio.ru/ru/docs/warnings/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\. Она уже доступна для скачивания на нашем [сайте](https://pvs-studio.ru/ru/pvs-studio/try-free/)\. 

Удачного использования\!