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

30 Ноя 2015

Теги:

#CSharp

Экспериментальная версия PVS-Studio, поддерживающая C#

30 Ноя 2015

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

PVS-Studio
Проверка проекта SharpDevelop
Наиболее подозрительные фрагменты кода
Запахи
Заключение

У нас появилась экспериментальная версия анализатора PVS-Studio, который умеет анализировать C#-проекты и который можно показать миру. Это не Release, и даже не Beta. Это просто текущая сборка PVS-Studio. Мы хотим как можно раньше начать получать отзывы от наших пользователей или потенциальных пользователей касательно поддержки C#. Поэтому предлагаем энтузиастам попробовать новую версию PVS-Studio на своих C#-проектах и рассказать нам о результатах, недостатках и высказать свои пожелания. Ах да, и, конечно, в статье будут описаны результаты проверки очередного проекта - SharpDevelop.

PVS-Studio

Сейчас одним из важных вопросов является: "Зачем делать очередной инструмент анализа кода для C#?".

Попробуем ответить как потенциальным пользователям, так и самим себе, чтобы ясно понимать куда и зачем мы движемся.

Мы успешно создали и продолжаем развивать анализатор PVS-Studio для языка C/C++. В этом анализаторе реализовано много интересных и уникальных идей по выявлению разнообразных типов ошибок. Со временем стало понятно, что многие из реализованных диагностик никак не связаны с конкретным языком программирования. Не важно, какой язык вы используете. Всегда будут опечатки, ошибки из-за невнимательности или неудачного Copy-Paste.

И тогда мы решили попробовать применить свой опыт к другому языку программирования, к C#. Насколько это будет успешным начинанием, покажет время. Сами мы считаем, что постепенно сможем создать очень интересный инструмент, пользу от которого может получить большое количество C#-разработчиков.

Сейчас наша задача - начать как можно раньше получать отзывы от наших потенциальных пользователей. Полноценная версия анализатора PVS-Studio ещё не готова. Сейчас в ней мало диагностик (на момент написания статьи их было 36). Но эту версию можно установить и попробовать. И мы будем благодарны всем, кто это сделает. Нам важно убедиться, что мы вообще движемся в верном направлении и что анализатор в целом работоспособен. А у уж добавлять новые и новые диагностики мы будем очень быстро.

Примечание. Со временем приведенная выше ссылка станет недействительной. Поэтому, если вы читаете эту статью по пришествию месяца или более с момента публикации, то предлагаем вам установить текущую версию дистрибутива: http://www.viva64.com/ru/pvs-studio/download/

Если читатель ещё не разу не пробовал PVS-Studio, то предлагаю ознакомиться со статьёй "PVS-Studio для Visual C++". Как вы видите, она ориентирована на C++, но на самом деле никакой разницы нет. С точки зрения интерфейса вообще почти нет никакой разницы, работаете вы с C++ или с C# проектами.

Чтобы отправить свои отзывы и пожелания, вы можете воспользоваться страницей обратной связи.

Проверка проекта SharpDevelop

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

Не вижу смысла изобретать велосипед. Теперь тем же методом я буду очаровывать C# программистов. И вот перед вами очередная статья про проверку открытого проекта SharpDevelop.

SharpDevelop - свободная среда разработки для C#, Visual Basic .NET, Boo, IronPython, IronRuby, F#, C++. Обычно используется как альтернатива Visual Studio .NET. Существует также форк на Mono/GTK+ — MonoDevelop.

Для нас важно, что проект написан полностью на C#. А значит мы можем его проверить экспериментальной версией PVS-Studio. В проекте 8522 файлов с расширением "cs", суммарный размер которых равен 45 мегабайт.

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

Фрагмент N1

public override string ToString()
{
  return String.Format("Thread Name = {1} Suspended = {2}",
                       ID, Name, Suspended);
}

Предупреждение PVS-Studio: V3025 Incorrect format. A different number of actual arguments is expected while calling 'Format' function. Expected: 2. Present: 3. Thread.cs 235

Переменная ID никак не используется. Возможно никакой настоящей ошибки здесь нет. Однако это место явно стоит проверить. Возможно здесь планировалось сформировать совсем иную строку.

Фрагмент N2

public override string ToString ()
{
  return
    String.Format ("[Line {0}:{1,2}-{3,4}:{5}]",
                   File, Row, Column, EndRow, EndColumn, Offset);
}

