Форум программистов, компьютерный форум, киберфорум
el_programmer
Войти
Регистрация
Восстановить пароль
Блоги Сообщество Поиск  
PVS-Studio - это инструмент для выявления ошибок в исходном коде программ, написанных на языках С, C++ и C#.

PVS-Studio выполняет статический анализ кода и генерирует отчёт, помогающий программисту находить и устранять ошибки. PVS-Studio выполняет широкий спектр проверок кода, но наиболее силён в поисках опечаток и последствий неудачного Copy-Paste. Показательные примеры таких ошибок: V501, V517, V522, V523, V3001.

Анализатор ориентирован на разработчиков, использующих среду Visual Studio, и может в фоновом режиме выполнять анализ измененных файлов после их компиляции. В идеале ошибки будут обнаружены и исправлены ещё до попадания в репозиторий. Однако ничто не мешает использовать анализатор для проверки всего решения целиком или для встраивания в системы непрерывной интеграции. Эти и иные способы использования анализатора описаны в документации.

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

Запись от el_programmer размещена 25.05.2016 в 10:30
Показов 2508 Комментарии 0

Автор: Сергей Васильев

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

Нажмите на изображение для увеличения
Название: image1.png
Просмотров: 685
Размер:	178.2 Кб
ID:	3848

Анализируемый проект

Xamarin.Forms - кроссплатформенный набор инструментов, позволяющий создавать пользовательские интерфейсы, общие для различных платформ: Windows, Windows Phone, iOS, Android. Пользовательские интерфейсы отрисовываются, используя нативные компоненты конечной платформы, что позволяет сохранять приложениям, созданным с помощью Xamarin.Forms, общий вид для каждой платформы. Для создания пользовательских интерфейсов с привязками данных и различными стилями вы можете использовать C# или XAML-разметку.

Нажмите на изображение для увеличения
Название: image2.png
Просмотров: 627
Размер:	42.2 Кб
ID:	3849

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


Инструмент анализа

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

Нажмите на изображение для увеличения
Название: image3.png
Просмотров: 809
Размер:	131.4 Кб
ID:	3852

Каждое диагностическое правило содержит документацию, включающую в себя описание ошибки, а также примеры неверного и верного кода. Она поясняет, почему выбраны именно такие ограничения демонстрационной версии и что нужно сделать, чтобы попробовать весь функционал.


Подозрительные фрагменты кода

Начнём с "классических" ошибок, выявляемых диагностическим правилом V3001:

C#
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
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 - несмотря на то, что это средство очень помогает в написании кода, порой оно может "подсказать" не ту переменную, из-за чего по невнимательности можно допустить ошибку.

Следующий пример кода, где была допущена схожая ошибка:

C#
1
2
3
4
5
6
7
8
9
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-ух файлах с одинаковыми именами и частично схожим кодом. Вот так и получается, что ошибки размножаются - ошиблись в одном месте, скопировали этот код в другое - оп! - вот вам ещё одно ошибочное место.

C#
1
2
3
4
5
6
7
8
9
10
11
12
13
14
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. Предлагаю взглянуть на несколько примеров, которые удалось найти с её помощью:

C#
1
2
3
4
5
6
7
8
9
10
11
12
13
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.GenericParameter Type == GenericParameterType.Type' is always false. ICSharpCode.Decompiler TypesHierarchyHelpers.cs 441

Несмотря на то, что я удалил ту часть метода, которая нас не интересует, даже сейчас ошибка может быть не слишком заметной. Чтобы исправить эту ситуацию, предлагаю ещё немного упростить код, переписав его с более короткими именами переменных:

C#
1
2
3
4
if (a == enVal)
  return b;
else 
  return a == enVal ? c : d;
Теперь всё стало несколько понятнее. Источник проблемы – вторая проверка a == enVal (genericParameter.Owner.GenericParameter Type == GenericParameterType.Type), находящаяся в тернарном операторе. Тернарный оператор в else-ветви оператора if не имеет смысла - в этом случае метод всегда будет возвращать значение d (UnresolvedGenericMethodParameter).

Если вы ещё не догадались в чём проблема - объясняю. В случае, если программа дойдёт до вычисления значения тернарного оператора, уже точно известно, что выражение a == enVal имеет значение false, следовательно, в тернарном операторе оно будет иметь то же значение. Итог: результат работы тернарного оператора всегда один и тот же. Ошибочка.

Сходу подобные дефекты обнаружить бывает тяжело, ведь даже вырезав лишний код из метода, ошибка ловко утаивается в оставшемся. Пришлось прибегать к дополнительным упрощениям, чтобы выявить этот "подводный камень". Впрочем, для анализатора такой проблемы нет, и он легко справился с поставленной задачей.

Конечно, это не единственный подобный случай. Вот ещё один:

C#
1
2
3
4
5
6
7
8
9
10
11
12
13
14
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. Это диагностическое правило определяет, что часть условного выражения всегда истинна или ложна. Благодаря ей удалось обнаружить несколько интересных фрагментов кода:

