# Проверяем исходный код Roslyn

Время от времени мы возвращаемся к проектам, которые уже проверяли ранее с помощью PVS\-Studio и писали про это статьи\. Делать это интересно по двум причинам\. Во\-первых, чтобы понять, насколько лучше стал наш анализатор\. Во\-вторых, чтобы отследить, обратили ли авторы проекта внимание на нашу статью, а также на отчет об ошибках, который мы им обычно предоставляем\. Конечно, ошибки могут быть исправлены и без нашего участия\. Но всегда приятно, когда именно наши усилия помогают сделать какой\-то проект лучше\. Не стал исключением и Roslyn\. Предыдущая статья о проверке этого проекта датируется 23 декабря 2015 года\. Это довольно давно, учитывая путь, который за это время проделал в своём развитии наш анализатор\. Дополнительный интерес лично для нас Roslyn представляет ещё и тем, что на нём базируется ядро C\# анализатора PVS\-Studio\. Таким образом, мы очень заинтересованы в качестве кода этого проекта\. Устроим ему повторную проверку и выясним, что же нового и интересного \(но будем надеяться, что ничего существенного\) там сможет найти PVS\-Studio\.

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

Roslyn \(или \.NET Compiler Platform\) наверняка знаком многим из наших читателей\. Если кратко, то это набор компиляторов с открытым исходным кодом и API для анализа кода для языков C\# и Visual Basic \.NET от Microsoft\. Исходный код проекта доступен на [GitHub](https://github.com/dotnet/roslyn)\. 

Не буду приводить подробного описания этой платформы, а всем интересующимся рекомендую статью моего коллеги Сергея Васильева "[Введение в Roslyn\. Использование для разработки инструментов статического анализа](https://pvs-studio.ru/ru/blog/posts/csharp/0399/)"\. Из этой статьи можно узнать не только об особенностях архитектуры Roslyn, но и как именно мы используем эту платформу\.

Как я уже упоминал ранее, с момента написания последней статьи моего коллеги Андрея Карпова о проверке Roslyn "[Новогодний релиз PVS\-Studio 6\.00: проверяем Roslyn](https://pvs-studio.ru/ru/blog/posts/csharp/0363/)" прошло более трёх лет\. За это время C\# анализатор PVS\-Studio обзавёлся многими новыми возможностями\. Вообще, статья Андрея была неким "пробным шаром", ведь C\# анализатор тогда только был добавлен в PVS\-Studio\. Несмотря на это, уже тогда в безусловно качественном проекте Roslyn удалось найти интересные ошибки\. Что же изменилось в анализаторе для C\# кода к настоящему времени, что потенциально позволит провести более глубокий анализ?

За прошедшее время развивались как ядро анализатора, так и инфраструктура\. Была добавлена поддержка Visual Studio 2017 и Roslyn 2\.0, а также осуществлена глубокая интеграция с MSBuild\. Подробнее про наш подход к интеграции с MSBuild и про причины, которые побудили нас к его принятию, можно прочитать в статье моего коллеги Павла Еремеева "[Поддержка Visual Studio 2017 и Roslyn 2\.0 в PVS\-Studio: иногда использовать готовые решения не так просто, как кажется на первый взгляд](https://pvs-studio.ru/ru/blog/posts/csharp/0503/)"\.

Сейчас мы активно работаем над переходом на Roslyn 3\.0 по той же схеме, по которой изначально поддержали Visual Studio 2017, то есть через свой собственный toolset, идущий в дистрибутиве PVS\-Studio c "заглушкой" в виде пустого файла MSBuild\.exe\. Несмотря на то, что это выглядит как "костыль" \(API MSBuild не очень дружелюбен для переиспользования в third\-patry проектах из\-за невысокой портабельности библиотек\), такой подход уже помог нам относительно безболезненно пережить несколько обновлений Roslyn в течение жизни Visual Studio 2017, и сейчас, хотя и с большим количеством накладок, пережить обновление до Visual Studio 2019, а также сохранять полную обратную совместимость и работоспособность на системах с более старыми версиями MSBuild\.

Ядро анализатора также претерпело ряд улучшений\. Одно из главных нововведений \- полноценный межпроцедурный анализ с учётом входных и выходных значений методов, с расчётом, в зависимости от этих параметров, достижимости веток выполнения и точек возврата\. 

Уже близка к завершению задача отслеживания параметров внутри методов, с сохранением авто\-аннотаций для того, что с этими параметрами там происходит \(например, потенциально опасное разыменование\)\. Это позволит для любой диагностики, использующей механизм data\-flow, учитывать опасные ситуации, происходящие при передаче параметра в метод\. Раньше при анализе таких опасных мест предупреждение не генерировалось, так как мы не могли знать всех возможных входных значений в такой метод\. Сейчас же мы можем обнаружить опасность, так как во всех местах вызова этого метода эти входные параметры будут учтены\.

Замечание: вы можете ознакомиться с основными механизмами анализатора, такими как data\-flow и другими, из статьи "[Технологии, используемые в анализаторе кода PVS\-Studio для поиска ошибок и потенциальных уязвимостей](https://pvs-studio.ru/ru/blog/posts/cpp/0592/)"\.

