Сергей Васильев

27 Дек 2021

Теги:

#CSharp

Ошибки и подозрительные места в исходниках .NET 6

27 Дек 2021

Автор: Сергей Васильев

Про проверку
Подозрительные места
Заключение

Релиз .NET 6 вышел достаточно громким – если вы пишете под эту платформу, то навряд ли могли пропустить такое событие. Мы тоже не смогли пройти мимо и решили проверить, что интересного удастся найти в исходном коде библиотек .NET.

Про проверку

Исходники я взял из ветки релиза .NET 6 на GitHub. В статье приводятся подозрительные места только из состава библиотек (то, что лежит в src/libraries). Сам рантайм не анализировал – может быть, в следующий раз. :)

Код проверял с помощью статического анализатора PVS-Studio. Кстати, как вы могли догадаться из этой статьи, PVS-Studio 7.16 поддерживает анализ проектов под .NET 6. О том, что ещё нового появилось в этом релизе, можно почитать здесь. К слову, сам анализатор PVS-Studio C#, если говорить про версии под Linux и macOS, теперь тоже работает под .NET 6.

К слову, кроме поддержки .NET 6 в C# анализаторе за год появилось много нового: были добавлены security-диагностики, плагин для Visual Studio 2022, сам анализатор стал работать значительно быстрее на крупных проектах.

Но вы же пришли сюда почитать про .NET 6, верно? Что ж, тогда не будем терять времени.

Подозрительные места

Разное

В этом разделе я собрал различные интересные места, которые не смог объединить какой-то общей категорией.

Issue 1

Начнём с чего-нибудь попроще.

public enum CompressionLevel
{
  Optimal,
  Fastest,
  NoCompression,
  SmallestSize
}

internal static void GetZipCompressionMethodFromOpcCompressionOption(
  CompressionOption compressionOption,
  out CompressionLevel compressionLevel)
{
  switch (compressionOption)
  {
    case CompressionOption.NotCompressed:
      {
        compressionLevel = CompressionLevel.NoCompression;
      }
      break;
    case CompressionOption.Normal:
      {
        compressionLevel = CompressionLevel.Optimal;  // <=
      }
      break;
    case CompressionOption.Maximum:
      {
        compressionLevel = CompressionLevel.Optimal;  // <=
      }
      break;
    case CompressionOption.Fast:
      {
        compressionLevel = CompressionLevel.Fastest;
      }
      break;
    case CompressionOption.SuperFast:
      {
        compressionLevel = CompressionLevel.Fastest;
      }
      break;

    // fall-through is not allowed
    default:
      {
        Debug.Fail("Encountered an invalid CompressionOption enum value");
        goto case CompressionOption.NotCompressed;
      }
  }
}

Предупреждение PVS-Studio: V3139 Two or more case-branches perform the same actions. ZipPackage.cs 402

Фактически данный метод осуществляет маппинг с CompressionOption на CompressionLevel. Подозрительно здесь то, что значения CompressionOption.Normal и CompressionOption.Maximum маппятся на значение CompressionLevel.Optimal.

Возможно, CompressionOption.Maximum должен соответствовать CompressionLevel.SmallestSize.

Issue 2

Теперь давайте немного попрактикуемся. Для экспериментов возьмём тип System.Text.Json.Nodes.JsonObject. При желании вы можете повторить описанные операции, если возьмёте релизную версию .NET 6 SDK.

У типа JsonObject есть 2 конструктора – один принимает только опции, другой – свойства и опции. В принципе, понятно, чего от них ждать. Документация доступна здесь.

Создадим два экземпляра типа JsonObject, использовав каждый из конструкторов.

static void JsonObject_Test()
{
  var properties = new Dictionary<String, JsonNode?>();
  var options = new JsonNodeOptions()
  {
    PropertyNameCaseInsensitive = true
  };

  var jsonObject1 = new JsonObject(options);
  var jsonObject2 = new JsonObject(properties, options);
}

Теперь проверим состояние созданных объектов.

Если состояние jsonObject1 ожидаемое, то, почему в поле _options объекта jsonObject2 записано значение null, не очень понятно. Что ж, заглянем в исходный код и посмотрим на эти конструкторы.

