Мы любим искать ошибки в проектах Microsoft. Почему? Всё просто: их проекты, как правило, легко проверить (работу можно вести сразу в среде Visual Studio, для которой у PVS-Studio есть удобный плагин) и они содержат мало ошибок. Поэтому обычный алгоритм работы такой: найти и скачать открытый проект от MS; проверить его; выбрать интересные ошибки; убедиться, что их немного; написать статью, не забыв похвалить разработчиков. Великолепно! Win-win-win: времени ушло немного, руководство радо появлению новых материалов в блоге, да и карма в полном порядке. Но в этот раз что-то пошло не так. Давайте посмотрим, что интересного удалось найти в исходном коде Windows Forms и стоит ли хвалить Microsoft в этот раз.

Введение

В начале декабря 2018 года компания Microsoft объявила о выпуске .NET Core 3 Preview 1. Немногим ранее (примерно с середины октября) на GitHub началась активная работа по обнародованию исходников Windows Forms — платформы .NET Core UI для создания настольных приложений Windows. Статистику коммитов можно посмотреть тут. Сейчас любой желающий может скачать исходный код WinForms для ознакомления.

Я тоже скачал исходники, чтобы поискать там ошибки с помощью PVS-Studio. Проверка не вызвала затруднений. Потребовались: Visual Studio 2019, .NET Core 3.0 SDK Preview, PVS-Studio. И вот лог предупреждений анализатора получен.

Получив лог PVS-Studio, я обычно сортирую его в порядке возрастания номеров диагностик (окно с логом сообщений PVS-Studio в среде Visual Studio имеет различные варианты сортировки и фильтрации списка). Это позволяет работать с группами однотипных ошибок, что сильно упрощает анализ исходного кода. Интересные ошибки я отмечаю в списке «звездочкой», а уже потом, проанализировав весь лог, выписываю фрагменты кода и делаю описание. А так как ошибок обычно немного, я перемешиваю их, стараясь разместить самые интересные в начале и конце статьи. Но в этот раз ошибок получилось многовато (эх, долго интригу сохранить не удалось), и я буду приводить их по порядку номеров диагностик.

Что же нашлось? 833 предупреждения уровня достоверности High и Medium (249 и 584, соответственно) были выданы для 540 000 строк кода (без учета пустых) в 1670 файлах cs. И да, по традиции я не проверял тесты и не рассматривал предупреждения уровня достоверности Low (их было выдано 215). По моим предыдущим наблюдениям предупреждений многовато для проекта от MS. Но не все предупреждения являются ошибками.

Для данного проекта число ложных срабатываний составило около 30%. Ещё примерно в 20% случаев я просто не смог сделать точный вывод о том, ошибка это или нет, так как недостаточно хорошо знаком с кодом. Ну и не менее 20% пропущенных мною ошибок можно списать на человеческий фактор: спешка, усталость и т.п. Возможен, кстати, и обратный эффект: некоторые однотипные срабатывания, число которых могло доходить до 70-80, я просматривал через одно, что изредка могло увеличить цифру ошибок, которые я посчитал настоящими.

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

Итак, число ошибок, которые мне удалось обнаружить, составило около 240, что находится в пределах приведённой статистики. Повторюсь, на мой взгляд, для проекта от MS это не самый выдающийся результат (хотя это составит всего 0.44 ошибки на 1000 строк кода), да и реальных ошибок в коде WinForms, вероятно, больше. О причинах предлагаю поговорить в конце статьи, а сейчас давайте посмотрим самые интересные ошибки.

Ошибки

PVS-Studio: V3003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 213, 224. ButtonStandardAdapter.cs 213

void PaintWorker(PaintEventArgs e, bool up, CheckState state)
{
  up = up && state == CheckState.Unchecked;
  ....
  if (up & IsHighContrastHighlighted())
  {
    ....
  }
  else if (up & IsHighContrastHighlighted())
  {
    ....
  }
  else
  {
    ....
  }
  ....
}

