Эволюция PVS-Studio: анализ потока данных для связанных переменных

Связанные переменные – одна из главных проблем статического анализа. Данная статья посвящена разбору этой темы и рассказу о том, как разработчики PVS-Studio сражаются с ложными срабатываниями, появившимися из-за различных связей.

О чём статья?

Команда разработки PVS-Studio постоянно работает над повышением качества анализа. Но мало просто добавить в продукт улучшение – о нём, конечно же, надо рассказать! И сегодня мы решили поведать о том, как связи между переменными мешают статическому анализатору и как C# анализатор PVS-Studio старается их учитывать. Приятного чтения!

Немного об анализе потока данных

Начнём немного издалека. Одним из важнейших механизмов, используемых C# анализатором PVS-Studio, является анализ потока данных (data flow analysis). Если не вдаваться в детали, это технология, позволяющая анализатору отслеживать возможные значения переменных. В PVS-Studio анализ потока данных тесно взаимодействует с другими технологиями, о которых можно прочитать здесь.

Числовые и логический типы

Возможности анализа потока данных проще всего продемонстрировать на примере числовых и логических переменных:

int a = 5;
int b = 3;
bool flag = a > b;

if (flag) // always true
{
  ....
}

Анализ потока данных позволяет PVS-Studio рассчитать точное значение flag и сообщить о том, что проверка не имеет смысла, ведь a всегда больше b.

Во многих случаях выражения и, в частности, переменные могут иметь любое значение из некоторого множества. Например:

void MyFunc(bool flag)
{
  int a = flag ? 1 : 10;
  bool greater = a > 5;

  if (greater)
    Console.WriteLine("a > 5");

  if (a == 5) 
    Console.WriteLine("a = 5");
}

В зависимости от значения, которое будет передано в параметр flag, переменная a будет равна 1 или 10. Следовательно, значение переменной greater может быть как true, так и false. Поэтому анализатор не будет считать проверку значения greater бессмысленной.

С другой стороны, PVS-Studio точно знает, что a никогда не равно 5. Именно поэтому анализатор выдаст предупреждение:

V3022 Expression 'a == 5' is always false.

В некоторых случаях "лишние" проверки появляются из-за опечаток или логических ошибок. К примеру, иногда программист проверяет значение не той переменной, которой нужно.

Null-state анализ

С переменными ссылочных типов дела обстоят несколько иначе. Здесь анализатор отслеживает возможность равенства null, то есть производит null-state анализ. Каждая переменная ссылочного типа с точки зрения PVS-Studio может быть в одном из 4 состояний:

• Unknown – когда нет информации о том, может переменная быть равна null или нет. Данное состояние все переменные ссылочных типов имеют по умолчанию;

• Null – когда переменная точно равна null;

• NotNull – когда переменная точно не равна null;

• PotentialNull – когда в некоторых случаях переменная точно равна null.

Пример:

void TestReferences(bool flag)
{
  string potentialNullStr = flag ? "not null" : null;
    
  _ = potentialNullStr.GetHashCode();
}

На момент вызова GetHashCode переменная potentialNullStr может быть равна или не равна null. Разыменование ссылки, потенциально имеющей значение null, может привести к выбрасыванию исключения, поэтому анализатор генерирует соответствующее предупреждение:

V3080 Possible null dereference. Consider inspecting 'potentialNullStr'.

Что же делать? Самое простое – всё-таки проверить, что переменная не равна null:

void TestReferences(bool flag)
{
  string potentialNullStr = flag ? "not null" : null;
    
  if (potentialNullStr != null)
    _ = potentialNullStr.GetHashCode();
}

Анализатор сможет легко вычислить, что в теле оператора if переменная potentialNullStr точно не равна null, а значит, вызов GetHashCode не приведёт к выбрасыванию исключения.

Связанные переменные

Всё бы хорошо, но иногда в реальном коде проверки на null производятся более изощрённым способом. И нет, речь не про null-условный оператор – его поддержка является более-менее тривиальной задачей. В самом простом случае нам достаточно просто не ругаться, если доступ к члену осуществляется через "?.". Реальной же сложностью является именно проверка на null с помощью связанной переменной.

Чтобы лучше понять тему, давайте вернёмся к ранее приведённому примеру:

