Первая статья про проверку C# проекта

Андрей Карпов
Статей: 328



Сейчас команда PVS-Studio активно занимается разработкой статического анализатора C# кода. Мы планируем выпустить первую версию анализатора к концу 2015 года. А пока моя задача - написать несколько статей, чтобы заранее заинтересовать C# программистов инструментом PVS-Studio. Сегодня мне выдали обновлённый инсталлятор. Теперь стало возможным установить PVS-Studio с поддержкой C# и даже что-то проверить. Я не замедлил этим воспользоваться и решил проверить первое, что подвернется под руку. Первым подвернулся проект Umbraco. Конечно, пока текущая версия анализатора умеет мало, но этого мне уже сейчас хватит для написания маленькой статьи.

Picture 2

Umbraco

Umbraco - это платформа системы управления контента с открытым кодом, использующаяся для публикации контента во всемирной сети. Она написана на языке C#. С момента выхода 4.5 вся система стала доступной под лицензией MIT.

Проект имеет средний размер. Однако часть, написанная на C#, не очень велика. Большая часть проекта написана на JavaScript. Всего в проекте около 3200 файлов с расширением ".cs", суммарным размером в 15 мегабайт. Количество строк С# кода: 400 KLOC.

О PVS-Studio 6.00

Проверка будет осуществлять с помощью Альфа-версии анализатора PVS-Studio 6.00. В этой версии произойдут два больших изменения:

  • Будет поддержан анализ C# проектов.
  • Анализатор перестанет поддерживать VS2005, VS2008. Небольшому количеству пользователей, работающих с VS2005/2008 мы предлагаем продолжить использовать версию 5.31 или более старшие версии 5.xx, если будут исправляться какие-то ошибки.

Ценовая политика останется прежней. Мы не делаем новый продукт, а расширяем возможности существующего. Мы просто поддержим ещё один язык программирования. Раньше можно было купить PVS-Studio и использовать его для проверки проектов, написанных на языках: C, C++, C++/CLI, C++/CX. Теперь дополнительно появляется возможность проверять C# проекты. На цену это никак не повлияет. Те, кто уже приобрел анализатор для C++, смогут заодно проверять и C# код.

Почему C#?

На конференциях я нередко говорил, что делать C# анализатор не очень интересно. Множество ошибок, которые присутствуют в C++, в языке C# просто невозможны. Это действительно так. Например, в C# нет таких функций как memset(), соответственно и нет массы проблем (см. примеры, связанные с memset(): V511, V512, V575, V579, V597, V598).

Однако постепенно я изменил своё мнение. Большое количество ошибок, обнаруживаемых PVS-Studio, связано не с какими-то особенностями языка программирования, а с невнимательностью программистов. Я имею ввиду опечатки и неудачное изменение кода после copy-paste. Именно в этом силён анализатор PVS-Studio для C++, и мы решили, что эти наработки можно применить и для C#.

Язык C# не защищает от того, чтобы перепутать имя переменной или от "эффекта последней строки", связанного с утратой внимания.

Ещё одним важным фактором, который подтолкнул решиться на создание C# анализатора, является появление Roslyn. Без него работа по созданию анализатора была бы слишком дорогой для нас.

Roslyn - это открытая платформа компиляции C# и Visual Basic. Roslyn выполняет два основных действия: строит синтаксическое дерево (парсинг) и компилирует его. Дополнительно позволяет анализировать исходный код, рекурсивно обходить его, работать с проектами Visual Studio, выполнять код на лету.

Что нашлось интересного?

Для C++ у меня есть любимая диагностика V501. Теперь есть её аналог и для C#: V3001. Начнем именно с этой диагностики.

Фрагмент кода N1

В коде есть свойство "фокальная точка":

[DataMember(Name = "focalPoint")]
public ImageCropFocalPoint FocalPoint { get; set; }

Данное свойство имеет тип 'ImageCropFocalPoint', определение которого приведено ниже:

public class ImageCropFocalPoint
{
  [DataMember(Name = "left")]
  public decimal Left { get; set; }

  [DataMember(Name = "top")]
  public decimal Top { get; set; }
}

Казалось бы, при работе с таким свойством невозможно ошибиться. Ан нет. В методе HasFocalPoint() допущена досадная опечатка:

public bool HasFocalPoint()
{
  return FocalPoint != null &&
   FocalPoint.Top != 0.5m && FocalPoint.Top != 0.5m;
}

Два раза проверяется 'Top', а про 'Left' забыли.

Соответствующее предупреждение PVS-Studio: V3001 There are identical sub-expressions 'FocalPoint.Top != 0.5m' to the left and to the right of the '&&' operator. ImageCropDataSet.cs 58

Фрагмент кода N2

protected virtual void OnBeforeNodeRender(ref XmlTree sender,
            ref XmlTreeNode node,
            EventArgs e)
{
  if (node != null && node != null)
  {
    if (BeforeNodeRender != null)
      BeforeNodeRender(ref sender, ref node, e);    
  }
}

