# Looking for errors in the C\# code of the Eto\.Forms GUI framework

GUI frameworks are becoming increasingly popular: new ones appear, and old ones get a new life\. At PVS\-Studio, we are watching this trend very closely\. Today we'll examine suspicious code fragments in one of C\# frameworks — Eto\.Forms\.

![0929_Eto/image1.png](https://import.viva64.com/docx/blog/0929_Eto/image1.png)

## Introduction

Eto\.Forms \(or just Eto\) is a GUI framework for development in the C\# and XAML languages\. The framework itself is written in C\#\. Most importantly, Eto is intended for cross\-platform development\. The framework allows creating GUI applications that run on the main desktop operating systems: Windows, Linux, and macOS\. Supporting the Android and iOS mobile platforms is under development\.

By the way, [PVS\-Studio](https://pvs-studio.com/en/) is the static analyzer that enabled us to collect errors for this review\. And it works on all these operating systems\. Aside from mobile platforms, of course :\)

While working on this article, we used the analyzer's 7\.17 version and the [Eto\.Forms source code](https://github.com/picoe/Eto) dated February 10, 2022\.

This is not our first time to check a framework intended for building GUI applications on C\#\. Before, we have checked the following:

* [Avalonia UI](https://pvs-studio.com/en/blog/posts/csharp/0701/);
* [Xamarin\.Forms](https://pvs-studio.com/en/blog/posts/csharp/0400/);
* [Windows Forms](https://pvs-studio.com/en/blog/posts/csharp/0653/)\.

## The analyzer's warnings

**Issue 1**

For a better understanding of the problem, I decided to list the method's entire code:

```cpp
/// <summary>
/// ....
/// </summary>
/// ....
/// <returns>True if successful, 
/// or false if the value could not be parsed
// </returns>
public static bool TryParse(string value, out DashStyle style)
{
  if (string.IsNullOrEmpty(value))
  {
    style = DashStyles.Solid;
    return true;
  }

  switch (value.ToUpperInvariant())
  {
    case "SOLID":
        style = DashStyles.Solid;
        return true;
      case "DASH":
        style = DashStyles.Dash;
        return true;
      case "DOT":
        style = DashStyles.Dot;
        return true;
      case "DASHDOT":
        style = DashStyles.DashDot;
        return true;
      case "DASHDOTDOT":
        style = DashStyles.DashDotDot;
        return true;
  }
  var values = value.Split(',');
  if (values.Length == 0)
  {
    style = DashStyles.Solid;
    return true;
  }
  float offset;
  if (!float.TryParse(values[0], out offset))
    throw new ArgumentOutOfRangeException("value", value);
  float[] dashes = null;
  if (values.Length > 1)
  {
    dashes = new float [values.Length - 1];
    for (int i = 0; i < dashes.Length; i++)
    {
      float dashValue;
      if (!float.TryParse(values[i + 1], out dashValue))
        throw new ArgumentOutOfRangeException("value", value);
      dashes[i] = dashValue;
    }
  }

  style = new DashStyle(offset, dashes);
  return true;
}
```

PVS\-Studio warns: [V3009](https://pvs-studio.com/en/docs/warnings/v3009/) It's odd that this method always returns one and the same value of 'true'\. Eto DashStyle\.cs 56

The analyzer warned that, in all of the numerous branches, the method always returns _true_\.

Let's figure out what's wrong in this code\. I'll start with the fact that methods, whose name includes the TryParse prefix, usually follow the corresponding pattern and have the following features:

* they return _bool_;
* they take an _out_ parameter;
* no exceptions are thrown\.

So here are the general expectations:

* when an operation is successful, the method returns _true,_ and the _out_ argument gets the required value;
* otherwise, the method returns _false_, and the _out_ argument gets the _default_ value\.

Then the developer must check the returned _bool_ and build the logic depending on the check's result\.

The Microsoft documentation [describes](https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/exceptions-and-performance) this pattern\. It was created to prevent exceptions during parsing\.

However, the method in the Eto code returns a value only if the input data is correct — otherwise an exception is thrown\. This logic is opposite to the logic of the Try\-Parse pattern — the method does not conform to this approach\. This makes the "TryParse" prefix dangerously confusing for those developers who know and use this pattern\.

By the way, this method has an XML comment: _<returns\>True if successful, or false if the value could not be parsed</returns\>_\. Unfortunately, the comment carries false information\.

**Issue 2**

```cpp
public static IEnumerable<IPropertyDescriptor> GetProperties(Type type)
{
  if (s_GetPropertiesMethod != null)
    ((ICollection)s_GetPropertiesMethod.Invoke(null, new object[] { type }))
                                       .OfType<object>()
                                       .Select(r => Get(r));  // <=
  return type.GetRuntimeProperties().Select(r => Get(r));
}
```

PVS\-Studio warns: [V3010](https://pvs-studio.com/en/docs/warnings/v3010/) The return value of function 'Select' is required to be utilized\. Eto PropertyDescriptorHelpers\.cs 209

The analyzer found that the value the _Select_ method returns is never used\.

_Select _is a LINQ extension method of type _IEnumerable<T\>_\. _Select_'s argument is a projecting function, while the result is an enumeration of elements that this function returns\. There is always a possibility that the _Get_ method has side effects\. However, since LINQ is lazy, _Get_ will not be executed for any element of the collection\. The error that involves the unused result becomes clear even here\.

If you take a closer look at the code, you'll find that the _Get_ method used in the lambda, returns _IPropertyDescriptor_:

```cpp
public static IPropertyDescriptor Get(object obj)
{
  if (obj is PropertyInfo propertyInfo)
    return new PropertyInfoDescriptor(propertyInfo);
  else
    return PropertyDescriptorDescriptor.Get(obj);
}
```

This means that the _Select_ method returns a collection of the following type: _IEnumerable<IPropertyDescriptor\>_\. This type is the same as the type of the value that the _GetProperties_ method returns\. This method's code triggered the analyzer\. Most likely, the developer lost the _return_ statement here:

```cpp
public static IEnumerable<IPropertyDescriptor> GetProperties(Type type)
{
  if (s_GetPropertiesMethod != null)
    return 
     ((ICollection)s_GetPropertiesMethod.Invoke(null, new object[] { type }))
                                        .OfType<object>()
                                        .Select(r => Get(r));
  return type.GetRuntimeProperties().Select(r => Get(r));
}
```

**Issue 3**

```cpp
public override string Text
{
  get { return base.Text; }
  set
  {
    var oldText = Text;
    var newText = value ?? string.Empty;               // <=
    if (newText != oldText)
    {
      var args = new TextChangingEventArgs(oldText, newText, false);
      Callback.OnTextChanging(Widget, args);
      if (args.Cancel)
        return;
      base.Text = value;
      if (AutoSelectMode == AutoSelectMode.Never)
        Selection = new Range<int>(value.Length,       // <=
                                   value.Length - 1);  // <=
    }
  }
```

PVS\-Studio warns: [V3125](https://pvs-studio.com/en/docs/warnings/v3125/) The 'value' object was used after it was verified against null\. Check lines: 329, 320\. Eto\.WinForms\(net462\) TextBoxHandler\.cs 329

The analyzer indicates that the reference was checked for _null_ but was then used without the check\.

So what's going to happen if the _value_ is _null_?

The null coalescing operator is used to check _value_ for _null_\. The _newText_ string gets the value of _string\.Empty_\. If _oldText_ did not contain an empty string before, the execution flow will follow to the _then_ branch\. Then _null_ is assigned to a property inside the branch:

```cpp
base.Text = value;
```

Now this looks strange\. Earlier the developer checked value for null and introduced the newText variable that is definitely not null\. It is possible there here and further on the developer intended to use _newText_\.

But wait a second, that's not all\. Let's look at the code further\. A few lines lower _value_ is dereferenced:

```cpp
Selection = new Range<int>(value.Length,  // <=
                           value.Length - 1);
```

Here _value_ can still be _null_\. If the execution flow reaches this code and _value_ will be _null_, the _NullReferenceException_ will be thrown\.

**Issue 4**

```cpp
protected virtual void OnChanging(BindingChangingEventArgs e)
{
  if (Changing != null)
    Changing(this, e);
}
```

PVS\-Studio warns: [V3083](https://pvs-studio.com/en/docs/warnings/v3083/) Unsafe invocation of event 'Changing', NullReferenceException is possible\. Consider assigning event to a local variable before invoking it\. Eto Binding\.cs 80

The analyzer reported that it's unsafe to raise the event, because there's no guarantee that subscribers exist\.

Yes, there is an if check _\(Changing \!\= null\)_\. However, the number of subscribers can change between the check and the call\. The error will appear if this event is used in multi\-threaded code\. The event is declared as follows:

```cpp
public event EventHandler<BindingChangingEventArgs> Changing;
```

The class that contains the event is also public:

```cpp
public abstract partial class Binding
```

The _public_ modifier raises the likelihood of someone using the _Changing_ event somewhere in the project's code, including mutithreaded code\.

To raise the event, we recommend using the _Invoke_ method and the Elvis operator:

```cpp
protected virtual void OnChanging(BindingChangingEventArgs e)
{
  Changing?.Invoke(this, e);
}
```

If this approach is for some reason impossible to use, we recommend employing a local variable to store the event handler reference — and working with that variable rather than the event handler\.

```cpp
protected virtual void OnChanging(BindingChangingEventArgs e)
{
  EventHandler<BindingChangingEventArgs> safeChanging = Changing;
  if (safeChanging != null)
    safeChanging(this, e);
}
```

**Issue 5**

```cpp
void UpdateColumnSizing(....)
{
  ....
  switch (FixedPanel)
  {
    case SplitterFixedPanel.Panel1:
      SetLength(0, new sw.GridLength(1, sw.GridUnitType.Star));  // <=
      break;
    case SplitterFixedPanel.Panel2:
      SetLength(0, new sw.GridLength(1, sw.GridUnitType.Star));  // <=
      break;
    case SplitterFixedPanel.None:
      SetLength(0, new sw.GridLength(1, sw.GridUnitType.Star));
      SetLength(2, new sw.GridLength(1, sw.GridUnitType.Star));
      break;
  }
  ....
}
```

PVS\-Studio warns: [V3139](https://pvs-studio.com/en/docs/warnings/v3139/) Two or more case\-branches perform the same actions\. Eto\.Wpf\(net462\) SplitterHandler\.cs 357

The analzyer detected that a _switch_ block contains different _case_ branches with identical code\.

_switch_ covers three _SplitterFixedPanel_ enumeration elements, two of which are named _Panel1_ and _Panel2_\. The _SetLength_ method has the following signature and is called in both branches:

```cpp
void SetLength(int panel, sw.GridLength value)
```

The _panel_ argument's value serves as an index inside the _SetLength_ method:

```cpp
Control.ColumnDefinitions[panel] = ....
```

The third branch covers the _None_ element\. I'll assume that it combines the code for both panels\. The use of magical numbers "0" and "2" is likely correct, because here we work with the "SplitContainer" standard control\. Number "1" corresponds to the separator that is not mentioned here\. We assume, the code must look as follows:

```cpp
void UpdateColumnSizing(....)
{
  ....
  switch (FixedPanel)
  {
    case SplitterFixedPanel.Panel1:
      SetLength(0, new sw.GridLength(1, sw.GridUnitType.Star));
      break;
    case SplitterFixedPanel.Panel2:
      SetLength(2, new sw.GridLength(1, sw.GridUnitType.Star));
      break;
    case SplitterFixedPanel.None:
      SetLength(0, new sw.GridLength(1, sw.GridUnitType.Star));
      SetLength(2, new sw.GridLength(1, sw.GridUnitType.Star));
      break;
  }
  ....
}
```

**Issue 6**

```cpp
public Font SelectionFont
{
  get
  {
    ....
    Pango.FontDescription fontDesc = null;
    ....
    foreach (var face in family.Faces)
    {
      var faceDesc = face.Describe();
      if (   faceDesc.Weight == weight 
          && faceDesc.Style == style 
          && faceDesc.Stretch == stretch)
      {
        fontDesc = faceDesc;
        break;
      }
    }
    if (fontDesc == null)
      fontDesc = family.Faces[0]?.Describe();   // <=
    var fontSizeTag = GetTag(FontSizePrefix);
    fontDesc.Size =   fontSizeTag != null       // <=
                    ? fontSizeTag.Size
                    : (int)(Font.Size * Pango.Scale.PangoScale);
    ....
  }
}
```

PVS\-Studio warns: [V3105](https://pvs-studio.com/en/docs/warnings/v3105/) The 'fontDesc' variable was used after it was assigned through null\-conditional operator\. NullReferenceException is possible\. Eto\.Gtk3 RichTextAreaHandler\.cs 328

The analyzer reports that the code uses a variable that has not been checked and can be _null_\. This happens because when assigning a value to the variable, the developer used a null\-conditional operator\.

The _fontDesc_ variable is assigned _null_ when declared\. If a new value hasn't been assigned inside the _foreach_ loop, there is one more branch that assigns a value to _fontDesc_\. However, the assignment code uses a null\-conditional \(Elvis\) operator:

```cpp
fontDesc = family.Faces[0]?.Describe();
```

This means that if an array's first element is _null_, then _fontDesc_ will be assigned _null_\. Then follows the dereference:

```cpp
fontDesc.Size = ....
```

If _fontDesc_ is _null_, attempting to assign a value to the _Size_ property will cause the _NullReferenceException_ exception\.

However, it looks like the developers missed the null\-conditional operator or added it accidentally\. If _family\.Faces\[0\]_ is assigned _null_, _NullReferenceException_ will be thrown as early as the _foreach_ loop\. There the dereference takes place:

```cpp
foreach (var face in family.Faces)
{
  var faceDesc = face.Describe(); // <=
  if (   faceDesc.Weight == weight 
      && faceDesc.Style == style 
      && faceDesc.Stretch == stretch)
  {
    fontDesc = faceDesc;
    break;
  }
}
```

**Issue 7**

```cpp
public override NSObject GetObjectValue(object dataItem)
{
  float? progress = Widget.Binding.GetValue(dataItem);  // <=
  if (Widget.Binding != null && progress.HasValue)      // <=
  {
    progress = progress < 0f ? 0f : progress > 1f ? 1f : progress;
    return new NSNumber((float)progress);
  }
  return new NSNumber(float.NaN);
}
```

PVS\-Studio warns: [V3095](https://pvs-studio.com/en/docs/warnings/v3095/) The 'Widget\.Binding' object was used before it was verified against null\. Check lines: 42, 43\. Eto\.Mac64 ProgressCellHandler\.cs 42

The analyzer pointed out that the code first dereferences the reference and only then checks it for _null_\.

If _Widget\.Binding_ is _null_, the _GetValue_ method will throw the _NullReferenceException_ exception\. The check that follows — _Widget\.Binding \!\= null_ — is useless\. To fix this code, you can change the condition and simplify the code by employing the Elvis operator we've already mentioned\. A better version of the code may look as follows:

```cpp
public override NSObject GetObjectValue(object dataItem)
{
  float? progress = Widget.Binding?.GetValue(dataItem);
  if (progress.HasValue)
  {
    progress =   progress < 0f 
               ? 0f 
               : (progress > 1f 
                  ? 1f 
                  : progress);
    return new NSNumber((float)progress);
  }
  return new NSNumber(float.NaN);
}
```

**Issue 8**

In the code below, try finding the error yourself:

```cpp
public bool Enabled
{
  get { return Control != null ? enabled : Control.Sensitive; }
  set {
    if (Control != null)
      Control.Sensitive = value;
    else
      enabled = value;
  }
}
```

Where is it?

![0929_Eto/image2.png](https://import.viva64.com/docx/blog/0929_Eto/image2.png)

It's here:

```cpp
get { return Control != null ? enabled : Control.Sensitive; }
```

PVS\-Studio warns: [V3080](https://pvs-studio.com/en/docs/warnings/v3080/) Possible null dereference\. Consider inspecting 'Control'\. Eto\.Gtk3 RadioMenuItemHandler\.cs 143

The analyzer reports a possible dereference of a null reference\.

The check is useless and does not protect against _NullReferenceException_\. If the condition is true, the ternary operator calculates the first expression, otherwise the operator calculates the second expression\.  If _Control_ is _null_, the expression becomes false, and a null reference is dereferenced\. This will obviously cause _NullReferenceException_\.

**Issue 9**

```cpp
public NSShadow TextHighlightShadow
{
  get
  {
    if (textHighlightShadow == null)
    {
      textHighlightShadow = new NSShadow();
      textHighlightShadow.ShadowColor = NSColor.FromDeviceWhite(0F, 0.5F);
      textHighlightShadow.ShadowOffset = new CGSize(0F, -1.0F);
      textHighlightShadow.ShadowBlurRadius = 2F;
    }
    return textHighlightShadow;
  }
  set { textShadow = value; }
}
```

PVS\-Studio warns: [V3140](https://pvs-studio.com/en/docs/warnings/v3140/) Property accessors use different backing fields\. Eto\.Mac64 MacImageAndTextCell\.cs 162

The analyzer detected that the property's getter and setter use different fields\. The setter uses _textShadow_, the getter _— textHighlightShadow_\. If we take a look at the property name — _TextHighlightShadow_ — it becomes clear that the correct field is _textHighlightShadow_\. Here is the field's declaration:

```cpp
public class MacImageListItemCell : EtoLabelFieldCell
{
  ....
  NSShadow textHighlightShadow;
}
```

The _textHighlightShadow_ field is initialized only inside the _TextHighlightShadow_ property\. This way, the value assigned to the property is not connected to the value this property returns\. The return value will always be the same object\. When the execution flow retrieves the property value for the first time, _textHighlightShadow_ is always null\. So, the getter creates this object and sets several properties of this object to predefined values\.  At the same time, the code contains the _TextShadow_ property that works with the _textShadow_ field:

```cpp
public NSShadow TextShadow
{
  get
  {
    if (textShadow == null)
    {
      textShadow = new NSShadow();
      textShadow.ShadowColor = NSColor.FromDeviceWhite(1F, 0.5F);
      textShadow.ShadowOffset = new CGSize(0F, -1.0F);
      textShadow.ShadowBlurRadius = 0F;
    }
    return textShadow;
  }
  set { textShadow = value; }
}
```

Since the _TextHighlightShadow_ setter uses the _textShadow_ field, _TextShadow_ will change each time _TextHighlightShadow_ changes\. We doubt that the developer intended to implement this behavior\.

**Issue 10**

```cpp
public static NSImage ToNS(this Image image, int? size = null)
{
  ....
  if (size != null)
  {
    ....
    var sz = (float)Math.Ceiling(size.Value / mainScale);  // <=
    sz = size.Value;  // <=
  }
  ....
}
```

PVS\-Studio warns: [V3008](https://pvs-studio.com/en/docs/warnings/v3008/) The 'sz' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 296, 295\. Eto\.Mac64 MacConversions\.cs 296

The analyzer warned that a variable that carries a value is assigned a different value — without its previous value used\.

The _sz_ variable is declared and initialized on one line\. On the next line, the _sz_ value is rewritten\. This makes calculating the initial value useless\.

**Issue 11**

```cpp
public static IBinding BindingOfType(....)
{
  ....
  var ofTypeMethod = bindingType.GetRuntimeMethods()
                                .FirstOrDefault(....);
  return (IBinding)ofTypeMethod.MakeGenericMethod(toType)
                               .Invoke(...);
}
```

PVS\-Studio warns: [V3146](https://pvs-studio.com/en/docs/warnings/v3146/) Possible null dereference of 'ofTypeMethod'\. The 'FirstOrDefault' can return default null value\. Eto BindingExtensionsNonGeneric\.cs 21

The analyzer reports that the _FirstOrDefault_ method, that is used to initialize the _ofTypeMethod_ variable, can return _null_\. Dereferencing _ofTypeMethod_, without first checking it for null, may cause _NullReferenceExpression_\.

If the developer is confident that the element will be found, we recommend using the _First_ method:

```cpp
var ofTypeMethod = bindingType.GetRuntimeMethods()
                               .First(r => 
                                         r.Name == "OfType"
                                      && r.GetParameters().Length == 2);
```

However, if there's no guarantee — and there is a chance the method fails to find an element that corresponds to the predicate, _First_ will throw _InvalidOperationException_\. We can argue on what is better: _NullReferenceException_ or _InvalidOperationException_\. This code may require a deeper refactoring\.

## Conclusion

There was a time when the \.NET reference implementation was closely tied to Windows\. One of the advantages the ecosystem offered was the ability to develop GUI applications quickly\. With time, we saw cross\-platform frameworks — Mono, Xamarin, and, eventually, \.NET Core\. One of the community's first wishes was porting GUI frameworks from Windows to new platforms\. The programming world saw many frameworks for C\# and XAML development: Avalonia UI, Uno Platform, and Eto\.Forms\. If you know of a similar project we haven't mentioned, please let us know in the comments\. It feels a bit strange to wish these good projects more competitors — but competition drives progress\. 

PVS\-Studio can help developers of these projects to enhance their code quality\. Moreover — non\-commercial open\-source projects can use the analyzer for [free](https://pvs-studio.com/en/blog/posts/0600/)\.

I hope this article showed you how the PVS\-Studio analyzer can find various mistakes\. I invite you to [try PVS\-Studio](https://pvs-studio.com/en/pvs-studio/try-free/) and check the projects you are interested in\.

Thank you for your time, see you in the next articles\!