Сергей Хренов

30 Янв 2017

Теги:

#CSharp

Повторная проверка SharpDevelop: что нового?

30 Янв 2017

Автор: Сергей Хренов

Введение
Результаты проверки
Заключение

Инструмент PVS-Studio постоянно совершенствуется. При этом наиболее динамично в настоящее время развивается анализатор C# кода: в 2016 году в него было добавлено девяносто новых диагностических правил. Ну а лучшим показателем качества работы анализатора являются обнаруживаемые им ошибки. Всегда интересно, а также достаточно полезно, проводить повторные проверки больших открытых проектов, сравнивая результаты. Сегодня я остановлюсь на повторной проверке проекта SharpDevelop.

Введение

Предыдущая статья про проверку SharpDevelop была опубликована Андреем Карповым в ноябре 2015 года. В то время мы только тестировали новый C# анализатор, готовясь к выпуску первого релиза. Тем не менее, уже тогда, при помощи бета-версии, Андрею удалось обнаружить в проекте SharpDevelop несколько любопытных ошибок. После этого SharpDevelop был "помещен на полку" и использовался вместе с другими проектами только для внутреннего тестирования при разработке новых диагностических правил. И вот, наконец, наступило время проверить SharpDevelop еще раз, но уже более "мускулистой" версией анализатора PVS-Studio 6.12.

Для проверки я загрузил актуальную версию исходного кода SharpDevelop с портала GitHub. Проект содержит около миллиона строк кода на языке C#. В процессе работы анализатор выдал 809 предупреждений. Из них первого уровня - 74, второго - 508, третьего - 227:

Не будем рассматривать предупреждения на уровне Low: статистически там велик процент ложных срабатываний. Анализ предупреждений на уровнях Meduim и High (582 предупреждения) выявил наличие около 40% ошибочных, либо крайне подозрительных конструкций. Это составляет 233 предупреждения. Другими словами, анализатор PVS-Studio в среднем нашел 0.23 ошибки на 1000 строк кода. Это говорит о высоком качестве кода проекта SharpDevelop. На многих других проектах всё выглядит куда более печально.

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

Результаты проверки

Эталонная ошибка Copy-Paste

Ошибка, которая по праву может занять почетное место в "палате мер и весов". Яркая иллюстрация полезности использования статического анализа кода и опасности Copy-Paste.

Предупреждение анализатора PVS-Studio: V3102 Suspicious access to element of 'method.SequencePoints' object by a constant index inside a loop. CodeCoverageMethodTreeNode.cs 52

public override void ActivateItem()
{
  if (method != null && method.SequencePoints.Count > 0) {
    CodeCoverageSequencePoint firstSequencePoint =  
      method.SequencePoints[0];
    ....
    for (int i = 1; i < method.SequencePoints.Count; ++i) {
      CodeCoverageSequencePoint sequencePoint = 
        method.SequencePoints[0];  // <=
      ....
    }
    ....
  }
  ....
}

На каждой итерации цикла for используют доступ к нулевому элементу коллекции. Я умышленно привел дополнительный фрагмент кода, находящийся сразу после условия if. Легко заметить, откуда была скопирована строка, помещенная внутрь цикла. Имя переменной firstSequencePoint заменили на sequencePoint, а вот выражение доступа по индексу изменить забыли. Правильный вариант данной конструкции имеет вид:

public override void ActivateItem()
{
  if (method != null && method.SequencePoints.Count > 0) {
    CodeCoverageSequencePoint firstSequencePoint =  
      method.SequencePoints[0];
    ....
    for (int i = 1; i < method.SequencePoints.Count; ++i) {
      CodeCoverageSequencePoint sequencePoint = 
        method.SequencePoints[i];
      ....
    }
    ....
  }
  ....
}

"Найдите 10 отличий", или снова Copy-Paste

Предупреждение анализатора PVS-Studio: V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless NamespaceTreeNode.cs 87

public int Compare(SharpTreeNode x, SharpTreeNode y)
{
  ....
  if (typeNameComparison == 0) {
    if (x.Text.ToString().Length < y.Text.ToString().Length)  // <=
      return -1;
    if (x.Text.ToString().Length < y.Text.ToString().Length)  // <=
      return 1;
  }  
  ....
}

