Сейчас команда 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. В этой версии произойдут два больших изменения:
- Будет поддержан анализ 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. Отделяет код от пояснения, в чем ошибка.
Предупреждение 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#-единорогу немного подрасти. Уж он вам тогда покажет!
Спасибо всем за внимание и желаю всем безбажных программ.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. The First C# Project Analyzed.
