PVS-Studio
Published in

PVS-Studio

PVS-Studio static analyzer to recheck Unity

Unity is one of the most popular game engines. It helps create many excellent cross-platform projects. It’s been 4 years since the last time we checked Unity’s source code. Time has come again to see what interesting things we can find.

Introduction

A while ago, we checked Unity and wrote an article about it. Click here to read it. Unity is indeed a large project that thousands of developers use daily. And don’t forget about all the users that spend their time playing games developed with Unity. I think that projects of this scale must be monitored regularly — errors in such projects can affect a large number of people.

In this article, I’ll analyze the source code of the Unity engine and editor of version 2022.1.0b8. Let’s go directly to the results of the check.

Check results

Issue 1

V3008 The ‘rect.y’ variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 370, 366. ShaderVariantCollectionInspector.cs 370

The analyzer reports that the same variable — rect.y — is assigned a value twice and the code does not use the variable between the assignments. If we take a closer look, we’ll see that the value for this variable is produced a bit higher in the code, under the m_NumFilteredVariants > maxFilteredLength condition — and is also lost.

Consequently, all variable value changes, except for the last one, make no sense.

Issue 2

V3042 Possible NullReferenceException. The ‘?.’ and ‘.’ operators are used for accessing members of the ‘version’ object UpmPackageDocs.cs 38

The analyzer found two ways to address members of the same object. If the value of version is null, the IsNullOrEmpty method will return true. When the execution flow attempts to access displayName, a NullReferenceException exception will be thrown.

Issue 3

V3029 The conditional expressions of the ‘if’ statements situated alongside each other are identical. Check lines: 930, 933. ZoomableArea.cs 930

The developers perform the same check twice. hZoomLockedByDefault is a field in a class. If we take a look at where this field is defined, we’ll see the vZoomLockedByDefault field nearby.

It all looks like a simple typo.

Issue 4

V3022 Expression ‘inputTextField.panel != null’ is always true. BaseSlider.cs 648

The analyzer reports that the inputTextField.panel != null expression is always true.

Indeed — part of the condition above already contains an identical check. The authors could have intended to check something else, but made a mistake.

Issue 5

The analyzer detected the following code:

V3022 Expression is always false. Probably the ‘||’ operator should be used here. EditorGUIExt.cs 141

Here the analyzer found an expression that is always false. Whichever value the property returns, one of the comparisons is always false.

Below is a possible way to fix the code:

Issue 6

V3078 Original sorting order will be lost after repetitive call to ‘OrderBy’ method. Use ‘ThenBy’ method to preserve the original sorting. MenuService.cs 499

The analyzer detected that this code calls OrderBy twice in a row.

I found this warning quite interesting. Of course, calling OrderBy twice is not an error pattern. Most likely, this is a code fragment that may cause an error if someone misunderstands how this code works. If the developer intended to sort the collection first by key, and they by priority, then this code will produce an error. Why?

Let’s have a look. In this code, the two OrderBy calls will sort the collection first by priority, and then by key. It’s not sufficiently clear, is it? I think that here, instead of the second OrderBy call, calling ThenBy would be a good idea. This way, the sorting wouldn’t be done “vice versa”. ThenBy will be easier to read and won’t raise any extra questions. For details, read the following note.

By the way, PVS-Studio found one more similar suspicious code fragment: V3078 Original sorting order will be lost after repetitive call to ‘OrderBy’ method. Use ‘ThenBy’ method to preserve the original sorting. SearchSelector.cs 177

Issue 7

V3061 Parameter ‘namedBuildTarget’ is always rewritten in method body before being used. PlayerSettingsIconsEditor.cs 396

This code fragment is fairly unusual. The method’s first parameter is overwritten before being used. What’s more, this parameter is used only inside the if (platformUsesStandardIcons) condition. As a result, the value passed to the method is always lost.

Issue 8

V3022 Expression ‘m_State.m_NewAssetIndexInList != -1’ is always true. ObjectListArea.cs 511

The analyzer detected an expression that is always true. m_State.m_NewAssetIndexInList is assigned a value that the IndexOfNewText method returns. Let’s take a look at this method’s implementation:

You can notice that the method returns idx that is always greater than or equal to 0.

As a result, the else branch is never executed. The error could hide inside the IndexOfNewText method. The developers had expected the method to be able to return -1.

Issue 9

V3138 String literal contains potential interpolated expression. Consider inspecting: type. OverlayUtilities.cs 116

