ScottPlot — библиотека под .NET для построения графиков. Из-за специфики подобных проектов их код нередко бывает запутанным. Сегодня мы попробуем распутать его и отыскать проблемные места с помощью статического анализатора.

Обо всех возможностях ScottPlot можно узнать на сайте проекта. На нём же представлен график, построенный с помощью библиотеки:

Как было сказано ранее, ScottPlot — библиотека. При разработке подобных решений стоит уделять особое внимание качеству кода. Ведь ошибки в таких проектах влияют не только на непосредственных пользователей библиотеки, но и на тех, кто использует приложения, написанные с её помощью. Это стало одной из причин, по которой мы решили проверить ScottPlot с помощью статического анализатора PVS-Studio.

Проверяемый код соответствует этому коммиту.

Приступим к разбору подозрительных мест.

Битовые фокусы

Фрагмент кода 1

public static Interactivity.Key GetKey(this Keys keys)
{
  
  Keys keyCode = keys & ~Keys.Modifiers;                   // <=
  Interactivity.Key key = keyCode switch
  {
    Keys.Alt => Interactivity.StandardKeys.Alt,            // <=
    Keys.Menu => Interactivity.StandardKeys.Alt,
    Keys.Shift => Interactivity.StandardKeys.Shift,        // <=
    Keys.ShiftKey => Interactivity.StandardKeys.Shift,
    Keys.LShiftKey => Interactivity.StandardKeys.Shift,
    Keys.RShiftKey => Interactivity.StandardKeys.Shift,
    Keys.Control => Interactivity.StandardKeys.Control,    // <=
    Keys.ControlKey => Interactivity.StandardKeys.Control,
    Keys.Down => Interactivity.StandardKeys.Down,
    Keys.Up => Interactivity.StandardKeys.Up,
    Keys.Left => Interactivity.StandardKeys.Left,
    Keys.Right => Interactivity.StandardKeys.Right,
    _ => Interactivity.StandardKeys.Unknown,
  };

  ....
}

Предупреждение PVS-Studio: V3202 Unreachable code detected. The 'case' value is out of range of the match expression. ScottPlot.WinForms FormsPlotExtensions.cs 106

Несколько значений паттернов внутри switch невозможны в текущем контексте. Давайте попробуем разобраться, в чём тут дело.

Сперва стоит рассмотреть значения, которые соответствуют "проблемным" элементам перечисления.

[Flags]
[TypeConverter(typeof(KeysConverter))]
[Editor(....)]
public enum Keys
{
  /// <summary>
  ///  The bit mask to extract modifiers from a key value.
  /// </summary>
  Modifiers = unchecked((int)0xFFFF0000),

  ....
  /// <summary>
  ///  The SHIFT modifier key.
  /// </summary>
  Shift = 0x00010000,

  /// <summary>
  ///  The  CTRL modifier key.
  /// </summary>
  Control = 0x00020000,

  /// <summary>
  ///  The ALT modifier key.
  /// </summary>
  Alt = 0x00040000
}

Далее переведём их в двоичную систему:

Имя	Значение (16-ая система)	Бинарное представление (2-ая система)
Modifiers	0xFFFF0000	1111 1111 1111 1111 0000 0000 0000 0000
Shift	0x00010000	0000 0000 0000 0001 0000 0000 0000 0000
Control	0x00020000	0000 0000 0000 0010 0000 0000 0000 0000
Alt	0x00040000	0000 0000 0000 0100 0000 0000 0000 0000

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

Значение, передаваемое в switch, получается из выражения keys & ~Keys.Modifiers. Это выражение исключает из keys значение Keys.Modifiers. Однако, помимо Keys.Modifiers, также будут исключены Shift, Control и Alt, так как Modifiers уже включает в себя эти значения (исходя из того, что для каждого ненулевого бита "проблемных" элементов перечисления у Modifiers есть ненулевой бит).

Из всего этого мы можем сделать вывод, что нет такой комбинации битов, которая позволит получить Shift, Control или Alt для операции keys & ~Keys.Modifiers.

Возможно, проблема заключается не в значениях перечисления, а в реализации switch.

Подозрительные свойства

Фрагмент кода 2

internal double BackAngleSweep
{
  get
  {
    double maxBackAngle = CircularBackground ? 360 : MaximumSizeAngle;
    if (!Clockwise) maxBackAngle = -maxBackAngle;
    return maxBackAngle;
  }

  private set { BackAngleSweep = value; }
}

Предупреждение PVS-Studio: V3110 Possible infinite recursion inside 'BackAngleSweep' property. ScottPlot RadialGauge.cs 45

Весьма необычная реализация setter'а свойства BackAngleSweep. Если приглядеться, то можно заметить, что в setter'е свойство присваивается само себе. Это приведёт к возникновению исключения "Stack Overflow".

Посмотрев на использования этого свойства, я обнаружил, что нет мест, где в него присваивается значение. Из этого следует, что setter вообще не отрабатывает. Если разработчик захочет что-то записать в BackAngleSweep, то неминуемо получит "Stack Overflow".