public sealed partial class JsonObject : JsonNode
{
  ....
  public JsonObject(JsonNodeOptions? options = null) : base(options) { }

  public JsonObject(IEnumerable<KeyValuePair<string, JsonNode?>> properties, 
                    JsonNodeOptions? options = null)
  {
    foreach (KeyValuePair<string, JsonNode?> node in properties)
    {
      Add(node.Key, node.Value);
    }
  }
  ....
}

Во втором конструкторе параметр options попросту затерялся – он никуда не передаётся и никак не используется. В то время как в первом конструкторе options передаются в конструктор базового класса, где и записываются в поле:

internal JsonNode(JsonNodeOptions? options = null)
{
  _options = options;
}

Соответствующее предупреждение PVS-Studio: V3117 Constructor parameter 'options' is not used. JsonObject.cs 35

Issue 3

Кстати, если говорить про забытые параметры, нашлось ещё одно интересное место.

public class ServiceNameCollection : ReadOnlyCollectionBase
{
  ....
  private ServiceNameCollection(IList list, string serviceName)
    : this(list, additionalCapacity: 1)
  { .... }
  
  private ServiceNameCollection(IList list, IEnumerable serviceNames)
    : this(list, additionalCapacity: GetCountOrOne(serviceNames))
  { .... }

  private ServiceNameCollection(IList list, int additionalCapacity)
  {
    Debug.Assert(list != null);
    Debug.Assert(additionalCapacity >= 0);

    foreach (string? item in list)
    {
      InnerList.Add(item);
    }
  }
  ....
}

Предупреждение PVS-Studio: V3117 Constructor parameter 'additionalCapacity' is not used. ServiceNameCollection.cs 46

Как видно из кода, параметр additionalCapacity последнего конструктора проверяется в Debug.Assert, но больше ни для чего не используется. Выглядит подозрительно. Особенно забавно, что этот параметр вполне себе используется другими конструкторами, которые передают какие-то значения для additionalCapacity.

Issue 4

Тест на способность предвидения (упс, спойлеры). Попробуйте угадать, что насторожило анализатор в следующем коде.

public override void CheckErrors()
{
  throw new XsltException(SR.Xslt_InvalidXPath, 
                          new string[] { Expression }, 
                          _baseUri, 
                          _linePosition, 
                          _lineNumber, 
                          null);
}

Казалось бы, просто выбрасывается исключение. Чтобы понять, что здесь не так, нужно посмотреть на конструктор XsltException.

internal XsltException(string res, 
                       string?[] args, 
                       string? sourceUri, 
                       int lineNumber, 
                       int linePosition, 
                       Exception? inner) : base(....)
{ .... }

Если сопоставить порядок аргументов и параметров, становится понятно, что насторожило анализатор. Похоже, что позиция и номер строки перепутаны местами.

Порядок аргументов:

_linePosition
_lineNumber

Порядок параметров:

lineNumber
linePosition

Предупреждение PVS-Studio: V3066 Possible incorrect order of arguments passed to 'XsltException' constructor: '_linePosition' and '_lineNumber'. Compiler.cs 1187

Issue 5

Ниже привожу достаточно объёмный фрагмент кода. Наверняка там запряталась какая-то опечатка... Не хотите попробовать её найти?

