Checking osu! and exploring features of static analyzers

• Introduction
• Saves time
• Deep analysis
• Understands method specifics
• Ensures correct program execution
• Catches copy-paste errors
• Catches what you might not know
• Always right (not quite)
• Conclusion

Many developers know about static analysis tools. What practical benefits do they offer, and why do so many teams introduce them? This time, we'll break down several key features of these tools by analyzing the source code of the osu! game.

Introduction

Static analyzers are common tools in software development. What if your workflow doesn't include one? What could motivate you to use it regularly—the number of diagnostic rules, the errors it finds? Instead of guessing, we'll highlight several key features of static analyzers that become apparent when checking a project.

To avoid taking my word for it, let's explore these features using the source code of the well-known osu! project. This is one of the most popular open-source rhythm games with a large and dedicated community. We expected the code to be high-quality, but PVS-Studio analyzer still found potential issues. Using these examples, I'll demonstrate what static analysis tools can do.

The project code is available on GitHub, the analysis is performed on commit 094f703.

Let's cut to the chase and dive into the key features of static analyzers.

Saves time

One of the advantages of static analyzers is their ability to save time during code reviews. They use a similar approach—examining source code—but the tool does all the work for you.

Let's start with a warm-up: Can you find the error yourself?

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;

   
  }
}

Found it? I never doubted you!

For the record, let's break down what happened. This appears to be a logical error, which can be hard to spot since not everyone scrutinizes every possible scenario. In this case, one scenario can immediately cause issues.

The code starts with two checks.

The first check:

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

The second check:

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

These checks likely handle different scenarios for the score and value variables—if score = null, if value = null, if they are equal, and others. However, if score = "NotNull" and value = null, both checks will pass without exiting the method. The code proceeds to null dereference:

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

This could lead to the NullReferenceException.

Deep analysis

Another strength of analyzers is the depth of their analysis. Unlike other code analysis tools, they see the entire codebase and can show you the execution path of a potential issue.

This in-depth analysis helps uncover more errors and vulnerabilities by tracking data passed between methods and files. This becomes possible due to interprocedural, context-sensitive analysis.

Let's look at the example of such an error in osu!.

The SaveFailedScoreButton.cs file:

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

We could stare at this code for a long time and miss the error because it lies deeper. Let's look at the analyzer warning.

The analyzer points to a problem in the PresentScore method, specifically the first argument, passed as a potential null:

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

Let's investigate by diving into the PresentScore method:

The OsuGame.cs file:

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

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

Well... Seems like it's not the end of story. As we can see, the first argument is passed to GetScore with no null checks. So, we go deeper.

The ScoreManager.cs file:

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

Still no check. Let's dig further into getDatabasedScoreInfo.

The ScoreManager.cs file:

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);
}

Aha! Gotcha! We've reached the point where a potential null is dereferenced without a check, which could again lead to an NRE.

Such errors are challenging to track during code reviews (unless you want to spend days on them), and tests for such specific issues might not cover them. While there were many checks, none handled the potential null. So you can simplify your work by using a static code analyzer :)

(P.S. try PVS-Studio)

Understands method specifics

Static analyzers, thanks to their developers' hard work, can flag errors related to method peculiarities. For example, PVS-Studio can identify methods that don't behave as a developer expects.

A great example comes from a recent analysis of the Lean trading engine:

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;
  }
  ....  
}

The developer intended to return one of two values randomly, but the Next method's second parameter is outside the range of the return values. Thus, random.Next(0, 1) always returns 0.

Now, let's look at an example from osu! where developers might have missed scenarios when working with indices. Here is the code:

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);
    }
}

The analyzer warns that the index could become -2. Here is the reason: the IndexOf method, which provides deletedIndex, can return -1.

While the developer might have assumed this wouldn't happen, adding a check would improve readability and safety, especially in a widely used open-source project.

Ensures correct program execution

Some errors have non-obvious consequences. Let's examine a potential issue flagged by a relatively new diagnostic rule:

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);
}

The analyzer notes that the Volume property is used in GetHashCode but not in Equals. Where is the error?

This can cause GetHashCode to return different values for two equivalent objects.