Фрагмент кода 3

public bool Rounded
{
    get => StrokeCap == SKStrokeCap.Round;
    set { StrokeCap = SKStrokeCap.Round; StrokeJoin = SKStrokeJoin.Round; }
}

Предупреждение PVS-Studio: V3077 The setter of 'Rounded' property does not utilize its 'value' parameter. ScottPlot LineStyle.cs 36

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

Сможем сравнить?

Фрагмент кода 4

public class CoordinateRangeMutable : IEquatable<CoordinateRangeMutable>
{
  ....
  public bool Equals(CoordinateRangeMutable? other)
  {
    if (other is null)
      return false;

    return Equals(Min, other.Min) && Equals(Min, other.Min);  // <=
  }

  public override bool Equals(object? obj)
  {
    if (obj is null)
      return false;

    if (obj is CoordinateRangeMutable other)
      return Equals(other);

    return false;
  }

  public override int GetHashCode()
  {
    return Min.GetHashCode() ^ Max.GetHashCode();             // <=
  }
}

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

V3192 The 'Max' property is used in the 'GetHashCode' method but is missing from the 'Equals' method. ScottPlot CoordinateRangeMutable.cs 198

V3001 There are identical sub-expressions 'Equals(Min, other.Min)' to the left and to the right of the '&&' operator. ScottPlot CoordinateRangeMutable.cs 172

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

Начнём со срабатывания диагностики V3192. В сообщении анализатора говорится, что свойство Max используется в методе GetHashCode, но не используется в методе Equals. Посмотрев на переопределённый метод Equals, можно заметить, что в его теле вызывается ещё один Equals. В нём мы видим следующую запись: Equals(Min, other.Min) && Equals(Min, other.Min). А на этот фрагмент указала диагностика V3001.

Очевидно, что один из операндов && должен иметь следующий вид: Equals(Max, other.Max).

Получается, что анализатор прав, в методе Equals действительно не фигурирует Max.

Фрагмент кода 5

public class CoordinateRangeMutable : IEquatable<CoordinateRangeMutable>
{
  ....
  public static bool operator ==(CoordinateRangeMutable a, 
                                 CoordinateRangeMutable b)
  {
    return a.Equals(b);
  }

  public static bool operator !=(CoordinateRangeMutable a, 
                                 CoordinateRangeMutable b)
  {
    return !a.Equals(b);
  }
}

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

V3115 Passing 'null' to '==' operator should not result in 'NullReferenceException'. ScottPlot CoordinateRangeMutable.cs 188

V3115 Passing 'null' to '!=' operator should not result in 'NullReferenceException'. ScottPlot CoordinateRangeMutable.cs 193

Анализатор не отпускает класс CoordinateRangeMutable. Следующие предупреждения указывают на то, что при использовании перегруженных операторов == и != возможно выбрасывание исключения NullReferenceException. И это действительно так: если при сравнении объектов типа CoordinateRangeMutable левый операнд будет null, то мы обязательно получим NullReferenceException.

Подобная ситуация возникает из-за того, что в реализациях перегрузок операторов сравнение вызывается Equals без проверки на null.

Всегда да

Фрагмент кода 6

private double GetIdealTickSpacing(CoordinateRange range,
                                   PixelLength axisLength,
                                   PixelLength maxLabelLength)
{
  int targetTickCount = (int)(axisLength.Length / maxLabelLength.Length) + 1;

  int radix = 10;                                          // <=
  int exponent = (int)Math.Log(range.Length, radix) + 1;
  double initialSpace = Math.Pow(radix, exponent);
  List<double> tickSpacings = [initialSpace];

  double[] divBy;
  if (radix == 10)                                         // <=
    divBy = [2, 2, 2.5]; // 10, 5, 2.5, 1
  else if (radix == 16)
    divBy = [2, 2, 2, 2]; // 16, 8, 4, 2, 1
  else
    throw new ArgumentException($"radix {radix} is not supported");

  ....
}

Предупреждение PVS-Studio: V3022 Expression 'radix == 10' is always true. ScottPlot DecimalTickSpacingCalculator.cs 42

Анализатор сообщает, что выражение radix == 10 всегда истинно. Не надо далеко ходить, чтобы убедиться в этом. Переменной radix единожды присваивается значение в самом начале метода. Следовательно, на момент проверки условия radix == 10 она всегда будет иметь значение 10.

Скорее всего, раньше значение этой переменной менялось при каком-то условии, но после внесения правок эта опция пропала. Стоит обращать внимание на такие моменты и своевременно подчищать код, который никак не влияет на работу программы.

Можно быстрее

Фрагмент кода 7

public void Apply(RenderPack rp, bool beforeLayout)
{
  ....

  if (isPanning & Math.Abs(newLimits.Max - oldRight) < tickDelta / 2)
  {
    newRight = oldRight;
    newLeft = oldLeft;
  }
  ....
}

Предупреждение PVS-Studio: V3093 The '&' operator evaluates both operands. Perhaps a short-circuit '&&' operator should be used instead. ScottPlot SnapToTicksX.cs 94

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

