Проверяем osu! и рассказываем про фишки статических анализаторов

• Введение
• Экономит время
• Глубокого копает
• Знает особенности методов
• Помогает программе работать корректно
• Позволяет не беспокоиться о копипастах
• Может найти то, о чём вы не знали
• Всегда прав (нет)
• Заключение

Про существование инструментов статического анализа известно многим, но почему их часто используют и в чём конкретно заключается практическая польза? В этот раз мы предлагаем рассмотреть несколько основных особенностей этого инструмента на примере анализа исходного кода игры osu!

Введение

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

Для того, чтобы вам не приходилось верить мне на слово, предлагаю рассмотреть их на примере анализа исходного кода известного проекта osu!. Это одна из самых известных open source ритм игр с огромным и крутым сообществом. Так что можно было сразу готовиться к тому, что код будет качественным. Но анализатор PVS-Studio все равно нашёл потенциальные проблемы в коде, и на их примере я хочу показать вам, на что способны инструменты статического анализа.

Код проекта доступен на GitHub (анализ проводился на коммите 094f703).

Давайте не будем ходить вокруг да около и перейдём к разбору основных фич статических анализаторов.

Экономит время

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

Предлагаю начать с небольшой разминки: сможете ли вы самостоятельно найти ошибку?

public partial class TopScoreStatisticsSection
  : CompositeDrawable
{ 
  public ScoreInfo Score
  {
    ....

    if (score == null && value == null) 
      return;

    if (score?.Equals(value) == true)
      return;

    score = value;

    accuracyColumn.Text = value.DisplayAccuracy;

    maxComboColumn.Text = value.MaxCombo
                               .ToLocalisableString(@"0\x");

    ppColumn.Alpha = value.BeatmapInfo!
                          .Status
                          .GrantsPerformancePoints() ? 1 : 0;

   
  }
}

Нашли? Ну я в вас и не сомневался :)

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

В начале есть две проверки.

Первая проверка:

if (score == null && value == null)
  return;

Вторая проверка:

if (score?.Equals(value) == true)
  return;

Скорее всего, они предназначались для обработки двух переменных по разным сценариям (если score = null, если value = null, если они равны и т. д.). Но вот если комбинация будет score = "NotNull" и value = null, то первая и вторая проверки отработают без выхода из метода, и мы пойдём дальше по коду, где непременно наткнёмся на разыменовывание свежеполученного null

accuracyColumn.Text = value.DisplayAccuracy;
maxComboColumn.Text = value.MaxCombo.ToLocalisableString(@"0\x");

А это, в свою очередь, может привести к исключению NullReferenceException.

Глубокого копает

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

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

Давайте рассмотрим пример подобной ошибки в проекте osu!.

Файл SaveFailedScoreButton.cs:

public partial class SaveFailedScoreButton 
  : CompositeDrawable, IKeyBindingHandler<GlobalAction>
{
  private void load(OsuGame? game, Player? player)
  {
    ....
    switch (state.Value)
    {
      case DownloadState.LocallyAvailable:
        game?.PresentScore(importedScore?.Value,   
                           ScorePresentType.Gameplay);
        break;

      case DownloadState.NotDownloaded:
        state.Value = DownloadState.Importing;

        if (importFailedScore != null){....}
        
        break;
    }
  }
}

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

Анализатор подсказывает, что проблема в методе PresentScore, а именно в первом его аргументе, который передаётся как "возможный null":

game?.PresentScore(importedScore?.Value, ScorePresentType.Gameplay);

Ну давайте узнаем, в чём проблема, и пойдём внутрь метода PresentScore:

Файл OsuGame.cs:

public void PresentScore(IScoreInfo score, ....)
{
  ....

  try
  {
    databasedScore = ScoreManager.GetScore(score);
  }
}

Так... Похоже, это не всё. Наш первый аргумент идёт в метод GetScore (проверок на null мы пока не видим), так что смотрим дальше.

Файл ScoreManager.cs:

