0832_foreach_ConditionalAccess_ru/image1.png
Любите оператор '?.'? А кто же не любит? Эти лаконичные проверки на null нравятся многим. Однако сегодня мы поговорим о случае, когда оператор '?.' только создаёт иллюзию безопасности. Речь пойдёт о его использовании в цикле foreach.


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


void ForeachTest(IEnumerable<String> collection)
{
  // #1
  foreach (var item in collection.NotNullItems())
    Console.WriteLine(item);

  // #2
  foreach (var item in collection?.NotNullItems())
    Console.WriteLine(item);
}

Как вы думаете, как отработает каждый из циклов, если collectionnull? Кажется, что вариант с использованием оператора ?. более безопасный. Но так ли это на самом деле? Название статьи уже должно было заложить в вашу душу сомнения.


В любом случае, со всем этим попробуем разобраться ниже. А к приведённой выше задачке мы ещё вернёмся в конце статьи, когда у нас будет больше информации.


Перед тем как начнём погружение, давайте немного определимся с терминологией. Посмотрим спецификацию C#, раздел "The foreach statement".


0832_foreach_ConditionalAccess_ru/image2.png


Выражение, по которому будет идти цикл, обозначено просто как "expression". Однако использовать прямой перевод слова — выражение — кажется не лучшей идеей, так как наверняка приведёт к путанице. Поэтому далее я буду использовать термин "перечислимое выражение", имея в виду "expression" из примера выше. Просто помните об этом.


Почему опасно использовать оператор '?.' в перечислимом выражении цикла foreach


Здесь стоит вспомнить, как работает оператор ?.


С ним закончим быстро.


var b = a?.Foo();

Итак:


  • если a == null, b == null;
  • если a != null, b == a.Foo().

Теперь посмотрим на цикл foreach. Не будем забывать, что это всего лишь горка синтаксического сахара.


void Foo1(IEnumerable<String> collection)
{
  foreach (var item in collection)
    Console.WriteLine(item);
}

Если посмотреть IL код, то приведённый выше фрагмент можно переписать на C#, не используя foreach. Выглядеть он будет примерно следующим образом:


void Foo2(IEnumerable<String> collection)
{
  var enumerator = collection.GetEnumerator();
  try
  {
    while (enumerator.MoveNext())
    {
      var item = enumerator.Current;
      Console.WriteLine(item);
    }
  }
  finally
  {
    if (enumerator != null)
    {
      enumerator.Dispose();
    }
  }
}

Примечание. В некоторых случаях foreach может раскрываться в другой код. Например, в код, идентичный тому, что генерируется для цикла for. Однако проблема всё также остаётся актуальной. Я думаю, возможные оптимизации цикла foreach мы более подробно рассмотрим в отдельной статье.


Ключевой момент здесь для нас — выражение collection.GetEnumerator(). Чёрным по белому (хотя зависит от вашей цветовой схемы) в коде написано, что происходит разыменование ссылки при вызове метода GetEnumerator. Если эта ссылка будет нулевой, получим исключение типа NullReferenceException.


Теперь рассмотрим, что же происходит при использовании оператора ?. в перечислимом выражении foreach:


static void Foo3(Wrapper wrapper)
{
  foreach (var item in wrapper?.Strings)
    Console.WriteLine(item);
}

Этот код можно переписать примерно следующим образом:


static void Foo4(Wrapper wrapper)
{
  IEnumerable<String> strings;
  if (wrapper == null)
  {
    strings = null;
  }
  else
  {
    strings = wrapper.Strings;
  }

  var enumerator = strings.GetEnumerator();
  try
  {
    while (enumerator.MoveNext())
    {
      var item = enumerator.Current;
      Console.WriteLine(item);
    }
  }
  finally
  {
    if (enumerator != null)
    {
      enumerator.Dispose();
    }
  }
}

Здесь, как и в прошлом случае, вызывается метод GetEnumerator (strings.GetEnumerator). Однако обратите внимание на то, что значение strings может иметь значение null, если wrappernull. В принципе, это ожидаемо, если вспомнить, как работает оператор ?. (мы обсуждали это выше). В таком случае при попытке вызова метода string.GetEnumerator() получим исключение типа NullReferenceException.


Именно поэтому использование оператора ?. в перечислимом выражении foreach не защищает от разыменования нулевых ссылок, а только создаёт иллюзию безопасности.


Как пришла идея доработки анализатора?