Межпроцедурный анализ в PVS\-Studio C\# не ограничен ни входными параметрами, ни глубиной\. Единственное ограничение \- виртуальные методы в незакрытых для наследования классах и попадание в рекурсию \(остановимся, когда увидели в стеке повторный вызов уже рассчитываемого метода\)\. При этом сам рекурсивный метод в итоге будет посчитан с допущением, что возвращаемое значение его саморекурсии неизвестно\.

Другим большим нововведением в C\# анализаторе стал учёт возможности разыменования потенциально нулевого указателя\. Раньше анализатор ругался на возможный null reference exception, если был уверен, что во всех ветках выполнения значение переменной будет принимать null\. Конечно, он иногда ошибался, поэтому диагностика [V3080](https://pvs-studio.ru/ru/docs/warnings/v3080/) уже раньше называлась potential null reference\. 

Теперь анализатор запоминает, что переменная могла быть null в одной из веток выполнения \(например, при определённом условии в if\)\. Если он увидит доступ к такой переменной без проверки, то выдаст сообщение V3080, но на более низком уровне важности, чем если он видит null во всех ветках\. В сочетании с улучшенным межпроцедурным анализом, такой механизм позволяет находить очень сложно обнаруживаемые ошибки\. Пример \- длинная цепочка вызовов методов, последний из которых вам не знаком, и который, например, при определённых обстоятельствах в catch возвращает null, но вы не защитились от этого, так как этого просто не знали\. В данном случае анализатор ругается только тогда, когда точно видит назначение null\. На наш взгляд, это качественно отличает наш подход от такого нововведения C\# 8\.0, как nullable reference тип, которое, по факту, сводится к установке проверок на null в каждом методе\. Мы же предлагаем альтернативу \- делать проверки только там, где null может действительно прийти, и наш анализатор теперь умеет искать такие ситуации\.

Итак, не откладывая в долгий ящик, перейдём к "разбору полётов" \- анализу результатов проверки Roslyn\. Сначала рассмотрим ошибки, найденные благодаря описанным выше нововведениям\. Вообще, для кода Roslyn в этот раз было выдано довольно много предупреждений\. Я думаю, это связано с тем, что платформа очень активно развивается \(объем кодовой базы на данный момент составляет около 2 770 000 строк кода без учета пустых\), а анализ этого проекта мы давно не делали\. Тем не менее, критичных ошибок не так много, а именно они представляют интерес для статьи\. И да, в Roslyn довольно много тестов, которые я, как обычно, исключил из проверки\.

Начну с ошибок V3080, с уровнем критичности Medium, в которых анализатор обнаружил возможный доступ по нулевой ссылке, но не во всех возможных случаях \(ветках кода\)\.

**Possible null dereference \- Medium**

V3080 Possible null dereference\. Consider inspecting 'current'\. CSharpSyntaxTreeFactoryService\.PositionalSyntaxReference\.cs 70

```cpp
private SyntaxNode GetNode(SyntaxNode root)
{
  var current = root;
  ....
  while (current.FullSpan.Contains(....))     // <=
  {
    ....
    var nodeOrToken = current.ChildThatContainsPosition(....);
    ....
    current = nodeOrToken.AsNode();           // <=
  }
  ....
}

public SyntaxNode AsNode()
{
  if (_token != null)
  {
    return null;
  }
  
  return _nodeOrParent;
}
```

Рассмотрим метод _GetNode_\. Анализатор считает, что возможен доступ по нулевой ссылке в условии блока _while_\. В теле блока _while_ переменной _current_ будет присвоено значение \- результат выполнения метода _AsNode_\. И это значение в ряде случаев будет _null_\. Хороший пример межпроцедурного анализа в действии\.

Теперь рассмотрим похожий случай, в котором межпроцедурный анализ был произведен через два вызова метода\.

V3080 Possible null dereference\. Consider inspecting 'directory'\. CommonCommandLineParser\.cs 911

```cpp
private IEnumerable<CommandLineSourceFile>
  ExpandFileNamePattern(string path, string baseDirectory, ....)
{
    string directory = PathUtilities.GetDirectoryName(path);
    ....
    var resolvedDirectoryPath = (directory.Length == 0) ?  // <=
      baseDirectory : 
      FileUtilities.ResolveRelativePath(directory, baseDirectory);
    ....
}

public static string GetDirectoryName(string path)
{
    return GetDirectoryName(path, IsUnixLikePlatform);
}

internal static string GetDirectoryName(string path, bool isUnixLike)
{
  if (path != null)
  {
    ....
  }
  
  return null;
}
```

Переменная _directory_ в теле метода _ExpandFileNamePattern_ получает значение из метода _GetDirectoryName\(string\)_\. Тот, в свою очередь, вернет результат работы перегруженного метода _GetDirectoryName\(string, bool\)_, значением которого может быть _null_\. Так как далее в теле метода _ExpandFileNamePattern_ переменная _directory_ используется без предварительной проверки на равенство _null_ \- можно говорить о правомерности выдачи предупреждения анализатором\. Это потенциально небезопасная конструкция\.

Ещё один фрагмент кода с ошибкой V3080, точнее, сразу с двумя ошибками, выданными для одной строки кода\. Здесь межпроцедурный анализ не понадобился\.

V3080 Possible null dereference\. Consider inspecting 'spanStartLocation'\. TestWorkspace\.cs 574

V3080 Possible null dereference\. Consider inspecting 'spanEndLocationExclusive'\. TestWorkspace\.cs 574