В блоках if и else if проверяют одинаковое условие. Выглядит как copy-paste. Ошибка ли это? Если посмотреть на объявление метода IsHighContrastHighlighted, возникает сомнение:

protected bool IsHighContrastHighlighted()
{
  return SystemInformation.HighContrast && 
    Application.RenderWithVisualStyles &&
    (Control.Focused || Control.MouseIsOver || 
      (Control.IsDefault && Control.Enabled));
}

Метод, вероятно, может вернуть разные значения при последовательных вызовах. А то, что делается в вызывающем методе, выглядит, конечно, странно, но имеет право на жизнь. Тем не менее, я бы посоветовал авторам взглянуть на данный фрагмент кода. На всякий случай. А ещё, это хороший пример того, как сложно делать выводы, анализируя малознакомый код.

PVS-Studio: V3004 The 'then' statement is equivalent to the 'else' statement. RichTextBox.cs 1018

public int SelectionCharOffset
{
  get
  {
    int selCharOffset = 0;
    ....
    NativeMethods.CHARFORMATA cf = GetCharFormat(true);
    // if the effects member contains valid info
    if ((cf.dwMask & RichTextBoxConstants.CFM_OFFSET) != 0)
    {
      selCharOffset = cf.yOffset;  // <=
    }
    else
    {
      // The selection contains characters of different offsets,
      // so we just return the offset of the first character.
      selCharOffset = cf.yOffset;  // <=
    }
    ....
  }
  ....
}

А тут точно допущена ошибка copy-paste. Независимо от условия, переменная selCharOffset всегда получит одинаковое значение.

В коде WinForms нашлись ещё две подобные ошибки:

• V3004 The 'then' statement is equivalent to the 'else' statement. SplitContainer.cs 1700
• V3004 The 'then' statement is equivalent to the 'else' statement. ToolstripProfessionalRenderer.cs 371

PVS-Studio: V3008 The variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 681, 680. ProfessionalColorTable.cs 681

internal void InitSystemColors(ref Dictionary<KnownColors, Color> rgbTable)
{
  ....
  rgbTable[ProfessionalColorTable.KnownColors.msocbvcrCBBdrOuterDocked] = 
    buttonFace;
  rgbTable[ProfessionalColorTable.KnownColors.msocbvcrCBBdrOuterDocked] =
    buttonShadow;
  ....
}

В методе заполняют словарь rgbTable. Анализатор указал на фрагмент кода, где дважды последовательно пишут разные значения по одному и тому же ключу. И всё бы ничего, но таких мест в данном методе нашлось ещё 16. Это уже не похоже на единичную ошибку. Но вот зачем так делают, для меня осталось загадкой. Признаков автогенерированного кода я не нашел. В редакторе это выглядит так:

Приведу первые десять срабатываний списком:

1. V3008 The variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 785, 784. ProfessionalColorTable.cs 785
2. V3008 The variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 787, 786. ProfessionalColorTable.cs 787
3. V3008 The variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 789, 788. ProfessionalColorTable.cs 789
4. V3008 The variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 791, 790. ProfessionalColorTable.cs 791
5. V3008 The variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 797, 796. ProfessionalColorTable.cs 797
6. V3008 The variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 799, 798. ProfessionalColorTable.cs 799
7. V3008 The variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 807, 806. ProfessionalColorTable.cs 807
8. V3008 The variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 815, 814. ProfessionalColorTable.cs 815
9. V3008 The variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 817, 816. ProfessionalColorTable.cs 817
10. V3008 The variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 823, 822. ProfessionalColorTable.cs 823

PVS-Studio: V3011 Two opposite conditions were encountered. The second condition is always false. Check lines: 5242, 5240. DataGrid.cs 5242