public void TestReferences(bool flag)
{
  string potentialNull = flag ? "not null" : null;

  if (potentialNull != null)
    _ = potentialNull.GetHashCode();
}

В переменную potentialNull может быть записан null. Однако перед разыменованием есть проверка, и анализ потока данных это учитывает. Но что если проверка на null производится неявно?

public void TestReferences(bool flag)
{
  string potentialNull = flag ? "not null" : null;

  if (flag)
    _ = potentialNull.GetHashCode();
}

С точки зрения статического анализатора значение flag неизвестно, а значит, в potentialNull может быть записан null. Дальнейшая проверка не даёт никакой информации о potentialNull, ведь эта переменная в условии даже не используется. Соответственно, анализатор предупредит о потенциальном разыменовании нулевой ссылки.

На самом же деле если flag = true, то и в potentialNull записана строка. Проверки на равенство null как бы нет, но никакого разыменования нулевой ссылки тут быть не может.

Связи могут быть построены множеством способов. Ранее мы рассмотрели пример с переменными логического и ссылочного типов. Однако в общем случае что угодно может зависеть от чего угодно. К примеру, ниже представлена связь двух переменных ссылочных типов:

public void RelatedVariables2(string param)
{
  string? potentialNull = param != null ? "not null" : null;

  if (param != null)
  {
    _ = potentialNull.GetHashCode();
  }
}

Переменная potentialNull равна null только в случае, если param равен null. Иначе говоря, либо обе переменные равны null, либо обе не равны. Соответственно, вызов GetHashCode здесь никогда не приведёт к выбросу исключения.

Но что-то я зациклился на переменных ссылочных типов. Давайте рассмотрим другой пример:

public void RelatedVariables3(int a, int[] array)
{
  int b = 0;
  int index = -1;

  if (a == 0)
  {
    b = 10;
    index = 1;
  }

  if (b > 0)
  {
    _ = array[index];
  }
}

Взгляните на этот код и скажите – может ли здесь произойти попытка обращения к элементу с индексом -1?

Пожалуй, в таком примере даже человек не сразу разберётся. Переменная index не может быть равна -1, если b > 0. Ведь b > 0, только если a = 0, а если a = 0, то и index = 1. Надеюсь, вы не запутались :).

Приведённые примеры являются синтетическими, и на реальном коде такое встречается не так часто. Тем не менее, пользователи анализатора время от времени сообщают о false positive срабатываниях, причиной которых является связанность переменных. К примеру, недавно нам написали о проблеме обработки кода следующего вида:

public void Test()
{
  var a = GetPotentialNull();
  bool z = a != null;

  if (z)
  {
    _ = a.GetHashCode(); // <=
  }
}

Увы и ах, раньше анализатор совершенно бессовестно лгал о возможном разыменовании нулевой ссылки!

Это, конечно, не катастрофа. Ложные срабатывания в общем случае неизбежны, но анализатор предоставляет различные возможности для работы с ними. Самое простое, что можно сделать – отметить предупреждение как ложное, чтобы оно не мозолило глаза. Подробнее об этом можно прочитать здесь.

Тем не менее, мы ведём беспрестанную борьбу с ложными срабатываниями, чтобы пользователи не тратили своё время на их изучение. Эта тема, кстати, хорошо раскрыта в статье "Как и почему статические анализаторы борются с ложными срабатываниями". Рекомендую к прочтению :).

Ты не туда воюешь!

Возможно, вам покажется, что мне не стоило всё это рассказывать. Ведь я говорю о слабостях статического анализа! Выглядит так, будто я играю не за ту команду :).

Вот только на самом деле это совсем не так. Подобные статьи в первую очередь посвящены развитию анализатора, рассказу о том, как мы смогли сделать продукт лучше. А любое развитие начинается именно с признания проблем. Анализатор работает неидеально? Да. Иной раз не ругается там, где нужно, а иной раз выдаёт ложные срабатывания? Бывает. Но мы действительно работаем над этим. Клиенты пишут нам о проблемах, которые их волнуют больше всего, и мы делаем всё, чтобы сделать PVS-Studio лучше.

Статьи же подобного плана позволяют нам рассказать миру о достигнутых успехах :). Кстати, о них...

PVS-Studio и связанные переменные

