Повторна перевірка SharpDevelop: що нового?

Інструмент PVS-Studio постійно вдосконалюється. При цьому найбільш динамічно в даний час розвивається аналізатор C# код: у 2016 році в нього було додано дев'яносто нових діагностичних правил. Ну а найкращим показником якості роботи аналізатора є виявлені ним помилки. Завжди цікаво, а також досить корисно, проводити повторні перевірки великих відкритих проектів, порівнюючи результати. Сьогодні я зупинюся на повторній перевірці проекту SharpDevelop.

Введення
Попередня стаття про перевірку SharpDevelop була опублікована Андрієм Карповим в листопаді 2015 року. В той час ми тільки тестували новий C# аналізатор, готуючись до випуску першого релізу. Тим не менш, вже тоді, за допомогою бета-версії, Андрію вдалося виявити в проекті SharpDevelop кілька цікавих помилок. Після цього SharpDevelop був поміщений на полицю» і використовувався разом з іншими проектами тільки для внутрішнього тестування при розробці нових діагностичних правил. І ось, нарешті, настав час перевірити SharpDevelop ще раз, але вже більш «мускулистими» версією аналізатора PVS-Studio 6.12.

Для перевірки я завантажив актуальну версію вихідного коду SharpDevelop з порталу GitHub. Проект містить близько мільйона рядків коду на мові C#. У процесі роботи аналізатор видав 809 попереджень. З них першого рівня — 74, другого — 508, третього — 227:


Не будемо розглядати запобігання на рівні Low: статистично там великий відсоток помилкових спрацьовувань. Аналіз попереджень на рівнях Meduim і High (582 попередження) виявив наявність близько 40% помилкових, або вкрай підозрілих конструкцій. Це становить 233 попередження. Іншими словами, аналізатор PVS-Studio в середньому знайшов 0.23 помилки на 1000 рядків коду. Це говорить про високу якість коду проекту SharpDevelop. На багатьох інших проектах все виглядає куди більш сумно.

У результаті повторної перевірки була виявлена частина помилок, описаних раніше Андрієм в його статті. Але більша частина знайдених помилок — нові. Далі я наведу найбільш цікаві з них.

Результати перевірки
Еталонна помилка Copy-Paste

Помилка, яка по праву може зайняти почесне місце в "палаті мір і ваг". Яскрава ілюстрація корисності використання статичного аналізу коду і небезпеки Copy-Paste.

Попередження аналізатора PVS-Studio: V3102 Suspicious access to element of 'method.SequencePoints' object by a constant index inside a loop. CodeCoverageMethodTreeNode.cs 52

public override void ActivateItem()
{
if (method != null && method.SequencePoints.Count > 0) {
CodeCoverageSequencePoint firstSequencePoint = 
method.SequencePoints[0];
....
for (int i = 1; i < method.SequencePoints.Count; ++i) {
CodeCoverageSequencePoint sequencePoint = 
method.SequencePoints[0]; // <=
....
}
....
}
....
}

На кожній ітерації циклу for використовують доступ до нульового елемента колекції. Я навмисне навів додатковий фрагмент коду, що знаходиться відразу після умови if. Легко помітити, звідки була скопійована рядок, поміщена всередину циклу. Ім'я змінної firstSequencePoint замінили на sequencePoint, а от вираз доступу за індексом змінити забули. Правильний варіант даної конструкції має вигляд:

public override void ActivateItem()
{
if (method != null && method.SequencePoints.Count > 0) {
CodeCoverageSequencePoint firstSequencePoint = 
method.SequencePoints[0];
....
for (int i = 1; i < method.SequencePoints.Count; ++i) {
CodeCoverageSequencePoint sequencePoint = 
method.SequencePoints[i];
....
}
....
}
....
}

«Знайдіть 10 відмінностей», або знову Copy-Paste

Попередження аналізатора PVS-Studio: 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 NamespaceTreeNode.cs 87

public int Compare(SharpTreeNode x, SharpTreeNode y)
{
....
if (typeNameComparison == 0) {
if (x.Text.ToString().Length < y.Text.ToString().Length) // <=
return -1;
if (x.Text.ToString().Length < y.Text.ToString().Length) // <=
return 1;
} 
....
}

Обидва блоки if містять однакову умова. В даному випадку важко судити про те, як повинен виглядати правильний варіант програми. Необхідно вивчення фрагмента коду його автором.