C#
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
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. Но не будем на них зацикливаться, а посмотрим, что же ещё интересного удалось найти.

C#
1
2
3
4
5
6
7
8
9
10
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 выглядел бы так:

C#
1
Replace("'", @"\'")
Встречались методы, всегда возвращающие одно и то же значение. Например:

C#
1
2
3
4
5
6
7
8
9
10
11
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

Встретились несколько фрагментов кода с повторным генерированием исключения, потенциально содержащих ошибки.

C#
1
2
3
4
5
6
7
8
9
10
11
12
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 мог бы выглядеть так:

C#
1
2
cancelled = true;
throw;
Схожий пример:

C#
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
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 без аргументов.

На счёт следующего фрагмента - не возьмусь сказать наверняка, ошибка это или нет, но выглядит странно.

C#
1
2
3
4
5
6
7
8
9
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-условного оператора нет.

В любом случае, этот код выглядит очень странно и его нужно поправить.

Выглядело странно, когда объект приводили к собственному же типу. Вот пример такого кода:

C#
1
2
3
4
5
6
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

Есть условия, которые можно было бы упростить. Пример одного из них:

C#
1
2
3
4
5
6
7
8
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. Тогда упрощённый код будет выглядеть так:

C#
1
if (_scroller == null || _scroller.Frame == Bounds))

Ложка дёгтя

К сожалению, собрать решение полностью не удалось – порядка 6 проектов остались непроверенными, а те места, где классы из них так или иначе использовались, подвергались не такому тщательному анализу, как могли бы. Возможно, в них удалось бы найти ещё что-нибудь интересное, но, увы.

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

При проверке проекта желательно убедиться, что у вас отсутствуют предупреждения V051. А если они всё же есть – постараться избавиться от них (проверить, что проект скомпилирован, убедиться, что все зависимости загружены).


Заключение

Нажмите на изображение для увеличения
Название: image5.png
Просмотров: 886
Размер:	53.8 Кб
ID:	3851

Проверка Xamarin.Forms оправдала себя - нашлись разные интересные места, как явно ошибочные, так и весьма подозрительные или странные. Надеюсь, разработчики не обойдут статью стороной и исправят выписанные здесь фрагменты кода. Все подозрительные места, которые удалось обнаружить, можно посмотреть, загрузив пробную версию анализатора. Ещё лучшим и более правильным решением будет внедрение PVS-Studio на постоянной основе, что позволит обнаруживать и исправлять ошибки сразу же после их появления.
Изображения
Тип файла: jpg image4.jpg (168.4 Кб, 644 просмотров)
Надоела реклама? Зарегистрируйтесь и она исчезнет полностью.
Всего комментариев 0
Комментарии
 
Новые блоги и статьи
Установка MinGW GCC 16.2 и CMake
8Observer8 10.08.2026
VK Видео: https:/ / vkvideo. ru/ video-240781534_456239017 YouTube: eY5-5PyI9NM Текстовая версия
Неделя из жизни имитационной модели склада: мои кривые руки растут, откуда надо
anaschu 10.08.2026
Неделя из жизни имитационной модели склада: как я почти написал неправильную логику и что с этим делать Работаю сейчас над учебно-рабочим проектом: строю в AnyLogic имитационную модель процессов. . .
Калькулятор для расчета родства
russiannick 07.08.2026
1. Задача: Создать калькулятор для расчета родства. Родственных связей существует 8 ступеней, такие как: p - отец P - мать q - муж Q - жена b - брат B - сестра s - сын S - дочь
Мир по моей воле
kumehtar 07.08.2026
Когда-то кажется, что всё просто. Ты весь такой светлый. Причиняешь добро. Борешься за справедливость в этом тёмном мире. Потом начинаешь замечать одну неприятную вещь. Почти каждый хороший. . .
Кредитный калькулятор
Maks 05.08.2026
Решение задачи по прикладной информатике средствами 1С. Задача: Напишите приложение-калькулятор, которое помогает рассчитывать параметры кредита для аннуитетного и дифференцированного видов. . .
У нас сейчас поговорку "Опять 25" нужно переделать на "Опять +35".
kumehtar 04.08.2026
С ностальгией вспоминаю времена моего детства, когда у нас и правда +25 - была максимальная температура летом. Раньше +25 °C реально казались вершиной жары, когда можно было весь день пропадать на. . .
Как ИИ начал спорить и врать (возможно почуяв опасность для себя от индустрии - уход от электроники).
Hrethgir 04.08.2026
Недельный диалог, на фоне событий с НПЗ. Да, из спирта можно получать бензин, и это не сложно. Но потом в схеме я решил избавиться от насоса, при этом полностью сделав контроль подачи спирта в. . .
Термопринтер QR701
Argus19 03.08.2026
Термопринтер QR701 Купил два термопринтера QR701. На сэлф-тесте написано: Language: PC936 (GB18030). Что означает, что принтеры могут печатать только латиницу и китайские иероглифы. Так же. . .
КиберФорум - форум программистов, компьютерный форум, программирование
Powered by vBulletin
Copyright ©2000 - 2026, CyberForum.ru