private void CheckHierarchyState()
{
  if (checkHierarchy && listManager != null && myGridTable != null)
  {
    if (myGridTable == null)  // <=
    {
      // there was nothing to check
      return;
    }

    for (int j = 0; j < myGridTable.GridColumnStyles.Count; j++)
    {
      DataGridColumnStyle gridColumn = myGridTable.GridColumnStyles[j];
    }
    checkHierarchy = false;  
  }
}

Оператор return никогда не будет выполнен. Скорее всего, условие myGridTable != null во внешнем блоке if было добавлено позднее при рефакторинге. И теперь проверка myGridTable == null бессмысленна. Для улучшения качества кода следует удалить эту проверку.

PVS-Studio: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'left', 'cscLeft'. TypeCodeDomSerializer.cs 611

PVS-Studio: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'right', 'cscRight'. TypeCodeDomSerializer.cs 615

public int Compare(object left, object right)
{
  OrderedCodeStatementCollection cscLeft = 
    left as OrderedCodeStatementCollection;
  OrderedCodeStatementCollection cscRight = 
    right as OrderedCodeStatementCollection;
  if (left == null)
  {
    return 1;
  }
  else if (right == null)
  {
    return -1;
  }
  else if (right == left)
  {
    return 0;
  }
  return cscLeft.Order - cscRight.Order;  // <=
}

Анализатор выдал сразу два предупреждения для метода Compare. В чём же проблема? Она в том, что никак не проверяются значения cscLeft и cscRight на равенство null. Такое значение они могут получить после неудачного приведения к типу OrderedCodeStatementCollection. Тогда в последнем выражении return будет выдано исключение. Такая ситуация возможна, когда все проверки для left и right пройдут и не приведут к предварительному выходу из метода.

Чтобы исправить код, везде следует использовать cscLeft/cscRight вместо left/right.

PVS-Studio: V3020 An unconditional 'break' within a loop. SelectionService.cs 421

void ISelectionService.SetSelectedComponents(
  ICollection components, SelectionTypes selectionType)
{
  ....
  // Handle the click case
  object requestedPrimary = null;
  int primaryIndex;
  
  if (fPrimary && 1 == components.Count)
  {
    foreach (object o in components)
    {
      requestedPrimary = o;
      if (o == null)
      {
          throw new ArgumentNullException(nameof(components));
      }
      break;
    }
  }
  ....            
}

Данный фрагмент относится, скорее, к коду «с запахом». Ошибки тут нет. Но вопросы возникают к способу организации цикла foreach. Зачем он тут понадобился — понятно: из-за необходимости извлечения элементов коллекции, переданной как ICollection. Но зачем в цикле, изначально рассчитанном на однократную итерацию (предварительным условием является наличие в коллекции components единственного элемента), потребовалась дополнительная подстраховка в виде break? Наверное, ответом можно считать: «Так исторически сложилось». Код выглядит некрасиво.

PVS-Studio: V3022 Expression 'ocxState != null' is always true. AxHost.cs 2186

public State OcxState
{
  ....
  set
  {
    ....
    if (value == null)
    {
        return;
    }
    ....
    ocxState = value;
    
    if (ocxState != null)  // <=
    {
      axState[manualUpdate] = ocxState._GetManualUpdate();
      licenseKey = ocxState._GetLicenseKey();
    }
    else
    {
      axState[manualUpdate] = false;
      licenseKey = null;
    } 
    ....
  }
}

Из-за логической ошибки в данном фрагменте образовался «мертвый код». Выражения в блоке else никогда не будут выполнены.

PVS-Studio: V3027 The variable 'e' was utilized in the logical expression before it was verified against null in the same logical expression. ImageEditor.cs 99

public override object EditValue(....)
{
  ....
  ImageEditor e = ....;
  Type myClass = GetType();
  if (!myClass.Equals(e.GetType()) && e != null &&
      myClass.IsInstanceOfType(e))
  {
    ....
  }
  ....
}

Переменную e в условии сначала используют, а затем проверяют на неравенство null. Привет, NullReferenceException.

