Топ 10 ошибок в проектах C# за 2016 год

Для оценки качества работы нашего анализатора, а также с целью популяризации методологии статического анализа, мы регулярно проверяем на наличие ошибок проекты с открытым исходным кодом и пишем про это статьи. Не стал исключением и прошедший 2016 год, который примечателен ещё и тем, что это было время своеобразного "взросления" C# анализатора. PVS-Studio получил значительное количество новых C# диагностик, улучшенный механизм работы с виртуальными значениями (symbolic execution) и многое другое. По результатам проделанной нашей командой работы, я составил своеобразный хит-парад наиболее интересных ошибок, обнаруженных в проектах С# в 2016 году.

Десятое место: когда в минуте не всегда 60 секунд

Начну хит-парад с ошибки, обнаруженной при проверке проекта Orchard CMS. Описание ошибки можно найти в статье. Вообще же, со всеми статьями про проверку проектов можно ознакомиться здесь.

V3118 Seconds component of TimeSpan is used, which does not represent full time interval. Possibly 'TotalSeconds' value was intended instead. AssetUploader.cs 182

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 секунды. Невероятно, но даже опытные разработчики допускают подобные ошибки.

Девятое место: выражение всегда истинно

Следующая ошибка приведена в статье о проверке проекта GitExtensions.

V3022 Expression 'string.IsNullOrEmpty(rev1) || string.IsNullOrEmpty(rev2)' is always true. GitUI FormFormatPatch.cs 155

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. Вероятно, ему вовсе тут не место. Невнимательность при рефакторинге или банальная усталость программиста, и вот мы получаем кардинальное изменение логики работы программы, что приводит к непредсказуемому поведению. Хорошо, что статический анализатор никогда не устаёт.

Восьмое место: возможная опечатка

В статье о проверке исходного кода FlashDevelop приведена интересная ошибка, связанная с опечаткой.

V3056 Consider reviewing the correctness of 'a1' item's usage. LzmaEncoder.cs 225

public void SetPrices(....)
{
    UInt32 a0 = _choice.GetPrice0();
    UInt32 a1 = _choice.GetPrice1();
    UInt32 b0 = a1 + _choice2.GetPrice0();  // <=
    UInt32 b1 = a1 + _choice2.GetPrice1();
    ....
}

Я согласен с анализатором, а также с автором статьи. Вместо переменной a1 в отмеченной строке напрашивается использование а0. В любом случае, не мешало бы дать переменным более понятные имена.

Седьмое место: логическая ошибка

По мотивам повторной проверки проекта Umbraco также была написана статья. Пример интересной, на мой взгляд, ошибки из этой статьи.

V3022 Expression 'name != "Min" || name != "Max"' is always true. Probably the '&&' operator should be used here. DynamicPublishedContentList.cs 415

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. И всё из-за ошибочного использования в условии оператора || вместо &&.

Шестое место: ошибочное условие выхода из цикла

Статья о проверке проекта Accord.Net содержит описание нескольких интересных ошибок. Я выбрал две, одна из которых вновь связана с опечаткой.

V3015 It is likely that a wrong variable is being compared inside the 'for' operator. Consider reviewing 'i' Accord.Audio SampleConverter.cs 611

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 для счётчиков - это, своего рода, классика жанра. К сожалению, эти переменные очень схожи по написанию и разработчики часто допускают опечатки в подобном коде. Не думаю, что в данном случае стоит рекомендовать использование более осмысленных имен. Все равно так делать никто не будет :). Поэтому дам другую рекомендацию: используйте статические анализаторы!

Пятое место: использование битового оператора вместо логического

Еще одна интересная и достаточно распространенная ошибка из статьи о проверке проекта Accord.Net.

V3093 The '&' operator evaluates both operands. Perhaps a short-circuit '&&' operator should be used instead. Accord.Math JaggedSingularValueDecompositionF.cs 461

public JaggedSingularValueDecompositionF(....)
{
  ....
  if ((k < nct) & (s[k] != 0.0))
  ....
}

Очевидно, что даже если не будет выполнено первое условие, ошибочное использование оператора & вместо && приведет к проверке второго условия, что, в свою очередь, повлечёт выход за пределы массива.

Четвертое место: раз кавычка, два кавычка

На четвертом месте - ошибка из статьи о проверке проекта Xamarin.Forms.

V3038 The first argument of 'Replace' function is equal to the second argument. ICSharpCode.Decompiler ReflectionDisassembler.cs 349

void WriteSecurityDeclarationArgument(CustomAttributeNamedArgument na)
{
  ....
  output.Write("string('{0}')",
    NRefactory.CSharp
              .TextWriterTokenWriter
              .ConvertString(
                (string)na.Argument.Value).Replace("'", "\'")); 
  ....
}

В данном случае будет произведена замена кавычки на... кавычку. Не думаю, что это именно то, чего добивался разработчик.

А анализатор - молодец!

Третье место: ThreadStatic

Проект Mono, о проверке которого написана статья, оказался богат на интересные ошибки. А одна из них - действительно редкий гость.

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

static class Profiler
{
  [ThreadStatic]
  private static Stopwatch timer = new Stopwatch();
  ....
}

Если кратко: выполняется некорректная инициализация поля, отмеченного атрибутом ThreadStatic. В документации к диагностике приведено подробное описание ситуации, а также даны советы, как можно избежать подобных ошибок. Великолепный пример ошибки, которую не так просто найти и исправить обычными средствами.

Второе место: Copy-Paste, эталонно!

Один из эталонных, на мой взгляд, примеров ошибки типа Copy-Paste содержится в уже упомянутой ранее статье о проверке проекта Mono.

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);

Вы спросите: "А что же тут такого?" Стоило ли помещать такую очевидную ошибку на второе место хит-парада? Дело в том, что приведенный фрагмент кода был дополнительно отформатирован для наглядности. А теперь подготовьтесь и представьте, что перед вами стоит задача поиска подобной ошибки в коде без использования инструментальных средств. Итак, слабонервных просьба удалиться, ниже следует скриншот, на котором содержится полный фрагмент кода с ошибкой (для увеличения можно кликнуть по изображению):

Очевидно, что подобную ошибку может допустить любой программист вне зависимости от его квалификации. А успешный поиск таких ошибок демонстрирует всю мощь статического анализа.

Первое место: PVS-Studio

Да, вам не показалось. Здесь действительно написано "PVS-Studio". И он на первом месте нашего хит-парада. Но не только потому, что это хороший статический анализатор. А ещё и потому, что в нашей команде работают обычные люди, которые допускают обычные человеческие ошибки в коде. Именно про это и была в своё время написана статья. Ошибка, а точнее сразу две, которые были обнаружены в коде PVS-Studio при помощи PVS-Studio.

V3022 Expression 'RowsCount > 100000' is always false. ProcessingEngine.cs 559

V3022 Expression 'RowsCount > 200000' is always false. ProcessingEngine.cs 561

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 в будущем будет очень маловероятно.

Заключение

Хочу заметить, что теперь любой из вас может составить свой хит-парад найденных ошибок, воспользовавшись возможностью бесплатного использования статического анализатора PVS-Studio для проверки собственных проектов.

Скачать и попробовать PVS-Studio: http://www.viva64.com/ru/pvs-studio/

По вопросам приобретения коммерческой лицензии просим Вас связаться с нами в почте. Вы также можете написать нам, чтобы получить временную лицензию для всестороннего изучения PVS-Studio, если хотите снять ограничения демонстрационной версии.