public Score? GetScore(IScoreInfo scoreInfo)
{
  ScoreInfo? databasedScoreInfo = getDatabasedScoreInfo(scoreInfo);
  ....
}

Всё ещё не конец, всё ещё нет проверки. Копаем дальше. Теперь идём в метод getDatabasedScoreInfo.

Файл ScoreManager.cs:

private ScoreInfo? getDatabasedScoreInfo(IScoreInfo originalScoreInfo)
{
  ....
  if (originalScoreInfo is ScoreInfo scoreInfo)
  {
    ....
  }

  if (originalScoreInfo.OnlineID > 0)
    databasedScoreInfo ??= Query
      (s => s.OnlineID == originalScoreInfo.OnlineID);

  if (originalScoreInfo.LegacyOnlineID > 0)
    databasedScoreInfo ??= Query
      (s => s.LegacyOnlineID == originalScoreInfo.LegacyOnlineID);
}

О, попался! Мы дошли с потенциальным null до момента, где его разыменовывают, как мы могли заметить, без проверки. В итоге опять можем получить NRE...

Подобную ошибку будет не так просто отследить на код-ревью (если вы, конечно, хотите его закончить раньше, чем за пару дней), а до тестов на такие специфичные проблемы ещё нужно дойти самому. Причём проверок было много и разных, но именно возможный null не проверяли. Облегчите себе работу и попробуйте для этих нужд воспользоваться статическим анализатором кода :)

(P.S. например PVS-Studio)

Знает особенности методов

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

Для примера хочу показать ошибку из недавней статьи про проверку кода трейдерского движка Lean:

public virtual DateTime NextDate(DateTime minDateTime, 
                                 DateTime maxDateTime, 
                                 DayOfWeek? dayOfWeek)
{
  ....

  // both are valid dates, so chose one randomly
  if (IsWithinRange(nextDayOfWeek, minDateTime, maxDateTime) &&
        IsWithinRange(previousDayOfWeek, minDateTime, maxDateTime))
  {
    return _random.Next(0, 1) == 0
        ? previousDayOfWeek
        : nextDayOfWeek;
  }
  ....  
}

Разработчик хотел, чтобы одно из двух значений возвращалось рандомно, но в методе Next второе значение не входит в диапазон возвращаемых, поэтому запись вида random.Next(0, 1) всегда будет возвращать 0.

А теперь разберём пример из osu!, в котором разработчики могли не учесть все сценарии при работе с индексами. Рассмотрим код:

private void deleteDifficulty()
{
  ....
  void delete()
  {
    BeatmapInfo difficultyToDelete = 
      playableBeatmap.BeatmapInfo;

    var difficultiesBeforeDeletion = 
      groupedOrderedBeatmaps.SelectMany(g => g).ToList();

    ....

    int deletedIndex = difficultiesBeforeDeletion.IndexOf(difficultyToDelete);

    BeatmapInfo nextToShow = 
      difficultiesBeforeDeletion
        [deletedIndex == 0 ? 1 : deletedIndex - 1];

    Beatmap.Value = beatmapManager.GetWorkingBeatmap(nextToShow);

    SwitchToDifficulty(nextToShow);
    }
}

Давайте сразу взглянем на предупреждение:

По мнению анализатора есть возможность получить индекс -2. Не будем ходить вокруг да около и сразу перейдём к причине: метод IndexOf (откуда мы как раз получаем deletedIndex) может вернуть -1.

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

Помогает программе работать корректно

Одна из особенностей некоторых ошибок — неочевидные последствия. Давайте разберём пример потенциальной ошибки от относительно нового диагностического правила:

public class SampleInfo : ISampleInfo, IEquatable<SampleInfo>
{
  ....
  public override int GetHashCode()
  {
    return HashCode.Combine(
               StructuralComparisons.StructuralEqualityComparer
                                    .GetHashCode(sampleNames)
                                    , Volume);
  }

  public bool Equals(SampleInfo? other)
    => other != null && sampleNames.SequenceEqual(other.sampleNames);

