# Ищем ошибки в C\# коде GUI\-фреймворка Eto\.Forms

Популярность GUI\-фреймворков для \.NET постоянно растёт – появляются новые, развиваются старые\. Мы решили не обходить эту тему стороной и рассмотреть подозрительные места, найденные в C\# коде одного из таких проектов – Eto\.Forms\.

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

## Введение

Eto\.Forms \(или просто Eto\) – это один из GUI\-фреймворков, использующих C\# и XAML для разработки\. Сам он также написан на C\#\. Важной чертой Eto является кроссплатформенность: он позволяет создавать приложения с графическим интерфейсом для основных десктопных ОС: Windows, Linux и macOS\. Поддержка мобильных платформ Android и iOS находится в разработке\.

Кстати, [PVS\-Studio](https://pvs-studio.ru/ru/) – тот самый анализатор, который позволил нам собрать ошибки для данного обзора, работает на всех этих ОС\. Кроме, конечно, мобильных платформ :\)

В этой статье был использован анализатор версии 7\.17 и [исходники Eto\.Forms](https://github.com/picoe/Eto) от 10\.02\.2022\.

Ранее мы уже проверяли несколько фреймворков для разработки GUI\-приложений, написанных на C\#:

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

## Предупреждения анализатора

**Issue 1**

Для лучшего понимания проблемы я решил привести код метода полностью:

```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: [V3009](https://pvs-studio.ru/ru/docs/warnings/v3009/) It's odd that this method always returns one and the same value of 'true'\. Eto DashStyle\.cs 56

Анализатор предупредил, что метод возвращает только _true_ во всех своих многочисленных ветках с возвратами\.

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

* возврат _bool_;
* наличие _out_\-параметра;
* отсутствие выброса исключений\.

Предполагается, что:

* когда операция успешна, возвращается _true_, а _out_\-аргумент получает требуемое значение;
* иначе возвращается _false_, а _out_\-аргумент получает _default_\-значение\.

Затем программист должен проверить возвращённый _bool_ и построить логику исходя из результатов проверки\.

Этот паттерн [описан](https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/exceptions-and-performance) в документации Microsoft\. Он был придуман, чтобы избежать выбрасывания исключений при парсинге\.

В этом методе возврат происходит только при изначально корректных входных данных – иначе выбрасывается исключение\. Эта логика противоположна описанной в паттерне Try\-Parse – метод не соответствует ему\. Это делает префикс "TryParse" опасно запутывающим для программистов, знающих этот паттерн\.

К слову, у метода есть XML\-комментарий: _<returns\>True if successful, or false if the value could not be parsed</returns\>_\. К сожалению, он несёт ложную информацию\.

**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: [V3010](https://pvs-studio.ru/ru/docs/warnings/v3010/) The return value of function 'Select' is required to be utilized\. Eto PropertyDescriptorHelpers\.cs 209

Анализатор обнаружил, что возвращаемое значение метода _Select_ не используется\.

_Select_ – это один из LINQ\-методов расширения типа _IEnumerable<T\>_\. Аргументом _Select_ является проецирующая функция, а результатом – перечисление элементов, возвращённых этой функцией\. Всегда есть вероятность того, что метод _Get_ имеет побочные эффекты, но из\-за ленивости LINQ ни для какого элемента коллекции _Get_ не будет выполнен\. Ошибка неиспользованного результата очевидна уже здесь\.

Если посмотреть код внимательнее, то окажется, что метод _Get_, используемый в лямбде, возвращает _IPropertyDescriptor_: 

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

Значит, типом возвращаемой методом _Select_ коллекции будет _IEnumerable<IPropertyDescriptor\>_\. Это точно такой же тип, как и у возвращаемого значения метода _GetProperties_, для кода которого было сгенерировано предупреждение\. Скорее всего, здесь был потерян _return_:

```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: [V3125](https://pvs-studio.ru/ru/docs/warnings/v3125/) The 'value' object was used after it was verified against null\. Check lines: 329, 320\. Eto\.WinForms\(net462\) TextBoxHandler\.cs 329

Анализатор указывает, что ссылка была проверена на _null_, но в дальнейшем использована без проверки\.

Что же произойдёт, если _value_ будет равен _null_?

Значение _value_ проверяется на _null_ с помощью null\-coalescing оператора\. Строка _newText_ получит значение _string\.Empty_\. Если _oldText_ ранее не содержал пустую строку, то управление перейдёт в _then_\-ветку\. Внутри ветки производится присваивание _null_ свойству:

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

Это уже выглядит странно\. Ранее значение _value_ было проверено на _null_, и была введена переменная _newText_, которая точно не является _null_\. Возможно, здесь и далее предполагалось использование _newText_\.

Но это не всё\. Посмотрим код дальше\. Несколькими строками ниже есть разыменование _value_:

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

И тут _value_ всё ещё может быть _null_\. Если управление дойдёт до этого кода и _value_ будет _null_, произойдёт выброс исключения _NullReferenceException_\.

**Issue 4**

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

Предупреждение PVS\-Studio: [V3083](https://pvs-studio.ru/ru/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

Анализатор выдал сообщение о том, что вызов события небезопасен, так как нет гарантии наличия подписчиков\.

Несмотря на наличие проверки _if \(Changing \!\= null\)_, количество подписчиков может измениться между проверкой и вызовом\. Ошибка проявится, если событие будет использовано в многопоточном коде\. Само событие объявлено так:

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

Класс, содержащий событие, также публичный:

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

Модификатор _public_ повышает вероятность использования события _Changing_ в любом коде, в том числе многопоточном\.

Следует использовать метод _Invoke_ и Elvis operator для вызова события:

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

Если такой способ недоступен, следует записать ссылку на обработчики события в локальную переменную, работать уже с ней:

```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: [V3139](https://pvs-studio.ru/ru/docs/warnings/v3139/) Two or more case\-branches perform the same actions\. Eto\.Wpf\(net462\) SplitterHandler\.cs 357

Анализатор обнаружил фрагмент конструкции _switch_, где разные _case_\-ветви содержат одинаковый код\.

_switch_ покрывает 3 элемента перечисления _SplitterFixedPanel_, два из которых имеют название _Panel1_ и _Panel2_\. В обеих ветках вызывается метод _SetLength_, который имеет такую сигнатуру:

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

Значение аргумента _panel_ используется в качестве индекса внутри метода _SetLength_:

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

Ещё одна ветвь покрывает элемент _None_\. Предположу, он объединяет код для обеих панелей\. Вероятно, использование магических чисел "0" и "2" вполне корректно, так как здесь производится работа со стандартным контролом "SplitContainer"\. Число "1" соответствует не упомянутому тут разделителю\. Возможно, код должен иметь такой вид:

```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: [V3105](https://pvs-studio.ru/ru/docs/warnings/v3105/) The 'fontDesc' variable was used after it was assigned through null\-conditional operator\. NullReferenceException is possible\. Eto\.Gtk3 RichTextAreaHandler\.cs 328

Анализатор предупреждает об использовании без проверки переменной, которая может быть _null_, так как при присвоении ей значения применяется null\-conditional оператор\.

Переменной _fontDesc_ при объявлении присваивается _null_\. Если новое значение не было присвоено в цикле _foreach_, то существует ещё одна ветка, где производится присваивание значения для _fontDesc_\. Но код присвоения использует null\-conditional \(Elvis\) оператор:

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

Это означает, что если в первом элементе массива будет _null_, то _fontDesc_ будет присвоен _null_\. А дальше производится разыменование:

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

Если _fontDesc_ будет _null_, то попытка присвоить значение свойству _Size_ приведёт к выбросу исключения _NullReferenceException_\.

Впрочем, всё выглядит так, будто null\-conditional оператор остался от рефакторинга или был добавлен случайно\. Если в _family\.Faces\[0\]_ будет находиться _null_, то выброс _NullReferenceException_ произойдёт ещё в цикле _foreach_\. Там происходит разыменование:

```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: [V3095](https://pvs-studio.ru/ru/docs/warnings/v3095/) The 'Widget\.Binding' object was used before it was verified against null\. Check lines: 42, 43\. Eto\.Mac64 ProgressCellHandler\.cs 42

Анализатор указал, что разыменование ссылки производится раньше её проверки на _null_\.

Если _Widget\.Binding_ будет _null_, то при вызове метода _GetValue_ будет выброшено исключение _NullReferenceException_\. Находящаяся ниже проверка _Widget\.Binding \!\= null_ является бесполезной\. Следует упростить код, используя уже упомянутый в этой статье Elvis оператор, и изменить условие\. Более корректный код может быть таким:

```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**

Предоставляю вам возможность найти ошибку самостоятельно:

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

Где же она?

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

Ошибка находится здесь:

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

Предупреждение PVS\-Studio: [V3080](https://pvs-studio.ru/ru/docs/warnings/v3080/) Possible null dereference\. Consider inspecting 'Control'\. Eto\.Gtk3 RadioMenuItemHandler\.cs 143

Анализатор сообщает о возможном разыменовании нулевой ссылки\.

Проверка бессмысленна и не защищает от _NullReferenceException_\. В тернарном операторе если условие истинно, то вычисляется только первое выражение\. Если условие ложно, то вычисляется второе выражение\. Когда _Control_ будет _null_, тогда условие будет ложным и произойдёт разыменование нулевой ссылки – очевидный _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: [V3140](https://pvs-studio.ru/ru/docs/warnings/v3140/) Property accessors use different backing fields\. Eto\.Mac64 MacImageAndTextCell\.cs 162

Анализатор обнаружил, что в сеттере и геттере свойства используются разные поля\. В сеттере используется _textShadow_, а в геттере – _textHighlightShadow_\. Взглянув на название свойства – _TextHighlightShadow_, можно понять, что правильным полем является _textHighlightShadow_\. Вот его объявление:

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

Поле _textHighlightShadow_ инициализируется только внутри свойства _TextHighlightShadow_\. Таким образом, присваиваемое свойству значение не связано с возвращаемым значением, которое всегда будет одним и тем же объектом\. При первом получении значения свойства, когда _textHighlightShadow_ всегда является _null_, геттер создаёт этот объект и присваивает значение нескольким его свойствам, используя предопределённые значения\. При этом существует свойство _TextShadow_, которое работает с полем _textShadow_:

```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; }
}
```

Так как в сеттере _TextHighlightShadow_ используется поле _textShadow_, то при каждом изменении _TextHighlightShadow_ будет меняться и _TextShadow_\. Сомнительно, что разработчик решил реализовать именно такое поведение\.

**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: [V3008](https://pvs-studio.ru/ru/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

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

На одной строке производятся объявление и инициализация переменной _sz_\. И сразу на следующей строке значение _sz_ перезаписывается, что делает вычисление инициализирующего значения бессмысленным\.

**Issue 11**

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

Предупреждение PVS\-Studio: [V3146](https://pvs-studio.ru/ru/docs/warnings/v3146/) Possible null dereference of 'ofTypeMethod'\. The 'FirstOrDefault' can return default null value\. Eto BindingExtensionsNonGeneric\.cs 21

Анализатор указывает, что метод _FirstOrDefault_, который используется для инициализации переменной _ofTypeMethod_, может вернуть _null_\. Разыменование _ofTypeMethod_ без проверки может привести к выбросу исключения _NullReferenceException_\.

Если есть гарантия того, что элемент будет найден, следует использовать метод _First_:

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

Впрочем, если никаких гарантий нет и соответствующий предикату элемент может быть не найден, то _First_ выбросит _InvalidOperationException_\. Можно поспорить, что лучше: _NullReferenceException_ или _InvalidOperationException_? Может быть, коду требуется куда большая доработка\.

## Заключение

Когда эталонная реализация \.NET была крепко привязана к Windows, одним из достоинств той экосистемы была возможность быстро разрабатывать GUI\-приложения\. Со временем появились кроссплатформенные Mono, Xamarin и, в конце концов, \.NET Core\. Одним из первых желаний сообщества было портирование GUI\-фреймворков с Windows на новые платформы\. Появилось много хороших похожих фреймворков, использующих C\# и XAML для разработки: Avalonia UI, Uno Platform и Eto\.Forms\. И, если вы знаете о похожем неупомянутом проекте, напишите, пожалуйста, о нём в комментариях\. Даже как\-то странно желать этим хороших проектам конкуренции, но конкуренция – двигатель прогресса\. 

PVS\-Studio может помочь разработчикам этих проектов сделать код качественнее\. Тем более что использовать анализатор в некоммерческих Open Source проектах можно [бесплатно](https://pvs-studio.ru/ru/blog/posts/0600/)\.

Надеюсь, это статья смогла показать вам, насколько разные ошибки может находить статический анализ\. Предлагаю вам [попробовать PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/try-free/), чтобы проверить интересующие вас проекты\.

Большое спасибо за внимание, увидимся в следующих статьях\!