Приходит ко мне коллега и говорит — вот код, ошибку не ловим. Я в удивлении. Точно помню, как я сам предлагал на доработку случай, когда в перечислимом выражении foreach может быть значение null. Проверяю. Действительно, на представленном ниже коде анализатор не срабатывает.


void Test1(IEnumerable<String> collection, 
          Func<String, bool> predicate)
{
  foreach (var item in collection?.Where(predicate))
    Console.WriteLine(item);
}

И на таком — тоже.


void Test2(IEnumerable<String> collection, 
          Func<String, bool> predicate)
{
  var query = collection?.Where(predicate);
  foreach (var item in query)
    Console.WriteLine(item);
}

А вот на таком фрагменте кода предупреждение уже есть.


void Test3(IEnumerable<String> collection, 
          Func<String, bool> predicate,
          bool flag)
{
  var query = collection != null ? collection.Where(predicate) : null;
  foreach (var item in query)
    Console.WriteLine(item);
}

Предупреждение PVS-Studio: V3080 Possible null dereference. Consider inspecting 'query'.


На следующий код предупреждение также будет выдано.


IEnumerable<String> GetPotentialNull(IEnumerable<String> collection,
                                     Func<String, bool> predicate,
                                     bool flag)
{
  return collection != null ? collection.Where(predicate) : null;
}

void Test4(IEnumerable<String> collection, 
          Func<String, bool> predicate,
          bool flag)
{
  foreach (var item in GetPotentialNull(collection, predicate, flag))
    Console.WriteLine(item);
}

Предупреждение PVS-Studio: V3080 Possible null dereference of method return value. Consider inspecting: GetPotentialNull(...).


Почему же в методах Test3 и Test4 предупреждение выдаётся, а в Test1 и Test2 — нет? Дело в том, что анализатор разделяет эти ситуации:


  • анализатор не выдавал предупреждение, если в переменную записывался результат работы оператора ?.;
  • если же использовалось выражение, в которое где-то явно записывалось значение null, предупреждение выдавалось.

Сделано это разделение для того, чтобы анализатор смог более точно обработать каждую ситуацию:


  • выдаётся более точное предупреждение;
  • возможность отдельно обрабатывать эти случаи (повышать / понижать уровень, подавлять / не подавлять и т.п.);
  • отдельная документация, более точно объясняющая суть проблемы.

Какие диагностики были улучшены?


По результатам были доработаны 2 диагностических правила: V3105 и V3153.


Примечание. Документация по этим диагностикам будет обновлена после релиза PVS-Studio 7.13, в который войдут соответствующие правки.


V3105 теперь обнаруживает подозрительные фрагменты кода, когда в переменную записывается результат работы оператора ?., а далее эта переменная используется в качестве перечислимого выражения foreach.


void Test(IEnumerable<String> collection, 
          Func<String, bool> predicate)
{
  var query = collection?.Where(predicate);
  foreach (var item in query)
    Console.WriteLine(item);
}

Предупреждение PVS-Studio: V3105 The 'query' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible.


V3153 теперь обнаруживает случаи, когда оператор ?. используется прямо в перечислимом выражении foreach.


void Test(IEnumerable<String> collection, 
          Func<String, bool> predicate)
{
  foreach (var item in collection?.Where(predicate))
    Console.WriteLine(item);
}

Предупреждение PVS-Studio: V3153 Enumerating the result of null-conditional access operator can lead to NullReferenceException. Consider inspecting: collection?.Where(predicate).


Примеры обнаруженных проблем


Самое приятное в доработках анализатора — когда видишь профит от них. Как мы неоднократно говорили, мы тестируем анализатор, в том числе и на базе open source проектов. И после правок V3105 и V3153 удалось найти несколько новых срабатываний!


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


RavenDB


private void HandleInternalReplication(DatabaseRecord newRecord, 
                                       List<IDisposable> instancesToDispose)
{
  var newInternalDestinations =
        newRecord.Topology?.GetDestinations(_server.NodeTag,
                                            Database.Name,
                                            newRecord.DeletionInProgress,
                                            _clusterTopology,
                                            _server.Engine.CurrentState);
  var internalConnections 
        = DatabaseTopology.FindChanges(_internalDestinations, 
                                       newInternalDestinations);

  if (internalConnections.RemovedDestiantions.Count > 0)
  {
    var removed = internalConnections.RemovedDestiantions
                                     .Select(r => new InternalReplication
      {
        NodeTag = _clusterTopology.TryGetNodeTagByUrl(r).NodeTag,
        Url = r,
        Database = Database.Name
      });

    DropOutgoingConnections(removed, instancesToDispose);
  }
  if (internalConnections.AddedDestinations.Count > 0)
  {
    var added = internalConnections.AddedDestinations
                                   .Select(r => new InternalReplication
    {
      NodeTag = _clusterTopology.TryGetNodeTagByUrl(r).NodeTag,
      Url = r,
      Database = Database.Name
    });
    StartOutgoingConnections(added.ToList());
  }
  _internalDestinations.Clear();
  foreach (var item in newInternalDestinations)
  {
    _internalDestinations.Add(item);
  }
}

