When developing the PVS-Studio static analyzer, we try to develop it in various directions. Thus, our team is working on plugins for the IDE (Visual Studio, Rider), improving integration with CI, and so on. Increasing the efficiency of project analysis under Unity is also one of our priority goals. We believe that static analysis will allow programmers using this game engine to improve the quality of their source code and simplify work on any projects. Therefore, we would like to increase the popularity of PVS-Studio among companies that develop under Unity. One of the first steps in implementing this idea was to write annotations for the methods defined in the engine. This allows a developer to control the correctness of the code related to calls of annotated methods.

Introduction

Annotations are one of the most important mechanisms of the analyzer. They provide various information about arguments, return values, and internal features of methods that can't be found out in the automatic mode. At the same time, the developer who annotates a method can assume its approximate internal structure and features of its operation, based on documentation and common sense.

For example, calling the GetComponent method looks somewhat strange if the value it returned isn't used. A trifling bug? In no way. Of course, this may simply be a redundant call, forgotten and abandoned by everyone. Or it may be that some important assignment was omitted. Annotations can help the analyzer find similar and many other errors.

Of course, we have already written a lot of annotations for the analyzer. For example, class methods from the System namespace are annotated. In addition, there is a mechanism to automatically annotate some methods. You can read here in detail about it. Note that this article tells more about the part of PVS-Studio that is responsible for analyzing projects in C++. However, there is no noticeable difference in the way annotations work for C# and C++.

Writing annotations for Unity methods

We strive to improve the quality of checking the code of projects that use Unity, which is why we decided to annotate methods of this engine.
The initial idea was to cover all Unity methods with annotations, however there has been a lot of them. As a result, we decided to start by annotating methods from the most commonly used classes.

Collecting information

First, we had to find out which classes are used more often than others. In addition, an important aspect was to ensure that you can collect annotation results – new errors that the analyzer will find in real projects thanks to the written annotations. Therefore, the first step was to search for appropriate open source projects. However, this was not so easy to do.

The problem is that many of the projects found were quite small in terms of the source code. If there are errors in such projects, their number is small. Not to mention the fact that it is less likely to find some warnings related to methods from Unity in them. Occasionally, some projects came up which almost haven't used (or haven't used at all) Unity-specific classes, although they were described as related to the engine in one way or another. Such finds were completely unsuitable for the task at hand.

Of course, in some cases I was lucky. For example, the gem in this collection is MixedRealityToolkit. There is already quite a lot of code in it, which means that the collected statistics on the use of Unity methods in such a project will be more complete.

Thus, there were 20 projects that use the engine's abilities. In order to find the most frequently used classes, a Roslyn based utility was written that counts method calls from Unity. This program, by the way, can also be called a static analyzer. After all, if you think about it, it really analyzes the source code, without running the project itself.

The written "analyzer" allowed us to find classes whose average frequency of use in the found projects was the highest:

• UnityEngine.Vector3
• UnityEngine.Mathf
• UnityEngine.Debug
• UnityEngine.GameObject
• UnityEngine.Material
• UnityEditor.EditorGUILayout
• UnityEngine.Component
• UnityEngine.Object
• UnityEngine.GUILayout
• UnityEngine.Quaternion
• and others.

Of course, this doesn't mean that these classes are actually used very often by developers – after all, statistics based on such a small set of projects aren't particularly trustworthy. However, to start with, this information was enough to make sure that the annotated methods' classes were used at least somewhere.

Annotating

After getting the necessary information, it's time to do the actual annotation. The documentation and the Unity editor, where the test project was created, were reliable helpers in this case. It was necessary to check some points that weren't specified in the documentation. For example, it was not always clear whether passing null as any argument would lead to an error, or whether the program would continue running without problems. Of course, passing null is usually not a good practice, but in this case, we only considered errors that interrupted the execution flow, or were logged by the Unity editor as an error.

During these checks, interesting features of some methods were found. For example, running the code

MeshRenderer renderer = cube.GetComponent<MeshRenderer>();
Material m = renderer.material;
List<int> outNames = null;
m.GetTexturePropertyNameIDs(outNames);

makes the Unity editor itself crash, although usually in such cases, the current script execution is interrupted and the corresponding error is logged. Of course, it is unlikely that developers often write such things, but the fact that the Unity editor can be crashed by running regular scripts isn't nice. The same thing happens in at least one other case:

MeshRenderer renderer = cube.GetComponent<MeshRenderer>();
Material m = renderer.material;
string keyWord = null;
bool isEnabled = m.IsKeywordEnabled(keyWord);

These issues are relevant for the Unity 2019.3.10f1 editor.

Collecting the results

