В нашей компании возникла потребность использования библиотеки для 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.
