# 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\.

![0932_Unity/image1.png](https://import.viva64.com/docx/blog/0932_Unity/image1.png)

## Introduction

A while ago, we checked Unity and wrote an article about it\. Click [here](https://pvs-studio.com/en/blog/posts/csharp/0568/) 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](https://github.com/Unity-Technologies/UnityCsReference/tree/98cc8a97afc8cb990bc0c89165bdb276cbcc8ec4)\. Let's go directly to the results of the check\.

## Check results

**Issue 1**

```cpp
private void Draw(Rect windowRect)
{
  var rect = new Rect(....);
  ....
  if (m_NumFilteredVariants > 0)
  {
    ....        
    if (m_NumFilteredVariants > maxFilteredLength)
    {
      GUI.Label(....);
      rect.y += rect.height;
    }
  }
  else
  {
    GUI.Label(rect, "No variants with these keywords");
    rect.y += rect.height;                               // <=
  }

  rect.y = windowRect.height - kMargin - kSpaceHeight – 
    EditorGUI.kSingleLineHeight;                         // <=
  ....
}
```

[V3008](https://pvs-studio.com/en/docs/warnings/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**

```cpp
public static string FetchBuiltinDescription(....)
{
  return string.IsNullOrEmpty(version?.packageInfo?.description) ?
    string.Format(L10n.Tr(....), version.displayName) :
    version.packageInfo.description.Split(....)[0];
}
```

[V3042](https://pvs-studio.com/en/docs/warnings/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**

```cpp
public void SetScaleFocused(Vector2 focalPoint,
                            Vector2 newScale,
                            bool lockHorizontal,
                            bool lockVertical)
{
  if (uniformScale)
    lockHorizontal = lockVertical = false;
  else
  {
    if (hZoomLockedByDefault)
      lockHorizontal = !lockHorizontal;

    if (hZoomLockedByDefault)
      lockVertical = !lockVertical;
  }
....
}
```

[V3029](https://pvs-studio.com/en/docs/warnings/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\.

```cpp
internal class ZoomableArea
{
  ....
  // Zoom lock settings
  public bool hZoomLockedByDefault = false;
  public bool vZoomLockedByDefault = false;
  ....
}
```

It all looks like a simple typo\.

**Issue 4**

```cpp
private void UpdateTextFieldVisibility()
{
  if (showInputField)
  {
    ....
  }
  else if (inputTextField != null && inputTextField.panel != null)
  {
    if (inputTextField.panel != null)                         // <=
      inputTextField.RemoveFromHierarchy();

    inputTextField.UnregisterValueChangedCallback(OnTextFieldValueChange);
    inputTextField.UnregisterCallback<FocusOutEvent>(OnTextFieldFocusOut);
    inputTextField = null;
  }
}
```

[V3022](https://pvs-studio.com/en/docs/warnings/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:

```cpp
public enum EventType
{
  ....
  // Mouse button was released.
  MouseUp = 1,
  ....
  // Already processed event.
  Used = 12,
  ....
}
public static void MinMaxScroller(....)
{
  ....
  if (   Event.current.type == EventType.MouseUp 
      && Event.current.type == EventType.Used) 
  {
    scrollControlID = 0;
  }

  ....
}
```

[V3022](https://pvs-studio.com/en/docs/warnings/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:

```cpp
public static void MinMaxScroller(....)
{
  ....
  if (   Event.current.type == EventType.MouseUp 
      || Event.current.type == EventType.Used) 
  {
    scrollControlID = 0;
  }

  ....
}
```

**Issue 6**

```cpp
private List<T> GetChildrenRecursively(....)
{
  if (result == null)
    result = new List<T>();
  if (m_Children.Any())
  {
    var children = sorted ? (....)m_Children.OrderBy(c => c.key)
                                            .OrderBy(c => c.m_Priority) 
                          : m_Children;
    foreach (var child in children)
      child.GetChildrenRecursively(sorted, result);
  }
  else if (value != null)
    result.Add(value);
  return result;
}
```

[V3078](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/blog/posts/csharp/0928/)\.

By the way, PVS\-Studio found one more similar suspicious code fragment: [V3078](https://pvs-studio.com/en/docs/warnings/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**

```cpp
public void IconSectionGUI(NamedBuildTarget namedBuildTarget,....)
{
  ....
  if (platformUsesStandardIcons)
  {
    var selectedDefault = (m_SelectedPlatform < 0);
    // Set default platform variables
    BuildPlatform platform = null;
    namedBuildTarget = NamedBuildTarget.Standalone;
    ....
  }
  ....
}
```

[V3061](https://pvs-studio.com/en/docs/warnings/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**

```cpp
internal void BeginNamingNewAsset(....)
{
  m_State.m_NewAssetIndexInList = m_LocalAssets.IndexOfNewText(....);
  if (m_State.m_NewAssetIndexInList != -1)
  {
    Frame(instanceID, true, false);
    GetRenameOverlay().BeginRename(newAssetName, instanceID, 0f);
  }
  else
  {
    Debug.LogError("Failed to insert new asset into list");
  }

  Repaint();
}
```

[V3022](https://pvs-studio.com/en/docs/warnings/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:

```cpp
public int IndexOfNewText(....)
{
  int idx = 0;
  if (m_ShowNoneItem)
    idx++;

  for (; idx < m_FilteredHierarchy.results.Length; ++idx)
  {
    FilteredHierarchy.FilterResult r = m_FilteredHierarchy.results[idx];
                    
    if (foldersFirst && r.isFolder && !isCreatingNewFolder)
      continue;
                    
    if (foldersFirst && !r.isFolder && isCreatingNewFolder)
      break;
                    
    string propertyPath = AssetDatabase.GetAssetPath(r.instanceID);
    if (EditorUtility.NaturalCompare(....) > 0)
    {
      return idx;
    }
  }
  return idx;
}
```

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**

```cpp
public static Overlay CreateOverlay(Type type)
{
  ....
  if (overlay == null)
  {
    Debug.LogWarning("Overlay of type {type} can not be instantiated." + ....);
    return null;
  }
  ....
}
```

[V3138](https://pvs-studio.com/en/docs/warnings/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**

```cpp
int GetCurveAtPosition(Vector2 viewPos, out Vector2 closestPointOnCurve)
{
  ....
  for (int i = m_DrawOrder.Count - 1; i >= 0; --i)
  {
    CurveWrapper wrapper = GetCurveWrapperFromID(m_DrawOrder[i]);

    if (wrapper.hidden || wrapper.readOnly || wrapper.curve.length == 0)
      continue;
    ....
  }
}
```

[V3080](https://pvs-studio.com/en/docs/warnings/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_:

```cpp
internal CurveWrapper GetCurveWrapperFromID(int curveID)
{
  if (m_AnimationCurves == null)
    return null;

  int index;
  if (curveIDToIndexMap.TryGetValue(curveID, out index))
    return m_AnimationCurves[index];

  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**

```cpp
internal static void MaterialShaderReferences(....)
{
  var material = context.target as Material;
  if (material == null || !material.shader)
    return;

  indexer.AddReference(context.documentIndex, "shader", material.shader);

  if (!indexer.settings.options.properties)
    return;

  var ownerPropertyType = typeof(Shader);
  var shaderName = $"{material.shader.name}/" ?? string.Empty;   // <=
  ....
}
```

[V3022](https://pvs-studio.com/en/docs/warnings/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**

```cpp
static int CountIntersections(....)
{
  ....
  int hitLength = s_RayCastHits.Length;
  float maxDist = 0;
  if (hitLength > 0)
    maxDist = s_RayCastHits[s_RayCastHits.Length - 1].distance;

  physicsScene.Raycast(....);
  if (s_RayCastHits.Length > 0)
  {
    float len = length - s_RayCastHits[0].distance;
    if (len > maxDist)
    {
      maxDist = len;                                 // <=
    }
  }

  return hitLength + s_RayCastHits.Length;
}
```

[V3137](https://pvs-studio.com/en/docs/warnings/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**

```cpp
public override DragAndDropVisualMode DoDrag(....)
{
  var hierarchyTargetItem = targetItem as GameObjectTreeViewItem;

  if (m_CustomDragHandling != null)
  {
    DragAndDropVisualMode dragResult = 
      m_CustomDragHandling(parentItem as GameObjectTreeViewItem,
                           hierarchyTargetItem,
                           ....);
    ....
  }
  DragAndDropVisualMode dragSceneResult =
    DoDragScenes(parentItem as GameObjectTreeViewItem,
                 hierarchyTargetItem,
                 ....);

  if (   targetItem != null 
      && !IsDropTargetUserModifiable(hierarchyTargetItem, dropPos)) // <=
  {
    return DragAndDropVisualMode.Rejected;
  }
  ....
}
```

[V3019](https://pvs-studio.com/en/docs/warnings/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:

```cpp
static bool IsDropTargetUserModifiable(GameObjectTreeViewItem targetItem, ....)
{
  if (targetItem.isSceneHeader && !targetItem.scene.isLoaded)
    return false;
  ....
}
```

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](https://github.com/Unity-Technologies/UnityCsReference/blob/e740821767d2290238ea7954457333f06e952bad/Editor/Mono/GUI/TreeView/AssetOrGameObjectTreeViewDragging.cs)\.

**Issue 14**

```cpp
static Vector3 ResizeHandlesGUI(....)
{
  ....
  Vector3 scale = Vector3.one; 
  ....
  if (uniformScaling)                                 // <=
  {
    float refScale = (xHandle == 1 ? scale.y : scale.x);
    scale = Vector3.one * refScale;
  }

  if (uniformScaling)                                 // <=
  {
    float refScale = (xHandle == 1 ? scale.y : scale.x);
    scale = Vector3.one * refScale;
  }
  ....
}
```

[V3029](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/pvs-studio/try-free/)\.