# Microsoft открыла исходники Xamarin\.Forms\. Мы не могли упустить шанс проверить их с помощью PVS\-Studio Не так давно, как вы наверняка знаете, корпорация Microsoft купила компанию Xamarin\. Даже несмотря на то, что в последнее время Microsoft начала постепенно открывать исходные коды своих продуктов, открытие кода Xamarin\.Forms стало большим сюрпризом\. Я не смог пройти мимо такого события, и решил проверить исходный код этого проекта с помощью статического анализатора кода\. ![0400_XamarinForms_ru/image1.png](https://import.viva64.com/docx/blog/0400_XamarinForms_ru/image1.png) ## Анализируемый проект [Xamarin\.Forms](https://dotnet.microsoft.com/apps/xamarin/xamarin-forms) \- кроссплатформенный набор инструментов, позволяющий создавать пользовательские интерфейсы, общие для различных платформ: Windows, Windows Phone, iOS, Android\. Пользовательские интерфейсы отрисовываются, используя нативные компоненты конечной платформы, что позволяет сохранять приложениям, созданным с помощью Xamarin\.Forms, общий вид для каждой платформы\. Для создания пользовательских интерфейсов с привязками данных и различными стилями вы можете использовать C\# или XAML\-разметку\. ![0400_XamarinForms_ru/image2.png](https://import.viva64.com/docx/blog/0400_XamarinForms_ru/image2.png) Код самого фреймворка также написан на языке C\# и доступен в [репозитории на GitHub](https://github.com/xamarin/xamarin.forms)\. ## Инструмент анализа Проект проверялся с помощью статического анализатора кода [PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/), в разработке которого я принимаю активное участие\. Мы постоянно работаем над его улучшением, в том числе дорабатывая существующие и добавляя новые диагностические правила\. Поэтому с каждой проверкой нового проекта нам удаётся выявлять всё больше разновидностей ошибок\. ![0400_XamarinForms_ru/image4.png](https://import.viva64.com/docx/blog/0400_XamarinForms_ru/image4.png) Каждое диагностическое правило содержит [документацию](https://pvs-studio.ru/ru/docs/warnings/), включающую в себя описание ошибки, а также примеры неверного и верного кода\. Загрузить пробную версию анализатора можно по [ссылке](https://pvs-studio.ru/ru/pvs-studio/download/)\. Также предлагаю познакомиться с [заметкой](https://pvs-studio.ru/ru/blog/posts/0395/), которую недавно написал мой коллега\. Она поясняет, почему выбраны именно такие ограничения демонстрационной версии и что нужно сделать, чтобы попробовать весь функционал\. Для тех, кто поленился читать, сразу подскажу \- достаточно [написать](https://pvs-studio.ru/ru/about-feedback/) нам\. **P\.S\.** Кроме того на сайте есть [база ошибок](https://pvs-studio.ru/ru/blog/examples/), найденных в open source проектах, и [каталог статей](https://pvs-studio.ru/ru/blog/posts/) \(о проверке open source проектов, технической направленности, и прочих\)\. Рекомендую ознакомиться\. ## Подозрительные фрагменты кода Начнём с "классических" ошибок, выявляемых диагностическим правилом V3001: ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v3001/) There are identical sub\-expressions 'RwWait' to the left and to the right of the '\|' operator\. SplitOrderedList\.cs 458 Как видно из кода, вычисляется значение какого\-то выражения с использованием битовых операций\. При этом в одном из подвыражений \- _RwWait \| RwWait_ участвуют одинаковые константные поля\. Это не имеет смысла\. При этом видно, что набор констант, объявленный выше, имеет значения, равные степеням двойки, следовательно, подразумевалось их использование как флагов \(что мы и видим на примере использования битовых операций\)\. Думаю, практичнее было бы вынести их в перечисление, отмеченное атрибутом \[Flags\], что дало бы ряд преимуществ при работе с этим перечислением \(см\. [документацию V3059](https://pvs-studio.ru/ru/docs/warnings/v3059/)\)\. Что до текущего примера \- скорее всего, подразумевалось использование константы _RwWrite_\. Это можно отнести к одному из минусов IntelliSense \- несмотря на то, что это средство очень помогает в написании кода, порой оно может "подсказать" не ту переменную, из\-за чего по невнимательности можно допустить ошибку\. Следующий пример кода, где была допущена схожая ошибка: ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/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_, так как это единственное свойство \(следуя логике и исходя из набора свойств\), не проверяемое в данном выражении\. Следующая ошибка интересна тем, что находится в двух файлах с одинаковыми именами и частично схожим кодом\. Вот так и получается, что ошибки размножаются \- ошиблись в одном месте, скопировали этот код в другое \- оп\! \- вот вам ещё одно ошибочное место\. ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/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 Благодаря механизму [виртуальных значений](https://pvs-studio.ru/ru/blog/posts/0394/) удалось значительно улучшить ряд диагностических правил, в том числе и диагностику [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/), определяющую, что выражение всегда имеет значение _true_ или _false_\. Предлагаю взглянуть на несколько примеров, которые удалось найти с её помощью: ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'genericParameter\.Owner\.GenericParameterType \=\= GenericParameterType\.Type' is always false\. ICSharpCode\.Decompiler TypesHierarchyHelpers\.cs 441 Несмотря на то, что я удалил ту часть метода, которая нас не интересует, даже сейчас ошибка может быть не слишком заметной\. Чтобы исправить эту ситуацию, предлагаю ещё немного упростить код, переписав его с более короткими именами переменных: ```cpp 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_, следовательно, в тернарном операторе оно будет иметь то же значение\. Итог: результат работы тернарного оператора всегда один и тот же\. Ошибочка\. Сходу подобные дефекты обнаружить бывает тяжело, ведь даже вырезав лишний код из метода, ошибка ловко утаивается в оставшемся\. Пришлось прибегать к дополнительным упрощениям, чтобы выявить этот "подводный камень"\. Впрочем, для анализатора такой проблемы нет, и он легко справился с поставленной задачей\. Конечно, это не единственный подобный случай\. Вот ещё один: ```cpp TypeReference DoInferTypeForExpression(ILExpression expr, TypeReference expectedType, bool forceInferChildren = false) { .... if (forceInferChildren) { .... if (forceInferChildren) { InferTypeForExpression(expr.Arguments.Single(), lengthType); } } .... } ``` **Предупреждение PVS\-Studio:** [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'forceInferChildren' is always true\. ICSharpCode\.Decompiler TypeAnalysis\.cs 632 Опять же, для того, чтобы легче заметить подвох, вырежем весь лишний код\. И вот оно \- 2 раза проверяется условие _forceInferChildren_, при этом между операторами _if_ данная переменная никак не используется\. Если учесть, что это параметр метода, можно сделать заключение, что ни другие потоки, ни какие\-либо методы не могут изменить его без прямого обращения\. Следовательно \- если выполняется первый оператор _if_, всегда будет выполняться и второй\. Странная логика\. Есть диагностика, схожая с V3022 \- V3063\. Это диагностическое правило определяет, что часть условного выражения всегда истинна или ложна\. Благодаря ей удалось обнаружить несколько интересных фрагментов кода: ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v3063/) A part of conditional expression is always true: exception \=\= null\. Xamarin\.Forms\.Xaml ApplyPropertiesVisitor\.cs 280 Нас интересует подвыражение _exception \=\= null_\. Очевидно, что оно всегда будет иметь значение _true_\. К чему тогда эта проверка? Неясно\. К слову, никаких комментариев, каким\-то образом сигнализирующих о том, что значение может быть изменено в процессе отладки \(вроде _// new Exception\(\);_\) здесь нет\. Это не единственные подозрительные места, найденные диагностическими правилами V3022 и V3063\. Но не будем на них зацикливаться, а посмотрим, что же ещё интересного удалось найти\. ```cpp void WriteSecurityDeclarationArgument(CustomAttributeNamedArgument na) { .... output.Write("string('{0}')", NRefactory.CSharp .TextWriterTokenWriter .ConvertString( (string)na.Argument.Value).Replace("'", "\'")); .... } ``` **Предупреждение PVS\-Studio:** [V3038](https://pvs-studio.ru/ru/docs/warnings/v3038/) The first argument of 'Replace' function is equal to the second argument\. ICSharpCode\.Decompiler ReflectionDisassembler\.cs 349 Из этого кода нас интересует метод _Replace_, вызываемый для некоторой строки\. Видимо, программист хотел заменить все символы _одинарной кавычки_ символом _слеша и кавычки_\. Но дело в том, что во втором случае символ слеша экранируется, поэтому вызов данного метода заменяет одинарную кавычку ей же\. Не верите? _Equals\("'", "\\'"\)_ в помощь\. Для кого\-то это может быть неочевидно, но не для анализатора\. Чтобы избежать экранирования, можно использовать перед строковым литералом символ _@_\. Тогда корректный вызов метода _Replace_ выглядел бы так: ```cpp Replace("'", @"\'") ``` Встречались методы, всегда возвращающие одно и то же значение\. Например: ```cpp static bool Unprocessed(ICollection extra, Option def, OptionContext c, string argument) { if (def == null) { .... return false; } .... return false; } ``` **Предупреждение PVS\-Studio:** [V3009](https://pvs-studio.ru/ru/docs/warnings/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 Встретились несколько фрагментов кода с повторным генерированием исключения, потенциально содержащих ошибки\. ```cpp static async Task GetStreamAsync (Uri uri, CancellationToken cancellationToken) { try { await Task.Delay (5000, cancellationToken); } catch (TaskCanceledException ex) { cancelled = true; throw ex; } .... } ``` **Предупреждение PVS\-Studio:** [V3052](https://pvs-studio.ru/ru/docs/warnings/v3052/) The original exception object 'ex' was swallowed\. Stack of original exception could be lost\. Xamarin\.Forms\.Core\.UnitTests ImageTests\.cs 221 Казалось бы, логика проста\. В случае возникновения исключения мы совершаем какие\-то действия и повторно генерируем его\. Но дьявол кроется в деталях\. В данном случае при повторной генерации исключения стек оригинального исключения полностью "затирается"\. Чтобы этого избежать, не нужно заново генерировать это же исключение, достаточно сделать "переброс" существующего, вызвав оператор _throw_\. Тогда код блока _catch_ мог бы выглядеть так: ```cpp cancelled = true; throw; ``` Схожий пример: ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v3052/) The original exception object 'ae' was swallowed\. Stack of original exception could be lost\. Xamarin\.Forms\.Xaml RegisterXNamesVisitor\.cs 38 В обоих случаях теряется информация о предыдущем исключении\. И если во втором случае ещё можно предположить, что эта информация не будет актуальной \(хотя всё равно странно\), то в первом \- вряд ли, скорее всего, хотели перебросить исключение вверх, но вместо этого сгенерировали новое\. Решение такое же, как и в предыдущем примере \- вызов оператора _throw_ без аргументов\. На счёт следующего фрагмента \- не возьмусь сказать наверняка, ошибка это или нет, но выглядит странно\. ```cpp void UpdateTitle() { if (Element?.Detail == null) return; ((ITitleProvider)this).Title = (Element.Detail as NavigationPage) ?.CurrentPage?.Title ?? Element.Title ?? Element?.Title; } ``` **Предупреждение PVS\-Studio:** [V3042](https://pvs-studio.ru/ru/docs/warnings/v3042/) Possible NullReferenceException\. The '?\.' and '\.' operators are used for accessing members of the Element object 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\-условного оператора нет\. В любом случае, этот код выглядит очень странно и его нужно поправить\. Выглядело странно, когда объект приводили к собственному же типу\. Вот пример такого кода: ```cpp public FormsPivot Control { get; private set; } Brush ITitleProvider.BarBackgroundBrush { set { (Control as FormsPivot).ToolbarBackground = value; } } ``` **Предупреждение PVS\-Studio:** [V3051](https://pvs-studio.ru/ru/docs/warnings/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 Есть условия, которые можно было бы упростить\. Пример одного из них: ```cpp public override void LayoutSubviews() { .... if (_scroller == null || (_scroller != null && _scroller.Frame == Bounds)) return; .... } ``` **Предупреждение PVS\-Studio:** [V3031](https://pvs-studio.ru/ru/docs/warnings/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_\. Тогда упрощённый код будет выглядеть так: ```cpp if (_scroller == null || _scroller.Frame == Bounds)) ``` ## Ложка дёгтя К сожалению, собрать решение полностью не удалось – порядка 6 проектов остались непроверенными, а те места, где классы из них так или иначе использовались, подвергались не такому тщательному анализу, как могли бы\. Возможно, в них удалось бы найти ещё что\-нибудь интересное, но, увы\. К слову, узнать о проблемах с анализом можно по сообщению [V051](https://pvs-studio.ru/ru/docs/warnings/v051/), находящемуся на 3 уровне\. Наличие таких предупреждений, как правило, является сигналом того, что C\# проект содержит какие\-то ошибки компиляции, из\-за чего анализатор не может получить полную информацию, необходимую для проведения сложного анализа\. Тем не менее, он постарается выполнить проверки, для которых не нужна подробная информация о типах и объектах\. При проверке проекта желательно убедиться, что у вас отсутствуют предупреждения V051\. А если они всё же есть – постараться избавиться от них \(проверить, что проект скомпилирован, убедиться, что все зависимости загружены\)\. ## Заключение ![0400_XamarinForms_ru/image5.png](https://import.viva64.com/docx/blog/0400_XamarinForms_ru/image5.png) Проверка Xamarin\.Forms оправдала себя \- нашлись разные интересные места, как явно ошибочные, так и весьма подозрительные или странные\. Надеюсь, разработчики не обойдут статью стороной и исправят выписанные здесь фрагменты кода\. Все подозрительные места, которые удалось обнаружить, можно посмотреть, загрузив пробную [версию анализатора](https://pvs-studio.ru/ru/pvs-studio/download/)\. Ещё лучшим и более правильным решением будет внедрение PVS\-Studio на постоянной основе, что позволит обнаруживать и исправлять ошибки сразу же после их появления\.