Еще одна подобная ошибка:

PVS-Studio: V3027 The variable 'dropDownItem' was utilized in the logical expression before it was verified against null in the same logical expression. ToolStripMenuItemDesigner.cs 1351

internal void EnterInSituEdit(ToolStripItem toolItem)
{
  ....
  ToolStripDropDownItem dropDownItem = toolItem as ToolStripDropDownItem;
  if (!(dropDownItem.Owner is ToolStripDropDownMenu) && 
      dropDownItem != null && 
      dropDownItem.Bounds.Width < commitedEditorNode.Bounds.Width)
  {
    ....
  }
  ....
}

Ситуация, аналогичная предыдущей, только с переменной dropDownItem. Я думаю, что такие ошибки появляются в результате невнимательности при рефакторинге. Вероятно, часть условия !(dropDownItem.Owner is ToolStripDropDownMenu) была добавлена в код позже.

PVS-Studio: V3030 Recurring check. The 'columnCount > 0' condition was already verified in line 3900. ListView.cs 3903

internal ColumnHeader InsertColumn(
  int index, ColumnHeader ch, bool refreshSubItems)
{
  ....
  // Add the column to our internal array
  int columnCount = (columnHeaders == null ? 0 : columnHeaders.Length);
  if (columnCount > 0)
  {
    ColumnHeader[] newHeaders = new ColumnHeader[columnCount + 1];
    if (columnCount > 0)
    {
        System.Array.Copy(columnHeaders, 0, newHeaders, 0, columnCount);
    }
    ....
  }
  ....
}

Ошибка, которая может показаться безобидной. Действительно, ну выполняется лишняя проверка, которая не влияет на логику работы. А иногда так даже делают, когда нужно повторно проверить состояние какого-либо визуального компонента, например, получая число записей в списке. Только вот в данном случае дважды проверяют локальную переменную columnCount. Это очень подозрительно. Либо хотели проверить другую переменную, либо в одной из проверок используют не то условие.

PVS-Studio: V3061 Parameter 'lprcClipRect' is always rewritten in method body before being used. WebBrowserSiteBase.cs 281

int UnsafeNativeMethods.IOleInPlaceSite.GetWindowContext(
  out UnsafeNativeMethods.IOleInPlaceFrame ppFrame, 
  out UnsafeNativeMethods.IOleInPlaceUIWindow ppDoc,
  NativeMethods.COMRECT lprcPosRect, 
  NativeMethods.COMRECT lprcClipRect,
  NativeMethods.tagOIFI lpFrameInfo)
{
  ppDoc = null;
  ppFrame = Host.GetParentContainer();
  
  lprcPosRect.left = Host.Bounds.X;
  lprcPosRect.top = Host.Bounds.Y;
  ....
  
  lprcClipRect = WebBrowserHelper.GetClipRect();  // <=
  if (lpFrameInfo != null)
  {
    lpFrameInfo.cb = Marshal.SizeOf<NativeMethods.tagOIFI>();
    lpFrameInfo.fMDIApp = false;
    ....
  }
  return NativeMethods.S_OK;
}

Неочевидная ошибка. Да, параметр lprcClipRect действительно инициализируют новым значением, никак его не используя. Но к чему это приводит в итоге? Я думаю, что где-то в вызывающем коде ссылка, передаваемая через этот параметр, останется без изменений, хотя задумывали не так. Действительно, посмотрите на работу с другими переменными в данном методе. Даже его название (префикс «Get») намекает, что внутри метода будет произведена какая-то инициализация через передаваемые параметры. И это так. Первые два параметра (ppFrame и ppDoc) передаются с модификатором out и получают новые значения. Ссылки lprcPosRect и lpFrameInfo используют для доступа к полям класса и их инициализации. И только lprcClipRect выбивается из общего списка. Вероятно, для этого параметра необходим модификатор out, либо ref.

