Не так давно, как вы наверняка знаете, корпорация 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, так как это единственное свойство (следуя логике и исходя из набора свойств), не проверяемое в данном выражении.
Следующая ошибка интересна тем, что находится в двух файлах с одинаковыми именами и частично схожим кодом. Вот так и получается, что ошибки размножаются — ошиблись в одном месте, скопировали этот код в другое — оп! — вот вам ещё одно ошибочное место.
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 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-условного оператора нет.
В любом случае, этот код выглядит очень странно и его нужно поправить.
Выглядело странно, когда объект приводили к собственному же типу. Вот пример такого кода:
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