public Parser(Compilation compilation, 
              in JsonSourceGenerationContext sourceGenerationContext)
{
  _compilation = compilation;
  _sourceGenerationContext = sourceGenerationContext;
  _metadataLoadContext = new MetadataLoadContextInternal(_compilation);

  _ilistOfTType = _metadataLoadContext.Resolve(
    SpecialType.System_Collections_Generic_IList_T);
  _icollectionOfTType = _metadataLoadContext.Resolve(
    SpecialType.System_Collections_Generic_ICollection_T);
  _ienumerableOfTType = _metadataLoadContext.Resolve(
    SpecialType.System_Collections_Generic_IEnumerable_T);
  _ienumerableType = _metadataLoadContext.Resolve(
    SpecialType.System_Collections_IEnumerable);

  _listOfTType = _metadataLoadContext.Resolve(typeof(List<>));
  _dictionaryType = _metadataLoadContext.Resolve(typeof(Dictionary<,>));
  _idictionaryOfTKeyTValueType = _metadataLoadContext.Resolve(
    typeof(IDictionary<,>));
  _ireadonlyDictionaryType = _metadataLoadContext.Resolve(
    typeof(IReadOnlyDictionary<,>));
  _isetType = _metadataLoadContext.Resolve(typeof(ISet<>));
  _stackOfTType = _metadataLoadContext.Resolve(typeof(Stack<>));
  _queueOfTType = _metadataLoadContext.Resolve(typeof(Queue<>));
  _concurrentStackType = _metadataLoadContext.Resolve(
    typeof(ConcurrentStack<>));
  _concurrentQueueType = _metadataLoadContext.Resolve(
    typeof(ConcurrentQueue<>));
  _idictionaryType = _metadataLoadContext.Resolve(typeof(IDictionary));
  _ilistType = _metadataLoadContext.Resolve(typeof(IList));
  _stackType = _metadataLoadContext.Resolve(typeof(Stack));
  _queueType = _metadataLoadContext.Resolve(typeof(Queue));
  _keyValuePair = _metadataLoadContext.Resolve(typeof(KeyValuePair<,>));

  _booleanType = _metadataLoadContext.Resolve(SpecialType.System_Boolean);
  _charType = _metadataLoadContext.Resolve(SpecialType.System_Char);
  _dateTimeType = _metadataLoadContext.Resolve(SpecialType.System_DateTime);
  _nullableOfTType = _metadataLoadContext.Resolve(
    SpecialType.System_Nullable_T);
  _objectType = _metadataLoadContext.Resolve(SpecialType.System_Object);
  _stringType = _metadataLoadContext.Resolve(SpecialType.System_String);

  _dateTimeOffsetType = _metadataLoadContext.Resolve(typeof(DateTimeOffset));
  _byteArrayType = _metadataLoadContext.Resolve(
    typeof(byte)).MakeArrayType();
  _guidType = _metadataLoadContext.Resolve(typeof(Guid));
  _uriType = _metadataLoadContext.Resolve(typeof(Uri));
  _versionType = _metadataLoadContext.Resolve(typeof(Version));
  _jsonArrayType = _metadataLoadContext.Resolve(JsonArrayFullName);
  _jsonElementType = _metadataLoadContext.Resolve(JsonElementFullName);
  _jsonNodeType = _metadataLoadContext.Resolve(JsonNodeFullName);
  _jsonObjectType = _metadataLoadContext.Resolve(JsonObjectFullName);
  _jsonValueType = _metadataLoadContext.Resolve(JsonValueFullName);

  // Unsupported types.
  _typeType = _metadataLoadContext.Resolve(typeof(Type));
  _serializationInfoType = _metadataLoadContext.Resolve(
    typeof(Runtime.Serialization.SerializationInfo));
  _intPtrType = _metadataLoadContext.Resolve(typeof(IntPtr));
  _uIntPtrType = _metadataLoadContext.Resolve(typeof(UIntPtr));
  _iAsyncEnumerableGenericType = _metadataLoadContext.Resolve(
    IAsyncEnumerableFullName);
  _dateOnlyType = _metadataLoadContext.Resolve(DateOnlyFullName);
  _timeOnlyType = _metadataLoadContext.Resolve(TimeOnlyFullName);

  _jsonConverterOfTType = _metadataLoadContext.Resolve(
    JsonConverterOfTFullName);

  PopulateKnownTypes();
}

Ну что, как успехи? А может здесь и вовсе нет никакой опечатки?

Давайте для начала взглянем на предупреждение анализатора: V3080 Possible null dereference of method return value. Consider inspecting: Resolve(...). JsonSourceGenerator.Parser.cs 203

Метод Resolve может вернуть значение null. Об этом говорит в том числе и его сигнатура. Об этом же предупреждает и PVS-Studio, межпроцедурно обнаружив возможность возврата значения null.

public Type? Resolve(Type type)
{
  Debug.Assert(!type.IsArray, 
               "Resolution logic only capable of handling named types.");
  return Resolve(type.FullName!);
}

Спускаемся ниже, в другую перегрузку Resolve.

public Type? Resolve(string fullyQualifiedMetadataName)
{
  INamedTypeSymbol? typeSymbol = 
    _compilation.GetBestTypeByMetadataName(fullyQualifiedMetadataName);
  return typeSymbol.AsType(this);
}