Специально привёл весь фрагмент кода целиком. Согласитесь, проблема не то чтобы очень заметна. Конечно, если знать, что ищешь, то дело идёт проще. ;)


Если код упростить, проблема становится более очевидной.


private void HandleInternalReplication(DatabaseRecord newRecord, 
                                       List<IDisposable> instancesToDispose)
{
  var newInternalDestinations = newRecord.Topology?.GetDestinations(....);
  ....
  foreach (var item in newInternalDestinations)
    ....
}

В переменную newInternalDestinations записали результат работы оператора ?.. Если newRecord.Topologynull, newInternalDestinations также будет иметь значение null. При попытке выполнения оператора foreach будет выброшено исключение типа NullReferenceException.


Предупреждение PVS-Studio: V3105 The 'newInternalDestinations' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. ReplicationLoader.cs 828


Что ещё интересно, эта же переменная — newInternalDestinations — передаётся в метод DatabaseTopology.FindChanges, где она проверяется на null (параметр newDestinations):


internal static 
(HashSet<string> AddedDestinations, HashSet<string> RemovedDestiantions)
FindChanges(IEnumerable<ReplicationNode> oldDestinations, 
            List<ReplicationNode> newDestinations)
{
  ....
  if (newDestinations != null)
  {
    newList.AddRange(newDestinations.Select(s => s.Url));
  }
  ....
}

MSBuild


public void LogTelemetry(string eventName, 
                         IDictionary<string, string> properties)
{
  string message 
           = $"Received telemetry event '{eventName}'{Environment.NewLine}";

  foreach (string key in properties?.Keys)
  {
    message += $"  Property '{key}' = '{properties[key]}'{Environment.NewLine}";
  }
  ....
}

Предупреждение PVS-Studio: V3153 Enumerating the result of null-conditional access operator can lead to NullReferenceException. Consider inspecting: properties?.Keys. MockEngine.cs 159


Здесь оператор ?. используется напрямую в перечислимом выражении foreach. Возможно, разработчик думал, что так будет безопаснее. Но мы-то с вами знаем, что безопаснее не будет. ;)


Nethermind


Пример похож на предыдущий.


public NLogLogger(....)
{
  ....

  foreach (FileTarget target in global::NLog.LogManager
                                            .Configuration
                                           ?.AllTargets
                                            .OfType<FileTarget>())
  {
    ....
  }
  ....
}

Предупреждение PVS-Studio: V3153 Enumerating the result of null-conditional access operator can lead to NullReferenceException. NLogLogger.cs 50


Разработчики также решили "обезопаситься" от NullReferenceException и использовали оператор ?. прямо в перечислимом выражении foreach. Возможно, им повезёт и свойство Configuration никогда не вернёт null. Иначе настанет час, когда этот код "выстрелит".


Roslyn


private ImmutableArray<char>
GetExcludedCommitCharacters(ImmutableArray<RoslynCompletionItem> roslynItems)
{
  var hashSet = new HashSet<char>();
  foreach (var roslynItem in roslynItems)
  {
    foreach (var rule in roslynItem.Rules?.FilterCharacterRules)
    {
      if (rule.Kind == CharacterSetModificationKind.Add)
      {
        foreach (var c in rule.Characters)
        {
          hashSet.Add(c);
        }
      }
    }
  }

  return hashSet.ToImmutableArray();
}

Предупреждение PVS-Studio: V3153 Enumerating the result of null-conditional access operator can lead to NullReferenceException. CompletionSource.cs 482


Ну здорово же, а? Люблю, когда PVS-Studio находит интересные места в компиляторах или других анализаторах.


PVS-Studio


А теперь самое время сказать, что вообще-то мы и сами не без греха, и тоже прошлись по этим же граблям. :)


0832_foreach_ConditionalAccess_ru/image3.png


Мы регулярно проверяем PVS-Studio с помощью PVS-Studio. Алгоритм работы примерно такой:


  • ночью на сборочном сервере собирается новая версия дистрибутива анализатора. В неё попадают правки, которые были заложены в основную ветку в течение дня;
  • этой новой версией проверяется код различных проектов, в том числе — самого PVS-Studio;
  • информация о выданных анализатором предупреждениях рассылается разработчикам и руководителям с помощью утилиты BlameNotifier;
  • найденные предупреждения исправляются.

