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

Umbraco


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

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

О PVS-Studio 6.00


Проверка будет осуществлять с помощью Альфа-версии анализатора PVS-Studio 6.00. В этой версии произойдут два больших изменения:
  1. Будет поддержан анализ C# проектов.
  2. Анализатор перестанет поддерживать 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# программистов нам только предстоит, перечислю ряд важных моментов:
  1. Статический анализатор — это инструмент регулярного использования. Его смысл находить ошибки на самом раннем этапе. Нет смысла в разовых проверках проекта. Как правило так выявляются ошибки, не оказывающие существенного влияния на работу программы или расположенные в редко используемых участках кода. Причина в том, что настоящие ошибки всё это время исправлялись потом и кровью. Их находили программисты, часами отлаживая код, их находили тестеры и что ещё хуже — пользователи. Многие из этих ошибок можно было бы сразу исправить, если регулярно использовать статический анализатор кода. Рассматривайте PVS-Studio как расширение предупреждений C# компилятора. Вы ведь, надеюсь, смотрите список предупреждений, выданных компилятором, не раз в год? Более подробно всё это разобрано в статье: "Лев Толстой и статический анализ кода".
  2. В статьях мы упоминаем только о тех фрагментах кода, которые показались нам интересными. Как правило мы не пишем о коде, который анализатор совершенно честно посчитал подозрительным, но при этом видно, что настоящей ошибки нет. Мы называем этот «код с запахом». Когда вы используете PVS-Studio, этот код стоит проверить. Но рассказывать про такие места в статье неуместно.
  3. Этого пункта нет для C++, но он существует пока для C#. Мы реализовали ещё не так много диагностик, но быстро движемся. Дайте нашему C#-единорогу немного подрасти. Уж он вам тогда покажет!

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


Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. The First C# Project Analyzed.

