Любите оператор '?.' ? А кто же не любит? Эти лаконичные проверки на 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" из примера выше. Просто помните об этом.
Здесь стоит вспомнить, как работает оператор ?.
С ним закончим быстро.
var 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 — нет? Дело в том, что анализатор разделяет эти ситуации:
Сделано это разделение для того, чтобы анализатор смог более точно обработать каждую ситуацию:
По результатам были доработаны 2 диагностических правила: V3105 и V3153.
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 удалось найти несколько новых срабатываний!
Примечание. Приведённые фрагменты кода относятся к кодовой базе, актуальной на момент добавления проектов в наши тесты. То есть в актуальной кодовой базе этих фрагментов кода может не быть, или они могут находиться на других строках.
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));
}
....
}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. Возможно, разработчик думал, что так будет безопаснее. Но мы-то с вами знаем, что безопаснее не будет. ;)
Пример похож на предыдущий.
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. Иначе настанет час, когда этот код "выстрелит".
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. Алгоритм работы примерно такой:
И вот, после того как были доработаны V3153 и V3105, мы обнаружили, что и на нашем коде появилось несколько предупреждений. Действительно, нашлись и прямые использования оператора ?. в перечислимом выражении foreach, и косвенные (с записью в переменную). Повезло нам, что код в этих местах не падал. В любом случае, предупреждения уже учтены, а соответствующие места исправлены. ;)
Пример кода, на который было выдано предупреждение:
public override void
VisitAnonymousObjectCreationExpression(
AnonymousObjectCreationExpressionSyntax node)
{
foreach (var initializer in node?.Initializers)
initializer?.Expression?.Accept(this);
}Да, операторов ?. здесь от души — найдите тот, который прострелит ногу. Казалось бы, максимум безопасности (прочитать голосом озвучки нанокостюма из Crysis), а на деле — нет.
Конечно, можно. И мы встречали такие примеры кода. Например, на помощь может прийти оператор ??.
Следующий код опасен и грозит потенциальным исключением типа 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() — не защищает.
Выводов здесь несколько:
Для себя же мы ещё раз отметили, что полезно не только разрабатывать новые диагностики, но и дорабатывать текущие.
Обсуждаемые правки вошли в релиз PVS-Studio 7.13. Интересно посмотреть, не использует ли кто-нибудь оператор ?. в перечислимых выражениях на вашей кодовой базе? Тогда приглашаю загрузить дистрибутив анализатора с сайта и проверить интересующий код.
И, как обычно, приглашаю подписываться на мой Twitter-аккаунт.