Самые быстрые отчёты на диком западе. И горстка багов в придачу…

    Picture 3

    Не только 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-странице проекта.

    Picture 8


    Со сборкой проекта никаких проблем не возникло — собирал его из 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);

    Внимание, ответ:

    Picture 2


    Неожиданно? Давайте посмотрим на тело соответствующего конструктора:

    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» :)

    Каким? Таким, например:

    Picture 1


    Проблема кроется в свойстве 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) и прочие.

    Если что-то из этого неактуально, можно:


    Заключение


    Picture 4


    Несмотря на то, что статья вышла небольшой, мне доставило удовольствие 'пощупать' предупреждения анализатора руками — посмотреть, как то, на что ругается анализатор, проявляется на практике.

    Авторам же проекта желаю успехов, исправления обнаруженных проблем и хочу похвалить за шаг навстречу open-source community!

    Остальным советую попробовать анализатор на своём коде и посмотреть, что интересного удастся найти.

    Всех благ!



    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Sergey Vasiliev. The Fastest Reports in the Wild West — and a Handful of Bugs...
    • +33
    • 5,6k
    • 6

    PVS-Studio

    440,00

    Ищем ошибки в C, C++ и C# на Windows, Linux, macOS

    Поделиться публикацией
    Комментарии 6
      +4
      Первых двух ошибок, скорее всего, удалось бы избежать, если бы использовалась общепринятая для C# конвенция по именам:
      • приватные поля в camelCase и начинаются со знака подчеркивания (например, private string _privateField),
      • публичные свойства — в PascalCase (например, public int PublicProperty { get; set; }),
      • локальные переменные и аргументы функций — в camelCase (например, int localVariable)

      Кроме того, следование таким соглашениям улучшает читабельность кода, особенно для новых людей на проекте
        +3
        Увы, даже правильное именование сущностей не всегда помогает.

        Например, я сразу вспомнил ошибку в конструкторе одного из проектов. Хотели записать значение соответствующего параметра в свойство, но ошиблись, и присвоили значение свойства ему же.

        Посмотрел срабатывания диагностики V3005 — оказалось, подобный пример не один, что, впрочем, ожидаемо.

        Вот несколько примеров:

        Sony ATF

        public ProgressCompleteEventArgs(Exception progressError, 
          object progressResult,
          bool cancelled)
        {
          ProgressError = ProgressError;
          ProgressResult = progressResult;
          Cancelled = cancelled;
        }


        Roslyn

        public DiagnosticAsyncToken(
          AsynchronousOperationListener listener,
          string name,
          object tag,
          string filePath,
          int lineNumber)
          : base(listener)
        {
          Name = Name;
          Tag = tag;
          FilePath = filePath;
          LineNumber = lineNumber;
          StackTrace = PortableShim.StackTrace.GetString();
        }


        MonoDevelop

        public ViMacro (char macroCharacter) {
          MacroCharacter = MacroCharacter;
        }
        
        public char MacroCharacter { get; set; }


        MonoDevelop

        public WhitespaceNode(string whiteSpaceText, TextLocation startLocation)
        {
          this.WhiteSpaceText = WhiteSpaceText;
          this.startLocation = startLocation;
        }
        
        public string WhiteSpaceText { get; set; }


        Если ещё порыскать по примерам ошибок / диагностикам, можно также найти много других примеров ошибок, связанных с очень похожими именами.
          0

          Последний пример крайне жесток, потому что подобный код генерирует решарпер.
          Я вообще за встроенную кодогенерацию.

        +3

        Узнал о Fast Report узнал на #dotnext. Обязательно постараюсь попробовать использовать в каком нибудь следующем проекте. А так желаю продукту развития и большого количества пользователей.

          0
          Ему сто лет в обед
          +1
          Хм… Надо попробовать на своих проектах.)

          Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

          Самое читаемое