# Проверяем исходный код FlashDevelop с помощью PVS\-Studio Для проверки качества диагностик нашего статического анализатора и его рекламы мы регулярно анализируем проекты с открытым исходным кодом\. Разработчики проекта FlashDevelop сами попросили нас проверить их продукт, что мы с радостью и сделали\. ![0412_FlashDevelop_ru/image1.png](https://import.viva64.com/docx/blog/0412_FlashDevelop_ru/image1.png) ## Введение [FlashDevelop](http://www.flashdevelop.org/) \- популярная среда разработки Flash\-приложений, поддерживающая Action Script версии 2 и 3, Haxe, JavaScript, HTML, PHP, C\#, и обладающая функционалом, присущим современным редакторам кода, например, автодополнение кода, встроенная поддержка svn, git, mercurial, шаблоны, сторонние плагины, темы подсветки синтаксиса и многое другое\. Примечательно, что FlashDevelop использовали Fireaxis Games при разработке [XCOM: Enemy Unknown](https://xcom.com/ru-RU/xcom-enemy-unknown/)\. ## Результаты проверки Учитывая то, что FlashDevelop \- продукт с открытым исходным кодом, и написан на языке C\#, мы захотели проверить его нашим анализатором\. Для анализа был использован статический анализатор PVS\-Studio v6\.05\. Поскольку разбирать все найденные проблемные места в рамках этой статьи не представляется возможным, рассмотрим наиболее интересные сообщения анализатора\. ### Неиспользуемые возвращаемые значения методов Как известно, строки в C\# \- иммутабельные объекты, и методы, отвечающие за изменение строки, на самом деле возвращают новый объект типа String, сохраняя изначальную строку неизменной\. Однако, как показывает практика, разработчики забывают про эту особенность\. Например, анализатор обнаружил следующие ошибки: [V3010](https://pvs-studio.ru/ru/docs/warnings/v3010/) The return value of function 'Insert' is required to be utilized\. ASPrettyPrinter\.cs 1263 ```cpp public void emit(IToken tok) { .... lineData.Insert(0, mSourceData.Substring(prevLineEnd, ((CommonToken)t).StartIndex - prevLineEnd)); .... } ``` [V3010](https://pvs-studio.ru/ru/docs/warnings/v3010/) The return value of function 'Insert' is required to be utilized\. MXMLPrettyPrinter\.cs 383 ```cpp private void prettyPrint(....) { .... while (aToken.Line == currentLine) { lineData.Insert(0, aToken.Text); .... } .... } ``` Вероятно, разработчик имел в виду такую конструкцию: ```cpp lineData = lineData.Insert(....); ``` Другой пример срабатывания диагностики V3010: [V3010](https://pvs-studio.ru/ru/docs/warnings/v3010/) The return value of function 'NextDouble' is required to be utilized\. ASFileParser\.cs 196 ```cpp private static string getRandomStringRepl() { random.NextDouble(); return "StringRepl" + random.Next(0xFFFFFFF); } ``` Этот код не содержит ошибки с точки зрения фунционала, тем не менее, вызов _random\.NextDouble\(\)_ не несет никакой смысловой нагрузки и может быть удален\. ### Проверка на null после приведения типов Стандартной практикой после операции приведения типа является проверка полученного значения на null на случай, если исходный тип не может быть приведен к желаемому\. При выполнении такой рутинной операции разработчик может быть невнимательным и проверить не ту переменную\. Наш анализатор не устаёт и внимательно следит за такими вещами: [V3019](https://pvs-studio.ru/ru/docs/warnings/v3019/) Possibly an incorrect variable is compared to null after type conversion using 'as' keyword\. Check variables 'item', 'val'\. WizardHelper\.cs 67 ```cpp public static void SetControlValue(....) { .... string val = item as string; if (item == null) continue; .... } ``` Очевидно, что в этом примере на null следует проверять переменную _val_, а не _item_, и код должен выглядеть следующим образом: ```cpp string val = item as string; if (val == null) continue; ``` ### Дублирование тел методов Когда в коде встречаются методы с одинаковыми телами, это всегда вызывает подозрения\. В лучшем случае, такой код требует рефакторинга, а в худшем \- механический копипаст искажает логику работы программы\. Чтобы не быть голословным, рассмотрим следующие примеры\. [V3013](https://pvs-studio.ru/ru/docs/warnings/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 ```cpp 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\(\)_\. Я предполагаю, что корректная реализация этого метода должна быть такой: ```cpp private void SuspendMdiClientLayout() { if (GetMdiClientController().MdiClient != null) GetMdiClientController().MdiClient.SuspendLayout(); // <= } ``` Другой пример\. В проекте реализован тип _Lexer_, предназначенный для лексического разбора чего\-то\. В этом типе реализовано 28 однотипных методов с сигнатурами вида _private static bool StateXX \(FsmContext ctx\)_, где XX находится в диапазоне от 1 до 28\. Нет ничего удивительного в том, что при выполнении такого объема рутинной работы глаз разработчика может замылиться, что привело появлению в коде ошибки, на которую анализатор PVS\-Studio реагирует следующим образом: [V3013](https://pvs-studio.ru/ru/docs/warnings/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 ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v3020/) An unconditional 'break' within a loop\. AirWizard\.cs 1760 ```cpp private void ExtensionBrowseButton_Click(....) { .... foreach (var existingExtension in _extensions) { if (existingExtension.ExtensionId == extensionId) extension = existingExtension; break; } .... } ``` Рискну предположить, что разработчик хотел пробежать по элементам коллекции _\_extensions_, найти первый объект _existingExtension_ с соответствующим _extensionId_ и выйти из цикла\. Но из\-за экономии на скобках цикл безусловно завершается после первой итерации, что существенно влияет на логику работы программы\. ### Выражение всегда истинно/ложно Другой распространенный источник ошибок \- это условные выражения\. Если выражение включает большое количество переменных, граничных значений, достаточно сложное ветвление, \- вероятность совершения ошибки увеличивается\. Рассмотрим, например, такой условный оператор: ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'setting \=\= "GlobalClasspath"' is always false\. PluginMain\.cs 1194 Действительно, условие _else if \(setting \=\= "GlobalClasspath"\)_ не будет выполнено никогда, потому что это же условие присутствует в самом первом _if_\. А ведь от выполнения этого условия зависит выполнение какой\-то логики\. Чтобы упростить читабельность этого метода, я бы переписал его с использованием оператора _switch_\. Следующий пример никогда не выполнимого условия: [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'high \=\= 0xBF' is always false\. JapaneseContextAnalyser\.cs 293 ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression '\!Outline\.FlagTestDrop' is always true\. DockPanel\.DockDragHandler\.cs 769 ```cpp private void TestDrop() { Outline.FlagTestDrop = false; .... if (!Outline.FlagTestDrop) { .... } .... } ``` В этом фрагменте мы видим, что поле _Outline\.FlagTestDrop_, которому присвоено значение false и которое дальше в коде не изменяется, используется в условном операторе _if_\. Возможно, в этом методе не был реализован функционал, изменяющий значение этого поля, ведь для чего\-то же разработчик реализовал проверку _if \(\!Outline\.FlagTestDrop\)_\. ### Использование экземпляра перед тем, как он был проверен на null В практике постоянно возникает необходимость проверить переменную на null, например, после приведения типа, выборке элемента из коллекции и т\.д\. В таких ситуациях мы проверяем, что полученная переменная не равна null, и только потом используем\. Однако, как показывает наша практика, разработчик может начать сразу использовать полученный объект, и только потом проверить, что он не равен null\. О таких ошибках сообщает диагностика V3095: [V3095](https://pvs-studio.ru/ru/docs/warnings/v3095/) The 'node' object was used before it was verified against null\. Check lines: 364, 365\. ProjectContextMenu\.cs 364 ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v3061/) Parameter 'b' is always rewritten in method body before being used\. InBuffer\.cs 56 ```cpp 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_"\)\. Возможно, сигнатура этого метода должна выглядеть следующим образом: ```cpp public bool ReadByte(ref byte b) { .... } ``` ### Неверный порядок аргументов, переданных в метод Следующее подозрительное место, найденное анализатором, не так просто заметить при проведении code review: [V3066](https://pvs-studio.ru/ru/docs/warnings/v3066/) Possible incorrect order of arguments passed to '\_channelMixer\_OVERLAY' method: 'back' and 'fore'\. BBCodeStyle\.cs 302 ```cpp private static float _channelMixer_HARDLIGHT(float back, float fore) { return _channelMixer_OVERLAY(fore, back); } ``` Метод \_channelMixer\_OVERLAY имеет такую сигнатуру: ```cpp static float _channelMixer_OVERLAY(float back, float fore) ``` Возможно именно так и задумано\. Однако есть вероятность, что при обращении к этому методу аргументы _fore_ и _back_ были перепутаны местами\. И анализатор помогает проверить такие места\. ### Небезопасный вызов обработчика события Диагностика [V3083](https://pvs-studio.ru/ru/docs/warnings/v3083/) предназначена для обнаружения потенциально небезопасных вызовов обработчиков событий\. В анализируемом проекте эта диагностика выявила большое количество таких мест\. Разберем ситуацию небезопасного вызова обработчика на конкретном примере: [V3083](https://pvs-studio.ru/ru/docs/warnings/v3083/) Unsafe invocation of event 'OnKeyEscape', NullReferenceException is possible\. Consider assigning event to a local variable before invoking it\. QuickFind\.cs 849 ```cpp protected void OnPressEscapeKey() { if (OnKeyEscape != null) OnKeyEscape(); } ``` На первый взгляд, код кажется абсолютно корректным: если поле OnKeyEscape не равно null, вызываем это событие\. Однако использовать такой подход не рекомендуется\. Допустим, что у события OnKeyEscape один подписчик, и допустим, что после проверки этого поля на null этот подписчик отписался от этого события \(в другом потоке, например\)\. После того, как у события не осталось подписчиков, поле OnKeyEscape будет содержать пустую ссылку, и попытка вызвать событие приведет в возникновению NullReferenceException\. Особенно неприятно, что это крайне трудновоспроизводимая ошибка\. Пользователь может пожаловаться, что нажатие ESC привело в ошибке\. Однако даже нажав ESC тысячу раз, вряд ли программисту удастся понять, что не так\. Обезопасить вызов события можно, объявив дополнительную промежуточную переменную: ```cpp var handler = OnKeyEscape if (handler != null) handler(); ``` В C\# версии 6 появился оператор проверки на null \(?\.\), позволяющий значительно упростить код: ```cpp OnKeyEscape?.Invoke(); ``` ### Возможные опечатки Эвристические возможности нашего анализатора позволяют обнаруживать весьма интересные подозрительные места в коде\. Например: [V3056](https://pvs-studio.ru/ru/docs/warnings/v3056/) Consider reviewing the correctness of 'a1' item's usage\. LzmaEncoder\.cs 225 ```cpp public void SetPrices(....) { UInt32 a0 = _choice.GetPrice0(); UInt32 a1 = _choice.GetPrice1(); UInt32 b0 = a1 + _choice2.GetPrice0(); UInt32 b1 = a1 + _choice2.GetPrice1(); .... } ``` Вполне вероятно, что этот код был написан методом копипаста\. И мне кажется, что для вычисления значения переменной _b0_ должна использоваться переменная _a0_ вместо _a1_\. В любом случае, найденное подозрительное место должно послужить поводом для разработчиков внимательно посмотреть на этот код\. И вообще, лучше использовать более информативные имена переменных\. ### Повторное пробрасывание исключений В коде было обнаружено несколько мест, в которых перехваченное исключение пробрасывается дальше\. Реализовано это, например, следующим образом: ```cpp public void Copy(string fromPath, string toPath) { .... try { .... } catch (UserCancelException uex) { throw uex; } .... } ``` Анализатор при проверке этого метода выдает сообщение: [V3052](https://pvs-studio.ru/ru/docs/warnings/v3052/) The original exception object 'uex' was swallowed\. Stack of original exception could be lost\. FileActions\.cs 598 Такое пробрасывание исключения приводит к тому, что оригинальный стек вызовов затирается новым, начинающимся с текущего метода\. Это сильно затруднит поиск места, в котором возникло оригинальное исключение, при отладке\. Чтобы сохранить оригинальный стек вызовов при повторном генерации исключений, нужно просто использовать оператор _throw_: ```cpp try { .... } catch (UserCancelException uex) { throw; } ``` ### Вероятное возникновение исключения InvalidCastException при обходе коллекции Дальнейший анализ кода выявил потенциально небезопасное место: [V3087](https://pvs-studio.ru/ru/docs/warnings/v3087/) Type of variable enumerated in 'foreach' is not guaranteed to be castable to the type of collection's elements\. VS2005DockPaneStrip\.cs 1436 ```cpp private void WindowList_Click(object sender, EventArgs e) { .... List tabs = new List(Tabs); foreach (TabVS2005 tab in tabs) .... } ``` Коллекция _tabs_ содержит элементы типа _Tab_, в то время как при итерации элементы этой коллекции приводятся к типу _TabVS2005_, который является наследником типа _Tab_\. Это приведение небезопасно и может привести к возникновению исключения _System\.InvalidCastException_\. Эта же диагностика обнаружила похожий небезопасный код: ```cpp public int DocumentsCount { get { int count = 0; foreach (DockContent content in Documents) count++; return count; } } ``` Здесь коллекция _Documents_ содержит элементы _IDockContent_, и их явное преобразование к типу _DockContent_ может быть небезопасным\. ### Избыточная проверка условий Ну, и напоследок давайте рассмотрим примеры кода, не содержащего ошибок, но, тем не менее, избыточно усложненного: [V3031](https://pvs-studio.ru/ru/docs/warnings/v3031/) An excessive check can be simplified\. The '\|\|' operator is surrounded by opposite expressions\. DockContentHandler\.cs 540 ```cpp internal void SetDockState(....) { .... if ((Pane != oldPane) || (Pane == oldPane && oldDockState != oldPane.DockState)) { RefreshDockPane(Pane); } .... } ``` Условия _Pane \!\= oldPane_ и _Pane \=\= oldPane_ являются взаимоисключающими, а, следовательно, это выражение можно упростить: ```cpp if (Pane != oldPane || oldDockState != oldPane.DockState) ``` Аналогично, условное выражение в другом методе: ```cpp void SetProject(....) { .... if (!internalOpening || (internalOpening && !PluginBase.Settings.RestoreFileSession)) { RestoreProjectSession(project); } .... } ``` также может быть упрощено до: ```cpp if (!internalOpening || !PluginBase.Settings.RestoreFileSession) ``` ## Заключение Проект FlashDevelop развивается уже более 10 лет и имеет достаточно большую кодовую базу\. Применение статических анализаторов кода на таких проектах приносит интересные результаты и позволяет повысить качество программного продукта\. Думаю, разработчикам проекта будет интересно взглянуть на найденные ошибки\. Предлагаю всем разработчикам на языках C, C\+\+ или C\# [скачать последнюю версию](https://pvs-studio.ru/ru/pvs-studio/download/) статического анализатора кода PVS\-Studio и проверить свои проекты\. Если триальной версии будет недостаточно \([подробности](https://pvs-studio.ru/ru/blog/posts/0395/)\), то предлагаем [связаться](https://pvs-studio.ru/ru/about-feedback/) с нами и получить ключ для более подробного изучения инструмента\.