.NET 7: Suspicious Places and Errors in the Source Code
.NET 7 has been released! It's time for us to dig into its source code and start looking for errors and strange code fragments. In this article, you'll see comments on our findings from the .NET developers. After all, they know the platform code better than anyone else. Buckle up!
I analyzed the release code of .NET 7.
There were two release candidates (RC) prior to the main release, so most of the bugs must have been fixed. It's more interesting that way — we can investigate whether some of them have gotten into production.
I created an issue on GitHub for each suspicious code fragment. This helped me understand which ones are redundant, which ones are incorrect, and what was fixed by developers.
Issue 1
Can you spot an error here? Let's check!
internal sealed record IncrementalStubGenerationContext(
StubEnvironment Environment,
SignatureContext SignatureContext,
ContainingSyntaxContext ContainingSyntaxContext,
ContainingSyntax StubMethodSyntaxTemplate,
MethodSignatureDiagnosticLocations DiagnosticLocation,
ImmutableArray<AttributeSyntax> ForwardedAttributes,
LibraryImportData LibraryImportData,
MarshallingGeneratorFactoryKey<
(TargetFramework, Version, LibraryImportGeneratorOptions)
> GeneratorFactoryKey,
ImmutableArray<Diagnostic> Diagnostics)
{
public bool Equals(IncrementalStubGenerationContext? other)
{
return other is not null
&& StubEnvironment.AreCompilationSettingsEqual(Environment,
other.Environment)
&& SignatureContext.Equals(other.SignatureContext)
&& ContainingSyntaxContext.Equals(other.ContainingSyntaxContext)
&& StubMethodSyntaxTemplate.Equals(other.StubMethodSyntaxTemplate)
&& LibraryImportData.Equals(other.LibraryImportData)
&& DiagnosticLocation.Equals(DiagnosticLocation)
&& ForwardedAttributes.SequenceEqual(other.ForwardedAttributes,
(IEqualityComparer<AttributeSyntax>)
SyntaxEquivalentComparer.Instance)
&& GeneratorFactoryKey.Equals(other.GeneratorFactoryKey)
&& Diagnostics.SequenceEqual(other.Diagnostics);
}
public override int GetHashCode()
{
throw new UnreachableException();
}
}
This code fragment checks whether the this
and other
objects are equivalent. However, the developer made a mistake and compared the DiagnosticLocation
property with itself.
Incorrect comparison:
DiagnosticLocation.Equals(DiagnosticLocation)
Correct comparison:
DiagnosticLocation.Equals(other.DiagnosticLocation)
I found this error in the LibraryImportGenerator
class. A bit later I found two more fragments — the same error, but in different classes:
- The JSImportGenerator class
- The JSExportGenerator class
Fun fact: .NET 7 has a test for this feature. However, the test is also incorrect, that's why it doesn't detect this error.
In .NET 8 the code is heavily rewritten. However, the developers haven't fixed the .NET 7 code yet — they decided to wait for the feedback. You can read more about it in the issue on GitHub.
Issue 2
internal static void CheckNullable(JSMarshalerType underlyingSig)
{
MarshalerType underlying = underlyingSig._signatureType.Type;
if (underlying == MarshalerType.Boolean
|| underlying == MarshalerType.Byte
|| underlying == MarshalerType.Int16
|| underlying == MarshalerType.Int32
|| underlying == MarshalerType.BigInt64
|| underlying == MarshalerType.Int52
|| underlying == MarshalerType.IntPtr
|| underlying == MarshalerType.Double
|| underlying == MarshalerType.Single // <=
|| underlying == MarshalerType.Single // <=
|| underlying == MarshalerType.Char
|| underlying == MarshalerType.DateTime
|| underlying == MarshalerType.DateTimeOffset
) return;
throw new ArgumentException("Bad nullable value type");
}
Location: JSMarshalerType.cs, 387
Here the developer double-checks if the underlying
variable equals to MarshalerType.Single
. Sometimes such checks hide errors: for example, the left
and right
variables should have been checked, but instead, the left
variable is checked twice. Here's a list of similar errors found in open-source projects.
I created an issue on GitHub. Luckily, this code fragment wasn't erroneous — it was just a redundant check.
Issue 3
public static bool TryParse(string text, out MetricSpec spec)
{
int slashIdx = text.IndexOf(MeterInstrumentSeparator);
if (slashIdx == -1)
{
spec = new MetricSpec(text.Trim(), null);
return true;
}
else
{
string meterName = text.Substring(0, slashIdx).Trim();
string? instrumentName = text.Substring(slashIdx + 1).Trim();
spec = new MetricSpec(meterName, instrumentName);
return true;
}
}
Location: MetricsEventSource.cs, 453
The TryParse
method always returns true
. This is weird. Let's see where this method is used:
private void ParseSpecs(string? metricsSpecs)
{
....
string[] specStrings = ....
foreach (string specString in specStrings)
{
if (!MetricSpec.TryParse(specString, out MetricSpec spec))
{
Log.Message($"Failed to parse metric spec: {specString}");
}
else
{
Log.Message($"Parsed metric: {spec}");
....
}
}
}
Location: MetricsEventSource.cs, 375
The return value of the TryParse
method is used as the condition of the if
statement. If specString
cannot be parsed, the original value should be logged. Otherwise, the received representation (spec
) is logged, and some operations are performed on it.
The problem is TryParse
always returns true
. Thus, the then
branch of the if
statement is never executed — the parsing is always successful (link here to the Issue on GitHub).
As a result of the fix, TryParse
became Parse
, and the caller method lost the if
statement. The developers also changed Substring
to AsSpan
in TryParse
.
By the way, this is the same code fragment that I noted when digging in the .NET 6 source code. But back then, the interpolation character was missing in the logging method:
if (!MetricSpec.TryParse(specString, out MetricSpec spec))
{
Log.Message("Failed to parse metric spec: {specString}");
}
else
{
Log.Message("Parsed metric: {spec}");
....
}
You can read more about this issue in the article about .NET 6 check (issue 14).
Issue 4
Since we mentioned methods with strange return values, let's look at another one:
public virtual bool TryAdd(XmlDictionaryString value, out int key)
{
ArgumentNullException.ThrowIfNull(value);
IntArray? keys;
if (_maps.TryGetValue(value.Dictionary, out keys))
{
key = (keys[value.Key] - 1);
if (key != -1)
{
// If the key is already set, then something is wrong
throw System.Runtime
.Serialization
.DiagnosticUtility
.ExceptionUtility
.ThrowHelperError(
new InvalidOperationException(SR.XmlKeyAlreadyExists));
}
key = Add(value.Value);
keys[value.Key] = (key + 1);
return true; // <=
}
key = Add(value.Value);
keys = AddKeys(value.Dictionary, value.Key + 1);
keys[value.Key] = (key + 1);
return true; // <=
}
Location: XmlBinaryWriterSession.cs, 28
The method either returns true
or throws an exception — it never returns false
. This is a public API, so there's more demand for quality.
Let's look at the description on learn.microsoft.com:
Oopsie. I created an issue on GitHub for it as well, but at the moment of writing this article, there was no news on it.
Issue 5
public static Attribute? GetCustomAttribute(ParameterInfo element,
Type attributeType,
bool inherit)
{
// ....
Attribute[] attrib = GetCustomAttributes(element, attributeType, inherit);
if (attrib == null || attrib.Length == 0)
return null;
if (attrib.Length == 0)
return null;
if (attrib.Length == 1)
return attrib[0];
throw new AmbiguousMatchException(SR.RFLCT_AmbigCust);
}
Location: Attribute.CoreCLR.cs, 617
In this code fragment, the same expression — attrib.Length == 0
— is checked twice: first as a right operand of the ||
operator, then as a condition of the if
statement.
Sometimes this may be an error — developers want to check one thing but instead, check another. We were lucky here: the second check was just redundant and the developers removed it.
See the issue on GitHub.
Issue 6
protected virtual XmlSchema? GetSchema()
{
if (GetType() == typeof(DataTable))
{
return null;
}
MemoryStream stream = new MemoryStream();
XmlWriter writer = new XmlTextWriter(stream, null);
if (writer != null)
{
(new XmlTreeGen(SchemaFormat.WebService)).Save(this, writer);
}
stream.Position = 0;
return XmlSchema.Read(new XmlTextReader(stream), null);
}
Location: DataTable.cs, 6678
The developer created an instance of the XmlTextWriter
type. Then a reference to this instance is assigned to the writer
variable. However, in the next line the developer checked writer
for null
. The check always returns true
, which means the condition is redundant here.
It's not horrific, but it's better to remove the check. The developers did that, actually (issue on GitHub).
Issue 7
Redundant code again, but this time it's less obvious:
public int ToFourDigitYear(int year, int twoDigitYearMax)
{
if (year < 0)
{
throw new ArgumentOutOfRangeException(nameof(year),
SR.ArgumentOutOfRange_NeedPosNum);
}
if (year < 100)
{
int y = year % 100;
return (twoDigitYearMax / 100 - (y > twoDigitYearMax % 100 ? 1 : 0))
* 100 + y;
}
....
}
Location: GregorianCalendarHelper.cs, 526
Let's look at how the range of the year
variable is checked throughout the code execution:
ToFourDigitYear(int year, int twoDigitYearMax)
year
is a parameter of the int
type. Which means its value is within the [int.MinValue; int.MaxValue]
range.
When the code is executed, the if
statement is met first; in this statement, an exception is thrown:
if (year < 0)
{
throw ....;
}
If there's no exception, then the year
value is within [0; int.MaxValue]
.
Then, another if
statement:
if (year < 100)
{
int y = year % 100;
....
}
If the code execution is in the then
branch of if
, then the year
value is within the [0; 99] range. This leads to an interesting result — to the operation of taking the remainder of the division:
int y = year % 100;
The year
value is always less than 100 (i.e., the value is between 0-99). Therefore, the result of the year % 100
operation is always equal to the left operand — year
. Thus, y
is always equal to year
.
Either the code is redundant or it's an error. After I opened the issue on GitHub, the code was fixed and the y
variable was removed.
Issue 8
internal ConfigurationSection
FindImmediateParentSection(ConfigurationSection section)
{
....
SectionRecord sectionRecord = ....
if (sectionRecord.HasLocationInputs)
{
SectionInput input = sectionRecord.LastLocationInput;
Debug.Assert(input.HasResult, "input.HasResult");
result = (ConfigurationSection)input.Result;
}
else
{
if (sectionRecord.HasIndirectLocationInputs)
{
Debug.Assert(IsLocationConfig,
"Indirect location inputs exist
only in location config record");
SectionInput input = sectionRecord.LastIndirectLocationInput;
Debug.Assert(input != null);
Debug.Assert(input.HasResult, "input.HasResult");
result = (ConfigurationSection)input.Result;
}
....
....
}
Location: MgmtConfigurationRecord.cs, 341
We need to dig a bit deeper here. First, let's look at the second if
statement:
if (sectionRecord.HasIndirectLocationInputs)
{
Debug.Assert(IsLocationConfig,
"Indirect location inputs exist
only in location config record");
SectionInput input = sectionRecord.LastIndirectLocationInput;
Debug.Assert(input != null);
Debug.Assert(input.HasResult, "input.HasResult");
result = (ConfigurationSection)input.Result;
}
The value of the LastIndirectLocationInput
property is written to the input
variable. After that, input
is checked in two asserts: it's checked for null (input != null)
and for the presence of result (input.HasResult
).
Let's look at the LastIndirectLocationInput
property's body to understand which value can be written to the input
variable:
internal SectionInput LastIndirectLocationInput
=> HasIndirectLocationInputs
? IndirectLocationInputs[IndirectLocationInputs.Count - 1]
: null;
On the one hand, the property may return null
. On the other hand, if HasIndirectLocationInputs
is true
, then IndirectLocationInputs[IndirectLocationInputs.Count - 1]
is returned instead of explicit null
.
The question is, can the value from the IndirectLocationInputs
collection be null
? Probably yes, although it's not clear from the code. By the way, nullable annotations could help here, but they are not enabled in all .NET projects.
Let's go back to if
:
if (sectionRecord.HasIndirectLocationInputs)
{
Debug.Assert(IsLocationConfig,
"Indirect location inputs exist
only in location config record");
SectionInput input = sectionRecord.LastIndirectLocationInput;
Debug.Assert(input != null);
Debug.Assert(input.HasResult, "input.HasResult");
result = (ConfigurationSection)input.Result;
}
The conditional expression is sectionRecord.HasIndirectLocationInputs
. It's the same property that's checked in LastIndirectLocationInput
, which means LastIndirectLocationInput
definitely doesn't return explicit null
. However, it's unclear which value will be received from IndirectLocationInputs
and written to input
.
The developer first checks that input != null
and only then checks for the presence of the result — input.HasResult
. Looks okay.
Now let's go back to the first if
statement:
if (sectionRecord.HasLocationInputs)
{
SectionInput input = sectionRecord.LastLocationInput;
Debug.Assert(input.HasResult, "input.HasResult");
result = (ConfigurationSection)input.Result;
}
Let's look at the LastLocationInput
property:
internal SectionInput LastLocationInput
=> HasLocationInputs
? LocationInputs[LocationInputs.Count - 1]
: null;
It's written the same way as LastIndirectLocationInput
. Just like in the previous case, depending on the flag (HasLocationInputs
), either null
or a value from the LocationInputs
collection is returned.
Now return to the if
statement. Its conditional expression is the HasLocationInputs
property, which is checked within LastLocationInput
. If the code is executed in the then
branch of the if
statement, this means LastLocationInput
cannot return explicit null
. Can the value from the LocationInputs
collection be null
? The question remains unanswered. If it can, then null
will be written to input
too.
As in the case of the first inspected if
, input.HasResult
is checked but there's no input != null
this time.
Once again. The first inspected code fragment:
SectionInput input = sectionRecord.LastIndirectLocationInput;
Debug.Assert(input != null);
Debug.Assert(input.HasResult, "input.HasResult");
result = (ConfigurationSection)input.Result;
The second one:
SectionInput input = sectionRecord.LastLocationInput;
Debug.Assert(input.HasResult, "input.HasResult");
result = (ConfigurationSection)input.Result;
Looks like the Debug.Assert(input != null)
expression is missing.
I opened an issue on GitHub where I described this and other suspicious places related to null
checks (you'll see them below).
The developers decided not to fix this fragment and left it as is:
Issues With Null Checks
I came across several places in code where a reference is dereferenced and only then it's checked for null
. I created one issue for all similar code fragments on GitHub.
Let's inspect.
Issue 9
private static RuntimeBinderException BadOperatorTypesError(Expr pOperand1,
Expr pOperand2)
{
// ....
string strOp = pOperand1.ErrorString;
Debug.Assert(pOperand1 != null);
Debug.Assert(pOperand1.Type != null);
if (pOperand2 != null)
{
Debug.Assert(pOperand2.Type != null);
return ErrorHandling.Error(ErrorCode.ERR_BadBinaryOps,
strOp,
pOperand1.Type,
pOperand2.Type);
}
return ErrorHandling.Error(ErrorCode.ERR_BadUnaryOp, strOp, pOperand1.Type);
}
Location: ExpressionBinder.cs, 798
First, pOperand1
is dereferenced (pOperand1.ErrorString
) and is checked for null
in Debug.Assert
in the next code line. If pOperand1
is null
, then the assert is not triggered, but an exception of the NullReferenceException
type is thrown instead.
The code was fixed — pOperand1
is checked before use.
Before:
string strOp = pOperand1.ErrorString;
Debug.Assert(pOperand1 != null);
Debug.Assert(pOperand1.Type != null);
After:
Debug.Assert(pOperand1 != null);
Debug.Assert(pOperand1.Type != null);
string strOp = pOperand1.ErrorString;
Issue 10
public void Execute()
{
var count = _callbacks.Count;
if (count == 0)
{
return;
}
List<Exception>? exceptions = null;
if (_callbacks != null)
{
for (int i = 0; i < count; i++)
{
var callback = _callbacks[i];
Execute(callback, ref exceptions);
}
}
if (exceptions != null)
{
throw new AggregateException(exceptions);
}
}
Location: PipeCompletionCallbacks.cs, 20
The _callbacks
variable is used first and only then it's checked for null
:
public void Execute()
{
var count = _callbacks.Count;
....
if (_callbacks != null)
....
}
At the time of writing this article, the developers removed checking _callbacks
for null
.
By the way, _callbacks
is a readonly
field that's initialized in a constructor:
internal sealed class PipeCompletionCallbacks
{
private readonly List<PipeCompletionCallback> _callbacks;
private readonly Exception? _exception;
public PipeCompletionCallbacks(List<PipeCompletionCallback> callbacks,
ExceptionDispatchInfo? edi)
{
_callbacks = callbacks;
_exception = edi?.SourceException;
}
....
}
In the thread with the fix, the developers discussed whether it was worth adding Debug.Assert
and checking _callbacks
for null
into a constructor. In the end, they decided it wasn't.
Issue 11
private void ValidateAttributes(XmlElement elementNode)
{
....
XmlSchemaAttribute schemaAttribute
= (_defaultAttributes[i] as XmlSchemaAttribute)!;
attrQName = schemaAttribute.QualifiedName;
Debug.Assert(schemaAttribute != null);
....
}
Location: DocumentSchemaValidator.cs, 421
The controversial code:
- The result of the
as
operator is written toschemaAttribute
. If_defaultAttributes[i]
–null
or the cast failed, the result will benull
. - The null-forgiving operator (
!
) implies that the result of casting cannot benull
. Therefore,schemaAttribute
cannot benull
. - In the next code line,
schemaAttribute
is dereferenced. Then in a line below, the reference is checked fornull
.
Here's the question. Can schemaAttribute
be null
? It's not very clear from the code.
The code was fixed like that:
....
XmlSchemaAttribute schemaAttribute
= (XmlSchemaAttribute)_defaultAttributes[i]!;
attrQName = schemaAttribute.QualifiedName;
....
During the discussion of the fix, the developer proposed moving the Debug.Assert
call in the line above instead of removing it. The code would look like that:
....
XmlSchemaAttribute schemaAttribute = (XmlSchemaAttribute)_defaultAttributes[i]!;
Debug.Assert(schemaAttribute != null);
attrQName = schemaAttribute.QualifiedName;
....
In the end, they decided not to return Assert
.
Issue 12
Let's look at the constructor of the XmlConfigurationElementTextContent
type:
public XmlConfigurationElementTextContent(string textContent,
int? linePosition,
int? lineNumber)
{ .... }
Location: XmlConfigurationElementTextContent.cs, 10
Now let's see where it's used:
public static IDictionary<string, string?> Read(....)
{
....
case XmlNodeType.EndElement:
....
var lineInfo = reader as IXmlLineInfo;
var lineNumber = lineInfo?.LineNumber;
var linePosition = lineInfo?.LinePosition;
parent.TextContent = new XmlConfigurationElementTextContent(string.Empty,
lineNumber,
linePosition);
....
break;
....
case XmlNodeType.Text:
....
var lineInfo = reader as IXmlLineInfo;
var lineNumber = lineInfo?.LineNumber;
var linePosition = lineInfo?.LinePosition;
XmlConfigurationElement parent = currentPath.Peek();
parent.TextContent = new XmlConfigurationElementTextContent(reader.Value,
lineNumber,
linePosition);
....
break;
....
}
Locations:
- XmlStreamConfigurationProvider.cs, 133
- XmlStreamConfigurationProvider.cs, 148
Have you noticed anything strange in the code?
Pay attention to the order of arguments and parameters:
- arguments:
lineNumber
,linePosition
- parameters:
linePosition
,lineNumber
I created an issue on GitHub, and the code was fixed: the developers put arguments in the correct order and added a test.
Issue 13
Another suspicious case:
public virtual bool Nested
{
get {....}
set
{
....
ForeignKeyConstraint? constraint
= ChildTable.Constraints
.FindForeignKeyConstraint(ChildKey.ColumnsReference,
ParentKey.ColumnsReference);
....
}
}
Location: DataRelation.cs, 486
Look at the FindForeignKeyConstraint
method:
internal ForeignKeyConstraint?
FindForeignKeyConstraint(DataColumn[] parentColumns,
DataColumn[] childColumns)
{ .... }
Location: ConstraintCollection.cs, 548
Seems like the argument order is mixed up again:
- parameters:
parent
,child
- arguments:
ChildKey
,ParentKey
There's another method call: the argument order is correct there.
ForeignKeyConstraint? foreignKey
= relation.ChildTable
.Constraints
.FindForeignKeyConstraint(relation.ParentColumnsReference,
relation.ChildColumnsReference);
I created an issue on GitHub. Unfortunately, I haven't received any comments on it at the moment of writing the article.
Issue 14
These are not all places where the argument order is mixed up — I found another one:
void RecurseChildren(....)
{
....
string? value
= processValue != null
? processValue(new ConfigurationDebugViewContext(
child.Key,
child.Path,
valueAndProvider.Value,
valueAndProvider.Provider))
: valueAndProvider.Value;
....
}
Location: ConfigurationRootExtensions.cs, 50
Look at the ConfigurationDebugViewContext
constructor:
public ConfigurationDebugViewContext(
string path,
string key,
string? value,
IConfigurationProvider configurationProvider)
{ .... }
Location: ConfigurationDebugViewContext.cs, 11
The order:
- parameters:
path
,key
- arguments:
child.Key
,child.Path
I created an issue on GitHub. According to the developers, this case doesn't have any issues despite the mistake.
However, they still fixed the order of arguments.
Conclusion
The .NET code is of high quality. I believe this is achieved by an established development process — the developers know the exact release date. Besides, release candidates help find the most serious errors and prepare the project for the release.
Nevertheless, I still manage to find something intriguing in the code. This time my favorites are arguments that were mixed up during method calls.
All code fragments described in this article were found by the PVS-Studio analyzer.