# WinForms: Errors, Holmes We like to search for errors in Microsoft projects\. Why? It's simple: their projects are usually easy to check \(you can work in Visual Studio environment for which PVS\-Studio has a convenient plugin\) and they contain few errors\. That's why the usual work algorithm is as follows: find and download an open source project from MS; check it; choose interesting errors; make sure there are few of them; write an article without forgetting to praise the developers\. Great\! Win\-win\-win: it took a little time, the bosses are glad to see new materials in the blog, and karma is fine\. But this time "something went wrong"\. Let's see what we have found in the source code of Windows Forms and whether we should speak highly of Microsoft this time\. ![0653_WinForms/image1.png](https://import.viva64.com/docx/blog/0653_WinForms/image1.png) **Introduction** In early December 2018, Microsoft [announced](https://devblogs.microsoft.com/dotnet/announcing-net-core-3-preview-1-and-open-sourcing-windows-desktop-frameworks/) the release of the \.NET Core 3 Preview 1\. A little earlier \(about mid\-October\), GitHub started to actively disclose the sources of [Windows Forms](https://github.com/dotnet/winforms) \- the \.NET Core UI platform for creating Windows desktop applications\. You can see the commit statistics [here](https://github.com/dotnet/winforms/graphs/commit-activity)\. Now anyone can download the WinForms source code for review\. I also downloaded the sources to search for errors there with PVS\-Studio\. The check did not cause any difficulties\. We needed: Visual Studio 2019, \.NET Core 3\.0 SDK Preview, PVS\-Studio\. And here we have the log of the analyzer's warnings\. Having received the PVS\-Studio report, I usually sort it by diagnostic numbers in the ascending order \(the window with the PVS\-Studio message log in Visual Studio environment has various options of sorting and filtering the list\)\. It allows you to work with groups of similar errors, which greatly simplifies source code analysis\. I mark interesting errors in the list with a "star" and only then, after analyzing the whole log, I write out code fragments and describe them\. Since there are usually few errors, I "stir" them trying to place the most interesting ones at the beginning and end of the article\. But this time it turned out to be a lot of errors \(eh, the intrigue has not been saved for a long time\) and I will cite them in the order of numbers of diagnostics\. What did we find? 833 High and Medium warnings \(249 and 584, respectively\) were issued for 540,000 lines of code \(not including empty ones\) in 1670 cs files\. And yes, traditionally I didn't check the tests and didn't consider the Low warnings \(there were 215 of them\)\. According to my previous observations, the warnings are too many for the MS project\. But not all the warnings are errors\. For this project the number of false alarms was about 30%\. In about 20% of cases, I just could not make an exact conclusion whether it was an error or not because I was not familiar with the code well enough\. And at least 20% of the errors I missed can be written off as "human factor": haste, tiredness, etc\. By the way, the opposite effect is also possible: some same\-type triggers, the number of which could reach 70\-80, I looked "next but one", which sometimes could increase the number of errors that I thought were real\. Anyway, 30% of the warnings indicate real errors, which is quite a large percentage if you take into account that the analyzer was not pre\-configured\. So, the number of errors I managed to find was about 240, which is within the range of the given statistics\. Again, in my opinion, this is not the most outstanding result for a MS project \(although it will make only 0\.44 errors per 1000 code lines\) and there are probably more real errors in WinForms code as well\. I suggest considering the reasons at the end of the article and now let's see the most interesting errors\. **Errors** PVS\-Studio: [V3003](https://pvs-studio.com/en/docs/warnings/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 ```cpp void PaintWorker(PaintEventArgs e, bool up, CheckState state) { up = up && state == CheckState.Unchecked; .... if (up & IsHighContrastHighlighted()) { .... } else if (up & IsHighContrastHighlighted()) { .... } else { .... } .... } ``` _If_ and _else if_ blocks check the same condition\. It looks like copy\-paste\. Is it an error? If you look at the declaration of the _IsHighContrastHighlighted_ method, you may doubt it: ```cpp protected bool IsHighContrastHighlighted() { return SystemInformation.HighContrast && Application.RenderWithVisualStyles && (Control.Focused || Control.MouseIsOver || (Control.IsDefault && Control.Enabled)); } ``` The method can probably return different values for sequential calls\. And what is happening in the caller method, of course, looks strange, but has the right to exist\. However, I would advise the authors to take a look at this code fragment\. Just in case\. It is also a good example of how difficult it is to draw conclusions when analyzing unfamiliar code\. PVS\-Studio: [V3004](https://pvs-studio.com/en/docs/warnings/v3004/) The 'then' statement is equivalent to the 'else' statement\. RichTextBox\.cs 1018 ```cpp 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; // <= } .... } .... } ``` And there is definitely a copy\-paste error here\. Regardless of the condition, the _selCharOffset_ variable will always get the same value\. There are two more such errors in WinForms code: * 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](https://pvs-studio.com/en/docs/warnings/v3008/) The variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 681, 680\. ProfessionalColorTable\.cs 681 ```cpp internal void InitSystemColors(ref Dictionary rgbTable) { .... rgbTable[ProfessionalColorTable.KnownColors.msocbvcrCBBdrOuterDocked] = buttonFace; rgbTable[ProfessionalColorTable.KnownColors.msocbvcrCBBdrOuterDocked] = buttonShadow; .... } ``` The method fills the _rgbTable_ dictionary\. The analyzer pointed to a code fragment where different values are written twice on the same key in sequence\. Things would be fine but there are still 16 such fragments in this method\. It does not look like a one\-of\-a\-kind error anymore\. But why they do this remains a mystery to me\. I didn't find any signs of autogenerated code\. It looks like this in the editor: ![0653_WinForms/image3.png](https://import.viva64.com/docx/blog/0653_WinForms/image3.png) I'll give you the first ten warnings on the list: 1. V3008 The variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 785, 784\. ProfessionalColorTable\.cs 785 1. V3008 The variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 787, 786\. ProfessionalColorTable\.cs 787 1. V3008 The variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 789, 788\. ProfessionalColorTable\.cs 789 1. V3008 The variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 791, 790\. ProfessionalColorTable\.cs 791 1. V3008 The variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 797, 796\. ProfessionalColorTable\.cs 797 1. V3008 The variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 799, 798\. ProfessionalColorTable\.cs 799 1. V3008 The variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 807, 806\. ProfessionalColorTable\.cs 807 1. V3008 The variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 815, 814\. ProfessionalColorTable\.cs 815 1. V3008 The variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 817, 816\. ProfessionalColorTable\.cs 817 1. V3008 The variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 823, 822\. ProfessionalColorTable\.cs 823 PVS\-Studio: [V3011](https://pvs-studio.com/en/docs/warnings/v3011/) Two opposite conditions were encountered\. The second condition is always false\. Check lines: 5242, 5240\. DataGrid\.cs 5242 ```cpp 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; } } ``` The _return_ operator will never be executed\. Most likely, the _myGridTable \!\= null_ condition in the external _if_ block was added later during refactoring\. And now the check of _myGridTable \=\= null_ is meaningless\. To improve the code quality, you should remove this check\. PVS\-Studio: [V3019](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/docs/warnings/v3019/) Possibly an incorrect variable is compared to null after type conversion using 'as' keyword\. Check variables 'right', 'cscRight'\. TypeCodeDomSerializer\.cs 615 ```cpp 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; // <= } ``` The analyzer generated two warnings for the _Compare_ method at once\. What is the problem? It is that _cscLeft_ and _cscRight_ values are not checked for _null_ at all\. They may get this value after unsuccessful casting to the _OrderedCodeStatementCollection_ type\. Then an exception will be thrown in the last _return_ expression\. This situation is possible when all the checks for _left_ and _right_ pass and do not lead to a preliminary exit from the method\. To fix the code, you should use _cscLeft/cscRight_ instead of _left/right_ everywhere\. PVS\-Studio: [V3020](https://pvs-studio.com/en/docs/warnings/v3020/) An unconditional 'break' within a loop\. SelectionService\.cs 421 ```cpp 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; } } .... } ``` This fragment refers rather to the "code smell"\. There is no error here\. But questions arise about the way the _foreach_ loop is organized\. It is clear why it is needed here: because of the need to extract elements of the collection, passed as _ICollection_\. But why did the loop, initially designed for single iteration \(the precondition is the presence of a single element in the collection _components_\), require additional support such as _break_? Probably, the answer can be considered as follows: "Historically, this has come to be"\. The code looks ugly\. PVS\-Studio: [V3022](https://pvs-studio.com/en/docs/warnings/v3022/) Expression 'ocxState \!\= null' is always true\. AxHost\.cs 2186 ```cpp 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; } .... } } ``` Because of a logical error, "dead code" occurred in this fragment\. Expressions in the _else_ block will never be executed\. PVS\-Studio: [V3027](https://pvs-studio.com/en/docs/warnings/v3027/) The variable 'e' was utilized in the logical expression before it was verified against null in the same logical expression\. ImageEditor\.cs 99 ```cpp public override object EditValue(....) { .... ImageEditor e = ....; Type myClass = GetType(); if (!myClass.Equals(e.GetType()) && e != null && myClass.IsInstanceOfType(e)) { .... } .... } ``` Variable _e_ in the condition is first used and then checked against _null_\. Hello, _NullReferenceException_\. One more such error: PVS\-Studio: [V3027](https://pvs-studio.com/en/docs/warnings/v3027/) The variable 'dropDownItem' was utilized in the logical expression before it was verified against null in the same logical expression\. ToolStripMenuItemDesigner\.cs 1351 ```cpp internal void EnterInSituEdit(ToolStripItem toolItem) { .... ToolStripDropDownItem dropDownItem = toolItem as ToolStripDropDownItem; if (!(dropDownItem.Owner is ToolStripDropDownMenu) && dropDownItem != null && dropDownItem.Bounds.Width < commitedEditorNode.Bounds.Width) { .... } .... } ``` The situation is similar to the previous one but with the _dropDownItem_ variable\. I think that such errors appear as a result of careless refactoring\. Probably, a part of the condition _\!\(dropDownItem\.Owner is ToolStripDropDownMenu\)_ was added into the code later\. PVS\-Studio: [V3030](https://pvs-studio.com/en/docs/warnings/v3030/) Recurring check\. The 'columnCount \> 0' condition was already verified in line 3900\. ListView\.cs 3903 ```cpp 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); } .... } .... } ``` A mistake that may seem harmless\. Indeed, an unnecessary check is performed which does not affect the operational logic\. And sometimes it is even done when you need to check the state of some visual component again, for example, getting the number of entries in the list\. But in this case the _local_ variable _columnCount_ is checked twice\. It is very suspicious\. Either they wanted to check another variable or they used a wrong condition in one of the checks\. PVS\-Studio: [V3061](https://pvs-studio.com/en/docs/warnings/v3061/) Parameter 'lprcClipRect' is always rewritten in method body before being used\. WebBrowserSiteBase\.cs 281 ```cpp 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(); lpFrameInfo.fMDIApp = false; .... } return NativeMethods.S_OK; } ``` An unevident mistake\. Yes, the _lprcClipRect_ parameter is actually initialized with a new value without using it in any way\. But what does it lead to in the end? I think that somewhere in the calling code the reference passed through this parameter will remain unchanged, although it was not intended to be so\. Really, appreciate the handling of other variables in this method\. Even its name \("Get" prefix\) hints that some initialization will be performed inside the method through passed parameters\. And it is so\. The first two parameters \(_ppFrame_ and _ppDoc_\) are passed with the _out_ modifier and they get new values\. References _lprcPosRect_ and _lpFrameInfo_ are used to access and initialize class fields\. Only _lprcClipRect_ stands out\. Probably, the _out_ or _ref_ modifier is required for this parameter\. PVS\-Studio: [V3066](https://pvs-studio.com/en/docs/warnings/v3066/) Possible incorrect order of arguments passed to 'AdjustCellBorderStyle' method: 'isFirstDisplayedRow' and 'isFirstDisplayedColumn'\. DataGridViewComboBoxCell\.cs 1934 ```cpp protected override void OnMouseMove(DataGridViewCellMouseEventArgs e) { .... dgvabsEffective = AdjustCellBorderStyle( DataGridView.AdvancedCellBorderStyle, dgvabsPlaceholder, singleVerticalBorderAdded, singleHorizontalBorderAdded, isFirstDisplayedRow, // <= isFirstDisplayedColumn); // <= .... } ``` The analyzer suspected that the last two arguments were mixed up\. Let's take a look at the declaration of the _AdjustCellBorderStyle_ method: ```cpp public virtual DataGridViewAdvancedBorderStyle AdjustCellBorderStyle( DataGridViewAdvancedBorderStyledataGridViewAdvancedBorderStyleInput, DataGridViewAdvancedBorderStyle dataGridViewAdvancedBorderStylePlaceholder, bool singleVerticalBorderAdded, bool singleHorizontalBorderAdded, bool isFirstDisplayedColumn, bool isFirstDisplayedRow) { .... } ``` Looks like a mistake\. Yes, some arguments are often passed in reverse order, for example, to exchange some variables\. But I don't think this is the case\. Nothing in the caller or callee methods indicates this usage pattern\. First, variables of the _bool_ type are mixed up\. Second, the names of the methods are also regular: no "Swap" or "Reverse"\. Besides, it is not so difficult to make a mistake like that\. People often perceive the order of the "row/column" pair differently\. For me, for example, it is the "row/column" that is familiar\. But for the author of the method called _AdjustCellBorderStyle_, obviously, the more usual order is "column/row"\. PVS\-Studio: [V3070](https://pvs-studio.com/en/docs/warnings/v3070/) Uninitialized variable 'LANG\_USER\_DEFAULT' is used when initializing the 'LOCALE\_USER\_DEFAULT' variable\. NativeMethods\.cs 890 ```cpp 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); .... } ``` Rare mistake\. The initialization order of class fields is mixed up\. To calculate the value of the field _LOCALE\_USER\_DEFAULT_ the _LANG\_USER\_DEFAULT_ field is used, which is not yet initialized and has a value of 0\. By the way, the _LANG\_USER\_DEFAULT_ variable is not used anywhere else in the code\. I went an extra mile and wrote a small console program that simulates the situation\. I substituted some constants used in WinForms code with their actual values: ```cpp 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); } } ``` As a result, the console will display: 0\. Now let's swap the declarations of the _LOCALE\_USER\_DEFAULT_ and _LANG\_USER\_DEFAULT_ fields\. The result of the program execution is as follows: 1024\. I think there is nothing more to comment on here\. PVS\-Studio: [V3080](https://pvs-studio.com/en/docs/warnings/v3080/) Possible null dereference\. Consider inspecting 'ces'\. CodeDomSerializerBase\.cs 562 ```cpp protected void DeserializeStatement( IDesignerSerializationManager manager, CodeStatement statement) { .... CodeExpressionStatement ces = statement as CodeExpressionStatement; if (ces != null) { .... } else { .... DeserializeExpression(manager, null, ces.Expression); // <= .... } .... } ``` The code that should "crash" rather regularly, because you can get into the _else_ branch just when the _ces_ reference equals _null_\. Another similar example: PVS\-Studio: [V3080](https://pvs-studio.com/en/docs/warnings/v3080/) Possible null dereference\. Consider inspecting 'comboBox'\. ComboBox\.cs 6610 ```cpp public void ValidateOwnerDrawRegions(ComboBox comboBox, ....) { .... if (comboBox != null) { return; } Rectangle topOwnerDrawArea = new Rectangle(0, 0, comboBox.Width, innerBorder.Top); .... } ``` The paradoxical code\. Apparently, the _if \(comboBox \!\= null\) _check was confused _ _with_if \(comboBox \=\= null\)_\. And so, we will get another _NullReferenceException\._ We have considered two rather obvious [V3080](https://pvs-studio.com/en/docs/warnings/v3080/) errors where you can visually trace a potential null reference usage within a method\. But the [V3080](https://pvs-studio.com/en/docs/warnings/v3080/) diagnostic is much more efficient and can find such errors for method call chains\. Not so long ago we have significantly improved the dataflow and interprocedural analysis mechanisms\. You may read about this in the article "[Nullable Reference types in C\# 8\.0 and static analysis](https://pvs-studio.com/en/blog/posts/csharp/0631/)"\. But here is such kind of error detected in WinForms: PVS\-Studio: [V3080](https://pvs-studio.com/en/docs/warnings/v3080/) Possible null dereference inside method at 'reader\.NameTable'\. Consider inspecting the 1st argument: contentReader\. ResXResourceReader\.cs 267 ```cpp 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 { .... } .... } ``` Look what happens to the _contentReader_ variable in the method body\. After initialization with null, it will be initialized again in one of the checks\. But the series of checks does not end with the _else_ block\. It means that in some rare case \(or due to refactoring in the future\) the reference **might** still remain null\. Then it will be passed to the _SetupNameTable_ method where it is used without any check: ```cpp private void SetupNameTable(XmlReader reader) { reader.NameTable.Add(ResXResourceWriter.TypeStr); reader.NameTable.Add(ResXResourceWriter.NameStr); .... } ``` This is potentially unsafe code\. And one more error where the analyzer had to go through the call chain to detect the problem: PVS\-Studio: [V3080](https://pvs-studio.com/en/docs/warnings/v3080/) Possible null dereference\. Consider inspecting 'layout'\. DockAndAnchorLayout\.cs 156 ```cpp private static Rectangle GetAnchorDestination( IArrangedElement element, Rectangle displayRect, bool measureOnly) { .... AnchorInfo layout = GetAnchorInfo(element); int left = layout.Left + displayRect.X; .... } ``` The analyzer claims that it is possible to get a null reference from the _GetAnchorInfo_ method, which will cause an exception when calculating the _left_ value\. Let's go through the whole call chain and check if it is true: ```cpp 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; } } ``` Indeed, in some cases, the _GetObject_ method that ends the call chain will return _null_, which will be passed to the caller method without any additional checks\. Probably, it is necessary to cover such a situation in the _GetAnchorDestination_ method\. There are quite a lot of such errors in WinForms code, **more than 70**\. They all look alike and I will not describe them in the article\. PVS\-Studio: [V3091](https://pvs-studio.com/en/docs/warnings/v3091/) Empirical analysis\. It is possible that a typo is present inside the string literal: "ShowCheckMargin"\. The 'ShowCheckMargin' word is suspicious\. PropertyNames\.cs 136 ```cpp internal class PropertyNames { .... public static readonly string ShowImageMargin = "ShowCheckMargin"; ... public static readonly string ShowCheckMargin = "ShowCheckMargin"; .... } ``` A good example of an error that is not so easy to find\. When initializing the class fields the same value is used although the author of the code obviously did not intend so \(copy\-paste is to blame\)\. The analyzer made this conclusion by comparing the names of variables and values of assigned strings\. I have given only lines with errors but you should check it out how it looks in the code editor: ![0653_WinForms/image5.png](https://import.viva64.com/docx/blog/0653_WinForms/image5.png) Detection of such errors is what demonstrates all the power and endless attention span of static analysis tools\. PVS\-Studio: [V3095](https://pvs-studio.com/en/docs/warnings/v3095/) The 'currentForm' object was used before it was verified against null\. Check lines: 3386, 3404\. Application\.cs 3386 ```cpp private void RunMessageLoopInner(int reason, ApplicationContext context) { .... hwndOwner = new HandleRef( null, UnsafeNativeMethods.GetWindowLong( new HandleRef(currentForm, currentForm.Handle), // <= NativeMethods.GWL_HWNDPARENT)); .... if (currentForm != null && ....) .... } ``` This is classic\. The _currentForm_ variable is used without any checks\. But then it's checked for _null_ in the code\. In this case I can advise you to be more attentive when working with reference types and also use static analyzers :\)\. One more such error: PVS\-Studio: [V3095](https://pvs-studio.com/en/docs/warnings/v3095/) The 'backgroundBrush' object was used before it was verified against null\. Check lines: 2331, 2334\. DataGrid\.cs 2331 ```cpp public Color BackgroundColor { .... set { .... if (!value.Equals(backgroundBrush.Color)) // <= { if (backgroundBrush != null && BackgroundBrush != DefaultBackgroundBrush) .... } } } ``` In WinForms code, I came across **more than 60** such errors\. In my opinion, all of them are rather critical and require attention of developers\. But it is not so interesting to tell about them in the article anymore, so I will limit myself to the two mentioned above\. PVS\-Studio: [V3125](https://pvs-studio.com/en/docs/warnings/v3125/) The '\_propInfo' object was used and was verified against null in different execution branches\. Check lines: 996, 982\. Binding\.cs 996 ```cpp private void SetPropValue(object value) { .... if (....) { if .... else if (_propInfo != null) .... } else { _propInfo.SetValue(_control, value); } .... } ``` For the completeness sake \- also a kind of classic, error [V3125](https://pvs-studio.com/en/docs/warnings/v3125/)\. The opposite situation\. At first, the developer uses a potentially _null_ reference safely, having checked it against null, but stops doing it further in the code\. And one more such error: PVS\-Studio: [V3125](https://pvs-studio.com/en/docs/warnings/v3125/) The 'owner' object was used after it was verified against null\. Check lines: 64, 60\. FlatButtonAppearance\.cs 64 ```cpp public int BorderSize { .... set { .... if (owner != null && owner.ParentInternal != null) { LayoutTransaction.DoLayoutIf(....); } owner.Invalidate(); // <= .... } } ``` Lovely\. But this an outside researcher's standpoint\. After all, the analyzer found **more than 50** such patterns in WinForms code besides these two [V3125](https://pvs-studio.com/en/docs/warnings/v3125/)\. Developers have a lot to work on\. And finally, there is an interesting error, in my opinion\. PVS\-Studio: [V3137](https://pvs-studio.com/en/docs/warnings/v3137/) The 'hCurrentFont' variable is assigned but is not used by the end of the function\. DeviceContext2\.cs 241 ```cpp 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; } } .... } ``` Let's see what alerted the analyzer, and why it may indicate a problem that a variable is assigned a value, but never used in the code\. The _DeviceContext2\.cs_ file contains a partial class\. The _DisposeFont_ method is used to free resources after working with graphics: device context and fonts\. For a better understanding I have given the whole _DisposeFont_ method\. Pay attention to the local variable _hCurrentFont_\. The problem is that the declaration of this variable in the method hides the class field of the same name\. I found two methods of the _DeviceContext_ class where the **field** with the name _hCurrentFont_ is used: ```cpp public IntPtr SelectFont(WindowsFont font) { .... hCurrentFont = font.Hfont; .... } public void ResetFont() { .... hCurrentFont = hInitialFont; } ``` Look at the _ResetFont_ method\. The last line there is exactly what the _DisposeFont_ method does in the subblock _if_ \(this is what the analyzer points to\)\. This _hCurrentFont_ field of the same name is declared in another part of the partial class in the _DeviceContext\.cs_ file: ```cpp sealed partial class DeviceContext : .... { .... IntPtr hInitialFont; .... IntPtr hCurrentFont; // <= .... } ``` Thus, an obvious mistake was made\. Another question is in its importance\. Now, as a result of the _DisposeFont_ method's work in the section marked with the comment "select initial font back in", the _hCurrentFont_ field will not be initialized\. I think only the authors of the code can give an exact verdict\. **Conclusions** So, this time, I'm gonna have to criticize MS a little bit\. In WinForms, there are a lot of errors that require close attention of developers\. Perhaps it is the fault of some haste with which MS work on \.NET Core 3 and components, including WinForms\. In my opinion, the WinForms code is still "raw", but I hope that the situation will change for the better soon\. The second reason for the large number of errors may be that our analyzer has simply become better at searching for them :\)\. By the way, an article of my colleague Sergey Vasiliev will soon be published in which he searches and finds quite a lot of problems in the code of \.NET Core libraries\. I hope that his work will also contribute to improving the characteristics of the \.NET platform, because we always try to inform the developers about the results of their projects' analysis\. And for those who want to improve their products on their own or search for errors in other people's projects, I suggest that you [download and try PVS\-Studio](https://pvs-studio.com/en/pvs-studio/download/)\. Clean code to everyone\!