Ищем и анализируем ошибки в коде GitExtensions
- GitExtensions
- Результаты проверки
- Выражение всегда истинно
- Одно из сравнений избыточно
- Переменная используется перед проверкой на null
- Возможно возникновение исключения NullReferenceException в переопределенном методе Equals
- Идентичные выражения в условии if
- Несоответствие количества параметров при форматировании строки
- Часть выражения всегда истинна
- Заключение
Как известно Ядро Git представляет собой набор утилит командной строки с параметрами. Для комфортной работы как правило используются утилиты, дающие нам привычный графический интерфейс. Вот и мне довелось в свое время поработать с такой утилитой для Git, как GitExtensions. Не скажу, что это самый удобный инструмент, которым мне доводилось пользоваться (к примеру, тот же TortoiseGit мне нравится больше), но он явно и не безосновательно занимает свою нишу в списке любимых и проверенных утилит для работы с Git.
Недавно я вспомнил про него, и решил проверить статическим анализатором на наличие ошибок и опечаток его исходный код. О том, какие ошибки были найдены после проверки я расскажу в этой статье.
GitExtensions
GitExtensions - кроссплатформенный визуальный клиент для работы с системой управления версиями Git с открытым исходным кодом.
Проект GitExtensions небольшой. В сумме это 10 основных проектов, 20 плагинов и 2 дополнительных проектов, компилируемых в вспомогательные библиотеки. В общей сложности 56 091 строк кода в 441 файле.
Давайте посмотрим, сможет ли что-то интересное найти в этом проекте статический анализатор PVS-Studio.
Результаты проверки
По итогам проверки было получено 121 предупреждение. Если рассматривать более подробно, то 16 предупреждений было получено на первом (высоком) уровне. 11 из них явно указывали на проблемные места или ошибки. На втором уровне (среднем) было получено 80 предупреждений. По моему субъективному мнению, 69 предупреждений были верными и указывали на места с ошибками или опечатками. Третий (низкий) уровень предупреждений мы рассматривать не будем, так как он в основном указывает на места, где возникновение ошибок маловероятно. И так, приступим к рассмотрению найденных ошибок.
Выражение всегда истинно
Начинает наш хит-парад довольно странный участок кода.
string rev1 = "";
string rev2 = "";
var revisions = RevisionGrid.GetSelectedRevisions();
if (revisions.Count > 0)
{
rev1 = ....;
rev2 = ....;
....
}
else
if (string.IsNullOrEmpty(rev1) || string.IsNullOrEmpty(rev2)) // <=
{
MessageBox.Show(....);
return;
}V3022 Expression 'string.IsNullOrEmpty(rev1) || string.IsNullOrEmpty(rev2)' is always true. GitUI FormFormatPatch.cs 155
Анализатор выдал предупреждение, что выражение с проверкой переменных rev1 и rev2 всегда будет истинным. Сначала я подумал, что это обычная опечатка, оплошность в логике алгоритма, которая никак не повлияет на корректность работы программы. Но рассмотрев код более детально заметил, по всей видимости лишний оператор else. Он находится перед второй проверкой, которая будет выполнена только в случае неравенства первой.
Одно из сравнений избыточно
Вторым номером в нашем хит-параде идет обычная опечатка. Она никак не влияет на логику работы программы, но данный пример хорошо показывает насколько полезен статический анализ.
public void EditNotes(string revision)
{
string editor = GetEffectivePathSetting("core.editor").ToLower();
if (editor.Contains("gitextensions") ||
editor.Contains("notepad") || // <=
editor.Contains("notepad++")) // <=
{
RunGitCmd("notes edit " + revision);
}
....
}V3053 An excessive expression. Examine the substrings 'notepad' and 'notepad++'. GitCommands GitModule.cs 691
В выражении ищется более длинная (notepad++) и более короткая (notepad) подстроки. При этом проверка с более длинной строкой никогда не сработает, так как проверка с поиском более короткой строки будет ее предотвращать. Как я уже упомянул, это не ошибка, а всего лишь опечатка, но в другой ситуации невинная избыточная проверка могла бы превратиться в потенциально коварный баг.
Переменная используется перед проверкой на null
Третье место занимает довольно распространённая ошибка, но сказать о том, что она в данном случае 100% вызовет некорректную работу программы я сказать не могу, так как не вникал слишком глубоко в логику работы данного кода. Лишь тот факт, что переменная проверяется на null может говорить о предполагаемом нулевом значении.
void DataReceived(string data)
{
if (data.StartsWith(CommitBegin)) // <=
{
....
}
if (!string.IsNullOrEmpty(data))
{
....
}
}V3095 The 'data' object was used before it was verified against null. Check lines: 319, 376. GitCommands RevisionGraph.cs 319
Переменная data используется перед проверкой на null, что потенциально может привести к возникновению исключения NullReferenceException. Если же переменная data никогда не является нулевой (null), то расположенная ниже проверка бесполезна и её следует удалить, чтобы она не вводила в заблуждение.
Возможно возникновение исключения NullReferenceException в переопределенном методе Equals
Данная ошибка во многом напоминает предыдущую. Если сравнивать два объекта используя переопределенный метод Equals, всегда есть вероятность, что в качестве второго объекта сравнения придет null.
public override bool Equals(object obj)
{
return GetHashCode() == obj.GetHashCode(); // <=
}V3115 Passing 'null' to 'Equals(object obj)' method should not result in 'NullReferenceException'. Git.hub User.cs 16
При дальнейшем вызове переопределенного метода Equals возможно возникновение исключения NullReferenceException, если параметр obj будет равен null. Для предотвращения данной ситуации необходимо использовать проверку на null. Например, так:
public override bool Equals(object obj)
{
return GetHashCode() == obj?.GetHashCode(); // <=
}Идентичные выражения в условии if
Под пятым номером гордо расположились 2 опечатки. Они не влияют на результат выполнения программы, но их мы можем отнести к разряду избыточных проверок.
private void ConfigureRemotes()
{
....
if (!remoteHead.IsRemote ||
localHead.IsRemote ||
!string.IsNullOrEmpty(localHead.GetTrackingRemote(localConfig)) ||
!string.IsNullOrEmpty(localHead.GetTrackingRemote(localConfig)) ||
remoteHead.IsTag ||
localHead.IsTag ||
!remoteHead.Name.ToLower().Contains(localHead.Name.ToLower()) ||
!remoteHead.Name.ToLower().Contains(_remote.ToLower()))
continue;
....
}V3001 There are identical sub-expressions to the left and to the right of the '||' operator. GitUI FormRemotes.cs 192
Если посмотреть внимательно, то можно заметить 2 одинаковых условия в проверке. Вероятнее всего это следствие копипасты. Так же есть вероятность ошибки, если учесть, что во втором выражении подразумевалось использование переменной remoteHead вместо localHead, но без глубокого анализа алгоритма работы программы сказать тяжело.
Так же была найдена еще одна подобная ошибка.
if (!curItem.IsSourceEqual(item.NeutralValue) && // <=
(!String.IsNullOrEmpty(curItem.TranslatedValue) &&
!curItem.IsSourceEqual(item.NeutralValue))) // <=
{
curItem.TranslatedValue = "";
}V3001 There are identical sub-expressions to the left and to the right of the '&&' operator. TranslationApp TranslationHelpers.cs 112
Несоответствие количества параметров при форматировании строки
Шестое место достается довольно распространённой ошибке, которая возникает в следствие невнимательности программистов при рефакторинге текста форматируемых строк.
Debug.WriteLine("Loading plugin...", pluginFile.Name); // <=V3025 Incorrect format. A different number of format items is expected while calling 'WriteLine' function. Arguments not used: pluginFile.Name. GitUI LoadPlugins.cs 35
В данной ситуации же могу предположить, что программист думал, что значение переменной pluginFile.Name будет добавлено в конец форматируемой строки при выводе в дебаг, но это не так. Корректный код будет выглядеть так:
Debug.WriteLine("Loading '{0}' plugin...", pluginFile.Name); // <=Часть выражения всегда истинна
И в завершение еще одна опечатка, которой можно было бы избежать.
private void toolStripButton(...)
{
var button = (ToolStripButton)sender;
if (!button.Enabled)
{
StashMessage.ReadOnly = true;
}
else if (button.Enabled && button.Checked) // <=
{
StashMessage.ReadOnly = false;
}
}V3063 A part of conditional expression is always true if it is evaluated: button.Enabled. GitUI FormStash.cs 301
Поскольку мы проверяем, что button.Enabled равен false, то в else данной проверки мы можем смело утверждать, что button.Enabled будет равен true и таким образом исключить данную проверку повторно.
Заключение
В данном проекте были найдены и другие ошибки, опечатки, недочёты. Но мне они не показались интересными, чтобы описывать их в статье. Разработчики GitExtensions легко смогут найти все недочёты, воспользовавшись инструментом PVS-Studio. Вы так же можете поискать ошибки в своих проектах воспользовавшись предложенным выше статическим анализатором.
Хочу напомнить, что основная польза от статического анализа заключается в регулярном его использовании. Скачать и разово проверить код, это несерьезно. Например, предупреждения компилятора программисты смотрят регулярно, а не включают их раз в 3 года перед одним из релизов. При регулярном использовании статический анализатор сэкономит массу времени на поиск опечаток и ошибок.
0