Microsoft відкрила вихідні коди Xamarin.Forms. Ми не могли упустити шанс перевірити їх за допомогою PVS-Studio


Не так давно, як ви напевно знаєте, корпорація Microsoft придбала компанію Xamarin. Навіть незважаючи на те, що останнім часом Microsoft почала поступово відкривати вихідні коди своїх продуктів, відкриття коду Xamarin.Forms стало великим сюрпризом. Я не зміг пройти повз такої події, і вирішив перевірити вихідний код цього проекту з допомогою статичного аналізатора коду.


Аналізований проект
Xamarin.Forms — багатоплатформовий набір інструментів, що дозволяє створювати користувальницькі інтерфейси, загальні для різних платформ: Windows, Windows Phone, iOS, Android. Інтерфейси відмальовує, використовуючи нативні компоненти кінцевої платформи, що дозволяє зберігати додатків, створених за допомогою Xamarin.Forms, загальний вигляд для кожної платформи. Для створення користувацьких інтерфейсів з прив'язками даних і різними стилями ви можете використовувати C# або XAML-розмітку.



Код самого фреймворку також написаний на мові C# і доступний в репозиторії на GitHub.

Інструмент аналізу
Проект перевірявся за допомогою статичного аналізатора коду PVS-Studio, в розробці якого я беру активну участь. Ми постійно працюємо над його покращенням, в тому числі допрацьовуючи існуючі і додаючи нові діагностичні правила. Тому з кожною перевіркою нового проекту нам вдається виявляти все більше різновидів помилок.



Кожне діагностичне правило містить документацію, що включає в себе опис помилки, а також приклади невірного і вірного коду. Завантажити пробну версію аналізатора можна посилання. Також пропоную познайомитися з заміткою, яку нещодавно написав мій колега. Вона пояснює, чому обрані саме такі обмеження демонстраційної версії і що потрібно зробити, щоб спробувати весь функціонал. Для тих, хто полінувався читати, відразу підкажу — досить надіслати.

p.s. Крім того на сайті є база помилок, знайдених в open source проектах, і каталог статей (про перевірку open source проектів, технічної спрямованості, та інших). Рекомендую ознайомитися.

Підозрілі фрагменти коду
Почнемо з «класичних» помилок, що виявляються діагностичним правилом V3001:
const int RwWait = 1;
const int RwWrite = 2;
const int RwRead = 4;
....

public void EnterReadLock()
{
....

if ((Interlocked.Add(ref _rwlock, RwRead) & 
(RwWait | RwWait)) == 0)
return;

....
}

Попередження PVS-Studio: V3001 There are identical sub-expressions 'RwWait' to the left and to the right of the '|' operator. SplitOrderedList.cs 458

Як видно з коду, обчислюється значення якогось виразу з використанням бітових операцій. При цьому в одному з подвираженій RwWait | RwWait беруть участь однакові константные поля. Це не має сенсу. При цьому видно, що набір констант, оголошений вище, має значення, рівні ступенями двійки, отже, малося на увазі їх використання як прапорів (що ми і бачимо на прикладі використання бітових операцій). Думаю, варто було б винести їх перерахування, зазначене атрибутом [Flags], що дало б ряд переваг при роботі з цим переліком (див. документацію V3059).

Що до поточного приклад — швидше за все, малося на увазі використання константи RwWrite. Це можна віднести до одного з мінусів IntelliSense — незважаючи на те, що цей засіб дуже допомагає в написанні коду, часом воно може «підказати» не ту змінну, з-за чого по неуважності можна допустити помилку.

Наступний приклад коду, де була допущена схожа помилка:
public double Left { get; set; }
public double Top { get; set; }
public double Right { get; set; }
public double Bottom { get; set; }

internal bool IsDefault
{
get { return Left == 0 && Top == 0 && Right == 0 && Left == 0; }
}