Разнообразие возможных связей переменных поражает, и их поддержка – задача весьма нетривиальная. Однако бороться с ложными срабатываниями нужно, поэтому мы решили постепенно покрывать наиболее распространённые способы связей.

Перед тем как начнём, небольшое отступление.

Многие фрагменты, представленные в этой статье, являются синтетическими. Они могут казаться странными, и вообще "непонятно, кто такое пишет", но, поверьте, все примеры основаны на реальном коде. Примеры являются максимально простыми, но при этом позволяют воспроизвести то или иное поведение анализатора.

Как разработчики PVS-Studio, мы очень благодарны пользователям за то, что они пишут нам о проблемах, с которыми столкнулись (в том числе и о ложных срабатываниях). Но ещё больше мы рады, когда нам присылают именно такие простые примеры кода, на которых некорректное поведение легко воспроизводится. Это невероятно ускоряет процесс внесения нужных исправлений :).

Эвристический алгоритм

Первым решением, позволившим избавиться от большого количества ложных срабатываний, стал специальным образом подобранный алгоритм. Он частично исключает предупреждения, появляющиеся из-за неявных связей различных значений с переменными ссылочных типов.

Изучая ложные срабатывания, мы заметили интересную закономерность. Если разыменование производится в теле условной конструкции, то null-state соответствующей переменной, скорее всего, связан с выражением в условии. Иначе говоря, разыменование, производимое под условием, обычно было безопасным, так как соответствующая ссылка неявно проверялась с помощью связанной переменной.

Приведём пример:

void Test(bool condition)
{
  object a;
  if (condition)
    a = new object();
  else
    a = null;

  ....

  if (condition)
    _ = a.ToString();
}

Так как разыменование переменной a производится в теле условной конструкции, анализатор как бы предполагает наличие связи между a и условием. За счёт этого PVS-Studio не будет выдавать предупреждение. В данном случае срабатывание на вызов ToString действительно было бы ложным, так как если condition = true, то a не равно null.

В таком виде алгоритм отсекал слишком много хороших срабатываний, поэтому мы стали думать над доработками. Наилучших результатов удалось добиться, добавив дополнительное условие исключения: установка null должна быть в том же методе, в котором производится разыменование. Именно в этих случаях null-state обычно связан с условием.

Приведём пример, в котором null может быть получен из другого метода:

bool _flag;

object GetPotentialNull() => _flag ? "not null" : null;

void Test(bool condition)
{
  object a = GetPotentialNull();

  if (condition)
    _ = a.ToString();
}

Переменная a действительно разыменовывается под условием, однако никакой связи между ней и condition нет. Данная эвристика позволила "спасти" многие хорошие срабатывания, хотя и добавила немного ложных.

Долгое время данный алгоритм был основным средством против связанных переменных. С его помощью и сейчас получается убирать значительную часть ложных срабатываний на коде реальных проектов. И всё же результаты работы такого исключения неидеальны: иной раз отсекаются хорошие срабатывания, а иногда напротив – "пропускаются" ложные. И если потеря пары хороших срабатываний является не столь критичной проблемой, то с ложными нужно что-то делать.

Не такое уж и бессмысленное присваивание

Вообще клиенты обычно не пишут о том, что нам стоит "поддержать связанные переменные". Это даже звучит очень абстрактно! Как пользователи, они заинтересованы именно в качественном выводе PVS-Studio, а как и что там внутри работает – это не так важно. Поэтому и пишут нам о конкретных ложных срабатываниях, выданных анализатором. Мы же уже разбираемся, в чём именно проблема, и стараемся её решить.

И вот в один прекрасный день пишет нам пользователь о предупреждении, выданном на код следующего вида:

static void Foo()
{
  Holder h = new Holder();
  Parameter p = h.GetParam();

  p.Text = "ABC"; // <=
  h.f();
  p.Text = "XYZ"; // <=
  h.f();
}

V3008 The 'p.Text' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 35, 33.

Предупреждение говорит о бессмысленности первого присваивания – значение "ABC" никак не используется. Что-то тут не так, код нужно изучить и поправить...

А вот и нет! Присваивание вовсе не бессмысленное. Но как же так? Первая мысль, которая может возникнуть, – нужно посмотреть, что это за свойство такое – Text. Может быть, присваивание этому свойству на что-то влияет? Ничего подобного:

class Parameter
{
  internal string Text { get; set; }
}

