В нашей компании возникла потребность использования библиотеки для Blazor компонентов. Мы остановились на MudBlazor и перед внедрением проверили качество её кода. В результате нашли ряд странностей и даже воспроизводящееся падение, о чём и расскажем в статье.

MudBlazor — одна из самых популярных библиотек для приложений под Blazor. Во многом наш выбор пал на неё из-за количества компонентов и красоты их реализации. Её исходный код написан на C#, поэтому перед непосредственным использованием мы решили проанализировать его. Для анализа взяли исходники версии v6.1.7. Дальше речь пойдёт о найденных ошибках.

Сомнительный вызов

Issue 1

public EndSlopeSpline(....):base(....)
{
  m = new Matrix(n);
  gauss = new MatrixSolver(n, m);
  a = new double[n];
  b = new double[n];
  c = new double[n];
  d = new double[n];
  h = new double[n];

  CalcParameters(firstSlopeDegrees, lastSlopeDegrees);
  Integrate();                                            // <=
  Interpolate();
}

Предупреждение PVS-Studio: V3010 The return value of function 'Integrate' is required to be utilized. EndSlopeSpline.cs 26

Чтобы разобраться в сути данного срабатывания, необходимо обратиться к реализации метода Integrate:

public double Integrate()
{
  double integral = 0;
  for (int i = 0; i < h.Length; i++)
  {
    double termA = a[i] * h[i];
    double termB = b[i] * Math.Pow(h[i], 2) / 2.0;
    double termC = c[i] * Math.Pow(h[i], 3) / 3.0;
    double termD = d[i] * Math.Pow(h[i], 4) / 4.0;
    integral += termA + termB + termC + termD;
  }
  return integral;
}

В методе не изменяется чьё-либо состояние (например, поля или свойства). Исходя из этого, можно сделать вывод, что единственное назначение этого метода — расчёт с последующим возвращением значения переменной integral. Это делает бессмысленным вызов Integrate без использования возвращаемого значения.

Стоит отметить, что методы CalcParameters и Interpolate (вызванные рядом с Integrate) как раз изменяют состояния ряда свойств и полей.

Issue 2

internal void DateValueChanged(DateTime? value)
{
  _valueDate = value;

  if (value != null)
  {
    var date = value.Value.Date;

    // get the time component and add it to the date.
    if (_valueTime != null)
    {
      date.Add(_valueTime.Value);                    // <=
    }

    _filterDefinition.Value = date;
  }
  else
    _filterDefinition.Value = value;

  _dataGrid.GroupItems();
}

Предупреждение PVS-Studio: V3010 The return value of function 'Add' is required to be utilized. Filter.cs 140

По своей сути срабатывание похоже на предыдущее. Здесь не было использовано возвращаемое значение метода Add, вызванного у переменной типа DateTime. Скорее всего, разработчик хотел добавить значение _valueTime.Value к date. Однако он не учёл, что метод Add для объекта типа DateTime возвращает результат добавления, а не изменяет исходный объект. В итоге имеем бесполезный вызов, который по задумке наверняка должен был на что-то влиять.

Неожиданный NullReferenceException

Issue 3

public static bool operator ==(ResizeOptions l, ResizeOptions r) 
                                                       => l.Equals(r);

public static bool operator !=(ResizeOptions l, ResizeOptions r) 
                                                       => !l.Equals(r);

Предупреждения PVS-Studio:

• V3115 Passing 'null' to '==' operator should not result in 'NullReferenceException'. ResizeOptions.cs 34

• V3115 Passing 'null' to '!=' operator should not result in 'NullReferenceException'. ResizeOptions.cs 35

При перегрузке операторов равенства и неравенства стоит учитывать возможность того, что один из операндов может иметь значение null. Подобные рекомендации даны в документации Microsoft.

Однако в реализации перегрузки операторов '==' и '!=' левый операнд не проверяется на null. Если значение параметра l – null, то при вызове метода Equals будет выброшено исключение типа NullReferenceException. Аргумент метода Equals, кстати, так же разыменовывается без проверки:

