# Top 10 errors found in C\# projects in 2023

In 2023, PVS\-Studio developers have written a lot of articles on checking open\-source C\# projects\. As always, we're sharing the top 10 bugs we found this year\. Have fun reading\!

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

## How can code get in the top?

There are several criteria the code of a project needs to meet to get in the top:

* the code is from an open\-source project;
* the project is analyzed using PVS\-Studio;
* the code contains errors;
* the code is interesting to check\.

As you can see, there aren't many conditions\. However, selecting the best 10 warnings wasn't a trivial task\. Yes, we'd like to look at more errors but still decided to follow the tradition and compile the top 10\.

You can find the previous compilations of C\# bugs here:

* [top 10 errors in 2022](https://pvs-studio.com/en/blog/posts/csharp/1020/)
* [top 10 errors in 2021](https://pvs-studio.com/en/blog/posts/csharp/0904/)
* [top 10 errors in 2020](https://pvs-studio.com/en/blog/posts/csharp/0787/)
* [top 10 errors in 2019](https://pvs-studio.com/en/blog/posts/csharp/0698/)

Well, let's take a look at the top warnings of 2023\!

P\.S\. Don't take the rankings too seriously\. The order is largely based on the author's subjective opinion\.

## 10th place\. Unexpected NullReferenceException

Our top opens with the analyzer warning from the [MudBlazor](https://pvs-studio.com/en/blog/posts/csharp/1032/) check:

```cpp
public static bool operator ==(ResizeOptions l, ResizeOptions r) 
                                                       => l.Equals(r);

public static bool operator !=(ResizeOptions l, ResizeOptions r) 
                                                       => !l.Equals(r);
```

The PVS\-Studio warnings:

* [V3115](https://pvs-studio.com/en/docs/warnings/v3115/) Passing 'null' to '\=\=' operator should not result in 'NullReferenceException'\. ResizeOptions\.cs 34
* [V3115](https://pvs-studio.com/en/docs/warnings/v3115/) Passing 'null' to '\!\=' operator should not result in 'NullReferenceException'\. ResizeOptions\.cs 35

In the implementations of overloading '\=\=' and '\!\=', the left operand isn't checked for _null_\. If the value of the _l_ parameter is _null_, the _NullReferenceException_ exception is thrown when the _Equals_ method is called\. The parameter of the _Equals_ method, by the way, is also dereferenced without checking:

```cpp
public bool Equals(ResizeOptions other)
{
  if (ReportRate != other.ReportRate || ....)     // <=
  {
    return false;
  }
  ....
}
```

Such behavior can be a very unpleasant surprise to the user if the class is public\. This is exactly what _ResizeOptions_ is\.

To make sure the issue exists, let's try to compare an object of the _ResizeOptions_ type to _null_\.

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

As expected, we get an exception of the _NullReferenceException_ type\. If the right operand is _null_, the behavior is the same\.

![1094_CSharpTop_2023/image3.png](https://import.viva64.com/docx/blog/1094_CSharpTop_2023/image3.png)

By the way, the [Microsoft documentation](https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/statements-expressions-operators/how-to-define-value-equality-for-a-type) mentions that such cases have to be taken into account\.

## 9th place\. Shift

The warning from the [Microsoft PowerToys check](https://pvs-studio.com/en/blog/posts/cpp/1078/) takes the 9th place\.

```cpp
public static List<PluginPair> AllPlugins
{
  get
  {
    ....
    try
    {
      // Return a comparable produce version.
      var fileVersion = FileVersionInfo.GetVersionInfo(x.ExecuteFilePath);
      return ((uint)fileVersion.ProductMajorPart << 48)
      | ((uint)fileVersion.ProductMinorPart << 32)
      | ((uint)fileVersion.ProductBuildPart << 16)
      | ((uint)fileVersion.ProductPrivatePart);
      }
    catch (System.IO.FileNotFoundException)
    {
      return 0U;
    }
    ....
  }
}
```

The PVS\-Studio warnings:

* [V3134](https://pvs-studio.com/en/docs/warnings/v3134/) Shift by 48 bits is greater than the size of 'UInt32' type of expression '\(uint\)fileVersion\.ProductMajorPart'\. PluginManager\.cs 62
* [V3134](https://pvs-studio.com/en/docs/warnings/v3134/) Shift by 32 bits is greater than the size of 'UInt32' type of expression '\(uint\)fileVersion\.ProductMinorPart'\. PluginManager\.cs 63

Please ignore the type of property and return value\. I've shortened the code, so the snippet shown here refers to the lambda expression\. The analyzer issues two warnings for this code\. The _uint_ type has a size of 32 bits\. It turns out that the first 48\-bit shift is equivalent to the 16\-bit shift\. The second 32\-bit shift is equivalent to the 0\-bit shift\. 

It's hard to tell what the developers wanted to do here, but they might have used _ulong_ instead of _uint_\.

## 8th place\. Questionable call

Another warning from the [aforementioned MudBlazor article](https://pvs-studio.com/en/blog/posts/csharp/1032/) comes in the 8th place:

```cpp
internal void DateValueChanged(DateTime? value)
{
  _valueDate = value;

  if (value != null)
  {
    var date = value.Value.Date;

    // get the time component and add it to the date.
    if (_valueTime != null)
    {
      date.Add(_valueTime.Value);                    // <=
    }

    _filterDefinition.Value = date;
  }
  else
    _filterDefinition.Value = value;

  _dataGrid.GroupItems();
}
```

The PVS\-Studio warning: [V3010](https://pvs-studio.com/en/docs/warnings/v3010/) The return value of function 'Add' is required to be utilized\. Filter\.cs 140

The return value of the _Add_ method called on a variable of the _DateTime_ type was not used here\. Most likely, the developer wanted to add _\_valueTime\.Value_ to _date_\. However, they didn't take into account that the _Add_ method for the _DateTime_ object returns the result of the addition and doesn't change the original object\. The result is a useless call that was probably meant to have an impact\. 

## 7th place\. Bitwise mess

We've taken the following warning from the [the Ryujinx check](https://pvs-studio.com/en/blog/posts/csharp/1059/):

```cpp
private void YesButton_Clicked(object sender, EventArgs args)
{
  ....
  Window.Functions = _mainWindow.Window.Functions =
    WMFunction.All & WMFunction.Close;
  ....
}
```

The PVS\-Studio warning: [V3182](https://pvs-studio.com/en/docs/warnings/v3182/) The result of 'WMFunction\.All & WMFunction\.Close' expression is '0'\. It is possible that the '\|' operator should be used instead\. UpdateDialog\.cs 69

As we can see, the analyzer complains about the '&' operator\. To understand why, let's take a look at the _WMFunction_ enumeration:

```cpp
[Flags]
public enum WMFunction
{
  All = 0x1,
  Resize = 0x2,
  Move = 0x4,
  Minimize = 0x8,
  Maximize = 0x10,
  Close = 0x20
}
```

We are dealing with bit flags, but in this case, the implementation of combining them doesn't work: the result of the bitwise AND \(&\) operation for the _WMFunction\.All_ and _WMFunction\.Close_ values is zero\.

Most likely, we need to replace '&' with '\|' for correct method operation\.

## 6th place\. Suspicious String\.Format

Moving on\. Here is a warning from the [check of the AWS SDK for \.NET](https://pvs-studio.com/en/blog/posts/csharp/1057/):

```cpp
private static string GetXamarinInformation()
{
  var xamarinDevice = Type.GetType("Xamarin.Forms.Device, Xamarin.Forms.Core");
  if (xamarinDevice == null)
  {
    return null;
  }

  var runtime = xamarinDevice.GetProperty("RuntimePlatform")
                            ?.GetValue(null)
                            ?.ToString() ?? "";

  var idiom = xamarinDevice.GetProperty("Idiom")
                          ?.GetValue(null)
                          ?.ToString() ?? "";

  var platform = runtime + idiom;

  if (string.IsNullOrEmpty(platform))
  {
    platform = UnknownPlatform;
  }

  return string.Format(CultureInfo.InvariantCulture, "Xamarin_{0}", "Xamarin");
}
```

The PVS\-Studio warning: [V3137](https://pvs-studio.com/en/docs/warnings/v3137/) The 'platform' variable is assigned but is not used by the end of the function\. InternalSDKUtils\.netstandard\.cs 70

The last line of the method looks strange\. The "Xamarin" string literal is substituted in the _"Xamarin\_\{0\}"_ template using _String\.Format\._ The value of the _platform_ variable, which can store the necessary information, is ignored\. That's strange\. 

We can assume that the _return_ statement should look like this:

```cpp
return string.Format(CultureInfo.InvariantCulture, "Xamarin_{0}", platform);
```

By the way, there's a similar method for getting the Unity game engine information\. It's written in a similar pattern, but the return value is produced correctly:

```cpp
private static string GetUnityInformation()
{
  var unityApplication 
    = Type.GetType("UnityEngine.Application, UnityEngine.CoreModule");
  if (unityApplication == null)
  {
    return null;
  }

  var platform = unityApplication.GetProperty("platform")
                                ?.GetValue(null)
                                ?.ToString() ?? UnknownPlatform;

  return string.Format(CultureInfo.InvariantCulture, "Unity_{0}", platform);
}
```



## 5th place\. More or less?

A warning from the [BTCPay Server check](https://pvs-studio.com/en/blog/posts/csharp/1051/) opens the second half of the top:

```cpp
private IActionResult Validate(StoreBaseData request)
{
  ....
  if (request.PaymentTolerance < 0 && request.PaymentTolerance > 100)
    ModelState.AddModelError(nameof(request.PaymentTolerance),
      "PaymentTolerance can only be between 0 and 100 percent");
  ....
}
```

The PVS\-Studio warning: [V3022](https://pvs-studio.com/en/docs/warnings/v3022/) Expression 'request\.PaymentTolerance < 0 && request\.PaymentTolerance \> 100' is always false\. Probably the '\|\|' operator should be used here\. BTCPayServer\\Controllers\\GreenField\\GreenfieldStoresController\.cs 241

Many people have faced transaction cancellations due to incorrect data input in financial apps\. Sometimes apps just notify users of incorrect data, but sometimes it's all about a silly bug in the code\.

So, in this code fragment, the '&&' and '\|\|' operators are mixed up\. The code was originally supposed to check for a value between 0 and 100, but the user wouldn't get a warning\.

As a result, an incorrect value traverses through the code and can lead to an error in the application logic\.

## 4th place\. Poorly set priorities

The warning, which is almost as good as the finalists, is in the 4th place\. It is taken from the already known [MudBlazor](https://pvs-studio.com/en/blog/posts/csharp/1032/) check\.

```cpp
internal async Task<bool> StartResizeColumn(....)
{
  ....
  // In case resize mode is column, we have to find any column right
  // of the current one that can also be resized and is not hidden.

  var nextResizableColumn = _columns.Skip(_....) + 1)
                                    .FirstOrDefault(c =>
                                        c.Resizable ?? true && !c.Hidden); // <=
  ....
}
```

The PVS\-Studio warning: [V3177](https://pvs-studio.com/en/docs/warnings/v3177/) The 'true' literal belongs to the '&&' operator with a higher priority\. It is possible the literal was intended to belong to '??' operator instead\. DataGridColumnResizeService\.cs 52

Let's look at the _c\.Resizable ?? true && \!c\.Hidden_ expression\. It's worth saying that the priority of '??' is lower than that of '&&'\. Based on this, if _c\.Resizable_ is _null_, then the null\-coalescing operator returns the result of the _true &&\!c\.Hidden_ operation\. The "&&" operation with the _true_ literal is pointless\. Most likely, the correct code looks as follows:

```cpp
(c.Resizable ?? true) && !c.Hidden
```

The difference between these variants can manifest itself if the values of _c\.Resizable_ and _c\.Hidden_ are _true_\.

There's another thing indicating that an error was made\. A line above has a comment saying that the column must be resizable \(the information on this can be found in _c\.Resizable_\) and not hidden \(you can learn about it in _c\.Hidden_\)\. If _c\.Resizable_ is _true_, then the logic of the operation is broken, since the value of _c\.Hidden_ doesn't affect the result in any way\.

## 3rd place\. Order does matter

And finally, we've made it to the top three\. It opens with a warning that is taken from the [article on checking the top\-3 open\-source games in C\#](https://pvs-studio.com/en/blog/posts/csharp/1056/)\. The project where we found the error is Barotrauma\. It's not easy to find a bug like this with your eyes, so the warning described here is a good example of how powerful static analysis is\. 

Let's look at the code fragment that contains the error:

```cpp
public void RecreateSprites()
{
  ....
  for (int i = 0; i < DecorativeSprites.Count; i++)
  {
    var decorativeSprite = DecorativeSprites[i];
    decorativeSprite.Remove();
    var source = decorativeSprite.Sprite.SourceElement;
    DecorativeSprites[i] = new DecorativeSprite(source, ....);          
  }
}
```

The PVS\-Studio warnings: [V3080](https://pvs-studio.com/en/docs/warnings/v3080/)\. Possible null dereference\. Consider inspecting 'decorativeSprite\.Sprite'\. Limb\.cs 462\.

In this case, the order of operations is wrong, which results in _NullReferenceException_\. It's clear from the implementation of the _decorativeSprite\.Remove_ method: 

```cpp
partial class DecorativeSprite : ISerializableEntity
{
  ....
  public Sprite Sprite { get; private set; }
  ....

  public void Remove()
  {
    Sprite?.Remove();
    Sprite = null;
    ....
  }
}
```

The method assigns the _null_ value to the _Sprite_ property\.

Looking again at the method in which _Remove_ is called, it's clear that the exception is thrown when accessing _decorativeSprite\.Sprite\.SourceElement_\. Invoking _Remove_ before the call causes this behavior\.

To avoid the exception, we can swap the calls of the _Remove_ method and initializations of the _source_ variable\.

## 2nd place\. Comparison to NAN

The second place, a little behind the winner, goes to the warning from the "[PVS\-Studio now analyzes Blazor components](https://pvs-studio.com/en/blog/posts/csharp/1023/)" article\. By the way, the article is titled that way for a reason :\)\. Indeed, PVS\-Studio can analyze Blazor components\. If you have a Blazor project, we invite you to [try](https://pvs-studio.com/en/pvs-studio/try-free/) the analyzer on it\. 

Let's get back to the warning\. The analyzer issued it for the MudBlazor code\.

Look at the suspicious fragment:

```cpp
@code
{
  ....
  public void Evaluate()
  {
    ....
    var exp = new Expression(CalcExpression);
    var result = exp.Eval();
    if (result == double.NaN)
    {
      Current = "ERROR";
      return;
    }
    Current = Math.Round( result,8).ToString(CultureInfo.InvariantCulture);
    CalcExpression = Current;
  }
  ....
}
```

The PVS\-Studio studio: [V3076](https://pvs-studio.com/en/docs/warnings/v3076/) Comparison of 'result' with 'double\.NaN' is meaningless\. Use 'double\.IsNaN\(\)' method instead\.

According to the analyzer, the comparison with _double\.NaN_ is pointless\. Why, though? According to [MSDN](https://learn.microsoft.com/en-us/dotnet/api/system.double.op_equality?view=netframework-4.8), comparing two values of _NaN_ using the '\=\=' operator always returns _false_\. To compare values correctly, we need to use the _double\.IsNaN_ method\. 

It's such a simple warning, but we think it deserves the second place in the top\. The specifics of the _NaN_ comparison may not be entirely clear\.

## 1st place\. Maybe there's no error at all?

Here is the crown jewel of today's top\. Why exactly does this warning take the first place? In our opinion, the issue it shows is the most difficult to identify compared to the other top representatives\. We've taken the following warning from the [Microsoft PowerToys check](https://pvs-studio.com/en/blog/posts/cpp/1078/)\.

Try to find the error yourself:

```cpp
private static int CalculateClosestSpaceIndex(List<int> spaceIndices,
                                              int firstMatchIndex)
{
  if (spaceIndices.Count == 0)
  {
    return -1;
  }
  else
  {
    int? ind = spaceIndices.OrderBy(item => (firstMatchIndex - item))
                           .Where(item => firstMatchIndex > item)
                           .FirstOrDefault();
    int closestSpaceIndex = ind ?? -1;
    return closestSpaceIndex;
  }
}
```

Did you succeed in detecting the issue here? We'd love to read your answers in the comments\. Just don't peek at the error description :\)\.

<details>
   <summary>It's hidden here\\\.</summary>

The PVS\-Studio warning: [V3022](https://pvs-studio.com/en/docs/warnings/v3022/) Expression 'ind' is always not null\. The operator '??' is excessive\. StringMatcher\.cs 230

The analyzer thinks that the operator '??' is needless because _ind_ is always _non\-null_\. However, is this really so? _Ind_ is nullable, which means it can be _null_\. The developers have used the _FirstOrDefault_ method, which may return _null_, to write the value into _Ind_\. It would seem that the case is closed — the analyzer made a mistake\. However, it's not that simple\. Let's dig a little deeper\.

In fact, _FirstOrDefault_ returns _default\(TSource\)_ instead of _null_, and _default\(int?\)_ is _null_\. However, _TSource_ turns into _int_ here because _TSource_ is the type of elements in the enumerated sequence\. In this case, the developers apply LINQ to the _spaceIndices_ parameter with the _List<int\>_ type\. This means that _default\(int\)_, i\.e\. 0, is written into _ind_\. Here comes the error in the program logic\. If the search method doesn't detect the collection item, it returns 0 instead of the intended \-1\.




</details>
## Conclusion

In 2023, we've checked quite a few C\# projects, but we can't say it's a lot\. However, that hasn't been an obstacle to the creation of this top\. On the contrary, there are some interesting warnings that we didn't include here because we didn't want to break the Top 10 tradition\. 

If you'd like to read more about these not\-included bugs, we have some worthwhile articles: 

* [Return to Grove Street\. Checking the Grand Theft Auto: San Andreas engine in Unity](https://pvs-studio.com/en/blog/posts/csharp/1083/)
* [Analysis of suspicious code fragments in MassTransit](https://pvs-studio.com/en/blog/posts/csharp/1061/)
* [5 amusing oddities in the code of Entity Framework Core](https://pvs-studio.com/en/blog/posts/csharp/1070/)

You can also compile your own top of warnings or just check a project :\)\. To do this, we invite you to [try PVS\-Studio](https://pvs-studio.com/en/pvs-studio/try-free/)\.