- PVSM.RU - https://www.pvsm.ru -
Для проверки качества диагностик нашего статического анализатора и его рекламы мы регулярно анализируем проекты с открытым исходным кодом. Разработчики проекта FlashDevelop сами попросили нас проверить их продукт, что мы с радостью и сделали.
FlashDevelop [1] — популярная среда разработки Flash-приложений, поддерживающая Action Script версии 2 и 3, Haxe, JavaScript, HTML, PHP, C#, и обладающая функционалом, присущим современным редакторам кода, например, автодополнение кода, встроенная поддержка svn, git, mercurial, шаблоны, сторонние плагины, темы подсветки синтаксиса и многое другое. Примечательно, что FlashDevelop использовали Fireaxis Games при разработке XCOM: Enemy Unknown [2].
Учитывая то, что FlashDevelop — продукт с открытым исходным кодом, и написан на языке C#, мы захотели проверить его нашим анализатором. Для анализа был использован статический анализатор PVS-Studio v6.05. Поскольку разбирать все найденные проблемные места в рамках этой статьи не представляется возможным, рассмотрим наиболее интересные сообщения анализатора.
Как известно, строки в C# — иммутабельные объекты, и методы, отвечающие за изменение строки, на самом деле возвращают новый объект типа String, сохраняя изначальную строку неизменной. Однако, как показывает практика, разработчики забывают про эту особенность. Например, анализатор обнаружил следующие ошибки:
V3010 [3] 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 [3] 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 [3] 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 [4] 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 [5] 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 [5] 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 [6] 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 [7] Expression 'setting == «GlobalClasspath»' is always false. PluginMain.cs 1194
Действительно, условие else if (setting == «GlobalClasspath») не будет выполнено никогда, потому что это же условие присутствует в самом первом if. А ведь от выполнения этого условия зависит выполнение какой-то логики. Чтобы упростить читабельность этого метода, я бы переписал его с использованием оператора switch.
Следующий пример никогда не выполнимого условия:
V3022 [7] 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 [7] 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 [8] 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 [9] 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 [10] 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 [11] предназначена для обнаружения потенциально небезопасных вызовов обработчиков событий. В анализируемом проекте эта диагностика выявила большое количество таких мест. Разберем ситуацию небезопасного вызова обработчика на конкретном примере:
V3083 [11] 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 [12] 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 [13] The original exception object 'uex' was swallowed. Stack of original exception could be lost. FileActions.cs 598
Такое пробрасывание исключения приводит к тому, что оригинальный стек вызовов затирается новым, начинающимся с текущего метода. Это сильно затруднит поиск места, в котором возникло оригинальное исключение, при отладке.
Чтобы сохранить оригинальный стек вызовов при повторном генерации исключений, нужно просто использовать оператор throw:
try
{
....
}
catch (UserCancelException uex)
{
throw;
}
Дальнейший анализ кода выявил потенциально небезопасное место:
V3087 [14] 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 [15] 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# скачать последнюю версию [16] статического анализатора кода PVS-Studio и проверить свои проекты.
Если триальной версии будет недостаточно (подробности [17]), то предлагаем связаться [18] с нами и получить ключ для более подробного изучения инструмента.
Автор: PVS-Studio
Источник [19]
Сайт-источник PVSM.RU: https://www.pvsm.ru
Путь до страницы источника: https://www.pvsm.ru/c-2/156659
Ссылки в тексте:
[1] FlashDevelop: http://www.flashdevelop.org/
[2] XCOM: Enemy Unknown: https://xcom.com/ru/xcom-enemy-unknown
[3] V3010: http://www.viva64.com/ru/d/0406/
[4] V3019: http://www.viva64.com/ru/d/0388/
[5] V3013: http://www.viva64.com/ru/d/0389/
[6] V3020: http://www.viva64.com/ru/d/0410/
[7] V3022: http://www.viva64.com/ru/d/0391/
[8] V3095: http://www.viva64.com/ru/d/0504/
[9] V3061: http://www.viva64.com/ru/d/0468/
[10] V3066: http://www.viva64.com/ru/d/0459/
[11] V3083: http://www.viva64.com/ru/d/0485/
[12] V3056: http://www.viva64.com/ru/d/0453/
[13] V3052: http://www.viva64.com/ru/d/0456/
[14] V3087: http://www.viva64.com/ru/d/0489/
[15] V3031: http://www.viva64.com/ru/d/0416/
[16] скачать последнюю версию: http://www.viva64.com/ru/pvs-studio-download/
[17] подробности: http://www.viva64.com/ru/b/0395/
[18] связаться: http://www.viva64.com/ru/about-feedback/
[19] Источник: https://habrahabr.ru/post/305718/?utm_source=habrahabr&utm_medium=rss&utm_campaign=best
Нажмите здесь для печати.