Попередження PVS-Studio: V3001 There are identical sub-expressions 'Left == 0' to the left and to the right of the '&&' operator. Thickness.cs 29

У виразі 2 рази зустрічається підвираз Left == 0. Очевидно, що це помилка. На місці останнього подвираженія повинен знаходитися наступний код Bottom == 0, так як це єдине властивість (слідуючи логіці і виходячи з набору властивостей), не перевіряється, в даному виразі.

Наступна помилка цікава тим, що знаходиться в 2-ух файли з однаковими іменами і частково схожим кодом. Ось так і виходить, що помилки розмножуються — помилилися в одному місці, скопіювали цей код в інше — оп! — ось вам ще одне помилкове місце.
public override SizeRequest GetDesiredSize(int widthConstraint, 
int heightConstraint)
{
....
int width = widthConstraint;
if (widthConstraint <= 0)
width = (int)Context.GetThemeAttributeDp(global::Android
.Resource
.Attribute
.SwitchMinWidth);
else if (widthConstraint <= 0)
width = 100;
....
}

Попередження PVS-Studio: V3003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 28, 30. Xamarin.Forms.Platform.Android SwitchRenderer.cs 28

У даному фрагменті коду спостерігається дивна логіка в операторі if. Перевіряється деякий умова (widthConstraint <= 0), і, якщо воно не виконується, знову перевіряється це ж умова. Помилка? Помилка. А ось як виправити, сказати вже складніше. Це завдання вже лягає на плечі програміста, який писав код.

Як я говорив, знайшлася точно така ж помилка у файлі з такою ж назвою. Ось відповідне повідомлення аналізатора: V3003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 26, 28. Xamarin.Forms.Platform.Android SwitchRenderer.cs 26

Завдяки механізму віртуальних значень вдалося значно поліпшити ряд діагностичних правил, у тому числі і діагностику V3022, що визначає, що вираз завжди має значення true або false. Пропоную поглянути на кілька прикладів, які вдалося знайти з її допомогою:
public TypeReference ResolveWithContext(TypeReference type)
{
....
if (genericParameter.Owner.GenericParameterType == 
GenericParameterType.Type)
return TypeArguments[genericParameter.Position];
else
return genericParameter.Owner.GenericParameterType 
== GenericParameterType.Type
? UnresolvedGenericTypeParameter : 
UnresolvedGenericMethodParameter;
....
}

Попередження PVS-Studio: V3022 Expression 'genericParameter.Owner.GenericParameterType == GenericParameterType.Type' is always false. ICSharpCode.Decompiler TypesHierarchyHelpers.cs 441

Незважаючи на те, що я видалив ту частину методу, яка нас не цікавить, навіть зараз помилка може бути не занадто помітної. Щоб виправити цю ситуацію, пропоную ще трохи спростити код, переписавши його з більш короткими іменами змінних:
if (a == enVal)
return b;
else 
return a == enVal ? c : d;

Тепер все стало трохи зрозуміліше. Джерело проблеми – друга перевірка a == enVal (genericParameter.Owner.GenericParameterType == GenericParameterType.Type), що знаходиться в тернарном операторі. Тернарний оператор else-гілки оператора if не має сенсу — в цьому випадку метод завжди буде повертати значення d (UnresolvedGenericMethodParameter).

Якщо ви ще не здогадалися, в чому проблема — пояснюю. У разі, якщо програма дійде до обчислення значення тернарного оператора, вже точно відомо, що вираз a == enVal має значення false, отже, в тернарном операторі воно буде мати те ж значення. Підсумок: результат роботи тернарного оператора завжди один і той же. Помилочка.

Одразу подібні дефекти виявити буває важко, адже навіть вирізавши зайвий код з методу, що помилка вправно приховують в останньому. Довелося вдаватися до додаткових спрощень, щоб виявити цей «підводний камінь». Втім, для аналізатора такої проблеми немає, і він легко впорався з поставленим завданням.

