Проверяем исходный код набора C#/.NET компонентов от Sony
Как некоторые из вас помнят — недавно мы выпустили версию анализатора, поддерживающую проверку C#-кода. С появлением возможности анализа проверки проектов, написанных на C#, открывается новый простор для творчества. Об анализе одного из таких проектов, разработанного компанией Sony Computer Entertainment (SCEI), и пойдёт речь в данной статье.
Что проверяли?
Sony Computer Entertainment — видеоигровая компания, одно из подразделений корпорации Sony, специализирующееся на видеоиграх и игровых приставках. Данная компания разрабатывает видеоигры, аппаратное и программное обеспечение для консолей PlayStation.
The Authoring Tools Framework (ATF) — набор C#/.NET компонентов для разработки инструментов под Windows. ATF используется большинством передовых студий из состава SCEI, занимающихся разработкой видеоигр для изготовления кастомизированных инструментов. Данным набором компонентов пользуются такие студии как Naughty Dog, Guerrilla Games, Quantic Dream. В частности, инструменты, разработанные с использованием данных программных компонентов, применялись при создании таких известных игр как The Last of Us и Killzone. ATF является проектом с открытым исходным кодом, который доступен в репозитории на GitHub.
Инструмент анализа
Для анализа исходного кода использовался статический анализатор PVS-Studio. Этот инструмент может проверять исходный код, написанный на языках C/C++/C#. Каждое диагностическое сообщение подробно описано в документации, где приводятся примеры некорректного кода и способы его исправления. Многие описания диагностик имеют ссылку на соответствующий раздел базы ошибок, где содержится информация о том, какие ошибки в реальных проектах удалось обнаружить с помощью тех или иных диагностик.
Загрузить и попробовать анализатор на своём (или чужом) проекте можно по ссылке.
Примеры ошибок
public static void DefaultGiveFeedback(IComDataObject data,
GiveFeedbackEventArgs e)
{
....
if (setDefaultDropDesc && (DropImageType)e.Effect != currentType)
{
if (e.Effect != DragDropEffects.None)
{
SetDropDescription(data,
(DropImageType)e.Effect, e.Effect.ToString(), null);
}
else
{
SetDropDescription(data,
(DropImageType)e.Effect, e.Effect.ToString(), null);
}
....
}
}
Предупреждение анализатора: V3004 The 'then' statement is equivalent to the 'else' statement. Atf.Gui.WinForms.vs2010 DropDescriptionHelper.cs 199
Как видно из кода, вне зависимости от истинности условия 'e.Effect!= DragDropEffects.None' будет вызван один и тот же метод с одинаковыми аргументами. Не являясь программистом, писавшим данный код, порой достаточно сложно сказать, как правильно исправить это место, но, думаю, ни у кого не возникло сомнений, что поправить что-то нужно. Что именно — решать уже автору кода.
Рассмотрим следующим фрагмент кода:
public ProgressCompleteEventArgs(Exception progressError,
object progressResult,
bool cancelled)
{
ProgressError = ProgressError;
ProgressResult = progressResult;
Cancelled = cancelled;
}
Предупреждение анализатора: V3005 The 'ProgressError' variable is assigned to itself. Atf.Gui.Wpf.vs2010 StatusService.cs 24
Подразумевалось, что при вызове метода свойствам будут присвоены значения, переданные в качестве аргументов, про этом названия свойств и параметров различаются только регистром первой буквы. Как результат — свойство 'ProgressError' записывается само в себя вместо того, чтобы получить значение параметра 'progressError'.
Что интересно — это далеко не единичный случай путаницы между заглавными и прописными буквами. В некоторых проверенных проектах встречались ошибки точно такого же типа. Есть подозрение, что скоро мы выявим новый типовой паттерн ошибки, свойственный программам, написанным на C#. Прослеживается тенденция инициализировать свойства в каком-либо методе, названия параметров которого отличаются от названия инициализируемых свойств только регистром первой буквы. Как результат — появляются ошибки, подобные этой. В следующем примере кода если и не является ошибочным, то выглядит как минимум странно:
public double Left { get; set; }
public double Top { get; set; }
public void ApplyLayout(XmlReader reader)
{
....
FloatingWindow window = new FloatingWindow(
this, reader.ReadSubtree());
....
window.Left = window.Left;
window.Top = window.Top;
....
}
Предупреждения анализатора:
- V3005 The 'window.Left' variable is assigned to itself. Atf.Gui.Wpf.vs2010 DockPanel.cs 706
- V3005 The 'window.Top' variable is assigned to itself. Atf.Gui.Wpf.vs2010 DockPanel.cs 707
Из кода и предупреждений анализатора видно, что свойства 'Left' и 'Top' объекта 'window' записываются сами в себя. Для некоторых случаев такой вариант действий приемлем, например, когда на метод доступа свойства 'повешена' специальная логика. Однако для этих свойств никакой дополнительной логики нет, поэтому причины написания такого кода остаются неясными.
Следующий пример:
private static void OnBoundPasswordChanged(DependencyObject d,
DependencyPropertyChangedEventArgs e)
{
PasswordBox box = d as PasswordBox;
if (d == null || !GetBindPassword(d))
{
return;
}
// avoid recursive updating by ignoring the box's changed event
box.PasswordChanged -= HandlePasswordChanged;
....
}
Предупреждение анализатора: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'd', 'box'. Atf.Gui.Wpf.vs2010 PasswordBoxBehavior.cs 38
Этот тип ошибок также неоднократно встречался в проверенных C#-проектах. В результате приведения объекта к совместимому типу с использованием оператора 'as' получают новый объект, однако дальше в коде проверяют на равенство 'null' исходный объект. Данный код может вполне корректно работать, если точно известно, что объект 'd' будет всегда совместим с типом 'PasswordBox'. Но если это не так (уже сейчас, или в дальнейшем что-то изменится в программе), то вы запросто получите 'NullReferenceException' на коде, который раньше работал нормально. Так что в любом случае данный код стоит исправить.
В следующем примере программист наоборот попытался обезопасить себя всеми доступными способами, только немного непонятно, зачем:
public Rect Extent
{
get { return _extent; }
set
{
if (value.Top < -1.7976931348623157E+308 ||
value.Top > 1.7976931348623157E+308 ||
value.Left < -1.7976931348623157E+308 ||
value.Left > 1.7976931348623157E+308 ||
value.Width > 1.7976931348623157E+308 ||
value.Height > 1.7976931348623157E+308)
{
throw new ArgumentOutOfRangeException("value");
}
_extent = value;
ReIndex();
}
}
Предупреждение анализатора: V3022 Expression is always false. Atf.Gui.Wpf.vs2010 PriorityQuadTree.cs 575
Данное условие всегда будет ложным. Если непонятно — давайте разбираться почему.
Это реализация свойства, имеющего типа 'Rect', следовательно, 'value' также имеет тип 'Rect'. 'Top', 'Left', 'Width', 'Height' — свойства этого типа, имеющие тип 'double'. В данном коде проверяется, не выходят ли значения этих свойств за диапазон значений, которые может принимать тип 'double'. Причём для сравнения используются 'магические числа', а не константы, определённые в типе 'double'. Так как значения типа 'double' всегда находятся в данном данных диапазонах, это условие всегда будет ложным.
Видимо программист хотел защитить свою программу от нестандартной реализации типа 'double' в компиляторе. Тем не менее, это выглядит странно, так что анализатор вполне резонно выдал предупреждение, предлагая программисту перепроверить код.
Идём дальше:
public DispatcherOperationStatus Status { get; }
public enum DispatcherOperationStatus
{
Pending,
Aborted,
Completed,
Executing
}
public object EndInvoke(IAsyncResult result)
{
DispatcherAsyncResultAdapter res =
result as DispatcherAsyncResultAdapter;
if (res == null)
throw new InvalidCastException();
while (res.Operation.Status != DispatcherOperationStatus.Completed
|| res.Operation.Status == DispatcherOperationStatus.Aborted)
{
Thread.Sleep(50);
}
return res.Operation.Result;
}
Предупреждение анализатора: V3023 Consider inspecting this expression. The expression is excessive or contains a misprint. Atf.Gui.Wpf.vs2010 SynchronizeInvoke.cs 74
Условие выполнения цикла 'while' является избыточным, его можно было бы упростить, убрав второе подвыражение. Тогда цикл можно было бы упростить до следующего вида:
while (res.Operation.Status != DispatcherOperationStatus.Completed)
Thread.Sleep(50);
Следующий интересный пример:
private Vec3F ProjectToArcball(Point point)
{
float x = (float)point.X / (m_width / 2); // Scale so bounds map
// to [0,0] - [2,2]
float y = (float)point.Y / (m_height / 2);
x = x - 1; // Translate 0,0 to the center
y = 1 - y; // Flip so +Y is up
if (x < -1)
x = -1;
else if (x > 1)
x = 1;
if (y < -1)
y = -1;
else if (y > 1)
y = 1;
....
}
Предупреждения анализатора:
- V3041 The expression was implicitly cast from 'int' type to 'float' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. Atf.Gui.OpenGL.vs2010 ArcBallCameraController.cs 216
- V3041 The expression was implicitly cast from 'int' type to 'float' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. Atf.Gui.OpenGL.vs2010 ArcBallCameraController.cs 217
Это опять один из тех случаев, когда стороннему разработчику очень сложно сказать наверняка, содержит код ошибку или нет. С одной стороны — выполнение целочисленного деления с последующим неявным приведением к вещественному типу выглядит странно. С другой — иногда так делают умышленно, пренебрегая потерянной точностью.
Что подразумевалось здесь — сказать сложно. Возможно, точность вообще не хотели терять, но в результате операции 'm_width / 2' эта потеря всё же произойдёт. Тогда код нужно было бы исправить на следующий:
float x = point.X / ((float)m_width / 2);
С другой стороны — возможно хотели записать в 'x' целое число, так как дальше производятся операции сравнения с целочисленными значениями. В таком случае не нужно было выполнять явное приведение кода к типу 'float':
float x = point.X / (m_width / 2);
Наш анализатор растёт и развивается, соответственно, пополняется новыми интересными диагностиками. Следующая ошибка как раз и была найдена одной из таких новых диагностик. Так как эта диагностика на момент написания статьи ещё не включена в релиз, то ссылку на документацию предоставить не могу. Надеюсь, тут и так всё понятно:
public static QuatF Slerp(QuatF q1, QuatF q2, float t)
{
double dot = q2.X * q1.X + q2.Y * q1.Y + q2.Z * q1.Z + q2.W * q1.W;
if (dot < 0)
q1.X = -q1.X; q1.Y = -q1.Y; q1.Z = -q1.Z; q1.W = -q1.W;
....
}
Предупреждение анализатора: V3043 The code’s operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. Atf.Core.vs2010 QuatF.cs 282
Из кода видно, что вычисляется сумма нескольких произведений и результат этой операции записывается в переменную 'dot', после чего, если значение 'dot' отрицательно, выполняется инверсия всех участвующих в операции значений. Точнее сказать, подразумевалась инверсия, судя по форматированию кода. На самом же деле в этом случае будет инвертировано только свойство 'X' объекта 'q1', а все остальные свойства будут инвертированы вне зависимости от значения переменной 'dot'. Решение проблемы — фигурные скобки:
if (dot < 0)
{
q1.X = -q1.X;
q1.Y = -q1.Y;
q1.Z = -q1.Z;
q1.W = -q1.W;
}
Идём дальше:
public float X;
public float Y;
public float Z;
public void Set(Matrix4F m)
{
....
ww = -0.5 * (m.M22 + m.M33);
if (ww >= 0)
{
if (ww >= EPS2)
{
double wwSqrt = Math.Sqrt(ww);
X = (float)wwSqrt;
ww = 0.5 / wwSqrt;
Y = (float)(m.M21 * ww);
Z = (float)(m.M31 * ww);
return;
}
}
else
{
X = 0;
Y = 0;
Z = 1;
return;
}
X = 0;
ww = 0.5 * (1.0f - m.M33);
if (ww >= EPS2)
{
double wwSqrt = Math.Sqrt(ww);
Y = (float)wwSqrt; //<=
Z = (float)(m.M32 / (2.0 * wwSqrt)); //<=
}
Y = 0; //<=
Z = 1; //<=
}
Предупреждения анализатора:
- V3008 The 'Y' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 221, 217. Atf.Core.vs2010 QuatF.cs 221
- V3008 The 'Z' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 222, 218. Atf.Core.vs2010 QuatF.cs 222
Я специально привёл дополнительный фрагмент кода, чтобы ошибка стала более наглядной. 'Y' и 'Z' — экземплярные поля (instance fields). В зависимости от некоторых условий в эти поля записываются те или иные значения, после чего выполнение метода прекращается. Но в теле последнего оператора 'if' забыли написать оператор 'return', из-за чего полям будут присвоены не те значения, которые подразумевались. Тогда корректный код мог бы выглядеть так:
X = 0;
ww = 0.5 * (1.0f - m.M33);
if (ww >= EPS2)
{
double wwSqrt = Math.Sqrt(ww);
Y = (float)wwSqrt;
Z = (float)(m.M32 / (2.0 * wwSqrt));
return;
}
Y = 0;
Z = 1;
На этом, пожалуй, остановимся. Эти места показались наиболее интересными, поэтому я решил выписать их и разобрать. Были найдены и другие ошибки, в частности — я вообще не рассматривал предупреждения с низким уровнем важности, из предупреждений среднего и высокого уровней выписал только несколько.
Заключение
Как видно, от ошибок не застрахован никто, и по невнимательности легко присвоить объект сам себе или пропустить какой-нибудь оператор. В большом объёме кода такие ошибки зачастую бывает сложно обнаружить визуально, к тому же не все из них проявляют себя сразу — некоторые из них прострелят вам ногу пару лет спустя. Чтобы этого не допустить, и необходим статический анализатор, позволяющий выявлять ошибки на раннем этапе, снижать стоимость разработки проекта, беречь ваши нервы и сохранять ноги в целости.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Sergey Vasiliev. Sony C#/.NET component set analysis.
