This article is about the check of the OpenRA project using the static PVS-Studio analyzer. What is OpenRA? It is an open source game engine designed to create real-time strategies. The article describes the analysis process, project features, and warnings that PVS-Studio has issued. And, of course, here we will discuss some features of the analyzer that made the project checking process more comfortable.
The project chosen for the check is a game engine for RTS in the style of games such as Command & Conquer: Red Alert. More information can be found on the website. The source code is written in C# and is available for viewing and using in the repository.
There were 3 reasons for choosing OpenRA for a review. First, it seems to be of interest to many people. In any case, this applies to the inhabitants of GitHub, since the repository has reached the rating of more than 8 thousand stars. Second, the OpenRA code base contains 1285 files. Usually this amount is quite enough to hope to find interesting warnings in them. And third... Game engines are cool :)
I analyzed OpenRA using PVS-Studio and at first was encouraged by the results:
I decided that among so many High level warnings, I could definitely find a whole lot of different sapid errors. Therefore, based on them, I would write the coolest and most intriguing article :) But no such luck!
One glance at the warnings and everything clicked into place. 1,277 of the 1,306 High level warnings were related to the V3144 diagnostic. It gives messages of the type "This file is marked with a copyleft license, which requires you to open the derived source code". This diagnostic is described in more detail here.
Obviously, I wasn't interested in warnings of such kind, as OpenRA is already an open source project. Therefore, they had to be hidden so that they didn't interfere with viewing the rest of the log. Since I used the Visual Studio plugin, it was easy to do so. I just had to right-click on one of the V3144 warnings and select "Hide all V3144 errors" in the opening menu.
You can also choose which warnings will be displayed in the log by going to the "Detectable Errors (C#)" section in the analyzer options.
To go to them using the plugin for Visual Studio 2019, click on the top menu Extensions->PVS-Studio->Options.
After the V3144 warnings were filtered out, there were significantly fewer warnings in the log:
Nevertheless, I managed to find worthy ones among them.
Quite a few positives pointed to unnecessary checks. This may indicate an error, because people usually don't write this code intentionally. However, in OpenRA, it often looks as if these unnecessary conditions were added on purpose. For example:
public virtual void Tick()
{
....
Active = !Disabled && Instances.Any(i => !i.IsTraitPaused);
if (!Active)
return;
if (Active)
{
....
}
}Analyzer warning: V3022 Expression 'Active' is always true. SupportPowerManager.cs 206
PVS-Studio quite rightly notes that the second check is meaningless, because if Active is false, it won't execute. It might be an error, but I think it was written intentionally. What for? Well, why not?
Perhaps, what we have here is a temporary solution, which was supposed to be refined later. In such cases, it is quite convenient that the analyzer will remind a developer of such shortcomings.
Let's look at another just-in-case check:
Pair<string, bool>[] MakeComponents(string text)
{
....
if (highlightStart > 0 && highlightEnd > highlightStart) // <=
{
if (highlightStart > 0) // <=
{
// Normal line segment before highlight
var lineNormal = line.Substring(0, highlightStart);
components.Add(Pair.New(lineNormal, false));
}
// Highlight line segment
var lineHighlight = line.Substring(
highlightStart + 1,
highlightEnd - highlightStart – 1
);
components.Add(Pair.New(lineHighlight, true));
line = line.Substring(highlightEnd + 1);
}
else
{
// Final normal line segment
components.Add(Pair.New(line, false));
break;
}
....
}Analyzer warning: V3022 Expression 'highlightStart > 0' is always true. LabelWithHighlightWidget.cs 54
Again, it is obvious that re-checking is completely pointless. The value of highlightStart is checked twice, right in neighbouring lines. A mistake? It is possible that in one of the conditions wrong variables are selected for checking. Anyway, it's hard to say for sure what's going on here. One thing is definitely clear - the code should be reviewed and corrected. Or there should be an explanation if additional check is still needed for some reason.
Here is another similar case:
public static void ButtonPrompt(....)
{
....
var cancelButton = prompt.GetOrNull<ButtonWidget>(
"CANCEL_BUTTON"
);
....
if (onCancel != null && cancelButton != null)
{
cancelButton.Visible = true;
cancelButton.Bounds.Y += headerHeight;
cancelButton.OnClick = () =>
{
Ui.CloseWindow();
if (onCancel != null)
onCancel();
};
if (!string.IsNullOrEmpty(cancelText) && cancelButton != null)
cancelButton.GetText = () => cancelText;
}
....
}Analyzer warning: V3063 A part of conditional expression is always true if it is evaluated: cancelButton != null. ConfirmationDialogs.cs 78
cancelButton can be null indeed, because the value returned by the GetOrNull method is written to this variable. However, it stands to reason that by no means will cancelButton turn to null in the body of the conditional operator. Yet the check is still present. If you don't pay attention to the external condition, you happen to be in a very strange situation. First the variable properties are accessed, and then the developer decides to make sure if there is null or not.
At first, I assumed that the project might be using some specific logic related to overloading the "==" operator. In my opinion, implementing something like this in a project for reference types is a controversial idea. Not to mention the fact that unusual behavior makes it harder for other developers to understand the code. At the same time, it is difficult for me to imagine a situation where you can't do without such tricks. Although it is likely that in some specific case this would be a convenient solution.
In the Unity game engine, for example, the "==" operator is redefined for the UnityEngine.Object class. The official documentation available by the link shows that comparing instances of this class with null doesn't work as usual. Well, the developer probably had reasons for implementing this unusual logic.
I didn't find anything like this in OpenRA :). So if there is any meaning in the null checks discussed earlier, it is something else.
PVS-Studio managed to find a few more similar cases, but there is no need to list them all here. Well, it's a bit boring to watch the same triggers. Fortunately (or not), the analyzer was able to find other oddities.
void IResolveOrder.ResolveOrder(Actor self, Order order)
{
....
if (!order.Queued || currentTransform == null)
return;
if (!order.Queued && currentTransform.NextActivity != null)
currentTransform.NextActivity.Cancel(self);
....
}Analyzer warning: V3022 Expression '!order.Queued && currentTransform.NextActivity != null' is always false. TransformsIntoTransforms.cs 44
Once again, we have a pointless check here. However, unlike the previous ones, this is not just an extra condition, but a real unreachable code. The always true checks above didn't actually affect the program's performance. You can remove them from the code, or you can leave them – nothing will change.
Whereas in this case, the strange check results in the fact that a part of the code isn't executed. At the same time, it is difficult for me to guess what changes should be made here as an amendment. In the simplest and most preferable scenario, unreachable code simply shouldn't be executed. Then there is no mistake. However, I doubt that the programmer deliberately wrote the line just for the sake of beauty.
public class CursorSequence
{
....
public readonly ISpriteFrame[] Frames;
public CursorSequence(
FrameCache cache,
string name,
string cursorSrc,
string palette,
MiniYaml info
)
{
var d = info.ToDictionary();
Start = Exts.ParseIntegerInvariant(d["Start"].Value);
Palette = palette;
Name = name;
if (
(d.ContainsKey("Length") && d["Length"].Value == "*") ||
(d.ContainsKey("End") && d["End"].Value == "*")
)
Length = Frames.Length - Start;
else if (d.ContainsKey("Length"))
Length = Exts.ParseIntegerInvariant(d["Length"].Value);
else if (d.ContainsKey("End"))
Length = Exts.ParseIntegerInvariant(d["End"].Value) - Start;
else
Length = 1;
Frames = cache[cursorSrc]
.Skip(Start)
.Take(Length)
.ToArray();
....
}
}Analyzer warning: V3128 The 'Frames' field is used before it is initialized in constructor. CursorSequence.cs 35
A nasty case. An attempt to get the Length property value from an uninitialized variable will inevitably result in the NullReferenceException. In a normal situation, it is unlikely that such an error would have gone unnoticed – yet the inability to create an instance of the class is easily detected. But here the exception will only be thrown if the condition
(d.ContainsKey("Length") && d["Length"].Value == "*") ||
(d.ContainsKey("End") && d["End"].Value == "*")is true.
It is difficult to judge how to correct the code so that everything is fine. I can only assume that the function should look something like this:
public CursorSequence(....)
{
var d = info.ToDictionary();
Start = Exts.ParseIntegerInvariant(d["Start"].Value);
Palette = palette;
Name = name;
ISpriteFrame[] currentCache = cache[cursorSrc];
if (
(d.ContainsKey("Length") && d["Length"].Value == "*") ||
(d.ContainsKey("End") && d["End"].Value == "*")
)
Length = currentCache.Length - Start;
else if (d.ContainsKey("Length"))
Length = Exts.ParseIntegerInvariant(d["Length"].Value);
else if (d.ContainsKey("End"))
Length = Exts.ParseIntegerInvariant(d["End"].Value) - Start;
else
Length = 1;
Frames = currentCache
.Skip(Start)
.Take(Length)
.ToArray();
....
}In this version, the stated problem is absent, but only the developer can tell to what extent it corresponds to the original idea.
public void Resize(int width, int height)
{
var oldMapTiles = Tiles;
var oldMapResources = Resources;
var oldMapHeight = Height;
var oldMapRamp = Ramp;
var newSize = new Size(width, height);
....
Tiles = CellLayer.Resize(oldMapTiles, newSize, oldMapTiles[MPos.Zero]);
Resources = CellLayer.Resize(
oldMapResources,
newSize,
oldMapResources[MPos.Zero]
);
Height = CellLayer.Resize(oldMapHeight, newSize, oldMapHeight[MPos.Zero]);
Ramp = CellLayer.Resize(oldMapRamp, newSize, oldMapHeight[MPos.Zero]);
....
}Analyzer warning: V3127 Two similar code fragments were found. Perhaps, this is a typo and 'oldMapRamp' variable should be used instead of 'oldMapHeight' Map.cs 964
The analyzer detected a suspicious fragment associated with passing arguments to the function. Let's look at the calls separately:
CellLayer.Resize(oldMapTiles, newSize, oldMapTiles[MPos.Zero]);
CellLayer.Resize(oldMapResources, newSize, oldMapResources[MPos.Zero]);
CellLayer.Resize(oldMapHeight, newSize, oldMapHeight[MPos.Zero]);
CellLayer.Resize(oldMapRamp, newSize, oldMapHeight[MPos.Zero]);Oddly enough, the last call passes oldMapHeight, not oldMapRamp. Of course, not all such cases are erroneous. It is quite possible that everything is written correctly here. But you will probably agree that this place looks unusual. I'm inclined to believe that there is an error for sure.
Note by a colleague Andrey Karpov. I don't see anything strange in this code :). It's a classic last line mistake!
If there is no error, then one should add some explanation. After all, if a snippet looks like an error, then someone will want to fix it.
The project revealed very peculiar methods, the return value of which is of the bool type. Their uniqueness lies in the fact that they return true under any conditions. For example:
static bool State(
S server,
Connection conn,
Session.Client client,
string s
)
{
var state = Session.ClientState.Invalid;
if (!Enum<Session.ClientState>.TryParse(s, false, out state))
{
server.SendOrderTo(conn, "Message", "Malformed state command");
return true;
}
client.State = state;
Log.Write(
"server",
"Player @{0} is {1}",
conn.Socket.RemoteEndPoint,
client.State
);
server.SyncLobbyClients();
CheckAutoStart(server);
return true;
}Analyzer warning: V3009 It's odd that this method always returns one and the same value of 'true'. LobbyCommands.cs 123
Is everything OK in this code? Is there an error? It looks extremely strange. Why haven't the developer used void?
It's not surprising that the analyzer finds such a place strange, but we still have to admit that the programmer actually had a reason to write this way. Which one?
I decided to check where this method is called and whether its returned always true value is used. It turned out that there is only one reference to it in the same class – in the commandHandlers dictionary, which has the type
IDictionary<string, Func<S, Connection, Session.Client, string, bool>>During the initialization, the following values are added to it
{"state", State},
{"startgame", StartGame},
{"slot", Slot},
{"allow_spectators", AllowSpectators}and others.
Here we have a rare (I'd like to think so) case of static typing that creates problems for us. After all, to make a dictionary in which the values are functions with different signatures... is at least challenging. commandHandlers is only used in the InterpretCommand method:
public bool InterpretCommand(
S server, Connection conn, Session.Client client, string cmd
)
{
if (
server == null ||
conn == null ||
client == null ||
!ValidateCommand(server, conn, client, cmd)
) return false;
var cmdName = cmd.Split(' ').First();
var cmdValue = cmd.Split(' ').Skip(1).JoinWith(" ");
Func<S, Connection, Session.Client, string, bool> a;
if (!commandHandlers.TryGetValue(cmdName, out a))
return false;
return a(server, conn, client, cmdValue);
}Apparently, the developer intended to have the universal possibility to match strings to certain operations. I think that the chosen method is not the only one, but it is not so easy to offer something more convenient/correct in such a situation. Especially if you don't use dynamic or something like that. If you have any ideas about this, please leave comments. I would be interested to look at various solutions to this problem:).
It turns out that warnings associated with always true methods in this class are most likely false. And yet... What disquiets me here is this ''most likely'' :) One has to really be careful and not miss an actual error among these positives.
All such warnings should be first carefully checked, and then marked as false if necessary. You can simply do it. You should leave a special comment in the place indicated by the analyzer:
static bool State(....) //-V3009There is another way: you can select the warnings that need to be marked as false, and click on "Mark selected messages as False Alarms" in the context menu.
You can learn more about this topic in the documentation.
static bool SyncLobby(....)
{
if (!client.IsAdmin)
{
server.SendOrderTo(conn, "Message", "Only the host can set lobby info");
return true;
}
var lobbyInfo = Session.Deserialize(s);
if (lobbyInfo == null) // <=
{
server.SendOrderTo(conn, "Message", "Invalid Lobby Info Sent");
return true;
}
server.LobbyInfo = lobbyInfo;
server.SyncLobbyInfo();
return true;
}Analyzer warning: V3022 Expression 'lobbyInfo == null' is always false. LobbyCommands.cs 851
Here we have another method that always returns true. However, this time we are looking at a different type of a warning. We have to pore over such places with all diligence, as there is no guarantee that we deal with redundant code. But first things first.
The Deserialize method never returns null – you can easily see this by looking at its code:
public static Session Deserialize(string data)
{
try
{
var session = new Session();
....
return session;
}
catch (YamlException)
{
throw new YamlException(....);
}
catch (InvalidOperationException)
{
throw new YamlException(....);
}
}For ease of reading, I have shortened the source code of the method. You can see it in full by clicking on the link. Or take my word for it that the session variable doesn't turn to null under any circumstances.
So what do we see at the bottom part? Deserialize doesn't return null, and if something goes wrong, it throws exceptions. The developer who wrote the null check after the call was of a different mind, apparently. Most likely, in an exceptional situation, the SyncLobby method should execute the code that is currently being executed... Actually, it is never executed, because lobbyInfo is never null:
if (lobbyInfo == null)
{
server.SendOrderTo(conn, "Message", "Invalid Lobby Info Sent");
return true;
}I believe that instead of this "extra" check, the author still needs to use try-catch. Or try another tack and write, let's say, TryDeserialize, which in case of an exceptional situation will return null.
public ConnectionSwitchModLogic(....)
{
....
var logo = panel.GetOrNull<RGBASpriteWidget>("MOD_ICON");
if (logo != null)
{
logo.GetSprite = () =>
{
....
};
}
if (logo != null && mod.Icon == null) // <=
{
// Hide the logo and center just the text
if (title != null)
title.Bounds.X = logo.Bounds.Left;
if (version != null)
version.Bounds.X = logo.Bounds.X;
width -= logo.Bounds.Width;
}
else
{
// Add an equal logo margin on the right of the text
width += logo.Bounds.Width; // <=
}
....
}Analyzer warning: V3125 The 'logo' object was used after it was verified against null. Check lines: 236, 222. ConnectionLogic.cs 236
As for this case, I'm sure as hell there is an error. We are definitely not looking at "extra" checks, because the GetOrNull method can indeed return a null reference. What happens if logo is null? Accessing the Bounds property will result in an exception, which was clearly not part of the developer's plans.
Perhaps, the fragment needs to be rewritten in the following way:
if (logo != null)
{
if (mod.Icon == null)
{
// Hide the logo and center just the text
if (title != null)
title.Bounds.X = logo.Bounds.Left;
if (version != null)
version.Bounds.X = logo.Bounds.X;
width -= logo.Bounds.Width;
}
else
{
// Add an equal logo margin on the right of the text
width += logo.Bounds.Width;
}
}This option is quite simple for comprehension, although the additional nesting doesn't look too great. As a more comprehensive solution, one could use the null-conditional operator:
// Add an equal logo margin on the right of the text
width += logo?.Bounds.Width ?? 0; // <=By the way, the first version looks more preferable to me. It is easy to read it and triggers no questions. But some developers appreciate brevity quite highly, so I also decided to cite the second version as well :).
public MapEditorLogic(....)
{
var editorViewport = widget.Get<EditorViewportControllerWidget>("MAP_EDITOR");
var gridButton = widget.GetOrNull<ButtonWidget>("GRID_BUTTON");
var terrainGeometryTrait = world.WorldActor.Trait<TerrainGeometryOverlay>();
if (gridButton != null && terrainGeometryTrait != null) // <=
{
....
}
var copypasteButton = widget.GetOrNull<ButtonWidget>("COPYPASTE_BUTTON");
if (copypasteButton != null)
{
....
}
var copyFilterDropdown = widget.Get<DropDownButtonWidget>(....);
copyFilterDropdown.OnMouseDown = _ =>
{
copyFilterDropdown.RemovePanel();
copyFilterDropdown.AttachPanel(CreateCategoriesPanel());
};
var coordinateLabel = widget.GetOrNull<LabelWidget>("COORDINATE_LABEL");
if (coordinateLabel != null)
{
....
}
....
}Analyzer warning: V3063 A part of conditional expression is always true if it is evaluated: terrainGeometryTrait != null. MapEditorLogic.cs 35
Let's delve into this fragment. Note that each time the GetOrNull method of the Widget class is used, a null equality check is performed. However, if Get is used, there is no check. This is logical – the Get method doesn't return null:
public T Get<T>(string id) where T : Widget
{
var t = GetOrNull<T>(id);
if (t == null)
throw new InvalidOperationException(....);
return t;
}If the element is not found, an exception is thrown – this is reasonable behavior. At the same time, the logical option would be to check the values returned by the GetOrNull method for equality to the null reference.
In the code above, the value returned by the Trait method is checked for null. Actually it is inside the Trait method where Get of the TraitDictionary class is called:
public T Trait<T>()
{
return World.TraitDict.Get<T>(this);
}Can it be that this Get behaves differently from the one we discussed earlier? Well, the classes are different. Let's check it out:
public T Get<T>(Actor actor)
{
CheckDestroyed(actor);
return InnerGet<T>().Get(actor);
}The InnerGet method returns an instance of TraitContainer<T>. The Get implementation in this class is very similar to Get of the Widget class:
public T Get(Actor actor)
{
var result = GetOrDefault(actor);
if (result == null)
throw new InvalidOperationException(....);
return result;
}The main similarity is that null is never returned here either. If something goes wrong, an InvalidOperationException is similarly thrown. Therefore, the Trait method behaves the same way.
Yes, there may just be an extra check that doesn't affect anything. Except that it looks strange, but you can't say that this code will confuse a reader much. But if the check is needed indeed, then in some cases an exception will be thrown unexpectedly. It is sad.
So in this fragment it seems more appropriate to call, for example, TraitOrNull. However, there is no such method:). But there is TraitOrDefault, which is the equivalent of GetOrNull for this case.
There is another similar case related to the Get method:
public AssetBrowserLogic(....)
{
....
frameSlider = panel.Get<SliderWidget>("FRAME_SLIDER");
if (frameSlider != null)
{
....
}
....
}Analyzer warning: V3022 Expression 'frameSlider != null' is always true. AssetBrowserLogic.cs 128
The same as in the code considered earlier, there is something wrong here. Either the check is really unnecessary, or one still needs to call GetOrNull instead of Get.
public SpawnSelectorTooltipLogic(....)
{
....
var textWidth = ownerFont.Measure(labelText).X;
if (textWidth != cachedWidth)
{
label.Bounds.Width = textWidth;
widget.Bounds.Width = 2 * label.Bounds.X + textWidth; // <=
}
widget.Bounds.Width = Math.Max( // <=
teamWidth + 2 * labelMargin,
label.Bounds.Right + labelMargin
);
team.Bounds.Width = widget.Bounds.Width;
....
}Analyzer warning : V3008 The 'widget.Bounds.Width' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 78, 75. SpawnSelectorTooltipLogic.cs 78
It seems that if the textWidth != cachedWidth condition is true, widget.Bounds.Width must be written to a specific value for this case. However, an assignment made below, regardless of whether this condition is true, makes the string
widget.Bounds.Width = 2 * label.Bounds.X + textWidth;pointless. It is likely that the author just forgot to write else here:
if (textWidth != cachedWidth)
{
label.Bounds.Width = textWidth;
widget.Bounds.Width = 2 * label.Bounds.X + textWidth;
}
else
{
widget.Bounds.Width = Math.Max(
teamWidth + 2 * labelMargin,
label.Bounds.Right + labelMargin
);
}public void DisguiseAs(Actor target)
{
....
var tooltip = target.TraitsImplementing<ITooltip>().FirstOrDefault();
AsPlayer = tooltip.Owner;
AsActor = target.Info;
AsTooltipInfo = tooltip.TooltipInfo;
....
}Analyzer warning: V3146 Possible null dereference of 'tooltip'. The 'FirstOrDefault' can return default null value. Disguise.cs 192
When is FirstOrDefault usually used instead of First? If the selection is empty, First throws an InvalidOperationException. FirstOrDefault doesn't throw an exception, but returns null for the reference type.
The ITooltip interface implements various classes in the project. Thus, if target.TraitsImplementing<ITooltip>() returns an empty selection, null is written to tooltip. Accessing the properties of this object, which is executed next, will result in a NullReferenceException.
In cases where the developer is sure that the selection won't be empty, it is better to use First. If one isn't sure, it's worth checking the value returned by FirstOrDefault. It is rather strange that we don't see it here. After all, the values returned by the GetOrNull method mentioned earlier were always checked. Why didn't they do it here?
Who knows?.. All right, the developer will answer these questions for sure. In the end, it is the code author who will be fixing it :)
OpenRA somehow turned out to be a project that was nice and interesting to scan. The developers did a lot of work and didn't forget that the source code should be easy to view. Of course, we did find some... controversies, but one can't simply do without them :)
At the same time, even with all the effort, alas, developers remain people. Some of the considered warnings are extremely difficult to notice without using the analyzer. It is sometimes difficult to find an error even immediately after writing it. Needless to say, how hard it is to search for error after a long time.
Obviously, it is much better to detect an error than its consequences. To do this, you can spend hours rechecking a huge number of new sources manually. Well, and have a bit of a look at the old ones - what if there is an oversight there? Yes, reviews are really useful, but if you have to view a large amount of code, then you stop noticing some things over time. And it takes a lot of time and effort.
Static analysis is just a convenient addition to other methods of checking the quality of source code, such as code review. PVS-Studio will find "simple" (and sometimes tricky) errors instead of a developer, allowing people to focus on more serious issues.
Yes, the analyzer sometimes gives false positives and is not able to find all the errors. But with it you will save a lot of time and nerves. Yes, it is not perfect and sometimes makes mistakes itself. However, in general, PVS-Studio makes the development process much easier, more enjoyable, and even (unexpectedly!) cheaper :).
In fact, you don't need to take my word for it - it's much better to make sure that the above is true yourself. You can use the link to download the analyzer and get a trial key. What could be simpler? :)
Well, that's it for this time. Thanks for your attention! I wish you clean code and an empty error log!