Любите оператор '?.'? А кто же не любит? Эти лаконичные проверки на 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);
}

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

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

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

Выражение, по которому будет идти цикл, обозначено просто как "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, если wrapper — null. В принципе, это ожидаемо, если вспомнить, как работает оператор ?. (мы обсуждали это выше). В таком случае при попытке вызова метода 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.Topology — null, 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

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

Мы регулярно проверяем 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 отработает успешно, даже если collection — null.

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

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

Заключение

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

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

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

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

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

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