```cpp
private void MapMarkupSpans(....)
{
  ....
  foreach (....)
  {
    ....
    foreach (....)
    {
      ....
      int? spanStartLocation = null;
      int? spanEndLocationExclusive = null;
  
      foreach (....)
      {
        if (....)
        {
          if (spanStartLocation == null &&
              positionInMarkup <= markupSpanStart && ....)
          {
            ....
            spanStartLocation = ....;
          }
  
          if (spanEndLocationExclusive == null &&
              positionInMarkup <= markupSpanEndExclusive && ....)
          {
            ....
            spanEndLocationExclusive = ....;
            break;
          }
          ....
        }
        ....
      }
  
      tempMappedMarkupSpans[key].
        Add(new TextSpan(
          spanStartLocation.Value,            // <=
          spanEndLocationExclusive.Value -    // <=
            spanStartLocation.Value));
    }
  }
  ....
}
```

Переменные _spanStartLocation_ и _spanEndLocationExclusive_ имеют тип _nullable int_ и инициализируются значением _null_\. Далее в коде им могут быть присвоены значения, но только при выполнении определенных условий\. В ряде случаев их значение останется равным _null_\. Далее в коде происходит доступ к этим переменным по ссылке без предварительной проверки на равенство _null_, на что и указывает анализатор\.

В коде Roslyn содержится довольно много подобных ошибок, более 100\. Зачастую паттерн этих ошибок одинаков\. Есть некий общий метод, который потенциально возвращает _null_\. Результат работы данного метода используется в большом числе мест, иногда через десятки промежуточных вызовов методов или дополнительных проверок\. Важно понимать, что эти ошибки не фатальны, но они потенциально могут приводить к доступу по нулевой ссылке\. А обнаружить такие ошибки очень сложно\. Поэтому в ряде случаев следует подумать о рефакторинге кода, при котором в случае возврата _null_ методом выбрасывалось бы исключение\. В противном случае вы сможете обезопасить свой код только тотальными проверками, что достаточно утомительно и ненадёжно\. Конечно, в каждом конкретном случае решение должно приниматься исходя из особенностей проекта\.

Примечание\. Бывает, что на данный момент не существует ситуаций \(входных данных\), при которых метод вернёт _null_ и никакой настоящей ошибки нет\. Однако такой код всё равно не надёжен, так как всё может измениться при внесении изменений в код\.