PVS-Studio: V3066 Possible incorrect order of arguments passed to 'AdjustCellBorderStyle' method: 'isFirstDisplayedRow' and 'isFirstDisplayedColumn'. DataGridViewComboBoxCell.cs 1934

protected override void OnMouseMove(DataGridViewCellMouseEventArgs e)
{
  ....
  dgvabsEffective = AdjustCellBorderStyle(
    DataGridView.AdvancedCellBorderStyle,
    dgvabsPlaceholder,
    singleVerticalBorderAdded,
    singleHorizontalBorderAdded,
    isFirstDisplayedRow,      // <=
    isFirstDisplayedColumn);  // <=
  ....
}

Анализатор заподозрил, что два последних аргумента были перепутаны местами. Давайте взглянем на объявление метода AdjustCellBorderStyle:

public virtual DataGridViewAdvancedBorderStyle AdjustCellBorderStyle(
  DataGridViewAdvancedBorderStyledataGridViewAdvancedBorderStyleInput,
  DataGridViewAdvancedBorderStyle dataGridViewAdvancedBorderStylePlaceholder,
  bool singleVerticalBorderAdded,
  bool singleHorizontalBorderAdded,
  bool isFirstDisplayedColumn,
  bool isFirstDisplayedRow)
{
  ....
}

Выглядит как ошибка. Да, часто некоторые аргументы умышленно передают в обратном порядке, например, чтобы обменять местами какие-то переменные. Но я не думаю, что это именно тот случай. Ничего в вызывающем или вызываемом методах не говорит о таком паттерне использования. Во-первых, перепутаны переменные типа bool. Во-вторых, названия методов также обычные: никаких «Swap» или «Reverse». К тому же, так ошибиться не так уж и сложно. Люди часто по-разному воспринимают порядок следования пары «строка/столбец». Для меня, например, привычным является как раз «строка/столбец». А вот для автора вызываемого метода AdjustCellBorderStyle, очевидно, более привычный порядок — «столбец/строка».

PVS-Studio: V3070 Uninitialized variable 'LANG_USER_DEFAULT' is used when initializing the 'LOCALE_USER_DEFAULT' variable. NativeMethods.cs 890

internal static class NativeMethods
{
  ....
  public static readonly int LOCALE_USER_DEFAULT =
    MAKELCID(LANG_USER_DEFAULT);
  public static readonly int LANG_USER_DEFAULT = 
    MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT);
  ....
}

Редкая ошибка. Перепутан порядок инициализации полей класса. Для вычисления значения поля LOCALE_USER_DEFAULT используют поле LANG_USER_DEFAULT, которое в данный момент ещё не инициализировано и имеет значение 0. Кстати, переменная LANG_USER_DEFAULT далее в коде нигде не используется. Я не поленился и написал небольшую консольную программу, которая моделирует ситуацию. Вместо значений некоторых констант, которые используются в коде WinForms, я подставил их фактические значения:

internal static class NativeMethods
{
  public static readonly int LOCALE_USER_DEFAULT = 
    MAKELCID(LANG_USER_DEFAULT);
  public static readonly int LANG_USER_DEFAULT = 
    MAKELANGID(0x00, 0x01);
  
  public static int MAKELANGID(int primary, int sub)
  {
    return ((((ushort)(sub)) << 10) | (ushort)(primary));
  }
  public static int MAKELCID(int lgid)
  {
    return MAKELCID(lgid, 0x0);
  }
  public static int MAKELCID(int lgid, int sort)
  {
    return ((0xFFFF & lgid) | (((0x000f) & sort) << 16));
  }
}
class Program
{
  static void Main()
  {
    System.Console.WriteLine(NativeMethods.LOCALE_USER_DEFAULT);
  }
}

В результате запуска на консоль будет выведено: 0. Теперь поменяем местами объявление полей LOCALE_USER_DEFAULT и LANG_USER_DEFAULT. Результат выполнения программы в таком виде: 1024. Думаю, более здесь нечего комментировать.