Звичайно, це не єдиний подібний випадок. Ось ще один:
TypeReference DoInferTypeForExpression(ILExpression expr, 
TypeReference expectedType, 
bool forceInferChildren = 
false)
{
....
if (forceInferChildren) {
....
if (forceInferChildren) { 
InferTypeForExpression(expr.Arguments.Single(), lengthType);
}
}
....
}

Попередження PVS-Studio: V3022 Expression 'forceInferChildren' is always true. ICSharpCode.Decompiler TypeAnalysis.cs 632

Знову ж таки, для того, щоб легше помітити підступ, виріжемо весь зайвий код. І ось воно — 2 рази перевіряється умова forceInferChildren, при цьому між операторами if дана змінна ніяк не використовується. Якщо врахувати, що це параметр методу, можна зробити висновок, що ні інші потоки, ні будь-які методи не можуть змінити його без прямого звернення. Отже, якщо виконується перший оператор if, завжди буде виконуватися і другий. Дивна логіка.

Є діагностика, схожа з V3022 — V3063. Це діагностичне правило визначає, що частина виразу завжди істинною чи хибною. Завдяки їй вдалося виявити кілька цікавих фрагментів коду:
static BindableProperty GetBindableProperty(Type elementType, 
string localName, 
IXmlLineInfo lineInfo,
bool throwOnError = false)
{
....
Exception exception = null;
if (exception == null && bindableFieldInfo == null)
{
exception = new XamlParseException(
string.Format("BindableProperty {0} not found on {1}", 
localName + "Property", elementType.Name), lineInfo);
}
....
}

Попередження PVS-Studio: V3063 A part of conditional expression is always true: exception == null. Xamarin.Forms.Xaml ApplyPropertiesVisitor.cs 280

