Dieser Artikel befasst sich mit der Überprüfung des OpenRA-Projekts mit dem statischen Analysator PVS-Studio. Was ist OpenRA? Es ist eine Open-Source-Spiel-Engine zum Erstellen von Echtzeit-Strategiespielen. Der Artikel beschreibt, wie die Analyse durchgeführt wurde, welche Merkmale des Projekts selbst entdeckt wurden und welche interessanten Auslöser PVS-Studio gab. Und natürlich werden wir hier einige der Funktionen des Analysators betrachten, die den Projektüberprüfungsprozess komfortabler gemacht haben.
OpenRA
Das zur Überprüfung ausgewählte Projekt ist eine RTS-Spiel-Engine im Stil von Spielen wie Command & Conquer: Red Alert. Weitere Informationen finden Sie auf der Website . Der Quellcode ist in C # geschrieben und kann im Repository angezeigt und verwendet werden .
OpenRA wurde aus drei Gründen zur Überprüfung ausgewählt. Erstens scheint es für viele Menschen von Interesse zu sein. Dies gilt in jedem Fall für die Bewohner von GitHub, da das Repository mehr als 8.000 Sterne gesammelt hat. Zweitens enthält die OpenRA-Codebasis 1285 Dateien. Normalerweise reicht diese Menge aus, um hoffentlich interessante Auslöser in ihnen zu finden. Und drittens ... Game Engines sind cool.
Extra positive
Ich habe OpenRA mit PVS-Studio analysiert und war zunächst von den Ergebnissen inspiriert:
Ich entschied, dass ich unter so vielen Warnungen sicherlich viele verschiedene Antworten finden kann. Und natürlich werde ich auf ihrer Grundlage den coolsten und interessantesten Artikel schreiben. Aber es war nicht da!
Man musste sich nur die Warnungen selbst ansehen, und alles passte sofort zusammen. Von den 1306 hohen Warnungen waren 1.277 mit der V3144- Diagnose verbunden . Es werden Meldungen wie "Diese Datei ist mit einer Copyleft-Lizenz gekennzeichnet, für die Sie den abgeleiteten Quellcode öffnen müssen" angezeigt. Diese Diagnose wird hier ausführlicher beschrieben .
Offensichtlich hat mich die Funktionsweise eines solchen Plans überhaupt nicht interessiert, da OpenRA bereits ein Open-Source-Projekt ist. Daher mussten sie ausgeblendet werden, damit sie den Rest des Protokolls nicht beeinträchtigten. Da ich ein Plugin für Visual Studio verwendet habe, war dies einfach. Sie mussten nur mit der rechten Maustaste auf eine der V3144-Warnungen klicken und im folgenden Menü "Alle V3144- Fehler ausblenden " auswählen .
Sie können auch auswählen, welche Warnungen im Protokoll angezeigt werden sollen, indem Sie in den Analysatoroptionen zum Abschnitt "Erkennbare Fehler (C #)" gehen.
Um mit dem Plugin für Visual Studio 2019 zu ihnen zu gelangen, müssen Sie auf das Hauptmenü Erweiterungen-> PVS-Studio-> Optionen klicken.
Testergebnisse
Nachdem die V3144-Trigger herausgefiltert wurden, enthält das Protokoll deutlich weniger Warnungen:
Trotzdem haben wir interessante Momente unter ihnen gefunden.
Sinnlose Bedingungen
Nicht wenige Auslöser wiesen auf unnötige Überprüfungen hin. Dies kann auf einen Fehler hinweisen, da normalerweise kein solcher Code absichtlich geschrieben wird. In OpenRA sieht es jedoch ziemlich oft so aus, als ob diese unnötigen Bedingungen absichtlich hinzugefügt wurden. Zum Beispiel:
public virtual void Tick()
{
....
Active = !Disabled && Instances.Any(i => !i.IsTraitPaused);
if (!Active)
return;
if (Active)
{
....
}
}Analyzer-Warnung : V3022 Der Ausdruck 'Aktiv' ist immer wahr. SupportPowerManager.cs 206
PVS-Studio ganz zu Recht aus, dass die zweite Prüfung bedeutungslos ist, weil , wenn aktiv ist falsch , dann ist die Ausführung wird es nicht erreichen. Es könnte ein Fehler sein, aber ich denke, es wurde absichtlich geschrieben. Wozu? Gut, warum nicht?
Vielleicht haben wir eine Art vorübergehende Lösung vor uns, deren Überarbeitung "für später" bleibt. In solchen Fällen ist es sehr praktisch, dass der Analysator den Entwickler an solche Mängel erinnert.
Schauen wir uns noch einen Check "nur für den Fall" an:
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-Warnung : V3022 Der Ausdruck 'HighlightStart> 0' ist immer wahr. LabelWithHighlightWidget.cs 54
Auch hier ist klar, dass eine erneute Überprüfung völlig bedeutungslos ist. Der HighlightStart- Wert wird zweimal und in benachbarten Zeilen überprüft. Error? Es ist möglich, dass unter einer der Bedingungen die falschen Variablen zum Testen ausgewählt wurden. In jedem Fall ist es schwer zu sagen, worum es geht. Eines ist klar: Der Code muss untersucht und korrigiert werden, oder es sollte eine Erklärung hinterlassen werden, wenn aus irgendeinem Grund noch eine zusätzliche Überprüfung erforderlich ist.
Hier ist ein weiterer ähnlicher Punkt:
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-Warnung : V3063 Ein Teil des bedingten Ausdrucks ist immer wahr, wenn er ausgewertet wird: cancelButton! = Null. ConfirmationDialogs.cs 78
cancelButton kann tatsächlich null sein , da der von der GetOrNull- Methode zurückgegebene Wert in diese Variable geschrieben wird . Es ist jedoch logisch anzunehmen, dass cancelButton im Hauptteil des bedingten Operators in keiner Weise null wird . Trotzdem gibt es noch einen Scheck. Wenn Sie den externen Zustand nicht beachten, stellt sich im Allgemeinen eine seltsame Situation heraus: Zuerst wird auf die Eigenschaften der Variablen zugegriffen, und dann hat der Entwickler entschieden, sicherzustellen, dass sie immer noch null ist oder nicht.
Zuerst nahm ich an, dass das Projekt möglicherweise eine bestimmte Logik verwendet, die mit dem Überladen des Operators "==" zusammenhängt. Meiner Meinung nach ist es eine kontroverse Idee, etwas Ähnliches für Referenztypen in einem Projekt zu implementieren. Das ungewöhnliche Verhalten erschwert es anderen Entwicklern jedoch, den Code zu verstehen. Gleichzeitig fällt es mir schwer, mir eine Situation vorzustellen, in der auf solche Tricks nicht verzichtet werden kann. Obwohl es wahrscheinlich ist, dass dies in einem bestimmten Fall eine bequeme Lösung wäre.
In der Unity-Spiel-Engine wird beispielsweise der Operator " == " für die UnityEngine.Object- Klasse neu definiert . Die offizielle Dokumentation unter dem Link zeigt, dass Instanzen dieser Klasse mit null verglichen werdenfunktioniert nicht wie gewohnt. Nun, sicherlich hatten die Entwickler Gründe, solch ungewöhnliche Logik zu implementieren.
So etwas habe ich in OpenRA nicht gefunden :). Wenn also die zuvor betrachteten Prüfungen auf Null einen Sinn haben, dann besteht sie aus etwas anderem.
PVS-Studio konnte mehrere ähnliche Momente erkennen, es ist jedoch nicht erforderlich, alle hier aufzulisten. Es ist immer noch langweilig, die gleichen positiven Ergebnisse zu sehen. Glücklicherweise (oder auch nicht) konnte der Analysator auch andere Kuriositäten finden.
Unerreichbarer Code
void IResolveOrder.ResolveOrder(Actor self, Order order)
{
....
if (!order.Queued || currentTransform == null)
return;
if (!order.Queued && currentTransform.NextActivity != null)
currentTransform.NextActivity.Cancel(self);
....
}Analyzer-Warnung : V3022- Ausdruck '! Order.Queued && currentTransform.NextActivity! = Null' ist immer falsch. TransformsIntoTransforms.cs 44
Wieder haben wir eine sinnlose Prüfung. Im Gegensatz zu den vorherigen wird hier jedoch nicht nur eine zusätzliche Bedingung dargestellt, sondern ein wirklich unerreichbarer Code. Die zuvor als immer wahr angesehenen Überprüfungen hatten tatsächlich keinen Einfluss auf den Betrieb des Programms. Sie können aus dem Code entfernt oder verlassen werden - nichts wird sich ändern.
Hier führt eine seltsame Überprüfung dazu, dass ein Teil des Codes nicht ausgeführt wird. Gleichzeitig fällt es mir schwer zu erraten, welche Änderungen hier als Änderung vorgenommen werden sollten. Im einfachsten und angenehmsten Fall sollte der nicht erreichbare Code einfach nicht ausgeführt werden. Dann gibt es keinen Fehler. Ich bezweifle jedoch, dass der Programmierer die Zeile absichtlich nur für die Schönheit geschrieben hat.
Nicht initialisierte Variable im Konstruktor
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-Warnung : V3128 Das Feld 'Frames' wird verwendet, bevor es im Konstruktor initialisiert wird. CursorSequence.cs 35
Ein sehr unangenehmer Moment. Ein Versuch, den Wert der Length- Eigenschaft von einer nicht initialisierten Variablen abzurufen, führt zwangsläufig dazu, dass eine NullReferenceException ausgelöst wird . In einer normalen Situation würde ein solcher Fehler kaum unbemerkt bleiben - dennoch zeigt sich leicht die Unmöglichkeit, eine Instanz einer Klasse zu erstellen. Aber hier wird die Ausnahme nur ausgelöst, wenn die Bedingung erfüllt ist
(d.ContainsKey("Length") && d["Length"].Value == "*") ||
(d.ContainsKey("End") && d["End"].Value == "*")wird wahr sein.
Es ist schwer zu beurteilen, wie Sie den Code reparieren müssen, damit alles gut funktioniert. Ich kann nur davon ausgehen, dass die Funktion ungefähr so aussehen sollte:
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 dieser Version fehlt das angegebene Problem, jedoch kann nur der Entwickler sagen, inwieweit es der ursprünglichen Idee entspricht.
Möglicher Tippfehler
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-Warnung : V3127 Es wurden zwei ähnliche Codefragmente gefunden. Möglicherweise handelt es sich um einen Tippfehler, und anstelle von 'oldMapHeight' Map.cs 964 sollte die Variable 'oldMapRamp' verwendet werden. 964
Der Analysator hat einen verdächtigen Moment im Zusammenhang mit der Übergabe von Argumenten an Funktionen festgestellt. Schauen wir uns die Anrufe separat an:
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]);Seltsamerweise passiert der letzte Aufruf oldMapHeight , nicht oldMapRamp . Natürlich sind nicht alle derartigen Fälle wirklich Fehler. Es ist durchaus möglich, dass hier alles richtig geschrieben ist. Aber Sie müssen zugeben, dass dieser Ort ungewöhnlich aussieht. Ich neige dazu zu glauben, dass hier tatsächlich ein Fehler gemacht wurde.
Eine Notiz von einem Kollegen - Andrey Karpov . Und ich sehe nichts Seltsames in diesem Code. Dies ist der klassische Fehler in der letzten Zeile !
Wenn hier immer noch kein Fehler vorliegt, sollten Sie eine Erklärung hinzufügen. Wenn ein Moment wie ein Fehler aussieht, wird jemand ihn definitiv beheben wollen.
Richtig, wahr und nichts als wahr
Das Projekt enthält sehr eigenartige Methoden, deren Rückgabewert den Typ bool hat . Ihre Besonderheit liegt in der Tatsache, dass sie unter allen Umständen wahr zurückkehren . Zum Beispiel:
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-Warnung : V3009 Es ist seltsam, dass diese Methode immer ein und denselben Wert von 'true' zurückgibt. LobbyCommands.cs 123
Ist dieser Code in Ordnung? Gibt es einen Fehler? Es sieht sehr seltsam aus. Warum hat der Entwickler void nicht verwendet ?
Es ist nicht überraschend, dass der Analysator einen solchen Ort für seltsam hält, aber wir müssen dennoch zugeben, dass der Programmierer tatsächlich einen Grund hatte, so zu schreiben. Welcher?
Ich habe mich entschlossen zu sehen, wo diese Methode aufgerufen wird und ob ihr immer wahrer Rückgabewert verwendet wird. Es stellte sich heraus, dass es in derselben Klasse nur einen Verweis darauf gibt - im commandHandlers- Wörterbuch , das den Typ hat
IDictionary<string, Func<S, Connection, Session.Client, string, bool>>Während der Initialisierung werden Werte hinzugefügt
{"state", State},
{"startgame", StartGame},
{"slot", Slot},
{"allow_spectators", AllowSpectators}usw.
Wir werden mit einem seltenen (ich möchte es glauben) Fall konfrontiert, in dem statisches Tippen Probleme für uns verursacht. Schließlich ist es zumindest problematisch, ein Wörterbuch zu erstellen, in dem die Werte mit unterschiedlichen Signaturen funktionieren. commandHandlers wird nur in der InterpretCommand- Methode verwendet :
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);
}Anscheinend war das Ziel des Entwicklers die universelle Fähigkeit, Zeichenfolgen bestimmter ausgeführter Operationen abzugleichen. Ich denke, dass die Art und Weise, wie er sich entschieden hat, bei weitem nicht die einzige ist, aber es ist nicht so einfach, in einer solchen Situation etwas Bequemeres / Richtigeres anzubieten. Vor allem, wenn Sie keine Dynamik oder ähnliches verwenden. Wenn Sie Ideen zu diesem Thema haben, hinterlassen Sie Kommentare. Es wäre interessant für mich, verschiedene Möglichkeiten zur Lösung dieses Problems zu prüfen.
Es stellt sich heraus, dass die mit den immer wahren Methoden in dieser Klasse verbundenen Warnungen höchstwahrscheinlich falsch sind. Und doch ... Es ist dieses "wahrscheinlichste", das Ihnen Angst macht. Sie müssen mit solchen Dingen vorsichtig sein, weil es tatsächlich einen Fehler zwischen ihnen geben kann.
Alle diese positiven Ergebnisse sollten sorgfältig geprüft und gegebenenfalls als falsch markiert werden. Dies geschieht ganz einfach. Ein besonderer Kommentar sollte an der vom Analysator angegebenen Stelle hinterlassen werden:
static bool State(....) //-V3009Es gibt noch eine andere Möglichkeit: Sie können die Positiven auswählen, die als falsch markiert werden müssen, und im Kontextmenü auf "Ausgewählte Nachrichten als Fehlalarme markieren" klicken.
Weitere Informationen zu diesem Thema finden Sie in der Dokumentation .
Extra auf Null prüfen?
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-Warnung : V3022 Der Ausdruck 'LobbyInfo == null' ist immer falsch. LobbyCommands.cs 851
Eine andere Methode, die immer true zurückgibt . Dieses Mal untersuchen wir jedoch eine andere Art von Trigger. Sie müssen solche Dinge sorgfältig genug studieren, da dies weit davon entfernt ist, dass dies nur redundanter Code ist. Aber das Wichtigste zuerst.
Die Deserialize- Methode gibt niemals null zurück. Sie können dies leicht überprüfen, indem Sie sich den Code ansehen :
public static Session Deserialize(string data)
{
try
{
var session = new Session();
....
return session;
}
catch (YamlException)
{
throw new YamlException(....);
}
catch (InvalidOperationException)
{
throw new YamlException(....);
}
}Zur besseren Lesbarkeit habe ich den Quellcode der Methode gekürzt. Sie können es vollständig sehen, indem Sie dem Link folgen . Nun, oder nehmen Sie mein Wort dafür, dass die Sitzungsvariable hier unter keinen Umständen zu null wird .
Was sehen wir unten? Deserialize gibt nicht null zurück . Wenn etwas schief gelaufen ist, werden Ausnahmen ausgelöst . Der Entwickler, der nach dem Aufruf den Scheck für null schrieb, schien anders zu denken. In einer Ausnahmesituation sollte die SyncLobby- Methode höchstwahrscheinlich den Code ausführen, der gerade ausgeführt wird ... ja, er wird niemals ausgeführt, da LobbyInfo niemals null ist :
if (lobbyInfo == null)
{
server.SendOrderTo(conn, "Message", "Invalid Lobby Info Sent");
return true;
}Ich glaube, dass Sie anstelle dieses "zusätzlichen" Checks immer noch try - catch verwenden sollten . Nun, oder gehen Sie von der anderen Seite und schreiben Sie TryDeserialize , das im Ausnahmefall null zurückgibt .
Mögliche 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; // <=
}
....
}Analyzer-Warnung : V3125 Das ' Logo' -Objekt wurde verwendet, nachdem es gegen Null verifiziert wurde. Überprüfen Sie die Zeilen: 236, 222. ConnectionLogic.cs 236
Irgendetwas sagt mir, dass hier ein 100% iger Fehler vorliegt. Wir haben definitiv keine "zusätzlichen" Prüfungen vor uns, da die GetOrNull- Methode höchstwahrscheinlich wirklich eine Nullreferenz zurückgeben kann. Was passiert , wenn das Logo ist null ? Das Aufrufen der Bounds- Eigenschaft löst eine Ausnahme aus, die eindeutig nicht in den Plänen des Entwicklers enthalten war.
Vielleicht muss das Fragment so umgeschrieben werden:
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;
}
}Diese Option ist einfach zu verstehen, obwohl die zusätzliche Verschachtelung nicht sehr cool aussieht. Als umfangreichere Lösung können Sie den nullbedingten Operator verwenden:
// Add an equal logo margin on the right of the text
width += logo?.Bounds.Width ?? 0; // <=Beachten Sie, dass mir das Top Fix besser gefällt. Es ist angenehm zu lesen und es stellen sich keine Fragen. Aber einige Entwickler legen großen Wert auf Kürze, deshalb habe ich mich auch für die zweite Option entschieden :).
Vielleicht ist es doch 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)
{
....
}
....
}Analyzer-Warnung : V3063 Ein Teil des bedingten Ausdrucks ist immer wahr, wenn er ausgewertet wird: TerrainGeometryTrait! = Null. MapEditorLogic.cs 35
Lassen Sie uns dieses Snippet analysieren. Beachten Sie, dass jedes Mal, wenn die GetOrNull- Methode der Widget- Klasse verwendet wird , sie auf null überprüft wird . Wenn Get verwendet wird , erfolgt jedoch keine Validierung. Dies ist logisch - die Get- Methode gibt nicht null zurück :
public T Get<T>(string id) where T : Widget
{
var t = GetOrNull<T>(id);
if (t == null)
throw new InvalidOperationException(....);
return t;
}Wenn das Element nicht gefunden wird, wird eine Ausnahme ausgelöst - dies ist ein vernünftiges Verhalten. Gleichzeitig besteht die logische Option darin, die von der GetOrNull- Methode zurückgegebenen Werte auf Gleichheit mit einer Nullreferenz zu überprüfen .
Im obigen Code wird der von der Trait- Methode zurückgegebene Wert auf null überprüft . In der Tat, in der Trait - Methode, Get heißt von der TraitDictionary Klasse :
public T Trait<T>()
{
return World.TraitDict.Get<T>(this);
}Könnte es sein, dass sich dieses Get anders verhält als das zuvor beschriebene? Die Klassen sind jedoch unterschiedlich. Lass uns das Prüfen:
public T Get<T>(Actor actor)
{
CheckDestroyed(actor);
return InnerGet<T>().Get(actor);
}Die InnerGet- Methode gibt eine Instanz von TraitContainer <T> zurück . Die Implementierung von Get in this class ist Get in the Widget sehr ähnlich :
public T Get(Actor actor)
{
var result = GetOrDefault(actor);
if (result == null)
throw new InvalidOperationException(....);
return result;
}Die Hauptähnlichkeit besteht darin, dass auch hier niemals null zurückgegeben wird . Falls etwas schief gelaufen ist, wird in ähnlicher Weise eine InvalidOperationException ausgelöst . Daher verhält sich die Trait- Methode genauso.
Ja, hier gibt es möglicherweise nur eine zusätzliche Überprüfung, die nichts beeinflusst. Vielleicht sieht es seltsam aus, aber man kann nicht sagen, dass ein solcher Code den Leser sehr verwirren wird. Wenn die Prüfung hier jedoch nur benötigt wird, wird in einigen Fällen unerwartet eine Ausnahme ausgelöst. Es ist traurig.
An dieser Stelle erscheint es angemessener, TraitOrNull aufzurufen . Es gibt jedoch keine solche Methode :). Es gibt jedoch TraitOrDefault , das GetOrNull entsprichtfür diesen Fall.
Es gibt noch einen ähnlichen Punkt im Zusammenhang mit der Get- Methode :
public AssetBrowserLogic(....)
{
....
frameSlider = panel.Get<SliderWidget>("FRAME_SLIDER");
if (frameSlider != null)
{
....
}
....
}Analyzer-Warnung : V3022 Der Ausdruck 'frameSlider! = Null' ist immer wahr. AssetBrowserLogic.cs 128
Wie im obigen Code stimmt hier etwas nicht. Entweder ist die Prüfung wirklich unnötig, oder anstelle von Get müssen Sie noch GetOrNull aufrufen .
Verlorene Aufgabe
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-Warnung : V3008 Der Variablen 'widget.Bounds.Width' werden zweimal nacheinander Werte zugewiesen. Vielleicht ist das ein Fehler. Überprüfen Sie die Zeilen: 78, 75. SpawnSelectorTooltipLogic.cs 78
Wenn die Bedingung textWidth! = CachedWidth erfüllt ist , sollte ein fallspezifischer Wert in widget.Bounds.Width geschrieben werden . Die folgende Zuordnung beraubt die Zeichenfolge jedoch, unabhängig davon, ob diese Bedingung erfüllt ist
widget.Bounds.Width = 2 * label.Bounds.X + textWidth;jeder Sinn. Es ist wahrscheinlich, dass sie einfach vergessen haben, das andere hier zu platzieren :
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
);
}Überprüfen des Standardwerts
public void DisguiseAs(Actor target)
{
....
var tooltip = target.TraitsImplementing<ITooltip>().FirstOrDefault();
AsPlayer = tooltip.Owner;
AsActor = target.Info;
AsTooltipInfo = tooltip.TooltipInfo;
....
}Analyzer-Warnung : V3146 Mögliche Null-Dereferenzierung von 'Tooltip'. Der 'FirstOrDefault' kann den Standardwert Null zurückgeben. Disguise.cs 192
Wann wird FirstOrDefault häufig anstelle von First verwendet ? Wenn die Auswahl leer ist, löst First eine InvalidOperationException aus . FirstOrDefault löst keine Ausnahme aus, gibt jedoch für den Referenztyp null zurück .
Im Projekt wird die ITooltip- Schnittstelle von verschiedenen Klassen implementiert. Wenn also target.TraitsImplementing <IToolTip> () eine leere Auswahl zurückgibt, null geschrieben werden , um Tooltip... Weiterer Zugriff auf die Eigenschaften dieses Objekts führt zu einer NullReferenceException .
In Fällen, in denen der Entwickler sicher ist, dass die Auswahl nicht leer ist, ist es korrekter, First zu verwenden . Wenn Sie sich nicht so sicher sind, sollten Sie den von FirstOrDefault zurückgegebenen Wert überprüfen . Es ist ziemlich seltsam, dass dies nicht hier ist. Immerhin wurden die von der zuvor diskutierten GetOrNull- Methode zurückgegebenen Werte immer überprüft. Warum haben sie es hier nicht getan?
Wer weiß ... Oh, genau! Sicherlich kann ein Entwickler diese Fragen beantworten. Am Ende sollte er diesen Code bearbeiten.
Fazit
OpenRA erwies sich irgendwie als ein Projekt, das angenehm und interessant war. Die Entwickler haben großartige Arbeit geleistet und gleichzeitig nicht vergessen, dass die Quelle leicht zu erlernen sein sollte. Natürlich gibt es hier verschiedene ... kontroverse Punkte, aber wo ohne sie.
Gleichzeitig bleiben Entwickler (leider) trotz aller Bemühungen menschlich. Einige der in Betracht gezogenen positiven Ergebnisse sind ohne Verwendung des Analysegeräts äußerst schwer zu erkennen. Es ist manchmal schwierig, einen Fehler auch unmittelbar nach dem Schreiben zu finden. Was können wir über das Finden nach langer Zeit sagen?
Offensichtlich ist es viel besser, einen Fehler zu erkennen als seine Folgen. Dafür können Sie Stunden damit verbringen, eine große Anzahl neuer Quellen manuell zu überprüfen. Nun, die Alten sehen gleichzeitig aus - haben plötzlich keinen Fehler mehr bemerkt? Ja, Bewertungen sind wirklich nützlich, aber wenn Sie viel Code durchsehen müssen, bemerken Sie mit der Zeit einige Dinge nicht mehr. Und viel Zeit und Mühe wird dafür aufgewendet.
Die statische Analyse ist nur eine praktische Ergänzung zu anderen Methoden zur Überprüfung der Qualität des Quellcodes, z. B. zur Codeüberprüfung. PVS-Studio findet "einfache" (und manchmal nicht nur) Fehler anstelle des Entwicklers, sodass sich die Benutzer auf schwerwiegendere Probleme konzentrieren können.
Ja, der Analysator erzeugt manchmal falsch positive Ergebnisse und kann überhaupt nicht alle Fehler finden. Aber wenn Sie es verwenden, sparen Sie viel Zeit und Nerven. Ja, er ist nicht perfekt und manchmal macht er selbst Fehler. Im Allgemeinen macht PVS-Studio den Entwicklungsprozess jedoch viel einfacher, angenehmer und sogar (unerwartet!) Billiger.
Tatsächlich müssen Sie mein Wort nicht dafür nehmen - es wäre viel besser, selbst von der Wahrheit des oben Gesagten überzeugt zu sein. Folgen Sie dem Link , um den Analysator herunterzuladen und einen Testschlüssel zu erhalten. Wie viel einfacher?
Nun, das ist alles. Vielen Dank für Ihre Aufmerksamkeit! Ich wünsche Ihnen sauberen Code und das gleiche saubere Fehlerprotokoll!
Wenn Sie diesen Artikel einem englischsprachigen Publikum zugänglich machen möchten, verwenden Sie bitte den Übersetzungslink: Nikita Lipilin. Einhörner brechen in RTS ein: Analyse des OpenRA-Quellcodes .