metrica
Мы используем куки, чтобы пользоваться сайтом было удобно.
Хорошо
to the top
close form

Заполните форму в два простых шага ниже:

Ваши контактные данные:

Шаг 1
Поздравляем! У вас есть промокод!

Тип желаемой лицензии:

Шаг 2
Team license
Enterprise license
** Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности
close form
Запросите информацию о ценах
Новая лицензия
Продление лицензии
--Выберите валюту--
USD
EUR
RUB
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Бесплатная лицензия PVS‑Studio для специалистов Microsoft MVP
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Для получения лицензии для вашего открытого
проекта заполните, пожалуйста, эту форму
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Мне интересно попробовать плагин на:
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
check circle
Ваше сообщение отправлено.

Мы ответим вам на


Если вы так и не получили ответ, пожалуйста, проверьте папку
Spam/Junk и нажмите на письме кнопку "Не спам".
Так Вы не пропустите ответы от нашей команды.

>
>
>
Повторная проверка SharpDevelop: что но…

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

30 Янв 2017

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

0469_SharpDevelop_ru/image1.png

Введение

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

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

0469_SharpDevelop_ru/image2.png

Не будем рассматривать предупреждения на уровне 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, если хотите снять ограничения демонстрационной версии.

Популярные статьи по теме
Распространённые паттерны опечаток при программировании

Дата: 25 Авг 2023

Автор: Андрей Карпов

Есть бесконечное количество способов ошибиться при написании кода. Однако иногда можно заметить явные интересные закономерности, как и где ошибаются программисты. Поговорим о коде, который...
Обзор подозрительных мест в исходном коде MassTransit

Дата: 09 Авг 2023

Автор: Никита Паневин

MassTransit — Open Source платформа распределённых приложений для .NET. В этой статье мы расскажем о проблемных местах в коде проекта. С поиском таких мест нам поможет статический анализатор...
Всегда ли в C# есть упаковка при конкатенации со строкой и интерполяции?

Дата: 03 Авг 2023

Автор: Никита Липилин

Разработчики на C# хорошо знакомы с термином "упаковка". Она может быть явной, а может быть незаметна. Например, к упаковке приводит сложение значимого типа со строкой. Или не приводит. Такая вот...
Ryujinx: повторная проверка эмулятора Nintendo Switch с помощью PVS-Studio

Дата: 21 Июл 2023

Автор: Глеб Асламов

Популярность Nintendo Switch не угасает, эксклюзивные игры получают награды, и желание в них поиграть только растет. Однако возможность опробовать портативную приставку есть не у каждого. Решает эту.…
Изучаем подозрительные места в коде AWS SDK для .NET

Дата: 04 Июл 2023

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

Сегодня под нашим скальпелем оказался проект AWS SDK для .NET. Мы посмотрим на подозрительные места из исходного кода, разберёмся, что в них происходит, и попробуем воспроизвести некоторые проблемы...


Комментарии (0)

Следующие комментарии next comments
close comment form