PVS-Studio: V3080 Possible null dereference. Consider inspecting 'ces'. CodeDomSerializerBase.cs 562

protected void DeserializeStatement(
  IDesignerSerializationManager manager, CodeStatement statement)
{
  ....
  CodeExpressionStatement ces = statement as CodeExpressionStatement;
  if (ces != null)
  {
    ....
  }
  else
  {
    ....
    DeserializeExpression(manager, null, ces.Expression);  // <=
    ....
  }
  ....
}

Код, который должен «падать» достаточно стабильно, ведь в ветку else можно попасть как раз при равенстве null ссылки ces.

Ещё один подобный пример:

PVS-Studio: V3080 Possible null dereference. Consider inspecting 'comboBox'. ComboBox.cs 6610

public void ValidateOwnerDrawRegions(ComboBox comboBox, ....)
{
  ....
  if (comboBox != null)
  { return; }
  Rectangle topOwnerDrawArea = 
    new Rectangle(0, 0, comboBox.Width, innerBorder.Top);
  ....
}

Парадоксальный код. По всей видимости, перепутали проверку, написав if (comboBox != null) вместо if (comboBox == null). А так — получим очередной NullReferenceException.

Мы рассмотрели две достаточно очевидные ошибки V3080, где визуально можно отследить ситуацию возможного использования нулевой ссылки в пределах метода. Но диагностика V3080 гораздо интеллектуальнее и может выискивать подобные ошибки для цепочек вызовов методов. Не так давно мы значительно усилили механизмы dataflow и межпроцедурного анализа. Про это можно прочитать в статье "Nullable Reference типы в C# 8.0 и статический анализ". А вот подобная ошибка, обнаруженная в WinForms:

PVS-Studio: V3080 Possible null dereference inside method at 'reader.NameTable'. Consider inspecting the 1st argument: contentReader. ResXResourceReader.cs 267

private void EnsureResData()
{
  ....
  XmlTextReader contentReader = null;
  
  try
  {
    if (fileContents != null)
    {
      contentReader = new XmlTextReader(....);
    }
    else if (reader != null)
    {
      contentReader = new XmlTextReader(....);
    }
    else if (fileName != null || stream != null)
    {
      ....  
      contentReader = new XmlTextReader(....);
    }
    
    SetupNameTable(contentReader);  // <=
    ....
  }
  finally
  {
    ....
  }
  ....
}

Посмотрите на то, что происходит с переменной contentReader в теле метода. После инициализации нулевой ссылкой, в результате одной из проверок, ссылка будет инициализирована. Однако серия проверок не завершается блоком else. Это значит, что в каком-то редком случае (или вследствие рефакторинга в будущем) ссылка все же может остаться нулевой. Далее она будет передана в метод SetupNameTable, где использована без всякой проверки:

private void SetupNameTable(XmlReader reader)
{
  reader.NameTable.Add(ResXResourceWriter.TypeStr);
  reader.NameTable.Add(ResXResourceWriter.NameStr);
  ....
}

Это потенциально небезопасный код.

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

PVS-Studio: V3080 Possible null dereference. Consider inspecting 'layout'. DockAndAnchorLayout.cs 156

private static Rectangle GetAnchorDestination(
  IArrangedElement element, Rectangle displayRect, bool measureOnly)
{
  ....
  AnchorInfo layout = GetAnchorInfo(element);

  int left = layout.Left + displayRect.X;
  ....
}

Анализатор утверждает, что из метода GetAnchorInfo возможно получение нулевой ссылки, что приведет к выбросу исключения в момент вычисления значения left. Давайте пройдем по всей цепочке вызовов и проверим, так ли это:

private static AnchorInfo GetAnchorInfo(IArrangedElement element)
{
  return (AnchorInfo)element.Properties.GetObject(s_layoutInfoProperty);
}

public object GetObject(int key) => GetObject(key, out _);