Чтобы закрыть тему с [V3080](https://pvs-studio.ru/ru/docs/warnings/v3080/), давайте взглянем на очевидные ошибки с уровнем достоверности High, когда доступ по нулевой ссылке более вероятен или даже неизбежен\.

**Possible null dereference \- High**

V3080 Possible null dereference\. Consider inspecting 'collectionType\.Type'\. AbstractConvertForToForEachCodeRefactoringProvider\.cs 137

```cpp
public override async Task 
ComputeRefactoringsAsync(CodeRefactoringContext context)
{
  ....
  var collectionType = semanticModel.GetTypeInfo(....);
  if (collectionType.Type == null && 
      collectionType.Type.TypeKind == TypeKind.Error)
  {
    return;
  }
  ....  
}
```

Вследствие опечатки в условии \(вместо оператора _\|\|_ использовали _&&_\), код работает совсем не так, как задумано, и доступ к переменной _collectionType\.Type_ будет выполнен при её равенстве _null_\. Условие необходимо исправить следующим образом:

```cpp
if (collectionType.Type == null || 
    collectionType.Type.TypeKind == TypeKind.Error)
  ....
```

Кстати, возможен и второй вариант развития событий: в первой части условия перепутали операторы _\=\=_ и _\!\=_\. Тогда исправленный код имел бы вид:

```cpp
if (collectionType.Type != null &&
    collectionType.Type.TypeKind == TypeKind.Error)
  ....
```

Этот вариант кода менее логичен, но также исправляет допущенную ошибку\. Окончательное решение за авторами проекта\.

Ещё одна подобная ошибка\.

V3080 Possible null dereference\. Consider inspecting 'action'\. TextViewWindow\_InProc\.cs 372

```cpp
private Func<IWpfTextView, Task> GetLightBulbApplicationAction(....)
{
  ....
  if (action == null)
  {
    throw new
      InvalidOperationException(
        $"Unable to find FixAll in {fixAllScope.ToString()}
        code fix for suggested action '{action.DisplayText}'.");
  }
  ....
}
```

Ошибка допущена при формировании сообщения для исключения\. При этом производится попытка доступа к свойству _action\.DisplayText_ через переменную _action_, которая заведомо равна _null_\.

И последняя ошибка V3080 уровня High\.

V3080 Possible null dereference\. Consider inspecting 'type'\. ObjectFormatterHelpers\.cs 91

```cpp
private static bool IsApplicableAttribute(
  TypeInfo type, 
  TypeInfo targetType, 
  string targetTypeName)
{
  return type != null && AreEquivalent(targetType, type)
    || targetTypeName != null && type.FullName == targetTypeName;
}
```

Метод небольшой, поэтому я привожу его код целиком\. Условие в блоке _return_ некорректно\. В ряде случаев возможен выброс исключения _NullReferenceException_ при доступе _type\.FullName_\. Использую скобки \(здесь они не будут менять поведения\) для прояснения ситуации:

```cpp
return (type != null && AreEquivalent(targetType, type))
    || (targetTypeName != null && type.FullName == targetTypeName);
```

Именно так, в соответствии с приоритетом операций, будет работать данный код\. В случае, если переменная _type_ будет равна _null_, мы попадём в else\-проверку, где, убедившись в неравенстве _null_ переменной _targetTypeName_, используем нулевую ссылку _type_\. Исправить код можно, например, так:

```cpp
return type != null && 
  (AreEquivalent(targetType, type) || 
  targetTypeName != null && type.FullName == targetTypeName);
```

Думаю, на этом можно завершить изучение ошибок V3080 и посмотреть, что еще интересного удалось найти анализатору PVS\-Studio в коде Roslyn\.

**Опечатка**

[V3005](https://pvs-studio.ru/ru/docs/warnings/v3005/) The 'SourceCodeKind' variable is assigned to itself\. DynamicFileInfo\.cs 17

```cpp
internal sealed class DynamicFileInfo
{
  ....
  public DynamicFileInfo(
    string filePath,
    SourceCodeKind sourceCodeKind,
    TextLoader textLoader,
    IDocumentServiceProvider documentServiceProvider)
  {
    FilePath = filePath;
    SourceCodeKind = SourceCodeKind;  // <=
    TextLoader = textLoader;
    DocumentServiceProvider = documentServiceProvider;
  }
  ....
}
```

Из\-за неудачного названия переменных, в конструкторе класса _DynamicFileInfo_ допустили опечатку\. Полю _SourceCodeKind_ присваивают его же собственное значение, вместо использования параметра _sourceCodeKind_\. Для минимизации вероятности возникновения таких ошибок рекомендуется использовать префикс подчеркивания для имен параметров в таких случаях\. Приведу пример исправленного варианта кода:

```cpp
public DynamicFileInfo(
  string _filePath,
  SourceCodeKind _sourceCodeKind,
  TextLoader _textLoader,
  IDocumentServiceProvider _documentServiceProvider)
{
  FilePath = _filePath;
  SourceCodeKind = _sourceCodeKind;
  TextLoader = _textLoader;
  DocumentServiceProvider = _documentServiceProvider;
}
```

**Невнимательность**

[V3006](https://pvs-studio.ru/ru/docs/warnings/v3006/) The object was created but it is not being used\. The 'throw' keyword could be missing: throw new InvalidOperationException\(FOO\)\. ProjectBuildManager\.cs 61

```cpp
~ProjectBuildManager()
{
  if (_batchBuildStarted)
  {
    new InvalidOperationException("ProjectBuilderManager.Stop()
                                   not called.");
  }
}
```

При определенном условии деструктор должен выбросить исключение, но этого не происходит, а объект исключения просто создается\. Было пропущено ключевое слово _throw_\. Исправленный вариант кода:

```cpp
~ProjectBuildManager()
{
  if (_batchBuildStarted)
  {
    throw new InvalidOperationException("ProjectBuilderManager.Stop()
                                         not called.");
  }
}
```

Вопрос работы с деструкторами в C\# и выбросу из них исключений \- тема отдельной дискуссии, которая выходит за рамки данной статьи\.

**Когда результат не важен**

Некоторое количество предупреждений [V3009](https://pvs-studio.ru/ru/docs/warnings/v3009/) было получено для методов, которые во всех случаях возвращают одинаковое значение\. Иногда это не критично, или в вызывающем коде возвращаемое значение просто не проверяют\. Такие предупреждения я пропустил\. Но несколько фрагментов кода показались мне подозрительными\. Приведу один из них:

V3009 It's odd that this method always returns one and the same value of 'true'\. GoToDefinitionCommandHandler\.cs 62

```cpp
internal bool TryExecuteCommand(....)
{
  ....
  using (context.OperationContext.AddScope(....))
  {
    if (....)
    {
      return true;
    }  
  }
  ....
  return true;
}
```

Метод _TryExecuteCommand_ возвращает только _true_, и ничего кроме _true_\. При этом в вызывающем коде возвращаемое значение участвует в каких\-то проверках:

```cpp
public bool ExecuteCommand(....)
{
  ....
  if (caretPos.HasValue && TryExecuteCommand(....))
  {
    ....
  }
  ....
}
```

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

Другие подобные предупреждения:

* V3009 It's odd that this method always returns one and the same value of 'true'\. CommentUncommentSelectionCommandHandler\.cs 86
* V3009 It's odd that this method always returns one and the same value of 'true'\. RenameTrackingTaggerProvider\.RenameTrackingCommitter\.cs 99
* V3009 It's odd that this method always returns one and the same value of 'true'\. JsonRpcClient\.cs 138
* V3009 It's odd that this method always returns one and the same value of 'true'\. AbstractFormatEngine\.OperationApplier\.cs 164
* V3009 It's odd that this method always returns one and the same value of 'false'\. TriviaDataFactory\.CodeShapeAnalyzer\.cs 254
* V3009 It's odd that this method always returns one and the same value of 'true'\. ObjectList\.cs 173
* V3009 It's odd that this method always returns one and the same value of 'true'\. ObjectList\.cs 249

**Не то проверили**

[V3019](https://pvs-studio.ru/ru/docs/warnings/v3019/) Possibly an incorrect variable is compared to null after type conversion using 'as' keyword\. Check variables 'value', 'valueToSerialize'\. RoamingVisualStudioProfileOptionPersister\.cs 277

```cpp
public bool TryPersist(OptionKey optionKey, object value)
{
  ....
  var valueToSerialize = value as NamingStylePreferences;
  if (value != null)
  {
      value = valueToSerialize.CreateXElement().ToString();
  }
  ....
}
```

Переменную _value_ приводят к типу _NamingStylePreferences_\. Проблема в следующей за этим проверке\. Даже если переменная _value_ не нулевая, это не гарантирует, что приведение типа было успешным и _valueToSerialize_ содержит не _null_\. Возможен выброс исключения _NullReferenceException_\. Код необходимо исправить следующим образом:

```cpp
var valueToSerialize = value as NamingStylePreferences;
if (valueToSerialize != null)
{
  value = valueToSerialize.CreateXElement().ToString();
}
```

И ещё одна похожая ошибка\.

V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword\. Check variables 'columnState', 'columnState2'\. StreamingFindUsagesPresenter\.cs 181

```cpp
private void SetDefinitionGroupingPriority(....)
{
  ....
  foreach (var columnState in ....)
  {
    var columnState2 = columnState as ColumnState2;
    if (columnState?.Name ==                        // <=
        StandardTableColumnDefinitions2.Definition)
    {
      newColumns.Add(new ColumnState2(
        columnState2.Name,                          // <=
        ....));
    }
    ....
  }
  ....
}
```

Переменную _columnState_ приводят к типу _ColumnState2_\. Однако результат операции, переменную _columnState2_, далее не проверяют на равенство _null_\. Вместо этого проверяется переменная _columnState_ при помощи оператора условного _null_\. Чем опасен этот код? Как и в предыдущем примере, приведение типа при помощи оператора _as_ может закончиться неудачей, и переменная _columnState2_ будет равна _null_, что приведёт к дальнейшему выбросу исключения\. Кстати, виновата здесь может быть и опечатка\. Обратите внимание на условие в блоке _if_\. Возможно, вместо _columnState?\.Name_ хотели написать _columnState2?\.Name_\. Это весьма вероятно, если учесть довольно неудачные имена переменных _columnState и columnState2\._

**Избыточные проверки**

Довольно большое количество предупреждений \(более 100\) было выдано на некритичные, но потенциально небезопасные конструкции, связанные с избыточными проверками\. Для примера приведу одну из них\.

[V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'navInfo \=\= null' is always false\. AbstractSyncClassViewCommandHandler\.cs 101

```cpp
public bool ExecuteCommand(....)
{
  ....
  IVsNavInfo navInfo = null;
  if (symbol != null)
  {
    navInfo = libraryService.NavInfoFactory.CreateForSymbol(....);
  }
  
  if (navInfo == null)
  {
    navInfo = libraryService.NavInfoFactory.CreateForProject(....);
  }
  
  if (navInfo == null)    // <=
  {
    return true;
  }  
  ....
}

public IVsNavInfo CreateForSymbol(....)
{
  ....
  return null;
}

public IVsNavInfo CreateForProject(....)
{
  return new NavInfo(....);
}
```

Пожалуй, настоящей ошибки здесь нет\. Просто хороший повод продемонстрировать связку технологий "межпроцедурный анализ \+ анализ потока данных" в действии\. Анализатор считает, что вторая проверка _navInfo \=\= null_ избыточна\. Действительно, перед этим значение для присвоения _navInfo_ будет получено из метода _libraryService\.NavInfoFactory\.CreateForProject_, который сконструирует и вернет новый объект класса _NavInfo_\. Но никак не _null_\. Возникает вопрос, почему же анализатор не выдал предупреждение для первой проверки _navInfo \=\= null_? Этому есть объяснение\. Во\-первых, в случае, если переменная _symbol_ окажется равной _null_, то и значение _navInfo_ останется нулевой ссылкой\. Во\-вторых, даже если _navInfo_ получит значение из метода _libraryService\.NavInfoFactory\.CreateForSymbol_, это значение может быть равно _null_\. Таким образом, первая проверка _navInfo \=\= null_ действительно необходима\.

**Недостаточно проверок**

А теперь ситуация, обратная рассмотренной выше\. Несколько предупреждений [V3042](https://pvs-studio.ru/ru/docs/warnings/v3042/) было получено для кода, в котором возможен доступ по нулевой ссылке\. При этом всего одна\-две небольшие проверки могли бы всё исправить\.

Рассмотрим один интересный фрагмент кода, в котором содержатся две подобные ошибки\.

V3042 Possible NullReferenceException\. The '?\.' and '\.' operators are used for accessing members of the 'receiver' object Binder\_Expressions\.cs 7770

V3042 Possible NullReferenceException\. The '?\.' and '\.' operators are used for accessing members of the 'receiver' object Binder\_Expressions\.cs 7776

```cpp
private BoundExpression GetReceiverForConditionalBinding(
  ExpressionSyntax binding,
  DiagnosticBag diagnostics)
{
  ....
  BoundExpression receiver = this.ConditionalReceiverExpression;
  if (receiver?.Syntax !=                                // <=
    GetConditionalReceiverSyntax(conditionalAccessNode))
  {
    receiver = BindConditionalAccessReceiver(conditionalAccessNode,
                                             diagnostics);
  }
  
  var receiverType = receiver.Type;                      // <=
  if (receiverType?.IsNullableType() == true)
  {
    ....
  }

  receiver = new BoundConditionalReceiver(receiver.Syntax, 0,  // <=
    receiverType ?? CreateErrorType(), 
    hasErrors: receiver.HasErrors)                             // <=
    { WasCompilerGenerated = true };
  
  return receiver; 
}
```

Переменная _receiver_ может быть равна _null_\. Автор кода это знает, так как использует оператор условного _null_ в условии блока _if_ для доступа к _receiver?\.Syntax_\. Далее в коде переменная _receiver_ используется без всяких проверок для доступа к _receiver\.Type_, _receiver\.Syntax_ и _receiver\.HasErrors_\. Эти ошибки необходимо исправить:

```cpp
private BoundExpression GetReceiverForConditionalBinding(
  ExpressionSyntax binding,
  DiagnosticBag diagnostics)
{
  ....
  BoundExpression receiver = this.ConditionalReceiverExpression;
  if (receiver?.Syntax !=
    GetConditionalReceiverSyntax(conditionalAccessNode))
  {
    receiver = BindConditionalAccessReceiver(conditionalAccessNode,
                                             diagnostics);
  }
  
  var receiverType = receiver?.Type;
  if (receiverType?.IsNullableType() == true)
  {
    ....
  }

  receiver = new BoundConditionalReceiver(receiver?.Syntax, 0,
    receiverType ?? CreateErrorType(), 
    hasErrors: receiver?.HasErrors)
    { WasCompilerGenerated = true };
  
  return receiver; 
}
```

Также необходимо убедиться, что конструктор _BoundConditionalReceiver_ поддерживает получение значений _null_ для своих параметров, или провести дополнительный рефакторинг\.

Другие подобные ошибки:

* V3042 Possible NullReferenceException\. The '?\.' and '\.' operators are used for accessing members of the 'containingType' object SyntaxGeneratorExtensions\_Negate\.cs 240
* V3042 Possible NullReferenceException\. The '?\.' and '\.' operators are used for accessing members of the 'expression' object ExpressionSyntaxExtensions\.cs 349
* V3042 Possible NullReferenceException\. The '?\.' and '\.' operators are used for accessing members of the 'expression' object ExpressionSyntaxExtensions\.cs 349

**Ошибка в условии**

[V3057](https://pvs-studio.ru/ru/docs/warnings/v3057/) The 'Substring' function could receive the '\-1' value while non\-negative value is expected\. Inspect the second argument\. CommonCommandLineParser\.cs 109

```cpp
internal static bool TryParseOption(....)
{
  ....
  if (colon >= 0)
  {
      name = arg.Substring(1, colon - 1);
      value = arg.Substring(colon + 1);
  }
  ....
}
```

В случае, если переменная _colon_ будет равна 0, что допускает условие в коде, метод _Substring_ выбросит исключение _ArgumentOutOfRangeException_\. Необходимо исправление:

```cpp
if (colon > 0)
```

**Возможна опечатка**

[V3065](https://pvs-studio.ru/ru/docs/warnings/v3065/) Parameter 't2' is not utilized inside method's body\. CSharpCodeGenerationHelpers\.cs 84

```cpp
private static TypeDeclarationSyntax
  ReplaceUnterminatedConstructs(....)
{
  ....
  var updatedToken = lastToken.ReplaceTrivia(lastToken.TrailingTrivia,
    (t1, t2) =>
    {
      if (t1.Kind() == SyntaxKind.MultiLineCommentTrivia)
      {
        var text = t1.ToString();
        ....
      }
      else if (t1.Kind() == SyntaxKind.SkippedTokensTrivia)
      {
        return ReplaceUnterminatedConstructs(t1);
      }
      return t1;
    });
  ....
}
```

В лямбда\-выражение передают два параметра: t1 и t2\. Однако используют только t1\. Выглядит подозрительно, учитывая, как легко ошибиться при использовании переменных с такими именами\.

**Невнимательность**

[V3083](https://pvs-studio.ru/ru/docs/warnings/v3083/) Unsafe invocation of event 'TagsChanged', NullReferenceException is possible\. Consider assigning event to a local variable before invoking it\. PreviewUpdater\.Tagger\.cs 37

```cpp
public void OnTextBufferChanged()
{
  if (PreviewUpdater.SpanToShow != default)
  {
    if (TagsChanged != null)
    {
        var span = _textBuffer.CurrentSnapshot.GetFullSpan();
        TagsChanged(this, new SnapshotSpanEventArgs(span));  // <=
    }
  }
}
```

Событие _TagsChanged_ вызывают небезопасно\. Между проверкой на равенство _null_ и вызовом события, от него могут успеть отписаться, тогда будет выброшено исключение\. Более того, в теле блока _if_ непосредственно перед вызовом события делаются ещё какие\-то операции\. Я назвал эту ошибку "Невнимательность", потому что в других местах в коде с этим событием работают более аккуратно, например, так:

```cpp
private void OnTrackingSpansChanged(bool leafChanged)
{
  var handler = TagsChanged;
  if (handler != null)
  {
      var snapshot = _buffer.CurrentSnapshot;
      handler(this,
              new SnapshotSpanEventArgs(snapshot.GetFullSpan()));
  }
}
```

Использование дополнительной переменной _handler_ исключает появление проблемы\. В методе _OnTextBufferChanged_ необходимо внести правки для такой же безопасной работы с событием\.

**Пересекающиеся диапазоны**

[V3092](https://pvs-studio.ru/ru/docs/warnings/v3092/) Range intersections are possible within conditional expressions\. Example: if \(A \> 0 && A < 5\) \{ \.\.\. \} else if \(A \> 3 && A < 9\) \{ \.\.\. \}\. ILBuilderEmit\.cs 677

```cpp
internal void EmitLongConstant(long value)
{
  if (value >= int.MinValue && value <= int.MaxValue)
  {
    ....
  }
  else if (value >= uint.MinValue && value <= uint.MaxValue)
  {
    ....
  }
  else
  {
    ....
  }
}
```

Для лучшего понимания, перепишу данный фрагмент кода, заменив имена констант на их фактические значения:

```cpp
internal void EmitLongConstant(long value)
{
  if (value >= -2147483648 && value <= 2147483648)
  {
    ....
  }
  else if (value >= 0 && value <= 4294967295)
  {
    ....
  }
  else
  {
    ....
  }
}
```

Наверное, здесь нет настоящей ошибки, но условие выглядит странно\. Вторая его часть \(_else if_\) будет выполняться только для диапазона значений от 2147483648 \+ 1 до 4294967295\.

Ещё пара подобных предупреждений:

* V3092 Range intersections are possible within conditional expressions\. Example: if \(A \> 0 && A < 5\) \{ \.\.\. \} else if \(A \> 3 && A < 9\) \{ \.\.\. \}\. LocalRewriter\_Literal\.cs 109
* V3092 Range intersections are possible within conditional expressions\. Example: if \(A \> 0 && A < 5\) \{ \.\.\. \} else if \(A \> 3 && A < 9\) \{ \.\.\. \}\. LocalRewriter\_Literal\.cs 66

**Ещё о проверках на равенство null \(или их отсутствии\)**

Несколько ошибок [V3095](https://pvs-studio.ru/ru/docs/warnings/v3095/) о проверке переменной на равенство _null_ уже после использования\. Первая неоднозначна, рассмотрим код\.

V3095 The 'displayName' object was used before it was verified against null\. Check lines: 498, 503\. FusionAssemblyIdentity\.cs 498

```cpp
internal static IAssemblyName ToAssemblyNameObject(string displayName)
{
  if (displayName.IndexOf('\0') >= 0)
  {
      return null;
  }
  
  Debug.Assert(displayName != null);
  ....
}
```

Предполагается, что ссылка _displayName_ может быть нулевой\. Для этого сделали проверку _Debug\.Assert_\. Непонятно только, почему она идёт уже после использования строки\. Также следует учитывать, что для других, отличных от Debug конфигураций, компилятор уберёт _Debug\.Assert_ из кода вовсе\. Значит ли это, что только для Debug возможно получение нулевой ссылки? И если это не так, то почему не была сделана проверка _string\.IsNullOrEmpty\(string\)_, например\. Это вопросы к авторам кода\.

Следующая ошибка более очевидна\.

V3095 The 'scriptArgsOpt' object was used before it was verified against null\. Check lines: 321, 325\. CommonCommandLineParser\.cs 321

```cpp
internal void FlattenArgs(...., List<string> scriptArgsOpt, ....)
{
  ....
  while (args.Count > 0)
  {
    ....
    if (parsingScriptArgs)
    {
        scriptArgsOpt.Add(arg);  // <=
        continue;
    }
    
    if (scriptArgsOpt != null)
    {
      ....
    }
    ....
  }
}
```

Думаю, этот фрагмент кода не требует пояснений\. Приведу исправленный вариант:

```cpp
internal void FlattenArgs(...., List<string> scriptArgsOpt, ....)
{
  ....
  while (args.Count > 0)
  {
    ....
    if (parsingScriptArgs)
    {
        scriptArgsOpt?.Add(arg);
        continue;
    }
    
    if (scriptArgsOpt != null)
    {
      ....
    }
    ....
  }
}
```

В коде Roslyn нашлось еще 15 подобных ошибок:

* V3095 The 'LocalFunctions' object was used before it was verified against null\. Check lines: 289, 317\. ControlFlowGraphBuilder\.RegionBuilder\.cs 289
* V3095 The 'resolution\.OverloadResolutionResult' object was used before it was verified against null\. Check lines: 579, 588\. Binder\_Invocation\.cs 579
* V3095 The 'resolution\.MethodGroup' object was used before it was verified against null\. Check lines: 592, 621\. Binder\_Invocation\.cs 592
* V3095 The 'touchedFilesLogger' object was used before it was verified against null\. Check lines: 111, 126\. CSharpCompiler\.cs 111
* V3095 The 'newExceptionRegionsOpt' object was used before it was verified against null\. Check lines: 736, 743\. AbstractEditAndContinueAnalyzer\.cs 736
* V3095 The 'symbol' object was used before it was verified against null\. Check lines: 422, 427\. AbstractGenerateConstructorService\.Editor\.cs 422
* V3095 The '\_state\.BaseTypeOrInterfaceOpt' object was used before it was verified against null\. Check lines: 132, 140\. AbstractGenerateTypeService\.GenerateNamedType\.cs 132
* V3095 The 'element' object was used before it was verified against null\. Check lines: 232, 233\. ProjectUtil\.cs 232
* V3095 The 'languages' object was used before it was verified against null\. Check lines: 22, 28\. ExportCodeCleanupProvider\.cs 22
* V3095 The 'memberType' object was used before it was verified against null\. Check lines: 183, 184\. SyntaxGeneratorExtensions\_CreateGetHashCodeMethod\.cs 183
* V3095 The 'validTypeDeclarations' object was used before it was verified against null\. Check lines: 223, 228\. SyntaxTreeExtensions\.cs 223
* V3095 The 'text' object was used before it was verified against null\. Check lines: 376, 385\. MSBuildWorkspace\.cs 376
* V3095 The 'nameOrMemberAccessExpression' object was used before it was verified against null\. Check lines: 206, 223\. CSharpGenerateTypeService\.cs 206
* V3095 The 'simpleName' object was used before it was verified against null\. Check lines: 83, 85\. CSharpGenerateMethodService\.cs 83
* V3095 The 'option' object was used before it was verified against null\. Check lines: 23, 28\. OptionKey\.cs 23

Рассмотрим ошибки [V3105](https://pvs-studio.ru/ru/docs/warnings/v3105/)\. Здесь используется оператор условного _null_ при инициализации переменной, а вот далее в коде переменная используется без проверок на равенство _null_\. 

О следующей ошибке сигнализируют сразу два предупреждения\.

V3105 The 'documentId' variable was used after it was assigned through null\-conditional operator\. NullReferenceException is possible\. CodeLensReferencesService\.cs 138

V3105 The 'documentId' variable was used after it was assigned through null\-conditional operator\. NullReferenceException is possible\. CodeLensReferencesService\.cs 139

```cpp
private static async Task<ReferenceLocationDescriptor>
  GetDescriptorOfEnclosingSymbolAsync(....)
{
  ....
  var documentId = solution.GetDocument(location.SourceTree)?.Id;
  
  return new ReferenceLocationDescriptor(
    ....
    documentId.ProjectId.Id,
    documentId.Id,
    ....);
}
```

Переменная _documentId_ может быть инициализирована значением _null_\. В результате, создание объекта _ReferenceLocationDescriptor_ закончится выбросом исключения\. Код необходимо исправить:

```cpp
return new ReferenceLocationDescriptor(
  ....
  documentId?.ProjectId.Id,
  documentId?.Id,
  ....);
```

Также далее в коде необходимо предусмотреть возможность равенства _null_ переменных, передаваемых в конструктор\.

Другие похожие ошибки в коде:

* V3105 The 'symbol' variable was used after it was assigned through null\-conditional operator\. NullReferenceException is possible\. SymbolFinder\_Hierarchy\.cs 44
* V3105 The 'symbol' variable was used after it was assigned through null\-conditional operator\. NullReferenceException is possible\. SymbolFinder\_Hierarchy\.cs 51

**Приоритеты и скобки**

[V3123](https://pvs-studio.ru/ru/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 condition\. Edit\.cs 70

```cpp
public bool Equals(Edit<TNode> other)
{
  return _kind == other._kind
    && (_oldNode == null) ? other._oldNode == null :
        _oldNode.Equals(other._oldNode)
    && (_newNode == null) ? other._newNode == null :
       _newNode.Equals(other._newNode);
}
```

Условие в блоке return вычисляется совсем не так, как думал разработчик\. Предполагалось, что первым условием будет _\_kind \=\= other\.\_kin_d \(поэтому после этого условия сделан перенос строки\), а затем последовательно будут вычислены блоки условий с оператором "_?_"\. На самом деле, первым условием будет _\_kind \=\= other\.\_kind && \(\_oldNode \=\= null\)_\. Это связано с тем, что оператор _&&_ имеет более высокий приоритет, чем оператор "_?_"\. Для исправления ошибки, необходимо взять в скобки все выражения оператора "_?_":

```cpp
return _kind == other._kind
    && ((_oldNode == null) ? other._oldNode == null :
        _oldNode.Equals(other._oldNode))
    && ((_newNode == null) ? other._newNode == null :
       _newNode.Equals(other._newNode));
```

На этом я завершаю описание найденных ошибок\.

**Выводы**

Несмотря на значительное число ошибок, которые мне удалось обнаружить, в пересчете на размер кода проекта Roslyn \(2 770 000 строк\) это составит довольно незначительную величину\. Как и Андрей в предыдущей статье, я также готов признать высокое качество этого проекта\.

Хочу отметить, что подобные эпизодические проверки кода не имеют ничего общего с методологией статического анализа и практически не приносят пользы\. Статический анализ должен применяться регулярно, а не от случая к случаю\. Тогда многие ошибки будут исправлены на самых ранних этапах, а, следовательно, стоимость их исправления будет в десятки раз ниже\. Более подробно эта мысль изложена в этой маленькой [заметке](https://pvs-studio.ru/ru/blog/posts/0534/), с которой я очень прошу ознакомиться\.

Вы можете самостоятельно поискать ещё какие\-то ошибки как в рассмотренном проекте, так и в любом другом\. Для этого просто необходимо [скачать](https://pvs-studio.ru/ru/pvs-studio/download/) и попробовать наш анализатор\.