Проверка Telerik UI for UWP для знакомства с PVS-Studio08.10.2019 11:33

Уже стало традицией, что программисты, пополняющие команду PVS-Studio, начинают свою деятельность с написания статьи про анализ проекта с открытым исходным кодом. В этот раз таким проверенным проектом станет Telerik UI for UWP.

Анализатор кода PVS-Studio

PVS-Studio — это инструмент для выявления ошибок и потенциальных уязвимостей в исходном коде программ, написанных на языках C, C++, C# и Java. Работает на Windows, Linux и macOS.

Анализатор предоставляет различные сценарии использования:

• может использоваться локально на машинах разработчиков, интегрируясь как плагин с Visual Studio или IntelliJ IDEA;
• может интегрироваться с платформой обеспечения непрерывного контроля качества SonarQube;
• может использоваться независимо, интегрируясь в систему сборки;
• возможно использование утилиты мониторинга компиляции;
• возможна интеграция с Azure DevOps, Jenkins, TeamCity, Travis CI и подобными системами;
• и так далее.

Проверяемый проект

Telerik UI for UWP — это набор компонентов пользовательского интерфейса для универсальной платформы Windows (UWP). Исходный код проекта можно найти на Github. Набор включает в себя более 20 компонентов, позволяющих визуализировать данные в виде графиков, создавать списки и таблицы, использовать карту для демонстрации данных, привязанных к местоположению.

Фрагменты, привлекшие внимание при изучении отчёта анализатора

Предупреждение PVS-Studio: V3013 It is odd that the body of 'OnMinValuePropertyChanged' function is fully equivalent to the body of 'OnMaxValuePropertyChanged' function. RadGauge.cs 446

private static void OnMinValuePropertyChanged(
                      DependencyObject sender,
                      DependencyPropertyChangedEventArgs args)
{
  double newVal = (double)args.NewValue;
  ValidateValue(newVal);

  RadGauge gauge = sender as RadGauge;
  if (gauge.panel != null)
  {
    gauge.panel.UpdateOnMinMaxValueChange();
  } 
  if(AutomationPeer.ListenerExists(AutomationEvents.PropertyChanged))
  {
    var peer = FrameworkElementAutomationPeer.FromElement(gauge) 
                                      as RadGaugeAutomationPeer;
    if (peer != null)
    {
      peer.RaiseMinimumPropertyChangedEvent((double)args.OldValue, 
                                           (double)args.NewValue);
    }
  }
}

private static void OnMaxValuePropertyChanged(
                      DependencyObject sender, 
                      DependencyPropertyChangedEventArgs args)
{
  double newVal = (double)args.NewValue;
  ValidateValue(newVal);

  RadGauge gauge = sender as RadGauge;
  if (gauge.panel != null)
  {
    gauge.panel.UpdateOnMinMaxValueChange();
  }

  if (AutomationPeer.ListenerExists(AutomationEvents.PropertyChanged))
  {
    var peer = FrameworkElementAutomationPeer.FromElement(gauge) 
                                       as RadGaugeAutomationPeer;
    if (peer != null)
    {
      peer.RaiseMinimumPropertyChangedEvent((double)args.OldValue, 
                                           (double)args.NewValue);
    }
  }
}

Анализатор обнаружил два метода, OnMinValuePropertyChanged и OnMaxValuePropertyChanged, выполняющих одинаковые действия. У меня есть сильные подозрения, что в этот код закралась ошибка. Обратите внимание, что и в методе OnMinValuePropertyChanged, и в методе OnMaxValuePropertyChanged, используется RaiseMinimumPropertyChangedEvent. При этом в классе RadGaugeAutomationPeer можно встретить как метод для «Minimum», так и для «Maximum»:

internal void RaiseMaximumPropertyChangedEvent(double oldValue, double newValue)
{
  this.RaisePropertyChangedEvent(
    RangeValuePatternIdentifiers.MaximumProperty, 
    oldValue, 
    newValue);
}

internal void RaiseMinimumPropertyChangedEvent(double oldValue, double newValue)
{
  this.RaisePropertyChangedEvent(
    RangeValuePatternIdentifiers.MinimumProperty,
    oldValue,
    newValue);
}

Метод RaiseMaximumPropertyChangedEvent в коде не используется ни разу, а вот RaiseMinimumPropertyChangedEvent  — дважды. И, знаете, работоспособность метода OnMaxValuePropertyChanged вызывает вопросы… Я думаю, что предполагалось написать следующее:

private static void OnMaxValuePropertyChanged(
                      DependencyObject sender,
                      DependencyPropertyChangedEventArgs args)
{
  ....
  peer.RaiseMaximumPropertyChangedEvent((double)args.OldValue,
                                        (double)args.NewValue);
  ....
}

Но даже так код выглядит не очень аккуратно из-за большого количества повторяющихся элементов. Он сложен для восприятия, а повторяющиеся строки притупляют внимание программиста, и проводить code review в таких условиях становится сложнее. Зато с проверкой такого кода отлично справляются инструменты статического анализа (но это не значит, что можно обойтись без рефакторинга и особенно без сокращения количества повторяющихся строк).