Нас цікавить підвираз exception == null. Очевидно, що воно завжди буде мати значення true. До чого тоді ця перевірка? Неясно. До речі, ніяких коментарів, якимось чином сигналізують про те, що значення може бути змінене в процесі налагодження (зразок // new Exception(); ) тут немає.

Це не єдині підозрілі місця, знайдені діагностичними правилами V3022 і V3063. Але не будемо на них зациклюватися, а подивимося, що ж ще цікавого вдалося знайти.
void WriteSecurityDeclarationArgument(CustomAttributeNamedArgument na)
{
....
output.Write("string('{0}')", 
NRefactory.CSharp
.TextWriterTokenWriter
.ConvertString(
(string)na.Argument.Value).Replace("'", "\'")); 
....
}

Попередження PVS-Studio: V3038 The first argument of 'Replace' function is equal to the second argument. ICSharpCode.Decompiler ReflectionDisassembler.cs 349

З цього коду нас цікавить метод Replace, що викликається для деякої рядка. Мабуть, програміст хотів замінити всі символи одинарних лапок символом слеша і лапки. Але справа в тому, що в другому випадку символ слеша екранується, тому виклик даного методу замінює одинарну лапку їй же. Не вірите? Equals("'", "\'") на допомогу. Для когось це може бути неочевидно, але не для аналізатора. Щоб уникнути екранування можна використовувати перед строковим литералом символ @. Тоді коректний виклик методу Replace виглядав би так:
Replace("'", @"\'")

Зустрічалися методи, завжди повертають одне і те ж значення. Наприклад:
static bool Unprocessed(ICollection<string> extra, Option def, 
OptionContext c, string argument)
{
if (def == null)
{
....
return false;
}
....
return false;
}

Попередження PVS-Studio: V3009 it's odd that this method always returns one and the same value of 'false'. Xamarin.Forms.UITest.TestCloud OptionSet.cs 239

Незалежно від того, які аргументи прийшли на вхід і що виконується в цьому методі, він завжди повертає значення false. Погодьтеся, виглядає це якось дивно.

До речі, цей код зустрівся ще раз — метод повністю скопіювали і перенесли в інше місце. Повідомлення аналізатора: V3009 it's odd that this method always returns one and the same value of 'false'. Xamarin.Forms.Xaml.Xamlg Options.cs 1020

Зустрілися декілька фрагментів коду з повторним генеруванням винятку, потенційно містять помилки.
static async Task<Stream> 
GetStreamAsync (Uri uri, CancellationToken cancellationToken)
{
try {
await Task.Delay (5000, cancellationToken);
} catch (TaskCanceledException ex) {
cancelled = true;
throw ex;
}

....
}

Попередження PVS-Studio: V3052 The original exception object 'ex' was swallowed. Stack of original exception could be lost. Xamarin.Forms.Core.UnitTests ImageTests.cs 221

Здавалося б, логіка проста. У разі виникнення виключення ми здійснюємо якісь дії і повторно генеруємо його. Але диявол криється в деталях. В даному випадку при повторної генерації виключення стек оригінального виключення повністю «затирається». Щоб цього уникнути, не потрібно заново генерувати це ж виключення, достатньо зробити «перекидання» існуючого, викликавши оператор throw. Тоді код блоку catch міг би виглядати так:
cancelled = true;
throw;

Схожий приклад:
public void Visit(ValueNode node, INode parentNode)
{
....
try
{
....
}
catch (ArgumentException ae)
{
if (ae.ParamName != "name")
throw ae;
throw new XamlParseException(
string.Format("An element with the name \"{0}\" 
already exists in this NameScope", 
(string)node.Value), node);
}
}

Попередження PVS-Studio: V3052 The original exception object 'ae' was swallowed. Stack of original exception could be lost. Xamarin.Forms.Xaml RegisterXNamesVisitor.cs 38

В обох випадках втрачається інформація про попередньому виключення. І якщо у останньому випадку ще можна припустити, що ця інформація не буде актуальною (хоча все одно дивно), то в першому — навряд чи, швидше за все, хотіли перекинути виняток вгору, але замість цього згенерували нове. Рішення таке ж, як і в попередньому прикладі — виклик оператора throw без аргументів.

На рахунок наступного фрагмента — не візьмуся сказати напевно, помилка це чи ні, але виглядає дивно.
void UpdateTitle()
{
if (Element?.Detail == null)
return;

((ITitleProvider)this).Title = (Element.Detail as NavigationPage)
?.CurrentPage?.Title 
?? Element.Title ?? Element?.Title;
}

Попередження PVS-Studio: V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the object Element Xamarin.Forms.Platform.WinRT MasterDetailPageRenderer.cs 288

Аналізатор насторожив той факт, що звернення до властивості Title виконується різними способами — Element.Title Element?.Title, причому спочатку звертаються безпосередньо, а потім — з використанням null-умовного оператора. Але тут все не так однозначно.

Як ви могли помітити, на початку методу виконується перевірка Element?.Detail == null, передбачає, що якщо Element == null, то вихід здійсниться тут і до подальших операцій справа не дійде.

У той же час вираз Element?.Title передбачає, що на момент його виконання Element може мати значення null. Якщо це так, то на попередньому етапі, у момент звернення до властивості Title безпосередньо, буде згенеровано виняток типу NullReferenceException, а отже – ніякого толку від використання null-умовного оператора немає.

У будь-якому випадку, цей код виглядає дуже дивно і його потрібно поправити.

Виглядало дивно, коли об'єкт приводили до власного ж типу. Ось приклад такого коду:
public FormsPivot Control { get; private set; }

Brush ITitleProvider.BarBackgroundBrush
{
set { (Control as FormsPivot).ToolbarBackground = value; }
}

Попередження PVS-Studio: V3051 An excessive type cast. The object is already of the 'FormsPivot' type. Xamarin.Forms.Platform.UAP TabbedPageRenderer.cs 73

В даному випадку це не помилка, але виглядає код як мінімум підозріло, з урахуванням того, що об'єкт Control вже має тип FormsPivot. До речі, це не єдине попередження подібного роду, зустрілися й інші:
  • V3051 An excessive type cast. The object is already of the 'FormsPivot' type. Xamarin.Forms.Platform.UAP TabbedPageRenderer.cs 78
  • V3051 An excessive type cast. The object is already of the 'FormsPivot' type. Xamarin.Forms.Platform.UAP TabbedPageRenderer.cs 282
  • V3051 An excessive type cast. The object is already of the 'FormsPivot' type. Xamarin.Forms.Platform.WinRT.Phone TabbedPageRenderer.cs 175
  • V3051 An excessive type cast. The object is already of the 'FormsPivot' type. Xamarin.Forms.Platform.WinRT.Phone TabbedPageRenderer.cs 197
  • V3051 An excessive type cast. The object is already of the 'FormsPivot' type. Xamarin.Forms.Platform.WinRT.Phone TabbedPageRenderer.cs 205
Є умови, які можна було б спростити. Приклад одного з них:
public override void LayoutSubviews()
{
....
if (_scroller == null || (_scroller != null && 
_scroller.Frame == Bounds))
return;
....
}

Попередження PVS-Studio: V3031 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions. Xamarin.Forms.Platform.iOS.Classic ContextActionCell.cs 102

Цей вираз можна спростити, прибравши підвираз _scroller != null. Воно буде обчислюватися тільки у разі, коли помилково вираз, що стоїть ліворуч від оператора '| | ' _scroller == null, отже — _scroller не дорівнює null і можна не побоюватися отримати виняток NullReferenceException. Тоді спрощений код буде виглядати так:
if (_scroller == null || _scroller.Frame == Bounds))