PVS-Studio indicates that the string interpolation character is missing. Such mistakes often complicate any attempts to search for problems in code, because default error messages will contain inaccurate information.

Issue 10

V3080 Possible null dereference. Consider inspecting ‘wrapper’. CurveEditor.cs 1889

The analyzer detected a code fragment that can lead to dereferencing a reference whose value is null.

The GetCurveWrapperFromID method can return null:

The method’s return value is stored to the wrapper variable. Then the link is dereferenced — and that can cause an exception. The developer could have been confident that the method would never return null, but nevertheless, this code needs a closer look.

Issue 11

V3022 Expression ‘$”{material.shader.name}/”’ is always not null. The operator ‘??’ is excessive. IndexerExtensions.cs 190

The analyzer warns that the $”{material.shader.name}/” is always not null. It’s hard to disagree with this statement. Consequently, using the ‘??’ operator and conducting a null check is unnecessary.

Issue 12

V3137 The ‘maxDist’ variable is assigned but is not used by the end of the function. TreeAOImporter.cs 142

The analyzer points out that the local variable is assigned a value but then this value is never used. You may have also noticed that starting with if (s_RayCastHits.Length > 0), the code does not do anything meaningful. All assignments in this code fragment are done through local variables that do not affect the return value in any way.

Issue 13

V3019 Possibly an incorrect variable is compared to null after type conversion using ‘as’ keyword. Check variables ‘targetItem’, ‘hierarchyTargetItem’. AssetOrGameObjectTreeViewDragging.cs 153

The analyzer points out that the developer casts targetItem to GameObjectTreeViewItem by use of the as operator. However, then the original reference — instead of the resulting reference — is checked for null.

If the conversion by use of the as operator fails, hierarchyTargetItem will contain null. Passing the hierarchyTargetItem null value to IsDropTargetUserModifiable will cause the NullReferenceException exception that everyone loves so much.

This method’s simplified code looks as follows:

It’s worth noting that hierarchyTargetItem is used earlier as a second argument when the m_CustomDragHandling delegate and the DoDragScenes method are called. In the first case, it’s unclear which methods the delegate points to, and, consequently, whether dereferencing a null reference may happen. In the second case, the DoDragScenes method always does a null check, so no exception will be thrown. You can find this method’s code here.

Issue 14

V3029 The conditional expressions of the ‘if’ statements situated alongside each other are identical. Check lines: 722, 728. BuiltinTools.cs 722

The analyzer found a suspicious code fragment where two if blocks with identical conditions follow one another. One could assume that the second if block is redundant code that does not affect anything. But this assumption is not quite correct, because the code uses the scale value to form the refScale value. This means, the second block still affects the result.

It’s worth noting that the uniformScaling does not change inside the conditional blocks. This means, all calculations could be placed under one if.

Conclusion

Looks like checking this project again was a good idea. I found several code fragments that were definitely worth my attention. Which of those are errors and which are just flaws? It’s upon developers to decide. From the outside, alas, it can be difficult to determine whether a warning is critical.

In any case, I thank the Unity team for their hard work! I want to believe that this article made a small contribution to the project’s quality.

You can also download PVS-Studio and check your project. To do this, you can get a trial key on our website.

--

--

--

PVS-Studio is a tool for detecting bugs and security weaknesses in the source code of programs, written in C, C++, C# and Java. It works under 64-bit systems in Windows, Linux and macOS environments, and can analyze source code intended for 32-bit, 64-bit and embedded ARM platfor

Recommended from Medium

As developers, we also and foremost are users

Create a Kubernetes Cluster and Deploy an App on Amazon EKS using AWS CLI

IBM App Connect: AI powered data mapping with Mapping Assist

Learning Test Automation — What’s It Like?

Find an optimal set of nodes for a Kubernetes cluster

In this tutorial we’ll be digging deeper into the Front Controller Pattern, part of the Enterprise…

Why is Software Development Outsourcing a Preferable Option over In-house Development?

How to Choose Redmine Hosting: Cost & Feature Comparison

Get the Medium app

A button that says 'Download on the App Store', and if clicked it will lead you to the iOS App store
A button that says 'Get it on, Google Play', and if clicked it will lead you to the Google Play store
Unicorn Developer

Unicorn Developer

The developer, the debugger, the unicorn. I know all about static analysis and how to find bugs and errors in C, C++, C#, and Java source code.

More from Medium

Why you should use XML data for your Unity/C# games

Checking Barotrauma with the PVS-Studio static analyzer

Tips to Improve Unity Performance

Using threads in Unity Engine