Оба блока if содержат одинаковое условие. В данном случае трудно судить о том, как должен выглядеть правильный вариант программы. Необходимо изучение фрагмента кода его автором.

Несвоевременная проверка на равенство null

Предупреждение анализатора PVS-Studio: V3095 The 'position' object was used before it was verified against null. Check lines: 204, 206. Task.cs 204

public void JumpToPosition()
{
  if (hasLocation && !position.IsDeleted)  // <=
    ....
  else if (position != null)
    ....
}

Переменную position используют, не проверив на равенство null. Проверка производится в другом условии, в блоке кода else. Правильный вариант кода мог бы иметь вид:

public void JumpToPosition()
{
  if (hasLocation && position != null && !position.IsDeleted)
    ....
  else if (position != null)
    ....
}

Пропущенная проверка на равенство null

Предупреждение анализатора PVS-Studio: V3125 The 'mainAssemblyList' object was used after it was verified against null. Check lines: 304, 291. ClassBrowserPad.cs 304

void UpdateActiveWorkspace()
{
  var mainAssemblyList = SD.ClassBrowser.MainAssemblyList;
  if ((mainAssemblyList != null) && (activeWorkspace != null)) {
    ....
  }
  ....
  mainAssemblyList.Assemblies.Clear();  // <=
  ....
}

Переменную mainAssemblyList используют без предварительной проверки на равенство null. При этом другой фрагмент кода содержит такую проверку. Корректный вариант кода:

void UpdateActiveWorkspace()
{
  var mainAssemblyList = SD.ClassBrowser.MainAssemblyList;
  if ((mainAssemblyList != null) && (activeWorkspace != null)) {
    ....
  }
  ....
  if (mainAssemblyList != null) {
    mainAssemblyList.Assemblies.Clear();
  }  
  ....
}

Неожиданный результат сортировки

Предупреждение анализатора PVS-Studio: V3078 Original sorting order will be lost after repetitive call to 'OrderBy' method. Use 'ThenBy' method to preserve the original sorting. CodeCoverageMethodElement.cs 124

void Init()
{
  ....
  this.SequencePoints.OrderBy(item => item.Line)
                     .OrderBy(item => item.Column);  // <=
}

Результатом работы данного фрагмента кода будет сортировка коллекции SequencePoints только по полю Column. По всей видимости, это не совсем то, что ожидал автор кода. Проблема заключается в том, что повторный вызов метода OrderBy выполнит сортировку коллекции без учета результата предыдущей сортировки. Для исправления ситуации необходимо использовать ThenBy вместо повторного вызова OrderBy:

void Init()
{
  ....
  this.SequencePoints.OrderBy(item => item.Line)
                     .ThenBy(item => item.Column);
}

Потенциальная возможность деления на ноль

Предупреждение анализатора PVS-Studio: V3064 Potential division by zero. Consider inspecting denominator 'workAmount'. XamlSymbolSearch.cs 60

public XamlSymbolSearch(IProject project, ISymbol entity)
{
  ....
  interestingFileNames = new List<FileName>();
  ....
  foreach (var item in ....)
    interestingFileNames.Add(item.FileName);
  ....
  workAmount = interestingFileNames.Count;
  workAmountInverse = 1.0 / workAmount;  // <=
}

В случае, если коллекция interestingFileNames окажется пустой, произойдет деление на ноль. Здесь достаточно трудно предложить подходящий вариант исправления ошибки. Но, в любом случае, требуется доработка алгоритма вычисления значения переменной workAmountInverse в ситуации, когда переменная workAmount будет равна нулю.

Повторное присваивание

Предупреждение анализатора PVS-Studio: V3008 The 'ignoreDialogIdSelectedInTextEditor' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 204, 201. WixDialogDesigner.cs 204

void OpenDesigner()
{
  try {
    ignoreDialogIdSelectedInTextEditor = true;  // <=
    WorkbenchWindow.ActiveViewContent = this;
  } finally {
    ignoreDialogIdSelectedInTextEditor = false;  // <=
  }
}

Переменная ignoreDialogIdSelectedInTextEditor получит значение false независимо от результата выполнения блока try. Для исключения вероятности наличия "подводных камней" ознакомимся с объявлением используемых переменных. Объявление ignoreDialogIdSelectedInTextEditor имеет вид:

bool ignoreDialogIdSelectedInTextEditor;

