В ноябре прошлого года в нашем блоге была опубликована статья о разработке и использовании плагина PVS-Studio для SonarQube. Мы получили много откликов от клиентов и просто заинтересованных пользователей с просьбами провести тестирование плагина на реальном проекте. Так как интерес к этому вопросу не ослабевает, было решено провести тестирование на C# проекте PascalABC.NET. Также не будем забывать, что SonarQube содержит собственный статический анализатор C# кода - SonarC#. Для полноты картины проведем исследование и SonarC#. Целью данной работы является не сравнение анализаторов, а показ основных особенностей их взаимодействия с сервисом SonarQube. Прямое сравнение анализаторов было бы не вполне корректным по той причине, что PVS-Studio является специализированным инструментом поиска ошибок и потенциальных уязвимостей, в то время как SonarQube - это сервис оценки качества кода по большому числу параметров: дублирование кода, соблюдение стандартов кодирования, покрытие кода модульными тестами, возможные ошибки в коде, плотность комментариев в коде, технический долг и т.д.
Предварительно рекомендую ознакомиться с материалами статьи, где мы рассказываем о платформе SonarQube и интеграции с ней анализатора PVS-Studio.
Теперь немного об исследуемом проекте. PascalABC.NET – это реализация языка программирования Pascal нового поколения, содержащая собственную среду разработки, а также Web-среду для создания программ на языках PascalABC.NET, C#, Visual Basic.NET, F#, IronPython. Проект разработан на языке C# и распространяется под свободной лицензией LGPLv3. Сайт проекта. Исходный код можно загрузить из репозитория на GitHub.
Решение PascalABC.NET включает 2628 файлов с расширением '.cs', которые содержат около 752 тысяч строк кода (метрики получены с помощью утилиты SourceMonitor). Таким образом, проект имеет вполне подходящий размер для наших исследовательских целей.
Как уже говорилось ранее, сервис SonarQube имеет в своем составе, в том числе, статический анализатор C# кода. Чтобы, как в нашем случае, добавить открытый проект на сайт, а также произвести его анализ, достаточно нескольких несложных действий.
Для регистрации на сайте SonarQube я использовал учетную запись GitHub. Далее воспользовался инструкцией по быстрому старту. Весь процесс настройки, включая привязку проекта PascalABC.NET к аккаунту, получение уникального ключа организации и настройку на локальном компьютере, занял у меня около 15 минут. Еще 10 минут ушло на анализ проекта. После этого результат был загружен на сайт SonarQube, с ним может ознакомиться любой желающий.
SonarQube выдал 3636 предупреждений о возможных ошибках в коде PascalABC.NET:
Из них: 8 блокирующих (требуют немедленного устранения), 64 критичных, 1742 важных и 1822 не критичных. Информационных сообщений выдано не было. Давайте попробуем ознакомиться с полученными предупреждениями, найти интересные ошибки и понять, каков процент ложных срабатываний. Для этого воспользуемся удобными средствами фильтрации в различных измерениях, предоставляемыми сервисом SonarQube. Начнем с блокирующих предупреждений.
Как видим, блокирующие предупреждения выданы для двух правил: бесконечная рекурсия и очистка IDisposable-ресурсов. Вот пример одного из blocker-предупреждений:
В get-секции свойства Instance ошибочно возвращается Instance, вместо instance, что порождает бесконечную рекурсию.
Все остальные предупреждения на уровне Blocker также являются ошибками.
На уровне Critical было выдано 64 предупреждения для правила о недопустимом приведении типов. Рассмотрим одно из таких предупреждений:
Изучив код и список имплементаций, я соглашусь с анализатором: в данный момент действительно нет ни одного типа, реализующего сразу оба интерфейса IBaseScope и IComparable, вследствие чего результатом проверки boxItem.Item is IComparable всегда будет false. Однако, я бы не стал говорить об ошибке в данном случае, так как, во-первых, само наличие такой проверки исключает последующее возникновение исключения при попытке приведения типа (IComparable)boxItem.Item. Во-вторых, в любой момент к решению может быть подключена, например, некая dll, в которой будет объявлен тип, реализующий оба интерфейса IBaseScope и IComparable. Возможно, на это и рассчитывал разработчик, реализуя приведение типа только после проверки. Рассмотренное предупреждение, на мой взгляд, следует отнести к разряду сообщений для секции Minor, а не критичных для выполнения, и наличие его на уровне Critical считать ложным срабатыванием.
Оставшиеся 63 предупреждения аналогичны рассмотренному.
На данном уровне было выдано достаточно много предупреждений - 1742, для пятнадцати типов диагностик:
Давайте пройдем по списку предупреждений с целью поиска реальных ошибок и оценки возможностей анализатора.
General exceptions should never be thrown
Правило сообщает о выбросе исключения общего типа при помощи throw. В коде проекта PascalABC.NET было найдено 634 подобные конструкции. Подавляющее большинство имеет следующий вид:
Также встречается очень много (более 600) конструкций, похожих на "заглушки" в коде, умышленно оставленные разработчиками:
Конечно, выброс исключений общего типа является "дурным тоном". Тем не менее, как мне кажется, это вовсе не ошибки. Тем более маловероятно, чтобы авторы кода умышленно наплодили их в таком количестве. Да, обработка исключений в проекте PascalABC.NET, по всей видимости, не на высоте. Тем не менее, место всем этим 634 однотипным предупреждениям в секции Minor или вовсе в ложных срабатываниях анализатора.
Кстати, это хороший пример, в чем разница между SonarC# и анализатором PVS-Studio. SonarC# указывает на "запахи" в коде и совершенно прав, выдав эти предупреждения. Они позволяют судить о качестве проекта. С точки же нас, разработчиков анализатора PVS-Studio, это ложные срабатывания, так как мы ориентированы на поиск ошибок и дефектов безопасности.
Dead stores should be removed
Также весьма обширная группа из 618 предупреждений о повторном присвоении значения переменной, если при этом между присвоениями она никак не используется. Здесь преобладает следующий паттерн:
Переменную инициализируют при объявлении, а затем, ни разу не использовав сохраненное значение, присваивают новое. Конечно, так делать не следует. Тут и вопросы экономии ресурсов, и возможные подозрения на другую ошибку или опечатку. Но фактически - ни одна из таких конструкций ошибкой не является. Вновь непонятно, почему все подобные предупреждения были помещены в секцию ошибок высокой важности? На мой взгляд все это - ложные срабатывания.
Есть и несколько абсолютно однозначных false-positive предупреждений вида:
Если в данном случае последовать рекомендациям анализатора, то логика работы программы будет нарушена.
Таким образом, мне так и не удалось найти среди 618 предупреждений из рассмотренной группы ни одной реальной ошибки.
Floating point numbers should not be tested for equality
151 предупреждение было выдано для конструкций сравнения, в которых один или оба из сравниваемых операндов имеют вещественный тип. Действительно, такие сравнения часто дают ошибочный результат, который связан с особенностями хранения вещественных переменных в памяти и может варьироваться, например, в зависимости от настроек компилятора. Такие конструкции могут очень долгое время работать без проблем. При этом необходимо в каждом конкретном случае принимать решение об ошибочности такого кода. Например, если сравниваемые значения являются результатом математических вычислений, то прямое сравнение этих значений обычно ошибочно. Если же вы производите сравнение двух вещественных констант, то, вероятно, это делается осмысленно и ошибки не будет.
В коде PascalABC.NET мне встретился, преимущественно, следующий паттерн сравнения с вещественной переменной:
Обратите внимание, что сравнение производится как двух вещественных переменных, так и вещественной переменной с переменной целого типа. Конечно, такой код не вполне безопасен, так как неизвестно, каким образом были получены сравниваемые значения. Стоит ли здесь говорить о явной ошибке? Сложно дать однозначный ответ. Но код, вероятно, требует доработки.
Кстати, анализатор PVS-Studio также предупреждает о таких подозрительных сравнениях, но эти диагностики относятся к уровню достоверности Low и не рекомендуются нами к изучению.
Также среди выданных анализатором предупреждений есть очевидные ложные срабатывания, например:
В данном случае производится сравнение двух переменных типа byte. Переменные left и right имеют тип byte_const_node:
public class byte_const_node : concrete_constant<byte>,
SemanticTree.IByteConstantNode
{
public byte_const_node(byte value, location loc)
: base(value, loc)
{
}
....
}
public abstract class concrete_constant<ConstantType> : constant_node
{
private ConstantType _constant_value;
public concrete_constant(ConstantType value, location loc) :
base(compiled_type_node.get_type_node(typeof(ConstantType)), loc)
{
_constant_value = value;
}
....
public ConstantType constant_value
{
get
{
return _constant_value;
}
....
}
....
}
....
}Думаю, данная группа предупреждений обоснованно находится в секции Major. Однако, я бы не стал считать все найденные предупреждения ошибками. Решение должен принимать автор кода в каждом конкретном случае.
Multiline blocks should be enclosed in curly braces
Группа из 108 предупреждений, включающая потенциальные ошибки форматирования, влияющие на логику выполнения программы. Здесь я обнаружил довольно подозрительные конструкции. Пример:
В данном фрагменте, возможно, пропущены скобки. В любом случае, разработчику следует отформатировать код для лучшего понимания логики работы программы.
Ещё одно подобное предупреждение:
Ошибки нет, но код выглядит неаккуратно. Необходим рефакторинг.
В принципе, все предупреждения из данной группы выданы по делу, но настоящих ошибок они не выявили.
Null pointers should not be dereferenced
75 предупреждений о возможном доступе по нулевой ссылке. В данном блоке я обнаружил интересные ошибки:
Действительно, ранее в коде переменная returned_scope всегда проверяется на равенство null перед использованием, но в данном случае про это забыли:
public override void visit(....)
{
....
if (returned_scope != null && ....)
{
....
}
else if (returned_scope != null)
{
....
}
returned_scope.declaringUnit = entry_scope; // <=
....
}Ещё одна похожая ошибка:
В первом случае переменную pi проверяют на равенство null перед использованием, но далее, при следующем обращении к pi.CompilationUnit, это сделать забывают.
Блок предупреждений содержит некоторое количество не столь очевидных ошибок, а также ложных срабатываний. Я бы оценил процент нахождения реальных ошибок здесь равным 85%. Очень хороший результат.
Conditions should not unconditionally evaluate to "true" or to "false"
Блок предупреждений об условиях, которые выполнимы вне зависимости от логики работы программы. Типичная из найденных ошибок:
Странный код, требующий доработки автором. Возможно, допущена серьезная ошибка.
В целом, группа содержит порядка 70% подобных ошибок.
Exceptions should not be thrown from property getters
Не следует выбрасывать исключения в get-секции свойства, а при необходимости - использовать методы вместо свойств. В данной группе содержится 46 таких предупреждений. Подавляющее большинство из них - это "заглушки", оставленные разработчиками умышленно или по забывчивости:
Есть и не вполне корректные конструкции, требующие рефакторинга:
Тем не менее, я не считаю данные предупреждения ошибками. Думаю, рациональней их было-бы отнести к секции Minor.
Static fields should not be updated in constructors
Диагностика об опасности обновления статических полей в конструкторах: это может привести к несогласованному поведению программы, так как поле будет заново инициализировано для всех экземпляров класса. Всего для проекта PascalABC.NET анализатор выдал 26 подобных предупреждений. Я не нашёл среди них реальных ошибок. Вот пара примеров обнаруженных фрагментов кода:
В статическую переменную _instance каждый раз записывают ссылку на новый экземпляр класса. Судя по имени переменной - так и задумывалось.
Флаг parsers_loaded сигнализирует о том, что хотя бы один экземпляр класса уже был создан. Ничего криминального.
"=+" should not be used instead of "+="
Интересная диагностика о том, что вместо оператора "-=", например, ошибочно использовали "=-". Анализатор выдал 9 подобных предупреждений. К сожалению, все они являются ложными срабатываниями. 6 предупреждений выдано для конструкций, являющихся объявлением переменных, где в принципе невозможно использование оператора "-=" или "+=":
Оставшиеся 3 предупреждения связаны с тем, что авторы кода, видимо, не слишком любят использовать пробелы для форматирования своего кода:
Related "if/else if" statements should not have the same condition
5 предупреждений было выдано для фрагментов кода с одинаковым условием в блоках if и else if. Часто такой код либо уже является ошибочным, либо содержит потенциальную возможность ошибки. В нашем случае 4 из 5 предупреждений содержали простое дублирование условия, а также блока выполнения, что, конечно, подозрительно, но не является грубой ошибкой. Одно предупреждение более интересно:
До того, как часть условия в первом блоке if закомментировали, оно отличалось от условия в следующем за ним блоке else if. Также обратите внимание на блок выполнения этого второго else if: он пуст. Там присутствует только один оператор: ";". Очень странный и подозрительный код.
Short-circuit logic should be used in boolean contexts
Диагностика предупреждает, например, о возможном ошибочном использовании оператора & вместо && для выражений типа bool. Всего было найдено 5 подозрительных конструкций. Все они так или иначе требуют к себе внимания, хотя, возможно, и не содержат ошибок. Вот пример одной и них:
В данном случае нельзя точно утверждать, что использование оператора "|" ошибочно, так как в правой его части проверяется свойство со сложной логикой внутри. Возможно, разработчик добивался именно того, чтобы всегда проверялись оба условия.
Exceptions should not be explicitly rethrown
Диагностика про потерю стека исключения. Анализатор выдал 4 однотипных предупреждения, вида:
Конечно, так делать не следует. Дальнейшая отладка приложения будет затруднена. Но все эти предупреждения не настолько критичны. По моему мнению, их место в секции Minor.
Variables should not be self-assigned
3 предупреждения о присваивании значения переменной самой себе. Вот пример одного из найденных фрагментов кода:
Странный и явно ошибочный код. Объявление visitNode имеет вид:
protected bool visitNode = true;Всего в данной группе предупреждений содержится две ошибки.
Identical expressions should not be used on both sides of a binary operator
Диагностика производит поиск условий, в которых есть одинаковые подвыражения. Было найдено 2 подозрительные конструкции. Явной ошибки нет ни в одной из них, но, возможно, код должен был выглядеть и работать иначе. Пример одного из предупреждений:
Странный код. Возможно, забыли заменить вторую проверку.
"ToString()" method should not return null
Последняя группа предупреждений в секции Major. Перегрузка метода ToString() реализована некорректно. Выдано 2 предупреждения, и оба являются ошибками. Пример одной из них:
Некорректно возвращать null из перегруженного метода ToString(). Необходимо использовать string.Empty.
Здесь было выдано 1822 предупреждения. Так как данный уровень не является критичным, маловероятно, что здесь я обнаружу действительно интересные ошибки. Также, обычно, на этом уровне регистрируется достаточно много ложных срабатываний. Поэтому я не буду рассматривать предупреждения уровня Minor в данном исследовании.
Подводя итог могу сказать, что в целом анализатор нашёл реальные ошибки на уровнях Blocker, Critical и Major (я насчитал 268 ошибочных или крайне подозрительных конструкций на 1814 предупреждений), некоторые из которых действительно интересны. Тем не менее, значение процента ложных срабатываний довольно велико и составляет более 85%. Это значительно затрудняет анализ результатов работы.
Вопросам интеграции результатов работы анализатора PVS-Studio в SonarQube посвящен раздел документации на нашем сайте. Для настройки интеграции "с нуля" мне понадобилось около 15 минут. Еще столько же заняла проверка проекта и загрузка результатов на локальный сервер SonarQube.
PVS-Studio выдал 1039 предупреждений в ходе проверки кода PascalABC.NET. Из них: 156 предупреждений уровня Critical, 541 - уровня Major, 342 - уровня Minor.
Предупреждения уровня Minor рассматривать не будем, так как среди них обычно высок процент ложных срабатываний.
Распределение предупреждений по диагностикам на уровне Critical:
Распределение предупреждений по диагностикам на уровне Major:
Проанализировав 697 предупреждений на уровнях Critical и Major, я выяснил, что 204 из них можно отнести к ложным срабатываниям. Это составляет около 29% от общего числа предупреждений на первом и втором уровне важности. Таким образом, процент выявления реальных ошибок и подозрительных конструкций для проекта PascalABC.NET равен 71%. В пересчете на количество строк кода (KLOC) это 0.66 ошибки на KLOC. Давайте взглянем на наиболее интересные из обнаруженных ошибок. Для удобства я буду приводить ошибки по возрастанию номера диагностического правила.
Copy-Paste
V3001 There are identical sub-expressions 'token.Kind == openBracketToken' to the left and to the right of the '||' operator. ICSharpCode.SharpDevelop NRefactoryInsightWindowHandler.cs 66
readonly int eofToken,
commaToken,
openParensToken,
closeParensToken,
openBracketToken,
closeBracketToken,
openBracesToken,
closeBracesToken,
statementEndToken;
public void InitializeOpenedInsightWindow(....)
{
....
if (token.Kind == openParensToken ||
token.Kind == openBracketToken ||
token.Kind == openBracketToken) { // <=
bracketCount++;
}
....
}В условии блока if дважды проверяется равенство token.Kind == openBracketToken. Среди полей, объявленных в классе, можно найти поле с очень похожим именем openBracesToken. Вероятно, именно это поле было пропущено в условии. В таком случае, исправленный вариант кода мог бы иметь вид:
public void InitializeOpenedInsightWindow(....)
{
....
if (token.Kind == openParensToken ||
token.Kind == openBracketToken ||
token.Kind == openBracesToken) {
bracketCount++;
}
....
}Подобные ошибки в коде:
Невнимательность
V3003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 597, 631. ParserTools SyntaxTreeComparer.cs 597
public void CompareInternal(....)
{
....
if (left is ident)
CompareInternal(left as ident, right as ident);
....
else if (left is int64_const)
CompareInternal(left as int64_const, right as int64_const);
....
else if (left is int64_const)
CompareInternal(left as int64_const, right as int64_const);
....
}Приведенный фрагмент кода в действительности содержит около тридцати однотипных проверок, две из которых полностью идентичны. Возможно, здесь и не содержится ошибка, а код просто продублирован из-за невнимательности. Но одна из проверок, по замыслу разработчика, могла выглядеть и иначе. В таком случае мы имеем дело с серьезной логической ошибкой.
Аналогичные ошибки:
Copy-Paste v2.0
V3004 The 'then' statement is equivalent to the 'else' statement. VisualPascalABCNET CodeCompletionWindow.cs 204
public void HandleMouseWheel(....)
{
....
if (System.Windows.Forms.SystemInformation.MouseWheelScrollLines
> 0) {
newValue = this.vScrollBar.Value -
(control.TextEditorProperties.MouseWheelScrollDown ? 1 : -1) *
multiplier;
} else {
newValue = this.vScrollBar.Value -
(control.TextEditorProperties.MouseWheelScrollDown ? 1 : -1) *
multiplier;
}
....
}Обе ветви блока if содержат идентичные подвыражения. В данном случае сложно сделать вывод о правильном варианте данного фрагмента, но в приведенном виде код будет работать не так, как ожидается.
Подобные ошибки в коде:
Я привел только первые 10 подобных ошибок из 20.
Переменная присваивается сама себе
V3005 The 'miGenerateRealization.Visible' variable is assigned to itself. VisualPascalABCNET OptionsManager.cs 342
public void UpdateUserOptions()
{
....
tsViewIntellisensePanel.Visible = tssmIntellisence.Visible =
tsGotoDefinition.Visible = tsGotoRealization.Visible =
tsFindAllReferences.Visible = miGenerateRealization.Visible =
miGenerateRealization.Visible = cmGenerateRealization.Visible =
cmsCodeCompletion.Visible = cmFindAllReferences.Visible =
cmGotoDefinition.Visible = cmGotoRealization.Visible =
UserOptions.AllowCodeCompletion;
}Переменная miGenerateRealization.Visible получает одинаковое значение дважды в ходе присваивания. Вероятно, лишнее присваивание добавлено случайно. Однако, вместо одной из переменных miGenerateRealization.Visible могла бы находиться какая-то другая переменная, которая теперь не инициализируется.
Найдена ещё одна аналогичная ошибка:
V3005 The 'visitNode' variable is assigned to itself. SyntaxVisitors SimplePrettyPrinterVisitor.cs 106
Повторное присваивание
V3008 The 'codeCompileUnit' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 126, 124. VisualPascalABCNET CodeDomHostLoader.cs 126
CodeCompileUnit codeCompileUnit = null;
private DesignSurface Designer;
....
protected override CodeCompileUnit Parse()
{
....
CodeCompileUnit ccu = null;
DesignSurface ds = new DesignSurface();
....
ccu = cg.GetCodeCompileUnit(idh);
....
codeCompileUnit = ccu;
Designer = ds;
codeCompileUnit = ccu; // <=
....
}Из кода видно, что нет совершенно никакого логического объяснения повторному присваиванию переменной codeCompileUnit того же самого значения.
Подобные ошибки в коде:
Результат работы метода всегда одинаков
V3009 It's odd that this method always returns one and the same value of 'false'. NETGenerator NETGenerator.cs 5434
private bool BeginOnForNode(IStatementNode value)
{
//if (value is IForNode) return true;
IStatementsListNode stats = value as IStatementsListNode;
if (stats == null) return false;
if (stats.statements.Length == 0) return false;
//if (stats.statements[0] is IForNode) return true;
return false;
}Вероятно, здесь мы имеем дело с невнимательностью при рефакторинге. Ранее в коде были блоки кода, возвращающие true. Однако теперь они закомментированы, а метод, независимо от результата своей работы, вернет false.
Подобные ошибки в коде:
Невнимательность
V3010 The return value of function 'OrderBy' is required to be utilized. ICSharpCode.SharpDevelop RefactoringService.cs 86
static IEnumerable<ITreeNode<IClass>> FindDerivedClassesTree(....)
{
....
var result = new List<TreeNode<IClass>>();
....
result.OrderBy(node => node.Content.FullyQualifiedName); // <=
return result;
}Результат сортировки списка result никуда не сохраняется. Исправленный вариант приведенного фрагмента:
static IEnumerable<ITreeNode<IClass>> FindDerivedClassesTree(....)
{
....
var result = new List<TreeNode<IClass>>();
....
return result.OrderBy(node => node.Content.FullyQualifiedName);
}И ещё одна такая ошибка:
V3010 The return value of function 'ToString' is required to be utilized. CodeCompletion SymTable.cs 2145
Проблема с логикой
V3018 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. VisualPascalABCNET InsightWindow.cs 145
public void HandleMouseWheel(MouseEventArgs e)
{
....
if (e.Delta > 0) {
if (control.TextEditorProperties.MouseWheelScrollDown) {
CurrentData = (CurrentData + 1) % DataProvider.InsightDataCount;
} else {
CurrentData = (CurrentData + DataProvider.InsightDataCount - 1)
% DataProvider.InsightDataCount;
}
} if (e.Delta < 0) { // <=
if (control.TextEditorProperties.MouseWheelScrollDown) {
CurrentData = (CurrentData + DataProvider.InsightDataCount
- 1) % DataProvider.InsightDataCount;
} else {
CurrentData = (CurrentData + 1) %
DataProvider.InsightDataCount;
}
}
....
}Обратите внимание на условие if (e.Delta < 0). Исходя из того, как отформатирован код, а также из логики программы, следует вывод: возможно, пропущено ключевое слово else. Тем не менее, только автор может дать точный ответ об особенностях данной конструкции.
Классическая ошибка при работе с оператором "as"
V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'baseScope', 'this.baseScope'. CodeCompletion SymTable.cs 3497
public TypeScope(...., SymScope baseScope)
{
....
this.baseScope = baseScope as TypeScope;
....
if (baseScope == null)
{
....
}
....
}После приведения аргумента baseScope к типу TypeScope по ошибке на равенство null проверяется не поле this.baseScope, а аргумент baseScope. Исправленный вариант кода:
public TypeScope(...., SymScope baseScope)
{
....
this.baseScope = baseScope as TypeScope;
....
if (this.baseScope == null)
{
....
}
....
}Подобные ошибки в коде:
Неаккуратный код
V3022 Expression 't == null' is always true. VisualPascalABCNET Debugger.cs 141
public static Type GetTypeForStatic(string name)
{
Type t = stand_types[name] as Type;
if (t != null) return t;
if (t == null) // <=
foreach (string s in ns_ht.Keys)
{
....
}
t = PascalABCCompiler.NetHelper.NetHelper.FindType(name);
....
}Ошибки нет, но программа выглядит неаккуратно.
Подобные конструкции в коде:
Я привёл только первые 10 подобных предупреждений из более чем 45.
Избыточная проверка или ошибка?
V3030 Recurring check. The 'upperScopeWhereVarsAreCaptured != scope' condition was already verified in line 383. TreeConverter CapturedVariablesSubstitutionClassGenerator.cs 391
private void VisitCapturedVar(....)
{
....
if (upperScopeWhereVarsAreCaptured != scope)
{
....
if (upperScopeWhereVarsAreCaptured != scope)
{
....
}
....
}
....
}Обычно такие конструкции не являются ошибкой, но существует вероятность, что одна из проверок должна была содержать другое условие.
Подобные ошибки в коде:
Странное форматирование
V3033 It is possible that this 'else' branch must apply to the previous 'if' statement. TreeConverter syntax_tree_visitor.cs 14894
public override void visit(....)
{
....
if (_var_def_statement.inital_value != null)
if (is_event) AddError(....);
else
{
....
}
....
}По логике работы программы, ключевое слово else относится к блоку условия if (is_event). Однако, код отформатирован таким образом, что создается совсем другое впечатление. Вероятно, использование скобок { } решило бы проблему.
Опечатка
V3038 The 'enum_consts[i]' argument was passed to 'Compare' method several times. It is possible that other argument should be passed instead. CodeCompletion SymTable.cs 2206
private List<string> enum_consts = new List<string>();
public override bool IsEqual(SymScope ts)
{
EnumScope es = ts as EnumScope;
if (es == null) return false;
if (enum_consts.Count != es.enum_consts.Count) return false;
for (int i = 0; i < es.enum_consts.Count; i++)
if (string.Compare(enum_consts[i],
this.enum_consts[i], true) != 0) // <=
return false;
return true;
}К сожалению, метод IsEqual не содержит объявления локальной переменной enum_consts. Поэтому внутри цикла for элементы списка enum_consts многократно сравниваются сами с собой. По виду метода IsEqual можно сделать предположение о правильном варианте кода:
public override bool IsEqual(SymScope ts)
{
....
for (int i = 0; i < es.enum_consts.Count; i++)
if (string.Compare(enum_consts[i],
es.enum_consts[i], true) != 0)
....
}Проблема с логикой v2.0
V3043 The code's operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. VBNETParser LanguageInformation.cs 1002
public override string FindExpression(....)
{
....
switch (ch)
{
....
case '(':
if (kav.Count == 0)
{
....
}
else sb.Insert(0, ch); punkt_sym = true;
break;
}
....
}Присваивание punkt_sym = true будет выполнено независимо от результата проверки kav.Count == 0. Тем не менее, код отформатирован таким образом, что создается впечатление, что это будет сделано только при условии kav.Count != 0.
Ещё одна подобная ошибка:
V3043 The code's operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. ICSharpCode.SharpDevelop AbstractConsolePad.cs 159
Потеря стека исключения
V3052 The original exception object 'e' was swallowed. Stack of original exception could be lost. NETGenerator NETGenerator.cs 925
public void ConvertFromTree(....)
{
....
try
{
....
}
catch (System.Runtime.InteropServices.COMException e)
{
throw new TreeConverter.SaveAssemblyError(e.Message);
}
....
}Из объекта выброшенного исключения типа COMException разработчик использует только текст сообщения. По всей видимости, это осмысленное решение, так как далее выбрасывается исключение типа SaveAssemblyError, конструктор которого ничего кроме текста сообщения не требует:
public class SaveAssemblyError : CompilationError
{
....
public SaveAssemblyError(string text)
{
_text = text;
}
....
}Конечно, такой вариант реализации - это право автора. Однако, на мой взгляд, обработка исключения в данном случае не выглядит полноценной.
Подобные ошибки в коде:
Ошибка работы с подстроками
V3053 An excessive expression. Examine the substrings 'reduction' and 'reduction('. TreeConverter OpenMP.cs 267
private void ProcessClauses(string Text, ....)
{
....
if (....)
{
....
}
else if (AllowReduction &&
(Text.StartsWith("reduction") ||
Text.StartsWith("reduction(")))
{
....
}
....
}В данном случае поиск подстроки "reduction(" лишен смысла, так как ранее всегда будет найдена подстрока "reduction".
Ошибочный порядок инициализации
V3070 Uninitialized variable 'event_add_method_prefix' is used when initializing the 'event_add_method_nameformat' variable. TreeConverter compiler_string_consts.cs 313
public static class compiler_string_consts
{
....
public static string event_add_method_nameformat =
event_add_method_prefix + "{0}";
....
public static string event_add_method_prefix = "add_";
....
}В результате выполнения приведенного фрагмента кода, строка event_add_method_nameformat получит значение "{0}", вместо ожидаемого "add_{0}". Для исправления ошибки следует поменять местами строки инициализации полей:
public static class compiler_string_consts
{
....
public static string event_add_method_prefix = "add_";
....
public static string event_add_method_nameformat =
event_add_method_prefix + "{0}";
....
}Ещё одна такая же ошибка:
V3070 Uninitialized variable 'event_remove_method_prefix' is used when initializing the 'event_remove_method_nameformat' variable. TreeConverter compiler_string_consts.cs 314
Доступ по нулевой ссылке: небрежный рефакторинг
V3080 Possible null dereference. Consider inspecting 'tc'. CodeCompletion CodeCompletionPCUReader.cs 736
private TypeScope GetTemplateInstance()
{
TypeScope tc = null;//GetTemplateClassReference();
int params_count = br.ReadInt32();
for (int i = 0; i < params_count; i++)
{
tc.AddGenericInstanciation(GetTypeReference()); // <=
}
return tc;
}Как видим, ранее переменная tc инициализировалась значением GetTemplateClassReference(). Однако, теперь - это значение null. В результате, на первой же итерации цикла for будет возникать ошибка доступа по нулевой ссылке. Возможно, ошибка пока никак не проявила себя, так как вызовы метода GetTemplateInstance() в коде отсутствуют. Но нет никакой гарантии того, что так будет и в дальнейшем.
Подобные ошибки в коде:
Доступ по нулевой ссылке: невнимательность
V3095 The 'VisualEnvironmentCompiler.RemoteCompiler' object was used before it was verified against null. Check lines: 52, 54. CompilerController CompilerControllerPlugin.cs 52
public CompilerController_VisualPascalABCPlugin(....)
{
....
VisualEnvironmentCompiler.RemoteCompiler.InternalDebug.RunOnMono =
CompilerInformation.cbRunMono.Checked;
....
if (VisualEnvironmentCompiler.RemoteCompiler != null)
....
}Проверку на возможное равенство переменной null делают уже после её использования. Исправленный вариант кода:
public CompilerController_VisualPascalABCPlugin(....)
{
....
if (VisualEnvironmentCompiler.RemoteCompiler != null)
{
VisualEnvironmentCompiler.RemoteCompiler.
InternalDebug.RunOnMono =
CompilerInformation.cbRunMono.Checked;
....
}
}Подобные ошибки в коде:
Я привел только первые 10 подобных ошибок из более чем 40.
Бесконечная рекурсия: x2
V3110 Possible infinite recursion inside 'SetRange' method. TreeConverter SymbolInfoArrayList.cs 439
V3110 Possible infinite recursion inside 'SetRange' method. TreeConverter SymbolInfoArrayList.cs 444
public void SetRange(int index,SymbolInfo[] tnarr)
{
SetRange(index,tnarr);
}
public void SetRange(int index,SymbolInfoArrayList tnarl)
{
SetRange(index,tnarl);
}Сразу два метода, реализующих бесконечную рекурсию. Оба метода похожи и отличаются только типом второго аргумента. Нигде в коде не используются. По крайней мере пока не используются.
Подобные ошибки в коде:
Небрежная реализация метода Equals
V3115 Passing 'null' to 'Equals' method should not result in 'NullReferenceException'. ICSharpCode.SharpDevelop ServiceReferenceMapFile.cs 31
public override bool Equals(object obj)
{
var rhs = obj as ServiceReferenceMapFile;
return FileName == rhs.FileName; // <=
}Автор данного фрагмента кода довольно халатно отнёсся к вопросам безопасности его работы. Не хватает как минимум одной проверки на равенство null переменной rhs после её инициализации. А чтобы вообще не делать лишней работы, необходима предварительная проверка на null переменной obj:
public override bool Equals(object obj)
{
if (obj == null || !(obj is ServiceReferenceMapFile))
return false;
var rhs = obj as ServiceReferenceMapFile;
return FileName == rhs.FileName;
}Недостаточно проверок
V3125 The 'resources' object was used after it was verified against null. Check lines: 215, 211. VisualPascalABCNET DesignerResourceService.cs 215
public System.Resources.IResourceReader
GetResourceReader(System.Globalization.CultureInfo info)
{
....
if (resources != null && resources.ContainsKey(info.Name)) {
resourceStorage = resources[info.Name];
} else {
resourceStorage = new ResourceStorage();
resources[info.Name] = resourceStorage; // <=
}
....
}В коде присутствует проверка на равенство null переменной resources, но этого недостаточно, так как блок else такой проверки не содержит. При определённом стечении обстоятельств это неминуемо приведет к доступу по нулевой ссылке. Код необходимо откорректировать:
public System.Resources.IResourceReader
GetResourceReader(System.Globalization.CultureInfo info)
{
....
if (resources != null) {
if (resources.ContainsKey(info.Name)) {
resourceStorage = resources[info.Name];
} else {
resourceStorage = new ResourceStorage();
resources[info.Name] = resourceStorage;
}
}
....
}Подобные ошибки в коде:
Я привел только первые 10 подобных ошибок из более чем 80 (восьмидесяти!).
Ошибочный порядок инициализации
V3128 The 'dockPanel' field is used before it is initialized in constructor. ICSharpCode.SharpDevelop SearchResultsPad.cs 49
....
DockPanel dockPanel;
....
public SearchResultsPad()
{
....
defaultToolbarItems = ToolBarService.
CreateToolBarItems(dockPanel, ....); // <=
foreach (object toolBarItem in defaultToolbarItems) {
toolBar.Items.Add(toolBarItem);
}
....
dockPanel = new DockPanel {
Children = { toolBar, contentPlaceholder }
};
....
}Поле dockPanel сначала используют в конструкторе SearchResultsPad, а затем инициализируют. При этом, даже если в методе CreateToolBarItems или вложенных методах предусмотрено равенство null первого аргумента, метод, вероятно, вернет null. Это, в свою очередь, приведет к дальнейшим ошибкам при использовании переменной defaultToolbarItems.
Я вижу общую картину следующим образом. Анализаторы SonarC# и PVS-Studio решают разные задачи. SonarC# предназначен для оценки и контроля качества кода. Поэтому он сообщает как об ошибках, так и о "запахах" кода. PVS-Studio ориентирован на поиск ошибок или мест в коде, которые могут впоследствии привести к ошибкам. Конечно, выдаваемые сообщения этих анализаторов частично пересекаются, но рассчитаны для разных потребностей:
Приведу сводную таблицу по результатам проверки проекта PascalABC.NET (взяты уровни предупреждений Blocker, Critical и Major):
Ещё раз хочу отметить, что анализаторы нельзя напрямую сравнивать по количеству найденных ошибок и количеству ложных срабатываний. SonarC# старается выдать предупреждение на код, который хотя и плох, но не содержит ошибки. Это как раз и позволяет оценивать качество кода. В свою очередь, анализатор PVS-Studio в таком случае предпочитает промолчать или выдать предупреждение с минимальным уровнем достоверности. При этом он старается выявить как можно больше ошибок и обучен выявлять большее количество дефектов, приводящих к сбоям в работе программы.
Итак, как и ожидалось, при работе с плагинами PVS-Studio и SonarC# для SonarQube у меня не возникло никаких проблем. Все функции и особенности инструментов задокументированы. После загрузки результатов на сервер SonarQube вы получаете доступ к множеству функциональных возможностей для оценки качества своего программного продукта. Что касается собственно поиска ошибок анализаторами исходного кода, то оба инструмента показали достойный результат.
Для загрузки и анализа проекта онлайн на сайте SonarQube вам потребуется минимум усилий и времени.
Использование плагина PVS-Studio для интеграции результатов его работы в SonarQube также не вызывает трудностей. Единственное ограничение - вам понадобится Enterprise версия анализатора. Если же у вас нет необходимости интеграции с SonarQube, вы можете использовать PVS-Studio как независимый инструмент.
Скачать и попробовать PVS-Studio: https://pvs-studio.com/ru/pvs-studio/try-free/
По вопросам приобретения коммерческой лицензии PVS-Studio просим Вас связаться с нами в почте. Вы также можете написать нам, чтобы получить временную лицензию для всестороннего изучения PVS-Studio, если хотите снять ограничения демонстрационной версии.