Обычное автосвойство. Присваивание ему значения не приводит к выполнению каких-то особых действий. Соответственно, присваивать ему значение 2 раза подряд... Несколько странно. Однако срабатывание всё же ложное.

Чтобы наконец стало понятно, в чём же тут дело, следует взглянуть на класс Holder:

class Holder
{
  private Parameter param;
  internal Parameter GetParam() 
  {
    return param;
  }
  
  internal Holder() 
  {
    param = new Parameter();
    param.Text = "";
  }
  
  internal void f()
  {
    Console.WriteLine("Holder: {0}", param.Text);
  }
}

Оказывается, метод f использует значение свойства param.Text. Зная это, давайте вернёмся к исходному примеру:

static void Foo()
{
  Holder h = new Holder();
  Parameter p = h.GetParam();

  p.Text = "ABC";
  h.f();
  p.Text = "XYZ";
  h.f();
}

Фактически в переменную p записывается ссылка на поле param объекта h. При вызове метода f это поле используется – точнее, используется его свойство Text. При первом вызове f в Text записано "ABC", а при втором – "XYZ". Таким образом, каждое присваивание сыграло свою роль и никакой ошибки тут нет.

В данном случае ложное срабатывание вызвано наличием достаточно необычной связи между свойством p.Text и переменной h. Вызов h.f() использует значение, записанное в p.Text, и диагностике нужно это как-то учитывать.

Для решения данной проблемы мы решили доработать одно из исключений, которые уже были в диагностике. Суть исключения состоит в предотвращении срабатывания для случаев, когда между двумя присваиваниями объект был использован. Например:

void Test()
{
  int a, x;
  a = 10;
  x = a; // a is used
  a = 20;
}

Анализатор не срабатывает на таком коде, так как переменная a была использована между присваиваниями ей значений. В отличие от предыдущего случая, здесь переменная a используется явно, поэтому и исключить срабатывание здесь легко. Но что делать, когда использование присвоенного значения производится неявно, при вызове метода? Давайте разбираться:

static void Foo()
{
  Holder h = new Holder();
  Parameter p = h.GetParam();

  p.Text = "ABC";
  h.f();        // p.Text is used here
  p.Text = "XYZ";
  h.f();        // and here
}

Чтобы решить проблему, мы решили доработать правило V3008. Теперь при исследовании кода эта диагностика сохраняет пары потенциально связанных переменных. Если одна из них каким-либо способом используется, то использованной считается и другая. p считается потенциально связанной с h, так как её значение получено при вызове h.GetParam(). В то же время вызов h.f() означает не только использование h, но и потенциальное использование связанной с ней p и её свойств. Поэтому и срабатывание на "лишнее присваивание" p.Text больше не генерируется.

Реальный пример связи

Синтетика – это, конечно, хорошо, но кому она интересна? Да, круто, что анализатор стал лучше работать на выдуманных примерах. Правда толку от этого немного, если люди попросту не пишут код, на котором улучшение заметно. Про оценку анализаторов на синтетических примерах, кстати, есть достаточно яркая заметка. Там про C++, но суть не меняется.

У нас же совсем другой случай. Во-первых, правка вносилась в первую очередь по просьбе клиента, то есть как минимум на коде в его проекте от ложных срабатываний мы избавились. Во-вторых, улучшения работы анализатора хорошо заметны и на других реальных проектах. К примеру, давайте взглянем на код из RavenDB, который мы используем для тестирования PVS-Studio:

[Fact]
public void CRUD_Operations_With_Array_In_Object_2()
{
  ....
  var family = new Family()
  {
    Names = new[] { "Hibernating Rhinos", "RavenDB" }
  };
  newSession.Store(family, "family/1");
  newSession.SaveChanges();

  var newFamily = newSession.Load<Family>("family/1");

  newFamily.Names = new[] {"Hibernating Rhinos", "RavenDB"};   // <=
  Assert.Equal(newSession.Advanced.WhatChanged().Count, 0);

  newFamily.Names = new[] { "RavenDB", "Hibernating Rhinos" }; // <=
  Assert.Equal(newSession.Advanced.WhatChanged().Count, 1);

  newSession.SaveChanges();
  ....
}

V3008 The 'newFamily.Names' variable is assigned values twice successively. Perhaps this is a mistake.

