Шукаємо і аналізуємо помилки в коді GitExtensions


Як изестно Ядро 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 переопределенном методі Дорівнює.
Дана помилка в чому нагадує попередню. Якщо порівнювати два об'єкта використовуючи перевизначено метод Дорівнює, завжди є ймовірність, що в якості другого об'єкта порівняння прийде 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

При подальшому виклик переопределенного методу Дорівнює можливо виникнення виключення 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 format of 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 роки перед одним з релізів. При регулярному використанні статичний аналізатор заощадить масу часу на пошук помилок і помилок.


Якщо хочете поділитися цією статтею з англомовної аудиторією, то прошу використовувати посилання на переклад: Ivan Kishchenko. GitExtensions bugs found and analyzed.

Прочитали статтю і є питання?Часто до наших статей задають одні і ті ж питання. Відповіді на них ми зібрали тут: Відповіді на питання читачів статей про PVS-Studio, версія 2015. Будь ласка, ознайомтеся зі списком.

Джерело: Хабрахабр

0 коментарів

Тільки зареєстровані та авторизовані користувачі можуть залишати коментарі.