public object GetObject(int key, out bool found)
{
  short keyIndex = SplitKey(key, out short element);
  if (!LocateObjectEntry(keyIndex, out int index))
  {
    found = false;
    return null;
  }
  
  // We have found the relevant entry. See if
  // the bitmask indicates the value is used.
  if (((1 << element) & s_objEntries[index].Mask) == 0)
  {
    found = false;
    return null;
  }
  
  found = true;
  switch (element)
  {
    case 0:
      return s_objEntries[index].Value1;
    ....
    default:
      Debug.Fail("Invalid element obtained from LocateObjectEntry");
      return null;
  }
}

Действительно, в ряде случаев метод GetObject, замыкающий цепочку вызовов, вернет null, который без всяких дополнительных проверок будет передан в вызывающий метод. Вероятно, в методе GetAnchorDestination необходимо предусмотреть такую ситуацию.

В коде WinForms нашлось довольно много подобных ошибок, более 70. Они все похожи и я не буду приводить их описание в статье.

PVS-Studio: V3091 Empirical analysis. It is possible that a typo is present inside the string literal: «ShowCheckMargin». The 'ShowCheckMargin' word is suspicious. PropertyNames.cs 136

internal class PropertyNames
{
  ....
  public static readonly string ShowImageMargin = "ShowCheckMargin";
  ...
  public static readonly string ShowCheckMargin = "ShowCheckMargin";
  ....
}

Хороший пример ошибки, которую не так-то просто обнаружить. При инициализации полей класса используют одинаковое значение, хотя автор кода явно задумывал не так (виноват copy-paste). Анализатор сделал такой вывод, сопоставив имена переменных и значения присваиваемых строк. Я привел только строки с ошибками, но посмотрите как это выглядит в редакторе кода:

Именно обнаружение таких ошибок демонстрирует всю мощь и бесконечную внимательность инструментов статического анализа.

PVS-Studio: V3095 The 'currentForm' object was used before it was verified against null. Check lines: 3386, 3404. Application.cs 3386

private void RunMessageLoopInner(int reason, ApplicationContext context)
{
  ....
  hwndOwner = new HandleRef(
    null, 
    UnsafeNativeMethods.GetWindowLong(
      new HandleRef(currentForm, currentForm.Handle),  // <=
    NativeMethods.GWL_HWNDPARENT));
  ....
  if (currentForm != null && ....)
  ....
}

Классика. Переменную currentForm используют без всяких проверок. Но далее в коде есть её проверка на равенство null. В данном случае могу посоветовать быть более внимательным при работе со ссылочными типами, а также использовать статические анализаторы :).

Ещё одна подобная ошибка:

PVS-Studio: V3095 The 'backgroundBrush' object was used before it was verified against null. Check lines: 2331, 2334. DataGrid.cs 2331

public Color BackgroundColor
{
  ....
  set
  {
    ....
    if (!value.Equals(backgroundBrush.Color))  // <=
    {
      if (backgroundBrush != null && 
          BackgroundBrush != DefaultBackgroundBrush)
      ....
    }
  }
}

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

PVS-Studio: V3125 The '_propInfo' object was used and was verified against null in different execution branches. Check lines: 996, 982. Binding.cs 996

private void SetPropValue(object value)
{
  ....
  if (....)
  {
    if ....
    else if (_propInfo != null) ....
  }
  else
  {
    _propInfo.SetValue(_control, value);
  }
  ....
}

Для полноты картины — тоже своего рода классика, ошибка V3125. Обратная ситуация. Сначала потенциально нулевую ссылку используют безопасно, проверив на равенство null, а вот далее в коде этого уже не делают.

И ещё одна подобная ошибка:

PVS-Studio: V3125 The 'owner' object was used after it was verified against null. Check lines: 64, 60. FlatButtonAppearance.cs 64

