# Как найти ошибки в C\# проекте, работая под Linux и macOS

PVS\-Studio – известный статический анализатор кода, позволяющий найти множество каверзных ошибок, скрытых в исходниках\. Недавно завершился бета\-тест новой версии, в которой появилась возможность анализа C\# проектов под Linux и macOS\. Кроме того, теперь анализатор можно интегрировать с кроссплатформенной IDE от JetBrains – Rider\. Данная статья позволит познакомиться с этими возможностями на примере проверки open source проекта RavenDB\.

![0740_PVS-Studio_for_Linux_and_macOS_ru/image1.png](https://import.viva64.com/docx/blog/0740_PVS-Studio_for_Linux_and_macOS_ru/image1.png)

## Введение

Некоторое время назад мой коллега [Сергей Васильев](https://pvs-studio.ru/ru/blog/posts/?author=sergey-vasiliev) написал [заметку](https://pvs-studio.ru/ru/blog/posts/0730/) об открытии бета\-теста новой версии разрабатываемого нами статического анализатора PVS\-Studio\. Сейчас бета\-тест завершён и новую версию можно загрузить, перейдя по [ссылке](https://pvs-studio.ru/ru/pvs-studio/download/)\. В этой же статье мы рассмотрим, как проводится анализ C\# проектов под Linux/macOS с использованием консольного интерфейса и [Rider](https://www.jetbrains.com/ru-ru/rider/)\. После этого проведём традиционный разбор некоторых интересных срабатываний анализатора\.

## RavenDB

![0740_PVS-Studio_for_Linux_and_macOS_ru/image2.png](https://import.viva64.com/docx/blog/0740_PVS-Studio_for_Linux_and_macOS_ru/image2.png)

Для проверки был выбран открытый проект RavenDB, [репозиторий](https://github.com/ravendb/ravendb) которого насчитывает почти 5 тысяч файлов исходного кода\. Он представляет собой достаточно популярную NoSQL\-базу данных\. Подробности можно узнать на [сайте](https://ravendb.net)\. Нетрудно догадаться, что меня привлёк его размер, ведь в столь большом и, вне всяких сомнений, серьёзном проекте наверняка найдётся что\-нибудь интересное\.

## Command Line Interface

Для начала рассмотрим, как производится анализ через консоль\. Этот раздел, на мой взгляд, будет особенно интересен тем, кто желает интегрировать анализатор в CI\-систему\. У команды, запускающей анализ, есть ряд интересных опций, однако в общем случае всё достаточно тривиально\. Чтобы провести анализ RavenDB, я перехожу в папку с проектом и ввожу в консоли следующее:

```cpp
pvs-studio-dotnet -t ./RavenDB.sln
```

Флаг \-t \(сокращение от target\) служит для указания файла решения или проекта для проверки\. Представленная выше строка запускает анализ и в результате работы формирует файл, содержащий найденные ошибки\. Всё просто, не правда ли?

## Rider

Работа с анализатором в Rider представляет собой примерно то же самое, что и в Visual Studio\. Простой и понятный интерфейс плагина позволит проверить проект буквально в пару кликов\. Это не преувеличение – для проведения анализа RavenDB мне было достаточно кликнуть в верхнем меню Tools, навести на "PVS\-Studio" и кликнуть "Check Current Solution/Project"\.

![0740_PVS-Studio_for_Linux_and_macOS_ru/image3.png](https://import.viva64.com/docx/blog/0740_PVS-Studio_for_Linux_and_macOS_ru/image3.png)

Результаты анализа будут отображены в нижней части окна на вкладке PVS\-Studio \(ну а на какой же ещё? :\) \)

![0740_PVS-Studio_for_Linux_and_macOS_ru/image5.png](https://import.viva64.com/docx/blog/0740_PVS-Studio_for_Linux_and_macOS_ru/image5.png)

Как и в случае с плагином для Visual Studio, двойной клик по предупреждению отобразит место, с которым оно связано\. Всё удобно и понятно\.

Что ещё более важно, инструмент PVS\-Studio не просто указывает на ошибки, а имеет инфраструктуру, позволяющую легко внедрить методологию статического анализа даже в большой старый проект\.

Общая идея в следующем\. Пользователь запустил анализатор и получил множество предупреждений\. Раз проект, разрабатываемый много лет, жив, развивается и приносит деньги, то, скорее всего, в отчёте не будет много предупреждений, указывающих на критические дефекты\. Другими словами, критические баги так или иначе уже поправлены более дорогими способами или благодаря фидбеку от клиентов\. Соответственно, всё, что сейчас находит анализатор, можно считать техническим долгом, который непрактично стараться устранить сразу\. Рационально пока игнорировать эти предупреждения, но писать новый код уже выполняя регулярный анализ\.

Можно указать PVS\-Studio считать эти предупреждения пока неактуальными \(отложить технический долг на потом\), и больше их не показывать\. Анализатор создаёт специальный файл, где сохраняет информацию о пока неинтересных ошибках\. И теперь PVS\-Studio будет выдавать предупреждения только на новый или измененный код\. Причем, всё это реализовано умно\. Если, например, в начало некоего файла будет добавлена пустая строка, то анализатор понимает, что, по сути, ничего не изменилось, и по\-прежнему будет молчать\. Этот файл разметки можно заложить в систему контроля версий\. Файл большой, но это не страшно, так как часто его закладывать смысла нет\.

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

Для подавления предупреждений на существующий код в Rider необходимо просто перейти в верхнем меню в Tools\-\>PVS\-Studio и кликнуть "Suppress All Messages"\.

![0740_PVS-Studio_for_Linux_and_macOS_ru/image6.png](https://import.viva64.com/docx/blog/0740_PVS-Studio_for_Linux_and_macOS_ru/image6.png)

В появившемся окне, предупреждающем о том, что в suppress\-лист попадут все текущие срабатывания, кликаем на "Ok"\. В результате в папке с проектом будет создан suppress\-файл, который будет учитываться анализатором при дальнейшей работе\.

Надо отметить, что в Rider уже есть анализатор, который успешно подсвечивает некоторые ошибки\. Таким образом, ряд предупреждений PVS\-Studio указывает на код, который выглядит подозрительно и с точки зрения редактора\. Тем не менее, PVS\-Studio довольно часто находит ошибки, которые смогли уйти от чуткого взора анализатора от JetBrains\. Именно поэтому наиболее эффективным решением будет позволить им работать в команде\.

![0740_PVS-Studio_for_Linux_and_macOS_ru/image7.png](https://import.viva64.com/docx/blog/0740_PVS-Studio_for_Linux_and_macOS_ru/image7.png)

## А на десерт

Теперь, как и обещал, рассмотрим, какие интересные предупреждения отобразил анализатор по результатам проверки\. Проект содержит огромное количество файлов исходного кода, поэтому неудивительно было найти в нём множество подозрительных моментов\. Ничего не поделаешь – все совершают ошибки, однако важно прикладывать максимум усилий к их своевременному обнаружению и исправлению\. Статический анализ позволяет существенно упростить эту задачу\.

В результате проверки было выведено около тысячи предупреждений: 

![0740_PVS-Studio_for_Linux_and_macOS_ru/image8.png](https://import.viva64.com/docx/blog/0740_PVS-Studio_for_Linux_and_macOS_ru/image8.png)

Подробнее об уровнях предупреждений можно прочитать, перейдя по [ссылке](https://pvs-studio.ru/ru/docs/manual/0007/)\. 

Конечно, не все срабатывания указывают на супер\-страшные ошибки\. Если бы это было так, вряд ли бы в проекте хоть что\-то работало :\)\. Но важно понимать – раз анализатор "ругается", значит код выглядит странно, и его стоит внимательно изучить\.

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

### Всего лишь дополнительная проверка?

```cpp
public static void EnsurePathExists(string file)
{
  var dirpath = Path.GetDirectoryName(file);
  List<string> dirsToCreate = new List<string>();
  while (Directory.Exists(dirpath) == false)
  {
    dirsToCreate.Add(dirpath);
    dirpath = Directory.GetParent(dirpath).ToString();
    if (dirpath == null)                                  // <=
      break;
  }
  dirsToCreate.ForEach(x => Directory.CreateDirectory(x));
}
```

**Предупреждение анализатора**: [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'dirpath \=\= null' is always false\. PosixHelper\.cs\(124\) Voron

Данное срабатывание можно рассмотреть по\-разному\. С одной стороны, лишняя проверка – это, конечно, не очень хорошо, но сама по себе она ошибкой не является\. С другой – стоит задуматься: действительно ли этот код работает так, как задумывал программист?

Возможно, разработчик и правда не знал, что _ToString_ никогда не вернёт _null_\. Если же это не так, то можно сделать предположение о том, чего он хотел добиться\. 

Быть может, _break_ должен вызываться, когда не удаётся получить родителя для рассматриваемой директории\. Если это так, то проверка на _null_ обретает смысл\. Однако рассматривать нужно не результат работы _ToString_, а значение, возвращаемое методом _GetParent_:

```cpp
dirsToCreate.Add(dirpath);
var dir = Directory.GetParent(dirpath);    
if (dir == null)
  break;

dirpath = dir.ToString();
```

В противном случае, возвращение _null_ методом _GetParent_ приводит к исключению при вызове _ToString_\.

### Типичный null

```cpp
public long ScanOldest()
{
  ....
  for (int i = 0; i < copy.Length; i++)
  {
    var item = copy[i].Value;
    if (item != null || item == InvalidLowLevelTransaction) // <=
    {
      if (val > item.Id)                                    // <=
        val = item.Id;
    }
  }
  ....
}
```

**Предупреждение анализатора**: [V3125](https://pvs-studio.ru/ru/docs/warnings/v3125/) The 'item' object was used after it was verified against null\. Check lines: 249, 247\. ActiveTransactions\.cs\(249\), ActiveTransactions\.cs\(247\) Voron

Код выглядит странно из\-за происходящего в том случае, когда _item_ действительно _null_\. Действительно, если _InvalidLowLevelTransaction_ тоже окажется _null_, условие окажется истинным и попытка получения _item\.Id_ приведёт к выбрасыванию исключения\. А если _InvalidLowLevelTransaction_ не может оказаться _null_, то условие "_item \=\= InvalidLowLevelTransaction_" попросту лишнее\. Всё потому, что оно проверяется лишь в случае, когда _item \=\= null_\. Ну а если вдруг _item_ не может быть _null_, то вообще всё условие теряет смысл и лишь добавляет ненужную вложенность\.

Я думаю, что здесь, возможно, выбран неверный логический оператор\. Если заменить в условии "\|\|" на "&&", то код сразу начинает выглядеть логично\. Ну и никаких проблем в таком случае возникнуть не может\.

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

### Снова лишняя проверка?

```cpp
public void WriteObjectEnd()
{
  ....
  if (_continuationState.Count > 1)
  {
    var outerState = 
      _continuationState.Count > 0 ? _continuationState.Pop() : currentState
    ;
    if (outerState.FirstWrite == -1)
      outerState.FirstWrite = start;
    _continuationState.Push(outerState);
  }  
   ....
}
```

**Предупреждение анализатора**: [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression '\_continuationState\.Count \> 0' is always true\. ManualBlittableJsonDocumentBuilder\.cs\(152\) Sparrow

Сначала во внешнем условии проверяется, что количество элементов в коллекции больше 1, а затем на следующей же строке тернарный оператор проверяет, что их число больше 0\. Создаётся впечатление, что одна из проверок должна выглядеть иначе\. Так или иначе, этот код весьма подозрительно выглядит и должен быть внимательно изучен и переработан при необходимости\.

### Возможный NRE

```cpp
protected override Expression VisitIndex(IndexExpression node)
{
  if (node.Object != null)
  {
    Visit(node.Object);
  }
  else
  {
    Out(node.Indexer.DeclaringType.Name); // <=
  }
  if (node.Indexer != null)               // <=
  {
    Out(".");
    Out(node.Indexer.Name);
  }
  VisitExpressions('[', node.Arguments, ']');
  return node;
}
```

**Предупреждение анализатора**: [V3095](https://pvs-studio.ru/ru/docs/warnings/v3095/) The 'node\.Indexer' object was used before it was verified against null\. Check lines: 1180, 1182\. ExpressionStringBuilder\.cs\(1180\), ExpressionStringBuilder\.cs\(1182\) Raven\.Client

На самом деле, это ещё одно место, которое считает подозрительным и PVS\-Studio, и Rider\. Правда, формулировки несколько отличаются: анализатор от JetBrains просто подсвечивает _node\.Indexer\.DeclaringType_ с комментарием "Possible NullReferenceException"\. 

Оба проверяющих утверждают, что в этом фрагменте действительно может быть выброшено исключение\. Должен заметить, что предупреждение от PVS\-Studio не только говорит, что тут возможна ошибка, но и объясняет, почему у него сложилось такое мнение\. Вроде мелочь, а приятно\.

На самом деле, не факт, что тут и правда допущена ошибка\. Вполне возможно, что если _node\.Object \=\= null_, то _node\.Indexer_ точно задан\. При этом допустима ситуация, когда _node\.Object _и _node\.Indexer_ оба не равны _null_\. Только в таком случае можно считать данное срабатывание обоих анализаторов ложным\. Поэтому стоит внимательно проанализировать все возможные варианты\.

### А если копнуть глубже?

```cpp
private async Task LoadStartingWithInternal(....)
{
  ....
  var command = operation.CreateRequest();
  if (command != null)                       // <=
  {
    await RequestExecutor
      .ExecuteAsync(command, Context, SessionInfo, token)
      .ConfigureAwait(false)
    ;

    if (stream != null)
      Context.Write(stream, command.Result.Results.Parent);
    else
      operation.SetResult(command.Result);
  }
  ....
}
```

**Предупреждение анализатора**: [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'command \!\= null' is always true\. AsyncDocumentSession\.Load\.cs\(175\) Raven\.Client

Предупреждение выдаётся из\-за того, что метод _CreateRequest_ никогда не возвращает _null_\. В самом деле, достаточно взглянуть на его код, чтобы убедиться в этом:

```cpp
public GetDocumentsCommand CreateRequest()
{
  _session.IncrementRequestCount();
  if (Logger.IsInfoEnabled)
    Logger.Info(....);

  return new GetDocumentsCommand(....);
}
```

В целом, эта проверка не такая уж и проблема, хотя может быть так, что метод раньше и правда возвращал null в определённых обстоятельствах, а теперь в случае чего бросает исключение\. Кто знает, возможно, что вместо той проверки на _null_ сейчас должен быть try\-catch\.

У вас может возникнуть вполне разумный вопрос: а где тут бросается исключение\-то? Если их нет, то тогда перед нами всё\-таки просто лишняя проверка, и никакой ошибки тут быть не может\.

Но увы, при выполнении метода исключение действительно может быть выброшено, причём дважды\. Сначала в методе _IncrementRequestCount_:

```cpp
public void IncrementRequestCount()
{
  if (++NumberOfRequests > MaxNumberOfRequestsPerSession)
    throw new InvalidOperationException(....);
}
```

Затем – в конструкторе _GetDocumentsCommand_:

```cpp
public GetDocumentsCommand(string startWith, ....)
{
  _startWith = startWith ?? throw new ArgumentNullException(nameof(startWith));
  ....
}
```

### Традиционный copy\-paste

```cpp
public override void WriteTo(StringBuilder writer)
{
  ....
  if (SqlConnectionStringsUpdated)
    json[nameof(SqlConnectionStringsUpdated)] = SqlConnectionStringsUpdated;

  if (ClientConfigurationUpdated)
    json[nameof(ClientConfigurationUpdated)] = ClientConfigurationUpdated;

  if (ConflictSolverConfigUpdated)
    json[nameof(ConflictSolverConfigUpdated)] = ClientConfigurationUpdated;

  if (PeriodicBackupsUpdated)
    json[nameof(PeriodicBackupsUpdated)] = PeriodicBackupsUpdated;

  if (ExternalReplicationsUpdated)
    json[nameof(ExternalReplicationsUpdated)] = ExternalReplicationsUpdated;
  ....
}
```

**Предупреждение анализатора**: [V3127](https://pvs-studio.ru/ru/docs/warnings/v3127/) Two similar code fragments were found\. Perhaps, this is a typo\. SmugglerResult\.cs\(256\), SmugglerResult\.cs\(253\) Raven\.Client

Сильно сомневаюсь, что хоть кто\-нибудь увидел бы тут странность, взглянув на код\. Функция состоит из 14 подобных условий и все переменные заканчиваются на Updated\. Даже когда тут приведена малая её часть, ошибку видно не сразу\.

Человеческий мозг буквально отказывается искать в таком коде что\-либо\. В то же время, PVS\-Studio с легкостью обнаружил, что присваивается, скорее всего, совершенно не то, должно:

```cpp
if (ClientConfigurationUpdated)
    json[nameof(ClientConfigurationUpdated)] = ClientConfigurationUpdated;

if (ConflictSolverConfigUpdated)
    json[nameof(ConflictSolverConfigUpdated)] = ClientConfigurationUpdated;
```

Логично, что в нижней строке справа от оператора присваивания должно быть _ConflictSolverConfigUpdated_\. Я уверен, что без статического анализа данная странность была бы найдена только в случае, если бы из\-за неё сломалось что\-то достаточно серьёзное\. Программист сможет заметить, что в этой функции скрыта проблема, разве что только если будет знать об этом заранее\.

### Ох уж этот "??"

```cpp
public int Count => 
  _documentsByEntity.Count + _onBeforeStoreDocumentsByEntity?.Count ?? 0;
```

**Предупреждение анализатора**: [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 left part\. InMemoryDocumentSessionOperations\.cs\(1952\) Raven\.Client

Конечно, сохраняется возможность, что это вовсе не ошибка, что так всё и задумано\. И всё же это место выглядит очень подозрительно\. Ведь логично предположить, что идея функции состоит вовсе не в том, чтобы возвращать 0 при _\_onBeforeStoreDocumentsByEntity \=\= null\._

Я думаю, что здесь действительно присутствует ошибка, связанная с приоритетами операторов\. В этом случае, необходимо поставить скобки:

```cpp
_documentsByEntity.Count + (_onBeforeStoreDocumentsByEntity?.Count ?? 0)
```

Ну а если вдруг и правда всё так и задумано, то, возможно, стоит указать на это явно – тогда у анализатора и программистов, читающих код, вопросов не возникнет:

```cpp
(_documentsByEntity.Count + _onBeforeStoreDocumentsByEntity?.Count) ?? 0
```

Но это уже дело вкуса, конечно\.

### Передача параметров

```cpp
private static void UpdateEnvironmentVariableLicenseString(....)
{
  ....
  if (ValidateLicense(newLicense, rsaParameters, oldLicense) == false)
    return;
  ....
}
```

**Предупреждение анализатора**: [V3066](https://pvs-studio.ru/ru/docs/warnings/v3066/) Possible incorrect order of arguments passed to 'ValidateLicense' method: 'newLicense' and 'oldLicense'\. LicenseHelper\.cs\(177\) Raven\.Server

Аргументы переданы в метод в подозрительном порядке\. В самом деле, взглянем на объявление:

```cpp
private static bool ValidateLicense(
  License oldLicense, 
  RSAParameters rsaParameters, 
  License newLicense
)
```

Весьма приятно, что PVS\-Studio способен находить даже такие ошибки\. Это отличный пример к вопросу о преимуществах статического анализа перед динамическим\. 

Несмотря на вышесказанное, я поначалу предположил, что, возможно, не имеет значения, в каком порядке эти аргументы передавать\. Конечно, в этом случае были бы не совсем корректно подобраны имена, но что ж поделать\. Однако внутренняя структура _ValidateLicense_ говорит о том, что эти параметры всё\-таки имеют различный смысл\. Код этой функции можно посмотреть, перейдя по [ссылке](https://github.com/ravendb/ravendb/blob/15cb8a716f94e1af1d37d292576fbd53c439180d/src/Raven.Server/Commercial/LicenseHelper.cs#L196)\.

### Never continue

```cpp
private List<CounterOperation> GetCounterOperationsFor(RavenEtlItem item)
{
  ....
  for (var i = 0; i < counters.Count; i++)
  {
    counters.GetPropertyByIndex(i, ref prop);

    if (
      GetCounterValueAndCheckIfShouldSkip(
        item.DocumentId, 
        null, 
        prop, 
        out long value, 
        out bool delete
      )
    ) continue;
    ....
  }
  ....
}
```

**Предупреждение анализатора**: [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'GetCounterValueAndCheckIfShouldSkip\(item\.DocumentId, null, prop, out long value, out bool delete\)' is always false\. RavenEtlDocumentTransformer\.cs\(362\) Raven\.Server

Полностью данный метод можно рассмотреть, перейдя по [ссылке](https://github.com/ravendb/ravendb/blob/15cb8a716f94e1af1d37d292576fbd53c439180d/src/Raven.Server/Documents/ETL/Providers/Raven/RavenEtlDocumentTransformer.cs#L348)\.

Предупреждение говорит о том, что вызов _continue_ в этом цикле недостижим\. И если это так, то фрагмент действительно странный\. Но может это просто ложное срабатывание? Всё же Rider по этому поводу не ругается\.

Взглянем на метод _GetCounterValueAndCheckIfShouldSkip_:

```cpp
private bool GetCounterValueAndCheckIfShouldSkip(
  LazyStringValue docId, 
  string function, 
  BlittableJsonReaderObject.PropertyDetails prop, 
  out long value, 
  out bool delete
)
{
  value = 0;

  if (prop.Value is LazyStringValue)
  {
    delete = true;
  }

  else
  {
    delete = false;
    value = CountersStorage.InternalGetCounterValue(
      prop.Value as BlittableJsonReaderObject.RawBlob, 
      docId, 
      prop.Name
    );

    if (function != null)
    {
      using (var result = BehaviorsScript.Run(
        Context, 
        Context, 
        function, 
        new object[] { docId, prop.Name }
      ))
      {
        if (result.BooleanValue != true)
          return true;
      }
    }
  }

  return false;
}
```

Очевидно, этот метод может вернуть _true_ только в том случае, если _function_ \!\= _null_\. В коде же, рассмотренном ранее, на место этого параметра передаётся именно нулевой указатель\. Значит, вызов _continue_ действительно недостижим\.

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

### Доверяй, но проверяй

```cpp
public LicenseType Type
{
  get
  {
    if (ErrorMessage != null)
      return LicenseType.Invalid;

    if (Attributes == null)
      return LicenseType.None;

    if (Attributes != null &&                             // <=
        Attributes.TryGetValue("type", out object type) &&
        type is int
    )
    {
      var typeAsInt = (int)type;
      if (Enum.IsDefined(typeof(LicenseType), typeAsInt))
        return (LicenseType)typeAsInt;
    }

    return LicenseType.Community;
  }
}
```

**Предупреждение анализатора**: [V3063](https://pvs-studio.ru/ru/docs/warnings/v3063/) A part of conditional expression is always true if it is evaluated: Attributes \!\= null\. LicenseStatus\.cs\(28\) Raven\.Server

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

### Nullable, который никогда не null

```cpp
public Task SuspendObserver()
{
  if (ServerStore.IsLeader())
  {
    var suspend = GetBoolValueQueryString("value");
    if (suspend.HasValue)                                  // <=
    {
      Server.ServerStore.Observer.Suspended = suspend.Value;
    }

    NoContentStatus();
    return Task.CompletedTask;
  }

  RedirectToLeader();

  return Task.CompletedTask;
}
```

**Предупреждение анализатора**: [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'suspend\.HasValue' is always true\. RachisAdminHandler\.cs\(116\) Raven\.Server

Очередная с виду безобидная "лишняя" проверка\. Хотя пока не ясно, с чего анализатор считает её таковой\.

Обратимся к _GetBoolValueQueryString_:

```cpp
protected bool? GetBoolValueQueryString(string name, bool required = true)
{
  var boolAsString = GetStringQueryString(name, required);
  if (boolAsString == null)
    return null;

  if (bool.TryParse(boolAsString, out bool result) == false)
    ThrowInvalidBoolean(name, boolAsString);

  return result;
}
```

Действительно, иногда эта функция возвращает _null_\. Да и Rider не считал ту проверку лишней\. Неужели Единорог подвёл?

![0740_PVS-Studio_for_Linux_and_macOS_ru/image9.png](https://import.viva64.com/docx/blog/0740_PVS-Studio_for_Linux_and_macOS_ru/image9.png)

А что, если взглянуть на метод _GetStringQueryString_?

```cpp
protected string GetStringQueryString(string name, bool required = true)
{
  var val = HttpContext.Request.Query[name];
  if (val.Count == 0 || string.IsNullOrWhiteSpace(val[0]))
  {
    if (required)
      ThrowRequiredMember(name);

    return null;
  }

  return val[0];
}
```

Хм, если параметр _required_ \=\= _true_, то будет вызван метод _ThrowRequiredMember_\. Интересно, что же он делает? :\) Ну, чтобы уж точно не осталось сомнений:

```cpp
private static void ThrowRequiredMember(string name)
{
  throw new ArgumentException(
    $"Query string {name} is mandatory, but wasn't specified."
  );
}
```

Итак, резюмируем\. Разработчик вызывает метод _GetBoolValueQueryString_\. Вероятно, он считает, что потенциально метод не получит необходимое значение\. Ну и как итог – вернёт _null_\. Внутри вызывается _GetStringQueryString_\. В случае если возникают проблемы, он либо вернёт null, либо выбросит исключение\. Второе происходит в том случае, если параметр _required_ установлен как _true_\. При этом это его значение по умолчанию\. В то же время, при вызове _GetBoolValueQueryString_ он, если посмотреть на код выше, как раз не передаётся\.

Давайте рассмотрим ещё раз код метода _SuspendObserver_, на фрагмент которого и ругался анализатор:

```cpp
public Task SuspendObserver()
{
  if (ServerStore.IsLeader())
  {
    var suspend = GetBoolValueQueryString("value");
    if (suspend.HasValue)
    {
      Server.ServerStore.Observer.Suspended = suspend.Value;
    }

    NoContentStatus();
    return Task.CompletedTask;
  }

  RedirectToLeader();

  return Task.CompletedTask;
}
```

Создаётся впечатление, что по задумке поток выполнения тут не должен прерываться, если _GetBoolValueQueryString_ не смог получить значение\. В самом деле, после блока с проверкой на _null _производятся различные действия и возвращается значение\. Полагаю, по задумке эти действия производятся **независимо **от** **успешности работы метода_ GetBoolValueQueryString_\. Что же произойдёт на самом деле? Поток выполнения будет прерван исключением\.

Чтобы этот момент поправить, нужно при вызове _GetBoolValueQueryString_ передать в качестве второго параметра _required_ значение _false_\. Тогда всё действительно будет работать так, как ожидается\.

Как я и говорил ранее, иногда кажется, что анализатор ошибается \(чего греха таить, бывает и такое\)\. Также достаточно часто предупреждение выглядит незначительным\. Казалось бы, есть тут лишняя проверка, да и ладно\. Ну или ещё лучше – уберём её и никаких проблем – предупреждение ведь пропадёт\!

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

### Странности

```cpp
private async Task<int> WriteDocumentsJsonAsync(...., int numberOfResults) // <=
{
  using (
    var writer = new AsyncBlittableJsonTextWriter(
      context, 
      ResponseBodyStream(), 
      Database.DatabaseShutdown
    )
  )
  {
    writer.WriteStartObject();
    writer.WritePropertyName(nameof(GetDocumentsResult.Results));
    numberOfResults = await writer.WriteDocumentsAsync(                    // <=
      context, 
      documentsToWrite, 
      metadataOnly
    );

    ....
  }
  return numberOfResults;
}
```

**Предупреждение анализатора**: [V3061](https://pvs-studio.ru/ru/docs/warnings/v3061/) Parameter 'numberOfResults' is always rewritten in method body before being used\. DocumentHandler\.cs\(273\), DocumentHandler\.cs\(267\) Raven\.Server

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

Мне стало любопытно взглянуть на то, как используется этот метод в существующем коде\. Я понадеялся, что раз он приватный, их не должно быть особо много\. Спасибо Rider, я с лёгкостью нашёл, где же производится вызов\. Это было единственное место:

```cpp
private async Task GetDocumentsByIdAsync(....)
{
  ....            
  int numberOfResults = 0;

  numberOfResults = await WriteDocumentsJsonAsync(
    context, 
    metadataOnly, 
    documents, 
    includes, 
    includeCounters?.Results, 
    numberOfResults
  );

  ....
}
```

Переменной присваивается 0, затем она передаётся в метод, результат работы которого присваивается ей же\. И внутри метода этот параметр никак не используется\. Эм\. Зачем всё это?

![0740_PVS-Studio_for_Linux_and_macOS_ru/image10.png](https://import.viva64.com/docx/blog/0740_PVS-Studio_for_Linux_and_macOS_ru/image10.png)

### Не тот логический оператор

```cpp
private OrderByField ExtractOrderByFromMethod(....)
{
  ....
  if (me.Arguments.Count < 2 && me.Arguments.Count > 3)
    throw new InvalidQueryException(....);
  ....
}
```

**Предупреждение анализатора**: [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'me\.Arguments\.Count < 2 && me\.Arguments\.Count \> 3' is always false\. Probably the '\|\|' operator should be used here\. QueryMetadata\.cs\(861\) Raven\.Server

Полностью данный метод можно посмотреть [здесь](https://github.com/ravendb/ravendb/blob/1f13285cd47b33720e1f5e1de4a0bf2769551ef8/src/Raven.Server/Documents/Queries/QueryMetadata.cs#L779)\.

В этот раз перед нами очевидная ошибка, состоящая в использовании неправильного логического оператора\. В текущем виде проверка количества аргументов попросту не работает, ведь нет такого значения, которое одновременно и меньше 2, и больше 3\. Истинные же намерения разработчика легко раскрываются первым аргументом, передаваемым в конструктор исключения:

```cpp
"Invalid ORDER BY 'spatial.distance(from, to, roundFactor)' call, 
expected 2-3 arguments, got " + me.Arguments.Count
```

Чтобы проверка работала корректно, нужно просто заменить "&&" на "\|\|"\.

### Странный try\-метод

```cpp
private bool Operator(OperatorField fieldOption, out QueryExpression op)
{ 
  ....
  switch (match)
  {
    ....
    case "(":
      var isMethod = Method(field, out var method); // <=
      op = method;

      if (isMethod && Operator(OperatorField.Optional, out var methodOperator))
      {
        ....
      }

      return isMethod;
    ....
  }
}
```

**Предупреждение анализатора**: [V3063](https://pvs-studio.ru/ru/docs/warnings/v3063/) A part of conditional expression is always true if it is evaluated: isMethod\. QueryParser\.cs\(1797\) Raven\.Server

Полностью данный метод можно посмотреть [здесь](https://github.com/ravendb/ravendb/blob/1f13285cd47b33720e1f5e1de4a0bf2769551ef8/src/Raven.Server/Documents/Queries/Parser/QueryParser.cs#L1701)\.

Конструкция _var isMethod \= Method\(field, out var method\) _напомнила мне о стандартных методах вроде _Int\.TryParse_\. Такие методы производят попытку получить результат и записать её в out\-переменную, а флаг успешности проведения операции является возвращаемым значением\. Код, использующий такие функции, обычно производит проверку возвращаемого значения, а затем на её основе выполняет те или иные операции\.

На мой взгляд, функция _Method_ используется здесь именно таким образом\. Результат работы _Method_, кроме того, является возвращаемым значением метода _Operator_, вызывающего её\.

Анализатор же указывает, что переменная _isMethod_ всегда будет иметь значение _true_ и её проверка в условии бессмысленна\. Это означает, что функция _Method_ никогда не возвращает _false_\. Какой же тогда смысл в использовании такой конструкции?

Для начала, убедимся в том, что анализатор не ошибся:

```cpp
private bool Method(FieldExpression field, out MethodExpression op)
{
  var args = ReadMethodArguments();

  op = new MethodExpression(field.FieldValue, args);
  return true;
}
```

Действительно, возвращаемое значение этого метода всегда _true_\. И если всё так и задумывалось, то это\.\.\. странно, но не беда, в принципе\. Но что, если нет?

Функция _ReadMethodArguments_, код которой можно посмотреть [здесь](https://github.com/ravendb/ravendb/blob/1f13285cd47b33720e1f5e1de4a0bf2769551ef8/src/Raven.Server/Documents/Queries/Parser/QueryParser.cs#L1856), выбрасывает исключения в некоторых случаях\. Такое происходит, когда метод не может корректно выполнить свою задачу\.

Создаётся впечатление, что код, вызывающий функцию _Method, _не рассчитан на выбрасывание исключений\. Скорее всего предполагается, что когда корректно выполнить получение значения out\-переменной не удаётся, функция _Method_ вернёт _false_\. Однако при текущей реализации вместо этого будет выброшено исключение\.

Как бы то ни было, стоит обратить внимание на данный фрагмент\.

### null \!\= null?

```cpp
private Address GetNextEdge()
{
  if (m_curEdgeBlock == null || m_curEdgeBlock.Count <= m_curEdgeIdx)
  {
    m_curEdgeBlock = null;
    if (m_edgeBlocks.Count == 0)
    {
      throw new ApplicationException(
        "Error not enough edge data.  Giving up on heap dump."
      );
    }

    var nextEdgeBlock = m_edgeBlocks.Dequeue();
    if (
      m_curEdgeBlock != null &&                       // <=
      nextEdgeBlock.Index != m_curEdgeBlock.Index + 1
    )
    {
      throw new ApplicationException(
        "Error expected Node Index " + (m_curEdgeBlock.Index + 1) + 
        " Got " + nextEdgeBlock.Index + " Giving up on heap dump."
      );
    }

    m_curEdgeBlock = nextEdgeBlock;
    m_curEdgeIdx = 0;
  }
  return m_curEdgeBlock.Values(m_curEdgeIdx++).Target;
}
```

**Предупреждение анализатора**: [V3063](https://pvs-studio.ru/ru/docs/warnings/v3063/) A part of conditional expression is always false if it is evaluated: m\_curEdgeBlock \!\= null\. DotNetHeapDumpGraphReader\.cs\(803\) Raven\.Debug

Переменной присваивается нулевой указатель, а затем через несколько строчек производится проверка на её неравенство _null_\. При этом бессмысленным становится код, производящий проверку _nextEdgeBlock\.Index \!\= m\_curEdgeBlock\.Index \+ 1_\. Кроме того, никогда не произойдёт выбрасывание исключения\.

Логично предположить, что тут что\-то работает не так, как должно, ведь фрагмент выглядит очень странно\. Либо проверка тут вообще не нужна, либо она реализована как\-то неправильно\.

Можно рассмотреть срабатывание и с другой стороны – пойти от обратного\. Попробуем представить ситуацию, при которой это срабатывание является ложным\. Я думаю, что это возможно, только если значение переменной может быть изменено при вызове _Deque_\. Однако _m\_curEdgeBlock_ является приватным полем, а _m\_edgeBlocks_ – это стандартная очередь, которая инициализируется в этом же классе\. Таким образом, весьма сомнительно, что вызов _Dequeue_ как\-то может повлиять на значение _m\_curEdgeBlock_\. Следовательно, срабатывание, скорее всего, не является ложным\.

### First or null

```cpp
public HashSet<string> FindSpecialColumns(string tableSchema, string tableName)
{
  var mainSchema = GetTable(tableSchema, tableName);

  var result = new HashSet<string>();
  mainSchema.PrimaryKeyColumns.ForEach(x => result.Add(x)); // <=

  foreach (var fkCandidate in Tables)
    foreach (var tableReference in fkCandidate.References.Where(
        x => x.Table == tableName && x.Schema == tableSchema
      )
    )
    {
      tableReference.Columns.ForEach(x => result.Add(x));
    }

  return result;
}
```

**Предупреждение анализатора**: [V3146](https://pvs-studio.ru/ru/docs/warnings/v3146/) Possible null dereference of 'mainSchema'\. The 'Tables\.FirstOrDefault' can return default null value\. DatabaseSchema\.cs\(31\) Raven\.Server

Срабатывание, на первый взгляд, может показаться непонятным\. Действительно, при чём тут вообще _FirstOrDefault_? Чтобы стало ясно, отчего же анализатор ругается, необходимо взглянуть на функцию _GetTable_:

```cpp
public TableSchema GetTable(string schema, string tableName)
{
  return Tables.FirstOrDefault(
    x => x.Schema == schema && x.TableName == tableName
  );
}
```

Вызов метода _FirstOrDefault_ вместо _First_ может быть обусловлен тем, что в коллекции может не быть элементов, соответствующих заданному условию\. В таком случае _FirstOrDefault_, а, следовательно, и _GetTable_ вернёт _null_, так как _TableSchema_ – это ссылочный тип\. Именно поэтому PVS\-Studio и говорит о том, что в данном коде может произойти попытка разыменования нулевого указателя\.

Возможно, всё же стоит проверять такой случай, чтобы выполнение не прерывалось с _NullReferenceException_\. Если же вариант, при котором _Tables\.FirstOrDefault _вернёт _null_ невозможен, то тогда нет смысла в использовании _FirstOrDefault_ вместо _First_\.

### Always true

```cpp
public override void VerifyCanExecuteCommand(
  ServerStore store, TransactionOperationContext context, bool isClusterAdmin
)
{
  using (context.OpenReadTransaction())
  {
    var read = store.Cluster.GetCertificateByThumbprint(context, Name);
    if (read == null)
      return;

    var definition = JsonDeserializationServer.CertificateDefinition(read);
    if (
      definition.SecurityClearance != SecurityClearance.ClusterAdmin || // <=
      definition.SecurityClearance != SecurityClearance.ClusterNode     // <=
    )
      return;
  }

  AssertClusterAdmin(isClusterAdmin);
}
```

**Предупреждение анализатора**: [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression is always true\. Probably the '&&' operator should be used here\. DeleteCertificateFromClusterCommand\.cs\(21\) Raven\.Server

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

Я полагаю, что "\|\|" должен быть заменён на "&&"\. Тогда в написанном будет смысл\. Если логический оператор всё же выбран верно, то скорее всего в одном из условий должно происходить сравнение других переменных\. Так или иначе, этот фрагмент очень подозрительно выглядит и его обязательно нужно проанализировать\.

## Заключение

В первую очередь хочу поблагодарить всех, кто добрался до этого места\. Статья вышла достаточно объёмная, но, надеюсь, вам было интересно вместе со мной разбираться с новой версией анализатора PVS\-Studio и изучать найденные ошибки\.

Важно помнить, что разработчик должен основной своей целью ставить вовсе не уменьшение количества срабатываний\. Не ради достижения пустого лога ошибок нужно использовать PVS\-Studio\. Бороться со срабатываниями – всё равно что бороться с симптомами болезни, от которой страдает исходный код\.

Рассматривая сообщения анализатора, всегда нужно стараться понять, почему выдаётся то или иное предупреждение\. Лишь осознав логику, по которой анализатор выдал предупреждение, можно сделать вывод о том, указывает оно на ошибку или нет\. Именно в этом случае вы будете бороться не с симптомом, а с болезнью\. И именно таким образом ваш код станет чище и здоровее\. И конечно же, проблем с таким прекрасным исходником будет намного меньше\. Хотя лучше я пожелаю вам, чтобы их не было вообще :\)

![0740_PVS-Studio_for_Linux_and_macOS_ru/image11.png](https://import.viva64.com/docx/blog/0740_PVS-Studio_for_Linux_and_macOS_ru/image11.png)