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

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

![0469_SharpDevelop_ru/image1.png](https://import.viva64.com/docx/blog/0469_SharpDevelop_ru/image1.png)

## Введение

Предыдущая [статья](https://pvs-studio.ru/ru/blog/posts/csharp/0359/) про проверку [SharpDevelop](https://github.com/icsharpcode) была опубликована Андреем Карповым в ноябре 2015 года\. В то время мы только тестировали новый C\# анализатор, готовясь к выпуску первого релиза\. Тем не менее, уже тогда, при помощи бета\-версии, Андрею удалось обнаружить в проекте SharpDevelop несколько любопытных ошибок\. После этого SharpDevelop был "помещен на полку" и использовался вместе с другими проектами только для внутреннего тестирования при разработке новых диагностических правил\. И вот, наконец, наступило время проверить SharpDevelop еще раз, но уже более "мускулистой" версией анализатора [PVS\-Studio 6\.12](https://pvs-studio.ru/ru/pvs-studio/download/)\.

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

![0469_SharpDevelop_ru/image2.png](https://import.viva64.com/docx/blog/0469_SharpDevelop_ru/image2.png)

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

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

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

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

Ошибка, которая по праву может занять почетное место в "[палате мер и весов](https://ru.wikipedia.org/wiki/%D0%9C%D0%B5%D0%B6%D0%B4%D1%83%D0%BD%D0%B0%D1%80%D0%BE%D0%B4%D0%BD%D0%BE%D0%B5_%D0%B1%D1%8E%D1%80%D0%BE_%D0%BC%D0%B5%D1%80_%D0%B8_%D0%B2%D0%B5%D1%81%D0%BE%D0%B2)"\. Яркая иллюстрация полезности использования статического анализа кода и опасности Copy\-Paste\.

**Предупреждение анализатора PVS\-Studio**: [V3102](https://pvs-studio.ru/ru/docs/warnings/v3102/) Suspicious access to element of 'method\.SequencePoints' object by a constant index inside a loop\. CodeCoverageMethodTreeNode\.cs 52

```cpp
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_, а вот выражение доступа по индексу изменить забыли\. Правильный вариант данной конструкции имеет вид:

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v3095/) The 'position' object was used before it was verified against null\. Check lines: 204, 206\. Task\.cs 204

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

Переменную _position_ используют, не проверив на равенство _null_\. Проверка производится в другом условии, в блоке кода _else_\. Правильный вариант кода мог бы иметь вид:

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

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

**Предупреждение анализатора PVS\-Studio**: [V3125](https://pvs-studio.ru/ru/docs/warnings/v3125/) The 'mainAssemblyList' object was used after it was verified against null\. Check lines: 304, 291\. ClassBrowserPad\.cs 304

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

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

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

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

**Предупреждение анализатора PVS\-Studio**: [V3078](https://pvs-studio.ru/ru/docs/warnings/v3078/) Original sorting order will be lost after repetitive call to 'OrderBy' method\. Use 'ThenBy' method to preserve the original sorting\. CodeCoverageMethodElement\.cs 124

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

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

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

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

**Предупреждение анализатора PVS\-Studio**: [V3064](https://pvs-studio.ru/ru/docs/warnings/v3064/) Potential division by zero\. Consider inspecting denominator 'workAmount'\. XamlSymbolSearch\.cs 60

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v3008/) The 'ignoreDialogIdSelectedInTextEditor' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 204, 201\. WixDialogDesigner\.cs 204

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

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

```cpp
bool ignoreDialogIdSelectedInTextEditor;
```

Объявление _IWorkbenchWindow_ и _ActiveViewContent_:

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

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

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

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

**Предупреждение анализатора PVS\-Studio**: [V3053](https://pvs-studio.ru/ru/docs/warnings/v3053/) An excessive expression\. Examine the substrings '/debug' and '/debugport'\. NDebugger\.cs 287

```cpp
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" бессмысленным\. Пожалуй, это не ошибка, но код можно упростить:

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

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

**Предупреждение анализатора 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\. ReferenceFolderNodeCommands\.cs 130

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

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

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

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

**Предупреждение анализатора PVS\-Studio**: [V3128](https://pvs-studio.ru/ru/docs/warnings/v3128/) The 'contentPanel' field is used before it is initialized in constructor\. SearchResultsPad\.cs 66

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

Поле _contentPanel_ передается в качестве одного из параметров в метод _CreateToolBarItems _в конструкторе класса _SearchResultsPad_\. При этом инициализация данного поля производится уже после использования\. Возможно, в данном случае ошибки нет, так как в теле метода _CreateToolBarItems_ и далее по стеку может быть учтена возможность равенства _null_ переменной _contentPanel_\. Но код выглядит подозрительно и требует проверки автором\.

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

## Заключение

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

Напоминаю, что и вы можете в любое время включиться в данный процесс, воспользовавшись возможностью [бесплатного](https://pvs-studio.ru/ru/blog/posts/0457/) использования статического анализатора PVS\-Studio для проверки собственных проектов\.

[Скачать и попробовать PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/)\.

По вопросам приобретения коммерческой лицензии просим Вас [связаться](https://pvs-studio.ru/ru/about-feedback/) с нами в почте\. Вы также можете написать нам, чтобы получить временную лицензию для всестороннего изучения PVS\-Studio, если хотите снять [ограничения](https://pvs-studio.ru/ru/docs/manual/0009/) демонстрационной версии\.