Несвоєчасна перевірка на рівність null

Попередження аналізатора PVS-Studio: V3095 The 'position' object was used before it was verified against null. Check lines: 204, 206. Task.cs 204

public void JumpToPosition()
{
if (hasLocation && !position.IsDeleted) // <=
....
else if (position != null)
....
}

Змінну position використовують, не перевіривши на рівність null. Перевірка проводиться в іншому умови, в блоці коду else. Правильний варіант коду міг би мати вигляд:

public void JumpToPosition()
{
if (hasLocation && position != null && !position.IsDeleted)
....
else if (position != null)
....
}

Пропущена перевірка на рівність null

Попередження аналізатора PVS-Studio: V3125 The 'mainAssemblyList' object was used after it was verified against null. Check lines: 304, 291. ClassBrowserPad.cs 304

void UpdateActiveWorkspace()
{
var mainAssemblyList = SD.ClassBrowser.MainAssemblyList;
if ((mainAssemblyList != null) && (activeWorkspace != null) {
....
}
....
mainAssemblyList.Assemblies.Clear(); // <=
....
}

Змінну mainAssemblyList використовують без попередньої перевірки на рівність null. При цьому інший фрагмент коду містить таку перевірку. Коректний варіант коду:

void UpdateActiveWorkspace()
{
var mainAssemblyList = SD.ClassBrowser.MainAssemblyList;
if ((mainAssemblyList != null) && (activeWorkspace != null) {
....
}
....
if (mainAssemblyList != null) {
mainAssemblyList.Assemblies.Clear();
} 
....
}

Несподіваний результат сортування

Попередження аналізатора PVS-Studio: V3078 Original sorting order will be lost after repetitive call to 'OrderBy' method. Use 'ThenBy' method to preserve the original sorting. CodeCoverageMethodElement.cs 124

void Init()
{
....
this.SequencePoints.OrderBy(item => item.Line)
.OrderBy(item => item.Column); // <=
}

Результатом роботи даного фрагмента коду буде сортування колекції SequencePoints тільки по полю Column. По всій видимості, це не зовсім те, що очікував автор коду. Проблема полягає в тому, що повторний виклик методу OrderBy виконає сортування колекції без урахування результату попередньої сортування. Для виправлення ситуації необхідно використовувати ThenBy замість повторного виклику OrderBy:

void Init()
{
....
this.SequencePoints.OrderBy(item => item.Line)
.ThenBy(item => item.Column);
}

Потенційна можливість ділення на нуль

Попередження аналізатора PVS-Studio: V3064 Potential division by zero. Consider inspecting denominator 'workAmount'. XamlSymbolSearch.cs 60

public XamlSymbolSearch(IProject project, ISymbol entity)
{
....
interestingFileNames = new List<FileName>();
....
foreach (var item in ....)
interestingFileNames.Add(item.FileName);
....
workAmount = interestingFileNames.Count;
workAmountInverse = 1.0 / workAmount; // <=
}

У разі, якщо колекція interestingFileNames виявиться порожньою, відбудеться ділення на нуль. Тут досить важко запропонувати підходящий варіант виправлення помилки. Але, в будь-якому випадку, потрібно доопрацювання алгоритму обчислення значення змінної workAmountInverse в ситуації, коли змінна workAmount дорівнюватиме нулю.

Повторне призначення

Попередження аналізатора PVS-Studio: V3008 The 'ignoreDialogIdSelectedInTextEditor' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 204, 201. WixDialogDesigner.cs 204

void OpenDesigner()
{
try {
ignoreDialogIdSelectedInTextEditor = true; // <=
WorkbenchWindow.ActiveViewContent = this;
} finally {
ignoreDialogIdSelectedInTextEditor = false; // <=
}
}

Змінна ignoreDialogIdSelectedInTextEditor отримає значення false незалежно від результату виконання блоку try. Для виключення вірогідності наявності «підводних каменів» ознайомимося з оголошенням використовуваних змінних. Оголошення ignoreDialogIdSelectedInTextEditor має вигляд:

bool ignoreDialogIdSelectedInTextEditor;

Оголошення IWorkbenchWindow ActiveViewContent:

public IWorkbenchWindow WorkbenchWindow {
get { return workbenchWindow; }
}
IViewContent ActiveViewContent {
get;
set;
}

Як бачимо, немає ніяких очевидних причин для повторного присвоєння змінній ignoreDialogIdSelectedInTextEditor значення. Ризикну припустити, що коректний варіант наведеної конструкції міг би відрізнятися від оригіналу використанням ключового словаcatch замість finally:

void OpenDesigner()
{
try {
ignoreDialogIdSelectedInTextEditor = true;
WorkbenchWindow.ActiveViewContent = this;
} catch {
ignoreDialogIdSelectedInTextEditor = false;
}
}

Помилковий пошук підрядка

Попередження аналізатора PVS-Studio: V3053 An excessive expression. Examine the substrings '/debug' and '/debugport'. NDebugger.cs 287

public bool IsKernelDebuggerEnabled {
get {
....
if (systemStartOptions.Contains("/debug") ||
systemStartOptions.Contains("/crashdebug") ||
systemStartOptions.Contains("/debugport")|| // <=
systemStartOptions.Contains("/baudrate")) {
return true;
}
....
}
}

У рядку systemStartOptions проводиться послідовний пошук однією з підрядків "/debug" або "/debugport". Проблема полягає в тому, що рядок "/debug" сама є підрядком для "/debugport". Таким чином, знаходження підрядка "/debug" робить подальший пошук підрядка "/debugport" безглуздим. Мабуть, це не помилка, але код можна спростити:

public bool IsKernelDebuggerEnabled {
get {
....
if (systemStartOptions.Contains("/debug") ||
systemStartOptions.Contains("/crashdebug") ||
systemStartOptions.Contains("/baudrate")) {
return true;
}
....
}
}

Помилка обробки виключення

Попередження аналізатора PVS-Studio: V3052 The original exception object 'ex' was swallowed. Stack of original exception could be lost. ReferenceFolderNodeCommands.cs 130

DiscoveryClientProtocol DiscoverWebServices (....)
{
try {
....
} catch (WebException ex) {
if (....) {
....
} else {
throw ex; // <=
}
}
....
}

В даному випадку виклик throw ex призведе до «затирання» стека оригінального винятку, так як перехоплене виняток буде згенеровано повторно. Виправлений варіант:

DiscoveryClientProtocol DiscoverWebServices (....)
{
try {
....
} catch (WebException ex) {
if (....) {
....
} else {
throw;
}
}
....
}

Використання неинициализированного поля в конструкторі класу

Попередження аналізатора PVS-Studio: V3128 The 'contentPanel' field is used before it is initialized in constructor. SearchResultsPad.cs 66

Grid contentPanel;
public SearchResultsPad()
{
....
defaultToolbarItems = ToolBarService
.CreateToolBarItems(contentPanel, ....); // <=
....
contentPanel = new Grid {....};
....
}

Поле contentPanel передається в якості одного з параметрів метод CreateToolBarItems в конструкторі класу SearchResultsPad. При цьому ініціалізація даного поля проводиться вже після використання. Можливо, в даному випадку помилки немає, так як в тілі методу CreateToolBarItems і далі по стеку може бути врахована можливість рівності null змінної contentPanel. Але код виглядає підозріло і вимагає перевірки автором.

Ще раз підкреслю, що я описав у статті далеко не всі помилки, які виявив аналізатор PVS-Studio, а тільки ті, що здалися мені цікавими. Автори проекту можуть звернутися до нас і отримати тимчасовий ліцензійний ключ, щоб виконати більш ретельну перевірку проекту.

Висновок
І знову PVS-Studio не підвів: в ході повторної перевірки проекту SharpDevelop були знайдені нові цікаві помилки. А це означає, що аналізатор відмінно справляється зі своєю роботою, дозволяючи робити світ навколо нас трохи більш досконалим.

Нагадую, що ви можете в будь-який час включитися в цей процес, скориставшись можливістю безкоштовного статичного аналізатора PVS-Studio для перевірки власних проектів.

Завантажити та спробувати PVS-Studio: http://www.viva64.com/ru/pvs-studio/

З питань придбання комерційної ліцензії просимо Вас зв'язатися з нами поштою. Ви також можете написати нам, щоб отримати тимчасову ліцензію для всебічного вивчення PVS-Studio, якщо хочете зняти обмеження демонстраційної версії.



Якщо хочете поділитися цією статтею з англомовної аудиторією, то прошу використовувати посилання на переклад: Sergey Khrenov. Rechecking SharpDevelop: Any New Bugs?

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

0 коментарів

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