PVS-Studio и protobuf-net: лучшие предупреждения в один клик

21 Дек 2022

Автор: Артём Ровенский

О чём речь?
Демонстрация работы
Итоги

Разбираем best practice знакомства с PVS-Studio. Покажем быстрый старт работы с анализатором на примере проекта protobuf-net.

1019_BestWarnings_Protobuf_net_ru/image1.png

О чём речь?

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

Действительно ли в один клик? Действительно! В окне PVS-Studio рядом с уровнями предупреждений есть кнопка 'Best'. Именно она и отвечает за показ лучших, по мнению анализатора, предупреждений.

Вот так выглядит окно PVS-Studio 7.22 в Visual Studio 2022:

1019_BestWarnings_Protobuf_net_ru/image2.png

При активации Best Warnings окно выглядит следующим образом:

1019_BestWarnings_Protobuf_net_ru/image3.png

С версии PVS-Studio 7.22 данный механизм реализован и в других IDE:

Rider;
CLion;
IntelliJ IDEA.

Демонстрация работы

Показать возможности данного механизма легче всего на примере Open Source проекта. Для демонстрации Best Warnings был выбран protobuf-net. Если говорить простыми словами, то это сериализатор, который преобразует данные в формат Protocol Buffers. Для анализа был взят последний релиз версии 3.1.4.

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

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

Перейдём ближе к коду и ошибкам.

1019_BestWarnings_Protobuf_net_ru/image4.png

Нужно больше проверок

public virtual string GetName(FileDescriptorProto definition)
{
  var ns = definition?.Options?.GetOptions()?.Namespace;
  if (!string.IsNullOrWhiteSpace(ns)) return ns;
  ns = definition.Options?.CsharpNamespace;
  if (string.IsNullOrWhiteSpace(ns)) ns = GetName(definition.Package); // <=

  if (string.IsNullOrEmpty(ns)) ns = definition?.DefaultPackage;       // <=

  return string.IsNullOrWhiteSpace(ns) ? null : ns;
}

V3095 The 'definition' object was used before it was verified against null. Check lines: 139, 141. NameNormalizer.cs 139

В данном случае параметр definition сначала разыменовывается без проверки на null, а потом на null всё-таки проверяется. Стоит обратить внимание, что ошибка с отсутствием проверки даст о себе знать ещё раньше.

Давайте разбираться.

Если значение definition окажется равно null, то в переменную ns будет записан null. После этого следует проверка ns на null с помощью метода IsNullOrWhiteSpace. Если ns всё-таки равен null, то выхода из метода не произойдёт и уже на следующей строчке будет сгенерировано исключение NullReferenceException.

Если честно, то подобное форматирование кода крайне неудачно и тяжело читаемо. Может, из-за этого в методе и оказалась ошибка.

null? Используй!

internal static MethodInfo GetGetMethod(....)
{
  if (property is null) return null;
  MethodInfo method = property.GetGetMethod(nonPublic);
  if (method is null && !nonPublic && allowInternal)
  {
    method = property.GetGetMethod(true);
    if (method is null && !(   method.IsAssembly           // <=
                            || method.IsFamilyOrAssembly)) 
    {
      method = null;
    }
  }
  return method;
}

V3080 Possible null dereference. Consider inspecting 'method'. Helpers.cs 74

В данном случае допущена ошибка в логике условного выражения. Получается, что разработчик проверяет значение локальной переменной method на равенство null, после чего производит обращение к нему. В итоге это может привести к исключению NullReferenceException.

Взглянем на название рассматриваемого метода. В нём можно заметить слово Get. Логично предположить, что должен быть и Set. Метод GetSetMethod действительно существует. Причём в нём есть такая же ошибка. Кстати, в Best Warnings есть предупреждение и о ней. Эх, Copy-Paste.

Преждевременное использование

public EnumMemberSerializer(Type enumType)
{
  if (!enumType.IsEnum)                                                   // <=
    ThrowHelper.ThrowInvalidOperationException("...." +
                                               enumType.NormalizeName()); 
  ExpectedType = enumType ??                                              // <= 
                 throw new ArgumentNullException(nameof(enumType)); 
  _tail = Type.GetTypeCode(enumType) switch
  {
    ....
  };
  if (_tail is null)
    ThrowHelper.ThrowInvalidOperationException("...." + 
                                               enumType.NormalizeName());
}

V3095 The 'enumType' object was used before it was verified against null. Check lines: 13, 14. EnumMemberSerializer.cs 13

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

Итоги

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

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

#CSharp