According to Microsoft documentation,GetHashCode must return the same hash code for any two objects where Equals returns True. This inconsistency can also affect handling collections Hashtable, Dictionary<TKey,TValue>, and others.

Catches copy-paste errors

Copy-paste mistakes are common and easy to miss even during most meticulous code reviews, especially in large, repetitive functions—checks, comparisons, switch cases, and many others. Look at the example:

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;
      ....
    }
}

The error is obvious here, catching it is not a big deal. The analyzer confirms this is an error:

The Room.Type case was likely copied from Room.Name but not updated. While this particular mistake is easy to spot because it's at the function start, it still made its way into the code.

Readers have an advantage over developers: they know where to look. So, let's not blame the developers too harshly and simply advise them to be cautious with copy pasting.

Because that's not all...

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})");
}

Another funny mistake here. Here, the developer likely confused minimum and maximum in the first case.

A cool PVS-Studio feature is its ability to highlight code fragments (in the IDE) and point out the error's origin, even suggesting where the copy paste might have come from.

Catches what you might not know

Another common error source is simple lack of knowledge, which is ok—anyone can face it.

Unfortunately, code doesn't care and crashes regardless of the author's knowledge or experience.

Look at the example:

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

The analyzer notes the?? operator low precedence. The developer might not have expected or intended it.

Let's add parentheses to the code for clarity—the diagnostic rule documentation also recommends adding them to avoid such errors. Based on the current code, we can assume that the developer expected the following check:

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

But the author ended up with the following check by not taking into account the specifics of the ?? operator.

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

Why do I think it's an error? This could be the logic the developer wanted.

Here we've come to the standard practice of working with a static code analyzer—breaking down an error. Let's look at the broader context:

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 is a constant, and there are no similar patterns elsewhere. It would be strange to reset the entire value, including BASE_STARS instead of modRateAdjust?.SpeedChange.Value if modRateAdjust is null.

I've already mentioned that detailed documentation is another PVS-Studio's strong point. It's comprehensive and includes rule descriptions, examples, fixes, and additional details, for example, CWE mappings, real-world examples.

You can access it directly from the IDE.

Always right (not quite)

Static analyzers aren't magic wands or silver bullets that eliminate all problems.

They won't replace testing but can complement existing quality control stages and find errors other tools might miss.

Perhaps you might want to correct every issue, but you actually shouldn't. Here we've come to some cons of static analyzers—they identify potential errors, which means that not all of them will turn out to be real. The analyzer might lack context and produce false positives.

Let's look at an example of a warning similar to the previous one from the same project (with the diagnostic rule number 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;
}

At first glance, it's unclear if there's an error. But it all falls into place when we look at the comment for this code:

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

Comments clearly state what is expected from the code, so we can grasp what exactly the developer wanted. If Previous?.GroupInterval is absent (null), the expression should evaluate to 1. If we add parentheses, we get:

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

As we can see, there's no error. Does this mean the analyzer is flawed? Not at all—such warnings are not a problem, but a distinctive feature of the technology that we should learn to work with.

We continuously refine diagnostic rules to minimize false positives, eliminate obvious and not-so-obvious cases, considering various known patterns of certain structs. Alas, sometimes context is missing. But we still can do something about it: configuring a static analyzer for your project can significantly reduce the number of such false positives. This article covers this topic in more detail.

What should we do? Ignore the warning? No! Mark it as a False Alarm by right-clicking the analyzer message and selecting the appropriate option:

This moves the warning to the "False Alarms" category without removing it.

You can also use suppression files for mass suppression. This mechanism proves useful in several scenarios. For instance, when you first integrate a static analyzer into your project, the initial scan may generate a large number of warnings. You can suppress these warnings, effectively treating them as technical debt to address later. This way, the analyzer retains awareness of these issues, but they won't interfere with analyzing new code, keeping subsequent analysis reports clean.

Conclusion

We've explored several features that make static analyzers powerful tools. While we haven't covered all pros and cons, this should help you decide if such tools interest you.

If you're curious about what else PVS-Studio found in osu! (we've only scratched the surface—the full report contains over 1,000 warnings) or want to check your own project, you're welcome to download and try the analyzer here.