هذه المقالة مخصصة للتحقق من مشروع OpenRA باستخدام محلل PVS-Studio الثابت. ما هو OpenRA؟ إنه محرك لعبة مفتوح المصدر لإنشاء ألعاب استراتيجية في الوقت الفعلي. تصف المقالة كيفية إجراء التحليل ، وما هي ميزات المشروع نفسها التي تم اكتشافها وما هي المحفزات المثيرة للاهتمام التي قدمها برنامج PVS-Studio. وبالطبع ، سننظر هنا في بعض ميزات المحلل التي جعلت عملية التحقق من المشروع أكثر راحة.
أوبنرا
المشروع الذي تم اختياره للمراجعة هو محرك لعبة RTS بأسلوب ألعاب مثل Command & Conquer: Red Alert. يمكن العثور على مزيد من المعلومات على الموقع . تمت كتابة الكود المصدري بلغة C # وهو متاح للعرض والاستخدام في المستودع .
تم اختيار OpenRA للمراجعة لثلاثة أسباب. أولاً ، يبدو أنه يثير اهتمام الكثير من الناس. على أي حال ، ينطبق هذا على سكان GitHub ، حيث جمع المستودع أكثر من 8 آلاف نجمة. ثانيًا ، يحتوي كود OpenRA على 1285 ملفًا. عادةً ما يكون هذا المبلغ كافيًا لإيجاد محفزات مثيرة للاهتمام فيها. وثالثاً ... محركات اللعبة رائعة.
ايجابيات اضافية
لقد قمت بتحليل OpenRA باستخدام PVS-Studio واستلهمت في البداية من النتائج:
قررت أنه من بين العديد من التحذيرات الكبيرة ، يمكنني بالتأكيد العثور على مجموعة كبيرة من الردود المختلفة. وبالطبع ، سأكتب على أساسها أروع مقالة وأكثرها إثارة للاهتمام. لكنها لم تكن هناك!
كان على المرء فقط أن ينظر إلى التحذيرات بأنفسهم ، وسرعان ما سقط كل شيء في مكانه. من بين التحذيرات الشديدة 1306 ، كان هناك 1277 مرتبطًا بالتشخيص V3144 . يعرض رسائل مثل "تم تمييز هذا الملف بترخيص حقوق متروكة ، والذي يتطلب منك فتح رمز المصدر المشتق". يتم وصف هذه التشخيصات بمزيد من التفاصيل هنا .
من الواضح أن طريقة عمل هذه الخطة لم تهمني على الإطلاق ، لأن 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 "نشط" يكون دائمًا صحيحًا. SupportPowerManager.cs 206
PVS-Studio يلاحظ بحق أن الفحص الثاني لا معنى له ، لأنه إذا كان Active غير صحيح ، فلن يصل التنفيذ إليه. قد يكون خطأ ، لكنني أعتقد أنه كتب عن قصد. لاجل ماذا؟ حسنا لما لا؟
ربما أمامنا نوع من الحل المؤقت ، ومراجعته متروكة "لوقت لاحق". في مثل هذه الحالات ، من المريح جدًا أن يقوم المحلل بتذكير المطور بمثل هذه العيوب.
دعنا نلقي نظرة على فحص آخر "فقط في حالة":
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 التعبير "تمييز البدء > 0" يكون دائمًا صحيحًا. 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 جزء من التعبير الشرطي يكون دائمًا صحيحًا إذا تم تقييمه : إلغاء زر ! = لاغ. ConfirmationDialogs.cs 78
cancellButton يمكن بالفعل أن يكون فارغًا ، لأن القيمة التي تم إرجاعها بواسطة أسلوب GetOrNull مكتوبة إلى هذا المتغير . ومع ذلك ، فمن المنطقي أن نفترض أن الأمر cancellButton في جسم العامل الشرطي لن يصبح فارغًا بأي حال من الأحوال . ومع ذلك ، لا يزال هناك شيك. إذا لم تنتبه للحالة الخارجية ، فسيظهر موقف غريب بشكل عام: أولاً ، يتم الوصول إلى خصائص المتغير ، ثم قرر المطور التأكد - هل ما زال فارغًا أم لا؟
في البداية ، افترضت أن المشروع قد يستخدم بعض المنطق المحدد المتعلق بزيادة التحميل على عامل التشغيل "==". في رأيي ، تنفيذ شيء مشابه لأنواع المراجع في مشروع هو فكرة مثيرة للجدل. ومع ذلك ، فإن السلوك غير المعتاد يجعل من الصعب على المطورين الآخرين فهم الكود. في الوقت نفسه ، يصعب علي تخيل موقف لا يمكن فيه الاستغناء عن مثل هذه الحيل. على الرغم من أنه من المحتمل أنه في بعض الحالات المحددة ، سيكون هذا حلاً مناسبًا.
في محرك اللعبة Unity ، على سبيل المثال ، تم تجاوز عامل التشغيل " == " لفئة UnityEngine.Object . توضح الوثائق الرسمية المتوفرة على الرابط أن مقارنة حالات هذه الفئة مع قيمة فارغةلا يعمل كالمعتاد. حسنًا ، بالتأكيد كان لدى المطورين أسباب لتنفيذ مثل هذا المنطق غير العادي.
لم أجد شيئًا كهذا في OpenRA :). لذلك ، إذا كان هناك أي معنى في الشيكات التي تم اعتبارها سابقًا للخسارة ، فإنها تتكون من شيء آخر.
تمكن برنامج 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' دائمًا ما يكون خطأ. TransformsIntoTransforms.cs 44
مرة أخرى لدينا فحص لا طائل من ورائه. صحيح ، على عكس السابق ، هنا ليس فقط شرطًا إضافيًا ، ولكن أكثر الكود الحقيقي الذي يتعذر الوصول إليه. إن الشيكات التي كانت تعتبر في السابق صحيحة دائمًا لم تؤثر في الواقع على تشغيل البرنامج. يمكن إزالتها من الكود ، أو يمكن تركها - لن يتغير شيء.
هنا ، يؤدي فحص غريب إلى حقيقة أن جزءًا من الكود لم يتم تنفيذه. في الوقت نفسه ، يصعب علي تخمين التغييرات التي ينبغي إجراؤها هنا كتعديل. في أبسط الحالات وأكثرها متعة ، لا ينبغي ببساطة تنفيذ الكود الذي لا يمكن الوصول إليه. ثم ليس هناك خطأ. ومع ذلك ، أشك في أن المبرمج كتب عن عمد الخط من أجل الجمال فقط.
متغير غير مهيأ في المُنشئ
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 . بالطبع ، ليست كل هذه الحالات أخطاء في الحقيقة. من الممكن أن يكون كل شيء مكتوبًا بشكل صحيح هنا. لكن يجب أن تعترف أن هذا المكان يبدو غير عادي. إنني أميل إلى الاعتقاد بأنه قد تم بالفعل ارتكاب خطأ هنا.
ملاحظة من زميل - أندريه كاربوف . ولا أرى شيئًا غريبًا في هذا الكود. هذا هو خطأ السطر الأخير الكلاسيكي !
إذا لم يكن هناك خطأ حتى الآن ، فإن الأمر يستحق إضافة بعض الشرح. بعد كل شيء ، إذا بدت لحظة ما وكأنها خطأ ، فسيرغب شخص ما بالتأكيد في إصلاحها.
صحيح وصحيح ولا شيء غير صحيح
يحتوي المشروع على طرق غريبة للغاية ، حيث تحتوي قيمة الإرجاع على نوع منطقي . تكمن خصوصيتها في حقيقة أنها تحت أي ظرف من الظروف تعود صحيحة . على سبيل المثال:
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 من الغريب أن تقوم هذه الطريقة دائمًا بإرجاع قيمة واحدة ونفس قيمة "صواب". LobbyCommands.cs 123
هل هذا الرمز مناسب ؟ هل هناك خطأ؟ يبدو غريبا جدا. لماذا لم يستخدم المطور باطل ؟
ليس من المستغرب أن يعتبر المحلل مثل هذا المكان غريبًا ، لكن لا يزال يتعين علينا الاعتراف بأن المبرمج لديه بالفعل سبب للكتابة بهذه الطريقة. أي واحدة؟
قررت معرفة مكان استدعاء هذه الطريقة وما إذا كان يتم استخدام قيمة الإرجاع الحقيقية دائمًا . اتضح أنه لا يوجد سوى مرجع واحد له في نفس الفئة - في قاموس 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);
}على ما يبدو ، كان هدف المطور هو القدرة العالمية على مطابقة سلاسل عمليات معينة يتم إجراؤها. أعتقد أن الطريقة التي اختارها ليست الطريقة الوحيدة ، ولكن ليس من السهل تقديم شيء أكثر ملاءمة / صحة في مثل هذه الحالة. خاصة إذا كنت لا تستخدم نوعًا من الديناميكية أو شيء مشابه. إذا كان لديك أي أفكار حول هذا الموضوع ، اترك تعليقات. سيكون من المثير للاهتمام بالنسبة لي أن ألقي نظرة على الخيارات المختلفة لحل هذه المشكلة.
اتضح أن التحذيرات المرتبطة بالطرق الصحيحة دائمًا في هذه الفئة خاطئة على الأرجح. ومع ذلك ... هذا "على الأرجح" هو ما يخيفك. يجب أن تكون حذراً في مثل هذه الأشياء ، لأنه قد يكون هناك بالفعل خطأ بينهم.
يجب فحص كل هذه الإيجابيات بعناية ثم تمييزها على أنها خاطئة إذا لزم الأمر. يتم ذلك بكل بساطة. يجب ترك تعليق خاص في المكان الذي يشير إليه المحلل:
static bool State(....) //-V3009هناك طريقة أخرى: يمكنك تحديد الإيجابيات التي تحتاج إلى وضع علامة كاذبة عليها ، وفي قائمة السياق ، انقر فوق "وضع علامة على الرسائل المحددة كإنذارات كاذبة".
يمكنك معرفة المزيد حول هذا الموضوع في الوثائق .
الشيك الإضافي لصفر؟
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 == فارغ 'هو دائما كاذبة. LobbyCommands.cs 851
طريقة أخرى تُرجع دائمًا صحيحًا . ومع ذلك ، نحن هذه المرة نفحص نوعًا مختلفًا من المشغلات. أنت بحاجة إلى دراسة مثل هذه الأشياء بعناية كافية ، لأنها بعيدة كل البعد عن حقيقة أن هذا مجرد رمز زائد عن الحاجة. لكن أول الأشياء أولاً. لا تُرجع
طريقة Deserialize قيمة خالية أبدًا - يمكنك التحقق من ذلك بسهولة من خلال النظر إلى الكود الخاص بها:
public static Session Deserialize(string data)
{
try
{
var session = new Session();
....
return session;
}
catch (YamlException)
{
throw new YamlException(....);
}
catch (InvalidOperationException)
{
throw new YamlException(....);
}
}لسهولة القراءة ، قمت باختصار الكود المصدري للطريقة. يمكنك رؤيته بالكامل باتباع الرابط . حسنًا ، أو اعتبر أن متغير الجلسة هنا لا يتحول تحت أي ظرف من الظروف إلى قيمة خالية .
ماذا نرى في الأسفل؟ لا يعود إلغاء التسلسل فارغًا ، إذا حدث خطأ ما ، فإنه يطرح استثناءات. يبدو أن المطور الذي كتب الشيك لـ null بعد المكالمة يفكر بشكل مختلف. على الأرجح ، في حالة استثنائية ، يجب أن تقوم طريقة SyncLobby بتنفيذ الكود الذي يتم تنفيذه حاليًا ... نعم ، لا يتم تنفيذه أبدًا ، لأن loopInfo ليست فارغة أبدًا :
if (lobbyInfo == null)
{
server.SendOrderTo(conn, "Message", "Invalid Lobby Info Sent");
return true;
}أعتقد أنه بدلاً من هذا الاختيار "الإضافي" ، لا يزال يتعين عليك استخدام try - catch . حسنًا ، أو انتقل من الجانب الآخر واكتب بعض TryDeserialize ، والذي في حالة الاستثناء سيعود فارغًا .
ممكن 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 بعد التحقق من عدم وجوده. تحقق من الأسطر: 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 يكون جزء من التعبير الشرطي صحيحًا دائمًا إذا تم تقييمه: terrainGeometryTrait! = Null. 35 MapEditorLogic.cs
دعونا نحلل هذا المقتطف. لاحظ أنه في كل مرة يتم فيها استخدام أسلوب GetOrNull لفئة عنصر واجهة المستخدم ، يتم فحصه بحثًا عن قيمة خالية . ومع ذلك ، إذا تم استخدام Get ، فلا يوجد تحقق من الصحة. هذا منطقي - طريقة Get لا تعيد قيمة فارغة :
public T Get<T>(string id) where T : Widget
{
var t = GetOrNull<T>(id);
if (t == null)
throw new InvalidOperationException(....);
return t;
}إذا لم يتم العثور على العنصر ، فسيتم طرح استثناء - وهذا سلوك معقول. وفي الوقت نفسه ، سيكون الخيار المنطقي هو التحقق من القيم التي تم إرجاعها بواسطة أسلوب GetOrNull للمساواة مع مرجع فارغ.
في الكود أعلاه ، تم التحقق من القيمة التي تُرجعها طريقة السمات بحثًا عن قيمة خالية . في الواقع ، داخل طريقة Trait ، يتم استدعاء Get من فئة TraitDictionary :
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> . تطبيق Get in هذا الفصل مشابه جدًا لتطبيق Get of the Widget :
public T Get(Actor actor)
{
var result = GetOrDefault(actor);
if (result == null)
throw new InvalidOperationException(....);
return result;
}التشابه الرئيسي هو أنه لا يتم إرجاع القيمة الفارغة هنا أيضًا . في حالة حدوث خطأ ما ، يتم إلقاء InvalidOperationException بالمثل . لذلك ، تتصرف طريقة السمات بنفس الطريقة.
نعم ، قد يكون هناك فقط شيك إضافي هنا ، والذي لا يؤثر على أي شيء. ربما يبدو الأمر غريبًا ، لكن لا يمكننا القول إن مثل هذه الشفرة ستربك القارئ كثيرًا. ولكن إذا كان الشيك مطلوبًا هنا فقط ، فسيتم طرح استثناء في بعض الحالات بشكل غير متوقع. أنه لأمر محزن.
في هذه المرحلة ، يبدو أنه من الأنسب استدعاء نوع من TraitOrNull . ومع ذلك ، لا توجد مثل هذه الطريقة :). ولكن هناك TraitOrDefault ، وهو مشابه لـ GetOrNullلهذه الحالة.
هناك نقطة أخرى مشابهة تتعلق بطريقة Get :
public AssetBrowserLogic(....)
{
....
frameSlider = panel.Get<SliderWidget>("FRAME_SLIDER");
if (frameSlider != null)
{
....
}
....
}تحذير المحلل : التعبير V3022 'frameSlider! = Null' يكون دائمًا صحيحًا. 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 صحيحًا ، يجب كتابة بعض القيمة المحددة في 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 احتمال وجود مرجع فارغ لـ "تلميح الأدوات". يمكن لـ "FirstOrDefault" إرجاع قيمة فارغة افتراضية. Disguise.cs 192
متى يتم استخدام FirstOrDefault بشكل شائع بدلاً من First ؟ إذا كان التحديد فارغًا ، فسيقوم First بإلقاء InvalidOperationException . لن يقوم FirstOrDefault بطرح استثناء ، ولكنه سيعود فارغًا لنوع المرجع.
في المشروع ، يتم تنفيذ واجهة ITooltip بواسطة فئات مختلفة. وبالتالي ، إذا كانت target.TraitsImplementing <ITooltip> () تُرجع تحديدًا فارغًا ، فستتم كتابة القيمة null في تلميح الأدوات... سيؤدي الوصول الإضافي إلى خصائص هذا الكائن إلى NullReferenceException .
في الحالات التي يكون فيها المطور متأكدًا من أن التحديد لن يكون فارغًا ، فمن الأصح استخدام أولاً . إذا لم تكن متأكدًا ، فمن المفيد التحقق من القيمة التي تم إرجاعها بواسطة FirstOrDefault. الغريب أن هذا ليس هنا. بعد كل شيء ، تم دائمًا التحقق من القيم التي تم إرجاعها بواسطة طريقة GetOrNull التي تمت مناقشتها مسبقًا . لماذا لم يفعلوا ذلك هنا؟
من يدري ... أوه ، بالضبط! بالتأكيد يمكن للمطور الإجابة على هذه الأسئلة. في النهاية ، يجب عليه تعديل هذا الرمز.
خاتمة
تحول OpenRA بطريقة ما إلى مشروع كان ممتعًا وممتعًا للتحقق منه. لقد قام المطورون بعمل رائع وفي نفس الوقت لم ينسوا أن المصدر يجب أن يكون سهل التعلم. طبعا هنا توجد نقاط مختلفة .. مثيرة للجدل ولكن اين بدونها.
في الوقت نفسه ، حتى مع كل جهودهم ، يظل المطورون (للأسف) بشرًا. من الصعب للغاية ملاحظة بعض الإيجابيات المدروسة دون استخدام المحلل. يصعب أحيانًا العثور على خطأ حتى بعد كتابته مباشرة. ماذا نقول عن العثور عليهم بعد وقت طويل.
من الواضح أن اكتشاف الخطأ أفضل بكثير من اكتشاف عواقبه. لهذا يمكنك قضاء ساعات في إعادة التحقق يدويًا من عدد كبير من المصادر الجديدة. حسنًا ، ينظر القديمون في نفس الوقت - فجأة لم يلاحظوا أي خطأ سابقًا؟ نعم ، المراجعات مفيدة حقًا ، ولكن إذا كان عليك النظر في الكثير من التعليمات البرمجية ، فعندئذٍ ستتوقف بمرور الوقت عن ملاحظة بعض الأشياء. ويتم إنفاق الكثير من الوقت والجهد على ذلك.
التحليل الثابت هو مجرد إضافة ملائمة للطرق الأخرى للتحقق من جودة الكود المصدري ، مثل مراجعة الكود. سيجد PVS-Studio أخطاء "بسيطة" (وأحيانًا ليس فقط) بدلاً من المطور ، مما يسمح للأشخاص بالتركيز على مشكلات أكثر خطورة.
نعم ، يُنتج المحلل أحيانًا إيجابيات خاطئة ولا يمكنه العثور على جميع الأخطاء على الإطلاق. لكن استخدامه سيوفر عليك الكثير من الوقت والأعصاب. نعم ، إنه ليس مثاليًا وأحيانًا يخطئ بنفسه. ومع ذلك ، بشكل عام ، يجعل PVS-Studio عملية التطوير أسهل بكثير وأكثر متعة وحتى (بشكل غير متوقع!) أرخص.
في الواقع ، لست بحاجة إلى أن تأخذ كلامي على محمل الجد - سيكون من الأفضل بكثير التحقق من حقيقة ما ورد أعلاه بنفسك. اتبع الرابط لتنزيل المحلل والحصول على مفتاح تجريبي. كم أسهل؟
حسنا هذا كل شيء. شكرآ لك على أهتمامك! أتمنى لك كود نظيف ونفس سجل الأخطاء النظيف!
إذا كنت ترغب في مشاركة هذه المقالة مع جمهور يتحدث الإنجليزية ، فيرجى استخدام رابط الترجمة: Nikita Lipilin. اقتحام Unicorns RTS: تحليل كود مصدر OpenRA .