Предупреждение PVS-Studio: V3025 Incorrect format. A different number of actual arguments is expected while calling 'Format' function. Expected: 4. Present: 6. MonoSymbolTable.cs 235

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

[Line file.cs:10,20-30,40:7]

Но видимо он пропустил некоторые фигурные скобки. Поэтому получается, что ",2" и ",4" задаёт выравнивание полей, а вовсе не выводят значения переменных EndRow и EndColumn.

Рискну предположить, что правильной будет следующая строка форматирования:

String.Format ("[Line {0}:{1},{2}-{3},{4}:{5}]",
               File, Row, Column, EndRow, EndColumn, Offset);

Фрагмент N3

static MemberCore GetLaterDefinedMember(MemberSpec a, MemberSpec b)
{
  var mc_a = a.MemberDefinition as MemberCore;
  var mc_b = b.MemberDefinition as MemberCore;

  ....

  if (mc_a.Location.File != mc_a.Location.File)
    return mc_b;

  return mc_b.Location.Row > mc_a.Location.Row ? mc_b : mc_a;
}

Предупреждение PVS-Studio: V3001 There are identical sub-expressions 'mc_a.Location.File' to the left and to the right of the '!=' operator. membercache.cs 1306

Здесь мы имеем дело с опечаткой. Я думаю, правильным вариантом будет следующее сравнение:

if (mc_a.Location.File != mc_b.Location.File)

Фрагмент N4

public WhitespaceNode(string whiteSpaceText,
                      TextLocation startLocation)
{
  this.WhiteSpaceText = WhiteSpaceText;
  this.startLocation = startLocation;
}

Предупреждение PVS-Studio: V3005 The 'this.WhiteSpaceText' variable is assigned to itself. WhitespaceNode.cs 65

Красивая ошибка. Здесь статический анализатор показал свою главную суть. Он внимателен и в отличии от человека не устаёт. Поэтому он заметил опечатку. А вы её видите? Согласитесь, найти ошибку не просто.

Итак, опечатка в одной букве. Надо было написать "=whiteSpaceText". А написано "=WhiteSpaceText". В результате значение 'WhiteSpaceText' в классе остаётся неизменным.

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

Фрагмент N5

new public bool Enabled {
  get { return base.Enabled; }
  set {
    if (this.InvokeRequired) {
      base.Enabled = this.VScrollBar.Enabled =
      this.hexView.Enabled =this.textView.Enabled =
      this.side.Enabled = this.header.Enabled = value;
    } else {
      base.Enabled = this.VScrollBar.Enabled =
      this.hexView.Enabled = this.textView.Enabled =
      this.side.Enabled = this.header.Enabled = value;
    }
  }
}

Предупреждение PVS-Studio: V3004 The 'then' statement is equivalent to the 'else' statement. Editor.cs 225

Очень подозрительно, что независимо от значения 'this.InvokeRequired' будут выполнены одни и те-же действия. У меня сильные подозрения, что строка "base.Enabled = ....." была скопирована. А потом в ней что-то забыли изменить.

Фрагмент N6, N7, N8, N9

