WinForms: ошибки, Холмс

Лида Шиндлер07.08.2019

0 2 Время чтения: 15 мин.

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

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

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

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

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

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

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

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

Ошибки

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

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

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

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

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. Это уже не похоже на единичную ошибку. Но вот зачем так делают, для меня осталось загадкой. Признаков автогенерированного кода я не нашел. В редакторе это выглядит так:

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

V3008 The variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 785, 784. ProfessionalColorTable.cs 785
V3008 The variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 787, 786. ProfessionalColorTable.cs 787
V3008 The variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 789, 788. ProfessionalColorTable.cs 789
V3008 The variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 791, 790. ProfessionalColorTable.cs 791
V3008 The variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 797, 796. ProfessionalColorTable.cs 797
V3008 The variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 799, 798. ProfessionalColorTable.cs 799
V3008 The variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 807, 806. ProfessionalColorTable.cs 807
V3008 The variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 815, 814. ProfessionalColorTable.cs 815
V3008 The variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 817, 816. ProfessionalColorTable.cs 817
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 бессмысленна. Для улучшения качества кода следует удалить эту проверку.

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

Check variables 'right', 'cscRight'. PVS-Studio: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. 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.

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

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? Наверное, ответом можно считать: «Так исторически сложилось». Код выглядит некрасиво.

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

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 никогда не будут выполнены.

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

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

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

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

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

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) была добавлена в код позже.

The 'columnCount > 0' condition was already verified in line 3900. PVS-Studio: V3030 Recurring check. 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. Это очень подозрительно. Либо хотели проверить другую переменную, либо в одной из проверок используют не то условие.

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

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.

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

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, очевидно, более привычный порядок — «столбец/строка».

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

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. Думаю, более здесь нечего комментировать.

Consider inspecting 'ces'. PVS-Studio: V3080 Possible null dereference. 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.

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

Consider inspecting 'comboBox'. PVS-Studio: V3080 Possible null dereference. 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, где визуально можно отследить ситуацию возможного использования нулевой ссылки в пределах метода. Про это можно прочитать в статье "Nullable Reference типы в C# 8. Не так давно мы значительно усилили механизмы dataflow и межпроцедурного анализа. А вот подобная ошибка, обнаруженная в WinForms: 0 и статический анализ".

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

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); .... }

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

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

Consider inspecting 'layout'. PVS-Studio: V3080 Possible null dereference. 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.

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

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

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

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

Check lines: 3386, 3404. PVS-Studio: V3095 The 'currentForm' object was used before it was verified against null. 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. В данном случае могу посоветовать быть более внимательным при работе со ссылочными типами, а также использовать статические анализаторы :).

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Выводы

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

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

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

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

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