Объявление IWorkbenchWindow и ActiveViewContent:

public IWorkbenchWindow WorkbenchWindow {
  get { return workbenchWindow; }
}
IViewContent ActiveViewContent {
  get;
  set;
}

Как видим, нет никаких очевидных причин для повторного присвоения переменной ignoreDialogIdSelectedInTextEditor значения. Рискну предположить, что корректный вариант приведенной конструкции мог бы отличаться от оригинала использованием ключевого слова catch вместо finally:

void OpenDesigner()
{
  try {
    ignoreDialogIdSelectedInTextEditor = true;
    WorkbenchWindow.ActiveViewContent = this;
  } catch {
    ignoreDialogIdSelectedInTextEditor = false;
  }
}

Ошибочный поиск подстроки

Предупреждение анализатора PVS-Studio: V3053 An excessive expression. Examine the substrings '/debug' and '/debugport'. NDebugger.cs 287

public bool IsKernelDebuggerEnabled {
  get {
    ....
    if (systemStartOptions.Contains("/debug") ||
     systemStartOptions.Contains("/crashdebug") ||
     systemStartOptions.Contains("/debugport") ||  // <=
     systemStartOptions.Contains("/baudrate")) {
      return true;
    }
    ....
  }
}

В строке systemStartOptions производится последовательный поиск одной из подстрок "/debug" или "/debugport". Проблема заключается в том, что строка "/debug" сама является подстрокой для "/debugport". Таким образом, нахождение подстроки "/debug" делает дальнейший поиск подстроки "/debugport" бессмысленным. Пожалуй, это не ошибка, но код можно упростить:

public bool IsKernelDebuggerEnabled {
  get {
    ....
    if (systemStartOptions.Contains("/debug") ||
     systemStartOptions.Contains("/crashdebug") ||
     systemStartOptions.Contains("/baudrate")) {
      return true;
    }
    ....
  }
}

Ошибка обработки исключения

Предупреждение анализатора PVS-Studio: V3052 The original exception object 'ex' was swallowed. Stack of original exception could be lost. ReferenceFolderNodeCommands.cs 130

DiscoveryClientProtocol DiscoverWebServices(....)
{
  try {
    ....
  } catch (WebException ex) {
    if (....) {
      ....
    } else {
      throw ex;  // <=
    }
  }
  ....
}

В данном случае вызов throw ex приведет к "затиранию" стека оригинального исключения, так как перехваченное исключение будет сгенерировано повторно. Исправленный вариант:

DiscoveryClientProtocol DiscoverWebServices(....)
{
  try {
    ....
  } catch (WebException ex) {
    if (....) {
      ....
    } else {
      throw;
    }
  }
  ....
}

Использование неинициализированного поля в конструкторе класса

Предупреждение анализатора PVS-Studio: V3128 The 'contentPanel' field is used before it is initialized in constructor. SearchResultsPad.cs 66

Grid contentPanel;
public SearchResultsPad()
{
  ....
  defaultToolbarItems = ToolBarService
    .CreateToolBarItems(contentPanel, ....);  // <=
  ....
  contentPanel = new Grid {....};
  ....
}

Поле contentPanel передается в качестве одного из параметров в метод CreateToolBarItems в конструкторе класса SearchResultsPad. При этом инициализация данного поля производится уже после использования. Возможно, в данном случае ошибки нет, так как в теле метода CreateToolBarItems и далее по стеку может быть учтена возможность равенства null переменной contentPanel. Но код выглядит подозрительно и требует проверки автором.

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

Заключение

И вновь PVS-Studio не подвел: в ходе повторной проверки проекта SharpDevelop были найдены новые интересные ошибки. А это значит, что анализатор отлично справляется со своей работой, позволяя делать мир вокруг нас чуть более совершенным.

Напоминаю, что и вы можете в любое время включиться в данный процесс, воспользовавшись возможностью бесплатного использования статического анализатора PVS-Studio для проверки собственных проектов.

Скачать и попробовать PVS-Studio: http://www.viva64.com/ru/pvs-studio/

По вопросам приобретения коммерческой лицензии просим Вас связаться с нами в почте. Вы также можете написать нам, чтобы получить временную лицензию для всестороннего изучения PVS-Studio, если хотите снять ограничения демонстрационной версии.

#CSharp