Не только Microsoft в последнее время выкладывает код собственных проектов в открытый доступ — другие компании тоже следуют этой тенденции. Для нас же — разработчиков PVS-Studio — это отличный способ ещё раз протестировать анализатор, посмотреть, что интересного он сможет найти, и сообщить об этом авторам проекта. Сегодня заглядываем внутрь проекта компании Fast Reports.
Что проверяли?
FastReport — генератор отчётов, разрабатываемый компанией Fast Reports. Написан на C# и совместим с .NET Standard 2.0+. Исходный код проекта недавно выложили на GitHub, откуда он и был загружен для последующего анализа.
Отчёты поддерживают использование текста, изображений, линий, фигур, диаграмм, таблиц, штрихкодов и пр. Могут быть одностраничными и многостраничными, в том числе содержать помимо данных обложку и заднюю страницу. В качестве источников данных могут выступать XML, CSV, Json, MS SQL, MySql, Oracle, Postgres, MongoDB, Couchbase, RavenDB, SQLite.
Существуют различные способы создания шаблонов отчётов: из кода; как XML-файла; с использованием онлайн дизайнера или FastReport Designer Community Edition.
При необходимости библиотеки можно загрузить как NuGet пакеты.
Более полно о возможностях продукта можно почитать на GitHub-странице проекта.
Со сборкой проекта никаких проблем не возникло — собирал его из Visual Studio 2017, откуда в дальнейшем и проверил с помощью плагина PVS-Studio.
PVS-Studio — статический анализатор, который ищет ошибки в коде на C, C++, C#, Java. При анализе C# кода можно использовать анализатор c IDE Visual Studio, используя для этого плагин PVS-Studio, или же можно проверять проекты из командной строки, для чего используется command-line утилита PVS-Studio_Cmd.exe. При желании можно настроить анализ на сборочном сервере или подцепить результаты анализа к SonarQube.
Что ж, давайте посмотрим, что интересного нашло��ь на этот раз.
Так как проект небольшой, на множество опечаток и подозрительных мест рассчитывать не стоит. Посмотрим на то, что нашлось, а что-то даже попробуем воспроизвести на практике.
Очепятки и не только
Встретился следующий метод:
public override string ToString() { if (_value == null) return null; return this.String; }
Предупреждение PVS-Studio: V3108 It is not recommended to return 'null' from 'ToSting()' method. Variant.cs 1519
Да, возвращение null из переопределённого метода ToString() само по себе не является ошибкой, но всё же это — плохой стиль. Это указано, в том числе, в документации от Microsoft: Your ToString() override should not return Empty or a null string. Разработчики, не ожидающие возврата null в качестве возвращаемого значения ToString(), могут быть неприятно удивлены, когда в ходе исполнения представленного ниже кода будет сгенерировано исключение ArgumentNullException (при условии, что вызывается метод расширения для IEnumerable<T>).
Variant varObj = new Variant(); varObj.ToString().Contains(character);
Можете придраться к тому, что пример синтетический, но суть от этого не меняется.
Более того, данный код прокомментирован следующим образом:
/// <summary> /// Returns <see cref="String"/> property unless the value on the right /// is null. If the value on the right is null, returns "". /// </summary> /// <returns></returns>
Упс. Вместо "" возвращает null.
Продолжим.
В библиотеке есть класс FastString. Описание: Fast alternative of StringBuilder. Собственно, этот класс содержит поле типа StringBuilder. Конструкторы класса FastString вызывают метод Init, который выполняет инициализацию соответствующего поля.
Код одного из конструкторов:
public FastString() { Init(initCapacity); }
И код метода Init:
private void Init(int iniCapacity) { sb = new StringBuilder(iniCapacity); //chars = new char[iniCapacity]; //capacity = iniCapacity; }
При желании можно обратиться к полю sb через свойство StringBuilder:
public StringBuilder StringBuilder { get { return sb; } }
Всего FastString имеет 3 конструктора:
public FastString(); public FastString(int iniCapacity); public FastString(string initValue);
Тело первого конструктора я уже показал, что делают два остальных догадаться, думаю, тоже несложно. А теперь, внимание. Угадайте, что выведет следующий код:
FastString fs = new FastString(256); Console.WriteLine(fs.StringBuilder.Capacity);
Внимание, ответ:
Неожиданно? Давайте посмотрим на тело соответствующего конструктора:
public FastString(int iniCapacity) { Init(initCapacity); }
У постоянных читателей наших статей уже должен быть намётан глаз, чтобы найти здесь проблему. У анализатора глаз (нюх, логика, называйте как хотите) точно намётан, и проблему он нашёл: V3117 Constructor parameter 'iniCapacity' is not used. FastString.cs 434
Какое совпадение, что в коде класса содержится константное поле initCapacity, которое и передаётся в качестве аргумента методу Init вместо параметра конс��руктора iniCapacity…
private const int initCapacity = 32;
При использовании схожих имён нужно быть очень, очень внимательным. Где только ни встречались ошибки, связанные с использованием похожих имён — проекты на C, C++, C#, Java — везде были опечатки подобного рода…
Кстати, про опечатки.
Сделаем следующий простенький пример и посмотрим, как он работает:
static void Main(string[] args) { TextObject textObj = new TextObject(); textObj.ParagraphFormat = null; Console.WriteLine("Ok"); }
Как вы уже догадались, вывод будет другим, нежели строка «Ok» :)
Каким? Таким, например:
Проблема кроется в свойстве ParagraphFormat и в использовании схожих имён:
public ParagraphFormat ParagraphFormat { get { return paragraphFormat; } set { ParagraphFormat = value; } }
Предупреждение PVS-Studio: V3110 Possible infinite recursion inside 'ParagraphFormat' property. TextObject.cs 281
Свойство ParagraphFormat является обёрткой над полем paragraphFormat. Причём его get property accessor написан правильно, а вот set property accessor содержит досадную опечатку: вместо поля запись происходит в это же свойство, из-за чего возникает рекурсия. Опять ошибка, связанная со схожими именами.
Рассмотрим следующий фрагмент кода.
public override Run Split(float availableWidth, out Run secondPart) { .... if (r.Width > availableWidth) { List<CharWithIndex> list = new List<CharWithIndex>(); for (int i = point; i < size; i++) list.Add(chars[i]); secondPart = new RunText(renderer, word, style, list, left + r.Width, charIndex); list.Clear(); for (int i = 0; i < point; i++) list.Add(chars[i]); r = new RunText(renderer, word, style, list, left, charIndex); return r; } else { List<CharWithIndex> list = new List<CharWithIndex>(); for (int i = point; i < size; i++) list.Add(chars[i]); secondPart = new RunText(renderer, word, style, list, left + r.Width, charIndex); list.Clear(); for (int i = 0; i < point; i++) list.Add(chars[i]); r = new RunText(renderer, word, style, list, left, charIndex); return r; } .... }
Предупреждение PVS-Studio: V3004 The 'then' statement is equivalent to the 'else' statement. HtmlTextRenderer.cs 2092
Немного копипаста, и теперь вне зависимости от значения выражения r.Width > availableWidth будут выполняться одни и те же действия. Стоит или убрать оператор if, или изменить логику в одной из веток.
public static string GetExpression(FindTextArgs args, bool skipStrings) { while (args.StartIndex < args.Text.Length) { if (!FindMatchingBrackets(args, skipStrings)) break; return args.FoundText; } return ""; }
Предупреждение анализатора: V3020 An unconditional 'return' within a loop. CodeUtils.cs 262
Из-за безусловного оператора return для приведённого выше цикла выполнится не более одной итерации. Возможно, такой код получился после рефакторинга, а может, это просто необычный способ сделать то, что можно было бы сделать без цикла.
private int FindBarItem(string c) { for (int i = 0; i < tabelle_cb.Length; i++) { if (c == tabelle_cb[i].c) return i; } return -1; } internal override string GetPattern() { string result = tabelle_cb[FindBarItem("A")].data + "0"; foreach (char c in text) { int idx = FindBarItem(c.ToString()); result += tabelle_cb[idx].data + "0"; } result += tabelle_cb[FindBarItem("B")].data; return result; }
Предупреждение PVS-Studio: V3106 Possible negative index value. The value of 'idx' index could reach -1. BarcodeCodabar.cs 70
Потенциально опасный код. Метод FindBarItem может вернуть значение -1, если не найдёт элемента, переданного в качестве параметра. В вызывающем же коде (метод GetPattern) это значение записывается в переменную idx и используется в качестве индекса массива tabelle_cb без предварительной проверки. При обращении по индексу -1 будет сгенерировано исключение типа IndexOutOfRangeException.
Продолжим.
protected override void Finish() { .... if (saveStreams) { FinishSaveStreams(); } else { if (singlePage) { if (saveStreams) { int fileIndex = GeneratedFiles.IndexOf(singlePageFileName); DoPageEnd(generatedStreams[fileIndex]); } else { .... } .... } .... } .... }
Предупреждение PVS-Studio: V3022 Expression 'saveStreams' is always false. HTMLExport.cs 849
Приведённый код с получением значения fileIndex и вызовом метода DoPageEnd никогда не выполнится, так как результат второго приведённого в коде выражения saveStreams всегда будет иметь значение false.
Из наиболее интересного это, пожалуй, всё (вы же не ждали статьи в духе анализа Mono?). Были и другие предупреждения анализатора, но они не показались мне достаточно интересными, чтобы включать их в статью (часть всегда остаётся за кадром).
Для их анализа будет полезным знание проекта, так что в идеале авторам следовало бы посмотреть на эти предупреждения самостоятельно. Это такие предупреждения, как V3083 (потенциально опасный вызов обработчиков события), V3022 (условие всегда true / false (в данном случае часто из-за методов, которые возвращают одно значение)), V3072, V3073 (работа с IDisposable) и прочие.
Если что-то из этого неактуально, можно:
- выключить диагностику, если обнаруживаемый ей паттерн точно корректен на проекте;
- разметить предупреждения как ложные срабатывания, если их не очень много;
- добавить предупреждения в базу подавления, если не хочется вставлять комментарии в код. Но тогда потребуется доступ к базе подавления для всех, кто работает с анализатором.
Заключение
Несмотря на то, что статья вышла небольшой, мне доставило удовольствие 'пощупать' предупреждения анализатора руками — посмотреть, как то, на что ругается анализатор, проявляется на практике.
Авторам же проекта желаю успехов, исправления обнаруженных проблем и хочу похвалить за шаг навстречу open-source community!
Остальным советую попробовать анализатор на своём коде и посмотреть, что интересного удастся найти.
Всех благ!
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Sergey Vasiliev. The Fastest Reports in the Wild West — and a Handful of Bugs...