Обратите внимание, что typeSymbol записан как допускающий null-значение: INamedTypeSymbol?. Опускаемся ниже – в метод AsType.

public static Type AsType(this ITypeSymbol typeSymbol, 
                          MetadataLoadContextInternal metadataLoadContext)
{
  if (typeSymbol == null)
  {
    return null;
  }

  return new TypeWrapper(typeSymbol, metadataLoadContext);
}

Как видно, если первый аргумент – нулевая ссылка, то и из метода возвращается значение null.

А теперь вернёмся в конструктор типа Parser. В нём обычно результат вызова метода Resolve просто записывается в какое-то поле. Но обнаружилось исключение, о котором и предупреждает PVS-Studio:

_byteArrayType = _metadataLoadContext.Resolve(typeof(byte)).MakeArrayType();

Здесь для результата вызова метода Resolve будет вызван экземплярный метод MakeArrayType. Как следствие, если Resolve вернёт null, возникнет исключение NullReferenceException.

Issue 6

public abstract partial class Instrument<T> : Instrument where T : struct
{
  [ThreadStatic] private KeyValuePair<string, object?>[] ts_tags;
  ....
}

Предупреждение PVS-Studio: V3079 'ThreadStatic' attribute is applied to a non-static 'ts_tags' field and will be ignored Instrument.netfx.cs 20

