Крупные проекты проверять интересно. Как правило, в них удаётся найти различные интересные ошибки и рассказать о них людям. Также это хороший способ тестирования нашего анализатора и улучшения различных его аспектов. Я давно хотел проверить 'Mono', и наконец-то такая возможность появилась. И, стоит сказать, проверка себя оправдала, так как удалось найти много занимательного. О том, что же нашлось, какие нюансы возникли при проверке, и будет эта статья.
О проекте
Mono — проект по созданию полноценного воплощения системы .NET Framework на базе свободного программного обеспечения. Основной разработчик проекта Mono — корпорация Xamarin, ранее Novell.
Mono включает компилятор языка C#, среду исполнения .NET — mono (с поддержкой JIT) и mint (без поддержки JIT), отладчик, а также ряд библиотек, включая реализацию WinForms, ADO.NET и ASP.NET, а также компиляторы smcs (для создания приложений для Moonlight) и vbc (для приложений, написанных на VB.NET).
В рамках проекта также разрабатываются привязки для графической библиотеки GTK+ на платформу .NET.
Исходный код доступен для загрузки в репозитории на GitHub. Количество строк кода при проверке загруженного с GitHub репозитория составило около 6.3 миллионов (исключая пустые строки). Такой объём кодовой базы выглядит крайне привлекательно — наверняка где-то в коде закрались ошибки. С другой стороны, проверка такого большого проекта будет полезна и анализатору, так как послужит ему хорошим стресс-тестом.
Инструмент анализа, особенности проверки
В качестве инструмента анализа выступал статический анализатор кода PVS-Studio. На данный момент C#-анализатор включает в себя свыше 100 диагностических правил, каждое из которых сопровождается документацией, содержащей информацию об ошибке, возможных последствиях и способах исправления. С момента релиза удалось проверить много различных проектов, написанных на C#, таких как Roslyn, Xamarin.Forms, Space Engineers, CoreFX, Code Contracts и пр. (с полным списком можно ознакомиться по ссылке).
Сам анализатор доступен для загрузки по ссылке. Пробной версии должно быть достаточно для первого знакомства с анализатором. Если инструмент вас заинтересовал, напишите нам, и мы предоставим ключ для более плотного знакомства и поможем с его настройкой.
Также хочу отметить, что в статье не приводились ошибки из файлов, содержащих в начале файла какое-то упоминание корпорации Microsoft. Это сделано в основном для того, чтобы избежать возможных пересечений с ошибками, упоминавшимися в других статьях. В любом случае, материала и так набралось достаточно.
Как и всегда, в статье приведены не все ошибки, так как это сильно бы увеличило её объем. Я постарался выбрать наиболее интересные места, но многие осталась всё же за рамками статьи. Не подумайте, что я хочу упрекнуть в чем-то авторов 'Mono'. Количество ошибок обусловлено большим размером проекта, и это понятно. Тем не менее, хорошо бы исправить имеющиеся ошибки и не допускать появления новых. Помочь с этим поможет внедрение статического анализа в процесс разработки. Подробнее об этом написано ниже в соответствующем разделе.
Пара слов о том, почему проверка проектов — занятие нетривиальное
В идеальном мире проверка проектов и написание статьи осуществляется по следующему сценарию: нашёл проект -> собрал -> проверил анализатором -> нашёл достаточное количество ошибок -> написал статью. Все рады, все довольны: мы поставили себе галочку о проверенном проекте, люди прочитали новую статью, разработчики узнали об ошибках в коде, автор — молодец.
Увы, наш мир не идеален. На тех или иных этапах часто возникают проблемы, в этот раз — со сборкой. Если есть подробная инструкция по сборке, или же она легко проводится своими силами — это замечательно! Тогда можно смело приступать к проверке проекта и написанию статьи. Иначе начинается головная боль. Так вышло и с 'Mono'. Решение net_4_x.sln, объединяющее в себе C#-проекты, не собирается 'из коробки' (т.е. сразу после загрузки из репозитория). Один из скриптов сборки оказался нерабочим (неправильно был прописан путь (возможно из-за того, что иерархия директорий со временем менялась)), но его исправление также не принесло плодов.
Конечно, отступать не хотелось, поэтому я экспериментировал со сборкой даже во внерабочее время, но результата оказалось немного. В итоге, провозившись с этим порядочное количество времени, было принято решение написать статью «как есть».
Периодически в статьях я повторяю, что для полноценной проверки проект должен быть скомпилированным — со всеми зависимостями, без ошибок и п.р. Как правило, я стараюсь этого добиваться, но есть исключения из правил, как в данном случае.
Конечно, проверять несобранный проект нехорошо по нескольким причинам:
- анализ проекта получается не таким качественным, как мог бы. Это факт. В чём именно снижается качество — зависит от реализации диагностического правила. Возможно, появится ложное срабатывание, или же наоборот, полезное срабатывание не будет выдано;
- при просмотре лога нужно быть на порядок внимательнее, так как возникает (пусть и небольшая) вероятность появления ложных срабатываний, которых не было бы при корректной сборке проекта;
- так как некоторые полезные срабатывания пропадают, возможен пропуск интересных ошибок, которые могли бы попасть в статью и о которых узнали бы разработчики (однако они могут узнать об этих ошибках, самостоятельно проверив проект);
- приходится писать разделы «Пара слов о том, почему проверка проектов...».
Тем не менее, нашлось много подозрительных мест, некоторые из которых и будут рассмотрены ниже.
Результаты проверки
В последнее время в статьях мы стараемся приводить статистику по проверенному проекту: общее количество предупреждений, количество ложных срабатываний и реальных ошибок.
К сожалению, в этот раз я не могу привести такую статистику. Во-первых, кода, как и предупреждений, очень много. Если общее количество анализируемых предупреждений составляет несколько десятков, их можно просмотреть и дать приблизительную оценку. Когда счет предупреждениям идёт на сотни, задача анализа становится нетривиальной.
Во-вторых, на полностью собранном проекте эта статистика может поменяться в любую из сторон: уменьшения или увеличения количества предупреждений. В собранном проекте анализатору доступно больше семантической информации, следовательно, он может провести более глубокий анализ (исчезнут ложные срабатывания, появятся новые предупреждения). Для интересующихся, как семантическая информация влияет на анализ и по каким принципам всё это работает, предлагаю ознакомиться со статьёй "Введение в Roslyn. Использование для разработки инструментов статического анализа".
Но вам наверняка не терпится посмотреть, что же интересного удалось найти в коде проекта? Что ж, давайте рассмотрим некоторые фрагменты кода.
Одинаковые подвыражения в пределах одного выражения
Одна из самых распространённых ошибок. Причин для возникновения — множество. К ним можно отнести copy-paste, схожие названия переменных, злоупотребление IntelliSense, банальную невнимательность. Отвлеклись на минутку — сделали ошибку.
public int ExactInference (TypeSpec u, TypeSpec v)
{
....
var ac_u = (ArrayContainer) u;
var ac_v = (ArrayContainer) v;
....
var ga_u = u.TypeArguments;
var ga_v = v.TypeArguments;
....
if (u.TypeArguments.Length != u.TypeArguments.Length) // <=
return 0;
....
}Предупреждение PVS-Studio: V3001 There are identical sub-expressions 'u.TypeArguments.Length' to the left and to the right of the '!=' operator. generic.cs 3135
Теперь, когда код метода упрощён донельзя, заметить ошибку в условии оператора if не составит труда — в качество экземпляра типа TypeSpec во втором подвыражении должен был выступать параметр v, а не u. Возможно, ошибка была допущена из-за того, что внешне символы u и v схожи, и, не акцентируя внимания на этом выражении, можно легко не заметить подвоха.
Остальной код был приведён для того, чтобы указать, что эти параметры в основном используются вместе, на основании чего можно сделать вывод об ошибке и о корректном варианте кода:
if (u.TypeArguments.Length != v.TypeArguments.Length)
return 0;Не менее интересный случай.
bool BetterFunction (....)
{
....
int j = 0;
....
if (!candidate_params &&
!candidate_pd.FixedParameters [j - j].HasDefaultValue) { // <=
return true;
}
....
if (!candidate_pd.FixedParameters [j - 1].HasDefaultValue &&
best_pd.FixedParameters [j - 1].HasDefaultValue)
return true;
if (candidate_pd.FixedParameters [j - 1].HasDefaultValue &&
best_pd.HasParams)
return true;
....
}Предупреждение PVS-Studio: V3001 There are identical sub-expressions 'j' to the left and to the right of the '-' operator. ecore.cs 4832
В одном из выражений вычисления индекса программист ошибся, написав выражение j — j. Таким образом, всегда будет выполняться доступ к первому элементу массива. Если бы это и требовалось, логичнее было использовать целочисленный литерал, равный 0. То, что это действительно ошибка, подтверждают другие обращения по индексу к этому же массиву: j — 1. Предположу, что, опять же, ошибка не была замечена из-за некоторой схожести в написании символов j и 1, так что при беглом просмотре кода этого можно было не заметить.
Примечание коллеги Андрея Карпова. Когда я читал черновик статьи, то уже собирался сделать Сергею пометку, что он видимо не тот участок кода выписал. Я смотрел в код и не видел ошибку. Только начав читать описание, я понял, в чем дело. Подтверждаю, эту опечатку очень сложно заметить. Наш PVS-Studio шикарен!
Продолжим «ломать глаза»:
internal void SetRequestLine (string req)
{
....
if ((ic >= 'A' && ic <= 'Z') ||
(ic > 32 && c < 127 && c != '(' && c != ')' && c != '<' &&
c != '<' && c != '>' && c != '@' && c != ',' && c != ';' &&
c != ':' && c != '\' && c != '"' && c != '/' && c != '[' &&
c != ']' && c != '?' && c != '=' && c != '{' && c != '}'))
continue;
....
}Предупреждение PVS-Studio: V3001 There are identical sub-expressions 'c != '<'' to the left and to the right of the '&&' operator. HttpListenerRequest.cs 99
В пределах выражения дважды встречается подвыражение c != '<'. Наверное, просто лишнее сравнение.
protected virtual bool ShouldSerializeLinkHoverColor ()
{
return grid_style.LinkHoverColor != grid_style.LinkHoverColor;
}Предупреждение PVS-Studio: V3001 There are identical sub-expressions 'grid_style.LinkHoverColor' to the left and to the right of the '!=' operator. DataGrid.cs 2225
Упрощать метод, чтобы выделить ошибку, не понадобилось. В сравнении участвуют одинаковые подвыражения — grid_style.LinkHoverColor.
Корректный код должен выглядеть так:
protected virtual bool ShouldSerializeLinkHoverColor ()
{
return grid_style.LinkHoverColor != default_style.LinkHoverColor;
}Почему именно так? Выше есть ряд методов, в которых различные свойства объекта grid_style сравниваются со свойствами объекта default_style. Но в последнем случае расслабились и допустили ошибку. Хм, "эффект последней строки"?
Ну а это классика опечаток:
static bool AreEqual (VisualStyleElement value1,
VisualStyleElement value2)
{
return
value1.ClassName == value1.ClassName && // <=
value1.Part == value2.Part &&
value1.State == value2.State;
}Предупреждение PVS-Studio: V3001 There are identical sub-expressions 'value1.ClassName' to the left and to the right of the '==' operator. ThemeVisualStyles.cs 2141
Случайно сравнили подвыражение value1.ClassName само с собой. Конечно же, во втором случае должен был использоваться объект value2.
Думаю, если оформлять такой код табличным методом, то ошибку будет гораздо проще заметить. Это хорошая профилактика таких опечаток:
static bool AreEqual (VisualStyleElement value1,
VisualStyleElement value2)
{
return
value1.ClassName == value1.ClassName
&& value1.Part == value2.Part
&& value1.State == value2.State;
}Отформатированный таким образом код намного проще читать, и легче заметить, что с одним столбцом что-то не так. Подробнее про выравнивание кода смотрите главу 13 из книги "Главный вопрос программирования, рефакторинга и всего такого".
Остальные подозрительные места, обнаруженные диагностическим правилом V3001, приведены в файле.
Одинаковые условия в конструкции 'else if'
enum TitleStyle {
None = 0,
Normal = 1,
Tool = 2
}
internal TitleStyle title_style;
public Point MenuOrigin {
get {
....
if (this.title_style == TitleStyle.Normal) { // <=
pt.Y += caption_height;
} else if (this.title_style == TitleStyle.Normal) { // <=
pt.Y += tool_caption_height;
}
....
}Предупреждение PVS-Studio: V3003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 597, 599. Hwnd.cs 597
Два раза проверяется одинаковое выражение this.title_style == TitleStyle.Normal. Очевидно, что этот код содержит ошибку. Ведь вне зависимости от значения вышеприведённого выражения, выражение pt.Y += tool_caption_height никогда не будет выполнено. Предположу, что втором случае подразумевалось сравнение поля title_style с константой TitleStyle.Tool.
Одинаковые тела 'if-then' и 'if-else'
public static void DrawText (....)
{
....
if (showNonPrint)
TextRenderer.DrawTextInternal (g, text, font,
new Rectangle (new Point ((int)x, (int)y), max_size), color,
TextFormatFlags.NoPadding | TextFormatFlags.NoPrefix, false);
else
TextRenderer.DrawTextInternal (g, text, font,
new Rectangle (new Point ((int)x, (int)y), max_size), color,
TextFormatFlags.NoPadding | TextFormatFlags.NoPrefix, false);
....
}Предупреждение PVS-Studio: V3004 The 'then' statement is equivalent to the 'else' statement. System.Windows.Forms-net_4_x TextBoxTextRenderer.cs 79
Вне зависимости от значения переменной showNonPrint будет вызван статический метод DrawTextInternal класса TextRenderer с одними и теми же аргументами. Возможно, что ошибку допустили из-за использования copy-paste. Вызов метода скопировали, а исправить аргументы забыли.
Возвращаемое значение метода не используется
public override object ConvertTo(.... object value,
Type destinationType)
{
....
if (destinationType == typeof(string)) {
if (value == null) {
return String.Empty;
}
else {
value.ToString();
}
}
....
}Предупреждение PVS-Studio: V3010 The return value of function 'ToString' is required to be utilized. ColumnTypeConverter.cs 91
Весьма интересная ошибка, судя по всему, с далеко идущими последствиями. Из кода видно, что выполняется проверка типа, и, если тип — string, осуществляется проверка на равенство null. А дальше — самое интересное. Если ссылка value имеет значение null, возвращается пустая строка, а иначе… Скорее всего предполагалось, что будет возвращено строковое представление объекта, но оператор return отсутствует. Следовательно, возвращаемое значение метода ToString() никак не будет использовано, а метод ConvertTo продолжит своё выполнение. Таким образом, из-за забытого оператора return изменилась логика работы программы. Предполагаю, что корректный вариант кода должен выглядеть так:
if (value == null) {
return String.Empty;
}
else {
return value.ToString();
}
Какая ошибка описана здесь, узнаете позже
Обычно я упрощаю методы, чтобы ошибку было легче заметить. Предлагаю сыграть в игру. Найдите ошибку в следующем фрагменте метода. Чтобы было интереснее, я не буду писать тип ошибки и упрощать весь код (и так предоставляю только часть метода).
Ну что, как успехи? Мне почему-то кажется, что большинство даже не пытались. Не буду вас мучить.
Предупреждение PVS-Studio: V3012 The '?:' operator, regardless of its conditional expression, always returns one and the same value: Color.FromArgb (150, 179, 225). ProfessionalColorTable.cs 258
Вот он, злосчастный тернарный оператор:
button_pressed_highlight = use_system_colors ?
Color.FromArgb (150, 179, 225) :
Color.FromArgb (150, 179, 225);Независимо от значения переменной use_system_colors объекту button_pressed_highlight будет присвоено одно и то же значение. Если вы думаете, что такие ошибки сложно отследить, предлагаю вам взглянуть на файл целиком (ProfessionalColorTable.cs) и понять, что такие ошибки не просто сложно отследить самостоятельно — это просто нереально.
Подобных мест, кстати, весьма много (целых 32), что даже наводит на мысль о том, что это вовсе не ошибка, а таков замысел. Тем не менее, код выглядит странно, и я бы советовал его проверить. Даже если это не ошибка, а ожидаемая логика, проще ведь было бы использовать обыкновенное присваивание, чем писать странные тернарные операторы, сбивающие с толку. Остальные предупреждения V3012 приведены в файле.
Использование счетчика другого цикла
public override bool Equals (object obj)
{
if (obj == null)
return false;
PermissionSet ps = (obj as PermissionSet);
if (ps == null)
return false;
if (state != ps.state)
return false;
if (list.Count != ps.Count)
return false;
for (int i=0; i < list.Count; i++) {
bool found = false;
for (int j=0; i < ps.list.Count; j++) {
if (list [i].Equals (ps.list [j])) {
found = true;
break;
}
}
if (!found)
return false;
}
return true;
}Предупреждение PVS-Studio: V3015 It is likely that a wrong variable is being compared inside the 'for' operator. Consider reviewing 'i' corlib-net_4_x PermissionSet.cs 607
Подозрительным выглядит условие выхода из вложенного цикла: i < ps.list.Count. В качестве счётчика цикла выступает переменная j, однако в условии выхода используется счетчик внешнего цикла — переменная i.
Намерения автора кода понятны — проверить, что в коллекциях содержатся одинаковые элементы. Но если получится, что какой-то элемент коллекции list не содержится в ps.list, выход из вложенного цикла не будет осуществлён с помощью оператора break. В то же время внутри этого же цикла не изменяется переменная i, т.е. выражение i < ps.list.Count неизменно будет иметь значение true. В итоге цикл будет выполняться до тех пор, пока не произойдет выход за границу коллекции (из-за постоянного инкремента счётчика j).
Проверка не той ссылки на null после приведения с использованием оператора as
Как оказалось, это типовая ошибка для языка C#. Она находится чуть ли не в каждом проекте, по которому написана статья. Как правило, диагностическое правило V3019 обнаруживает случаи следующего вида:
var derived = base as Derived;
if (base == null) {
// do something
}
// work with 'derived' objectПроверка base == null спасёт только в том случае, если base действительно имеет значение null, и тогда нам неважно, удалось выполнить приведение или нет. Но очевидно, что подразумевалась проверка ссылки derived. И тогда, если base != null и не удалось выполнить преобразование, а дальше по методу идёт работа с членами объекта derived, будет сгенерировано исключение типа NullReferenceException.
Мораль: если вы используете данный паттерн, убедитесь, что проверяете на равенство null нужную сслыку.
Но это теория. Давайте посмотрим, что удалось обнаружить на практике:
public override bool Equals (object o)
{
UrlMembershipCondition umc = (o as UrlMembershipCondition);
if (o == null)
return false;
....
return (String.Compare (u, 0, umc.Url, ....) == 0); // <=
}Предупреждение PVS-Studio: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'o', 'umc'. UrlMembershipCondition.cs 111
Паттерн — один в один описанный выше. Если тип объекта o несовместим с типом UrlMembershipCondition, и при этом объект o не равен null, то при попытке обращения к свойству umc.Url будет сгенерировано исключение типа NullReferenceException.
Соответственно, для исправления ошибки необходимо исправить проверку:
if (umc == null)
return false;
Взглянем на ещё один ляп.
static bool QSortArrange (.... ref object v0, int hi,
ref object v1, ....)
{
IComparable cmp;
....
cmp = v1 as IComparable;
if (v1 == null || cmp.CompareTo (v0) < 0) {
....
}
....
}Предупреждение PVS-Studio: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'v1', 'cmp'. Array.cs 1487
Ситуация аналогична описанным выше. Разве что в случае неудачного приведения исключение NullReferenceException будет сгенерировано тут же — прямо при проверке выражения.
В общем-то, ситуация в других местах аналогична, так что просто приведу ещё 12 предупреждений в текстовом файле.
Безусловный выброс исключения
public void ReadEmptyContent(XmlReader r, string name)
{
....
for (r.MoveToContent();
r.NodeType != XmlNodeType.EndElement;
r.MoveToContent())
{
if (r.NamespaceURI != DbmlNamespace)
r.Skip();
throw UnexpectedItemError(r); // <=
}
....
}Предупреждение PVS-Studio: V3020 An unconditional 'throw' within a loop. System.Data.Linq-net_4_x XmlMappingSource.cs 180
На первой же итерации цикла будет сгенерировано исключение типа UnexpectedItemError. Как минимум, выглядит странно. К слову, Visual Studio подсвечивает объект r в секции изменения счётчика цикла с подсказкой о недостижимом коде. Но, вероятно, автор кода не использовал Visual Studio, или просто не заметил предупреждения, из-за чего ошибка осталась в коде.
Подозрительные операторы 'if'
Периодически встречаются ошибки такого типа, когда в методе присутствуют 2 одинаковых оператора 'if', причём значения объектов, используемых в условных выражениях этих операторов, между ними не меняются. При истинности любого из этих условных выражений будет осуществлён выход из тела метода. Таким образом, второй оператор 'if' никогда не будет выполнен. Давайте рассмотрим фрагмент кода, в котором была допущена подобная ошибка:
public int LastIndexOfAny (char [] anyOf, int startIndex, int count)
{
....
if (this.m_stringLength == 0)
return -1;
....
if (this.m_stringLength == 0)
return -1;
....
}Предупреждение PVS-Studio: V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless corlib-net_4_x String.cs 287
Выполнение метода никогда не дойдет до второго оператора if, приведённого в данном фрагменте, так как если this.m_stringLength == 0, выход будет осуществлён при выполнении первого условного оператора. Этот код был бы оправдан, если бы между операторами if изменялось бы значение поля m_stringLength, но это не так.
Следствия ошибки зависят от причины её возникновения:
- если оба условных выражения верны (с точки зрения логики), и второй код является просто избыточным — ничего страшного нет, но стоит его убрать, чтобы не сбивать с толку других людей;
- если в одном из операторов подразумевалась проверка другого выражения, или же в случае истинности выражения должны были выполняться другие действия — это более серьёзная проблема, свидетельствующая о наличии ошибки в логике программы. Тогда к решению вопроса стоит подойти более серьёзно.
Пример второго, более серьёзного случая ошибки, можно наблюдать в следующем фрагменте кода (нажмите на изображение для увеличения):
Заметить ошибку в этом коде не составит труда. Шучу, шучу, конечно составит. Но не для анализатора. Прибегнем к нашей стандартной методике упрощения кода, чтобы все стало понятнее:
private PaperKind GetPaperKind (int width, int height)
{
....
if (width == 1100 && height == 1700)
return PaperKind.Standard11x17;
....
if (width == 1100 && height == 1700)
return PaperKind.Tabloid;
....
}Предупреждение PVS-Studio: V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless System.Drawing-net_4_x PrintingServicesUnix.cs 744
При истинности выражения width == 1100 && height == 1700 будет выполнен только первый оператор if. Однако значения, возвращаемые при истинности данного выражения, отличаются, так что нельзя просто так сказать, что второй оператор if лишний. Более того — скорее всего в его условии должно находиться другое выражение. Налицо нарушение логики работы программы.
Напоследок я хотел бы рассмотреть ещё один фрагмент кода с подобной ошибкой:
private void SerializeCore (SerializationStore store,
object value, bool absolute)
{
if (value == null)
throw new ArgumentNullException ("value");
if (store == null)
throw new ArgumentNullException ("store");
CodeDomSerializationStore codeDomStore =
store as CodeDomSerializationStore;
if (store == null)
throw new InvalidOperationException ("store type unsupported");
codeDomStore.AddObject (value, absolute);
}Предупреждение PVS-Studio: V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless System.Design-plaindesign-net_4_x CodeDomComponentSerializationService.cs 562
Данное предупреждение пересекается с предупреждением V3019, так как здесь явно наблюдается паттерн проверки на null после приведения с использованием оператора as не той ссылки. Но какое бы предупреждение выдано ни было — ошибка очевидна.
Встречались и другие подобные предупреждения:
- V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless Mono.Data.Sqlite-net_4_x SQLiteDataReader.cs 270
- V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless System.Web-net_4_x HttpUtility.cs 220
- V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless System.Design-plaindesign-net_4_x CodeDomComponentSerializationService.cs 562
- V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless Mono.Security.Providers.DotNet-net_4_x DotNetTlsProvider.cs 77
Подозрительные строки форматирования
Диагностическое правило V3025 обнаруживает ошибочно составленные строки форматирования. Это также тип ошибок, встречающийся во многих проверяемых проектах. Обнаруживаются ситуации 2-ух видов:
- строка форматирования ожидает параметров больше, чем их представлено;
- строка форматирования ожидает параметров меньше, чем их представлено.
В первом случае будет сгенерировано исключение типа FormatException, во втором случае неиспользуемые аргументы просто будут проигнорированы. Так или иначе, стоит уделить внимание таким местам и исправить код должным образом.
Конечно, я бы не стал вспоминать это диагностическое правило, если бы в коде не нашлись подобные ошибки.
static IMessageSink GetClientChannelSinkChain(string url, ....)
{
....
if (url != null)
{
string msg = String.Format (
"Cannot create channel sink to connect to URL {0}.
An appropriate channel has probably not been registered.",
url);
throw new RemotingException (msg);
}
else
{
string msg = String.Format (
"Cannot create channel sink to connect to the remote object.
An appropriate channel has probably not been registered.",
url);
throw new RemotingException (msg);
}
....
}Предупреждение PVS-Studio: V3025 Incorrect format. A different number of format items is expected while calling 'Format' function. Arguments not used: url. corlib-net_4_x RemotingServices.cs 700
Хочу обратить ваше внимание на вторую строку форматирования. Она представляет собой строковый литерал, в котором не предусмотрена подстановка аргументов (в отличии от строки форматирования выше). Тем не менее, метод Format принимает в качестве второго аргумента объект url. Из вышесказанного следует, что объект url просто будет проигнорирован при формировании новой строки, а информация о нем не попадет в текст исключения.
В C# 6.0 были добавлены интерполированные строки, которые в некоторых случаях помогут избежать проблем, связанных с использованием строк форматирования, в том числе — с несоответствующим количеством аргументов.
Рассмотрим ещё один фрагмент ошибочного кода.
public override string ToString ()
{
return string.Format ("ListViewSubItem {{0}}", text);
}Предупреждение PVS-Studio: V3025 Incorrect format. A different number of format items is expected while calling 'Format' function. Arguments not used: text. System.Windows.Forms-net_4_x ListViewItem.cs 1287
Исходя из строки форматирования можно сделать вывод о том, что в итоговой строке должен был присутствовать текст в фигурных скобках. На деле же результирующая строка будет иметь следующий вид:
"ListViewSubItem {{0}}"Для исправления данной ошибки отлично бы подошли интерполированные строки, используя которые метод можно было бы переписать следующим образом:
public override string ToString ()
{
return $"ListViewSubItem {{{text}}}",;
}Или же, оставаясь верным методу String.Format, стоило добавить по одной фигурной скобке с каждой стороны. Тогда строка форматирования выглядела бы следующим образом:
"ListViewSubItem {{{0}}}"И последний фрагмент кода со строками форматирования. Как всегда, самое интересное осталось на десерт:
void ReadEntropy ()
{
if (reader.IsEmptyElement)
throw new XmlException (
String.Format ("WS-Trust Entropy element is empty.{2}",
LineInfo ()));
....
}Предупреждение PVS-Studio: V3025 Incorrect format. A different number of format items is expected while calling 'Format' function. Format items not used: {2}. Arguments not used: 1st. System.ServiceModel-net_4_x WSTrustMessageConverters.cs 147
Не знаю, каким образом в строку форматирования затесался элемент форматирования с индексом '2', но он приводит к весьма интересной ошибке. Планировалось выбросить исключение с каким-то текстом, составляемым строкой форматирования. И исключение будет сгенерировано. Исключение типа FormatException, ведь для текущей строки форматирования необходимы 3 аргумента (так как требуется именно третий), а представлен только один.
Если каким-то образом перепутали только номер запрашиваемого аргумента (например, в ходе рефакторинга), исправить ошибку не составит труда:
"WS-Trust Entropy element is empty.{0}"Прочие подозрительные места, обнаруженные правилом V3025, приведены в файле.
Разыменование нулевой ссылки
private bool IsContractMethod (string methodName,
Method m,
out TypeNode genericArgument)
{
....
return m.Name != null && m.Name == methodName &&
(m.DeclaringType.Equals (this.ContractClass) // <=
|| (m.Parameters != null &&
m.Parameters.Count == 3 &&
m.DeclaringType != null && // <=
m.DeclaringType.Name != ContractClassName));
}Предупреждение PVS-Studio: V3027 The variable 'm.DeclaringType' was utilized in the logical expression before it was verified against null in the same logical expression. Mono.CodeContracts-net_4_x ContractNodes.cs 211
Перед тем, как обратиться к свойству Name свойства DeclaringType, программист решил подстраховаться и проверить DeclaringType на неравенство null, чтобы случайно не разыменовать нулевую ссылку. Желание понятное и вполне законное — всё правильно. Вот только это все равно не возымеет действия, так как выше по коду у свойства DeclaringType вызывается экземплярный метод Equals, а значит, если DeclaringType == null, будут сгенерировано исключение типа NullReferenceException. Для решения проблемы можно, например, перенести проверку на неравенство null выше по коду, или использовать null-условный оператор ('?.'), доступный в C# 6.0.
Другой случай.
Node leftSentinel;
....
IEnumerator<T> GetInternalEnumerator ()
{
Node curr = leftSentinel;
while ((curr = curr.Nexts [0]) != rightSentinel && curr != null) {
....
}
}Предупреждение PVS-Studio: V3027 The variable 'curr' was utilized in the logical expression before it was verified against null in the same logical expression. Mono.Parallel-net_4_x ConcurrentSkipList.cs 306
И опять та же ситуация. Если curr == null, будет осуществлён выход из цикла. Но вот если curr изначально имел значение null (т.е. на момент выполнения кода leftSentinel == null), опять получим исключение NullReferenceException.
Избыточная проверка
В проверяемых проектах периодически встречаются выражения следующего вида или им подобные:
!aa || (aa && bb)Их можно упростить до выражения следующего вида:
!aa || bbВозможно, в некоторых случаях вы получите выигрыш в производительности (пусть и незначительный), но также второй вариант читается легче при том, что он логически эквивалентен первому (в случае, если подвыражение аa не меняется между вызовами).
Возможно, что вместо aa должно было находиться другое подвыражение:
!aa || (cc && bb)Тогда речь идёт о настоящей ошибке. Так или иначе, в анализаторе PVS-Studio присутствует диагностическое правило V3031, обнаруживающее подобные случи. Рассмотрим несколько фрагментов кода, которые удалось обнаружить с его помощью:
public void Emit(OpCode opc)
{
Debug.Assert(opc != OpCodes.Ret || (
opc == OpCodes.Ret && stackHeight <= 1));
....
}Предупреждение PVS-Studio: V3031 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions. mcs-net_4_x ILGenerator.cs 456
Избыточный код. Обращение к объекту opc не изменяет его значения, так что данное выражение можно упростить:
Debug.Assert(opc != OpCodes.Ret || stackHeight <= 1));Другой фрагмент кода:
public bool Validate (bool checkAutoValidate)
{
if ((checkAutoValidate && (AutoValidate != AutoValidate.Disable)) ||
!checkAutoValidate)
return Validate ();
return true;
}Предупреждение PVS-Studio: V3031 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions. System.Windows.Forms-net_4_x ContainerControl.cs 506
Ситуация аналогична предыдущей. Выражение легко и безболезненно упрощается до следующего вида:
!checkAutoValidate || (AutoValidate != AutoValidate.Disable)Некоторые отобранные мной предупреждения приведены в файле.
Форматирование кода, не соответствующее логике
При разработке диагностических правил, подобных V3033, были споры о том, насколько они актуальны. Дело в том, что диагностики, завязанные на форматировании кода, весьма специфичны, так как многие редакторы/среды разработки (та же Visual Studio) сами форматируют код уже по ходу его написания. Следовательно, вероятность допустить такую ошибку весьма мала. Я редко встречал в проверяемых проектах такие ошибки, но в 'Mono' парочка всё же нашлась.
public bool this [ string header ] {
set {
....
if (value)
if (!fields.Contains (header))
fields.Add (header, true);
else
fields.Remove (header);
}
}Предупреждение PVS-Studio: V3033 It is possible that this 'else' branch must apply to the previous 'if' statement. HttpCacheVaryByHeaders.cs 159
Код отформатирован так, что может показаться, будто else относится к первому оператору if. Но компилятору совершенно без разницы, как отформатирован ваш код, поэтому он расшифрует данный фрагмент по-своему, ассоциировав else со вторым оператором if, как и положено. Налицо интересная ошибка. Код, отформатированный в соответствии с заданной логикой должен выглядеть так:
if (value)
if (!fields.Contains (header))
fields.Add (header, true);
else
fields.Remove (header);Аналогичное предупреждение встретилось ещё раз: V3033 It is possible that this 'else' branch must apply to the previous 'if' statement. HttpCacheVaryByParams.cs 102
К этой же категории можно отнести ещё одно диагностическое правило — V3043.
Ошибочный код:
public void yyerror (string message, string[] expected) {
....
for (int n = 0; n < expected.Length; ++ n)
ErrorOutput.Write (" "+expected[n]);
ErrorOutput.WriteLine ();
....
}Предупреждение PVS-Studio: V3043 The code's operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. cs-parser.cs 175
Если судить по форматированию кода (забыв про правила языка программирования), можно подумать, что оба вызова метода (Write и WriteLine) относятся к оператору for. На самом деле в цикле будет вызываться только метод Write. Этот код точно в чём-то ошибочен! Если подразумевалась такая логика (казалось бы, всё логично — выводятся элементы, после чего вставляется пустая строка), к чему форматирование, вводящее в заблуждение? С другой стороны — бегло просматривая код, можно сразу и не понять истинную логику оператора. Не просто так же программисты придерживаются определённых стилей форматирования.
private string BuildParameters ()
{
....
if (result.Length > 0)
result.Append (", ");
if (p.Direction == TdsParameterDirection.InputOutput) // <=
result.Append (String.Format("{0}={0} output",
p.ParameterName));
else
result.Append (FormatParameter (p));
....
}Предупреждение PVS-Studio: V3043 The code's operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. Tds50.cs 379
Второй оператор if никак не относится к первому. Зачем тогда вводить в заблуждение людей, работающих с этим кодом?
public void Restore ()
{
while (saved_count < objects.Count)
objects.Remove (objects.Last ().Key);
referenced.Remove (objects.Last ().Key);
saved_count = 0;
referenced.RemoveRange (saved_referenced_count,
referenced.Count - saved_referenced_count);
saved_referenced_count = 0;
}Предупреждение PVS-Studio: V3043 The code's operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. XamlNameResolver.cs 81
Судя по всему, планировалось удалить из коллекций objects и referenced значения, соответствующие определённому ключу. Но забыли про фигурные скобки, в итоге из коллекции referenced будет удалено только одно значение. Что ещё интереснее — одними фигурными скобками тут не отделаться, так как в таком случае на каждой итерации цикла из коллекции referenced будет удаляться объект не по тому ключу, по которому происходило удаление из коллекции objects. Это связано с тем, что на момент вызова метода Remove для коллекции referenced коллекция objects уже будет изменена, а значит метод Last вернёт другой элемент.
Встречались ещё предупреждения об ошибках с форматированием, не соответствующим логике программы. Вот некоторые из них:
- V3043 The code's operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. ExpressionParser.cs 92
- V3043 The code's operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. EcmaUrlParser.cs 80
- V3043 The code's operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. ILParser.cs 167
Приведение объекта к собственному типу / проверка объекта на совместимость с собственным типом
За поиск подобных ситуаций отвечает диагностическое правило V3051. Как правило, оно находит избыточный код вида
String str;
String str2 = str as String;или
String str;
if (str is String)Но встречаются (хотя и редко) куда более интересные случаи.
Рассмотрим фрагмент кода:
public string GenerateHttpGetMessage (Port port,
OperationBinding obin,
Operation oper,
OperationMessage msg)
{
....
MimeXmlBinding mxb =
(MimeXmlBinding) obin.Output
.Extensions
.Find (typeof(MimeXmlBinding))
as MimeXmlBinding;
if (mxb == null) return req;
....
}Предупреждение PVS-Studio: V3051 An excessive type cast. The object is already of the 'MimeXmlBinding' type. SampleGenerator.cs 232
Казалось бы — лишнее приведение, ну и что? Ниже проверяем mxb на равенство null, так что, если тип несовместим — ничего страшного. Не тут-то было. Метод Find возвращает экземпляр типа Object, после чего его явно приводят к типу MimeXmlBinding и уже после этого выполняют приведение к этому же типу с использованием оператора as. Однако оператор явного приведения, в случае, если аргумент имеет значение null или несовместимый тип, не возвращает null (в отличии от оператора as), а генерирует исключение типа InvalidCastException. В итоге проверка mxb == null никак не поможет при неудачном привидении типов.
Остальные предупреждения не столь интересны (например, избыточное привидение), поэтому приведу их списком в текстовом файле.
Параметр перезаписывается в теле метода до того, как используется
Сам по себе факт того, что параметр метода сразу перезаписывается, выглядит подозрительно. Ведь пришедшее в метод значение никак не используется, а попросту игнорируется / теряется. В идеале стоит исправить сигнатуру метода, а параметр сделать локальной переменной. Правда такой подход не всегда возможен. Например, при реализации интерфейса или виртуальных функций.
internal static int SetErrorInfo (int dwReserved,
IErrorInfo errorInfo)
{
int retVal = 0;
errorInfo = null;
....
retVal = _SetErrorInfo (dwReserved, errorInfo);
....
}Предупреждение PVS-Studio: V3061 Parameter 'errorInfo' is always rewritten in method body before being used. corlib-net_4_x Marshal.cs 1552
Значение, пришедшее в качестве параметра errorInfo, никак не используется. Параметр сразу обнуляется, а затем передаётся в метод. В таком случае логично было бы сделать errorInfo локальной переменной (если возможно изменение сигнатуры метода).
Остальные места схожи, поэтому, как и раньше, привожу перечень предупреждений списком в файле.
Неверная инициализация статических членов
class ResXResourceWriter : IResourceWriter, IDisposable
{
....
public static readonly string ResourceSchema = schema;
....
static string schema = ....;
....
}
Предупреждение PVS-Studio: V3070 Uninitialized variable 'schema' is used when initializing the 'ResourceSchema' variable. ResXResourceWriter.cs 59
Программист хотел задать значение публичного статического поля ResourceSchema, доступного только для чтения, равным другому статическому полю — schema. Без ошибки не обошлось. На момент инициализации ResourceSchema поле schema будет проинициализировано значением по умолчанию (null в данном случае). Маловероятно, что именно этого разработчик и добивался.
Ошибочная инициализация статического поля, декорированного атрибутом [ThreadStatic]
Редкая и интересная ошибка. Рассмотрим фрагмент кода:
static class Profiler
{
[ThreadStatic]
private static Stopwatch timer = new Stopwatch();
....
}Предупреждение PVS-Studio: V3089 Initializer of a field marked by [ThreadStatic] attribute will be called once on the first accessing thread. The field will have default value on different threads. System.Data.Linq-net_4_x Profiler.cs 16
Декорирование поля атрибутом [ThreadStatic] означает, что в каждом потоке значение этого поля будет уникальным. Вроде бы ничего сложного. Но дело в том, что такие поля нельзя инициализировать ни при объявлении, ни в статическом конструкторе. Это отличный способ выстрелить себе в ногу (или даже обе) и получить трудноуловимую ошибку.
Дело в том, если выполняется инициализация при объявлении, поле будет проинициализировано этим значением только у первого обратившегося потока. Для остальных потоков поле будет содержать значение по умолчанию (в данном случае — null, так как Stopwatch — ссылочный тип). Статический конструктор будет вызван также только один раз — при обращении первого потока. Следовательно, в остальных потоках поле также будет проинициализировано значением по умолчанию.
Ошибка весьма заковыристая, так что настоятельно рекомендую ознакомиться с документацией к диагностическому правилу, чтобы не допустить подобных ситуаций и не тратить драгоценное время на отладку.
Пересекающиеся диапазоны
public void EmitLong (long l)
{
if (l >= int.MinValue && l <= int.MaxValue) {
EmitIntConstant (unchecked ((int) l));
ig.Emit (OpCodes.Conv_I8);
} else if (l >= 0 && l <= uint.MaxValue) {
EmitIntConstant (unchecked ((int) l));
ig.Emit (OpCodes.Conv_U8);
} else {
ig.Emit (OpCodes.Ldc_I8, l);
}
}Предупреждение PVS-Studio: V3092 Range intersections are possible within conditional expressions. Example: if (A > 0 && A < 5) {… } else if (A > 3 && A < 9) {… }. mcs-net_4_x codegen.cs 742
Анализатор счёл подозрительным пересечения диапазонов в выражениях:
- l >= int.MinValue && l <= int.MaxValue
- l >= 0 && l <= uint.MaxValue
Для обоих этих выражений общим является диапазон [0, Int32.MaxValue], так что если переменная l имеет значение, лежащее в этом диапазоне, будет выполнено первое условие, несмотря на то, что второе тоже могло бы выполниться.
Схожие предупреждения:
- V3092 Range intersections are possible within conditional expressions. Example: if (A > 0 && A < 5) {… } else if (A > 3 && A < 9) {… }. I18N.CJK-net_4_x CP51932.cs 437
- V3092 Range intersections are possible within conditional expressions. Example: if (A > 0 && A < 5) {… } else if (A > 3 && A < 9) {… }. I18N.CJK-net_4_x CP932.cs 552
- V3092 Range intersections are possible within conditional expressions. Example: if (A > 0 && A < 5) {… } else if (A > 3 && A < 9) {… }. I18N.CJK-net_4_x ISO2022JP.cs 460
Доступ к элементу коллекции по константному индексу, осуществляемый внутри цикла
Практика давать счётчикам циклов короткие имена распространена повсеместно. Нет ничего плохого в том, чтобы назвать счётчик цикла i или j — сразу понятно, в качестве чего выступает эта переменная (за исключением тех случаев, когда счётчики требуют все же более осмысленных имён). Но бывают случаи, когда в качестве индекса используют не счётчик цикла, и числовой литерал. Понятно, что иногда это делается специально, но порой это свидетельствует об ошибке. Диагностическое правило V3102 ищет подобные ошибочные места.
public void ConvertGlobalAttributes (
TypeContainer member,
NamespaceContainer currentNamespace,
bool isGlobal)
{
var member_explicit_targets = member.ValidAttributeTargets;
for (int i = 0; i < Attrs.Count; ++i) {
var attr = Attrs[0];
if (attr.ExplicitTarget == null)
continue;
....
}
}Предупреждение PVS-Studio: V3102 Suspicious access to element of 'Attrs' object by a constant index inside a loop. mcs-net_4_x attribute.cs 1272
На каждой итерации цикла переменная attr инициализируется одним и тем же значением — Attrs[0]. В дальнейшем с ней осуществляется некоторая работа (вызываются свойства, передаётся в метод). Навряд ли на всех итерациях цикла хотели работать с одним и тем же значением, так что, полагаю, правильная инициализация должна выглядеть следующим образом:
var attr = Attrs[i];Схожие ошибки встретились ещё в 2-ух местах:
- V3102 Suspicious access to element of 'seq' object by a constant index inside a loop. System.Xml-net_4_x XmlQueryRuntime.cs 679
- V3102 Suspicious access to element of 'state' object by a constant index inside a loop. System.Web-net_4_x Login.cs 1223
Небезопасные блокировки
Во многих проверяемых проектах приходится сталкиваться с кодом вида lock(this) или lock(typeof(....)). Это не лучший способ блокировки, так как он может привести к возникновению взаимоблокировки. Но давайте обо всем по порядку. Для начала посмотрим на опасный код:
public RegistryKey Ensure (....)
{
lock (typeof (KeyHandler)){
....
}
}Предупреждение PVS-Studio: V3090 Unsafe locking on a type. All instances of a type will have the same 'Type' object. corlib-net_4_x UnixRegistryApi.cs 245
Общая проблема потенциального возникновения взаимоблокировки связана с тем, что блокировка осуществляется по одному и тому же объекту. Следовательно, общий совет, который позволит избавиться от подобных проблем — использовать в качестве объекта блокировки недоступный извне объект — локальную переменную или приватное поле класса.
В чем же проблема с оператором typeof? Данный оператор всегда возвращает экземпляр типа Type, причем один и тот же для одного и того же аргумента, следовательно, нарушается правило, описанное выше — возникает проблема блокировки по одному и тому же объекту.
В коде проекта встретилось несколько таких мест:
- V3090 Unsafe locking on a type. All instances of a type will have the same 'Type' object. corlib-net_4_x UnixRegistryApi.cs 245
- V3090 Unsafe locking on a type. All instances of a type will have the same 'Type' object. corlib-net_4_x UnixRegistryApi.cs 261
- V3090 Unsafe locking on a type. All instances of a type will have the same 'Type' object. corlib-net_4_x UnixRegistryApi.cs 383
- V3090 Unsafe locking on a type. All instances of a type will have the same 'Type' object. corlib-net_4_x UnixRegistryApi.cs 404
- V3090 Unsafe locking on a type. All instances of a type will have the same 'Type' object. corlib-net_4_x UnixRegistryApi.cs 451
- V3090 Unsafe locking on a type. All instances of a type will have the same 'Type' object. corlib-net_4_x UnixRegistryApi.cs 469
- V3090 Unsafe locking on a type. All instances of a type will have the same 'Type' object. corlib-net_4_x UnixRegistryApi.cs 683
- V3090 Unsafe locking on a type. All instances of a type will have the same 'Type' object. corlib-net_4_x UnixRegistryApi.cs 698
- V3090 Unsafe locking on a type. All instances of a type will have the same 'Type' object. System-net_4_x NetworkChange.cs 66
- V3090 Unsafe locking on a type. All instances of a type will have the same 'Type' object. System-net_4_x NetworkChange.cs 74
- V3090 Unsafe locking on a type. All instances of a type will have the same 'Type' object. System-net_4_x NetworkChange.cs 85
- V3090 Unsafe locking on a type. All instances of a type will have the same 'Type' object. System-net_4_x NetworkChange.cs 93
Ситуация с методом GetType() принципиально ничем не отличается от вышеописанной:
void ConfigureHttpChannel (HttpContext context)
{
lock (GetType())
{
....
}
}Предупреждение PVS-Studio: V3090 Unsafe locking on a type. All instances of a type will have the same 'Type' object. System.Runtime.Remoting-net_4_x HttpRemotingHandlerFactory.cs 61
Метод GetType() также возвращает экземпляр типа Type, поэтому, если в другом месте блокировка будет осуществляться с использованием метода GetType() или оператора typeof, применительно к объекту того же типа — возникает вероятность взаимоблокировки.
Отдельно хотелось бы рассмотреть блокировку по объектам типа String, так как в этом случае ситуация становится куда интереснее, а ошибки — более вероятными и трудными в обнаружении.
const string Profiles_SettingsPropertyCollection =
"Profiles.SettingsPropertyCollection";
....
static void InitProperties ()
{
....
lock (Profiles_SettingsPropertyCollection) {
if (_properties == null)
_properties = properties;
}
}Предупреждение PVS-Studio: V3090 Unsafe locking on an object of type 'String'. System.Web-net_4_x ProfileBase.cs 95
Суть проблемы остается неизменной — блокировка по одному и тому же объекту, а вот детали уже более интересны. Доступ к объектам типа String можно получить даже из другого домена приложения (это связано с механизмом интернирования строк). Представьте, каково отлаживать взаимоблокировку, возникшую в результате того, что в разных доменах приложений использовалась одна и та же строка в качестве объекта блокировки. Совет краток — не используйте в качестве объектов блокировки объекты типа String (и Thread, к слову, тоже). Эти и другие проблемы, связанные с использованием механизмов синхронизации (а также способы их избежать), описаны в документации к диагностическому правилу V3090.
Ошибочное сравнение
public bool Equals (CounterSample other)
{
return
rawValue == other.rawValue &&
baseValue == other.counterFrequency &&
counterFrequency == other.counterFrequency &&
systemFrequency == other.systemFrequency &&
timeStamp == other.timeStamp &&
timeStamp100nSec == other.timeStamp100nSec &&
counterTimeStamp == other.counterTimeStamp &&
counterType == other.counterType;
}Предупреждение PVS-Studio: V3112 An abnormality within similar comparisons. It is possible that a typo is present inside the expression 'baseValue == other.counterFrequency'. System-net_4_x CounterSample.cs 139
Я умышленно не стал изменять форматирование кода, оставив его в таком же виде, каким оно было в первоисточнике. Замысел метода понятен и проблем для понимания не вызывает — просто сравниваются поля текущего объекта и объекта, пришедшего в качестве аргумента. Но даже такой, казалось бы, простой метод содержит ошибку. Уверен, что если бы форматирование кода было выполнено лучше, заметить её было бы проще. Так выглядит этот же фрагмент кода, но отформатированный:
public bool Equals (CounterSample other)
{
return
rawValue == other.rawValue &&
baseValue == other.counterFrequency && // <=
counterFrequency == other.counterFrequency && // <=
systemFrequency == other.systemFrequency &&
timeStamp == other.timeStamp &&
timeStamp100nSec == other.timeStamp100nSec &&
counterTimeStamp == other.counterTimeStamp &&
counterType == other.counterType;
}
Думаю, с таким форматированием ошибку заметить куда проще — видно, что с разными полями текущего объекта сравнивается одно и то же поле сравниваемого. Как следствие — нарушается логика работы программы.
Из этого фрагмента кода хорошо видно — форматирование играет важную роль! И если компилятору безразлично, как оформлен ваш код, для программистов это важно и позволит избежать досадных ошибок, а также упростит восприятие кода и понимание логики работы программы.
Как всё это править?
Хорошо, ошибки нашли. Много ошибок. Если посчитать ошибки, описанные в статье, а также те, которые были приведены в файлах, выходит 167 штук! Хочу отметить, что это далеко не все ошибочные / подозрительные места — я просто выбрал достаточно материала для статьи (что не составило проблем). Многие другие ошибки, пожалуй, даже большинство, оставил за рамками статьи, потому что она и так вышла достаточно объёмной.
Может возникнуть резонный вопрос, как всё это исправлять? Как внедрить статический анализатор в проект?
Маловероятно, что будет выделена отдельная команда, которая будет заниматься исключительно исправлением ошибок (разве что мы сами эти ошибки и будем править, как было в случае с Unreal Engine). Правильно будет зафиксировать имеющиеся ошибки, которые будут постепенно исправляться, и не допускать появления новых.
Для решения первой задачи поможет механизм массового подавления предупреждений. Это позволит дифференцировать старые и новые ошибки, наблюдая за появлением и исправлением ошибок только в новом коде. Старые ошибки правятся отдельно.
Режим инкрементального анализа призван решить вторую проблему. Этот режим запускает анализ на машине разработчика сразу же после компиляции, позволяя обнаруживать ещё свежие ошибки и исправлять их до попадания в систему контроля версий (СКВ).
Так или иначе, скорее всего какой-то процент ошибок попадёт в СКВ. Чтобы обнаруживать ошибочный код максимально быстро после попадания в СКВ, хорошим решением будет внедрение статического анализа в ночные сборки и использование результатов анализа. Каким образом? Можно, например, оповещать человека, ответственного за проект, а также программиста, который допустил попадание этой ошибки в репозиторий.
Важно исправлять такие ошибки незамедлительно, пока они не 'обросли' дополнительным кодом, и задача исправления не усложнилась в несколько раз.
Сочетая описанные выше советы (внедрение статического анализа как на билд-сервере, так и на машинах разработчиков) позволит сократить расходы на исправление ошибок, так как при правильной организации процесса они будут исправляться максимально быстро (не доходя до тестеров и уж тем более, не попадая в релизы).
Подробнее про внедрение статического анализа в процесс разработки можно прочитать в статье "Как быстро внедрить статический анализ в большой проект?".
Заключение
В комментариях к одной из статей писали: «Вы пишете, что проверяете open-source продукты вашим анализатором, но на самом деле вы проверяете ваш анализатор open-source продуктами!». В этом есть истина.
Действительно, мы постоянно работаем над улучшением анализатора, и проверка проектов позволяет нам делать его лучше — исправлять ложные срабатывания, учить анализатор находить новые ошибки и т.п. Эта сторона проверки обычно остается за рамками статьи, так как интересна скорее разработчикам анализатора.
Но мы все же проверяем проекты и, что важно, находим там реальные ошибки. Не просто находим, а стараемся донести эту информацию до разработчиков и всех интересующихся. А уж как этой информацией пользоваться — личное дело каждого. Думаю, польза от нашей работы для open-source сообщества однозначно есть. Не так давно мы перевалили за отметку 10,000 найденных ошибок!
А вообще, наша главная цель — популяризация методологии статического анализа в целом и анализатора PVS-Studio в частности. И наши статьи являются отличным способом демонстрации пользы этой методологии.
Попробуйте PVS-Studio на своём проекте: http://www.viva64.com/ru/pvs-studio/
Конечно, немного грустно, что не удалось проверить скомпилированный проект, из-за чего действительно полного и глубокого анализа не вышло. С другой стороны — материала на статью хватило и без этого, так что разработчикам стоит задуматься об исправлении ошибок и внедрении статического анализа в процесс разработки. А мы всегда рады им в этом помочь.