Итак, анализатор сообщал, что в newFamily.Names дважды присваивается значение, при этом первое значение никак не используется. И действительно, по коду явного использования не видно. Но что если взглянуть получше?

Объект класса Family сохраняется в сессию. На этот момент он содержит имена "Hibernating Rhinos" и "RavenDB". Затем этот же объект (или, как минимум, объект, содержащий те же значения) из сессии загружается. После этого в него записываются те же самые имена. А далее производится вызов:

Assert.Equal(newSession.Advanced.WhatChanged().Count, 0);

Очевидно, что эта проверка учитывает записанное ранее значение. Данный тест проверяет, что изменений нет – ведь имена записаны те же самые. Чуть ниже по коду имена меняются местами и опять производится подобная проверка. Там уже ожидается именно наличие изменений. Очевидна связь между вызовами newSession.Advanced.WhatChanged() и newFamily.Names.

Получается, выдавать тут предупреждения про "ненужное" присваивание не стоит. И знаете что? Теперь PVS-Studio не будет делать этого :). Следовательно, разработчики не будут тратить время на исследование лишних срабатываний.

Мы находили и другие примеры исчезновения ложных срабатываний. Однако все они сильно напоминают то, что мы разобрали ранее, поэтому предлагаю перейти к следующему разделу.

Связи через оператор as

Не успели мы порадоваться победе над ложными срабатываниями, сообщавшими о "лишних" присваиваниях, как другой клиент прислал нам новый пример:

void Test(object obj)
{
  if (obj != null)
    Console.WriteLine("obj is not null");

  string str = obj as string;

  if (str != null)
    Console.WriteLine(obj.GetHashCode()); // <=
}

V3125 The 'obj' object was used after it was verified against null.

Ну, давайте разбираться.

Вначале параметр obj проверяется на равенство null. Получается, что метод предполагает: в obj может быть передана нулевая ссылка. Затем c помощью оператора as производится преобразование obj к типу String, а результат записывается в переменную str.

Самое интересное происходит дальше. Если str не равна null, то производится обращение к методу GetHashCode. Однако вызывается он не у str, а у obj! Получается, что тут проверили не ту переменную. Даже если str не равна null, то obj всё ещё остаётся потенциальным null-значением.

По крайней мере так может показаться. На самом деле, если str != null, то и obj != null. Почему?

Допустим, obj действительно равен null. Тогда первая проверка даст false – ну и ладно. Далее происходит вычисление значения для str. Так как переменная obj равна null, то и в str точно будет null. Из этого следует обратный вывод: если в str не null, то и в obj тоже не null.

Круто, конечно, что мы это поняли, но надо бы и анализатор тоже научить. В этом нам помогает существующая реализация Data Flow. Для подходящих выражений из анализируемого кода PVS-Studio создаёт специальные объекты, которые хранят информацию о возможных значениях. Такие объекты мы называем виртуальными значениями. В них также содержатся и вспомогательные данные, широко используемые диагностиками. К примеру, Data Flow отслеживает, является ли значение переменной:

• результатом вызова FirstOrDefault;

• потенциально заражённым (подробнее тут);

• результатом приведения через оператор as;

• и так далее.

Чтобы стало понятно, как именно анализатор начал учитывать связи через оператор as, вернёмся к примеру:

void Test(object obj)
{
  if (obj != null)
    Console.WriteLine("obj is not null");

  string str = obj as string;

  if (str != null)
    Console.WriteLine(obj.GetHashCode());
}

В переменную str записывается результат приведения obj через оператор as. Data Flow запишет в соответствующее виртуальное значение информацию об этом. Данный функционал уже был реализован и активно использовался некоторыми правилами. Одно из них — V3149.

В момент обработки условия str != null анализатор вычисляет, что если это выражение истинно, то str точно не равна null. При этом анализатору уже известно, что значение str получено в результате приведения obj через оператор as. Получается, что и значение obj также вполне справедливо может считаться не равным null.

Реальные примеры связей через оператор as

Честно говоря, мы даже сами не ожидали такого результата, но пропала целая куча ложных срабатываний. Кто бы мог подумать, что такая вот проверка на null через оператор as встречается столь часто?

Issue 1

В качестве первого примера приведу фрагмент кода из проекта SpaceEngineers:

