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.
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 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 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:
Issue 1
For a better understanding of the problem, I decided to list the method's entire code:
/// <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 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:
So here are the general expectations:
Then the developer must check the returned bool and build the logic depending on the check's result.
The Microsoft documentation describes 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
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 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:
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:
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
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 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:
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:
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
protected virtual void OnChanging(BindingChangingEventArgs e)
{
if (Changing != null)
Changing(this, e);
}PVS-Studio warns: 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:
public event EventHandler<BindingChangingEventArgs> Changing;The class that contains the event is also public:
public abstract partial class BindingThe 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:
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.
protected virtual void OnChanging(BindingChangingEventArgs e)
{
EventHandler<BindingChangingEventArgs> safeChanging = Changing;
if (safeChanging != null)
safeChanging(this, e);
}Issue 5
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 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:
void SetLength(int panel, sw.GridLength value)The panel argument's value serves as an index inside the SetLength method:
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:
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
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 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:
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:
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:
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
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 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:
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:
public bool Enabled
{
get { return Control != null ? enabled : Control.Sensitive; }
set {
if (Control != null)
Control.Sensitive = value;
else
enabled = value;
}
}Where is it?
It's here:
get { return Control != null ? enabled : Control.Sensitive; }PVS-Studio warns: 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
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 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:
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:
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
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 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
public static IBinding BindingOfType(....)
{
....
var ofTypeMethod = bindingType.GetRuntimeMethods()
.FirstOrDefault(....);
return (IBinding)ofTypeMethod.MakeGenericMethod(toType)
.Invoke(...);
}PVS-Studio warns: 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:
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.
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.
I hope this article showed you how the PVS-Studio analyzer can find various mistakes. I invite you to try PVS-Studio and check the projects you are interested in.
Thank you for your time, see you in the next articles!