public override void Run()
{
  ....
  ISolutionFolderNode solutionFolderNode =
    node as ISolutionFolderNode;

  if (node != null)
  {
    ISolutionFolder newSolutionFolder =
      solutionFolderNode.Folder.CreateFolder(....);
    solutionFolderNode.Solution.Save();
  ....
}

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

Хотели выполнить некоторые действия, если 'node' наследуется от интерфейса 'ISolutionFolderNode'. Но проверили не ту переменную. Правильный вариант:

ISolutionFolderNode solutionFolderNode =
  node as ISolutionFolderNode;
if (solutionFolderNode != null)
{

Кстати, это достаточно распространённый паттерн ошибки в C#-программах. Например, в проекте SharpDevelop встретилось ещё 3 таких ошибки:

V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'geometry', 'g'. PathHandlerExtension.cs 578
V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'oldTransform', 'tg'. ModelTools.cs 420
V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'node', 'solutionFolderNode'. SolutionNodeCommands.cs 104

Фрагмент N10

public override void VisitInvocationExpression(....)
{
  ....
  foundInvocations = (idExpression.Identifier == _varName);
  foundInvocations = true;
  ....
}

Предупреждение PVS-Studio: V3008 The 'foundInvocations' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 211, 209. RedundantAssignmentIssue.cs 211

Очень подозрительное повторное присваивание. Возможно, второе присваивание написали в процессе отладки кода, а потом забыли про него.

Ошибка N11

public static Snippet CreateAvalonEditSnippet(....)
{
  ....
  int pos = 0;
  foreach (Match m in pattern.Matches(snippetText)) {
    if (pos < m.Index) {
      snippet.Elements.Add(....);
      pos = m.Index;
    }
    snippet.Elements.Add(....);
    pos = m.Index + m.Length;
  }
  ....
}

Предупреждение PVS-Studio: V3008 The 'pos' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 151, 148. CodeSnippet.cs 151

Ещё одно подозрительное повторное присваивание. Здесь или ошибка, или присваивание "pos = m.Index;" является лишним.

Фрагмент N12

....
public string Text { get; set; }
....
protected override void OnKeyUp(KeyEventArgs e)
{
  ....
  editor.Text.Insert(editor.CaretIndex, Environment.NewLine);
  ....
}

Предупреждение PVS-Studio: V3010 The return value of function 'Insert' is required to be utilized. InPlaceEditor.cs 166

Строки в языке C# являются неизменяемыми. Поэтому, если мы что-то делаем со строкой, результат нужно где-то сохранить. Однако про это легко забыть, как например это случилось здесь. Разработчик решил, что вызывая метод Insert() он что-то добавит к строке. Но это не так. Правильный вариант кода:

editor.Text =
  editor.Text.Insert(editor.CaretIndex, Environment.NewLine);

Фрагмент N13, N14

public IEnumerable<PropertyMapping>
GetMappingForTable(SSDL.EntityType.EntityType table)
{
  var value = GetSpecificMappingForTable(table);
  var baseMapping = BaseMapping;
  if (baseMapping != null)
    value.Union(baseMapping.GetMappingForTable(table));
  return value;
}

Предупреждение PVS-Studio: V3010 The return value of function 'Union' is required to be utilized. MappingBase.cs 274

Вообще, у меня складывается предчувствие, что в C# проектах я буду встречать достаточно много ошибок, связанных с тем, что программист ожидает изменения объекта, а этого не происходит.

Метод-расширение 'Union', определённый для коллекций, реализующих интерфейс IEnumerable, позволяет получить пересечение двух множество. Однако, контейнер 'value' при этом не изменяется. Правильный вариант:

value = value.Union(baseMapping.GetMappingForTable(table));

Ещё одну такую ситуацию можно встретить здесь: V3010 The return value of function 'OrderBy' is required to be utilized. CodeCoverageMethodElement.cs 124

Фрагмент N15

Анализатор PVS-Studio пытается выявить ситуации, где программист мог что-то забыть сделать в switch(). Логика принятия решения, предупреждать или нет, достаточно сложная. Иногда получаются ложные срабатывания, иногда находятся явные ошибки. Рассмотрим одно из таких срабатываний.

Итак, в коде имеет место вот такое перечисление:

public enum TargetArchitecture {
  I386,
  AMD64,
  IA64,
  ARMv7,
}

Местами используются все варианты этого перечисления:

TargetArchitecture ReadArchitecture ()
{
  var machine = ReadUInt16 ();
  switch (machine) {
  case 0x014c:
    return TargetArchitecture.I386;
  case 0x8664:
    return TargetArchitecture.AMD64;
  case 0x0200:
    return TargetArchitecture.IA64;
  case 0x01c4:
    return TargetArchitecture.ARMv7;
  }
  throw new NotSupportedException ();
}

Однако есть и подозрительные места. Например, анализатор обратил моё внимание на следующий код:

ushort GetMachine ()
{
  switch (module.Architecture) {
  case TargetArchitecture.I386:
    return 0x014c;
  case TargetArchitecture.AMD64:
    return 0x8664;
  case TargetArchitecture.IA64:
    return 0x0200;
  }
  throw new NotSupportedException ();
}

Предупреждение PVS-Studio: V3002 The switch statement does not cover all values of the 'TargetArchitecture' enum: ARMv7. ImageWriter.cs 209

Как видите, не рассмотрен случай, если архитектурой является ARMv7. Я не знаю, ошибка это или нет. Но мне кажется, что это именно ошибка. Имя ARMv7 находится в конце перечисления, а значит добавлялось в последнюю очередь. В результате программист мог забыть поправить функцию GetMachine() и учесть эту архитектуру.

Фрагмент N15

void DetermineCurrentKind()
{
  .....
  else if (Brush is LinearGradientBrush) {
    linearGradientBrush = Brush as LinearGradientBrush;
    radialGradientBrush.GradientStops =
      linearGradientBrush.GradientStops;
    CurrentKind = BrushEditorKind.Linear;
  }
  else if (Brush is RadialGradientBrush) {
    radialGradientBrush = Brush as RadialGradientBrush;
    linearGradientBrush.GradientStops =
      linearGradientBrush.GradientStops;
    CurrentKind = BrushEditorKind.Radial;
  }
}

Предупреждение PVS-Studio: V3005 The 'linearGradientBrush.GradientStops' variable is assigned to itself. BrushEditor.cs 120

Достаточно тяжелый фрагмент кода для чтения. И видимо поэтому в нем допущена ошибка. Скорее всего код писался методом Copy-Paste и в одно месте был неправильно изменён.

По все видимости вместо:

linearGradientBrush.GradientStops =
  linearGradientBrush.GradientStops;

Должно было быть написано:

linearGradientBrush.GradientStops =
  radialGradientBrush.GradientStops;

Запахи

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

Выше я рассмотрел много кода, который по всей видимости содержит ошибки. Теперь приведу несколько примеров запахов. Все ситуации я рассматривать не буду, это не интересно. Ограничусь 3 примерами. С остальными запахами разработчики могут ознакомиться самостоятельно, проверив проект SharpDevelop.

Фрагмент кода с запахом N1

protected override bool CanExecuteCommand(ICommand command)
{
  ....
  }
  else if (command == DockableContentCommands.ShowAsDocument)
  {
    if (State == DockableContentState.Document)
    {
      return false;
    }
  }
  ....
  else if (command == DockableContentCommands.ShowAsDocument)
  {
    if (State == DockableContentState.Document)
    {
      return false;
    }
  }
  ....
}

Предупреждение PVS-Studio: V3003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 773, 798. DockableContent.cs 773

Как видите, программа содержит два идентичных блока. Условие нижнего блока 'if' никогда не выполнится. Но на мой взгляд это не ошибка. Мне кажется, что просто случайно продублировали блок, и он является лишним. Тем не менее это место, которое стоит посмотреть и исправить.

Фрагмент кода с запахом N2

void PropertyExpandButton_Click(object sender, RoutedEventArgs e)
{
  ....
  ContentPropertyNode clickedNode =
    clickedButton.DataContext as ContentPropertyNode;
  clickedNode = clickedButton.DataContext as ContentPropertyNode;
  if (clickedNode == null)
  ....
}

Предупреждение PVS-Studio: V3008 The 'clickedNode' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 105, 104. PositionedGraphNodeControl.xaml.cs 105

Код избыточный и его можно упростить до:

ContentPropertyNode clickedNode =
  clickedButton.DataContext as ContentPropertyNode;
if (clickedNode == null)

Фрагмент кода с запахом N3

IEnumerable<ICompletionData>
CreateConstructorCompletionData(IType hintType)
{
  ....
  if (!(hintType.Kind == TypeKind.Interface &&
        hintType.Kind != TypeKind.Array)) {
  ....
}

Предупреждение PVS-Studio: V3023 Consider inspecting this expression. The expression is excessive or contains a misprint. CSharpCompletionEngine.cs 2392

Избыточный код. Выражение можно упростить:

if (hintType.Kind != TypeKind.Interface) {

Я могу продолжать, но, пожалуй, хватит. Все остальные "запахи" достаточно однообразны и похожи на уже перечисленные.

Заключение

Ну что, как видите, сам по себе язык C# не защищает от бестолковых ошибок. Потому я с чистой совестью могу привести здесь эту картинку.

0359_Experimental_version_of_PVS_Studio_with_CSharp_support_ru/image2.png

Да здравствует единорог, который теперь научился находить ошибки и в C# программах!

А если серьезно, то:

При программировании мы все допускаем не только сложные, но и простые ошибки. Суммарно на поиск простых ошибок тратится много времени. А иногда очень много.
Большое количество простых ошибок можно выявить ещё на этапе написания кода, если использовать инструменты статического анализ кода. Использование таких инструментов существенно экономит время, которое может быть потрачено на поиск и отладку многих ошибок.
Самое главное в статическом анализе - регулярное применение. Нет смысла в разовых проверках статического анализа. Вся его суть в том, чтобы найти ошибку сразу после того, как она появилась в коде. Редкие проверки отнимают много времени и приносят мало пользы. Ведь те ошибки, которые можно было найти быстро и легко, уже к этому моменту поправлены потом и кровью.

#CSharp