Docotic.Pdf: Какие проблемы PVS-Studio обнаружит в зрелом проекте?
Качество для нас важно. И о PVS-Studio мы наслышаны. Все это привело к желанию проверить Docotic.Pdf и узнать, что еще можно улучшить.
Docotic.Pdf — библиотека общего назначения для работы с PDF файлами. Написана на C#, нет unsafe кода, нет внешних зависимостей кроме .NET runtime. Работает как под .NET 4+, так и под .NET Standard 2+.
Библиотека в разработке чуть больше 10 лет и в ней 110 тысяч строк кода без учета тестов, примеров и прочего. Для статического анализа мы постоянно используем Code Analysis и StyleCop. Несколько тысяч автоматических тестов охраняют нас от регрессий. Наши клиенты из разных стран и разных индустрий доверяют качеству библиотеки.
Какие проблемы обнаружит PVS-Studio?
Установка и первое впечатление
Скачал пробную версию с сайта PVS-Studio. Приятно удивил небольшой размер установщика. Установил с настройками по умолчанию: analysis engines, отдельная среда PVS-Studio, интеграция в Visual Studio 2017.
После установки ничего не запустилось, а в меню Пуск добавились два ярлыка с одинаковыми иконками: Standalone и PVS-Studio. На мгновение задумался, что же нужно запустить. Запустил Standalone и был неприятно удивлен интерфейсом. Масштаб 200%, выставленный для моей Windows, поддерживается криво. Часть текста слишком мелкая, часть текста не помещается в отведенное для него место. Название, Единорог и список Actions обрезаны при любом размере окна. Даже при полноэкранном.
Ну да ладно, решил открыть файл своего проекта. Внезапно, в меню Файл не нашел такой возможности. Там мне предложили только открывать отдельные файлы. Спасибо, подумал я, попробую-ка я лучше другой вариант. Запустил PVS-Studio — мне показали окно с размытым текстом. Масштаб 200% опять дал о себе знать. Текст сообщал: ищите меня в «Трех Коронах» ищите меню PVS-Studio в Visual Studio. Хорошо, открыл Студию.
Открыл solution. Действительно, есть меню PVS-Studio, а в нем есть возможность проверить «Current Project». Сделал нужный мне проект текущим и запустил проверку. Снизу в Студии всплыло окно с результатами анализа. В фоне появилось еще и окно с прогрессом проверки, но я его не сразу обнаружил. Сначала возникло ощущение, что проверка не началась или сразу кончилась.
Результат первой проверки
Анализатор проверил все 1253 файла проекта за примерно 9 минут и 30 секунд. К концу проверки счетчик файлов менялся не так быстро, как в начале. Возможно, есть некоторая нелинейная зависимость длительности проверки от числа проверяемых файлов.
В окне результатов появилась информация о 81 High, 109 Medium и 175 Low предупреждениях. Если посчитать частоту, то получается 0.06 High предупреждений / файл, 0.09 Medium предупреждений / файл, и 0.14 Low предупреждений / на файл. Или
0.74 High предупреждения на тысячу строк кода, 0.99 Medium предупреждений на тысячу строк кода, и 1.59 Low предупреждений на тысячу строк кода.
Вот в этой статье указано, что в CruiseControl.NET при его 256 тысячах строк кода анализатор нашел 15 High, 151 Medium и 32 Low предупреждений.
Получается, что в процентном соотношении для Docotic.Pdf было выдано больше предупреждений в каждой из групп.
Что же найдено?
Предупреждения Low я решил оставить без внимания на данном этапе.
Отсортировал предупреждения по колонке Code и получилось, что абсолютным рекордсменом по частоте стали V3022 «Expression is always true/false» и V3063 «A part of conditional expression is always true/false if it is evaluated». На мой взгляд они примерно об одном. В сумме эти два предупреждения дают 92 случая из 190. Относительная частота = 48%.
Логика деления на High и Medium не совсем ясна. Я ожидал V3072 «The 'A' class containing IDisposable members does not itself implement IDisposable» и V3073 «Not all IDisposable members are properly disposed. Call 'Dispose' when disposing 'A' class» в группе High, например. Но это вкусовщина, конечно.
Удивило, что V3095 «The object was used before it was verified against null. Check lines: N1, N2» помечена два раза как High и один раз как Medium. Баг?
Доверяй, но проверяй
Настало время проверить, насколько обоснованы полученные предупреждения. Найдены ли какие-нибудь реальные ошибки? Есть ли некорректные предупреждения?
Я разделил найденные предупреждения на группы, приведенные ниже.
Важные предупреждения
Их исправление повысило стабильность работы, решило проблемы с утечками памяти и т.п. Реальные ошибки/несовершенства.
Таких было выдано 16 штук, что составляет около 8% от всех предупреждений.
Приведу некоторые примеры.
V3019 «Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'color', 'indexed'»
public override bool IsCompatible(ColorImpl color)
{
IndexedColorImpl indexed = color as IndexedColorImpl;
if (color == null)
return false;
return indexed.ColorSpace.Equals(this);
}
Как можно видеть, вместо indexed, с null сравнивается переменная color. Это неправильно и может привести к NRE.
V3080 «Possible null dereference. Consider inspecting 'cstr_index.tile_index'»
Небольшой фрагмент для иллюстрации:
if (cstr_index.tile_index == null)
{
if (cstr_index.tile_index[0].tp_index == null)
{
// ..
}
}
Очевидно, что в первом условии подразумевалось != null. В текущем виде код при каждом вызове кинет NRE.
V3083 «Unsafe invocation of event 'OnProgress', NullReferenceException is possible. Consider assigning event to a local variable before invoking it.»
public void Updated()
{
if (OnProgress != null)
OnProgress(this, new EventArgs());
}
Предупреждение помогло исправить потенциальное исключение. Почему оно может возникнуть? На Stackoverflow есть хорошее объяснение.
V3106 «Possibly index is out of bounds. The '0' index is pointing beyond 'v' bound»
var result = new List();
for (int i = 0; i < text.Length; ++i)
{
var v = new List();
createPairs(text[i].ToString(CultureInfo.InvariantCulture));
result.Add(v[0]);
}
Ошибка в том, что результат createPairs игнорируется, а вместо этого идет обращение к пустому списку. Видимо, изначально createPairs принимал список в качестве параметра, но в процессе изменений метода была внесена ошибка.
V3117 «Constructor parameter 'validateType' is not used
Предупреждение было выдано для кода, похожего на такой
public SomeClass(IDocument document, bool validateType = true)
: base(document, true)
{
m_provider = document;
}
Само предупреждение не кажется важным. Но проблема серьезнее, чем кажется на первый взгляд. В процессе добавления optional параметра validateType вызов конструктора базового класса забыли исправить.
V3127 «Two similar code fragments were found. Perhaps, this is a typo and 'range' variable should be used instead of 'domain'»
private void fillTransferFunction(PdfStreamImpl function)
{
// ..
PdfArrayImpl domain = new PdfArrayImpl();
domain.AddReal(0);
domain.AddReal(1);
function.Add(Field.Domain, domain);
PdfArrayImpl range = new PdfArrayImpl();
range.AddReal(0);
range.AddReal(1);
function.Add(Field.Range, domain);
// ....
}
Возможно, предупреждение не будет выдано, если части кода будут чуть сильнее отличаться. Но в данном случае ошибка при использовании copy-paste была обнаружена.
Теоретические/формальные предупреждения
Они либо корректны, но их исправление не исправляет каких-либо конкретных ошибок и не влияет на читабельность кода. Либо они указывают на места, где могла быть ошибка, но ее нет. Например, порядок параметров намеренно изменен. Для таких предупреждений не требуется что-либо менять в программе.
Таких было выдано 57 штук, что составляет около 30% от всех предупреждений. Я приведу примеры для случаев, заслуживающих внимания.
V3013 «It is odd that the body of 'BeginText' function is fully equivalent to the body of 'EndText' function (166, line 171)»
public override void BeginText()
{
m_state.ResetTextParameters();
}
public override void EndText()
{
m_state.ResetTextParameters();
}
У обеих функций тела на самом деле одинаковые. Но это правильно. И так ли уж странно, если тела функций из одной строки совпадают?
V3106 «Possible negative index value. The value of 'c1' index could reach -1»
freq[256] = 1;
// ....
c1 = -1;
v = 1000000000L;
for (i = 0; i <= 256; i++)
{
if (freq[i] != 0 && freq[i] <= v)
{
v = freq[i];
c1 = i;
}
}
// ....
freq[c1] += freq[c2];
Согласен, кусочек не самого понятного алгоритма я привел. Но, по-моему, в данном случае анализатор зря переживает.
V3107 «Identical expression 'neighsum' to the left and to the right of compound assignment»
Предупреждение вызвано вполне банальным кодом:
neighsum += neighsum;
Да, его можно переписать через умножение. Но ошибки тут нет.
V3109 «The 'l_cblk.data_current_size' sub-expression is present on both sides of the operator. The expression is incorrect or it can be simplified.»
/* Check possible overflow on size */
if ((l_cblk.data_current_size + l_seg.newlen) < l_cblk.data_current_size)
{
// ...
}
Комментарий в коде поясняет замысел. Снова ложная тревога.
Обоснованные предупреждения
Их исправление положительно повлияло на читабельность кода. То есть сократило ненужные условия, проверки и т.п. Влияние на то, как код работает — неочевидно.
Таких было выдано 103 штуки, что составляет около 54% от всех предупреждений.
V3008 «The 'l_mct_deco_data' variable is assigned values twice successively. Perhaps this is a mistake»
if (m_nb_mct_records == m_nb_max_mct_records)
{
ResizeMctRecords();
l_mct_deco_data = (OPJ_INT32)m_nb_mct_records;
}
l_mct_deco_data = (OPJ_INT32)m_nb_mct_records;
Анализатор прав: присваивание внутри if не нужно.
V3009 «It is odd that this method always returns one and the same value»
private static bool opj_dwt_decode_tile(opj_tcd_tilecomp_t tilec, OPJ_UINT32 numres)
{
if (numres == 1U)
return true;
// ...
return true;
}
По совету анализатора метод был изменен и больше ничего не возвращает.
V3022 «Expression '! add' is always true»
private void addToFields(PdfDictionaryImpl controlDict, bool add)
{
// ...
if (add)
{
// ..
return;
}
if (!add)
{
// ...
}
// ...
}
Действительно, нет смысла во втором if. Условие всегда будет истинным.
V3029 «The conditional expression of the 'if' statements situated alongside each other are identical»
if (stroke)
extGState.OpacityStroke = opacity;
if (stroke)
state.AddReal(Field.CA, opacity);
Неясно, как такой код возник. Но теперь мы его исправили.
V3031 «An excessive check can be simplified. The '||' operator is surrounded by opposite expressions»
Вот это кошмарное условие:
if (!(cp.m_enc.m_tp_on != 0 &&
((!opj_codec_t.OPJ_IS_CINEMA(cp.rsiz) && (t2_mode == J2K_T2_MODE.FINAL_PASS)) || opj_codec_t.OPJ_IS_CINEMA(cp.rsiz))))
{
// ...
}
После изменений стало гораздо лучше
if (!(cp.m_enc.m_tp_on != 0 &&
(opj_codec_t.OPJ_IS_CINEMA(cp.rsiz) || t2_mode == J2K_T2_MODE.FINAL_PASS)))
{
// ...
}
V3063 «A part of conditional expression is always true if it is evaluated: x!= null»
V3022 «Expression 'x!= null' is always true»
Сюда я отнес предупреждения о том, что проверка на null не имеет смысла. Правильно ли так поступать — вопрос неоднозначный. Ниже я подробнее описал суть вопроса.
Необоснованные предупреждения
False positives. Из-за ошибок в реализации конкретной проверки или каком-то недостатке анализатора.
Таких было выдано 14 штук, что составляет около 7% от всех предупреждений.
V3081 «The 'i' counter is not used inside a nested loop. Consider inspecting usage of 'j' counter»
Немного упрощенный вариант кода, для которого было выдано это предупреждение:
for (uint i = 0; i < initialGlyphsCount - 1; ++i)
{
for (int j = 0; j < initialGlyphsCount - i - 1; ++j)
{
// ...
}
}
Очевидно, что i используется во вложенном цикле.
V3125 «The object was used after it was verified against null»
Код, для которого выдается такое предупреждение:
private static int Compare_SecondGreater(cmapEncodingRecord er1, cmapEncodingRecord er2)
{
if (er1 == er2)
return 0;
if (er1 != null && er2 == null)
return 1;
if (er1 == null && er2 != null)
return -1;
return er1.CompareTo(er2);
}
er1 не может быть равен null в момент вызова CompareTo ().
Другой код, для которого выдается такое предупреждение:
private static void realloc(ref int[][] table, int newSize)
{
int[][] newTable = new int[newSize][];
int existingSize = 0;
if (table != null)
existingSize = table.Length;
for (int i = 0; i < existingSize; i++)
newTable[i] = table[i];
for (int i = existingSize; i < newSize; i++)
newTable[i] = new int[4];
table = newTable;
}
table не может быть равно null в цикле.
V3134 «Shift by [32…255] bits is greater than the size of 'UInt32' type of expression '(uint)1'»
Кусочек кода, для которого выдается такое предупреждение:
byte bitPos = (byte)(numBits & 0x1F);
uint mask = (uint)1 << bitPos;
Видно, что bitPos может иметь значение из диапазона [0…31], но анализатор считает, что она может иметь значение из диапазона [0…255], что неверно.
Другие подобные случаи приводить не буду, так как они эквивалентны.
Дополнительные мысли по поводу некоторых проверок
Мне показалось нежелательным предупреждать, что 'x!= null' is always true в случаях, когда x — это результат вызова некоторого метода. Пример:
private X GetX(int y)
{
if (y > 0)
return new X(...);
if (y == 0)
return new X(...);
throw new ArgumentOutOfRangeException(nameof(x));
}
private Method()
{
// ...
X x = GetX(..);
if (x != null)
{
// ...
}
}
Да, формально анализатор прав: x всегда будет не равно null, потому что GetX либо вернет полноценный instance, либо кинет исключение. Но улучшит ли код удаление проверки на null? Что если GetX изменится позже? Обязан ли Method знать реализацию GetX?
Внутри команды мнения разделились. Высказывалось мнение, что у текущего метода есть контракт, по которому он не должен возвращать null. И нет смысла писать избыточный код «на будущее» при каждом вызове. А если контракт меняется — надо обновлять и вызывающий код.
В поддержку такого мнения приводилось такое суждение: проверять на null — это как оборачивать каждый вызов в try/catch на случай если метод в будущем начнет бросать исключения.
В итоге, согласно принципу YAGNI, решили не держаться за проверки и удалили их. Все предупреждения были перенесены из теоретических/формальных в обоснованные.
Буду рад прочитать ваше мнение в комментариях.
Выводы
Статический анализ — полезная штука. PVS-Studio позволяет найти реальные ошибки.
Да, есть и необоснованные / некорректные предупреждения. Но все-таки PVS-Studio нашел реальные ошибки в проекте, где уже используется Code Analysis. Наш продукт достаточно хорошо покрыт тестами, его так или иначе тестируют наши клиенты, но robots do it better статический анализ все-таки приносит пользу.
Напоследок немного статистики.
Топ-3 необоснованных предупреждений
V3081 «The 'X' counter is not used inside a nested loop. Consider inspecting usage of 'Y' counter»
1 из 1 признано необоснованным
V3125 «The object was used after it was verified against null. Check lines: N1, N2»
9 из 10 признаны необоснованными
V3134 «Shift by N bits is greater than the size of type»
4 из 5 признаны необоснованными
Топ-3 важных предупреждений
V3083 «Unsafe invocation of event, NullReferenceException is possible. Consider assigning event to a local variable before invoking it»
5 из 5 признаны важными
V3020 «An unconditional 'break/continue/return/goto' within a loop»
V3080 «Possible null dereference»
2 из 2 признаны важными
V3019 «It is possible that an incorrect variable is compared with null after type conversion using 'as' keyword»
V3127 «Two similar code fragments were found. Perhaps, this is a typo and 'X' variable should be used instead of 'Y'»
1 из 1 признано важным
