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.

Introduction

In early December 2018, Microsoft announced 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 — the .NET Core UI platform for creating Windows desktop applications. You can see the commit statistics here. 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 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 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:

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

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

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:

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

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 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;  // <=
}

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

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

Because of a logical error, «dead code» occurred in this fragment. Expressions in the else block will never be executed.

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

Variable e in the condition is first used and then checked against null. Hello, NullReferenceException.

One more such error:

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

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

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

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

The analyzer suspected that the last two arguments were mixed up. Let's take a look at the declaration of the AdjustCellBorderStyle method:

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

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:

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

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

The paradoxical code. Apparently, the if (comboBox != null) check was confused withif (comboBox == null). And so, we will get another NullReferenceException.

We have considered two rather obvious V3080 errors where you can visually trace a potential null reference usage within a method. But the 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". But here is such kind of error detected in 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
  {
    ....
  }
  ....
}

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:

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

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:

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

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:

Detection of such errors is what demonstrates all the power and endless attention span of static analysis tools.

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

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

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

For the completeness sake — also a kind of classic, error 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 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();  // <=
    ....
  }
}

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. Developers have a lot to work on.

And finally, there is an interesting error, in my opinion.

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

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:

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:

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.

Clean code to everyone!

Автор: n0mo

Источник