void Toolbar_ItemChanged(MyToolbar self, MyToolbar.IndexArgs index)
{
  Debug.Assert(self == Toolbar);
    
  var tItem = ToolbarItem.FromItem(self.GetItemAtIndex(index.ItemIndex));
  ....
}

V3080 Possible null dereference of method return value when it is passed to method as its 1st argument.

Итак, срабатывание говорило о том, что в метод ToolbalItem.FromItem может передаваться null, что приведёт к выбрасыванию исключения. Так ли это?

В первую очередь стоит взглянуть на метод GetItemAtIndex:

public MyToolbarItem GetItemAtIndex(int index)
{
  if (!IsValidIndex(index)) 
    return null;

  return this[index];
}

Анализ потока данных позволил анализатору выяснить, что вызов этого метода в некоторых случаях возвращает null. Но приведёт ли это к проблемам? Давайте теперь перейдём к определению метода FromItem:

public static ToolbarItem FromItem(MyToolbarItem item)
{
  var tItem = new ToolbarItem();
  tItem.EntityID = 0;
  var terminalItem = item as MyToolbarItemTerminalBlock;
  if (terminalItem != null)
  {
    var block = item.GetObjectBuilder() as ....; // <=
    ....
  }
  ....
  return tItem;
}

Ранее мы видели, что в параметре item может быть записан null. Здесь же производится разыменование, а сам item перед этим не проверяется. Зато проверяется terminalItem! А если terminalItem не равен null, то и item точно не равен null.

Issue 2

Похожий пример мы обнаружили в проекте SharpDevelop:

DocumentScript GetScript(string fileName)
{
  ....
  var formattingOptions
       = CSharpFormattingPolicies.Instance
                                 .GetProjectOptions(compilation.GetProject());
  ....
}

V3080 Possible null dereference of 'compilation.GetProject()' method return value at 'project.FileName' when it is passed to method as its 1st argument.

Итак, анализатор предупреждал о возможном разыменовании нулевой ссылки внутри метода GetProjectOptions. Причиной этого является передача compilation.GetProject() в качестве первого аргумента. Давайте разбираться.

Межпроцедурный анализ позволил выяснить, что GetProject действительно иногда возвращает null. Что же насчёт GetProjectOptions? Давайте взглянем:

public CSharpFormattingPolicy GetProjectOptions(IProject project)
{
  if (!initialized)
    return GlobalOptions;

  var csproject = project as CSharpProject;
  if (csproject != null) {
    string key = project.FileName;            // <=
    ....
  }

  return SolutionOptions ?? GlobalOptions;
}

Да, здесь производится обращение к свойству первого аргумента. Однако лишь в том случае, если он не равен null! Просто проверяется не сам project, а результат его приведения через as.

Issue 3

Ещё одно пропавшее ложное срабатывание выдавалось на код проекта ILSpy:

protected override Expression DoResolve (ResolveContext ec)
{
  var res = expr.Resolve(ec);
  var constant = res as Constant;

  if (constant != null && constant.IsLiteral)
  {
    return Constant.CreateConstantFromValue(res.Type,           // <=
                                            constant.GetValue(),
                                            expr.Location);
  }

  return res;
}

V3080 Possible null dereference. Consider inspecting 'res'.

res получает своё значение из вызова expr.Resolve(ec). В некоторых случаях он действительно возвращает null. Но в момент обращения к свойству Type переменная уже точно не равна null. Как и ранее, проверка произведена неявно, и если constant != null, то и res тоже.

Были и многие другие ложные срабатывания, которые "ушли" благодаря поддержке связей через оператор as. Но все они так или иначе напоминают те, что мы здесь уже привели. Если у вас есть желание самостоятельно проверить, как PVS-Studio обрабатывает случаи такого вида, то вы можете бесплатно сделать это, загрузив анализатор по ссылке. Have fun!

Типичные связанные переменные

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

Ранее я уже приводил пример, демонстрирующий такую связь:

public void Test()
{
  var a = GetPotentialNull();
  bool flag = a != null;

  if (flag)
  {
    _ = a.GetHashCode(); // <=
  }
}

V3080 Possible null dereference. Consider inspecting 'a'.

Если flag = true, то переменная a не может быть равна null. Таким образом неявная проверка защищает от проблем.

