Разбираем best practice знакомства с PVS-Studio. Покажем быстрый старт работы с анализатором на примере проекта protobuf-net.
Первое знакомство со статическим анализатором может стать тяжёлой задачкой, особенно на крупных проектах. Отчёт анализатора может содержать сотни, а иногда и тысячи предупреждений. Просматривать все будет действительно трудно. Вот именно для таких ситуаций и создавался механизм Best Warnings. Он поможет в один клик увидеть самые лучшие предупреждения – те, которые вероятнее всего указывают на ошибку.
Действительно ли в один клик? Действительно! В окне PVS-Studio рядом с уровнями предупреждений есть кнопка 'Best'. Именно она и отвечает за показ лучших, по мнению анализатора, предупреждений.
Вот так выглядит окно PVS-Studio 7.22 в Visual Studio 2022:
При активации Best Warnings окно выглядит следующим образом:
С версии PVS-Studio 7.22 данный механизм реализован и в других IDE:
Показать возможности данного механизма легче всего на примере Open Source проекта. Для демонстрации Best Warnings был выбран protobuf-net. Если говорить простыми словами, то это сериализатор, который преобразует данные в формат Protocol Buffers. Для анализа был взят последний релиз версии 3.1.4.
Стоит сказать, что мы не будем рассматривать все предупреждения, которые были получены в результате фильтрации с помощью Best Warnings. Поглядим только на самые интересные и явно ошибочные места в коде. Вы же можете попробовать на своём проекте данный функционал в полном объёме. Потребуется всего лишь взять пробный ключ здесь.
Кстати, для всех рассмотренных фрагментов кода был создан issue на GitHub. Разработчик проекта ответил на него и подтвердил, что описанные ошибки нужно исправить.
Перейдём ближе к коду и ошибкам.
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.
Если честно, то подобное форматирование кода крайне неудачно и тяжело читаемо. Может, из-за этого в методе и оказалась ошибка.
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 не является заменой обычной работы с отчётом. Приятного использования :).