Ищем ошибки в Mono: сотни их
Крупные проекты проверять интересно. Как правило, в них удаётся найти различные интересные ошибки и рассказать о них людям. Также это хороший способ тестирования нашего анализатора и улучшения различных его аспектов. Я давно хотел проверить '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 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 будет удалено только одно значение. Что ещё интереснее — одн
