- PVSM.RU - https://www.pvsm.ru -

Проверяем исходный код FlashDevelop с помощью PVS-Studio

Проверяем исходный код FlashDevelop с помощью PVS-Studio - 1Для проверки качества диагностик нашего статического анализатора и его рекламы мы регулярно анализируем проекты с открытым исходным кодом. Разработчики проекта 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 после приведения типов

Стандартной практикой после операции приведения типа является проверка полученного значения на 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, и только потом используем. Однако, как показывает наша практика, разработчик может начать сразу использовать полученный объект, и только потом проверить, что он не равен 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;
}

Вероятное возникновение исключения InvalidCastException при обходе коллекции

Дальнейший анализ кода выявил потенциально небезопасное место:

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