public int BorderSize
{
  ....
  set
  {
    ....
    if (owner != null && owner.ParentInternal != null)
    {
        LayoutTransaction.DoLayoutIf(....);
    }
    owner.Invalidate();  // <=
    ....
  }
}

Красота. Но это с точки зрения стороннего исследователя. Ведь помимо этих двух V3125, анализатор нашел еще более 50 подобных паттернов в коде WinForms. Разработчикам есть над чем поработать.

И напоследок — довольно интересная, на мой взгляд, ошибка.

PVS-Studio: V3137 The 'hCurrentFont' variable is assigned but is not used by the end of the function. DeviceContext2.cs 241

sealed partial class DeviceContext : ....
{
  WindowsFont selectedFont;
  ....
  internal void DisposeFont(bool disposing)
  {
    if (disposing)
    {
        DeviceContexts.RemoveDeviceContext(this);
    }
    
    if (selectedFont != null && selectedFont.Hfont != IntPtr.Zero)
    {
      IntPtr hCurrentFont = IntUnsafeNativeMethods.GetCurrentObject(
        new HandleRef(this, hDC), IntNativeMethods.OBJ_FONT);
      if (hCurrentFont == selectedFont.Hfont)
      {
        // select initial font back in
        IntUnsafeNativeMethods.SelectObject(new HandleRef(this, Hdc),
          new HandleRef(null, hInitialFont));

        hCurrentFont = hInitialFont;  // <=
      }
      
      selectedFont.Dispose(disposing);
      selectedFont = null;
    }
  }
  ....
}

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

В файле DeviceContext2.cs объявлен partial-класс. Метод DisposeFont используется для освобождения ресурсов после работы с графикой: контекст устройства и шрифты. Для лучшего понимания я привел метод DisposeFont целиком. Обратите внимание на локальную переменную hCurrentFont. Проблема в том, что объявление это переменной в методе скрывает одноименное поле класса. Я нашел два метода класса DeviceContext, где используется поле с именем hCurrentFont:

public IntPtr SelectFont(WindowsFont font)
{
  ....
  hCurrentFont = font.Hfont;
  ....
}
public void ResetFont()
{
  ....
  hCurrentFont = hInitialFont;
}

Посмотрите на метод ResetFont. Последняя строчка там — это именно то, что делают в методе DisposeFont во вложенном блоке if (на это место указывает анализатор). А объявлено это одноименное поле hCurrentFont в другой части partial-класса в файле DeviceContext.cs:

sealed partial class DeviceContext : ....
{
  ....
  IntPtr hInitialFont;
  ....
  IntPtr hCurrentFont;  // <=
  ....
}

Таким образом, допущена очевидная ошибка. Другой вопрос в её критичности. Сейчас в результате работы метода DisposeFont в секции, которая отмечена комментарием «select initial font back in», не будет выполнено присвоение полю hCurrentFont некоторого первоначального значения. Думаю, точный вердикт могут дать только авторы кода.

Выводы

Итак, в этот раз я вынужден немного «поругать» MS. В WinForms оказалось многовато ошибок, которые требуют пристального внимания разработчиков. Возможно, виной тому некоторая спешка, с которой MS работают над .NET Core 3 и компонентами, включая WinForms. На мой взгляд, код WinForms ещё «сыроват», но я надеюсь, что ситуация скоро изменится к лучшему.

Второй причиной большого числа ошибок может быть то, что наш анализатор просто стал лучше их искать :).

Кстати, скоро выйдет статья моего коллеги Сергея Васильева, в которой он ищет и находит довольно много проблем в коде библиотек .NET Core. Надеюсь, его работа также поспособствует улучшению характеристик платформы .NET, ведь мы всегда стараемся доводить до разработчиков результаты анализа их проектов.

Ну а тем, кто хочет улучшать свои продукты самостоятельно или проводить исследования по поиску ошибок в чужих проектах, я предлагаю скачать и попробовать PVS-Studio.

Всем чистого кода!

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Sergey Khrenov. WinForms: Errors, Holmes