- PVSM.RU - https://www.pvsm.ru -

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

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

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

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

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

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

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

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

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

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

V3056 [7] 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 также была написана статья [8]. Пример интересной, на мой взгляд, ошибки из этой статьи.

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

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

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

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

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

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

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

V3093 [11] 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))
  ....
}

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

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

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

V3038 [13] 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("'", "'")); 
  ....
}

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

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

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

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

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

V3089 [15] 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. В документации [16] к диагностике приведено подробное описание ситуации, а также даны советы, как можно избежать подобных ошибок. Великолепный пример ошибки, которую не так просто найти и исправить обычными средствами.

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

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

V3012 [17] 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);

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

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

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

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

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

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

V3022 [19] 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.

К счастью, мы уже проделали определенную работу в этом направлении.

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

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

Заключение

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

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

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

Автор: PVS-Studio

Источник [24]


Сайт-источник PVSM.RU: https://www.pvsm.ru

Путь до страницы источника: https://www.pvsm.ru/c-2/246597

Ссылки в тексте:

[1] статье: http://www.viva64.com/ru/b/0456/

[2] здесь: http://www.viva64.com/ru/inspections/

[3] V3118: http://www.viva64.com/ru/w/V3118/

[4] статье: http://www.viva64.com/ru/b/0440/

[5] V3022: http://www.viva64.com/ru/w/V3022/

[6] статье: http://www.viva64.com/ru/b/0412/

[7] V3056: http://www.viva64.com/ru/d/0453/

[8] статья: http://www.viva64.com/ru/b/0461/

[9] Статья: http://www.viva64.com/ru/b/0410/

[10] V3015: https://www.viva64.com/ru/w/V3015/

[11] V3093: http://www.viva64.com/ru/d/0494/

[12] статьи: http://www.viva64.com/ru/b/0400/

[13] V3038: http://www.viva64.com/ru/d/0423/

[14] статья: http://www.viva64.com/ru/b/0431/

[15] V3089: http://www.viva64.com/ru/d/0496/

[16] документации: http://www.viva64.com/ru/w/V3089/

[17] V3012: http://www.viva64.com/ru/d/0383/

[18] статья: http://www.viva64.com/ru/b/0382/

[19] V3022: http://www.viva64.com/ru/d/0391/

[20] бесплатного: http://www.viva64.com/ru/b/0457/

[21] http://www.viva64.com/ru/pvs-studio/: http://www.viva64.com/ru/pvs-studio/

[22] связаться: http://www.viva64.com/ru/about-feedback/

[23] ограничения: http://www.viva64.com/ru/m/0009/

[24] Источник: https://habrahabr.ru/post/322662/?utm_source=habrahabr&utm_medium=rss&utm_campaign=best