В нашей компании возникла потребность использования библиотеки для 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 возвращает результат добавления, а не изменяет исходный объект. В итоге имеем бесполезный вызов, который по задумке наверняка должен был на что-то влиять.
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:
При перегрузке операторов равенства и неравенства стоит учитывать возможность того, что один из операндов может иметь значение 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-компонентах. Некоторые найденные в таких компонентах ошибки описаны в этой статье.