Для проверки качества диагностик нашего статического анализатора и его рекламы мы регулярно анализируем проекты с открытым исходным кодом. Разработчики проекта FlashDevelop сами попросили нас проверить их продукт, что мы с радостью и сделали.
FlashDevelop - популярная среда разработки Flash-приложений, поддерживающая Action Script версии 2 и 3, Haxe, JavaScript, HTML, PHP, C#, и обладающая функционалом, присущим современным редакторам кода, например, автодополнение кода, встроенная поддержка svn, git, mercurial, шаблоны, сторонние плагины, темы подсветки синтаксиса и многое другое. Примечательно, что FlashDevelop использовали Fireaxis Games при разработке XCOM: Enemy Unknown.
Учитывая то, что FlashDevelop - продукт с открытым исходным кодом, и написан на языке C#, мы захотели проверить его нашим анализатором. Для анализа был использован статический анализатор PVS-Studio v6.05. Поскольку разбирать все найденные проблемные места в рамках этой статьи не представляется возможным, рассмотрим наиболее интересные сообщения анализатора.
Как известно, строки в C# - иммутабельные объекты, и методы, отвечающие за изменение строки, на самом деле возвращают новый объект типа String, сохраняя изначальную строку неизменной. Однако, как показывает практика, разработчики забывают про эту особенность. Например, анализатор обнаружил следующие ошибки:
V3010 The return value of function 'Insert' is required to be utilized. ASPrettyPrinter.cs 1263
public void emit(IToken tok)
{
....
lineData.Insert(0, mSourceData.Substring(prevLineEnd,
((CommonToken)t).StartIndex - prevLineEnd));
....
}V3010 The return value of function 'Insert' is required to be utilized. MXMLPrettyPrinter.cs 383
private void prettyPrint(....)
{
....
while (aToken.Line == currentLine)
{
lineData.Insert(0, aToken.Text);
....
}
....
}Вероятно, разработчик имел в виду такую конструкцию:
lineData = lineData.Insert(....);Другой пример срабатывания диагностики V3010:
V3010 The return value of function 'NextDouble' is required to be utilized. ASFileParser.cs 196
private static string getRandomStringRepl()
{
random.NextDouble();
return "StringRepl" + random.Next(0xFFFFFFF);
}Этот код не содержит ошибки с точки зрения фунционала, тем не менее, вызов random.NextDouble() не несет никакой смысловой нагрузки и может быть удален.
Стандартной практикой после операции приведения типа является проверка полученного значения на null на случай, если исходный тип не может быть приведен к желаемому. При выполнении такой рутинной операции разработчик может быть невнимательным и проверить не ту переменную. Наш анализатор не устаёт и внимательно следит за такими вещами:
V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'item', 'val'. WizardHelper.cs 67
public static void SetControlValue(....)
{
....
string val = item as string;
if (item == null) continue;
....
}Очевидно, что в этом примере на null следует проверять переменную val, а не item, и код должен выглядеть следующим образом:
string val = item as string;
if (val == null) continue;Когда в коде встречаются методы с одинаковыми телами, это всегда вызывает подозрения. В лучшем случае, такой код требует рефакторинга, а в худшем - механический копипаст искажает логику работы программы. Чтобы не быть голословным, рассмотрим следующие примеры.
V3013 It is odd that the body of 'SuspendMdiClientLayout' function is fully equivalent to the body of 'PerformMdiClientLayout' function (377, line 389). DockPanel.MdiClientController.cs 377
private void SuspendMdiClientLayout()
{
if (GetMdiClientController().MdiClient != null)
GetMdiClientController().MdiClient.PerformLayout(); // <=
}
private void PerformMdiClientLayout()
{
if (GetMdiClientController().MdiClient != null)
GetMdiClientController().MdiClient.PerformLayout();
}Как мы видим, тела методов SuspendMdiClientLayout и PerformMdiClientLayout абсолютно идентичны. Вероятно, это произошло из-за копирования кода. Название метода SuspendMdiClientLayout предполагает, что он отвечает за приостановку лэйаута, однако вместо этого выполняется перерисовка лэйаута: MdiClient.PerformLayout(). Я предполагаю, что корректная реализация этого метода должна быть такой:
private void SuspendMdiClientLayout()
{
if (GetMdiClientController().MdiClient != null)
GetMdiClientController().MdiClient.SuspendLayout(); // <=
}Другой пример. В проекте реализован тип Lexer, предназначенный для лексического разбора чего-то. В этом типе реализовано 28 однотипных методов с сигнатурами вида private static bool StateXX (FsmContext ctx), где XX находится в диапазоне от 1 до 28. Нет ничего удивительного в том, что при выполнении такого объема рутинной работы глаз разработчика может замылиться, что привело появлению в коде ошибки, на которую анализатор PVS-Studio реагирует следующим образом:
V3013 It is odd that the body of 'State11' function is fully equivalent to the body of 'State15' function (532, line 589). Lexer.cs 532
private static bool State11 (FsmContext ctx)
{
ctx.L.GetChar ();
switch (ctx.L.input_char) {
case 'e':
ctx.Return = true;
ctx.NextState = 1;
return true;
default:
return false;
}
}
private static bool State15 (FsmContext ctx)
{
ctx.L.GetChar ();
switch (ctx.L.input_char) {
case 'e':
ctx.Return = true;
ctx.NextState = 1;
return true;
default:
return false;
}
}Тот факт, что два метода обрабатывают одну и ту же ситуацию, кажется весьма подозрительным. Неясно, как исправить эту проблему, логика работы известна только автору. Также я очень сомневаюсь, что эта проблема легко могла быть обнаружена во время проведения code review, ведь читать большое количество монотонного кода гораздо труднее, чем его писать. С другой стороны, здесь хорошо помогают статические анализаторы.
При дальнейшем анализе был обнаружен такой интересный момент:
V3020 An unconditional 'break' within a loop. AirWizard.cs 1760
private void ExtensionBrowseButton_Click(....)
{
....
foreach (var existingExtension in _extensions)
{
if (existingExtension.ExtensionId
== extensionId) extension = existingExtension;
break;
}
....
}Рискну предположить, что разработчик хотел пробежать по элементам коллекции _extensions, найти первый объект existingExtension с соответствующим extensionId и выйти из цикла. Но из-за экономии на скобках цикл безусловно завершается после первой итерации, что существенно влияет на логику работы программы.
Другой распространенный источник ошибок - это условные выражения. Если выражение включает большое количество переменных, граничных значений, достаточно сложное ветвление, - вероятность совершения ошибки увеличивается. Рассмотрим, например, такой условный оператор:
private void SettingChanged(string setting)
{
if (setting == "ExcludedFileTypes"
|| setting == "ExcludedDirectories"
|| setting == "ShowProjectClasspaths"
|| setting == "ShowGlobalClasspaths"
|| setting == "GlobalClasspath")
{
Tree.RebuildTree();
}
else if (setting == "ExecutableFileTypes")
{
FileInspector.ExecutableFileTypes =
Settings.ExecutableFileTypes;
}
else if (setting == "GlobalClasspath") // <=
{
// clear compile cache for all projects
FlexCompilerShell.Cleanup();
}
}Статический анализатор PVS-Studio сообщает о следующей ошибке:
V3022 Expression 'setting == "GlobalClasspath"' is always false. PluginMain.cs 1194
Действительно, условие else if (setting == "GlobalClasspath") не будет выполнено никогда, потому что это же условие присутствует в самом первом if. А ведь от выполнения этого условия зависит выполнение какой-то логики. Чтобы упростить читабельность этого метода, я бы переписал его с использованием оператора switch.
Следующий пример никогда не выполнимого условия:
V3022 Expression 'high == 0xBF' is always false. JapaneseContextAnalyser.cs 293
protected override int GetOrder(byte[] buf, int offset,
out int charLen)
{
byte high = buf[offset];
//find out current char's byte length
if (high == 0x8E || high >= 0xA1 && high <= 0xFE)
charLen = 2;
else if (high == 0xBF)
charLen = 3;
....
}Анализатор сообщает нам, что выражение 'high == 0xBF' всегда ложно. Это действительно так, потому что значение 0xBF попадает в диапазон high >= 0xA1 && high <= 0xFE, проверяемый в первом if.
Еще один пример сообщения от диагностики V3022:
V3022 Expression '!Outline.FlagTestDrop' is always true. DockPanel.DockDragHandler.cs 769
private void TestDrop()
{
Outline.FlagTestDrop = false;
....
if (!Outline.FlagTestDrop)
{
....
}
....
}В этом фрагменте мы видим, что поле Outline.FlagTestDrop, которому присвоено значение false и которое дальше в коде не изменяется, используется в условном операторе if. Возможно, в этом методе не был реализован функционал, изменяющий значение этого поля, ведь для чего-то же разработчик реализовал проверку if (!Outline.FlagTestDrop).
В практике постоянно возникает необходимость проверить переменную на null, например, после приведения типа, выборке элемента из коллекции и т.д. В таких ситуациях мы проверяем, что полученная переменная не равна null, и только потом используем. Однако, как показывает наша практика, разработчик может начать сразу использовать полученный объект, и только потом проверить, что он не равен null. О таких ошибках сообщает диагностика V3095:
V3095 The 'node' object was used before it was verified against null. Check lines: 364, 365. ProjectContextMenu.cs 364
private void AddFolderItems(MergableMenu menu, string path)
{
....
DirectoryNode node = projectTree.SelectedNode
as DirectoryNode;
if (node.InsideClasspath == node)
menu.Add(RemoveSourcePath, 2, true);
else if (node != null && ....)
{
menu.Add(AddSourcePath, 2, false);
}
....
}В этом примере поле projectTree.SelectedNode имеет тип GenericNode, который является базовым типом для DirectoryNode. Приведение объекта базового типа к производному типу может быть неуспешным, и в результате переменная node может содержать пустую ссылку. Однако, как мы видим, после операции приведения типа разработчик сразу обращается к полю node.InsideClasspath, и только потом проверяет переменную node на null. Такая реализация может привести к возникновению NullReferenceException.
Анализатор выявил такое потенциально проблемное место в коде:
V3061 Parameter 'b' is always rewritten in method body before being used. InBuffer.cs 56
public bool ReadByte(byte b) // check it
{
if (m_Pos >= m_Limit)
if (!ReadBlock())
return false;
b = m_Buffer[m_Pos++]; // <=
return true;
}Значение переданного в этот метод аргумента b не используется, потом перезаписывается, и все равно далее не используется. Можно предположить, что этот метод реализован не так, как задумано (на это намекает и комментарий "// check it"). Возможно, сигнатура этого метода должна выглядеть следующим образом:
public bool ReadByte(ref byte b)
{
....
}Следующее подозрительное место, найденное анализатором, не так просто заметить при проведении code review:
V3066 Possible incorrect order of arguments passed to '_channelMixer_OVERLAY' method: 'back' and 'fore'. BBCodeStyle.cs 302
private static float _channelMixer_HARDLIGHT(float back,
float fore)
{
return _channelMixer_OVERLAY(fore, back);
}Метод _channelMixer_OVERLAY имеет такую сигнатуру:
static float _channelMixer_OVERLAY(float back, float fore)Возможно именно так и задумано. Однако есть вероятность, что при обращении к этому методу аргументы fore и back были перепутаны местами. И анализатор помогает проверить такие места.
Диагностика V3083 предназначена для обнаружения потенциально небезопасных вызовов обработчиков событий. В анализируемом проекте эта диагностика выявила большое количество таких мест. Разберем ситуацию небезопасного вызова обработчика на конкретном примере:
V3083 Unsafe invocation of event 'OnKeyEscape', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. QuickFind.cs 849
protected void OnPressEscapeKey()
{
if (OnKeyEscape != null) OnKeyEscape();
}На первый взгляд, код кажется абсолютно корректным: если поле OnKeyEscape не равно null, вызываем это событие. Однако использовать такой подход не рекомендуется. Допустим, что у события OnKeyEscape один подписчик, и допустим, что после проверки этого поля на null этот подписчик отписался от этого события (в другом потоке, например). После того, как у события не осталось подписчиков, поле OnKeyEscape будет содержать пустую ссылку, и попытка вызвать событие приведет в возникновению NullReferenceException.
Особенно неприятно, что это крайне трудновоспроизводимая ошибка. Пользователь может пожаловаться, что нажатие ESC привело в ошибке. Однако даже нажав ESC тысячу раз, вряд ли программисту удастся понять, что не так.
Обезопасить вызов события можно, объявив дополнительную промежуточную переменную:
var handler = OnKeyEscape
if (handler != null) handler();В C# версии 6 появился оператор проверки на null (?.), позволяющий значительно упростить код:
OnKeyEscape?.Invoke();Эвристические возможности нашего анализатора позволяют обнаруживать весьма интересные подозрительные места в коде. Например:
V3056 Consider reviewing the correctness of 'a1' item's usage. LzmaEncoder.cs 225
public void SetPrices(....)
{
UInt32 a0 = _choice.GetPrice0();
UInt32 a1 = _choice.GetPrice1();
UInt32 b0 = a1 + _choice2.GetPrice0();
UInt32 b1 = a1 + _choice2.GetPrice1();
....
}Вполне вероятно, что этот код был написан методом копипаста. И мне кажется, что для вычисления значения переменной b0 должна использоваться переменная a0 вместо a1. В любом случае, найденное подозрительное место должно послужить поводом для разработчиков внимательно посмотреть на этот код. И вообще, лучше использовать более информативные имена переменных.
В коде было обнаружено несколько мест, в которых перехваченное исключение пробрасывается дальше. Реализовано это, например, следующим образом:
public void Copy(string fromPath, string toPath)
{
....
try
{
....
}
catch (UserCancelException uex)
{
throw uex;
}
....
}Анализатор при проверке этого метода выдает сообщение:
V3052 The original exception object 'uex' was swallowed. Stack of original exception could be lost. FileActions.cs 598
Такое пробрасывание исключения приводит к тому, что оригинальный стек вызовов затирается новым, начинающимся с текущего метода. Это сильно затруднит поиск места, в котором возникло оригинальное исключение, при отладке.
Чтобы сохранить оригинальный стек вызовов при повторном генерации исключений, нужно просто использовать оператор throw:
try
{
....
}
catch (UserCancelException uex)
{
throw;
}Дальнейший анализ кода выявил потенциально небезопасное место:
V3087 Type of variable enumerated in 'foreach' is not guaranteed to be castable to the type of collection's elements. VS2005DockPaneStrip.cs 1436
private void WindowList_Click(object sender, EventArgs e)
{
....
List<Tab> tabs = new List<Tab>(Tabs);
foreach (TabVS2005 tab in tabs)
....
}Коллекция tabs содержит элементы типа Tab, в то время как при итерации элементы этой коллекции приводятся к типу TabVS2005, который является наследником типа Tab. Это приведение небезопасно и может привести к возникновению исключения System.InvalidCastException.
Эта же диагностика обнаружила похожий небезопасный код:
public int DocumentsCount
{
get
{
int count = 0;
foreach (DockContent content in Documents)
count++;
return count;
}
}Здесь коллекция Documents содержит элементы IDockContent, и их явное преобразование к типу DockContent может быть небезопасным.
Ну, и напоследок давайте рассмотрим примеры кода, не содержащего ошибок, но, тем не менее, избыточно усложненного:
V3031 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions. DockContentHandler.cs 540
internal void SetDockState(....)
{
....
if ((Pane != oldPane) || (Pane == oldPane
&& oldDockState != oldPane.DockState))
{
RefreshDockPane(Pane);
}
....
}Условия Pane != oldPane и Pane == oldPane являются взаимоисключающими, а, следовательно, это выражение можно упростить:
if (Pane != oldPane ||
oldDockState != oldPane.DockState)Аналогично, условное выражение в другом методе:
void SetProject(....)
{
....
if (!internalOpening || (internalOpening
&& !PluginBase.Settings.RestoreFileSession))
{
RestoreProjectSession(project);
}
....
}также может быть упрощено до:
if (!internalOpening || !PluginBase.Settings.RestoreFileSession)Проект FlashDevelop развивается уже более 10 лет и имеет достаточно большую кодовую базу. Применение статических анализаторов кода на таких проектах приносит интересные результаты и позволяет повысить качество программного продукта. Думаю, разработчикам проекта будет интересно взглянуть на найденные ошибки. Предлагаю всем разработчикам на языках C, C++ или C# скачать последнюю версию статического анализатора кода PVS-Studio и проверить свои проекты.
Если триальной версии будет недостаточно (подробности), то предлагаем связаться с нами и получить ключ для более подробного изучения инструмента.