While developing the PVS-Studio static analyzer, we try to develop it in various directions. So, our team is working on plugins for IDE (Visual Studio, Rider), improving integration with CI, etc. Increasing the efficiency of project analysis for 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 project. Therefore, I would like to increase the popularity of PVS-Studio among companies developing for Unity. One of the first steps in the implementation of this idea was to write annotations for the methods defined in the engine. This allows you to control the correctness of the code associated with calls to annotated methods.
Introduction
Annotations are one of the most important analyzer mechanisms. They provide various information about arguments, return values, and internal features of methods that cannot be figured out automatically. At the same time, the annotating programmer can assume the approximate internal structure of the method and the specifics of its operation, based on the documentation and common sense.
For example, calling the GetComponent method looks somewhat strange if the value returned by it is not used. Bullshit mistake? Not at all. Of course, this may simply be an extra challenge, forgotten and abandoned by everyone. Or it may be that some important assignment is missing. 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, annotated class methods from the System namespace . In addition, there is a mechanism for automatically annotating some methods. You can read more about this here . Note that this article tells more about the part of PVS-Studio that is responsible for analyzing projects in C ++. However, there is no tangible difference in how annotations work for C # and C ++.
Writing annotations for Unity methods
We strive to improve the quality of code review of projects using Unity, and therefore it was decided to annotate the methods of this engine.
The original idea was to cover all Unity methods with annotations in general, however, there were a lot of them. As a result, we decided to begin by annotating methods from the most commonly used classes.
Collection of information
First, it was necessary to find out which classes are used more often than others. In addition, an important aspect is the ability to collect annotation results - new errors that the analyzer will find in real projects thanks to written annotations. Therefore, the first step was to find relevant open-source projects. However, it was not so easy to do this.
The problem is that many of the projects found were quite small in terms of source code. In such, if there are errors, then there are not very many of them, and it is even less likely to find there any positives related specifically to methods from Unity. Sometimes there were projects that practically did not use (or did not use at all) Unity-specific classes, although according to the description they were somehow connected with the engine. Such finds were completely unsuitable for the task at hand.
Of course, sometimes lucky. For example, the gem in this collection is MixedRealityToolkit . There is already decent code in it, which means that the collected statistics on the use of Unity methods in such a project will be more complete.
Thus, 20 projects were recruited using the capabilities of the engine. In order to find the most commonly used classes, a Roslyn-based utility was written to count method calls from Unity. By the way, such a program can also be called a static analyzer. After all, if you think about it, she really analyzes the source without resorting to launching the project itself.
The written “analyzer” made it possible to find classes whose average frequency of use in found projects is highest:
- UnityEngine.Vector3
- UnityEngine.Mathf
- UnityEngine.Debug
- UnityEngine.GameObject
- UnityEngine.Material
- UnityEditor.EditorGUILayout
- UnityEngine.Component
- UnityEngine.Object
- UnityEngine.GUILayout
- UnityEngine.Quaternion
- Etc.
Of course, this does not mean at all that these classes are really used by developers so often - after all, statistics based on such a small sample of projects are not particularly reliable. However, to start this information was enough to be sure that the methods of classes that are used at least somewhere are annotated.
Annotation
After getting the information you need, it's time to get started with the actual annotation. The true helpers in this matter were the documentation and the Unity editor, where a test project was created. This was necessary to check some points not specified in the documentation. For example, it was far from always clear whether passing null in any argument would lead to an error, or whether the program would continue without problems. Of course, passing null , as a rule, is not entirely good, but in this case we considered only errors that interrupted the thread of execution, or it was logged by the Unity editor as an error.
During such checks, interesting features of the work of some methods were found. For example, running code
MeshRenderer renderer = cube.GetComponent<MeshRenderer>();
Material m = renderer.material;
List<int> outNames = null;
m.GetTexturePropertyNameIDs(outNames);leads to the fact that the Unity editor itself is crashing, although usually in such cases the execution of the current script is interrupted and the corresponding error is logged. Of course, it is unlikely that developers often write this, but the fact that the Unity editor can be brought to a crash by running ordinary scripts is not very good. The same thing happens in at least one more case:
MeshRenderer renderer = cube.GetComponent<MeshRenderer>();
Material m = renderer.material;
string keyWord = null;
bool isEnabled = m.IsKeywordEnabled(keyWord);These issues are relevant for Unity Editor 2019.3.10f1.
Collecting results
After annotating is done, you need to check how this will affect the issued warnings. Before adding annotations, a log with errors is generated for each of the selected projects, which we call the reference. Then new annotations are built into the analyzer and the projects are re-checked. The generated lists of warnings, thanks to annotations, will differ from the reference ones.
The annotation testing procedure is performed automatically using the CSharpAnalyserTester program specially written for these needs. It launches analysis on projects, after which it compares the resulting logs with the reference ones and generates files containing information about the differences.
The described approach is also used to find out what changes in the logs appear when adding a new or changing an existing diagnostic.
As noted earlier, it was difficult to find large open source projects for Unity. This is unpleasant, because the analyzer could have produced more interesting triggers for them. At the same time, there would be many more differences between the reference logs and the logs generated after annotation.
Nevertheless, the written annotations helped to identify several suspicious points in the projects under consideration, which is also a pleasant result of the work.
For example, a somewhat strange GetComponent call 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 must somehow be used. Therefore, when annotated, it was marked accordingly. Immediately, the call result is not assigned to anything, which looks a little strange.
And here is another example of additional analyzer operations:
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 does not pay attention to passing LocalPlayer to string.Format , as this will not result in an error. And the code looks like it was written on purpose.
In this case, the effect of annotations is not so obvious. However, it was they who caused these positives to appear. The question arises - why these warnings were not there before?
The fact is that several calls are made in the DebugReturn method , 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 does not know the peculiarities of the called methods, which means that it does not know how they will affect the situation. So, PVS-Studio assumes that the value of this.CurrentRoom could have changed while the DebugReturn method was running , so the next check is performed.
The annotations provided information that the methods called inside DebugReturn will not affect the values of other variables. Therefore, using a variable before checking it for null is suspect.
Conclusion
Summing up, it's worth saying that annotating Unity-specific methods will undoubtedly allow you to find more various errors in projects using this engine. However, annotation coverage of all available methods will take a lot of time. More effective will annotate primarily the most commonly used ones. However, understanding which classes are used more often requires suitable projects with a large codebase. In addition, large projects give you much better control over annotation performance. 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 capabilities. Thus, over time, the efficiency of PVS-Studio is growing. Therefore, if you have not tried PVS-Studio, it's time to fix this by downloading it from the corresponding page . There you can also get a trial key for the analyzer to get acquainted with its capabilities by checking various projects.
If you want to share this article with an English-speaking audience, please use the translation link: Nikita Lipilin. How the PVS-Studio analyzer began to find even more errors in Unity projects .