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

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

Введение

Некоторое время назад мой коллега Сергей Васильев написал заметку об открытии бета-теста новой версии разрабатываемого нами статического анализатора PVS-Studio. Сейчас бета-тест завершён и новую версию можно загрузить, перейдя по ссылке. В этой же статье мы рассмотрим, как проводится анализ C# проектов под Linux/macOS с использованием консольного интерфейса и Rider. После этого проведём традиционный разбор некоторых интересных срабатываний анализатора.

RavenDB

Для проверки был выбран открытый проект RavenDB, репозиторий которого насчитывает почти 5 тысяч файлов исходного кода. Он представляет собой достаточно популярную NoSQL-базу данных. Подробности можно узнать на сайте. Нетрудно догадаться, что меня привлёк его размер, ведь в столь большом и, вне всяких сомнений, серьёзном проекте наверняка найдётся что-нибудь интересное.

Command Line Interface

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

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

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

Rider

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

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

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

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

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

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

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

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

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

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

А на десерт

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

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

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

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

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

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

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 Expression 'dirpath == null' is always false. PosixHelper.cs(124) Voron

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

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

Быть может, break должен вызываться, когда не удаётся получить родителя для рассматриваемой директории. Если это так, то проверка на null обретает смысл. Однако рассматривать нужно не результат работы ToString, а значение, возвращаемое методом GetParent:

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

dirpath = dir.ToString();

В противном случае, возвращение null методом GetParent приводит к исключению при вызове ToString.

Типичный null

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 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, тоже помечает данный фрагмент как потенциально опасный.

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

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 Expression '_continuationState.Count > 0' is always true. ManualBlittableJsonDocumentBuilder.cs(152) Sparrow

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

Возможный NRE

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 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. Только в таком случае можно считать данное срабатывание обоих анализаторов ложным. Поэтому стоит внимательно проанализировать все возможные варианты.

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

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 Expression 'command != null' is always true. AsyncDocumentSession.Load.cs(175) Raven.Client

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

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

  return new GetDocumentsCommand(....);
}

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

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

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

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

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

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

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

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 Two similar code fragments were found. Perhaps, this is a typo. SmugglerResult.cs(256), SmugglerResult.cs(253) Raven.Client

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

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

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

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

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

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

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

Предупреждение анализатора: 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.

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

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

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

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

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

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

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

Предупреждение анализатора: V3066 Possible incorrect order of arguments passed to 'ValidateLicense' method: 'newLicense' and 'oldLicense'. LicenseHelper.cs(177) Raven.Server

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

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

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

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

Never continue

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 Expression 'GetCounterValueAndCheckIfShouldSkip(item.DocumentId, null, prop, out long value, out bool delete)' is always false. RavenEtlDocumentTransformer.cs(362) Raven.Server

Полностью данный метод можно рассмотреть, перейдя по ссылке.

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

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

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 действительно недостижим.

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

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

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 A part of conditional expression is always true if it is evaluated: Attributes != null. LicenseStatus.cs(28) Raven.Server

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

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

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 Expression 'suspend.HasValue' is always true. RachisAdminHandler.cs(116) Raven.Server

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

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

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 не считал ту проверку лишней. Неужели Единорог подвёл?

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

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. Интересно, что же он делает? :) Ну, чтобы уж точно не осталось сомнений:

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, на фрагмент которого и ругался анализатор:

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. Тогда всё действительно будет работать так, как ожидается.

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

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

Странности

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 Parameter 'numberOfResults' is always rewritten in method body before being used. DocumentHandler.cs(273), DocumentHandler.cs(267) Raven.Server

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

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

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

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

  ....
}

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

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

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

Предупреждение анализатора: 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

Полностью данный метод можно посмотреть здесь.

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

"Invalid ORDER BY 'spatial.distance(from, to, roundFactor)' call, 
expected 2-3 arguments, got " + me.Arguments.Count
Чтобы проверка работала корректно, нужно просто заменить "&&" на "||".

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

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 A part of conditional expression is always true if it is evaluated: isMethod. QueryParser.cs(1797) Raven.Server

Полностью данный метод можно посмотреть здесь.

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

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

Анализатор же указывает, что переменная isMethod всегда будет иметь значение true и её проверка в условии бессмысленна. Это означает, что функция Method никогда не возвращает false. Какой же тогда смысл в использовании такой конструкции?

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

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

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

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

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

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

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

null != null?

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 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

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 Possible null dereference of 'mainSchema'. The 'Tables.FirstOrDefault' can return default null value. DatabaseSchema.cs(31) Raven.Server

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

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

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 Expression is always true. Probably the '&&' operator should be used here. DeleteCertificateFromClusterCommand.cs(21) Raven.Server

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

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

Заключение

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

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

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

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Nikita Lipilin. How to find errors in a C# project working under Linux and macOS.