Процитируем документацию: Note that in addition to applying the ThreadStaticAttribute attribute to a field, you must also define it as a static field (in C#) or a Shared field (in Visual Basic).

Как видно из кода, поле ts_tags – экземплярное. Следовательно, вешать на него атрибут ThreadStatic нет смысла. Или же здесь творится какая-то чёрная магия...

Issue 7

private static JsonSourceGenerationOptionsAttribute? 
GetSerializerOptions(AttributeSyntax? attributeSyntax)
{
  ....
  foreach (AttributeArgumentSyntax node in attributeArguments)
  {
    IEnumerable<SyntaxNode> childNodes = node.ChildNodes();
    NameEqualsSyntax? propertyNameNode 
      = childNodes.First() as NameEqualsSyntax;
    Debug.Assert(propertyNameNode != null); 

    SyntaxNode? propertyValueNode = childNodes.ElementAtOrDefault(1);
    string propertyValueStr = propertyValueNode.GetLastToken().ValueText;
    ....
  }
  ....
}

Предупреждение PVS-Studio: V3146 Possible null dereference of 'propertyValueNode'. The 'childNodes.ElementAtOrDefault' can return default null value. JsonSourceGenerator.Parser.cs 560

Если в коллекции childNodes меньше двух элементов, вызов ElementAtOrDefault вернёт значение default(SyntaxNode) (то есть null, так как SyntaxNode – класс). В таком случае на следующей строке будет выброшено исключение NullReferenceException. Особенно странно, что propertyValueNode – nullable reference type, однако разыменовывается без проверки.

Возможно, здесь есть какой-то неявный контракт на то, что в childNodes всегда больше одного элемента. Например, что если есть propertyNameNode, то будет и propertyValueNode. В таком случае можно было бы использовать вызов метода ElementAt, чтобы не было лишних вопросов.

Issue 8

Есть такая структура – Microsoft.Extensions.FileSystemGlobbing.FilePatternMatch. Она, что логично, переопределяет метод Equals(Object). Документация с описанием метода.

Допустим, у нас есть такой код, который вызывает этот метод:

static void FPM_Test(Object? obj)
{
  FilePatternMatch fpm = new FilePatternMatch();
  var eq = fpm.Equals(obj);
}

Как вы думаете, что произойдёт, если FPM_Test вызвать со значением null? В переменную eq будет записано значение false? Ну, почти.

Похожая история будет, если мы передадим в качестве аргумента экземпляр типа, отличного от FilePatternMatch. Например... Пусть будет массив какой-нибудь.

Уже догадались, почему так происходит? Дело в том, что в методе Equals аргумент никак не проверяется ни на null-значение, ни на совместимость типов, а просто безусловно распаковывается:

public override bool Equals(object obj)
{
  return Equals((FilePatternMatch) obj);
}

Предупреждение PVS-Studio: V3115 Passing 'null' to 'Equals' method should not result in 'NullReferenceException'. FilePatternMatch.cs 61

Конечно, если судить по документации, никто не обещал нам, что Equals(Object) будет возвращать false, если принимает не FilePatternMatch. Но, наверное, это было бы наиболее ожидаемым поведением.

Дублирующиеся проверки

С дублирующимися проверками есть интересный момент: со стороны не всегда может быть понятно — просто это избыточный код или вместо одной из них должно быть что-то иное. Так или иначе, предлагаю взглянуть на несколько примеров.

Issue 9

internal DeflateManagedStream(Stream stream, 
                              ZipArchiveEntry.CompressionMethodValues method, 
                              long uncompressedSize = -1)
{
  if (stream == null)
    throw new ArgumentNullException(nameof(stream));
  if (!stream.CanRead)
    throw new ArgumentException(SR.NotSupported_UnreadableStream, 
                                nameof(stream));
  if (!stream.CanRead)
    throw new ArgumentException(SR.NotSupported_UnreadableStream, 
                                nameof(stream));

  Debug.Assert(method == ZipArchiveEntry.CompressionMethodValues.Deflate64);

  _inflater 
    = new InflaterManaged(
        method == ZipArchiveEntry.CompressionMethodValues.Deflate64, 
        uncompressedSize);

  _stream = stream;
  _buffer = new byte[DefaultBufferSize];
}

Предупреждение PVS-Studio: V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless DeflateManagedStream.cs 27

В начале метода идёт ряд проверок. Но, вот незадача, одна из них (!stream.CanRead) полностью продублирована (как условие, так и then-ветвь оператора if).

Issue 10

public static object? Deserialize(ReadOnlySpan<char> json, 
                                  Type returnType, 
                                  JsonSerializerOptions? options = null)
{
  // default/null span is treated as empty
  if (returnType == null)
  {
    throw new ArgumentNullException(nameof(returnType));
  }

  if (returnType == null)
  {
    throw new ArgumentNullException(nameof(returnType));
  }

  JsonTypeInfo jsonTypeInfo = GetTypeInfo(options, returnType);
  return ReadFromSpan<object?>(json, jsonTypeInfo)!;
}

Ага, похожая ситуация, но уже совсем в другом месте. Перед использованием проверяют параметр returnType на null. Дело хорошее, вот только делают это 2 раза.

Issue 11

private void WriteQualifiedNameElement(....)
{
  bool hasDefault = defaultValue != null && defaultValue != DBNull.Value;
  if (hasDefault)
  {
    throw Globals.NotSupported(
      "XmlQualifiedName DefaultValue not supported.  Fail in WriteValue()");
  }
  ....
  if (hasDefault)
  {
    throw Globals.NotSupported(
      "XmlQualifiedName DefaultValue not supported.  Fail in WriteValue()");
  }
}

Здесь ситуация немного интереснее. Если предыдущие дублирующиеся проверки следовали одна за другой, то здесь они расположены на разных концах метода – почти в 20 строках друг от друга. Однако проверяемая локальная переменная hasDefault за это время не изменяется. Соответственно, либо исключение будет выброшено уже в первый раз, либо так и не будет выброшено вовсе.

Issue 12

internal static bool AutoGenerated(ForeignKeyConstraint fk, bool checkRelation)
{
  ....

  if (fk.ExtendedProperties.Count > 0)
    return false;


  if (fk.AcceptRejectRule != AcceptRejectRule.None)
    return false;
  if (fk.DeleteRule != Rule.Cascade)  // <=
    return false;
  if (fk.DeleteRule != Rule.Cascade)  // <=
    return false;

  if (fk.RelatedColumnsReference.Length != 1)
    return false;
  return AutoGenerated(fk.RelatedColumnsReference[0]);
}

Предупреждение PVS-Studio: V3022 Expression 'fk.DeleteRule != Rule.Cascade' is always false. xmlsaver.cs 1708

Вопрос традиционный – нужно было проверять другое значение или это просто избыточный код?

Пропущенная интерполяция

Сначала посмотрим на пару найденных предупреждений, а затем я расскажу небольшую историю.

Issue 13

internal void SetLimit(int physicalMemoryLimitPercentage)
{
  if (physicalMemoryLimitPercentage == 0)
  {
    // use defaults
    return;
  }
  _pressureHigh = Math.Max(3, physicalMemoryLimitPercentage);
  _pressureLow = Math.Max(1, _pressureHigh - 9);
  Dbg.Trace($"MemoryCacheStats", 
            "PhysicalMemoryMonitor.SetLimit: 
              _pressureHigh={_pressureHigh}, _pressureLow={_pressureLow}");
}

Предупреждение PVS-Studio: V3138 String literal contains potential interpolated expression. Consider inspecting: _pressureHigh. PhysicalMemoryMonitor.cs 110

Очень похоже, что здесь хотели залогировать поля _pressureHigh и _pressureLow. Однако подстановка значений не сработает, так как строка не интерполирована. Зато символ интерполяции стоит на первом аргументе метода Dbg.Trace, где подставлять нечего. :)

Issue 14

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

Предупреждение PVS-Studio: V3138 String literal contains potential interpolated expression. Consider inspecting: spec. MetricsEventSource.cs 381

В коде пытаются распарсить строку specString. Если не получается – нужно залогировать исходную строку, если получается – залогировать результат (переменная spec) и выполнить ещё какие-то операции.

Проблема опять в том, что и в первом, и во втором случае пропущен символ интерполяции. Как следствие – значения переменных specString и spec подставлены не будут.

А теперь обещанная история.

Ранее я уже писал о том, что в 2019 году проверял библиотеки .NET Core. Тогда тоже нашлись несколько строк, которые должны были быть интерполированными, но не были таковыми из-за пропущенного символа '$'. Соответствующие предупреждения в той статье описаны как issue 10 и issue 11.

Я отписал баг-репорт на GitHub, после чего команда разработки .NET поправила некоторые описанные в статье места. В их числе – ошибки с интерполированными строками. Соответствующий pull request.

Более того – в issue-трекере Roslyn Analyzers была открыта задача по разработке новой диагностики, которая отлавливала бы подобные кейсы.

Немного более подробно вся эта история изложена здесь.

Возвращаемся в текущее время. Я всё это знал и помнил, поэтому очень удивился, когда вновь наткнулся на ошибки с пропущенной интерполяцией. Как же так? Ведь уже "из коробки" должна быть доступна соответствующая диагностика, которая помогла бы избежать этих ошибок.

Я решил проверить тот issue по разработке диагностики от 15 августа 2019 и оказалось... что диагностика-то ещё не готова. Вот и ответ на вопрос, откуда снова ошибки с интерполяцией.

PVS-Studio обнаруживает подобные проблемы с релиза 7.03 (25 июня 2019) – пользуйтесь. ;)