After the annotation is completed, you need to check how this will affect the warnings being issued. Before adding annotations, an error log is generated for each of the selected projects, which we call the reference log. Then the new annotations are embedded in the analyzer and the projects are checked again. The generated warning lists will differ from the reference ones due to annotations.

The annotation testing procedure is performed automatically using the CSharpAnalyserTester program specifically written for these needs. It runs analysis on projects, then compares the resulting logs with the reference ones and generates files containing information about differences.

The described approach is also used to find out what changes in logs appear when a new diagnostic is added or an existing one is changed.

As noted earlier, it was difficult to find large open projects under Unity. This is unpleasant, as the analyzer would be able to produce more interesting warnings for them. At the same time, there would be much more differences between reference logs and logs generated after annotation.

Nevertheless, the written annotations helped to identify several suspicious points in the projects under consideration, which is also a favorable result of the work.

For example, a bit strange call of GetComponent was found:

void OnEnable()
{
  GameObject uiManager = GameObject.Find("UIRoot");

  if (uiManager)
  {
    uiManager.GetComponent<UIManager>();
  }
}

Analyzer warning: V3010 The return value of function 'GetComponent' is required to be utilized. — ADDITIONAL IN CURRENT UIEditorWindow.cs 22

Based on the documentation, it is logical to conclude that the value returned by this method should be used in some way. Therefore, it was marked accordingly when annotated. In this case, the result of the call isn't assigned to anything, which looks a bit strange.

Here is another example of additional analyzer warnings:

public void ChangeLocalID(int newID)
{
  if (this.LocalPlayer == null)                          // <=
  {
    this.DebugReturn(
      DebugLevel.WARNING, 
      string.Format(
        ...., 
        this.LocalPlayer, 
        this.CurrentRoom.Players == null,                // <=
        newID  
      )
    );
  }

  if (this.CurrentRoom == null)                          // <=
  {
    this.LocalPlayer.ChangeLocalID(newID);               // <=
    this.LocalPlayer.RoomReference = null;
  }
  else
  {
    // remove old actorId from actor list
    this.CurrentRoom.RemovePlayer(this.LocalPlayer);

    // change to new actor/player ID
    this.LocalPlayer.ChangeLocalID(newID);

    // update the room's list with the new reference
    this.CurrentRoom.StorePlayer(this.LocalPlayer);
  }
}

Analyzer warnings:

• V3095 The 'this.CurrentRoom' object was used before it was verified against null. Check lines: 1709, 1712. — ADDITIONAL IN CURRENT LoadBalancingClient.cs 1709
• V3125 The 'this.LocalPlayer' object was used after it was verified against null. Check lines: 1715, 1707. — ADDITIONAL IN CURRENT LoadBalancingClient.cs 1715

Note that PVS-Studio doesn't pay attention to passing LocalPlayer to string.Format, since this won't cause an error. And the code looks like it was written intentionally.

In this case, the impact of annotations isn't so obvious. However, they are the reason for these triggerings. So here comes the question — why were there no such warnings before?

The fact is that the DebugReturn method makes several calls, which in theory could affect the value of the CurrentRoom property:

public virtual void DebugReturn(DebugLevel level, string message)
{
  #if !SUPPORTED_UNITY
  Debug.WriteLine(message);
  #else
  if (level == DebugLevel.ERROR)
  {
    Debug.LogError(message);
  }
  else if (level == DebugLevel.WARNING)
  {
    Debug.LogWarning(message);
  }
  else if (level == DebugLevel.INFO)
  {
    Debug.Log(message);
  }
  else if (level == DebugLevel.ALL)
  {
    Debug.Log(message);
  }
  #endif
}

The analyzer doesn't know how the called methods work, so it doesn't know how they will affect the situation. For example, PVS-Studio assumes that the value of this.CurrentRoom may have changed during the DebugReturn method, so the check is performed next.

The annotations also provided the information that methods called inside DebugReturn won't affect the values of other variables. Therefore, using a variable before checking it for null can be considered suspicious.

Conclusion

To sum up, annotating Unity-specific methods will undoubtedly allow you to find more errors in projects that use this engine. However, annotating all available methods will take quite a long time. It is more efficient to annotate the most frequently used ones first. However, in order to understand which classes are used more often, you need suitable projects with a large code base. In addition, large projects allow much better control over the effectiveness of annotation. We will continue to do all this in the near future.

The analyzer is constantly being developed and refined. Adding annotations to Unity methods is just one example of extending its abilities. Thus, over time, the efficiency of PVS-Studio increases. So if you haven't tried PVS-Studio yet, it's time to fix it by downloading it from the corresponding page. There you can also get a trial key for the analyzer to get acquainted with its abilities by checking various projects.