Ложка дьогтю
На жаль, зібрати рішення повністю не вдалося – близько 6 проектів залишилися неперевіреними, а ті місця, де класи з них так чи інакше використовувалися, не піддавалися такому ретельному аналізу, як могли б. Можливо, у них вдалося б знайти ще що-небудь цікаве, але, на жаль.

До речі, дізнатися про проблеми з аналізом можна за повідомленням V051, що знаходиться на 3 рівні. Наявність таких попереджень, як правило, є сигналом того, що C# проект містить якісь помилки компіляції, з-за чого аналізатор не може одержати повну інформацію, необхідну для проведення складного аналізу. Тим не менш, він постарається виконати перевірки, для яких не потрібна детальна інформація про типи і об'єктах.

При перевірці проекту бажано переконатися, що у вас відсутні попередження V051. А якщо вони все ж є – постаратися позбутися від них (перевірити, що проект скомпільовано, переконатися, що всі залежності завантажені).

Висновок


Перевірка Xamarin.Forms виправдала себе — знайшлися різні цікаві місця, як явно помилкові, так і вельми підозрілі або дивні. Сподіваюся, розробники не обійдуть статтю стороною і виправлять виписані тут фрагменти коду. Всі підозрілі місця, які вдалося виявити, можна переглянути, завантаживши пробну версію аналізатора. Ще кращим і більш правильним рішенням буде впровадження PVS-Studio на постійній основі, що дозволить виявляти і виправляти помилки відразу ж після їх появи.



Якщо хочете поділитися цією статтею з англомовної аудиторією, то прошу використовувати посилання на переклад: Sergey Vasiliev. Microsoft opened the source code of Xamarin.Forms. We couldn't miss a chance to check it with PVS-Studio.

Прочитали статтю і є питання?Часто до наших статей задають одні і ті ж питання. Відповіді на них ми зібрали тут: Відповіді на питання читачів статей про PVS-Studio, версія 2015. Будь ласка, ознайомтеся зі списком.

Джерело: Хабрахабр

0 коментарів

Тільки зареєстровані та авторизовані користувачі можуть залишати коментарі.