Что-то меняется, что-то остаётся

В ходе проверки я несколько раз натыкался на предупреждения, показавшиеся мне смутно знакомыми. Оказалось, что я уже описывал их в прошлый раз. Раз они так и остались в коде, предположу, что это были не ошибки.

Например, приведённый ниже код, похоже, действительно необычный способ выбросить ArgumentOutOfRangeException. Это issue 30 из прошлой проверки.

private ArrayList? _tables;
private DataTable? GetTable(string tableName, string ns)
{
  if (_tables == null)
    return _dataSet!.Tables.GetTable(tableName, ns);

  if (_tables.Count == 0)
    return (DataTable?)_tables[0];
  ....
}

Однако у меня есть несколько вопросов по поводу других фрагментов, уже обнаруженных ранее. Например, issue 25. В цикле обходят коллекцию seq, но постоянно обращаются только к её первому элементу – seq[0]. Это выглядит... необычно.

public bool MatchesXmlType(IList<XPathItem> seq, int indexType)
{
  XmlQueryType typBase = GetXmlType(indexType);

  XmlQueryCardinality card = seq.Count switch
  {
    0 => XmlQueryCardinality.Zero,
    1 => XmlQueryCardinality.One,
    _ => XmlQueryCardinality.More,
  };

  if (!(card <= typBase.Cardinality))
    return false;

  typBase = typBase.Prime;
  for (int i = 0; i < seq.Count; i++)
  {
    if (!CreateXmlType(seq[0]).IsSubtypeOf(typBase)) // <=
      return false;
  }

  return true;
}

