# Simple & quick search for bugs in Unity games \(for C\# developers\) 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\. ![1111_TowerDefenceDemo/image1.png](https://import.viva64.com/docx/blog/1111_TowerDefenceDemo/image1.png) 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?](https://pvs-studio.com/en/blog/posts/csharp/0945/)"\. 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\. ## How to get Best Warnings? Let's take a look at TowerDefense\-GameFramework\-Demo\. It's an open\-source project, the source code is available on [GitHub](https://github.com/DrFlower/TowerDefense-GameFramework-Demo)\. 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](https://pvs-studio.com/en/pvs-studio/try-free/) 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: ![1111_TowerDefenceDemo/image2.png](https://import.viva64.com/docx/blog/1111_TowerDefenceDemo/image2.png) 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\. ![1111_TowerDefenceDemo/image3.png](https://import.viva64.com/docx/blog/1111_TowerDefenceDemo/image3.png) After the analysis is completed, click the **Best** button, and that's all\. Then you'll see a list of the 10 analyzer warnings: ![1111_TowerDefenceDemo/image4.png](https://import.viva64.com/docx/blog/1111_TowerDefenceDemo/image4.png) ## Let's break down 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** ```cpp public void OnRecycle() { transform.SetParent(initRoot, false); transform.localPosition = transform.localPosition; transform.eulerAngles = initRotation; transform.localScale = initScale; .... } ``` The PVS\-Studio warning: [V3005](https://pvs-studio.com/en/docs/warnings/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** ```cpp 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](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/blog/posts/0908/)\. **Fragment 3** ```cpp 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](https://pvs-studio.com/en/docs/warnings/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** ```cpp 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](https://pvs-studio.com/en/docs/warnings/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** ```cpp 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](https://pvs-studio.com/en/docs/warnings/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\.Add_ method can't accept _null_\. Let's take a look at the _GetString_ method\. Its result is passed to the _Add_ method\. ```cpp 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** ```cpp 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](https://pvs-studio.com/en/docs/warnings/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\. ```cpp 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: ```cpp 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** ```cpp 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](https://pvs-studio.com/en/docs/warnings/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** ```cpp public DataTableProcessor(....) { .... m_Strings = strings.OrderBy(value => value.Key) .OrderByDescending(value => value.Value) .Select(value => value.Key).ToArray(); .... } ``` The PVS\-Studio warning: [V3078](https://pvs-studio.com/en/docs/warnings/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?](https://pvs-studio.com/en/blog/posts/csharp/0991/)"\. It seems that it's better to use _\.OrderBy\.ThenBy_\. This not only makes the code clearer but also more performant\. **Fragment 9** ```cpp 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](https://pvs-studio.com/en/docs/warnings/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 '\_': ```cpp _ = 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** ```cpp 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](https://pvs-studio.com/en/docs/warnings/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\. ## Conclusion 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](https://habr.com/en/articles/440610/)\. 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\!