Любите оператор ‘?.’? А кто же не любит? Эти лаконичные проверки на 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.
ссылка на оригинал статьи https://habr.com/ru/company/pvs-studio/blog/560824/
Добавить комментарий