  public override bool Equals(object? obj)
    => obj is SampleInfo other && Equals(other);
}

Анализатор указывает, что один из членов класса не используется в методе Equals, но при этом используется в методе GetHashCode. В нашем случае это поле Volume. Но в чём ошибка?

Это может привести к проблеме: метод GetHashCode может возвращать разные значения для двух эквивалентных объектов.

Исходя из документации Microsoft, метод GetHashCode должен возвращать одинаковый хэш-код для любых двух объектов, для которых вызов Equals возвращает True. Также подобная реализация может негативно сказаться на корректности работы с коллекциями Hashtable, Dictionary<TKey,TValue> и другими.

Позволяет не беспокоиться о копипастах

Одна из частых ошибок, которую находят анализаторы, — это копипасты. У них есть особенность, из-за которой их легко пропустить даже при тщательном обзоре кода: они возникают в больших и однотипных функциях: проверки, сравнение, switch-кейсы и многое другое. И во время обзора кода разработчики не нарочно пропускают их. Давайте рассмотрим пример подобной ошибки:

private void onRoomPropertyChanged(object? sender, 
                                   PropertyChangedEventArgs e)
{
  switch (e.PropertyName)
  {
      case nameof(Room.Name):
          updateRoomName();
          break;

      case nameof(Room.Type):
          updateRoomName();
          break;

      case nameof(Room.QueueMode):
          updateRoomQueueMode();
          break;

      case nameof(Room.Password):
          updateRoomPassword();
          break;
      ....
    }
}

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

Вполне очевидная ошибка, связанная с копипастой case-секций, где забыли поменять тело кейса Room.Type, и оно стало как у предыдущего кейса Room.Name. Повезло, что опечатка оказалась в самом начале функции, и её не так сложно заметить, но она всё равно оказалась в программе.

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

Потому что это ещё не всё...