Предупреждение PVS-Studio: V3001 There are identical sub-expressions 'node != null' to the left and to the right of the '&&' operator. BaseTree.cs 503

Дважды проверяется ссылка 'node'. Скорее всего хотели проверить ещё и ссылку 'sender'.

Фрагмент кода N3

public void Set (ExifTag key, string value)
{
  if (items.ContainsKey (key))
    items.Remove (key);
  if (key == ExifTag.WindowsTitle ||   // <=
      key == ExifTag.WindowsTitle ||   // <=
      key == ExifTag.WindowsComment ||
      key == ExifTag.WindowsAuthor ||
      key == ExifTag.WindowsKeywords ||
      key == ExifTag.WindowsSubject) {
    items.Add (key, new WindowsByteString (key, value));
  ....
}

Предупреждение PVS-Studio: V3001 There are identical sub-expressions 'key == ExifTag.WindowsTitle' to the left and to the right of the '||' operator. ExifPropertyCollection.cs 78

Ключ два раза сравнивается с константой 'ExifTag.WindowsTitle'. Мне сложно судить, насколько серьёзна эта ошибка. Возможно, одна из проверок просто лишняя, и её можно удалить. Но возможно, здесь должно быть сравнение с какой-то другой константой.

Фрагмент кода N4

Рассмотрим ещё один случай, где я вовсе не уверен в наличии ошибки. Однако этот код стоит лишний раз проверить.

Имеется перечисление с 4 именованными константами:

public enum DBTypes
{
  Integer,
  Date,
  Nvarchar,
  Ntext
}

А вот метод SetProperty() почему-то рассматривает только 3 варианта. Ещё раз повторюсь, я не говорю, что это ошибка. Однако анализатор предлагает обратить внимание на этот фрагмент кода и я с ним полностью согласен.

public static Content SetProperty(....)
{
  ....
  switch (((DefaultData)property.PropertyType.
    DataTypeDefinition.DataType.Data).DatabaseType)
  {
    case DBTypes.Ntext:
    case DBTypes.Nvarchar:
      property.Value = preValue.Id.ToString();
      break;

    case DBTypes.Integer:
      property.Value = preValue.Id;
      break;
  }
  ....
}

Предупреждение PVS-Studio: V3002 The switch statement does not cover all values of the 'DBTypes' enum: Date. ContentExtensions.cs 286

Фрагмент кода N5

public TinyMCE(IData Data, string Configuration)
{
  ....
  if (p.Alias.StartsWith("."))
    styles += p.Text + "=" + p.Alias;
  else
    styles += p.Text + "=" + p.Alias;
  ....
}

Предупреждение PVS-Studio: V3004 The 'then' statement is equivalent to the 'else' statement. TinyMCE.cs 170

Фрагмент кода N6, N7

В начале статьи я говорил, что C# не защищает от "эффекта последней строки". И вот мы добрались до соответствующего примера:

public void SavePassword(IMember member, string password)
{
  ....
  member.RawPasswordValue = result.RawPasswordValue;
  member.LastPasswordChangeDate = result.LastPasswordChangeDate;
  member.UpdateDate = member.UpdateDate;
}

Предупреждение PVS-Studio: V3005 The 'member.UpdateDate' variable is assigned to itself. MemberService.cs 114

Программист копировал члены класса из объекта 'result' в объект 'member'. Но в последний момент человек расслабился и скопировал член 'member.UpdateDate' сам в себя.

Дополнительно настораживает, что метод SavePassword() работает с паролями, а значит, требует повышенного к себе внимания.

Точно такой же фрагмент кода можно наблюдать в файле UserService.cs (см. строка 269). Скорее всего его просто туда скопировали, не проверив.

Фрагмент кода N8

private bool ConvertPropertyValueByDataType(....)
{
  if (string.IsNullOrEmpty(string.Format("{0}", result)))
  {
    result = false;
    return true;
  }
  ....
    return true;
  ....
    return true;
  ....
    return true;
  ....
    return true;
  ....
  ....
  return true;
}

Предупреждение PVS-Studio: V3009 It's odd that this method always returns one and the same value of 'true'. DynamicNode.cs 695

В методе используется много операторов 'if' и много операторов 'return'. Настораживает то, что все операторы 'return' возвращают значение 'true'. Нет-ли в этом ошибки? Быть может всё-таки где-то надо было вернуть 'false'?

Фрагмент кода N9

Предлагаю читателям проверить внимательность и найти ошибку в следующем фрагменте кода. Для этого изучите метод, но не читайте текст ниже. Чтобы не сделать это случайно, я вставил разделитель (единорога :).

public static string GetTreePathFromFilePath(string filePath)
{
  List<string> treePath = new List<string>();
  treePath.Add("-1");
  treePath.Add("init");
  string[] pathPaths = filePath.Split('/');
  pathPaths.Reverse();
  for (int p = 0; p < pathPaths.Length; p++)
  {
    treePath.Add(
      string.Join("/", pathPaths.Take(p + 1).ToArray()));
  }
  string sPath = string.Join(",", treePath.ToArray());
  return sPath;
}
Рисунок 1. Отделяет код от пояснения, в чем ошибка.

Рисунок 1. Отделяет код от пояснения, в чем ошибка.

Предупреждение PVS-Studio: V3010 The return value of function 'Reverse' is required to be utilized. DeepLink.cs 19

Вызывая метод Reverse(), программист планировал изменить массив 'pathPaths'. Возможно его сбило с толку то, что такая операция совершенно корректна, если речь идёт, например, о списках (List<T>.Reverse). Однако применительно к массивам, метод Reverse() не меняет исходный массив. Для массивов этот метод реализуется за счёт метода-расширения Reverse() класса 'Enumerable'. Этот метод не производит перестановку на месте, а возвращает изменённую коллекцию.

Правильно будет написать:

string[] pathPaths = filePath.Split('/');
pathPaths = pathPaths.Reverse().ToArray();

Или даже так:

string[] pathPaths = filePath.Split('/').Reverse().ToArray();

Фрагмент кода N10

Анализатор PVS-Studio выдал несколько предупреждений V3013, о подозрительном совпадении тел некоторых методов. На мой взгляд, все эти предупреждения ложные. Как мне кажется, внимания заслуживает только одно из этих предупреждений:

public void GetAbsolutePathDecoded(string input, string expected)
{
    var source = new Uri(input, UriKind.RelativeOrAbsolute);
    var output = source.GetSafeAbsolutePathDecoded();
    Assert.AreEqual(expected, output);
}
public void GetSafeAbsolutePathDecoded(string input, string expected)
{
    var source = new Uri(input, UriKind.RelativeOrAbsolute);
    var output = source.GetSafeAbsolutePathDecoded();
    Assert.AreEqual(expected, output);
}

Предупреждение PVS-Studio: V3013 It is odd that the body of 'GetAbsolutePathDecoded' function is fully equivalent to the body of 'GetSafeAbsolutePathDecoded' function. UriExtensionsTests.cs 141

Возможно внутри метода GetAbsolutePathDecoded(), нужно использовать не

source.GetSafeAbsolutePathDecoded()

а

source. GetAbsolutePathDecoded()

Я не уверен, что прав, но это место заслуживает проверки.

Ответы на вопросы

Статья рассчитана на новую аудиторию. Поэтому я заранее предвижу ряд вопросов, которые могут возникнуть. Попробую заранее ответить на часть из них.

Вы уведомили разработчиков проекта о найденных дефектах?

Да, мы всегда стараемся это сделать.

Вы используете PVS-Studio для проверки кода самого PVS-Studio?

Да.

PVS-Studio поддерживает Mono?

Нет.

Более подробно ответы на эти и другие вопросы даны в заметке: "Ответы на вопросы читателей статей про PVS-Studio".

Заключение

В проекте нашлось не так много ошибок. Наши читатели из C++ аудитории уже знают, почему так происходит. Но поскольку покорять и соблазнять C# программистов нам только предстоит, перечислю ряд важных моментов:

  • Статический анализатор — это инструмент регулярного использования. Его смысл находить ошибки на самом раннем этапе. Нет смысла в разовых проверках проекта. Как правило так выявляются ошибки, не оказывающие существенного влияния на работу программы или расположенные в редко используемых участках кода. Причина в том, что настоящие ошибки всё это время исправлялись потом и кровью. Их находили программисты, часами отлаживая код, их находили тестеры и что ещё хуже - пользователи. Многие из этих ошибок можно было бы сразу исправить, если регулярно использовать статический анализатор кода. Рассматривайте PVS-Studio как расширение предупреждений C# компилятора. Вы ведь, надеюсь, смотрите список предупреждений, выданных компилятором, не раз в год? Более подробно всё это разобрано в статье: "Лев Толстой и статический анализ кода".
  • В статьях мы упоминаем только о тех фрагментах кода, которые показались нам интересными. Как правило мы не пишем о коде, который анализатор совершенно честно посчитал подозрительным, но при этом видно, что настоящей ошибки нет. Мы называем этот "код с запахом". Когда вы используете PVS-Studio, этот код стоит проверить. Но рассказывать про такие места в статье неуместно.
  • Этого пункта нет для C++, но он существует пока для C#. Мы реализовали ещё не так много диагностик, но быстро движемся. Дайте нашему C#-единорогу немного подрасти. Уж он вам тогда покажет!

Спасибо всем за внимание и желаю всем безбажных программ.



Используйте PVS-Studio для поиска ошибок в C, C++ и C# коде

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

goto PVS-Studio;

Андрей Карпов
Статей: 328


А ты совершаешь ошибки в коде?

Проверь с помощью
PVS-Studio

Статический анализ
кода для C, C++ и C#

goto PVS-Studio;
На нашем сайте мы используем cookie для сбора информации технического характера.
Если вы не согласны, пожалуйста, покиньте сайт. Подробнее →
Больше не показывать