Предупреждение PVS-Studio: V3102 Suspicious access to element of 'seq' object by a constant index inside a loop. XmlQueryRuntime.cs 729

Меня такой код немного сбивает с толку, а вас?

Или вот возьмём issue 34.

public bool Remove(out int testPosition, out MaskedTextResultHint resultHint)
{
  ....
  if (lastAssignedPos == INVALID_INDEX)
  {
    ....
    return true; // nothing to remove.
  }
  ....

  return true;
}

Предупреждение PVS-Studio: V3009 It's odd that this method always returns one and the same value of 'true'. MaskedTextProvider.cs 1531

Метод как раньше всегда возвращал true, так и сейчас. При этом в комментарии всё также указано, что метод может вернуть и false: Returns true on success, false otherwise. С документацией та же история.

Следующий пример, который также был описан ещё в предыдущей статье, я даже вынесу в отдельный раздел. Мы немного порассуждаем не только над самим фрагментом кода, но и над используемой в нём фишкой – nullable reference types.

И снова про nullable reference types

В общем и целом, я пока ещё не понял, нравятся мне ссылочные типы, допускающие null, или нет.

С одной стороны – огромное их преимущество в более информативной сигнатуре методов. Одного взгляда достаточно, чтобы понять, может ли метод возвращать null, может ли определённый параметр иметь null-значение и т.п.

С другой стороны – всё это строится на доверии. Никто не запрещает написать код вот так:

static String GetStr()
{
  return null!;
}

static void Main(string[] args)
{
  String str = GetStr();
  Console.WriteLine(str.Length); // NRE, str - null
}

Да-да-да, это синтетика, но ведь можно так сделать! Если такой код написан у вас внутри компании – идём (условно) к автору GetStr и проводим беседу. Однако, если GetStr подтягивается из какой-то библиотеки, исходников которой у вас нет, сюрприз будет не то чтобы очень приятным.

Вернёмся от синтетических примеров к нашей основной теме – .NET 6. И с ним есть нюансы. Например, разные библиотеки разбиты по разным solution. И я, просматривая их, не раз задавался вопросом – а включен ли nullable context в данном проекте? А вот то, что здесь нет проверки на null – это ожидаемо или нет? Наверное, это не проблема при работе в рамках одного проекта, но при беглом анализе всех создаёт определённые трудности.

Дальше – больше. Разного рода штуки начинаются, когда происходит переход на nullable context. Вроде переменная и не может иметь значения null, а вроде и проверка есть. И в .NET таких мест, скажем прямо, немало. Давайте я покажу несколько.

private void ValidateAttributes(XmlElement elementNode)
{
  ....
  XmlSchemaAttribute schemaAttribute 
    = (_defaultAttributes[i] as XmlSchemaAttribute)!;
  attrQName = schemaAttribute.QualifiedName;
  Debug.Assert(schemaAttribute != null);
  ....
}

Предупреждение PVS-Studio: V3095 The 'schemaAttribute' object was used before it was verified against null. Check lines: 438, 439. DocumentSchemaValidator.cs 438

Символ '!' намекает, что здесь мы работаем с nullable-контекстом. Окей.

1. Почему для приведения используется оператор 'as', а не прямой каст? Если есть уверенность в том, что schemaAttribute не null (я так читаю неявный контракт с '!'), значит, _defaultAttributes[i] точно имеет тип XmlSchemaAttribute. Ну допустим, такой синтаксис нравится больше – ладно.

2. Если schemaAttribute не null, зачем ниже в Debug.Assert проверка на null?

3. Если проверка актуальна и schemaAttribute всё же может иметь значение null (вопреки семантике nullable reference types), то до Debug.Assert исполнение не дойдёт, так как будет выброшено исключение при обращении к schemaAttribute.QualifiedName.

Такой небольшой фрагмент кода лично у меня порождает сразу кучу вопросов.

Похожая история:

public Node DeepClone(int count)
{
  ....
  while (originalCurrent != null)
  {
    originalNodes.Push(originalCurrent);
    newNodes.Push(newCurrent);
    newCurrent.Left = originalCurrent.Left?.ShallowClone();
    originalCurrent = originalCurrent.Left;
    newCurrent = newCurrent.Left!;
  }
  ....
}

