Each of us encounters game bugs, and some games have more of them than others. Many games suffer from bugs caused by errors in code. Today, we'll see how static analysis can help easily detect errors and enhance your project.
The video game industry is growing fast. Every year, game development gets more expensive, and so do the games too. We have an article about it: "Why should Unity game developers use static analysis?".
Once you have read it and seen how well the static analysis helps game devs, let's talk about the Best Warnings mechanism. This is a good way to quickly get to grips with the static analyzer. You may ask, "Why do we need that?".
Well, we answer it. The analyzer report often may contain hundreds or even thousands of warnings on large projects. Working with them can be a challenge for developers. When you've already chosen a static analyzer, you can simply suppress irrelevant warnings and see only new ones. However, what do you do if you're new to the tool? We've purposely created the Best Warnings mechanism for it.
Let's take a look at TowerDefense-GameFramework-Demo. It's an open-source project, the source code is available on GitHub. Unfortunately, not so many Unity games are open-source, but we need them to show you results. But even on this small project, we still get to see the work of Best Warnings.
Firstly, install the analyzer and enter the license. If you haven't already done so, this page will guide you.
Open your project in Unity Hub. Next, in Unity, click Assets > Open C# Project. You will see the IDE set in the Unity editor settings. It's set here:
I'll be using the Visual Studio 2022 IDE that I use for work. To analyze the project in the IDE version, use the Extensions > PVS-Studio > Check Solution menu item.
After the analysis is completed, click the Best button, and that's all. Then you'll see a list of the 10 analyzer warnings:
As we already know, the Best Warnings section contains 10 warnings indicating the code where an error may occur. We'll study all 10. Spoiler: there will be real errors, just suspicious code, and, of course, false-positive warnings. Indeed, false positives are OK for the static analysis methodology. The main thing is that the percent of such warnings should not be very high. Diving in now!
Fragment 1
public void OnRecycle()
{
transform.SetParent(initRoot, false);
transform.localPosition = transform.localPosition;
transform.eulerAngles = initRotation;
transform.localScale = initScale;
....
}The PVS-Studio warning: V3005 The 'transform.localPosition' variable is assigned to itself. Item.cs 150
As you can see from the code, the transform.localPosition property is assigned to itself. This is a quite common error that we often see in analyzed projects.
Fragment 2
public static int Read7BitEncodedInt32(this BinaryReader binaryReader)
{
int value = 0;
int shift = 0;
byte b;
do
{
if (shift >= 35)
{
throw new GameFrameworkException("7 bit encoded int is invalid.");
}
b = binaryReader.ReadByte();
value |= (b & 0x7f) << shift;
shift += 7;
} while ((b & 0x80) != 0);
return value;
}The PVS-Studio warning: V3134 Shift by [32..34] bits is greater than the size of 'Int32' type of expression '(b & 0x7f)'. BinaryExtension.cs 37
Here, the analyzer is wrong. It displays a message about the incorrect shift. Unfortunately, the analyzer doesn't recognize that the shift increases by 7 each time. Therefore, it can't have values such as 32, 33, and 34. The code operates correctly. However, it's worth noting that we can replace the shift >= 35 check with shift >= 32. Then other developers and the analyzer will understand the possible value ranges.
If you don't know how data flow analysis works, or you're interested in the PVS-Studio technologies, I recommend you read the article.
Fragment 3
public void RemoveData(Data data)
{
if (data == null)
{
throw new GameFrameworkException(Utility.Text.Format("Data '{0}' is null.",
data.Name.ToString()));
}
....
}The PVS-Studio warning: V3080 Possible null dereference. Consider inspecting 'data'. DataManager.cs 122
The developer checks the parameter for validity. If the data parameter is equal to null, an exception will be thrown. However, when the exception message is generated, data is used and equal to null. As a result, we get an exception of the NullReferenceException type instead of GameFrameworkException.
Fragment 4
internal override void Update(float elapseSeconds, float realElapseSeconds)
{
while (m_RecycleQueue.Count > 0)
{
EntityInfo entityInfo = m_RecycleQueue.Dequeue();
IEntity entity = entityInfo.Entity;
EntityGroup entityGroup = (EntityGroup)entity.EntityGroup;
if (entityGroup == null)
{
throw new GameFrameworkException("Entity group is invalid.");
}
entityInfo.Status = EntityStatus.WillRecycle;
entity.OnRecycle();
entityInfo.Status = EntityStatus.Recycled;
....
}
....
}The PVS-Studio warning: V3008 The 'entityInfo.Status' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 176, 174. EntityManager.cs 176
Here, you can see the bug from the analyzer side. It doesn't detect that the entityInfo and entity variables are connected to each other. We left a note for future enhancement.
Fragment 5
public static FileSystem Load(....)
{
....
for (int i = 0; i < fileSystem.m_BlockDatas.Count; i++)
{
BlockData blockData = fileSystem.m_BlockDatas[i];
if (blockData.Using)
{
StringData stringData = fileSystem.ReadStringData(blockData.StringIndex);
fileSystem.m_StringDatas.Add(blockData.StringIndex, stringData);
fileSystem.m_FileDatas.Add(stringData.GetString(....); // <=
}
else
{
fileSystem.m_FreeBlockIndexes.Add(blockData.Length, i);
}
}
return fileSystem;
}The PVS-Studio warning: V3156 The first argument of the 'Add' method is not expected to be null. FileSystem.cs 206
When developing the analyzer, we consider the logic of some popular methods. This mechanism is called "method annotation". Here, the analyzer uses the information from this mechanism. The analyzer knows that the Dictionary<TKey, TValue>.Add method can't accept null.
Let's take a look at the GetString method. Its result is passed to the Add method.
public string GetString(byte[] encryptBytes)
{
if (m_Length <= 0)
{
return null;
}
....
}We only need the beginning of the method, when null is returned. As a result, if m_Length is less than or equal to 0, an ArgumentNullException will be thrown.
Fragment 6
private void RefreshCheckInfoStatus()
{
....
IFileSystem fileSystem = m_ResourceManager.GetFileSystem(....);
if (!fileSystem.SaveAsFile(resourceFullName, resourcePath))
{
throw new GameFrameworkException(Utility.Text.Format(
"Save as file '{0}' to '{1}' from file system '{2}' error.",
resourceFullName,
fileSystem.FullPath));
}
fileSystem.DeleteFile(resourceFullName);
....
}The PVS-Studio warning: V3025 The 1st argument '"Save as file '{0}' to '{1}' from file system '{2}' error."' is used as incorrect format string inside method. A different number of format items is expected while calling 'Format' function. Format items not used: {2}. ResourceManager.ResourceChecker.cs 152
Here is an example of how interprocedural analysis works. The analyzer "follows" a format string with three placeholders for arguments.
public static string Format(string format, object arg0, object arg1)
{
if (format == null)
{
throw new GameFrameworkException("Format is invalid.");
}
CheckCachedStringBuilder();
s_CachedStringBuilder.Length = 0;
s_CachedStringBuilder.AppendFormat(format, arg0, arg1); // <=
return s_CachedStringBuilder.ToString();
}This format string is used as the first argument to the StringBuilder.AppendFormat method. As we know, there are three placeholders, but only two arguments. Using this format string will result in a FormatException.
I assume that such an error may have occurred because of a copy-paste. There's a similar place in the code below:
if (!fileSystem.WriteFile(resourceFullName, resourcePath))
{
throw new GameFrameworkException(Utility.Text.Format(
"Write resource '{0}' to file system '{1}' error.",
resourceFullName,
fileSystem.FullPath));
}The developer could just copy a similar handling and modify the format string but not change the number of arguments.
Fragment 7
public void ShowPreviewTower(TowerData towerData)
{
....
TowerLevelData towerLevelData = towerData.GetTowerLevelData(0);
if (towerLevelData == null) // <=
{
Log.Error("Tower '{0}' Level '{1}' data is null.", towerData.Name, 0);
}
EntityDataRadiusVisualiser entityDataRadiusVisualiser =
EntityDataRadiusVisualiser.Create(towerLevelData.Range); // <=
....
}The PVS-Studio warning: V3125 The 'towerLevelData' object was used after it was verified against null. Check lines: 122, 117. LevelControl.cs 122
It seems that using a variable after checking it for null isn't a good idea. If towerLevelData is null, the case is logged. But that doesn't always happen. The compiler won't ignore the Log.Error method only when a conditional compilation symbol associated with logging or the Debug configuration is defined. In addition, it doesn't interrupt the execution flow (and yes, that would be weird). So, the execution will still reach the line indicated by the analyzer.
Fragment 8
public DataTableProcessor(....)
{
....
m_Strings = strings.OrderBy(value => value.Key)
.OrderByDescending(value => value.Value)
.Select(value => value.Key).ToArray();
....
}The PVS-Studio warning: V3078 Sorting keys priority will be reversed relative to the order of 'OrderBy...' method calls. Perhaps, 'ThenByDescending' should be used instead. DataTableProcessor.cs 179
Unfortunately, some devs may not know that the .OrderBy.OrderBy sorting pattern operates in reverse. We have an article on the topic: "Sorting in C#: OrderBy.OrderBy or OrderBy.ThenBy? What's more effective and why?". It seems that it's better to use .OrderBy.ThenBy. This not only makes the code clearer but also more performant.
Fragment 9
public bool BuildResources()
{
....
bool isSuccess = false;
isSuccess = BuildResources(Platform.Windows, ....);
if (!watchResult || isSuccess)
{
isSuccess = BuildResources(Platform.Windows64, ....);
}
....
if (!watchResult || isSuccess)
{
isSuccess = BuildResources(Platform.WebGL, ....);
}
....
}The PVS-Studio warning: V3137 The 'isSuccess' variable is assigned but is not used by the end of the function. ResourceBuilderController.cs 748
PVS-Studio warns that a value is assigned to the isSuccess variable. The variable isn't used after that. The BuildResources method writes its work result to isSuccess. There are six more assignments hidden under the second '....'. It's hard to tell if it's really a bug, but if the last return value of a method is to be ignored, it's better to use '_':
_ = BuildResources(Platform.WebGL, ....);It's not just a code to prevent the analyzer from issuing a warning. This writing style enhances code readability for other developers.
Fragment 10
private void RefreshSourceVersionCount()
{
m_SourceVersionIndexes[m_TargetVersionIndex + 1] = false; // <=
m_SourceVersionCount = 0;
if (m_SourceVersionIndexes == null) // <=
{
return;
}
for (int i = 0; i < m_SourceVersionIndexes.Length; i++)
{
if (m_SourceVersionIndexes[i])
{
m_SourceVersionCount++;
}
}
}The PVS-Studio warning: V3095 The 'm_SourceVersionIndexes' object was used before it was verified against null. Check lines: 338, 340. ResourcePackBuilder.cs 338
This warning is difficult to describe, somehow :) The m_SourceVersionIndexes field is checked for null after accessing array elements by index. It's better to move the check to the beginning of the method.
That's all :) We didn't waste much time, and it was easy to look into the bugs. Now you know how to quickly find errors in code. It's worth remembering that the Best Warnings mechanism is a good choice for newcomers to the static analysis world. It's better to use static analysis regularly. For example, you can check commits or run analysis on developers' computers. This way, you can fully evaluate the outcomes of using static analysis tools in your project.
Good luck!