Недавно я проводил сравнение C# анализаторов PVS-Studio и SonarQube на базе кода проекта PascalABC.NET. Исследование оказалось довольно интересным, поэтому я решил продолжить работу в данном направлении. В этот раз я сравниваю C# анализатор PVS-Studio со статическим анализатором, встроенным в Visual Studio. На мой взгляд, это весьма достойный соперник. Несмотря на то, что анализатор из комплекта Visual Studio, в первую очередь, рассчитан не на поиск ошибок, а на улучшение качества кода, это вовсе не означает, что с его помощью нельзя найти реальные ошибки, хотя это и трудно. Давайте разберемся, какие же особенности работы анализаторов будут выявлены в ходе нашего исследования на этот раз. Вперёд!
Сначала - небольшая секция FAQ для прояснения ряда моментов.
Q: Почему CruiseControl.NET? Что это за проект?
A: CruiseControl.NET - это сервер автоматической непрерывной интеграции, реализованный с использованием .NET Framework. Исходный код CruiseControl.NET доступен на GitHub. Проект уже некоторое время не развивается и не поддерживается, хотя, в недалёком прошлом пользовался определённой популярностью. Это не помешает применить его с целью сравнения анализаторов, скорее, даже наоборот, внесёт некий элемент стабильности в исследование. Можно быть уверенным, что никто не улучшал код, используя последние версии PVS-Studio или анализатора, встроенного в Visual Studio. Дополнительным плюсом является небольшой размер CruiseControl.NET: проект содержит около 256 тысяч строк кода.
Q: Вы использовали Visual Studio 2017? Хотелось бы ознакомиться с возможностями последних версий инструментов анализа.
A: Для анализа обоих инструментов была использована среда Visual Studio 2017 Community.
Q: А что с настройками анализаторов? Наверное, всё было "специально настроено", и поэтому PVS-Studio оказался лучше?
A: Для обоих анализаторов были использованы настройки "по умолчанию". Для чистоты эксперимента, установка и исследование проводились на "чистой" машине с Windows 10.
Q: Ну хорошо. Но ведь, наверняка, вы немного подтасовали результаты или произвели расчёты не совсем корректно? Например, для PVS-Studio вы могли не учитывать уровень достоверности предупреждений "Low", взяв только "High" и "Medium". Тогда PVS-Studio будет иметь преимущество над анализатором, встроенным в Visual Studio, так как последний не имеет аналогичной настройки.
A: При анализе результатов были учтены абсолютно все уровни предупреждений и включены все доступные типы диагностик.
Q: А как насчёт выбора файлов для анализа? Вы что-то добавляли в исключения, например, Unit-тесты?
A: Для обоих анализаторов производилась проверка всего решения, без исключений. При этом замечу, что CruiseControl.NET содержит проект с именем "UnitTests". Для данного проекта было выдано довольно много предупреждений, но все они не учитывались при поиске реальных ошибок, хотя и фигурируют в общем показателе числа выданных предупреждений.
Q: Реальные ошибки? Что это за термин?
A: В нашем понимании - это ошибки, критичные для выполнения, которые с большой долей вероятности приведут к выбросу исключения, неправильному поведению программы, выдаче некорректных результатов. Ошибки, которые нужно исправлять здесь и сейчас. Это не рекомендации по улучшению дизайна и не мелкие недочеты типа дублирования кода, которые не влияют на конечный результат. Пример реальной ошибки в коде CruiseControl.NET:
public override string Generate(IIntegrationResult integrationResult)
{
....
IntegrationSummary lastIntegration =
integrationResult.LastIntegration; // <=
if (integrationResult == null || ....) // <=
{
....
}
....
}Многие анализаторы выдадут для приведённого фрагмента кода предупреждение о том, что переменная integrationResult используется без предварительной проверки на равенство null. Это правильно, но обычно приводит к большому числу ложных срабатываний, среди которых очень сложно найти настоящую ошибку. Наш подход заключается в проведении дополнительного анализа, что повышает вероятность обнаружения реальной ошибки. В приведенном выше фрагменте, после использования переменной, производится её проверка на равенство null. Т.е. программист в данном случае предполагает, что значение переменной может быть null после передачи в метод, и делает проверку. Именно такую ситуацию мы будем считать ошибкой. Если бы метод не содержал проверку integrationResult на равенство null, то, по нашему мнению, это было бы ложным срабатыванием:
public override string Generate(IIntegrationResult integrationResult)
{
....
IntegrationSummary lastIntegration =
integrationResult.LastIntegration;
....
}Анализатор PVS-Studio не выдаст предупреждение на данный фрагмент кода, в то время как ряд анализаторов сделают это. И, как результат, такие сообщения будут проигнорированы или отключены программистом. Много предупреждений - не значит хорошо.
Q: Допустим, вы действительно все проделали корректно. Но почему я должен принимать все это на веру? Как я могу повторить ваше исследование?
A: Нет ничего проще. Анализатор, встроенный в Visual Studio, является бесплатным. Вам достаточно установить бесплатную версию Visual Studio 2017 Community. Для анализа CruiseControl.NET при помощи PVS-Studio, вам будет достаточно загрузить его и использовать в демонстрационном режиме. Да, провести полный анализ вам помешают ограничения, но вы можете написать нам, и мы вышлем временный лицензионный ключ.
Проверка кода проекта при помощи встроенного в Visual Studio анализатора заняла всего пару минут. Сразу после этого результаты имеют следующий вид (никакие фильтры не включены):
Анализатор выдал 10773 предупреждения. Да, искать ошибки здесь будет не так-то просто. Для начала я исключу из данного списка предупреждения по проекту "UnitTests" при помощи фильтра:
Хорошо. Почти половина предупреждений была получена для тестов, что неудивительно. Но более 5 тысяч оставшихся сообщений - тоже не мало. Эти предупреждения относятся к следующим группам:
Microsoft.Design: CA10XX (диагностик: 40, предупреждений: 1637)
Microsoft.Globalization: CA13XX (диагностик: 7, предупреждений: 1406)
Microsoft.Interoperability: CA14XX (диагностик: 2, предупреждений: 10)
Microsoft.Maintainability: CA15XX (диагностик: 3, предупреждений: 74)
Microsoft.Mobility: CA16XX (диагностик: 1, предупреждений: 1)
Microsoft.Naming: CA17XX (диагностик: 17, предупреждений: 1071)
Microsoft.Performance: CA18XX (диагностик: 15, предупреждений: 489)
Microsoft.Portability: CA19XX (диагностик: 1, предупреждений: 4)
Microsoft.Reliability: CA20XX (диагностик: 4, предупреждений: 158)
Microsoft.Globalization, Microsoft.Security: CA21XX (диагностик: 5,
предупреждений: 48)
Microsoft.Usage: CA22XX (диагностик: 18, предупреждений: 440)Также было выдано некоторое количество предупреждений компилятора. Видимо, не остается другого выбора, кроме как изучить описание каждой из полученных диагностик анализатора, а уже затем проводить изучение предупреждений в случае необходимости.
Скажу сразу, что я проделал эту работу и мне не удалось найти почти ничего, что помогло бы найти ошибки среди более чем пяти тысяч выданных анализатором предупреждений. Как правило, все сводится к рекомендациям по улучшению дизайна и оптимизации кода. Ввиду большого числа диагностических правил, я не буду приводить их полный список с описаниями. При желании вы можете досконально изучить этот список, самостоятельно проверив проект CruiseControl.NET при помощи анализатора, встроенного в Visual Studio. Подробное описание диагностик доступно в MSDN.
Я изучил существенную часть предупреждений, но не все. До конца многие группы сообщений просматривать смысла не было, так как все они были однотипны и речь явно шла не об ошибках. Чтобы не быть голословным, приведу по одному примеру для каждой группы.
Microsoft.Design
CA1002 Change 'List<NameValuePair>' in 'CruiseServerClient.ForceBuild(string, List<NameValuePair>)' to use Collection<T>, ReadOnlyCollection<T> or KeyedCollection<K,V> CruiseServerClient.cs 118
public override void ForceBuild(...., List<NameValuePair> parameters)
{
....
}Рекомендация о том, что следует использовать универсальную коллекцию (например, Collection), вместо List для параметра parameters метода.
Microsoft.Globalization
CA1300 Change 'AddProjects.RetrieveListOfProjects(BuildServer)' to call the MessageBox.Show overload that specifies MessageBoxOptions, and make sure to set MessageBoxOptions.RightAlign and MessageBoxOptions.RtlReading if RightToLeft is set to RightToLeft.Yes on the parent control. CCTrayLib AddProjects.cs 86
private void RetrieveListOfProjects(....)
{
....
MessageBox.Show(this, "Unable to connect to server " +
server.DisplayName + ": " + ex.Message, "Error");
....
}Рекомендация о необходимости использования перегрузки метода MessageBox.Show(), которая принимает аргумент MessageBoxOptions. Это необходимо для лучшей поддержки многоязычного интерфейса и языков, в которых используется порядок чтения справа налево.
Microsoft.Interoperability
CA1401 Change the accessibility of P/Invoke 'NativeMethods.SetForegroundWindow(IntPtr)' so that it is no longer visible from outside its assembly. CCTrayLib NativeMethods.cs 12
[DllImport("user32.dll")]
[return: MarshalAs(UnmanagedType.Bool)]
public static extern bool SetForegroundWindow(IntPtr handle);Рекомендация о том, что не следует указывать уровень доступа public для методов с атрибутом DllImportAttribute.
Microsoft.Maintainability
CA1500 'errorMessages', a variable declared in 'Response.ConcatenateErrors()', has the same name as an instance field on the type. Change the name of one of these items. Remote Response.cs 152
private List<ErrorMessage> errorMessages;
....
public virtual string ConcatenateErrors()
{
List<string> errorMessages = new List<string>();
....
}Предупреждение о том, что локальная переменная имеет такое же имя, как поле класса.
Microsoft.Mobility
CA1601 Modify the call to 'Timer.Timer(double)' in method FileChangedWatcher.FileChangedWatcher(params string[])' to set the timer interval to a value that's greater than or equal to one second. core FileChangedWatcher.cs 33
public FileChangedWatcher(....)
{
....
timer = new Timer(500);
....
}Предупреждение о том, что для таймера установлен интервал меньше одной секунды.
Microsoft.Naming
CA1702 In member 'Alienbrain.CreateGetProcess(string)', the discrete term 'filename' in parameter name 'filename' should be expressed as a compound word, 'fileName'. core Alienbrain.cs 378
public ProcessInfo CreateGetProcess(string filename)
{
....
}Предупреждение о необходимости использования Camel Case для именования составных имен переменных.
Microsoft.Performance
CA1800 'action', a variable, is cast to type 'AdministerAction' multiple times in method 'AdministerPlugin.NamedActions.get()'. Cache the result of the 'as' operator or direct cast in order to eliminate the redundant isint instruction. WebDashboard AdministerPlugin.cs 79
public INamedAction[] NamedActions
{
get
{
....
if (action is AdministerAction)
{
(action as AdministerAction).Password = password;
}
....
}
....
}Предупреждение о необходимости оптимизации многократно повторяющегося приведения типа.
Microsoft.Portability
CA1901 As it is declared in your code, parameter 'fdwSound' of P/Invoke 'Audio.PlaySound(byte[], short, long)' will be 8 bytes wide on 32-bit platforms. This is not correct, as the actual native declaration of this API indicates it should be 4 bytes wide on 32-bit platforms. Consult the MSDN Platform SDK documentation for help determining what data type should be used instead of 'long'. CCTrayLib Audio.cs 135
[DllImport ("winmm.dll")]
private static extern int PlaySound (byte[] pszSound, Int16 hMod,
long fdwSound);Предупреждение о том, что в импортируемом методе для параметра fdwSound использован непереносимый тип. Необходимо использовать IntPtr или UIntPtr.
Microsoft.Reliability
CA2000 In method 'About.famfamfamLink_LinkClicked(object, LinkLabelLinkClickedEventArgs)', call System.IDisposable.Dispose on object 'urlLink' before all references to it are out of scope. CCTrayLib About.cs 71
private void famfamfamLink_LinkClicked(....)
{
Process urlLink = new Process();
urlLink.StartInfo = new ProcessStartInfo(....);
urlLink.Start();
}Предупреждение о необходимости освобождения IDisposable-объекта urlLink до того, как он окажется вне области видимости. Можно, например, использовать using.
Microsoft.Globalization, Microsoft.Security
CA2101 To reduce security risk, marshal parameter 'lpszDomain' as Unicode, by setting DllImport.CharSet to CharSet.Unicode, or by explicitly marshaling the parameter as UnmanagedType.LPWStr. If you need to marshal this string as ANSI or system-dependent, specify MarshalAs explicitly, and set BestFitMapping=false; for added security, also set ThrowOnUnmappableChar=true. core Impersonation.cs 100
[DllImport("advapi32.dll", SetLastError = true)]
private static extern bool LogonUser(
string lpszUsername,
string lpszDomain,
string lpszPassword,
int dwLogonType,
int dwLogonProvider,
ref IntPtr phToken);Предупреждение о том, что не указан тип маршалинга для строковых аргументов, например, путем указания атрибутов следующим образом:
[DllImport("advapi32.dll", SetLastError = true,
CharSet = CharSet.Unicode)]Microsoft.Usage
CA2201 'CruiseServerClientFactory.GenerateClient(string, ClientStartUpSettings)' creates an exception of type 'ApplicationException', an exception type that is not sufficiently specific and should never be raised by user code. If this exception instance might be thrown, use a different exception type. Remote CruiseServerClientFactory.cs 97
public CruiseServerClientBase GenerateClient(....)
{
....
throw new ApplicationException("Unknown transport protocol");
....
}Предупреждение о выбросе исключения слишком общего типа. В данном случае рекомендуется выброс исключения более конкретного типа, не зарезервированного в среде выполнения.
Итог
В результате проделанной работы я пришел к выводу, что поиск реальных ошибок, в данном случае, имеет смысл производить только для сообщений с кодом CA1062 из группы Microsoft.Design. Это предупреждения о ситуации, когда параметр метода является ссылочным типом, при этом перед его использованием не производится проверка на равенство null. После применения фильтра для таких сообщений, мы получим следующее:
733 - всё ещё много. Но, согласитесь, это уже что-то. И если изучить найденные фрагменты кода, то они действительно потенциально небезопасны. Например, в файле ItemStatus.cs:
public void AddChild(ItemStatus child)
{
child.parent = this;
childItems.Add(child);
}Ссылка child на экземпляр класса ItemStatus никак не проверяется перед использованием. Да, это опасно. Но, к сожалению, это нельзя назвать ошибкой. Вероятно, проверки могут содержаться в вызывающем коде, хотя это и неправильно. Тем более, метод объявлен как public. Конечно, автору кода следует принять меры и как-то отреагировать на все эти предупреждения, но напомню, что их 733. Скорее всего, программист ничего не будет делать, так как "все и так работает". Именно в этом заключена опасность выдачи большого количества сообщений на всё мало-мальски подозрительное. По этой причине ранее я приводил пример реальной ошибки, такой, на которую стоит обратить внимание разработчика. Срабатывания, подобные этому, вполне можно считать в своём большинстве ложными. И это действительно так.
Потратив время еще, я выделил среди выданных 733 предупреждений - 5, которые можно интерпретировать как ошибки. Вот пример одной из них (файл BuildGraph.cs):
public override bool Equals(object obj)
{
if (obj.GetType() != this.GetType() )
return false;
....
}Переменная obj не проверяется на равенство null перед использованием. Так как речь идет о перегруженном методе Equals - мы имеем дело с ошибкой. Метод Equals обязан корректно обрабатывать нулевые ссылки. Возможно, в проекте CruiseControl.NET такие ситуации никогда не возникают, но код метода все равно ошибочен и его следует исправить.
Дотошный читатель может возразить, что я мог пропустить какую-то ошибку, не изучив внимательно каждое использование методов. Возможно. Но на практике, все равно так внимательно изучать каждое предупреждение никто не станет, уж очень велик процент ложных срабатываний.
Замечу, что несмотря на то, что мне удалось найти реальные ошибки в коде CruiseControl.NET при помощи анализатора, встроенного в Visual Studio, сам процесс занял у меня около 8 часов (весь рабочий день) и потребовал повышенного внимания и сосредоточенности. Возможно, если бы авторы проекта использовали статический анализ регулярно, общая картина была бы более радужной.
Проверка кода проекта с помощью PVS-Studio на моей машине заняла ровно минуту. Сразу после этого результаты имеют вид (никакие фильтры не включены):
Анализатор выдал 198 предупреждений. И них 45 было получено для проекта "UnitTests", а ещё 32 предупреждения имеют низкий приоритет (среди них сложно найти реальные ошибки). Итого - 121 сообщение для анализа, на который я потратил 30 минут. В результате было выявлено 19 ошибок:
Вот пример одной из них:
V3003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 120, 125. CCTrayLib CCTrayProject.cs 120
public override bool Equals(object obj)
{
....
if ((buildServer != null) &&
(objToCompare.buildServer != null))
{
// If both instances have a build server then compare the build
// server settings
isSame = string.Equals(buildServer.Url,
objToCompare.buildServer.Url);
}
else if ((buildServer != null) &&
(objToCompare.buildServer != null))
{
// If neither instance has a build server then they are the same
isSame = true;
}
....
}Оба блока if содержат одинаковое условие. Допущена серьёзная ошибка, влияющая на логику работы программы и приводящая к неожиданному результату.
Думаю, здесь мне больше нечего добавить. PVS-Studio быстро и качественно сделал свою работу по поиску реальных ошибок. Но ведь для этого он и существует!
Представлю полученные результаты в виде таблицы:
Конечно, очевидный перевес наблюдается на стороне PVS-Studio. Но, повторюсь, встроенный в Visual Studio анализатор предназначен, в первую очередь, для улучшения дизайна и оптимизации кода, а не для поиска ошибок. В то время как PVS-Studio, наоборот, "заточен" на поиск ошибок с выдачей минимально возможного уровня ложных предупреждений. К тому же, разработчики CruiseControl.NET, по всей видимости, не применяли вообще никаких анализаторов. Я уверен, что при регулярном использовании анализатора, встроенного в Visual Studio, качество их кода было бы значительно лучше, а вероятность возможной ошибки ниже. Что уж говорить о PVS-Studio. Такие инструменты позволяют достичь максимального эффекта именно при регулярном использовании, а не "раз в год".
Скачать и попробовать PVS-Studio: http://www.viva64.com/ru/pvs-studio/
По вопросам приобретения коммерческой лицензии PVS-Studio просим Вас связаться с нами в почте. Вы также можете написать нам, чтобы получить временную лицензию для всестороннего изучения PVS-Studio, если хотите снять ограничения демонстрационной версии.