# В "osu\!" играй, про ошибки не забывай

Приветствуем всех любителей экзотических и не очень ошибок в коде\. Сегодня на тестовом стенде PVS\-Studio достаточно редкий гость – игра на языке C\#\. А именно – "osu\!"\. Как обычно: ищем ошибки, думаем, играем\.

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

## Игра

Osu\! – музыкальная ритм\-игра с открытым исходным кодом\. Судя по информации с [сайта игры](https://osu.ppy.sh/home) – довольно популярная, так как заявлено более 15 миллионов зарегистрированных игроков\. Проект характеризуют бесплатный геймплей, красочное оформление с возможностью кастомизации карт, продвинутые возможности для составления онлайн\-рейтинга игроков, наличие мультиплеера, большой набор музыкальных композиций\. Подробно описывать игру не буду, интересующиеся легко найдут всю информацию в сети\. Например, [тут](https://en.wikipedia.org/wiki/Osu!)\.

Мне больше интересен исходный код проекта, который доступен для загрузки с [GitHub](https://github.com/ppy/osu)\. Сразу привлекает внимание значительное число коммитов \(более 24 тысяч\) в репозиторий, что говорит об активном развитии проекта, которое продолжается и в настоящее время \(игра была выпущена в 2007 году, но работы, вероятно, были начаты раньше\)\. При этом исходного кода не так много – 1813 файлов \.cs, которые содержат 135 тысяч строк кода без учёта пустых\. В этом коде присутствуют тесты, которые я обычно не учитываю в проверках\. Код тестов содержится в 306 файлах \.cs и, соответственно, 25 тысячах строк кода без учёта пустых\. Это маленький проект: для сравнения, ядро C\# анализатора PVS\-Studio содержит около 300 тысяч строк кода\.

Итого, для проверки игры на ошибки я использовал не тестовые проекты, содержащие 1507 файлов исходного кода и 110 тысяч строк\. Однако результат меня отчасти порадовал, так как нашлось несколько интересных ошибок, о которых я спешу вам рассказать\.

## Ошибки

[V3001](https://pvs-studio.ru/ru/docs/warnings/v3001/) There are identical sub\-expressions 'result \=\= HitResult\.Perfect' to the left and to the right of the '\|\|' operator\. DrawableHoldNote\.cs 266

```cpp
protected override void CheckForResult(....)
{
  ....
  ApplyResult(r =>
  {
    if (holdNote.hasBroken
      && (result == HitResult.Perfect || result == HitResult.Perfect))
      result = HitResult.Good;
    ....
  });
}
```

Хороший пример копипаст\-ориентированного программирования\. Шуточный термин, который недавно использовал \(ввёл\) мой коллега Валерий Комаров в своей статье "[Топ 10 ошибок в проектах Java за 2019 год](https://pvs-studio.ru/ru/blog/posts/java/0699/)"\.

Итак, две идентичных проверки следуют одна за другой\. Одна из проверок, скорее всего, должна содержать другую константу перечисления _HitResult_:

```cpp
public enum HitResult
{
    None,
    Miss,
    Meh,
    Ok,
    Good,
    Great,
    Perfect,
}
```

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

[V3001](https://pvs-studio.ru/ru/docs/warnings/v3001/) There are identical sub\-expressions 'family \!\= GetFamilyString\(TournamentTypeface\.Aquatico\)' to the left and to the right of the '&&' operator\. TournamentFont\.cs 64

```cpp
public static string GetWeightString(string family, FontWeight weight)
{
  ....
  if (weight == FontWeight.Regular
    && family != GetFamilyString(TournamentTypeface.Aquatico)
    && family != GetFamilyString(TournamentTypeface.Aquatico))
    weightString = string.Empty;
  ....
}
```

И снова copy\-paste\. Я отформатировал код, поэтому ошибка легко заметна\. В первоначальном варианте всё условие было записано одной строкой\. Здесь также трудно сказать, каким образом можно исправить код\. Перечисление _TournamentTypeface_ содержит единственную константу:

```cpp
public enum TournamentTypeface
{
  Aquatico
}
```

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

[V3009](https://pvs-studio.ru/ru/docs/warnings/v3009/) \[CWE\-393\] It's odd that this method always returns one and the same value of 'false'\. KeyCounterAction\.cs 19

```cpp
public bool OnPressed(T action, bool forwards)
{
  if (!EqualityComparer<T>.Default.Equals(action, Action))
    return false;

  IsLit = true;
  if (forwards)
    Increment();
  return false;
}
```

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

```cpp
public bool OnPressed(T action) =>
  Target.Children
    .OfType<KeyCounterAction<T>>()
    .Any(c => c.OnPressed(action, Clock.Rate >= 0));
```

Как видим, результат, возвращаемый методом _OnPressed_, используется\. И так как он всегда _false_, то и результат вызывающего _OnPressed_ также будет всегда _false_\. Думаю, следует лишний раз перепроверить этот код, так как высока вероятность ошибки\.

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

* V3009 \[CWE\-393\] It's odd that this method always returns one and the same value of 'false'\. KeyCounterAction\.cs 30

[V3042](https://pvs-studio.ru/ru/docs/warnings/v3042/) \[CWE\-476\] Possible NullReferenceException\. The '?\.' and '\.' operators are used for accessing members of the 'val\.NewValue' object TournamentTeam\.cs 41

```cpp
public TournamentTeam()
{
  Acronym.ValueChanged += val =>
  {
    if (....)
      FlagName.Value = val.NewValue.Length >= 2    // <=
        ? val.NewValue?.Substring(0, 2).ToUpper()
        : string.Empty;
  };
  ....
}
```

В условии оператора _?:_ с переменной _val\.NewValue_ работают небезопасно\. Анализатор сделал такой вывод, так как далее в then\-ветке для доступа к переменной используют безопасный вариант работы через оператор условного доступа _val\.NewValue?\.Substring\(\.\.\.\.\)_\.

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

* V3042 \[CWE\-476\] Possible NullReferenceException\. The '?\.' and '\.' operators are used for accessing members of the 'val\.NewValue' object TournamentTeam\.cs 48

[V3042](https://pvs-studio.ru/ru/docs/warnings/v3042/) \[CWE\-476\] Possible NullReferenceException\. The '?\.' and '\.' operators are used for accessing members of the 'api' object SetupScreen\.cs 77

```cpp
private void reload()
{
  ....
  new ActionableInfo
  {
    Label = "Current User",
    ButtonText = "Change Login",
    Action = () =>
    {
      api.Logout();    // <=
      ....
    },
    Value = api?.LocalUser.Value.Username,
    ....
  },
  ....
}

private class ActionableInfo : LabelledDrawable<Drawable>
{
  ....
  public Action Action;
  ....
}
```

Данный код менее однозначен, но я думаю, что ошибка тут всё же присутствует\. Создают объект типа _ActionableInfo_\. Поле _Action_ инициализируют лямбдой, в теле которой небезопасно работают с потенциально нулевой ссылкой _api_\. Анализатор посчитал такой паттерн ошибкой, так как далее при инициализации параметра _Value_ с переменной _api_ работают безопасно\. Ошибку я назвал неоднозначной, потому что код лямбды предполагает отложенное выполнение и тогда, возможно, разработчик как\-то гарантирует ненулевое значение ссылки _api_\. Но это только предположение, так как тело лямбды не содержит никаких признаков безопасной работы со ссылкой \(предварительных проверок, например\)\.

[V3066](https://pvs-studio.ru/ru/docs/warnings/v3066/) \[CWE\-683\] Possible incorrect order of arguments passed to 'Atan2' method: 'diff\.X' and 'diff\.Y'\. SliderBall\.cs 182

```cpp
public void UpdateProgress(double completionProgress)
{
  ....
  Rotation = -90 + (float)(-Math.Atan2(diff.X, diff.Y) * 180 / Math.PI);
  ....
}
```

Анализатор заподозрил, что при работе с методом _Atan2_ класса _Math_ разработчик перепутал порядок следования аргументов\. Объявление _Atan2_:

```cpp
// Parameters:
//   y:
//     The y coordinate of a point.
//
//   x:
//     The x coordinate of a point.
public static double Atan2(double y, double x);
```

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

[V3080](https://pvs-studio.ru/ru/docs/warnings/v3080/) \[CWE\-476\] Possible null dereference\. Consider inspecting 'Beatmap'\. WorkingBeatmap\.cs 57

```cpp
protected virtual Track GetVirtualTrack()
{
  ....
  var lastObject = Beatmap.HitObjects.LastOrDefault();
  ....
}
```

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

```cpp
public IBeatmap Beatmap
{
  get
  {
    try
    {
      return LoadBeatmapAsync().Result;
    }
    catch (TaskCanceledException)
    {
      return null;
    }
  }
}
```

Ну что же, анализатор прав\.

Подробнее про то, как PVS\-Studio находит такие ошибки, а также о нововведениях C\# 8\.0, связанных с подобной тематикой \(работа с потенциально нулевыми ссылками\), можно узнать из статьи "[Nullable Reference типы в C\# 8\.0 и статический анализ](https://pvs-studio.ru/ru/blog/posts/csharp/0631/)"\.

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

```cpp
private List<T> convertHitObjects(....)
{
  ....
  if (ObjectConverted != null)
  {
    converted = converted.ToList();
    ObjectConverted.Invoke(obj, converted);
  }
  ....
}
```

Некритичная и довольно часто встречающаяся ошибка\. Между проверкой события на равенство _null_ и его инвокацией, от события могут отписаться, что приведет к падению программы\. Один из вариантов исправления:

```cpp
private List<T> convertHitObjects(....)
{
  ....
  converted = converted.ToList();
  ObjectConverted?.Invoke(obj, converted);
  ....
}
```

[V3095](https://pvs-studio.ru/ru/docs/warnings/v3095/) \[CWE\-476\] The 'columns' object was used before it was verified against null\. Check lines: 141, 142\. SquareGraph\.cs 141

```cpp
private void redrawProgress()
{
  for (int i = 0; i < ColumnCount; i++)
    columns[i].State = i <= progress ? ColumnState.Lit : ColumnState.Dimmed;
  columns?.ForceRedraw();
}
```

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

[V3119](https://pvs-studio.ru/ru/docs/warnings/v3119/) Calling overridden event 'OnNewResult' may lead to unpredictable behavior\. Consider implementing event accessors explicitly or use 'sealed' keyword\. DrawableRuleset\.cs 256

```cpp
private void addHitObject(TObject hitObject)
{
  ....
  drawableObject.OnNewResult += (_, r) => OnNewResult?.Invoke(r);
  ....
}

public override event Action<JudgementResult> OnNewResult;
```

Анализатор предупреждает об опасности использования переопределённого или виртуального события\. В чём именно заключается опасность – предлагаю почитать в [описании](https://pvs-studio.ru/ru/docs/warnings/v3119/) к диагностике\. Также в своё время я писал на эту тему статью "[Виртуальные события в C\#: что\-то пошло не так](https://pvs-studio.ru/ru/blog/posts/csharp/0453/)"\.

Ещё одна подобная небезопасная конструкция в коде:

* V3119 Calling an overridden event may lead to unpredictable behavior\. Consider implementing event accessors explicitly or use 'sealed' keyword\. DrawableRuleset\.cs 257

[V3123](https://pvs-studio.ru/ru/docs/warnings/v3123/) \[CWE\-783\] 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\. OsuScreenStack\.cs 45

```cpp
private void onScreenChange(IScreen prev, IScreen next)
{
  parallaxContainer.ParallaxAmount =
    ParallaxContainer.DEFAULT_PARALLAX_AMOUNT *
      ((IOsuScreen)next)?.BackgroundParallaxAmount ?? 1.0f;
}
```

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

```cpp
x = (c * a) ?? b;
```

Ошибка была допущена вследствие того, что оператор "\*" имеет более высокий приоритет, чем оператор "??"\. Исправленный вариант кода \(добавлены скобки\):

```cpp
private void onScreenChange(IScreen prev, IScreen next)
{
  parallaxContainer.ParallaxAmount =
    ParallaxContainer.DEFAULT_PARALLAX_AMOUNT *
      (((IOsuScreen)next)?.BackgroundParallaxAmount ?? 1.0f);
}
```

Ещё одна подобная ошибка в коде:

[V3123](https://pvs-studio.ru/ru/docs/warnings/v3123/) \[CWE\-783\] 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\. FramedReplayInputHandler\.cs 103

```cpp
private bool inImportantSection
{
  get
  {
    ....
    return IsImportant(frame) &&
      Math.Abs(CurrentTime - NextFrame?.Time ?? 0) <= 
        AllowedImportantTimeSpan;
  }
}
```

Здесь, как и в предыдущем фрагменте кода, не учли приоритет операторов\. Сейчас выражение, передаваемое в метод _Math\.Abs_, вычисляется так:

```cpp
(a – b) ?? 0
```

Исправленный код:

```cpp
private bool inImportantSection
{
  get
  {
    ....
    return IsImportant(frame) &&
      Math.Abs(CurrentTime – (NextFrame?.Time ?? 0)) <= 
        AllowedImportantTimeSpan;
  }
}
```

[V3142](https://pvs-studio.ru/ru/docs/warnings/v3142/) \[CWE\-561\] Unreachable code detected\. It is possible that an error is present\. DrawableHoldNote\.cs 214

```cpp
public override bool OnPressed(ManiaAction action)
{
  if (!base.OnPressed(action))
    return false;

  if (Result.Type == HitResult.Miss)  // <=
    holdNote.hasBroken = true;
  ....
}
```

Анализатор утверждает, что код обработчика _OnPressed_, начиная со второго оператора _if_, является недостижимым\. Это следует из предположения, что первое условие всегда истинно, то есть метод _base\.OnPressed_ всегда вернет _false_\. Взглянем на метод _base\.OnPressed_:

```cpp
public virtual bool OnPressed(ManiaAction action)
{
  if (action != Action.Value)
    return false;
  
  return UpdateResult(true);
}
```

Переходим далее к методу _UpdateResult_:

```cpp
protected bool UpdateResult(bool userTriggered)
{
  if (Time.Elapsed < 0)
    return false;

  if (Judged)
    return false;

  ....

  return Judged;
}
```

Обратите внимание, реализация свойства _Judged_ здесь не важна, так как из логики метода _UpdateResult_ следует, что последний оператор _return_ эквивалентен такому:

```cpp
return false;
```

Таким образом, метод _UpdateResult_ всегда вернет _false_, что и приведет к возникновению ошибки с недостижимым кодом в коде выше по стеку\.

[V3146](https://pvs-studio.ru/ru/docs/warnings/v3146/) \[CWE\-476\] Possible null dereference of 'ruleset'\. The 'FirstOrDefault' can return default null value\. APILegacyScoreInfo\.cs 24

```cpp
public ScoreInfo CreateScoreInfo(RulesetStore rulesets)
{
  var ruleset = rulesets.GetRuleset(OnlineRulesetID);

  var mods = Mods != null ? ruleset.CreateInstance()          // <=
                                   .GetAllMods().Where(....)
                                   .ToArray() : Array.Empty<Mod>();
  ....
}
```

Анализатор считает небезопасным вызов _ruleset\.CreateInstance\(\)_\. Переменная _ruleset_ ранее получает значение в результате вызова _GetRuleset_:

```cpp
public RulesetInfo GetRuleset(int id) =>
  AvailableRulesets.FirstOrDefault(....);
```

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

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

В целом проект игры "osu\!" порадовал небольшим числом ошибок\. Тем не менее, я рекомендую разработчикам обратить внимание на обнаруженные проблемы\. И пусть игра и далее радует своих поклонников\. 

А для любителей поковыряться в коде напоминаю, что хорошим подспорьем будет анализатор PVS\-Studio, который легко [скачать](https://pvs-studio.ru/ru/pvs-studio/download/) с официального сайта\. Также замечу, что разовые проверки проектов, подобные описанной выше, не имеют ничего общего с использованием статического анализатора в реальной работе\. Максимальной эффективности в борьбе с ошибками можно добиться лишь при регулярном использовании инструмента как на сборочном сервере, так и непосредственно на компьютере разработчика \(инкрементальный анализ\)\. При этом задача максимум – вовсе не допустить попадания ошибок в систему контроля версий, исправляя дефекты уже на этапе написания кода\. 

Удачи и творческих успехов\!

## Ссылки

Это первая публикация в 2020 году\. Пользуясь случаем, я приведу ссылки на статьи о проверке C\#\-проектов за прошлый год:

* [Ищем ошибки в исходном коде Amazon Web Services SDK для \.NET](https://pvs-studio.ru/ru/blog/posts/csharp/0605/)
* [Проверяем исходный код Roslyn](https://pvs-studio.ru/ru/blog/posts/csharp/0622/)
* [Nullable Reference типы в C\# 8\.0 и статический анализ](https://pvs-studio.ru/ru/blog/posts/csharp/0631/)
* [WinForms: ошибки, Холмс](https://pvs-studio.ru/ru/blog/posts/csharp/0653/)
* [История о том, как PVS\-Studio нашёл ошибку в библиотеке, используемой в\.\.\. PVS\-Studio](https://pvs-studio.ru/ru/blog/posts/csharp/0654/)
* [Проверка исходного кода библиотек \.NET Core статическим анализатором PVS\-Studio](https://pvs-studio.ru/ru/blog/posts/csharp/0656/)
* [Проверяем Roslyn analyzers](https://pvs-studio.ru/ru/blog/posts/csharp/0664/)
* [Проверка Telerik UI for UWP для знакомства с PVS\-Studio](https://pvs-studio.ru/ru/blog/posts/csharp/0677/)
* [Azure PowerShell: "в основном безвреден"](https://pvs-studio.ru/ru/blog/posts/csharp/0678/)
* [Ищем и анализируем ошибки в коде Orchard CMS](https://pvs-studio.ru/ru/blog/posts/csharp/0681/)
* [Проверка обёртки OpenCvSharp над OpenCV с помощью PVS\-Studio](https://pvs-studio.ru/ru/blog/posts/csharp/0683/)
* [Azure SDK for \.NET: история о непростом поиске ошибок](https://pvs-studio.ru/ru/blog/posts/csharp/0692/)
* [SARIF SDK и его ошибки](https://pvs-studio.ru/ru/blog/posts/csharp/0694/)
* [Топ 10 ошибок в проектах C\# за 2019 год](https://pvs-studio.ru/ru/blog/posts/csharp/0698/)