Ищем ошибки в MonoDevelop
В жизни анализатора PVS-Studio состоялось важное событие — в последней версии была добавлена возможность проверки кода, написанного на C#. Являясь одним из разработчиков данного анализатора, я просто не мог пройти мимо, не проверив какой-нибудь проект. Понятно, что мало кому будет интересно читать про проверку маленьких и неизвестных проектов, поэтому нужно было выбрать что-то известное, и выбор пал на MonoDevelop.
Немного о проекте
MonoDevelop— свободная среда разработки, предназначенная для создания приложений C#, Java, Boo, Nemerle, Visual Basic .NET, Vala, CIL, Cи C++. Также планируется поддержка Oxygeneсо стороны Embarcadero Technologies.
Изначально это был порт SharpDevelop на Mono/GTK+, но с того времени проект далеко ушёл от своего начального состояния.
MonoDevelop является частью проекта Mono. Встроен в дистрибутив Unity3Dкак средство написания скриптов, но устаревшей версии (4.0.1).
Среди возможностей данной среды разработки выделяют подсветку синтаксиса, сворачивание кода, автодополнение кода, браузер классов, поддержку плагинов, встроенный отладчик, визуальный конструктор форм, модульное тестирование.
Исходный код проекта доступен в соответствующем репозитории на GitHub, а инструкции по сборке описаны на официальном сайте проекта.
Чем проверяли?
Как уже упоминалось выше, анализ проекта осуществлялся с помощью последней версии статического анализатора кода PVS-Studio, в которую была добавлена возможность анализа C#-кода. Эта первый релиз C#-анализатора, и на данный момент в нём реализовано более 40 диагностических правил. Понятно, что это версия ещё развита далеко не так сильно, как C++-анализатор, но используя данный инструмент уже можно найти достаточно интересные ошибки (некоторые из которых и будет приведены в этой статье). C#-анализатор не является отдельным продуктом, он входит в состав того же PVS-Studio, который теперь просто умеет анализировать код, написанный на ещё одном языке программирования.
Скачать последнюю версию анализатора можно по этой ссылке.
Несколько слов о результате анализа
В результате анализа было проверено 8457 файлов в составе 95 проектов.
Анализатор выдал 118 предупреждений первого, 128 предупреждений второго и 475 предупреждений третьего уровней.
Кто-то может сказать, что это не так уж и много для такого количества файлов. Но здесь стоит принять во внимание тот факт, что на данный момент реализовано меньшее количество диагностик, нежели в С++ анализаторе. Во-вторых, анализатор малоэффективен при разовых проверках. Хоть это неоднократно повторялось, но стоит упомянуть ещё раз — для получения полноценной пользы от использования инструментов статического анализа, они должны применяться регулярно, а не разово. Это сэкономит время на поиск и устранение ошибок, и как следствие — сделает разработку проекта дешевле и легче.
Результаты анализа
В статье будут рассмотрены некоторые наиболее интересные из найденных ошибок, так как обзор всех найденных ошибок увеличил бы объём этой статьи до неприличных размеров. Статья разбита на подразделы, содержащие в себе описание тех или иных типов ошибок с примерами кода из проекта. Так что вы можете сразу перейти к просмотру наиболее интересных для вас ошибок.
Одинаковые выражения слева и справа от оператора
В данном подразделе приводятся описания ошибок вида 'A || A'. Зачастую такие ошибки получаются в результате опечаток или неудачного 'copy-paste' и невнимательности программиста. Часто такие ошибки бывает сложно найти в больших объёмах кода, особенно если названия переменных достаточно длинные и различаются только одним символом. Как правило, подразумевается использование другой переменной, но порой подобные проверки являются просто избыточным кодом. Подробнее обо всём этом чуть ниже.
protected override SourceCodeLocation
GetSourceCodeLocation (string fixtureTypeNamespace,
string fixtureTypeName,
string methodName)
{
if (string.IsNullOrEmpty (fixtureTypeName) ||
string.IsNullOrEmpty (fixtureTypeName))
return null;
....
}
Предупреждение анализатора: V3001 There are identical sub-expressions 'string.IsNullOrEmpty (fixtureTypeName)' to the left and to the right of the '||' operator. MonoDevelop.NUnit NUnitProjectTestSuite.cs 84
Ошибку видно невооружённым глазом — в условии дважды проверяется одна и та же строковая переменная на равенство 'null' или на эквивалентность 'String.Empty'. Ниже по коду (здесь приведено не всё тело, чтобы не усложнять восприятие, так что поверьте на слово) схожая проверка осуществляется для переменной 'fixtureTypeNamespace', так что можно предположить, что вторая проверка данного условия в качестве аргумента метода должна была принимать переменную 'methodName' или вовсе отсутствовать.
Другой пример подобной ошибки:
bool TryAddDocument (string fileName,
out OpenRazorDocument currentDocument)
{
....
var guiDoc = IdeApp.Workbench.GetDocument (fileName);
if (guiDoc != null && guiDoc.Editor != null)
....
guiDoc.Closed += (sender, args) =>
{
var doc = sender as MonoDevelop.Ide.Gui.Document;
if (doc.Editor != null && doc.Editor != null)
....
}
....
}
Предупреждение анализатора: V3001 There are identical sub-expressions 'doc.Editor!= null' to the left and to the right of the '&&' operator. MonoDevelop.AspNet RazorCSharpParser.cs 180
Опять 2 одинаковые проверки в пределах одного выражения. Теоретически после приведения переменной 'sender' с использованием оператора 'as' в переменную 'doc' может быть записано значение 'null'. В результате, при выполнении проверки 'doc.Editor!= null' будет сгенерировано исключение типа 'NullReferenceException'. Исправленный вариант кода мог бы выглядеть так:
if (doc != null && doc.Editor != null)
Ещё один фрагмент кода с ошибкой:
static MemberCore GetLaterDefinedMember (MemberSpec a, MemberSpec b)
{
var mc_a = a.MemberDefinition as MemberCore;
var mc_b = b.MemberDefinition as MemberCore;
if (mc_a == null)
return mc_b;
if (mc_b == null)
return mc_a;
if (a.DeclaringType.MemberDefinition !=
b.DeclaringType.MemberDefinition)
return mc_b;
if (mc_a.Location.File != mc_a.Location.File)
return mc_b;
return mc_b.Location.Row > mc_a.Location.Row ? mc_b : mc_a;
}
Предупреждение анализатора: V3001 There are identical sub-expressions 'mc_a.Location.File' to the left and to the right of the '!=' operator. ICSharpCode.NRefactory.CSharp membercache.cs 1319
Подобная ошибка может и не броситься в глаза, но анализатор — не человек, и такие вещи не пропускает. Из кода видно, что свойство 'File' объекта 'mc_a' сравнивается само с собой, но очевидно, что оно должно было сравниваться с соответствующим свойством объекта 'mc_b'.
Корректный код:
if (mc_a.Location.File != mc_b.Location.File)
Избыточный код:
public override AppResult Property (string propertyName, object value)
{
if (resultIter != null && resultIter.HasValue) {
var objectToCompare = TModel.GetValue (resultIter.Value, Column);
return MatchProperty (propertyName, objectToCompare, value);
}
return MatchProperty (propertyName, ParentWidget, value);
}
TreeIter? resultIter;
Предупреждение анализатора: V3001 There are identical sub-expressions 'resultIter!= null' to the left and to the right of the '&&' operator. MonoDevelop.Ide GtkTreeModelResult.cs 125
Переменная 'resultIter' имеет nullable-тип, следовательно, проверки вида 'resultIter!= null' и 'resultIter.HasValue' являются идентичными и можно было ограничиться одной из них.
Точно такой же код встретился ещё 1 раз. Соответствующее предупреждение анализатора:
V3001 There are identical sub-expressions 'resultIter!= null' to the left and to the right of the '&&' operator. MonoDevelop.Ide GtkTreeModelResult.cs 135
Рассмотрим следующий фрагмент кода:
Accessibility DeclaredAccessibility { get; }
bool IsStatic { get; }
private bool MembersMatch(ISymbol member1, ISymbol member2)
{
if (member1.Kind != member2.Kind)
{
return false;
}
if (member1.DeclaredAccessibility != member1.DeclaredAccessibility
|| member1.IsStatic != member1.IsStatic)
{
return false;
}
if (member1.ExplicitInterfaceImplementations().Any() ||
member2.ExplicitInterfaceImplementations().Any())
{
return false;
}
return SignatureComparer
.HaveSameSignatureAndConstraintsAndReturnTypeAndAccessors(
member1, member2, this.IsCaseSensitive);
}
Предупреждения анализатора:
- V3001 There are identical sub-expressions 'member1.DeclaredAccessibility' to the left and to the right of the '!=' operator. CSharpBinding AbstractImplementInterfaceService.CodeAction.cs 544
- V3001 There are identical sub-expressions 'member1.IsStatic' to the left and to the right of the '!=' operator. CSharpBinding AbstractImplementInterfaceService.CodeAction.cs 545
Очередная опечатка. Причём не одна, а сразу две. Опять между собой выполняется сравнение свойств одного и того же объекта ('member1'). Так как свойства примитивные и никакой дополнительной логики в них нет, то и смысл подобные проверки тоже теряют. Да и из кода видно, что должны были сравниваться свойства объектов 'member1' и 'member2'. Корректный вариант кода:
if (member1.DeclaredAccessibility != member2.DeclaredAccessibility
|| member1.IsStatic != member2.IsStatic)
Присваивание переменной самой себе
Не такой часто встречающийся тип ошибок, как предыдущий, но не менее интересный. Зачастую ошибочными являются ситуации, когда какому-то члену класса в методе необходимо присвоить значение одного из переданных аргументов, причём эти имена зачастую отличаются только регистром первого символа. При этом легко допустить ошибку. Встречаются и простые случаи присваивания переменной самой себе и если это — свойства, компилятор не будет выдавать никаких предупреждений. Такие действия понятны, если на геттер/сеттер свойства повешена сложная логика, но если её нет — присваивание выглядит как минимум странно. Но не будем голословны, лучше взглянем на примеры таких ошибок.
public ViMacro (char macroCharacter) {
MacroCharacter = MacroCharacter;
}
public char MacroCharacter {get; set;}
Предупреждение анализатора: V3005 The 'MacroCharacter' variable is assigned to itself. Mono.TextEditor ViMacro.cs 57
О чём и говорилось выше — из-за того, что имена свойства и аргумента конструктора различаются только регистром первого символа, значение свойства записывается само в себя вместо того, чтобы в него записывалось значение, переданное в качестве аргумента. Посмотрев на определение свойства можно убедиться, что никакой дополнительной логики оно не содержит.
public ViMark (char markCharacter) {
MarkCharacter = MarkCharacter;
}
public char MarkCharacter {get; set;}
Предупреждение анализатора: V3005 The 'MarkCharacter' variable is assigned to itself. Mono.TextEditor ViMark.cs 45
Ошибка точь-в-точь аналогична предыдущей. Опять перепутан первый символ в названии переменный, из-за чего конструктор работает не так, как ожидалось.
public WhitespaceNode(string whiteSpaceText,
TextLocation startLocation)
{
this.WhiteSpaceText = WhiteSpaceText;
this.startLocation = startLocation;
}
public string WhiteSpaceText { get; set; }
Предупреждение анализатора: V3005 The 'this.WhiteSpaceText' variable is assigned to itself. ICSharpCode.NRefactory.CSharp WhitespaceNode.cs 65
Ошибка вновь аналогична предыдущим, но в этот раз код более интересен тем, что в одном из двух присваиваний программист не опечатался. В ходе быстрого набора текста подобную ошибку легко пропустить, тем более, если используются средства автоматической подстановки кода. Впрочем, этого можно было бы избежать, регулярно проверяя новый код с помощью статического анализатора. Например, в PVS-Studio имеется возможность автоматической проверки нового кода после компиляции (см. режим инкрементального анализа).
void OptionsChanged (object sender, EventArgs e)
{
gutterMargin.IsVisible = Options.ShowLineNumberMargin;
iconMargin.IsVisible = iconMargin.IsVisible;
....
}
public bool IsVisible { get; set; }
Предупреждение анализатора: V3005 The 'iconMargin.IsVisible' variable is assigned to itself. MonoDevelop.HexEditor HexEditor.cs 241
Это второй тип ошибки, описанный мной в начале подраздела. Значение свойства присваивается само себе, однако нет локальных переменных, названием схожих с данным свойством. При этом на свойство, опять же, не повязано никакой дополнительной логики. Возможно, корректный код должен был бы выглядеть так, хотя здесь уже нельзя сказать наверняка:
iconMargin.IsVisible = gutterMargin.IsVisible;
Иллюзия выбора
Интересный подзаголовок, не правда ли? Однако он, пожалуй, наиболее точно описывает некоторые типы ошибок, например такие, которые обнаруживаются с помощью диагностических сообщений V3004 или V3012. Суть данного типа ошибок в том, что вне зависимости от того, будет ли проверяемое условие (V3004 для оператора 'if' и V3012 для тернарного) истинным или ложным, всегда будут выполнены одни и те же действия, или возвращён один и тот же результат. К сожалению, ошибок, диагностируемых предупреждением V3004, в проекте не нашлось, на зато нашлась парочка предупреждений V3012, которые и будут рассмотрены ниже.
public enum WindowCommands
{
NextDocument,
PrevDocument,
OpenDocumentList,
OpenWindowList,
SplitWindowVertically,
SplitWindowHorizontally,
UnsplitWindow,
SwitchSplitWindow,
SwitchNextDocument,
SwitchPreviousDocument
}
protected static void Switch (bool next)
{
if (!IdeApp.Preferences.EnableDocumentSwitchDialog) {
IdeApp.CommandService.DispatchCommand (
next ? WindowCommands.NextDocument :
WindowCommands.NextDocument);
return;
}
var toplevel = Window.ListToplevels ()
.FirstOrDefault (w => w.HasToplevelFocus)
?? IdeApp.Workbench.RootWindow;
var sw = new DocumentSwitcher (toplevel, next);
sw.Present ();
}
Предупреждение анализатора: V3012 The '?:' operator, regardless of its conditional expression, always returns one and the same value: WindowCommands.NextDocument. MonoDevelop.Ide WindowCommands.cs 254
Тернарный оператор всегда будет возвращать один и тот же элемент перечисления ('WindowCommands.NextDocument'). Предположу, что в случае, если переменная 'next' имеет значение 'false', должен был возвращаться элемент 'WindowCommands.PrevDocument'.
Опять же, у меня есть подозрение, что подобные ошибки допускаются из-за использования средств автоподстановки кода. И при быстрой работе можно вовсе не заметить, что инструмент, который должен помочь в написании кода, помог в написании ошибки. Однако это только предположения и рассуждения на эту тему выходят за рамки данной статьи.
Встретился ещё один подобный пример:
private void StartTestElement(ITestResult result)
{
ITest test = result.Test;
TestSuite suite = test as TestSuite;
if (suite != null)
{
xmlWriter.WriteStartElement("test-suite");
xmlWriter.WriteAttributeString("type", suite.TestType);
xmlWriter.WriteAttributeString("name",
suite.TestType == "Assembly" ? result.Test.FullName
: result.Test.FullName);
}
....
}
Предупреждение анализатора: V3012 The '?:' operator, regardless of its conditional expression, always returns one and the same value: result.Test.FullName. GuiUnit_NET_4_5 NUnit2XmlOutputWriter.cs 207
Как видно из фрагмента кода, истинным будет выражение 'suite.TestType == «Assembly»' или ложным, результатом выполнения тернарного оператора будет значение свойства 'FullName'.
Проверка не той переменной на равенство 'null' после приведения оператором 'as'
А это уже ситуация, специфичная для C#. Причём, судя по проверенным проектам, это некий паттерн ошибок, а не единичные случаи. Как всем нам известно, в случае, если не удалось выполнить приведение, используя оператор 'as', результатом будет значение 'null' (в отличии от явного приведения с использованием синтаксиса '(type_name)arg', когда будет сгенерировано исключение типа 'InvalidCastException'). Часто после такого приведения выполняется проверка, чтобы убедиться, что оно прошло успешно. Однако нередко допускают ошибку, проверяя по невнимательности не результат приведения, а приводимую переменную. Несколько подобных случаев будут рассмотрены ниже.
public override bool Equals (object o)
{
SolutionItemReference sr = o as SolutionItemReference;
if (o == null)
return false;
return (path == sr.path) && (id == sr.id);
}
Предупреждение анализатора: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'o', 'sr'. MonoDevelop.Core SolutionItemReference.cs 81
В данном коде выполняется приведение переменной 'o' типа 'object' к типу 'SolutionItemReference'. В случае, если такое приведение выполнить не удастся, в переменную 'sr' будет записано значение 'null'. В итоге проверка 'o == null' пройдёт успешно (естественно, если 'o' — не 'null'), а при проверке 'path == sr.path' будет сгенерировано исключение типа 'NullReferenceException'. Всего этого можно было бы избежать, проверив правильную переменную в соответствующем месте:
if (sr == null)
return false;
Ещё один пример подобного кода:
void OnTokenSelectionChanged (object sender, EventArgs args)
{
TreeSelection selection = sender as TreeSelection;
if (sender != null)
{
TreeIter iter;
TreeModel model = (TreeModel)tokensStore;
if (selection.GetSelected (out model, out iter)) {
entryToken.Text = (string)tokensStore.GetValue (iter, 0);
comboPriority.Active = (int)tokensStore.GetValue (iter, 1);
} else
{
entryToken.Text = String.Empty;
comboPriority.Active = (int)TaskPriority.Normal;
}
}
}
Предупреждение анализатора: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'sender', 'selection'. MonoDevelop.Ide TasksOptionsPanel.cs 123
Ситуация точь-в-точь аналогична предыдущей. После приведения 'sender' к 'TreeSelection' на 'null' проверяется не та переменная, из-за чего возникает опасность получить 'NullReferenceException'.
Подобные примеры кода с такими же паттернами ошибок встретились ещё 2 раза:
- V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'data', 'urlMarker'. MonoDevelop.SourceEditor MarkerOperationsHandler.cs 43
- V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'symbol', 'method'. CSharpBinding FormatStringHelper.cs 59
Повторяющиеся проверки аналогичных условий
Встречаются случаи, когда дважды проверяется одно и то же условие, при этом переменные, используемые в этих выражениях, никак не изменяются между ними. Эта ошибка может повлечь за собой куда более серьёзные последствия, чем кажется на первый взгляд. Какие именно — лучше посмотреть на реальных примерах.
public override void VisitIndexerExpression(
IndexerExpression indexerExpression)
{
....
var localResolveResult = context.Resolve(indexerExpression.Target)
as LocalResolveResult;
if (localResolveResult == null)
return;
var resolveResult = context.Resolve(indexerExpression);
if (localResolveResult == null)
return;
....
}
Предупреждение анализатора: V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless ICSharpCode.NRefactory.CSharp.Refactoring ParameterCanBeDeclaredWithBaseTypeIssue.cs 356
Из данного фрагмента кода ясно видно, что вместо проверки 'resolveResult == null' дважды выполняется проверка 'localResolveResult == null'. Это хорошо видно из вырезанного фрагмента кода. Можно ли было бы также легко найти эту ошибку, просматривая код, включающий помимо этого фрагмента и основную логику методу (здесь не приводится, чтобы не растягивать пример) — большой вопрос. В любом случае, вместо того, чтобы выйти из метода в случае, если 'resolveResult' равен 'null', мы успешно продолжаем работу, а значит, что вся последующая логика, использующая 'resolveResult' летит в тар-тарары.
Вот ещё один пример подобной оплошности:
bool TryRemoveTransparentIdentifier(....)
{
....
string nae1Name = ExtractExpressionName(ref nae1);
if (nae1Name == null)
return false;
....
string nae2Name = ExtractExpressionName(ref nae2);
if (nae1Name == null)
return false;
....
}
Предупреждение анализатора: V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless ICSharpCode.NRefactory.CSharp CombineQueryExpressions.cs 114
Вновь из-за того, что перепутали переменную для проверки, не будет осуществлён выход из цикла и возвращено правильное значение, а дальнейшая логика работы метода будет нарушена.
А вот более интересный пример, однако, содержащий такую же ошибку:
public static SW.FontWeight ToWpfFontWeight (this FontWeight value)
{
if (value == FontWeight.Thin)
return SW.FontWeights.Thin;
if (value == FontWeight.Ultralight)
return SW.FontWeights.UltraLight;
if (value == FontWeight.Light)
return SW.FontWeights.Light;
if (value == FontWeight.Semilight)
return SW.FontWeights.Light;
if (value == FontWeight.Book)
return SW.FontWeights.Normal;
if (value == FontWeight.Medium)
return SW.FontWeights.Medium;
if (value == FontWeight.Semibold)
return SW.FontWeights.SemiBold;
if (value == FontWeight.Bold)
return SW.FontWeights.Bold;
if (value == FontWeight.Ultrabold)
return SW.FontWeights.UltraBold;
if (value == FontWeight.Heavy)
return SW.FontWeights.Black;
if (value == FontWeight.Ultraheavy)
return SW.FontWeights.UltraBlack;
return SW.FontWeights.Normal;
}
Ну что, нашли? Шучу, тут можно только ткнуть пальцем в небо. А вот для анализатора никакой проблемы нет, и он спокойно справился с задачей.
Предупреждение анализатора: V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless Xwt.WPF DataConverter.cs 217
Для того, чтобы лучше понять, в чём проблема, нужно взглянуть на перечисление FontWeight.
public enum FontWeight
{
/// The thin weight (100)
Thin = 100,
/// The ultra light weight (200)
Ultralight = 200,
/// The light weight (300)
Light = 300,
/// The semi light weight (350)
Semilight = 350,
/// The book weight (380)
Book = 350,
....
}
Константы 'Semilight' и 'Book' имеют одинаковые значения, хотя из комментариев чётко видно, что константа 'Book' должна иметь значение 380.
Что более интересно — если значение 'value' будет равно 380 — этот метод всё равно будет работать верно! В этом случае ни одно из перечисленных условий не будет выполнено, а следовательно — вернётся как раз то значение, которое вернулось бы при выполнении условия 'value == FontWeight.Book'. «Не баг, а фича»
Ну и в завершение этого подраздела:
public override object GetData (TransferDataType type)
{
if (type == TransferDataType.Text)
return clipboard.WaitForText ();
if (type == TransferDataType.Text)
return clipboard.WaitForImage ();
....
}
Предупреждение анализатора: V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless Xwt.Gtk ClipboardBackend.cs 86
В данном фрагменте кода легко заметить опечатку. Вместо повтора проверки 'type == TransferDataType.Text' необходимо было выполнить проверку 'type == TransferDataType.Image'.
Проверка противоречащих условий
Встречается код, когда в пределах одного выражения одна и та же переменная проверяется на равенство/неравенство какому-либо значению. Такие проверки как минимум избыточны, а возможно — даже содержат ошибку, связанную с тем, что второй раз проверяется значение не той переменной. Такие ошибки тоже нашлись в проекте.
IEnumerable
CreateConstructorCompletionData(IType hintType)
{
....
if (!(hintType.Kind == TypeKind.Interface &&
hintType.Kind != TypeKind.Array))
....
}
Предупреждение анализатора: V3023 Consider inspecting this expression. The expression is excessive or contains a misprint. ICSharpCode.NRefactory.CSharp CSharpCompletionEngine.cs 2397
Судя по окружению кода, здесь просто переусложнили с проверкой выражения. Непонятно, к чему такое усложнение, так как всё это условие можно было бы упростить до кода следующего вида:
if (hintType.Kind != TypeKind.Interface)
Схожий случай:
void OnUpdateClicked (object s, StatusBarIconClickedEventArgs args)
{
if (args.Button != Xwt.PointerButton.Right &&
args.Button == Xwt.PointerButton.Left) {
HideAlert ();
AddinManagerWindow.Run (IdeApp.Workbench.RootWindow);
}
}
Предупреждение анализатора: V3023 Consider inspecting this expression. The expression is excessive or contains a misprint. MonoDevelop.Ide AddinsUpdateHandler.cs 97
По данному фрагменту кода видно, что здесь не подразумевалось использования других переменных для сравнения, но, тем не менее, избыточное сравнение имеет место быть. На свойство 'Button' никакой дополнительной логики не повешено, следовательно, нет никаких «подводных камней» при его чтении. Опять-таки код легко упрощается:
if (args.Button == Xwt.PointerButton.Left)
Неверно составленные строки форматирования
Нередко встречается код, содержащий ошибки в строках форматирования. Как правило, такие ошибки бывают 2-ух видов:
- Количество ожидаемых аргументов меньше количества фактических. В таком случае неиспользуемые аргументы просто будут проигнорированы. Подобная ошибка может быть признаком того, что строка составляется неверно, иначе — зачем в ней неиспользуемый аргумент? Возможно, что он остался в результате рефакторинга.
- Количество ожидаемых аргументов больше количества фактических. Более неприятный случай, так как будет сгенерировано исключение типа 'FormatException'.
В данном проекте встречались только ошибки первого типа. Пример одной из них:
ConditionExpression ParseReferenceExpression (string prefix)
{
StringBuilder sb = new StringBuilder ();
string ref_type = prefix [0] == '$' ? "a property" : "an item list";
int token_pos = tokenizer.Token.Position;
IsAtToken (TokenType.LeftParen, String.Format (
"Expected {0} at position {1} in condition \"{2}\".
Missing opening parantheses after the '{3}'.",
ref_type, token_pos, conditionStr, prefix));
....
IsAtToken (TokenType.RightParen, String.Format (
"Expected {0} at position {1} in condition \"{2}\".
Missing closing parantheses'.",
ref_type, token_pos, conditionStr, prefix));
....
}
Предупреждение анализатора: V3025 Incorrect format. A different number of format items is expected while calling 'Format' function. Expected: 3. Present: 4. MonoDevelop.Core ConditionParser.cs 254
Скорее всего, данная ошибка получилась в результате неудачного 'copy-paste', так как второй вызов метода 'IsAtToken' схож с первым, различия лишь в том, что он касается закрывающей скобки. Однако аргумент 'prefix' в нём никак не используется. Не критично, но и толку от него нет.
Подобные предупреждения:
- V3025 Incorrect format. A different number of format items is expected while calling 'Format' function. Expected: 1. Present: 2. MonoDevelop.Xml XmlFormatterWriter.cs 1131;
- V3025 Incorrect format. A different number of format items is expected while calling 'Format' function. Expected: 4. Present: 6. ICSharpCode.NRefactory.CSharp MonoSymbolTable.cs 235
- V3025 Incorrect format. A different number of format items is expected while calling 'Format' function. Expected: 1. Present: 2. MonoDevelop.Ide HelpOperations.cs 212
- V3025 Incorrect format. A different number of format items is expected while calling 'Format' function. Expected: 4. Present: 6. Mono.Cecil.Mdb MonoSymbolTable.cs 235
- V3025 Incorrect format. A different number of format items is expected while calling 'Format' function. Expected: 2. Present: 3. MonoDevelop.TextEditor.Tests ViTests.cs 255
Потенциальное разыменовывание нулевой ссылки
Часто приходится проверять переменные на равенство 'null', особенно если эта переменная — аргумент метода, результат его работы, была получена с помощью оператора 'as'. Перед её использованием необходимо удостовериться, что переменная не содержит в себе значения 'null', так как иначе, если, например, будет осуществлена попытка вызова одного из членов объекта, будет сгенерировано исключение типа 'NullReferenceException'.
Но бывают случаи, когда программист из-за невнимательности осуществляет такую проверку уже после разыменовывания ссылки. Такие случаи встретились и здесь.
void Replace (RedBlackTreeNode oldNode, RedBlackTreeNode newNode)
{
....
if (oldNode.parent.left == oldNode ||
oldNode == null && oldNode.parent.left == null)
....
}
Предупреждение анализатора: V3027 The variable 'oldNode' was utilized in the logical expression before it was verified against null in the same logical expression. MonoDevelop.HexEditor RedBlackTree.cs 167
Как видно из кода, сначала какое-то поле объекта 'oldNode.parent.left' сравнивается с самим объектом 'oldNode', а затем этот объект и поле проверяется на равенство 'null'. Однако если 'oldNode' всё же имеет значение 'null', уже при первой проверке будет сгенерировано исключение типа 'NullReferenceException'. Верным решением было бы в первую очередь выполнить проверку объекта на равенство 'null'.
Заключение
Лично я остался доволен результатом проверки, так как удалось найти достаточно интересные ошибки. Далеко не все из них были рассмотрены в этой статье. Многие диагностические сообщения были рассмотрены поверхностно, так как почти сразу стало понято, что материала для статьи более чем достаточно.
Кто-то может сказать, что для проекта такого объёма нашлось не так уж и много ошибок. Но следует помнить о том, что многие ошибки выявляются на этапе тестирования, в то время как с использованием статического анализатора они могли бы выявляться и исправляться ещё на этапе разработки, что облегчает как сам процесс написания и отладки кода, так и уменьшает общую стоимость конечного продукта.
Другие проверенные C#-проекты
Возможно, вам будет интересно почитать про проверку других C#-проектов с открытым исходным кодом. Но учтите, что некоторые из данных проектов проверялись в ходе разработки анализатора, так что сейчас при их проверке он мог бы показать лучшие результаты.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Sergey Vasiliev. Looking for Bugs in MonoDevelop.