По рассмотренному и следующему фрагменту кода можно предположить, что авторы не прочь позаниматься copy-paste. Впрочем, как и все мы… :)

Предупреждение PVS-Studio: V3001 There are identical sub-expressions 'element.RenderSize == emptySize' to the left and to the right of the '||' operator. TiltInteractionEffect.cs 181

private static bool IsPointInElementBounds(FrameworkElement element, 
                                           Point position)
{
  Size emptySize = new Size(0, 0);
  if (element.RenderSize == emptySize || 
      element.RenderSize == emptySize)
  {
    return false;
  }

  return new Rect(....).Contains(position);
} 

Анализатор обнаружил ошибочный фрагмент кода, в котором справа и слева от оператора '||' в условии оператора if используются одинаковые подвыражения. Второе подвыражение явно должно было выглядеть иначе. Возможно, на месте второго RenderSize должен стоять DesiredSize. Или второго подвыражения вообще не должно быть. В любом случае, этот фрагмент кода требует исправления.

Предупреждение PVS-Studio: V3001 There are identical sub-expressions 'text[0] == '-'' to the left and to the right of the '||' operator. RadNumericBox.cs 1057

private void ValidateText()
{
  string text = this.textBox.Text;
  ....
  if (text.Length == 1 && (text[0] == '-' || text[0] == '-'))
  {
    if (this.isNegative)
    {
      this.isNegative = false;
    }
    else
    {
      this.SetText(string.Empty);
    }
    return;
  }
  ....
}

Тут разработчик записывает текст, введенный в поле текстбокса, в переменную. Затем первый символ строки сравнивается дважды с одним и тем же символом '-', что является подозрительным решением. Очевидно, валидация текста в этой функции работает не так, как изначально задумывалось.

Предупреждение PVS-Studio: V3001 There are identical sub-expressions 'newValue.HasValue' to the left and to the right of the '&&' operator. DateTimePicker.cs 576

private static void OnValueChanged(object sender, 
                                   DependencyPropertyChangedEventArgs args)
{
  DateTimePicker picker = sender as DateTimePicker;

  var newValue = (DateTime?)args.NewValue;

  if (newValue.HasValue && newValue != null)                     // <=
  ....
}

Выражение newValue.HasValue возвращает true, если newValue содержит какое-либо значение, и выражение newValue!=null делает то же самое. Анализатор обращает на это внимание, а что делать — убрать одно из подвыражений или заменить его на другое (если проверяться должно было что-то ещё), решать уже разработчикам.

Предупреждение PVS-Studio: V3125 The 'CurrentAttachedMenu' object was used after it was verified against null. Check lines: 98, 96. PopupService.cs 98

internal static class PopupService
{
  ....
  private static void Overlay_PointerPressed(....)
  {
    if (CurrentAttachedMenu == null ||
        !CurrentAttachedMenu.hitTestService.
            HitTest(e.GetCurrentPoint(CurrentAttachedMenu).Position).Any())
    {
      CurrentAttachedMenu.IsOpen = false;
      HideOverlay();
    }
  }
}

Если переменная CurrentAttachedMenu окажется равна null, то вычисление выражения CurrentAttachedMenu.IsOpen приведёт к возникновению исключения. С первого взгляда кажется, что это простая опечатка и имелось в виду не сравнение с null, а обратная операция — '!='. Но тогда возникнет исключение в условии оператора if, если переменная CurrentAttachedMenu будет иметь значение null.

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

Предупреждение PVS-Studio: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'dragDropElement', 'uiDragDropElement'. DragDrop.cs 91

internal static void StartDrag(....)
{
  var dragDropElement = sender as IDragDropElement;
  ....
  UIElement uiDragDropElement = dragDropElement as UIElement;
  ....
  if (dragDropElement == null ||
      !dragDropElement.CanStartDrag(trigger, initializeContext))
  {
    return;
  }
  ....
}

Тут же весьма вероятно, что автор перепутал переменные. На неравенство null проверяется не полученная в результате приведения ссылка, а исходная (dragDropElement). Скорее всего проверить предполагалось всё же ссылку uiDragDropElement. Догадку подтверждает и то, что программист далее использовал uiDragDropElement без проверок на значение null.

Предупреждение PVS-Studio: V3030 Recurring check. The '! showIndicatorWhenNoData' condition was already verified in line 139. RadDataBoundListBox.PullToRefresh.cs 141

internal void HandlePullToRefreshItemStateChanged(object item, ItemState state)
{
  ....
  bool showIndicatorWhenNoData = this.ShowPullToRefreshWhenNoData;

  if (this.realizedItems.Count == 0 && !showIndicatorWhenNoData)
  {
    if (state == ItemState.Recycled && !showIndicatorWhenNoData)
    {
      this.StopPullToRefreshLoading(false);
      this.HidePullToRefreshIndicator();
    }
    return;
  }
  ....
}

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

Предупреждение PVS-Studio: V3031 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions. SelectedItemCollection.cs 77

internal class SelectedItemCollection : ObservableCollection