Чтобы научить анализатор учитывать такие связи, мы вновь решили внести доработки в наш Data Flow. Однако данный случай был несколько сложнее.

В отличие от случая с оператором as, здесь понадобилось добавление нового типа информации о переменной. В частности – данных о связи с другой переменной. Обрабатывая объявление flag, анализатор вычисляет возможные значения переменных из выражения в случаях:

• если выражение (а следовательно, и flag) равно true;

• если выражение равно false.

Обработав объявление flag, анализатор добавляет в соответствующее виртуальное значение 2 правила:

• если flag == true, то a != null;

• если flag == false, то a == null.

Теперь, когда для flag имеются нужные данные, осталось лишь воспользоваться ими при обработке условия if (flag). Здесь Data Flow вычисляет возможные значения переменных в then-ветке. Соответственно, flag в ней всегда равна true, а связанная с этой переменной a точно не равна null.

Мы довольно долго страдали от таких ложных срабатываний, поэтому и приняли решение разобраться с ними. И, похоже, преуспели :). Теперь анализатор отслеживает связи такого вида и учитывает их в работе.

На синтетике всё это выглядит здорово, но давайте теперь посмотрим, что там в коде реальных проектов.

Типичные связи в реальном коде

Тут результат даже круче, чем со связями через оператор as. Что любопытно, правка позволила не только избавиться от ложных срабатываний, но и добавить несколько "истинных".

Issue 1

Для начала рассмотрим довольно простое ложное срабатывание на коде проекта BouncyCastle.

public static Stream ReplaceSigners(....)
{
  ....

  CmsTypedStream signedContent = parser.GetSignedContent();
  bool encapsulate = (signedContent != null);
  Stream contentOut = gen.Open(outStr,
                               parser.SignedContentType.Id,
                               encapsulate);
  if (encapsulate)
  {
    Streams.PipeAll(signedContent.ContentStream, contentOut); // <=
  }

  ....
}

V3080 Possible null dereference. Consider inspecting 'signedContent'.

Пропавшее ложное срабатывание говорило о возможном разыменовании нулевой ссылки. Если signedContent будет равно null, то обращение к свойству ContentStream приведёт к выбрасыванию исключения.

Но обратите внимание на проверку значения encapsulate. Именно она неявно защищает от разыменования нулевой ссылки, ведь encapsulate = true только тогда, когда signedContent != null. Последние правки "научили" PVS-Studio учитывать такие связи, поэтому ложное срабатывание и пропало.

Issue 2

Следующий пример взят из проекта ccnet:

public bool Authenticate(LoginRequest credentials)
{
  // Check that both the user name and the password match
  string userName = GetUserName(credentials);
  string password = NameValuePair.FindNamedValue(....);
  
  bool isValid =    !string.IsNullOrEmpty(userName)
                 && !string.IsNullOrEmpty(password);

  if (isValid)
  {
    isValid =    SecurityHelpers.IsWildCardMatch(userName,     // <=
                                                 this.userName)
              && ....;
  }

  return isValid;
}

V3080 Possible null dereference inside method at 'wildCard.Replace'. Consider inspecting the 1st argument: userName.

Это предупреждение говорило о том, что в метод IsWildCardMatch в качестве первого аргумента потенциально передаётся нулевая ссылка, а также о том, что внутри может произойти её разыменование. Соответственно, возможна ситуация, когда будет выброшено исключение типа NullReferenceException. Но так ли это на самом деле?

Значение первого аргумента – userName — приходит из вызова GetUserName. И оттуда действительно может приходить null, что и обнаружил анализатор. Да и в методе IsWildCardMatch действительно производится разыменование первого аргумента:

public static bool IsWildCardMatch(string wildCard, string value)
{
  Regex wildCardRegex = new Regex(wildCard.Replace("*",
                                                   "[a-zA-Z0-9_.@-]*"),
                                  RegexOptions.IgnoreCase);

  return wildCardRegex.IsMatch(value);
}

Вот только null туда не передаётся! Наверняка вы уже заметили проверку значения isValid здесь:

bool isValid =    !string.IsNullOrEmpty(userName)
               && !string.IsNullOrEmpty(password);

if (isValid)
{
  isValid =    SecurityHelpers.IsWildCardMatch(userName,
                                               this.userName)
            && ....;
}