public bool Equals(ResizeOptions other)
{
  if (ReportRate != other.ReportRate || ....)     //<=
  {
    return false;
  }
  ....
}

Такое поведение может стать весьма неприятным сюрпризом для пользователя, если класс публичный. Именно таким и является ResizeOptions.

Чтобы убедиться в наличии проблемы, попробуем сравнить объект типа ResizeOptions с null.

Вполне ожидаемо получаем исключение типа NullReferenceException. Если правый операнд – null, поведение будет аналогичным.

Необоснованное присваивание

Issue 4

protected override void OnParametersSet()
{
  if (....)
  {
    ....
    foreach (....)
    {
      if (firstTime)
      {
        ....
        gridValueY = verticalStartSpace;                   // <=
      }
      else
      {
        ....
        gridValueY = verticalStartSpace;                   // <=
      }
      gridValueY = yValue;                                 // <=
      ....
    }
  }
  else
  {
    ....
  }
  ....
}

Предупреждение PVS-Studio: V3008 The 'gridValueY' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 191, 189. Line.razor.cs 191

Рассмотрим вложенный if. В обеих ветвях этого оператора переменной gridValueY присваивается некоторое значение. Стоит отметить, что оно одинаково как в первом, так и во втором случае. Получается, что такое присваивание стоило вообще вынести за if. Однако основная проблема заключается не в этом: значение gridValueY изменяется сразу после if, хотя оно не использовалось.

Блок else оператора if верхнего уровня схож с блоком then:

foreach (....)
{
  if (firstTime)
  {
    ....
    gridValueY = verticalStartSpace;
  }
  else
  {
    ....
    gridValueY = verticalStartSpace;
  }
  ....
  gridValueY = boundHeight - (gridValueY + gridValue);   // <=
  ....
}

В данном случае значение gridValueY тоже перезаписывается сразу после if, однако старое значение при этом используется.

Неправильно расставленные приоритеты

Issue 5

internal async Task<bool> StartResizeColumn(....)
{
  ....
  // In case resize mode is column, we have to find any column right
  // of the current one that can also be resized and is not hidden.

  var nextResizableColumn = _columns.Skip(_....) + 1)
                                    .FirstOrDefault(c =>
                                        c.Resizable ?? true && !c.Hidden); // <=
  ....
}

Предупреждение PVS-Studio: V3177 The 'true' literal belongs to the '&&' operator with a higher priority. It is possible the literal was intended to belong to '??' operator instead. DataGridColumnResizeService.cs 52

Рассмотрим выражение c.Resizable ?? true && !c.Hidden. Стоит сразу сказать, что приоритет '??' ниже, чем у '&&'. Исходя из этого, если c.Resizable — null, то оператор null-coalescing вернёт результат операции true && !c.Hidden. Операция "&&" с литералом true бессмысленна. Скорее всего, правильный вариант выглядит так:

(c.Resizable ?? true) && !c.Hidden

Разница этих записей проявит себя, если значения c.Resizable и c.Hidden будут равны true.

Есть ещё один момент, указывающий на то, что была допущена ошибка. Строчкой выше написан комментарий о том, что столбец должен иметь возможность изменения размера (информация об этом содержится в c.Resizable) и не быть скрытым (это можно узнать из c.Hidden). Если c.Resizable — true, то логика работы будет нарушена, так как значение c.Hidden никак не повлияет на результат.

Подводя итог

Как мне кажется, каждое из разобранных срабатываний может указывать на ошибку, меняющую логику работы программы. В одном из случаев это удалось доказать. Перед написанием статьи я создал issue на GitHub. Однако, ответ от разработчиков пока не был получен. Надеюсь, что они рассмотрят перечисленные проблемы и смогут повысить качество кода своего проекта :).

Можно попробовать использованный в статье анализатор, скачав его отсюда. Кстати, недавно в PVS-Studio появилась возможность анализа блоков C# кода в Blazor-компонентах. Некоторые найденные в таких компонентах ошибки описаны в этой статье.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Nikita Panevin. Should we check libraries before using them? MudBlazor helps us find the answer.