protected void SetupZoom(float initial, float minimum, float maximum)
{
    if (minimum < 1)
        throw new ArgumentException($"{nameof(minimum)} 
          ({minimum}) must be >= 1.", nameof(maximum));

    if (maximum < 1)
        throw new ArgumentException($"{nameof(maximum)}
          ({maximum}) must be >= 1.", nameof(maximum));

    if (minimum > maximum)
        throw new ArgumentException($"{nameof(minimum)}
          ({minimum}) must be less than {nameof(maximum)} 
                                       ({maximum})");
}

Здесь не менее забавная ошибка. Скорее всего, разработчик в первом случае перепутал переменные minimum и maximum.

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

Может найти то, о чём вы не знали

Ещё одна частая причина ошибок — простое незнание. Это нормально, и любой человек может столкнуться с этим.

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

Рассмотрим пример:

public partial class TestSceneBeatmapDifficultyCache
{
  ....
  AddStep(....)
  {
    var modRateAdjust = (ModRateAdjust)lookup.OrderedMods.SingleOrDefault(....);
    return new StarDifficulty
     (BASE_STARS + modRateAdjust?.SpeedChange.Value ?? 0, 0);
  }
}

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

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

BASE_STARS + (modRateAdjust?.SpeedChange.Value ?? 0)

Но мог ошибиться, не приняв во внимание особенности работы оператора ??, и получил такую:

(BASE_STARS + modRateAdjust?.SpeedChange.Value) ?? 0

Но почему именно так, а не иначе? Вдруг разработчик так и задумал...

Тут уже вступает в работу стандартная практика работы со статическим анализатором кода — разбор ошибки. Если взглянуть на код более подробно, а не только на отрывок, отмеченный анализатором (что и требуется сделать при изучении потенциальной ошибки), мы сможем обнаружить важный контекст:

public partial class TestSceneBeatmapDifficultyCache
{
  public const double BASE_STARS = 5.55;
  ....
  AddStep(....)
  {
    var modRateAdjust = (ModRateAdjust)lookup.OrderedMods.SingleOrDefault(....);
    return new StarDifficulty
     (BASE_STARS + modRateAdjust?.SpeedChange.Value ?? 0, 0);
  }
  ....
  AddUntilStep($"star difficulty -> {BASE_STARS + 1.5}", 
    () =>   starDifficultyBindable.Value.Stars == BASE_STARS + 1.5);
  ....
  AddUntilStep($"star difficulty -> {BASE_STARS + 1.25}", 
    () =>   starDifficultyBindable.Value.Stars == BASE_STARS + 1.25);
  ....
  AddUntilStep($"star difficulty -> {BASE_STARS + 1.75}", 
    () =>   starDifficultyBindable.Value.Stars == BASE_STARS + 1.75);

}

Видим, что BASE_STARS является константой и подобного паттерна, как в потенциальной ошибке, дальше тоже нет. Было бы странно обнулять всё значение, включая BASE_STARS, а не modRateAdjust?.SpeedChange.Value в случае, если modRateAdjust будет null.

До этого я упомянул документацию диагностики, и это тоже одна из сильных сторон анализатора PVS-Studio. Документация достаточно обширная и охватывает все сценарии взаимодействия с анализатором, в том числе и диагностические правила. Внутри можно найти описание проблемы, пример ошибочного кода, исправления и различные дополнения (паттерн потенциальной ошибки по CWE, примеры ошибок из других проектов и т. д.).

А ещё документация открывается прямо в IDE:

Всегда прав (нет)

Чтобы не создавать ложное впечатление о том, что это не просто инструмент, а "волшебная палочка" для кода, которая избавит от всех проблем, давайте поговорим о минусах :)

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

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

Разберём на примере сообщения, аналогичного предыдущему (с номером диагностики V3123) из того же проекта:

public class SamePatternsGroupedHitObjects
{
  public IReadOnlyList<SameRhythmHitObjectGrouping> Groups { get; }

  public SamePatternsGroupedHitObjects? Previous { get; }

  public double GroupInterval => 
    Groups.Count > 1 ? Groups[1].Interval :   Groups[0].Interval;

  public double IntervalRatio => 
    GroupInterval / Previous?.GroupInterval ?? 1.0d;
}

С первого взгляда сложно сказать, есть ли тут ошибка, о которой говорит нам анализатор. Но все встаёт на свои места, если посмотреть комментарий к этому коду:

public class SamePatternsGroupedHitObjects
{
  ....

  /// The ratio of GroupInterval between this 
and the previous SamePatternsGroupedHitObjects. 
  ///In the case where there is no previous 
SamePatternsGroupedHitObjects, this will have a value of 1.

  public double IntervalRatio => 
    GroupInterval / Previous?.GroupInterval ?? 1.0d;
}

Тут явно прописано, что ожидается от кода, и мы можем понять, что же именно хотел разработчик. Если Previous?.GroupInterval отсутствует (null), то все выражение будет 1. Если добавить скобочки, то так:

public double IntervalRatio => 
  (GroupInterval / Previous?.GroupInterval) ?? 1.0d;

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

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

И что делать? Игнорировать срабатывание? Конечно же нет! Мы берём и отмечаем его ложным, нажав правую кнопку мыши по сообщению анализатора и выбрав первый пункт в меню:

Этот механизм не удаляет срабатывание из списка сообщений, а переносит его в категорию "ложных" (False Alarms).

Можно ещё воспользоваться механизмом массового подавления (suppression files). Он полезен в нескольких сценариях, к примеру, когда вы только внедрили статический анализатор в свой проект и при первоначальном анализе он выдал много срабатываний. Вы можете их подавить, поместив в "технический долг", чтобы вернуться к ним позже. Таким образом анализатор о них не забудет, но при этом они не помешают анализировать новый код (не будут засорять отчёт анализатора при последующих запусках).

Заключение

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

Если вам интересно, что ещё нашёл PVS-Studio в osu! (а мы рассмотрели лишь малу часть ошибок, в отчёте их более 1000) или вы хотите проверить свой собственный проект, то скачать и попробовать анализатор можно по ссылке.