Если isValid = true, то и userName никак не может быть null. Благодаря поддержке связей теперь и анализатор знает об этом.

Issue 3

Ещё одно интересное ложное срабатывание выдавалось на код из проекта FlashDevelop:

public void HandleEvent(Object sender, NotifyEvent e, HandlingPriority priority)
{
  ....
  features = enabledLanguages.ContainsKey(ext) ? enabledLanguages[ext] : null;
  
  if (completion == null)
    completion = new Completion(config, settingObject);

  completion.OnFileChanged(features);                      // <=

  if (features != null && features.Syntax != null)
    ....
  ....
}

V3080 Possible null dereference inside method at 'features.Mode'. Consider inspecting the 1st argument: features.

Срабатывание говорило о том, что переменная features, потенциально имеющая значение null, передаётся в метод OnFileChanged, что может привести к разыменованию нулевой ссылки.

По коду хорошо видно, что в features в некоторых случаях записывается null, а ниже присутствует и соответствующее условие. Однако перед передачей в метод OnFIleChanged переменная никак не проверяется – нет даже какой-нибудь неявной проверки через связь.

Так почему же поддержка связей привела к исчезновению этого срабатывания? Ответ находится в коде метода OnFileChanged:

internal void OnFileChanged(CssFeatures features)
{
  if (features == this.features) return;
  this.features = features;
  enabled = features != null;               // <=

  if (enabled)
  {
    wordChars = lang.characterclass.Characters;
    if (features.Mode != "CSS") wordChars += features.Trigger;
    InitBlockLevel();
  }
}

А вот и связанные переменные! Разыменование features производится только в случае, если enabled = true, а это возможно лишь в случае, когда features != null. Таким образом, срабатывание действительно было ложным.

Issue 4

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

Например, можно рассмотреть следующий фрагмент кода из Roslyn:

public override object GetFunctionExtender(string name,
                                           SyntaxNode node,
                                           ISymbol symbol)
{
  ....
  
  var methodSymbol = (IMethodSymbol)symbol;
  isDeclaration = methodSymbol.PartialDefinitionPart == null;
  hasOtherPart = isDeclaration
                    ? methodSymbol.PartialImplementationPart != null
                    : methodSymbol.PartialDefinitionPart != null;    // <=
    
  ....
}

V3022 Expression 'methodSymbol.PartialDefinitionPart != null' is always true.

Итак, научившись отслеживать связи соответствующего типа, PVS-Studio сформировал предупреждение о наличии в коде логического выражения, которое всегда возвращает true. Откуда взялось такое мнение?

Как и раньше, логика здесь проста. isDeclaration будет иметь значение true только в том случае, если methodSymbol.PartialDefinitionPart будет равным null. С другой стороны, если isDeclaration равен false, то и methodSymbol.PartialDefinitionPart точно не равно null.

Таким образом, последнее выражение тернарного оператора всегда будет иметь значение true. Иной раз always-true выражения представляют собой безобидный избыточный код, в других же случаях — свидетельствуют об ошибках. Иногда подобный код и правда пишут для улучшения читаемости. Какой именно случай здесь – сказать сложно.

Если ошибки тут нет, то код, в принципе, можно было бы написать чуть проще:

hasOtherPart =    !isDeclaration
               || methodSymbol.PartialImplementationPart != null;

С другой стороны, это лишь моё мнение и для кого-то проще будет именно тот код, который есть сейчас.

Заключение

Переменные могут быть связаны огромным количеством способов, поэтому и поддержать их все достаточно проблематично, если вообще возможно. Такие связи встречаются не так уж часто, но всё-таки время от времени они приводят к появлению ложных срабатываний. Разработчики PVS-Studio постоянно работают над улучшением анализатора, и поддержка связанных переменных – одно из направлений нашей деятельности. Конечно, наиболее важны для нас именно пожелания наших клиентов. Тем не менее, нам ценна любая обратная связь. Поэтому предлагаю и вам, дорогие читатели, бесплатно попробовать статический анализатор на своих проектах. Уверен, вы не будете разочарованы :).

А какие случаи связанности переменных встречали вы? Пишите в комментарии – посмотрим, сколько разных случаев мы сможем собрать.

До новых встреч!

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Nikita Lipilin. PVS-Studio evolution: data flow analysis for related variables.