С одной стороны, newCurrent.Left может иметь значение null, так как в него записывается результат выполнения оператора ?. (originalCurrent.Left?.ShallowClone()). С другой стороны, в последней строке мы видим аннотацию, что newCurrent.Left не null.

А теперь тот фрагмент кода из .NET 6, ради которого я этот раздел, по факту, и начал писать. Реализация IStructuralEquatable.Equals(object? other, IEqualityComparer comparer) в типе ImmutableArray<T>.

internal readonly T[]? array;
bool IStructuralEquatable.Equals(object? other, IEqualityComparer comparer)
{
  var self = this;
  Array? otherArray = other as Array;
  if (otherArray == null)
  {
    if (other is IImmutableArray theirs)
    {
      otherArray = theirs.Array;

      if (self.array == null && otherArray == null)
      {
        return true;
      }
      else if (self.array == null)
      {
        return false;
      }
    }
  }

  IStructuralEquatable ours = self.array!;
  return ours.Equals(otherArray, comparer);
}

Если взглянуть на последние строчки кода в Visual Studio, редактор услужливо подскажет, что ours не null. Оно и видно из кода – self.array также размечен как не хранящий null-значения.

Хорошо, давайте напишем такой код:

IStructuralEquatable immutableArr = default(ImmutableArray<String>);
var eq = immutableArr.Equals(null, EqualityComparer<String>.Default);

Запускаем его на исполнение и видим NullReferenceException.

Уууупс. Похоже, что переменная ours, которая не null, по факту всё-таки оказалось нулевой ссылкой.

Давайте разберём, как так получилось:

Поле array объекта immutableArr принимает дефолтное значение null.
other имеет значение null, значит otherArray также имеет значение null.
Проверка other is ImmutableArray даёт false.
На момент записи значения в ours поле self.array имеет значение null.
Дальше вы знаете.

Здесь можно контраргументировать тем, что иммутабельный массив находится в некорректном состоянии, так как был создан не через специальные методы/свойства, а через вызов оператора default. Но получить NRE на вызове Equals для такого объекта всё равно немного странно.

Однако дело даже не в этом. Из кода, аннотаций и подсказок следует, что ours не null. По факту же переменная имеет значение null. Лично для меня это немного подрывает доверие к nullable reference types.

PVS-Studio выдаёт здесь предупреждение: V3125 The 'ours' object was used after it was verified against null. Check lines: 1144, 1136. ImmutableArray_1.cs 1144

Кстати, об этой проблеме я писал в прошлой статье (issue 53). Тогда, правда, здесь ещё не было nullable-разметки.

Примечание. Возвращаясь к разговору про операции над экземплярами ImmutableArray<T> в дефолтном состоянии, в некоторых методах/свойствах используются специальные методы: ThrowNullRefIfNotInitialized и ThrowInvalidOperationIfNotInitialized, которые сообщают о непроинициализированном состоянии объекта. Причём в явных реализациях интерфейсных методов используется ThrowInvalidOperationIfNotInitialized. Возможно, он должен был использоваться и в описанном выше случае.

Тут хочу обратиться к читателям – а какой у вас опыт работы с nullable reference types? Нравится, не нравится? Внедряли ли на своих проектах? Что пошло нормально, какие трудности были? Интересен ваш опыт.

Кстати, про nullable reference types уже была пара статей от моих коллег: раз, два. Время идёт, но вопрос всё ещё остаётся дискуссионным.

Заключение

В заключение я хотел бы ещё раз поздравить команду разработки .NET 6 с релизом, а также сказать спасибо всем тем, кто вносит вклад в развитие этого проекта. Уверен, что недочёты будут исправлены, а впереди ещё много свершений.

Надеюсь также, что мне удалось ещё раз напомнить про пользу статического анализа в процессе разработки. Если интересно – можете попробовать PVS-Studio и на своём проекте. Кстати, при переходе по этой ссылке будет доступна расширенная лицензия, которая действует 30 дней, а не 7. Это ли не повод? ;)

И по доброй традиции приглашаю подписываться на мой Twitter, чтобы не пропустить ничего интересного.

#CSharp