.NET 7 зарелизился. Это хороший повод покопаться в исходниках, чтобы поискать ошибки и странные места. За комментариями по находкам обратимся к самим разработчикам .NET — кому знать код, как не им? Погнали!
Я анализировал релизный код .NET 7. Взять его можно на GitHub: ссылка.
Перед релизом было 2 выпуска RC (release candidate), а поэтому основные баги должны быть устранены. Но тем даже интереснее, просочилось ли что-нибудь "в прод".
На каждый подозрительный фрагмент кода я открыл issue на GitHub. Это помогло понять, какой код ошибочен, какой избыточен, и какие правки вносили разработчики.
Issue 1
Проверка внимательности — что не так с кодом ниже?
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 и other. Однако в одном из выражений допустили ошибку, сравнив свойство DiagnosticLocation с собой же.
Так неправильно:
DiagnosticLocation.Equals(DiagnosticLocation)Так правильно:
DiagnosticLocation.Equals(other.DiagnosticLocation)Эту проблему я нашёл в классе LibraryImportGenerator (ссылка на GitHub). Чуть позже нашёл ещё 2 записи с такими же ошибками, но уже в других классах:
Что интересно, в .NET 7 на эту функциональность есть тест. Загвоздка в том, что тест тоже ошибочный и поэтому проблему не выявлял.
В .NET 8 рассмотренный код сильно переписан, а для .NET 7 фикс пока не применялся. Подробности можно почитать в issue на 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 (link)
Здесь два раза проверили переменную underlying на равенство MarshalerType.Single. Иногда за подобными одинаковыми проверками скрываются ошибки: условно, должны были проверить переменные left и right, но два раза проверили left. Примеры подобных ошибок из Open Source проектов собраны здесь.
Открыл issue на GitHub: link. В рассматриваемом случае из .NET 7 повезло — проверка просто оказалась избыточной.
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 (link)
Метод TryParse всегда возвращает одно и то же значение – true. Это странно. Посмотрим, где он используется:
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 (link)
Возвращаемое значение метода TryParse используется как условие оператора if. Если не удалось распарсить строку specString, исходное значение должно логироваться. Иначе логируется полученное представление — spec —, и над ним выполняются ещё какие-то операции.
Проблема в том, что TryParse всегда возвращает true. Значит, then-ветвь оператора if никогда не выполняется, то есть парсинг всегда считается успешным.
Issue на GitHub: link.
В результате исправления TryParse превратился в Parse, а в вызывающем методе пропал оператор if. В TryParse заодно поменяли Substring на AsSpan.
Кстати, этот же фрагмент кода я отмечал в прошлый раз, когда копался в исходниках .NET 6. Тогда в методах логирования был пропущен символ интерполяции:
if (!MetricSpec.TryParse(specString, out MetricSpec spec))
{
Log.Message("Failed to parse metric spec: {specString}");
}
else
{
Log.Message("Parsed metric: {spec}");
....
}Подробнее про эту и подобную ошибки можно почитать в статье про проверку .NET 6 (issue 14).
Issue 4
Раз уж мы затронули тему методов со странными возвращаемыми значениями, рассмотрим ещё один:
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 (link)
Метод или возвращает true, или выбрасывает исключение, но никогда не возвращает false. Это уже публичный API, так что и спроса больше.
Посмотрим описание на learn.microsoft.com:
Нестыковочка. Я также открыл issue на GitHub (ссылка), но на момент написания статьи новостей не было.
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 (link)
В этом фрагменте кода 2 раза проверяют одно и то же выражение — attrib.Length == 0: сначала как правый операнд оператора '||', затем как условное выражение оператора if.
Иногда это признак ошибки — хотели проверить одно, а проверили другое. Здесь повезло: вторая проверка оказалась избыточной, и разработчики её убрали.
Issue на GitHub: link.
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 (link)
Создали экземпляр типа XmlTextWriter, ссылку на него записали в переменную writer и сразу на следующей строке проверяют её на неравенство null. Проверка всегда будет давать true, значит условие здесь лишнее.
Нестрашно, но проверку можно убрать. Так и сделали (issue на GitHub):
Issue 7
Тоже избыточный код, но чуть менее очевидный.
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 (link)
Давайте проследим за тем, как уточняется значение переменной year по мере исполнения кода.
ToFourDigitYear(int year, int twoDigitYearMax)year – параметр метода типа int. Значит, его значение находится в диапазоне [int.MinValue; int.MaxValue].
При исполнении кода первым делом встречается оператор if, в котором выбрасывается исключение:
if (year < 0)
{
throw ....;
}Если исключение не было выброшено, значит значение year лежит в диапазоне [0; int.MaxValue].
Дальше ещё один оператор if:
if (year < 100)
{
int y = year % 100;
....
}Если исполнение кода зашло в then-ветвь оператора if, значит значение year находится в диапазоне [0; 99]. Это уже приводит к интересному результату операции взятия остатка от деления:
int y = year % 100;Значение year всегда меньше 100 (от 0 до 99). Как следствие, результат операции year % 100 всегда будет равен левому операнду – year. Получается, что y и year всегда равны.
Итого, или код избыточный, или здесь какая-то ошибка. Открыл issue на GitHub: код поправили, убрав переменную y.
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 (link)
Здесь придётся немного повозиться. Для начала посмотрим на второй оператор 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;
}В переменную input записывается значение свойства LastIndirectLocationInput. После этого input проверяется в двух ассертах: на null (input != null) и наличие результата (input.HasResult).
Посмотрим на тело свойства LastIndirectLocationInput, чтобы понять, какое значение может быть записано в переменную input:
internal SectionInput LastIndirectLocationInput
=> HasIndirectLocationInputs
? IndirectLocationInputs[IndirectLocationInputs.Count - 1]
: null;Смотрите, с одной стороны, свойство может вернуть значение null. С другой — видно, что если HasIndirectLocationInputs — true, то возвращается IndirectLocationInputs[IndirectLocationInputs.Count - 1], а не явное значение null.
Вопрос вот в чём — может ли значение из коллекции IndirectLocationInputs быть равно null? Возможно — из этого кода непонятно. Кстати, тут могли бы помочь nullable-аннотации, но они включены не во всех проектах .NET.
Возвращаемся в 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;
}Условное выражение — sectionRecord.HasIndirectLocationInputs. Это то же самое свойство, которое проверяется в LastIndirectLocationInput. Значит, LastIndirectLocationInput точно не возвращает явный null. Однако какое значение будет получено из IndirectLocationInputs и записано в input — непонятно.
Разработчики сначала проверяют, что input != null, и лишь затем смотрят наличие результата — input.HasResult. Выглядит нормально.
Теперь вернёмся в первый оператор if:
if (sectionRecord.HasLocationInputs)
{
SectionInput input = sectionRecord.LastLocationInput;
Debug.Assert(input.HasResult, "input.HasResult");
result = (ConfigurationSection)input.Result;
}Посмотрим на свойство LastLocationInput:
internal SectionInput LastLocationInput
=> HasLocationInputs
? LocationInputs[LocationInputs.Count - 1]
: null;Оно написано по тому же принципу, что и LastIndirectLocationInput. Как и в предыдущем случае, в зависимости от флага (HasLocationInputs) возвращается или null, или значение из коллекции LocationInputs.
Возвращаемся к оператору if. Его условное выражение — свойство HasLocationInputs, которое проверяется и внутри LastLocationInput. Если зашли в тело if, значит LastLocationInput не может вернуть явный null. Может ли значение из коллекции LocationInputs быть null? Вопрос открытый. Если может, то и в input тоже будет записан null.
Как и в случае с первым рассмотренным if, выполняется проверка input.HasResult, а вот проверка input != null на этот раз отсутствует.
Ещё раз. Первый разобранный фрагмент кода:
SectionInput input = sectionRecord.LastIndirectLocationInput;
Debug.Assert(input != null);
Debug.Assert(input.HasResult, "input.HasResult");
result = (ConfigurationSection)input.Result;Второй:
SectionInput input = sectionRecord.LastLocationInput;
Debug.Assert(input.HasResult, "input.HasResult");
result = (ConfigurationSection)input.Result;Выглядит так, будто пропустили выражение Debug.Assert(input != null).
Я открыл issue на GitHub, где описал это и другие подозрительные места, связанные с null-проверками (их мы рассмотрим ниже).
Рассмотренный код исправлять не стали, оставив as is:
Issues c проверкой на null
В коде я встретил несколько мест, в которых ссылка сначала разыменовывается, а затем проверяется на null. Чтобы не плодить задачи, собрал их все в общем issue на GitHub.
Давайте разберём эти места.
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 (link)
Сначала pOperand1 разыменовывается (pOperand1.ErrorString), а уже на следующей строке проверяется на null в Debug.Assert. Если pOperand1 будет null, вместо срабатывания ассерта будет выброшено исключение типа NullReferenceException.
Код поправили, вынеся проверку pOperand1 до использования.
Было:
string strOp = pOperand1.ErrorString;
Debug.Assert(pOperand1 != null);
Debug.Assert(pOperand1.Type != null);Стало:
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 (link)
Переменную _callbacks сначала используют, а затем проверяют её значение на неравенство null:
public void Execute()
{
var count = _callbacks.Count;
....
if (_callbacks != null)
....
}На момент написания статьи код исправили, убрав проверку _callbacks на null.
Кстати, _callbacks – это readonly поле, которое инициализируется в конструкторе:
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;
}
....
}В треде с исправлениями обсуждали, стоит ли добавить в конструктор Debug.Assert с проверкой _callbacks на null. В итоге решили не делать.
Issue 11
private void ValidateAttributes(XmlElement elementNode)
{
....
XmlSchemaAttribute schemaAttribute
= (_defaultAttributes[i] as XmlSchemaAttribute)!;
attrQName = schemaAttribute.QualifiedName;
Debug.Assert(schemaAttribute != null);
....
}Location: DocumentSchemaValidator.cs, 421 (link)
Противоречивый код:
Внимание, вопрос. Может ли schemaAttribute иметь значение null? Из этого кода, конечно, не очень понятно.
Исправили код так:
....
XmlSchemaAttribute schemaAttribute
= (XmlSchemaAttribute)_defaultAttributes[i]!;
attrQName = schemaAttribute.QualifiedName;
....В обсуждении правок предлагали не удалять вызов Debug.Assert, а перенести его строкой выше. Код выглядел бы так:
....
XmlSchemaAttribute schemaAttribute = (XmlSchemaAttribute)_defaultAttributes[i]!;
Debug.Assert(schemaAttribute != null);
attrQName = schemaAttribute.QualifiedName;
....В итоге Assert решили не возвращать:
Issue 12
Взглянем на конструктор типа XmlConfigurationElementTextContent:
public XmlConfigurationElementTextContent(string textContent,
int? linePosition,
int? lineNumber)
{ .... }Location: XmlConfigurationElementTextContent.cs, 10 (link)
Теперь посмотрим, где он используется:
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:
Заметили подвох в коде?
Обратите внимание на порядок аргументов и параметров:
Я открыл issue на GitHub (link), код поправили: аргументы поменяли местами и добавили тест.
Issue 13
Ещё один подозрительный случай:
public virtual bool Nested
{
get {....}
set
{
....
ForeignKeyConstraint? constraint
= ChildTable.Constraints
.FindForeignKeyConstraint(ChildKey.ColumnsReference,
ParentKey.ColumnsReference);
....
}
}Location: DataRelation.cs, 486 (link)
Посмотрим на метод FindForeignKeyConstraint:
internal ForeignKeyConstraint?
FindForeignKeyConstraint(DataColumn[] parentColumns,
DataColumn[] childColumns)
{ .... }Location: ConstraintCollection.cs, 548 (link)
Похоже, порядок аргументов опять неверный:
Есть ещё один вызов этого метода: там к порядку аргументов нет вопросов.
ForeignKeyConstraint? foreignKey
= relation.ChildTable
.Constraints
.FindForeignKeyConstraint(relation.ParentColumnsReference,
relation.ChildColumnsReference);Открыл issue на GitHub: link. Увы, на момент написания статьи я не получил комментариев по этому коду.
Issue 14
Это не все места, где порядок аргументов перепутан — нашёл ещё одно:
void RecurseChildren(....)
{
....
string? value
= processValue != null
? processValue(new ConfigurationDebugViewContext(
child.Key,
child.Path,
valueAndProvider.Value,
valueAndProvider.Provider))
: valueAndProvider.Value;
....
}Location: ConfigurationRootExtensions.cs, 50 (link)
Посмотрим на конструктор ConfigurationDebugViewContext:
public ConfigurationDebugViewContext(
string path,
string key,
string? value,
IConfigurationProvider configurationProvider)
{ .... }Location: ConfigurationDebugViewContext.cs, 11 (link)
Последовательность:
Открыл issue на GitHub: link. Со слов разработчиков, несмотря на ошибку, этот кейс не вызывал проблем.
Тем не менее, порядок аргументов изменили на правильный.
Заключение
Код .NET 7 высокого качества. Я думаю, этому способствует в том числе налаженный процесс разработки: даты релизов известны, выпуски RC тоже проводятся не зря.
Тем не менее, каждый раз в исходниках получается найти что-то интересное. В этот раз фаворитами для меня стали аргументы, перепутанные при вызовах методов.
Все фрагменты кода, описанные в статье, я нашёл анализатором PVS-Studio. Да, теперь им можно проверять проекты на .NET 7.
Если захотите проверить свой проект (личный или коммерческий), анализатор можете загрузить с этой страницы. Там же есть ссылка на документацию: в ней мы описали, как ввести лицензию и запустить анализ. Если возникнут проблемы или вопросы — обязательно пишите нам, поможем.