# Bugs and suspicious code fragments in \.NET 10

A little over six months ago, \.NET 10 was released\. Have any defects remained in the project's source code after all these months? Let's try to find the answer to this question\. Enjoy the article\!

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

The new version brings a lot of changes: performance improvements, library updates, new APIs, and further enhancements to the compiler and runtime\. However, as experience shows, even code in projects of this level can contain errors\.

So, we checked the \.NET 10 source code \(version [10\.0\.8](https://github.com/dotnet/runtime/tree/v10.0.8)\) using [PVS\-Studio](https://pvs-studio.com/en/) static analyzer\.

During the analysis, we found multiple suspicious code fragments, potential bugs, and some curious snippets worth the developers' attention\. In this article, we'll go over the most exciting warnings the analyzer has issued\.

As mentioned earlier, quite some time has passed since the release of \.NET 10\. However, a quick refresher on the new features wouldn't hurt:

* [What's new in \.NET 10?](https://pvs-studio.com/en/blog/posts/csharp/1308/)
* [What's new in C\# 14: overview](https://pvs-studio.com/en/blog/posts/csharp/1301/)

Before we dive into the \.NET 10 code, I'd like to mention that PVS\-Studio continues to expand the list of supported languages\. You can now try the analyzers for Go, JavaScript, and TypeScript through the early access program\. Sign up [here](https://pvs-studio.com/en/pvs-studio-eap/)\.

Let's dig into the code\!

## Always false

**Code snippet 1**

```cpp
public static void ReflectionWriteValue(....)
{
  ....
  if (memberValue == null)                                      // <=
  {
    context.WriteNull(xmlWriter,
                    memberType, 
                    DataContract.IsTypeSerializable(memberType));
  }
  else
  {
    PrimitiveDataContract? primitiveContract = ....;
    if (   primitiveContract != null 
        && primitiveContract.UnderlyingType != Globals.TypeOfObject 
        && !writeXsiType)
    {
      primitiveContract.WriteXmlValue(xmlWriter, memberValue, context);
    }
    else
    {
      if (   memberValue == null                                // <=
          && (   memberType == Globals.TypeOfObject
              || (originValueIsNullableOfT && memberType.IsValueType)))
      {
        context.WriteNull(xmlWriter,
                          memberType,
                          DataContract.IsTypeSerializable(memberType));
      }
      else
      {
        ReflectionInternalSerialize(xmlWriter,
                                    context,
                                    memberValue!,
                                    memberValue!.GetType()
                                               .TypeHandle
                                               .Equals(memberType.TypeHandle),
                                    writeXsiType, 
                                    memberType,
                                    originValueIsNullableOfT);
      }
    }
  }
  ....
}
```

PVS\-Studio warning: [V3022](https://pvs-studio.com/en/docs/warnings/v3022/) Expression 'memberValue \=\= null && \(memberType \=\= Globals\.TypeOfObject \|\| \(originValueIsNullableOfT && memberType\.IsValueType\)\)' is always false\. [ReflectionClassWriter\.cs 94](https://github.com/dotnet/runtime/blob/v10.0.8/src/libraries/System.Private.DataContractSerialization/src/System/Runtime/Serialization/ReflectionClassWriter.cs#L94)

Take a look at the top\-level `if` statement that checks `memberValue` for `null`\. As a result, in the `else` block of this `if` statement, the `memberValue` variable isn't `null`\. However, this code block contains a check:

```cpp
if (   memberValue == null 
    && (   memberType == Globals.TypeOfObject
        || (originValueIsNullableOfT && memberType.IsValueType)))
```

This condition is always false because the `memberValue == null` check is an operand of the `&&` operator\.

## Precedence error

**Code snippet 2**

```cpp
protected override string GetName(NodeFactory factory)
{
  return   "Reflectable delegate type: " 
         + _delegateType?.ToString() ?? "All delegates";
}
```

PVS\-Studio warning: [V3123](https://pvs-studio.com/en/docs/warnings/v3123/) Perhaps the '??' operator works in a different way than it was expected\. Its priority is lower than priority of other operators in its left part\. [ReflectedDelegateNode\.cs 38](https://github.com/dotnet/runtime/blob/v10.0.8/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectedDelegateNode.cs#L38)

Quite often, developers forget that the `??` operator has one of the lowest precedence levels\. This may cause issues affecting program logic\. Fortunately, the logic in this example remains intact, but the issue still exists\. If the `delegateType` field is null when the `GetName` method executes, instead of the expected message "Reflectable delegate type: All delegates" we will get "Reflectable delegate type: "\. This happens because the `+` operator has higher precedence than `??`\. To fix the issue, wrap part of the expression in parentheses:

```cpp
"Reflectable delegate type: " + (_delegateType?.ToString() ?? "All delegates");
```

## They mixed them up

**Code snippet 3**

```cpp
public ISymbolNode GenericLookupHelper(
    CORINFO_RUNTIME_LOOKUP_KIND runtimeLookupKind,
    ReadyToRunHelperId helperId,
    object helperArgument,
    GenericContext methodContext)
{
  switch (helperId)
  {
    case ReadyToRunHelperId.TypeHandle:
      return GenericLookupTypeHelper(
          runtimeLookupKind,
          ReadyToRunFixupKind.TypeHandle,
          helperArgument,
          methodContext);

    case ReadyToRunHelperId.MethodHandle:
      return GenericLookupMethodHelper(
          runtimeLookupKind,
          ReadyToRunFixupKind.MethodHandle,
          (MethodWithToken)helperArgument,
          methodContext);

    case ReadyToRunHelperId.MethodEntry:
      return GenericLookupMethodHelper(
          runtimeLookupKind,
          ReadyToRunFixupKind.MethodEntry,
          (MethodWithToken)helperArgument,
          methodContext);

    case ReadyToRunHelperId.MethodDictionary:  // <=
      return GenericLookupMethodHelper(
          runtimeLookupKind,
          ReadyToRunFixupKind.MethodHandle,    // <=
          (MethodWithToken)helperArgument,
          methodContext);

    case ReadyToRunHelperId.TypeDictionary:
      return GenericLookupTypeHelper(
          runtimeLookupKind,
          ReadyToRunFixupKind.TypeDictionary,
          (TypeDesc)helperArgument,
          methodContext);

    case ReadyToRunHelperId.VirtualDispatchCell:
      return GenericLookupMethodHelper(
          runtimeLookupKind,
          ReadyToRunFixupKind.VirtualEntry,
          (MethodWithToken)helperArgument,
          methodContext);

    case ReadyToRunHelperId.FieldHandle:
      return GenericLookupFieldHelper(
          runtimeLookupKind,
          ReadyToRunFixupKind.FieldHandle,
          (FieldWithToken)helperArgument,
          methodContext);

    default:
      throw new NotImplementedException(helperId.ToString());
  }
}
```

PVS\-Studio warning: [V3139](https://pvs-studio.com/en/docs/warnings/v3139/) Two or more case\-branches perform the same actions\. [ReadyToRunSymbolNodeFactory\.cs 583](https://github.com/dotnet/runtime/blob/v10.0.8/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunSymbolNodeFactory.cs#L583)

The code contains many similar operations written in the same pattern, so an error could easily have slipped in\. If `helperId` equals `ReadyToRunHelperId.MethodDictionary`, then `ReadyToRunFixupKind.MethodHandle` is passed to the `GenericLookupMethodHelper` method\. This doesn't look quite right, since the `ReadyToRunFixupKind` enum already includes a `MethodDictionary` element\. It seems that `GenericLookupMethodHelper` should receive it when `helperId` matches `ReadyToRunHelperId.MethodDictionary`\. 

**Code snippet 4**

```cpp
partial class MutableModule
{
  ....

  public int CompareTo(MutableModule other)
  {
    return _index.CompareTo(_index);
  }
}
```

PVS\-Studio warning: [V3062](https://pvs-studio.com/en/docs/warnings/v3062/) An object '\_index' is used as an argument to its own method\. Consider checking the first actual argument of the 'CompareTo' method\. [MutableModule\.Sorting\.cs 19](https://github.com/dotnet/runtime/blob/v10.0.8/src/coreclr/tools/aot/ILCompiler.ReadyToRun/TypeSystem/Mutable/MutableModule.Sorting.cs#L19)

In the `CompareTo` method, the `_index` field is compared to itself\. Most likely, `other._index` should be passed as an argument\.

**Code snippet 5**

```cpp
public void ComputeReturnValueTreatment(....)
{
  ....

  if (descriptor.passedInRegisters)
  {
    if (descriptor.eightByteCount == 1)
    {
      if (descriptor.eightByteClassifications0 
                == SystemVClassificationType.SystemVClassificationTypeSSE)
      {
        // Structs occupying just one eightbyte are treated as int / double
        fpReturnSize = sizeof(double);
      }
    }
    else
    {
      // Size of the struct is 16 bytes
      fpReturnSize = 16;
      // The lowest two bits of the size encode 
      // the order of the int and SSE fields
      if (descriptor.eightByteClassifications0                            // <=
               == SystemVClassificationType.SystemVClassificationTypeSSE)
      {
        fpReturnSize += 1;
      }

      if (descriptor.eightByteClassifications0                            // <=
               == SystemVClassificationType.SystemVClassificationTypeSSE)
      {
        fpReturnSize += 2;
      }
    }

    break;
  }

  ....
}
```

PVS\-Studio warning: [V3029](https://pvs-studio.com/en/docs/warnings/v3029/) The conditional expressions of the 'if' statements situated alongside each other are identical\. Check lines: 364, 369\. [TransitionBlock\.cs 364](https://github.com/dotnet/runtime/blob/v10.0.8/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TransitionBlock.cs#L364)

The analyzer reported a series of `if` statements with identical conditions\. Indeed, `descriptor.eightByteClassifications0` is compared twice with `SystemVClassificationType.SystemVClassificationTypeSSE`\. It seems likely that the check is incorrect in one of the cases, given the presence of the `eightByteClassifications1` field in `descriptor`\. Most likely, the field should be used in one of the checks\.

## Are you sure it's not null?

**Code snippet 6**

```cpp
public static bool SubstEqualTypeArrays(TypeArray taDst,
                                        TypeArray taSrc,
                                        TypeArray typeArgsCls,
                                        TypeArray typeArgsMeth)
{
  // Handle the simple common cases first.
  if (taDst == taSrc || (taDst != null && taDst.Equals(taSrc))) // <=
  {
    ....
    return true;
  }
  if (taDst.Count != taSrc.Count)                               // <=
    return false;
  if (taDst.Count == 0)
    return true;

  var ctx = new SubstContext(typeArgsCls, typeArgsMeth, true);
  if (ctx.IsNop)
  {
    return false;
  }

  for (int i = 0; i < taDst.Count; i++)
  {
    if (!SubstEqualTypesCore(taDst[i], taSrc[i], ctx))
      return false;
  }

  return true;
}
```

PVS\-Studio warning: [V3125](https://pvs-studio.com/en/docs/warnings/v3125/) The 'taDst' object was used after it was verified against null\. Check lines: 354, 344\. [TypeManager\.cs 354](https://github.com/dotnet/runtime/blob/v10.0.8/src/libraries/Microsoft.CSharp/src/Microsoft/CSharp/RuntimeBinder/Semantics/Types/TypeManager.cs#L354)

In the first `if` statement, the `taDst` parameter is checked for `null`, suggesting that it may be `null`\. However, in the next `if` statement, the `taDst` parameter is dereferenced without being checked: `taDst.Count`\. If `taDst` is `null` and `taSrc` isn't, then a `NullReferenceException` will be thrown\.

**Code snippet 7**

```cpp
internal void WriteXmlnsAttribute(XmlDictionaryString ns)
{
  if (dictionaryWriter != null)
  {
    if (ns != null) 
      dictionaryWriter.WriteXmlnsAttribute(null, ns);
  }
  else
    WriteXmlnsAttribute(ns.Value);
}
```

PVS\-Studio warning: [V3125](https://pvs-studio.com/en/docs/warnings/v3125/) The 'ns' object was used and was verified against null in different execution branches\. Check lines: 71, 67\. [XmlWriterDelegator\.cs 71](https://github.com/dotnet/runtime/blob/v10.0.8/src/libraries/System.Private.DataContractSerialization/src/System/Runtime/Serialization/XmlWriterDelegator.cs#L71)

This issue is similar to the previous one\. In the `then` block of the first `if` statement, the `ns` parameter is checked for `null` before being used\. This check is missing from the `else` block\. The `ns` parameter may be related to the `dictionaryWriter` field, meaning we don't need to check `ns` for `null` if `dictionaryWriter` is `null`\. However, we couldn't find any evidence of this\.

It currently looks like if `dictionaryWriter` and `ns` are `null`, accessing `ns.Value` will throw a `NullReferenceException`\.

**Code snippet 8**

```cpp
private struct ModuleAndIntValueKey : IEquatable<ModuleAndIntValueKey>
{
  public bool Equals(ModuleAndIntValueKey other) => 
         IntValue == other.IntValue 
      && (   (Module == null && other.Module == null) 
          ||  Module.Equals(other.Module));
}
```

PVS\-Studio warning: [V3125](https://pvs-studio.com/en/docs/warnings/v3125/) The 'Module' object was used after it was verified against null\. Check lines: 180, 180\. [ReadyToRunCodegenNodeFactory\.cs 180](https://github.com/dotnet/runtime/blob/v10.0.8/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs#L180)

When comparing objects of the `ModuleAndIntValueKey` type, their `Module` fields are checked:

```cpp
(Module == null && other.Module == null) ||  Module.Equals(other.Module)
```

If `Module` is null and `other.Module` isn't, a `NullReferenceException` will be thrown\. Since the left operand of the `||` operator is `false`, the condition will be evaluated further, resulting in the dereferencing of `Module` when `Equals` is called\.

To fix this, we'll add a `null` check before accessing `Module`\.

**Code snippet 9**

```cpp
public bool IsInheritanceChainLayoutFixedInCurrentVersionBubble(TypeDesc type)
{
  // This method is not expected to be called for value types
  Debug.Assert(!type.IsValueType);

  if (type.IsObject)
    return true;

  if (!IsLayoutFixedInCurrentVersionBubble(type))
  {
    return false;
  }

  type = type.BaseType;

  if (type != null)
  {
    return false;

    while (!type.IsObject && type != null)
    {
      if (!IsLayoutFixedInCurrentVersionBubble(type))
      {
        return false;
      }
      type = type.BaseType;
    }
  }

    return true;
}
```

PVS\-Studio warning: [V3027](https://pvs-studio.com/en/docs/warnings/v3027/) The variable 'type' was utilized in the logical expression before it was verified against null in the same logical expression\. [ReadyToRunCodegenCompilation\.cs 596](https://github.com/dotnet/runtime/blob/v10.0.8/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs#L596)

In the `while` statement, the `IsObject` property of the `type` variable is accessed first, and then `type` is checked for `null`\. Such a check seems ineffective\. If `type` is null, the `NullReferenceException` is thrown\. 

To fix this, we need to either swap the `null` check and the property access, or, if `type` is never `null`, remove the `null` check\.

## Tricky comparison

**Code snippet 10**

```cpp
public class PropertyPseudoDesc : TypeSystemEntity
{
  ....

  public static bool operator ==(PropertyPseudoDesc a, PropertyPseudoDesc b) 
              => a._type == b._type && a._handle == b._handle;

  ....
}
```

PVS\-Studio warning: [V3115](https://pvs-studio.com/en/docs/warnings/v3115/) Passing 'null' to '\=\=' operator should not result in 'NullReferenceException'\. [PropertyPseudoDesc\.cs 89](https://github.com/dotnet/runtime/blob/v10.0.8/src/coreclr/tools/Common/Compiler/PropertyPseudoDesc.cs#L89)

If one of the operands is `null` when comparing objects of the `PropertyPseudoDesc` type, the `NullReferenceException` will be thrown\. To avoid this, we need to check whether the operand fields are `null` before accessing them\.

## Conclusion

The \.NET 10 check showed us that even six months after the release, the project still contains bugs and code snippets that could cause issues\. Some of them are classic errors: ambiguous conditions, redundant checks, potential NREs, and confusing logic branches\. Such issues are difficult to spot during a quick code review, especially in a large, platform\-level codebase\.

Every year, we check the latest \.NET version:

* [The \.NET 9 check](https://pvs-studio.com/en/blog/posts/csharp/1216/);
* [The \.NET 8 check](https://pvs-studio.com/en/blog/posts/csharp/1095/);
* [The \.NET 7 check](https://pvs-studio.com/en/blog/posts/csharp/1015/);
* [The \.NET 6 check](https://pvs-studio.com/en/blog/posts/csharp/0903/)\.

We also frequently post checks of other popular projects\. To stay up to date on new checks, [subscribe](https://pvs-studio.com/en/subscribe/) to our newsletter\.

If you'd like to try out PVS\-Studio yourself, click the [link](https://pvs-studio.com/en/pvs-studio/try-free/)\!