Nikita Panevin

Mar 20 2025

Tags:

#CSharp

Graph mess: what does ScottPlot have in store?

• Bit tricks
• Suspicious properties
• Can we compare?
• Always true
• It could go faster
• Negative size
• Unused parameter
• Mess
• Conclusion

ScottPlot is a library under .NET for creating graphs. The code in such projects tends to be confusing in nature. Today, we'll try to unravel it and find issues using a static analyzer.

You can learn about all ScottPlot features on the project website. It also shows a graph plotted using the library:

As mentioned earlier, ScottPlot is a library. Developers should pay special attention to code quality when developing such solutions. After all, errors in this kind of projects affect not only the direct library users, but also those who use applications written with the help of it. This was one of the reasons why we decided to check ScottPlot using the PVS-Studio static analyzer.

The checked code matches this commit.

Let's start breaking down the suspicious code fragments.

Bit tricks

Code fragment 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,
  };

  ....
}

The PVS-Studio warning: V3202 Unreachable code detected. The 'case' value is out of range of the match expression. ScottPlot.WinForms FormsPlotExtensions.cs 106

Multiple pattern values within switch are impossible in the current context. Let's see what's going on here.

First, we should look at the values that correspond to the erroneous elements of the enumeration.

[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
}

Next, let's convert them to binary:

It's clear now that Modifiers include each of the erroneous enumeration elements.

The value passed to switch is obtained from the keys & ~Keys.Modifiers expression. This expression excludes the Keys.Modifiers value from keys. In addition to Keys.Modifiers, however, Shift, Control, and Alt will also be excluded, since Modifiers already include these values (Modifiers have a non-zero bit for each non-zero bit of the erroneous enumeration elements).

From all this we can conclude that the bit combination that produces Shift, Control, or Alt for the keys & ~Keys.Modifiers operation doesn't exist.

The issue may lie in the switch implementation rather than the enumeration values.

Suspicious properties

Code fragment 2

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

  private set { BackAngleSweep = value; }
}

The PVS-Studio warning: V3110 Possible infinite recursion inside 'BackAngleSweep' property. ScottPlot RadialGauge.cs 45

A very unusual setter implementation of the BackAngleSweep property. If we look closely, we can see that the property is assigned to itself in the setter. This will result in a StackOverflowException.

When I looked at the uses of this property, I noticed that it has no value assigned to it anywhere. This implies that the setter doesn't even do anything. If a developer wants to write something to BackAngleSweep, they will inevitably get a "Stack Overflow".

Code fragment 3

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

The PVS-Studio warning: V3077 The setter of 'Rounded' property does not utilize its 'value' parameter. ScottPlot LineStyle.cs 36

Something obscure is brewing in the property setter again. Assigning any value to it results in the loss of that value because it isn't written to value or stored anywhere else.

Can we compare?

Code fragment 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();             // <=
  }
}

The PVS-Studio warnings:

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

The analyzer issued two warnings for this code fragment.

Let's start with the V3192 warning. The analyzer message says that the Max property is used in the GetHashCode method but not in the Equals method. If we look at the overridden Equals method, we can see that another Equals is called in its body. There we can see the following: Equals(Min, other.Min) && Equals(Min, other.Min). The V3001 diagnostic rule pointed out this fragment.

Clearly, one of the && operands must have the Equals(Max, other.Max) form.

So, the analyzer is right—Max doesn't appear in the Equals method.

Code fragment 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);
  }
}

The PVS-Studio warnings:

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

The analyzer keeps issuing warnings for the CoordinateRangeMutable class. The new warnings indicate that the == and != overloaded operators may cause a NullReferenceException. This is true: if the left operand is null when comparing objects of the CoordinateRangeMutable type, we will definitely get the NullReferenceException.

This happens because, in operator overloading implementations, Equals calls a comparison without performing the null check.

Always true

Code fragment 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");

  ....
}

The PVS-Studio warning: V3022 Expression 'radix == 10' is always true. ScottPlot DecimalTickSpacingCalculator.cs 42

The analyzer reports that the radix == 10 expression is always true—we don't have to look far to see it. At the beginning of the method, a value is assigned to the radix variable once. So, when the radix == 10 condition is checked, its value is always 10.

Most likely, the variable value used to change under some condition, but that condition disappeared after the code was edited. It's important to notice such things and promptly clean up the code that doesn't affect the program operation.

It could go faster

Code fragment 7

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

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

The PVS-Studio warning: V3093 The '&' operator evaluates both operands. Perhaps a short-circuit '&&' operator should be used instead. ScottPlot SnapToTicksX.cs 94

The analyzer suggests replacing & with && because calculating the right part of a logical expression is meaningless if the left operand is false.

Such a fix could change the program logic, for example, if the execution of the right operand code changes something in the global scope (a field or property). However, in our case, the method in the right operand doesn't change anything, it just returns a value.

Negative size

Code fragment 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;
  }

  ....
}

The PVS-Studio warning: V3171 The value used as the size of an array could reach -1. Consider inspecting: PlotHeights.Count - 1. ScottPlot DraggableRows.cs 42

Take a look at how the positions array is created. It uses PlotHeights.Count - 1 as its size. However, there's no guarantee that PlotHeights.Count will be greater than 0. The only PlotHeights size check ensures that the collection doesn't contain a single element when the array is created. It's worth noting that PlotHeights is initialized as an empty collection. This increases the chance of using a negative value when specifying the array size.

Unused parameter

Code fragment 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);
}

The PVS-Studio warning: 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

This is a rather common issue, but it doesn't make it any more harmless. The offset parameter isn't used in the method. The main issue is that the calling code calculates values to pass it as an argument to the corresponding parameter, which isn't really useful.

When using the RenderColorbarAxis method, a developer may overlook its implementation and mistakenly think that offset passed as an argument will affect the logic of the method.

Mess

Code fragment 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;
}

The PVS-Studio warning: V3066 Possible incorrect order of arguments passed to 'Rectangle' method: 'rect.Top' and 'rect.Bottom'. ScottPlot PlottableAdder.cs 1078

The analyzer reports that the argument order was mixed up when passing them to the Rectangle method. This is true: the rect.Top argument is passed to the bottom parameter, and the rect.Bottom argument is passed to top.

It's hard to tell for sure whether there's an error here, but it looks suspicious. Even though the error is there, it doesn't look critical. However, this fact doesn't mean it isn't there :)

Conclusion

The project code is quite clean—the analyzer issued a little more than 50 medium- and high-level warnings. Even so, I managed to pick 10 that seemed worth a look.

An interesting thing here is that the project has little to no warnings related to potential null dereferencing. Yet, such errors are the ones that developers usually encounter when analyzing projects. For comparison, I invite you to read another project check.

And if you want to check your project using PVS-Studio, you can follow this link to try the analyzer!

#CSharp

0

0

0

0

SHARE

Tags:

#CSharp

We can email you a selection of our best articles once a month