Подобное исправление могло бы изменить логику работы программы, например, если выполнение кода правого операнда изменяет что-то в глобальном скоупе (поле, свойство). Однако в нашем случае метод, находящийся в правом операнде, ничего не меняет, а лишь возвращает значение.

Отрицательный размер

Фрагмент кода 8

public class DraggableRows() : IMultiplotLayout
{

  readonly List<float> PlotHeights = [];

  ....

  float[] GetDividerPositions()
  {
    if (PlotHeights.Count == 1)
      return [PlotHeights[0]];

    float[] positions = new float[PlotHeights.Count - 1]; // <=

    positions[0] = PlotHeights[0];
    for (int i = 1; i < positions.Length; i++)
    {
      positions[i] = positions[i - 1] + PlotHeights[i];
    }

    return positions;
  }

  ....
}

Предупреждение PVS-Studio: V3171 The value used as the size of an array could reach -1. Consider inspecting: PlotHeights.Count - 1. ScottPlot DraggableRows.cs 42

Обратите внимание на создание массива positions. В качестве его размера используется PlotHeights.Count - 1. Однако нет гарантии, что PlotHeights.Count будет больше 0. Единственная проверка размера PlotHeights гарантирует, что на момент создания массива коллекция не будет содержать один элемент. Стоит отметить, что PlotHeights инициализируется пустой коллекцией. Это повышает вероятность использования отрицательного значения при указании размера массива.

Неиспользуемый параметр

Фрагмент кода 9

private void RenderColorbarAxis(RenderPack rp,
                                PixelRect colormapRect,
                                float size,
                                float offset)
{
  GenerateTicks(rp.DataRect);

  float size2 = Edge switch
  {
    Edge.Left => size - colormapRect.Width,
    Edge.Right => size - colormapRect.Width,
    Edge.Bottom => size - colormapRect.Height,
    Edge.Top => size - colormapRect.Height,
    _ => throw new NotImplementedException(),
  };

  float offset2 = Edge switch
  {
    Edge.Left => rp.DataRect.Left - colormapRect.Left,
    Edge.Right => colormapRect.Right - rp.DataRect.Right,
    Edge.Bottom => colormapRect.Bottom - rp.DataRect.Bottom,
    Edge.Top => rp.DataRect.Top - colormapRect.Top,
    _ => throw new NotImplementedException(),
  };

  Axis.Render(rp, size2, offset2);
}

Предупреждение PVS-Studio: V3196 The 'offset' parameter is not utilized inside the method body, but an identifier with a similar name is used inside the same method. ScottPlot ColorBar.cs 133

Достаточно тривиальная проблема, но безобиднее от этого она не становится. В методе не используется параметр offset. Основная проблема заключается в том, что вызывающий код высчитывает значения для передачи его в качестве аргумента (соответствующему параметру), что не совсем целесообразно.

При использовании метода RenderColorbarAxis разработчик может не обратить внимание на его реализацию. Соответственно, он будет думать, что offset, переданный в качестве аргумента, повлияет на логику работы метода.

Беспорядок

Фрагмент кода 10

public Rectangle Rectangle(CoordinateRect rect)
{
  return Rectangle(rect.Left, rect.Right, rect.Top, rect.Bottom);
}

public Rectangle Rectangle(double left, double right, double bottom, double top)
{
  Color color = GetNextColor();
  Rectangle rp = new()
  {
    X1 = left,
    X2 = right,
    Y1 = bottom,
    Y2 = top,
    LineColor = color,
    FillColor = color.WithAlpha(.5),
  };

  Plot.PlottableList.Add(rp);
  return rp;
}

Предупреждение PVS-Studio: V3066 Possible incorrect order of arguments passed to 'Rectangle' method: 'rect.Top' and 'rect.Bottom'. ScottPlot PlottableAdder.cs 1078

Анализатор сообщает, что при передаче аргументов в метод Rectangle был перепутан их порядок. И это действительно так: аргумент rect.Top соответствует параметру bottom, а аргумент rect.Bottom — параметру top.

Сложно однозначно сказать, допущена здесь ошибка или нет, но выглядит подозрительно. Даже если ошибка есть, то кажется, что она не является критичной, но это не отменяет факт её наличия :)

Заключение

На самом деле код проекта оказался достаточны чистым: анализатор выдал чуть более 50 предупреждений среднего и высокого уровня. Однако даже из их числа удалось выделить 10, как мне кажется, достойных для рассмотрения.

Интересной особенностью является то, что на проекте практически нет предупреждений, связанных с потенциальным разыменованием нулевой ссылки. А ведь именно такие ошибки обычно встречаются чаще всего при анализе проектов. В качестве примера предлагаю ознакомиться с другой проверкой проекта.

А если хотите проверить свой проект с помощью PVS-Studio, то попробовать анализатор можете по ссылке!

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Nikita Panevin. Graph mess: what does ScottPlot have in store?.

Графический переполох: что таит в себе ScottPlot?