# Топ 10 ошибок в проектах C\# за 2016 год Для оценки качества работы нашего анализатора, а также с целью популяризации методологии статического анализа, мы регулярно проверяем на наличие ошибок проекты с открытым исходным кодом и пишем про это статьи\. Не стал исключением и прошедший 2016 год, который примечателен ещё и тем, что это было время своеобразного "взросления" C\# анализатора\. PVS\-Studio получил значительное количество новых C\# диагностик, улучшенный механизм работы с виртуальными значениями \(symbolic execution\) и многое другое\. По результатам проделанной нашей командой работы, я составил своеобразный хит\-парад наиболее интересных ошибок, обнаруженных в проектах С\# в 2016 году\. ![0480_Top_10_CSharp_Bugs_2016_ru/image1.png](https://import.viva64.com/docx/blog/0480_Top_10_CSharp_Bugs_2016_ru/image1.png) **Десятое место: когда в минуте не всегда 60 секунд** Начну хит\-парад с ошибки, обнаруженной при проверке проекта Orchard CMS\. Описание ошибки можно найти в [статье](https://pvs-studio.ru/ru/blog/posts/csharp/0456/)\. Вообще же, со всеми статьями про проверку проектов можно ознакомиться [здесь](https://pvs-studio.ru/ru/blog/inspections/)\. [V3118](https://pvs-studio.ru/ru/docs/warnings/v3118/) Seconds component of TimeSpan is used, which does not represent full time interval\. Possibly 'TotalSeconds' value was intended instead\. AssetUploader\.cs 182 ```cpp void IBackgroundTask.Sweep() { .... // Don't flood the database with progress updates; // Limit it to every 5 seconds. if ((_clock.UtcNow - lastUpdateUtc).Seconds >= 5) { .... } } ``` Вместо _TotalSeconds_ в данном случае разработчик ошибочно использовал _Seconds_\. Таким образом, будет получено не полное число секунд, содержащееся между датами _\_clock\.UtcNow_ и _lastUpdateUtc_, как рассчитывал программист, а только остаточное значение диапазона\. Например, для значения диапазона 1 минута 4 секунды результатом будет не 64, а 4 секунды\. Невероятно, но даже опытные разработчики допускают подобные ошибки\. **Девятое место: выражение всегда истинно** Следующая ошибка приведена в [статье](https://pvs-studio.ru/ru/blog/posts/csharp/0440/) о проверке проекта GitExtensions\. [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'string\.IsNullOrEmpty\(rev1\) \|\| string\.IsNullOrEmpty\(rev2\)' is always true\. GitUI FormFormatPatch\.cs 155 ```cpp string rev1 = ""; string rev2 = ""; var revisions = RevisionGrid.GetSelectedRevisions(); if (revisions.Count > 0) { rev1 = ....; rev2 = ....; .... } else if (string.IsNullOrEmpty(rev1) || string.IsNullOrEmpty(rev2)) // <= { MessageBox.Show(....); return; } ``` Обратите внимание на ключевое слово _else_\. Вероятно, ему вовсе тут не место\. Невнимательность при рефакторинге или банальная усталость программиста, и вот мы получаем кардинальное изменение логики работы программы, что приводит к непредсказуемому поведению\. Хорошо, что статический анализатор никогда не устаёт\. **Восьмое место: возможная опечатка** В [статье](https://pvs-studio.ru/ru/blog/posts/csharp/0412/) о проверке исходного кода FlashDevelop приведена интересная ошибка, связанная с опечаткой\. [V3056](https://pvs-studio.ru/ru/docs/warnings/v3056/) Consider reviewing the correctness of 'a1' item's usage\. LzmaEncoder\.cs 225 ```cpp public void SetPrices(....) { UInt32 a0 = _choice.GetPrice0(); UInt32 a1 = _choice.GetPrice1(); UInt32 b0 = a1 + _choice2.GetPrice0(); // <= UInt32 b1 = a1 + _choice2.GetPrice1(); .... } ``` Я согласен с анализатором, а также с автором статьи\. Вместо переменной _a1_ в отмеченной строке напрашивается использование _а0_\. В любом случае, не мешало бы дать переменным более понятные имена\. **Седьмое место: логическая ошибка** По мотивам повторной проверки проекта Umbraco также была написана [статья](https://pvs-studio.ru/ru/blog/posts/csharp/0461/)\. Пример интересной, на мой взгляд, ошибки из этой статьи\. [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'name \!\= "Min" \|\| name \!\= "Max"' is always true\. Probably the '&&' operator should be used here\. DynamicPublishedContentList\.cs 415 ```cpp private object Aggregate(....) { .... if (name != "Min" || name != "Max") { throw new ArgumentException( "Can only use aggregate min or max methods on properties which are datetime"); } .... } ``` Исключение типа _ArgumentException_ будет выброшено при любом значении переменной _name_\. И всё из\-за ошибочного использования в условии оператора \|\| вместо &&\. **Шестое место: ошибочное условие выхода из цикла** [Статья](https://pvs-studio.ru/ru/blog/posts/csharp/0410/) о проверке проекта Accord\.Net содержит описание нескольких интересных ошибок\. Я выбрал две, одна из которых вновь связана с опечаткой\. [V3015](https://pvs-studio.ru/ru/docs/warnings/v3015/) It is likely that a wrong variable is being compared inside the 'for' operator\. Consider reviewing 'i' Accord\.Audio SampleConverter\.cs 611 ```cpp public static void Convert(float[][] from, short[][] to) { for (int i = 0; i < from.Length; i++) for (int j = 0; i < from[0].Length; j++) to[i][j] = (short)(from[i][j] * (32767f)); } ``` Ошибка содержится в условии второго цикла _for_, счётчиком которого является переменная _j_\. Использование имен переменных вида _i_, _j_ для счётчиков \- это, своего рода, классика жанра\. К сожалению, эти переменные очень схожи по написанию и разработчики часто допускают опечатки в подобном коде\. Не думаю, что в данном случае стоит рекомендовать использование более осмысленных имен\. Все равно так делать никто не будет :\)\. Поэтому дам другую рекомендацию: используйте статические анализаторы\! **Пятое место: использование битового оператора вместо логического** Еще одна интересная и достаточно распространенная ошибка из [статьи](https://pvs-studio.ru/ru/blog/posts/csharp/0410/) о проверке проекта Accord\.Net\. [V3093](https://pvs-studio.ru/ru/docs/warnings/v3093/) The '&' operator evaluates both operands\. Perhaps a short\-circuit '&&' operator should be used instead\. Accord\.Math JaggedSingularValueDecompositionF\.cs 461 ```cpp public JaggedSingularValueDecompositionF(....) { .... if ((k < nct) & (s[k] != 0.0)) .... } ``` Очевидно, что даже если не будет выполнено первое условие, ошибочное использование оператора _&_ вместо _&&_ приведет к проверке второго условия, что, в свою очередь, повлечёт выход за пределы массива\. **Четвертое место: раз кавычка, два кавычка** На четвертом месте \- ошибка из [статьи](https://pvs-studio.ru/ru/blog/posts/csharp/0400/) о проверке проекта Xamarin\.Forms\. [V3038](https://pvs-studio.ru/ru/docs/warnings/v3038/) The first argument of 'Replace' function is equal to the second argument\. ICSharpCode\.Decompiler ReflectionDisassembler\.cs 349 ```cpp void WriteSecurityDeclarationArgument(CustomAttributeNamedArgument na) { .... output.Write("string('{0}')", NRefactory.CSharp .TextWriterTokenWriter .ConvertString( (string)na.Argument.Value).Replace("'", "\'")); .... } ``` В данном случае будет произведена замена кавычки на\.\.\. кавычку\. Не думаю, что это именно то, чего добивался разработчик\. А анализатор \- молодец\! **Третье место: ThreadStatic** Проект Mono, о проверке которого написана [статья](https://pvs-studio.ru/ru/blog/posts/csharp/0431/), оказался богат на интересные ошибки\. А одна из них \- действительно редкий гость\. [V3089](https://pvs-studio.ru/ru/docs/warnings/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 ```cpp static class Profiler { [ThreadStatic] private static Stopwatch timer = new Stopwatch(); .... } ``` Если кратко: выполняется некорректная инициализация поля, отмеченного атрибутом _ThreadStatic\._ В [документации](https://pvs-studio.ru/ru/docs/warnings/v3089/) к диагностике приведено подробное описание ситуации, а также даны советы, как можно избежать подобных ошибок\. Великолепный пример ошибки, которую не так просто найти и исправить обычными средствами\. **Второе место: Copy\-Paste, эталонно\!** Один из эталонных, на мой взгляд, примеров ошибки типа Copy\-Paste содержится в уже упомянутой ранее [статье](https://pvs-studio.ru/ru/blog/posts/csharp/0431/) о проверке проекта Mono\. [V3012](https://pvs-studio.ru/ru/docs/warnings/v3012/) The '?:' operator, regardless of its conditional expression, always returns one and the same value: Color\.FromArgb \(150, 179, 225\)\. ProfessionalColorTable\.cs 258 Вот краткий фрагмент кода, в котором была найдена ошибка: ```cpp button_pressed_highlight = use_system_colors ? Color.FromArgb (150, 179, 225) : Color.FromArgb (150, 179, 225); ``` Вы спросите: "А что же тут такого?" Стоило ли помещать такую очевидную ошибку на второе место хит\-парада? Дело в том, что приведенный фрагмент кода был дополнительно отформатирован для наглядности\. А теперь подготовьтесь и представьте, что перед вами стоит задача поиска подобной ошибки в коде без использования инструментальных средств\. Итак, слабонервных просьба удалиться, ниже следует скриншот, на котором содержится полный фрагмент кода с ошибкой \(для увеличения можно кликнуть по изображению\): ![0480_Top_10_CSharp_Bugs_2016_ru/image2.png](https://import.viva64.com/docx/blog/0480_Top_10_CSharp_Bugs_2016_ru/image2.png) Очевидно, что подобную ошибку может допустить любой программист вне зависимости от его квалификации\. А успешный поиск таких ошибок демонстрирует всю мощь статического анализа\. **Первое место: PVS\-Studio** Да, вам не показалось\. Здесь действительно написано "PVS\-Studio"\. И он на первом месте нашего хит\-парада\. Но не только потому, что это хороший статический анализатор\. А ещё и потому, что в нашей команде работают обычные люди, которые допускают обычные человеческие ошибки в коде\. Именно про это и была в своё время написана [статья](https://pvs-studio.ru/ru/blog/posts/csharp/0382/)\. Ошибка, а точнее сразу две, которые были обнаружены в коде PVS\-Studio при помощи PVS\-Studio\. [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'RowsCount \> 100000' is always false\. ProcessingEngine\.cs 559 [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'RowsCount \> 200000' is always false\. ProcessingEngine\.cs 561 ```cpp public void ProcessFiles(....) { .... int RowsCount = DynamicErrorListControl.Instance.Plog.NumberOfRows; if (RowsCount > 20000) DatatableUpdateInterval = 30000; //30s else if (RowsCount > 100000) DatatableUpdateInterval = 60000; //1min else if (RowsCount > 200000) DatatableUpdateInterval = 120000; //2min .... } ``` Результатом работы данного фрагмента кода \(при условии, что _RowsCount \> 20000_\) всегда будет значение _DatatableUpdateInterval_ равное 30000\. К счастью, мы уже проделали определенную работу в этом направлении\. Благодаря повсеместному использованию инкрементального анализа в нашей команде, появление статей о поиске ошибок в PVS\-Studio при помощи PVS\-Studio в будущем будет очень маловероятно\. **Заключение** Хочу заметить, что теперь любой из вас может составить свой хит\-парад найденных ошибок, воспользовавшись возможностью [бесплатного](https://pvs-studio.ru/ru/blog/posts/0457/) использования статического анализатора PVS\-Studio для проверки собственных проектов\. [Скачать](https://pvs-studio.ru/ru/pvs-studio/) и попробовать PVS\-Studio\. По вопросам приобретения коммерческой лицензии просим Вас [связаться](https://pvs-studio.ru/ru/about-feedback/) с нами в почте\. Вы также можете написать нам, чтобы получить временную лицензию для всестороннего изучения PVS\-Studio, если хотите снять [ограничения](https://pvs-studio.ru/ru/docs/manual/0009/) демонстрационной версии\.