И вот, после того как были доработаны V3153 и V3105, мы обнаружили, что и на нашем коде появилось несколько предупреждений. Действительно, нашлись и прямые использования оператора ?. в перечислимом выражении foreach, и косвенные (с записью в переменную). Повезло нам, что код в этих местах не падал. В любом случае, предупреждения уже учтены, а соответствующие места исправлены. ;)


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


public override void
VisitAnonymousObjectCreationExpression(
  AnonymousObjectCreationExpressionSyntax node)
{
  foreach (var initializer in node?.Initializers)
    initializer?.Expression?.Accept(this);
}

Да, операторов ?. здесь от души — найдите тот, который прострелит ногу. Казалось бы, максимум безопасности (прочитать голосом озвучки нанокостюма из Crysis), а на деле — нет.


Можно ли использовать оператор '?.' в перечислимом выражении цикла foreach безопасно?


Конечно, можно. И мы встречали такие примеры кода. Например, на помощь может прийти оператор ??.


Следующий код опасен и грозит потенциальным исключением типа NullReferenceException:


static void Test(IEnumerable<String> collection,
                 Func<String, bool> predicate)
{
  foreach (var item in collection?.Where(predicate))
    Console.WriteLine(item);
}

А на таком варианте кода исключение уже выброшено не будет:


static void Test(IEnumerable<String> collection,
                 Func<String, bool> predicate)
{
  foreach (var item in    collection?.Where(predicate) 
                       ?? Enumerable.Empty<String>())
  {
    Console.WriteLine(item);
  }
}

Если оператор ?. в результате своей работы вернёт значение null, то результатом работы оператора ?? будет выражение Enumerable.Empty<String>() — следовательно, исключение выброшено не будет. Но, конечно, стоит подумать, не проще ли будет добавить явную проверку на null.


static void Test(IEnumerable<String> collection,
                 Func<String, bool> predicate)
{
  if (collection != null)
  {
    foreach (var item in collection.Where(predicate))
      Console.WriteLine(item);
  }
}

Выглядит не так модно, конечно, но, может, даже более очевидно и понятно.


Разбираем задачку из начала статьи


Напоминаю, что мы анализировали следующий код.


void ForeachTest(IEnumerable<String> collection)
{
  // #1
  foreach (var item in collection.NotNullItems())
    Console.WriteLine(item);

  // #2
  foreach (var item in collection?.NotNullItems())
    Console.WriteLine(item);
}

Теперь вы знаете, что вариант #2 совсем не безопасный и не защищает от NullReferenceException. А что же с вариантом #1? На первый взгляд, кажется, что здесь также будет выброшено исключение NullReferenceException — при вызове collection.NotNullItems(). А вот и не факт! Предположим, что NotNullItems — метод расширения, имеющий следующее тело:


public static IEnumerable<T>
NotNullItems<T>(this IEnumerable<T> collection) where T : class
{
  if (collection == null)
    return Enumerable.Empty<T>();

  return collection.Where(item => item != null);
}

Как мы видим, в метод зашита проверка на случай, когда collection имеет значение null. Так как в этом случае возвращается значение Enumerable.Empty<T>(), при попытке его обхода в цикле foreach никакого исключения выброшено не будет. То есть цикл #1 отработает успешно, даже если collectionnull.


А вот второй цикл как был опасным, так и остался. Если collectionnull, метод NotNullItems вызван не будет. Следовательно, не сработает и заложенная в него защита от случая, когда collectionnull. В итоге, получаем всё ту же ситуацию, которую мы неоднократно рассматривали: попытка вызова метода GetEnumerator() через нулевую ссылку.


Такой вот интересный момент выходит, когда явный вызов метода collection.NotNullItems() защищает от исключения NullReferenceException, а 'безопасный вызов' — collection?.NotNullItems() — не защищает.


Заключение


Выводов здесь несколько:


  • не используйте оператор ?. в перечислимом выражении foreach прямо или косвенно — это только иллюзия безопасности;
  • используйте статический анализатор регулярно.

Для себя же мы ещё раз отметили, что полезно не только разрабатывать новые диагностики, но и дорабатывать текущие.


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


И, как обычно, приглашаю подписываться на мой Twitter-аккаунт.


Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Sergey Vasiliev. The '?.' Operator in foreach Will Not Protect From NullReferenceException.