本文致力于使用PVS-Studio静态分析器检查OpenRA项目。什么是OpenRA？它是用于创建实时策略游戏的开源游戏引擎。本文介绍了如何进行分析，发现了项目本身的哪些功能以及PVS-Studio带来了哪些有趣的触发因素。当然，这里我们将考虑分析仪的某些功能，这些功能使项目验证过程更加舒适。
OpenRA
选择进行审查的项目是RTS游戏引擎，其风格类似于Command＆Conquer：Red Alert。可以在网站上找到更多信息。源代码用C＃编写，可在存储库中查看和使用。
选择OpenRA的原因有3个。首先，它似乎引起了很多人的兴趣。无论如何，这都适用于GitHub上的居民，因为该存储库已收集了8000多个星星。其次，OpenRA代码库包含1285个文件。通常，此金额足以在其中找到有趣的触发器。第三，游戏引擎很棒。
额外的积极
我使用PVS-Studio分析了OpenRA，最初受到了结果的启发：
我认为，在众多警告中，我当然可以找到很多不同的答案。而且，当然，在他们的基础上，我将写出最酷，最有趣的文章。但是它不在那里！
一个人只需要看一下警告本身，一切便立即就位。在1306个高警告中，有1277个与V3144诊断相关。它显示类似“此文件已标记为copyleft许可证，这需要您打开派生的源代码”之类的消息。此诊断将在此处更详细地描述。
显然，这样的计划对我完全没有兴趣，因为OpenRA已经是一个开源项目。因此，需要将它们隐藏起来，以免干扰查看其余日志。由于我使用的是Visual Studio插件，因此很容易做到。您只需要右键单击V3144警报之一，然后在打开的菜单中选择“隐藏所有V3144错误”。
您还可以通过转到分析器选项中的“可检测错误（C＃）”部分，选择将在日志中显示的警告。
为了使用Visual Studio 2019的插件访问它们，您需要单击顶部菜单扩展-> PVS-Studio->选项。
检测结果
之后V3144触发器被过滤掉了，有日志中显著较少的警告：
然而，我们设法在其中找到了有趣的时刻。
没有意义的条件
少数触发器指示不必要的检查。这可能表明存在错误，因为通常人们不会故意编写此类代码。但是，在OpenRA中，很多时候似乎是故意添加了这些不必要的条件。例如：
public virtual void Tick()
{
....
Active = !Disabled && Instances.Any(i => !i.IsTraitPaused);
if (!Active)
return;
if (Active)
{
....
}
}分析器警告：V3022表达式“活动”始终为true。 SupportPowerManager.cs 206
PVS-Studio非常正确地指出，第二项检查是没有意义的，因为如果Active为false，则执行将无法进行。在这里可能是一个错误，但我认为它是故意写的。做什么的？好吧，为什么不呢？
也许我们面前有一种临时解决方案，其修订版留待以后使用。在这种情况下，分析仪会提醒开发人员这种缺陷非常方便。
让我们再看看“以防万一”：
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;
}
....
}分析器警告：V3022表达式'highlightStart> 0'始终为true。 LabelWithHighlightWidget.cs 54
同样，很明显，重新检查完全没有意义。HighlightStart值在相邻的行中被检查了两次。错误？在一种情况下，可能选择了错误的变量进行测试。无论哪种方式，都很难确定这是什么。有一件事很清楚-需要研究和更正代码，或者如果由于某种原因仍需要进行额外检查，则应留下解释。
这是另一个类似的观点：
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;
}
....
}分析器警告：V3063如果条件表达式的一部分被求值，则始终为true：cancelButton！= Null。 ConfirmationDialogs.cs 78
cancelButton的确可以为null，因为GetOrNull方法返回的值已写入此变量。但是，逻辑上要考虑的是，在条件语句的主体中cancelButton不会变成null。但是，仍然存在检查。如果您不注意外部条件，那么通常会遇到一种奇怪的情况：首先，访问变量的属性，然后开发人员决定确定-它是否仍然为null？
首先，我假设项目可能正在使用一些与重载“ ==”运算符有关的特定逻辑。我认为，在项目中为引用类型实现类似的操作是一个有争议的想法。尽管如此，异常行为仍使其他开发人员更难理解代码。同时，我很难想象无法放弃这种窍门的情况。尽管在某些特定情况下，这可能是一个方便的解决方案。
例如，在Unity游戏引擎中，为UnityEngine.Object类重新定义了“ == ”运算符。链接上的官方文档显示，将此类的实例与null进行比较不能照常工作。好吧，开发人员肯定有理由实施这种不寻常的逻辑。
我在OpenRA中找不到类似的东西：）。因此，如果以前考虑过的null检查存在任何意义，则它包含其他内容。
PVS-Studio能够找到更多类似的时刻，但无需在此处列出所有这些时刻。看到同样的积极性仍然很无聊。幸运的是（或者没有）分析仪也能够发现其他异常情况。
无法访问的代码
void IResolveOrder.ResolveOrder(Actor self, Order order)
{
....
if (!order.Queued || currentTransform == null)
return;
if (!order.Queued && currentTransform.NextActivity != null)
currentTransform.NextActivity.Cancel(self);
....
}分析器警告：V3022表达式'！Order.Queued && currentTransform.NextActivity！= Null'始终为false。 TransformsIntoTransforms.cs 44
同样，我们进行了无意义的检查。但是，与以前的代码不同，这里给出的代码不仅是一个额外的条件，而且是真正无法实现的代码。实际上，以前认为始终为true的检查实际上并不影响程序的运行。它们可以从代码中删除，也可以保留-不会改变。
在这里，奇怪的检查导致部分代码没有执行的事实。同时，我很难猜测应该在此处进行哪些更改作为修正。在最简单，最愉快的情况下，不应执行无法访问的代码。那就没有错。但是，我怀疑程序员是否故意为美而写这行代码。
构造函数中未初始化的变量
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();
....
}
}分析器警告：V3128在构造函数中初始化“框架”字段之前，请先使用该字段。CursorSequence.cs 35
一个非常不愉快的时刻。试图从未初始化的变量中获取Length属性的值将不可避免地引发NullReferenceException。在正常情况下，这样的错误几乎不会被忽略-尽管如此，创建类实例的可能性很容易暴露出来。但是这里只有在条件满足的情况下才会抛出异常
(d.ContainsKey("Length") && d["Length"].Value == "*") ||
(d.ContainsKey("End") && d["End"].Value == "*")会是真的。
很难判断您需要如何修复代码才能使一切正常运行。我只能假设该函数应如下所示：
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();
....
}在此版本中，没有指定的问题，但是，只有开发人员才能说出它与原始想法对应的程度。
潜在的错字
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]);
....
}分析器警告：V3127找到两个相似的代码片段。也许这是一个错字，应该使用'oldMapRamp'变量而不是'oldMapHeight'Map.cs 964
分析器检测到与将参数传递给函数有关的可疑时刻。让我们分别看一下这些调用：
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]);奇怪的是，最后一次调用传递了oldMapHeight，而不是oldMapRamp。当然，并非所有此类情况都是错误的。很有可能在这里正确编写了所有内容。但是您必须承认，这个地方看起来很特别。我倾向于相信这里确实犯了一个错误。
同事安德烈·卡波夫（Andrey Karpov）的笔记。而且我在这段代码中看不到任何奇怪的地方。这是经典的最后一行错误！
如果此处仍然没有错误，则值得添加一些说明。毕竟，如果片刻看起来像是一个错误，那么肯定会有人修复它。
真实，真实，只有真实
该项目包含一些非常特殊的方法，其返回值为bool类型。它们的独特之处在于在任何情况下它们都返回true的事实。例如：
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;
}分析器警告：V3009奇怪的是，此方法始终返回一个相同的'true'值。LobbyCommands.cs 123
这个代码可以吗？有错吗 看起来很奇怪。开发人员为什么不使用void？
分析器认为这样的地方很奇怪也就不足为奇了，但是我们仍然必须承认程序员实际上有理由以这种方式编写。哪一个？
我决定查看在何处调用此方法，以及是否正在使用其始终为true的返回值。原来，在同一类中只有一个引用-在commandHandlers字典中，该字典的类型为
IDictionary<string, Func<S, Connection, Session.Client, string, bool>>在初始化期间，将值添加到其中
{"state", State},
{"startgame", StartGame},
{"slot", Slot},
{"allow_spectators", AllowSpectators}等等
当静态类型给我们带来问题时，我们将遇到一种罕见的情况（我想相信）。毕竟，制作一个其中值将是具有不同签名的函数的字典...至少是有问题的。commandHandlers仅在InterpretCommand方法中使用：
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);
}显然，开发人员的目标是对执行的某些操作的字符串进行匹配的通用功能。我认为他选择的方式远非唯一，但是在这种情况下提供更方便/更正确的方法并非易事。特别是如果您不使用某种动态或类似方法。如果您对此主题有任何想法，请发表评论。对于我来说，解决这个问题的各种选择将是很有趣的。
事实证明，与此类中始终为true的方法相关的警告很可能为false。但是……正是这种“最有可能的”使您感到恐惧，您需要对这些事情保持谨慎，因为它们之间确实确实存在错误。
所有这些肯定的结果都应仔细检查，然后在必要时标记为“假”。这很简单。在分析仪指示的地方应留下特殊注释：
static bool State(....) //-V3009还有另一种方法：您可以选择需要标记为肯定的肯定，然后在上下文菜单中单击“将所选消息标记为假警报”。
您可以在文档中了解有关此主题的更多信息。
是否需要额外检查null？
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;
}分析器警告：V3022表达式'lobbyInfo == null'始终为false。 LobbyCommands.cs 851
始终返回true的另一种方法。但是，这次我们正在研究另一种类型的触发器。您需要足够仔细地研究这些事情，因为这与多余的代码相去甚远。但是首先是第一件事。Deserialize
方法永远不会返回null-您可以通过查看其代码轻松地验证此结果：
public static Session Deserialize(string data)
{
try
{
var session = new Session();
....
return session;
}
catch (YamlException)
{
throw new YamlException(....);
}
catch (InvalidOperationException)
{
throw new YamlException(....);
}
}为了缩短可读性，我已经缩短了该方法的源代码。您可以通过以下链接完整查看它。好吧，或者我相信，这里的session变量在任何情况下都不会变为null。
我们在底部看到什么？反序列化不会返回null，如果出现问题会引发异常。调用后为空写检查的开发人员似乎有不同的想法。在一种特殊情况下，最有可能的是，SyncLobby方法应执行当前正在执行的代码...是的，它永远不会执行，因为lobbyInfo从不为null：
if (lobbyInfo == null)
{
server.SendOrderTo(conn, "Message", "Invalid Lobby Info Sent");
return true;
}我相信，除了此“额外”检查之外，您还应该使用try - catch。好吧，或者从另一边写一些TryDeserialize，如果发生异常，它将返回null。
可能的NullReferenceException
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; // <=
}
....
}分析器警告：V3125验证了null后使用了“徽标”对象。检查行：236，222。ConnectionLogic.cs 236
某些信息告诉我这里有一个100％错误。我们绝对没有在我们前面进行“额外”检查，因为GetOrNull方法很可能可以真正返回空引用。如果徽标为空会怎样？调用Bounds属性将引发异常，这显然不在开发人员的计划中。
也许该片段需要重写如下：
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;
}
}尽管附加的嵌套看起来并不酷，但此选项很容易理解。作为更宽敞的解决方案，您可以使用空条件运算符：
// Add an equal logo margin on the right of the text
width += logo?.Bounds.Width ?? 0; // <=请注意，我更喜欢顶部修复。读起来很愉快，没有问题。但是有些开发人员高度重视简洁性，因此我决定也提供第二种选择：）。
毕竟这是OrDefault吗？
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)
{
....
}
....
}分析器警告：V3063如果对条件表达式进行评估，则条件表达式的一部分始终为true：terrainGeometryTrait！= Null。 MapEditorLogic.cs 35
让我们分析一下这段代码。请注意，每次使用Widget类的GetOrNull方法时，都会将其检查为null。但是，如果使用Get，则不会进行验证。这是逻辑的-Get方法不会返回null：
public T Get<T>(string id) where T : Widget
{
var t = GetOrNull<T>(id);
if (t == null)
throw new InvalidOperationException(....);
return t;
}如果未找到该元素，则将引发异常-这是合理的行为。同时，逻辑选项是检查由GetOrNull方法返回的值是否等于空引用。
在上面的代码中，检查Trait方法返回的值是否为null。实际上，在Trait方法内部，从TraitDictionary类调用Get：
public T Trait<T>()
{
return World.TraitDict.Get<T>(this);
}难道这个Get的行为与前面讨论的有所不同？但是类别是不同的。让我们检查：
public T Get<T>(Actor actor)
{
CheckDestroyed(actor);
return InnerGet<T>().Get(actor);
}InnerGet 方法返回TraitContainer <T>的实例。实施获取这一类非常相似，获取在窗口小部件类：
public T Get(Actor actor)
{
var result = GetOrDefault(actor);
if (result == null)
throw new InvalidOperationException(....);
return result;
}主要的相似之处是null也不在这里返回。万一出了问题，同样会抛出InvalidOperationException。因此，特质方法的行为方式相同。
是的，这里可能只需要进行额外的检查，这不会影响任何内容。也许看起来很奇怪，但是不能说这样的代码会使读者大为困惑。但是，如果仅在此处需要进行检查，则在某些情况下会意外抛出异常。这是可悲的。
在这一点上，调用一些TraitOrNull似乎更合适。但是，没有这样的方法：）。但是有TraitOrDefault，它类似于GetOrNull。对于这种情况。
还有另一个与Get方法有关的要点：
public AssetBrowserLogic(....)
{
....
frameSlider = panel.Get<SliderWidget>("FRAME_SLIDER");
if (frameSlider != null)
{
....
}
....
}分析器警告：V3022表达式'frameSlider！= Null'始终为true。AssetBrowserLogic.cs 128
与上面的代码一样，此处出了点问题。要么检查实际上是不必要的，要么代替Get，您仍然需要调用GetOrNull。
作业丢失
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;
....
}分析仪警告：V3008``widget.Bounds.Width ''变量已连续两次分配了值。也许这是一个错误。检查行：78，75. SpawnSelectorTooltipLogic.cs 78
看起来如果textWidth！= CachedWidth条件为true，则应将一些特定于大小写的值写入widget.Bounds.Width。但是，无论此条件是否为真，下面的赋值都会剥夺字符串
widget.Bounds.Width = 2 * label.Bounds.X + textWidth;每一种感觉。他们很可能只是忘了把其他东西放在这里：
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;
....
}分析器警告：V3146可能对'tooltip'的null取消引用。 “ FirstOrDefault”可以返回默认的空值。 Disguise.cs 192
什么时候通常使用FirstOrDefault代替First？如果选择为空，则First将抛出InvalidOperationException。FirstOrDefault将不会引发异常，但是将为引用类型返回null。
在项目中，ITooltip接口由各种类实现。因此，如果target.TraitsImplementing <ITooltip>（）返回空选择，则该工具提示将被写为null... 进一步访问此对象的属性将导致NullReferenceException。
如果开发人员确定选择不会为空，则使用First更为正确。如果不确定，则应检查FirstOrDefault返回的值。奇怪的是，这里不存在。毕竟，始终会检查之前讨论的GetOrNull方法返回的值。他们为什么不在这里做？
谁知道...哦，是的！开发人员肯定可以回答这些问题。最后，他应该编辑此代码。
结论
某种程度上，OpenRA证明是一个令人愉快且有趣的项目。开发人员做得很好，同时也没有忘记源应该易于学习。当然，这里有不同的...争议点，但是没有它们。
同时，即使他们全力以赴，开发人员（alas）仍然是人类。如果不使用分析仪，则很难发现其中一些阳性。有时即使在编写错误后也很难立即发现错误。经过很长一段时间后，我们能说些什么。
显然，发现错误比其后果要好得多。您可以花几个小时手动为此重新检查大量新资源。好吧，那些旧的同时看-突然间没有注意到任何错误吗？是的，复查确实很有用，但是如果您需要仔细阅读大量代码，那么随着时间的流逝，您将不再注意到某些事情。并且花费了大量时间和精力。
静态分析只是其他检查源代码质量的方法（例如代码审查）的一种方便补充。 PVS-Studio会发现“简单的”（有时不仅是唯一的）错误，而不是开发人员，从而使人们可以专注于更严重的问题。
是的，分析仪有时会产生误报，根本无法发现所有错误。但是使用它可以节省大量时间和精力。是的，他并不完美，有时自己会犯错误。但是，总的来说，PVS-Studio使开发过程变得更加轻松，愉悦，甚至更便宜。
实际上，您不需要信服我-验证上述事实的真实性会更好。按照链接下载分析仪并获得试用密钥。有多少容易？
好，仅此而已。感谢您的关注！希望您清除代码并清除错误日志！
如果您想与说英语的人分享这篇文章，请使用翻译链接：Nikita Lipilin。独角兽闯入RTS：分析OpenRA源代码。