Комментарии (26)


  1. QtRoS
    13.11.2015 15:14
    +1

    А демо-версия будет, как для C++?


    1. Andrey2008
      13.11.2015 15:16
      +2

      Всё будет, как и сейчас. Просто помимо C++ проектов, можно будет проверять и C# проекты. Никого деления не будет. Просто анализатор научится понимать +1 язык.
      Соответственно demo-версию так же можно будет скачать и попробовать на C# проекте.


  1. Evengard
    13.11.2015 15:17
    +2

    … а планируется ли поддержка mono? Понятно что сейчас не поддерживает. А в будущем?


    1. Andrey2008
      13.11.2015 15:19

      Пока не планируется. Пока не до этого.


  1. astudent
    13.11.2015 16:18
    +2

    А какие проблемы находит PVS-Studio из тех, которые не находит ReSharper? Из перечисленных кажется решарпер не предупредит только о N4. А учитывая, что он условно бесплатный и очень популярен, предупреждает об ошибках еще на этапе написания кода, вам будет трудно конкурировать с ним. Например, нам несколько раз предлагали купить Coverity, но каждый тестовый прогон проекта не выявлял существенных проблем, и мы отказывались. В любом случае это хорошо иметь несколько инструментов, подстегнет всех улучать свои продукты.


    1. a553
      13.11.2015 16:59

      У решарпера есть предупреждение «Some values of the enum are not processed inside switch». А вот N1 не поймает.


    1. Andrey2008
      13.11.2015 17:01
      +3

      Это инструменты разного класса. ReSharper это productivity tool. PVS-Studio это статический анализатор кода, специализирующийся на поиске дефектов. PVS-Studio будет находить больше ошибок и более качественно (если не сейчас, то в недалёком будущем). Ибо он специализирован для этого.

      То как Вы описали «пробу Coverity» это типовой неправильный способ применения статического анализатора. Суть статического анализа — регулярно применение. Разовые запуски находят как правило некритичные ошибки или ошибки в редко исполняемом коде. Настоящие ошибки к тому моменту исправлены потом и кровью. Эти усилия можно было бы существенно сократить, находя многие ошибки/опечатки сразу после написания кода. Очень прошу познакомиться со статьёй: www.viva64.com/ru/b/0105


  1. Andriyan
    13.11.2015 16:20

    Спасибо огромное! Давно ждали!


  1. SvyatoslavMC
    13.11.2015 16:22

    Новое диагностическое правило, которое находит ошибки в функциях форматирования.

    V3025 Incorrect format. A different number of actual arguments is expected while calling 'Format' function. Expected: 6. Present: 7. SliderPrevalueEditor.cs 221

    Далее приведён большой фрагмент кода из реального приложения, в котором пропустили вывод для аргумента под номером 2.

    Пример кода
    // add jquery window load event for toggling fields.
                var javascriptMethod = string.Format(
                    @"
    $('#{0}').click(function(){{
        var disable = !$(this).attr('checked');
        $('#{1},#{3}').attr('disabled', disable);
        $('#{6}').val(disable && !checkDecimals() ? 'Integer' : 'Nvarchar');
        if(!disable) disable = $('#{1}').val() != '';
        
    }});
    $('#{1}').change(function(){{
        var disable = $(this).val() != '';
        $('#{3}').attr('disabled', disable);
    }});
    $('#{4}').click(function(){{
        var disable = !$(this).attr('checked');
        $('#{5}').attr('disabled', disable);
    }});
    $('#{6}').change(function(){{
        var disable = $(this).val() == 'Integer';
        if (checkDecimals() && disable) {{
            $('#{6}').val('Nvarchar');
            alert('Please remove decimal points below if you wish to use the Integer datatype');  
        }}
        else {{
        $('#{0}').removeAttr('checked');
        $('#{1},#{3}').attr('disabled', disable);
        }}
    }});
    $('.slider-numeric').keydown(function(event) {{
        // Allow only backspace and delete
        if ( event.keyCode == 46 || event.keyCode == 8 || ($(this).hasClass('slider-decimal') && (event.keyCode == 110 || event.keyCode == 190))) {{
            // let it happen, don't do anything
        }} else {{
            // Ensure that it is a number and stop the keypress
            if ( (event.keyCode < 48 || event.keyCode > 57 ) && (event.keyCode < 96 || event.keyCode > 105 ) ) {{
                event.preventDefault();
            }}
        }}
    }});
    $('.slider-numeric').keyup(function(event) {{
        if ($('#{6}').val() != 'Nvarchar' && checkDecimals()) {{
            $('#{6}').val('Nvarchar');
        }}
    }});
    function checkDecimals() {{
        foundDecimals = false;
        $('.slider-numeric').each(function() {{
                if ($(this).val().indexOf('.') >= 0) {{
                    foundDecimals = true;
                    return false;
                }}  
            }});
        return foundDecimals;
    }}
    ",
                    this.EnableRange.ClientID,
                    this.RangeValue.ClientID,
                    this.Value.ClientID,
                    this.Value2.ClientID,
                    this.EnableStep.ClientID,
                    this.StepValue.ClientID,
                    this.DatabaseDataType.ClientID);
    


    1. mayorovp
      13.11.2015 17:24
      +4

      Убивать за такой код надо, а не ошибки в нем искать :)


    1. SvyatoslavMC
      13.11.2015 17:42

      Тоже из проекта Umbraco


    1. Andrey2008
      13.11.2015 17:45

      «Далее приведён большой фрагмент кода из реального приложения»… Ох. Перевожу с программистского языка на рекламный:
      Пока писалась и переводилась эта статья, анализатор научился находить новые ошибки. Например, в рассматриваемом проекте Umbraco, нашлось вот такое. Вот как быстро и славно растёт PVS-Studio!


  1. niq
    13.11.2015 16:29

    Отбой, про решарпер уже спросили


  1. leotsarev
    13.11.2015 17:01
    +3

    Дадите попробовать?


    1. Andrey2008
      13.11.2015 17:02

      Подождите немного. Пока рано.


  1. xoposhiy
    13.11.2015 17:50
    +1

    Мог бы еще подозрительный лишний вызов ToArray внутри string.Join поймать. Решарпер его тоже не ловит, кстати.

    Честно говоря, не понятно как вы от Resharper-а отстраиваться будете. Он уже очень много чего ловит. Глобально запустить проверку можно. Запустить проверку на билд-сервере без VS тоже уже можно. Общие слова про разные классы инструментов больше похожи на маркетологический булшит.


    1. Andrey2008
      13.11.2015 17:55

      А мы не будем отстраиваться, сравниваться и так далее относительно Resharper. Мы будем просто брать и находить в проектах ошибки. И будем это показывать. Ничего более. И я уверен, нас ждёт успех.

      Для С++ тоже много подобных инструментов, в том числе бесплатных. Но это не мешает нам продавать PVS-Studio.


      1. xoposhiy
        13.11.2015 18:29
        +2

        Как я понял, вы пока сами не понимаете как будете убеждать владельцев решарпера покупать PVS-Studio. ОК, подождем :)


        1. Andrey2008
          13.11.2015 19:44
          +5

          Мы будем убеждать владельцев решарпера, точно также как убеждаем владельцев статических анализаторов для C++ (Clang, Cppcheck, PC-Lint и так далее). Статьями мы будем соблазнять попробовать наш анализатор на их проекте. И если PVS-Studio находит интересные ошибки, несмотря на использование Clang/Cppcheck/PC-Lint/Resharper, мы можем заинтересовать этого человека.

          Возможно будут какие-то нюансы. Но пока я не вижу никакой разницы, по сравнению с завоеванием рынка C++.


          1. gorohoroh
            14.11.2015 00:55

            Ну конечно, будет находить. Потому что обнаружение ошибки вообще не гарантирует её исправление.


          1. lair
            14.11.2015 03:13
            +7

            Мы будем убеждать владельцев решарпера, точно также как убеждаем владельцев статических анализаторов для C++ (Clang, Cppcheck, PC-Lint и так далее).

            Понимаете ли, в чем дело. Когда вы «убеждаете» владельца другого статического анализатора, в результате он заменяет тот анализатор, которым он раньше пользовался, на ваш. Но вы не можете сделать то же самое с решарпером, потому что вы никогда не замените основную функциональность решарпера. И, значит, вам придется убеждать человека купить ваш продукт в дополнение к решарперу.


  1. sentyaev
    14.11.2015 00:12

    Инструмент хороший, но нужно писать тесты)


  1. skobkin
    14.11.2015 11:56
    +2

    А проверьте KeePass?


    1. Andrey2008
      14.11.2015 12:03
      +2

      Пометил себе.


  1. servekon
    15.11.2015 23:51

    Ну вот и у нашей конторки скоро появится повод скачать демо-версию. Ждем с нетерпением.


  1. focus
    16.11.2015 20:06
    +1

    Было бы здорово посмотреть и на результаты проверки ShareX в будущем.